diff --git a/CLAUDE.md b/CLAUDE.md index 49dfaa6ed..755f2ea74 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -152,7 +152,7 @@ The load-bearing few. **Full detail for every relocated item → [tutorials-ims- - **`@cap-js/ai` for RPT-1 ValueList recommendations (#959)** — `AICore-mocked` locally, `AICore-btp` in hybrid/prod; per-field opt-out `@UI.RecommendationState: 0`. Ref: [cap-ai-plugin.md](docs/developers/reference/cap-ai-plugin.md). - **Knowledge-graph feature flags (`KG_*`), all default OFF + DEV-only, all fail-open** — PageRank #916, WCC isolation #918, on-demand extraction #948, Louvain communities #917, orphan retirement #1115, community peers/labels #1126, community search weight #1171, coverage nudge #1172, cluster Q&A #1173. Toggles, nightly jobs, and fail-open specifics → gotchas.md "Knowledge graph feature flags". - **HCQL protocol adapter (#2247, CAP 10.1.0)** — `POST /hcql/` paths (NOT co-mounted on OData URLs) on 5 authenticated services only (`AdminService`, `AuthorService`, `AnalyticsService`, `ExportsService`, `ConsolidationService`); requires `@sap/cds >= 10.1.0`. Malformed-CQN process-exit DoS from 10.0.3 **fixed in 10.1.0** (now returns 400, server stays up). Auth enforced before CQN parse. Kill: delete `srv/hcql-enablement.cds` + drop the `hcql` entry from `AdminService`'s `@protocol` list in `srv/admin-service-mcp.cds`, then `cds build --production` + redeploy. Ref: [hcql-support.md](docs/developers/reference/hcql-support.md). -- **cds-caching CDS-DB store + metrics ON (#1222)** — `store:"cds"` in hybrid/prod, `memory` in base/unit. CF resolve-guard needs BOTH the baked csn entities AND `srv/lib/strip-precompiled-plugin-roots.js` (both load-bearing). Ref: [cds-caching-store.md](docs/developers/reference/cds-caching-store.md). +- **cds-caching CDS-DB store + metrics ON (#1222)** — `store:"cds"` in hybrid/prod, `memory` in base/unit. CF resolve-guard needs BOTH the baked csn entities AND `srv/lib/strip-precompiled-plugin-roots.js` (both load-bearing). A committed `patch-package` patch (`patches/cds-caching+2.1.0.patch`, `postinstall`) makes `KeyvCDS.set` an atomic UPSERT to kill PROD PK-collision error noise on concurrent `kg:rl:*` writes (#3102); **re-verify the patch on any `cds-caching` bump** (patch-package fails install loudly on drift). Ref: [cds-caching-store.md](docs/developers/reference/cds-caching-store.md). - **User-facing UI changes want a committed e2e spec** — advisory PR nudge on `app/**`/`hugo/**` changes; real coverage runs in the post-DEV-deploy `e2e` job. Ref: [e2e-coverage-pattern.md](docs/developers/reference/e2e-coverage-pattern.md). - **`test:e2e` is post-deploy only, not on PRs** — self-skips without `SMOKE_BASE_URL`. Served tutorials render `
`+`

`, NOT `
`. Runbook: `test/e2e/README.md`. - **Freshness detector grounding needs the corpus-embedding backfill** — until `srv/jobs/freshness-corpus-embedding-job.js` runs, every API-obsolescence claim degrades to `confidence: Low`. Tutorial source from `ContentFiles.sourceContent` via `getTutorialSource(slug)`, NOT `Steps.description`. → gotchas.md "Freshness detector". diff --git a/docs/developers/reference/cds-caching-store.md b/docs/developers/reference/cds-caching-store.md index b325a8634..4aaaeea95 100644 --- a/docs/developers/reference/cds-caching-store.md +++ b/docs/developers/reference/cds-caching-store.md @@ -356,3 +356,62 @@ command-palette concept search): `@cache: { ttl: 300000, tags: [{ value: re-enabled metrics actually accumulate against the deployed DEV container (clear the stale zeroed rows first), then let PROD traffic post-cutover give the baseline before annotating more surfaces. + +## `KeyvCDS.set` UPSERT patch (#3102) + +**Symptom (PROD, 2026-11-11):** recurring `level:"error"` lines from the +`cds-caching` logger: + +``` +KeyvCDS set error for key kg:rl:api:auth:auth:: unique constraint violated: +Table(PLUGIN_CDS_CACHING_CACHESTORE), Index(_SYS_TREE_CS_#..._#P0) ... key: ID=kg:rl:... +``` + +Two namespaces seen — both the KG rate-limiter (`srv/lib/rate-limit/shared-limiter.js`, +key prefix `kg:rl:`): `kg:rl:api:auth:auth:` and `kg:rl:homepage:anon:`. + +**Root cause:** the plugin's CDS-DB store adapter, +`node_modules/cds-caching/lib/support/KeyvCDS.js`, implemented `set()` as a +**read-then-create/update** (TOCTOU): + +```js +const existing = await db.read(entity, key); +if (existing) { await db.update(entity, key).with({ value, expiresAt }); } +else { await db.create(entity, { ID: key, value, expiresAt }); } +``` + +Under multi-instance (HA) CF, two instances both read "no row" for the same +`kg:rl:*` counter, both take the `else` branch, and the second `create` violates +the `CacheStore` PK unique index. The adapter's own `catch` logs at **error** +level and swallows the exception — so the collision never reaches our +`shared-limiter.js` fail-open (the limiter already over-allows under races, which +is acceptable), but it spams PROD error monitoring and the losing write is +dropped. + +**Fix:** a committed **patch-package** patch, +`patches/cds-caching+2.1.0.patch`, replaces that branch with a single atomic +UPSERT: + +```js +await db.run(cds.ql.UPSERT.into(entity).entries({ ID: key, value, expiresAt })); +``` + +`CacheStore` has a single-column PK (`ID : String(900)`), so UPSERT compiles to +HANA `UPSERT ... WITH PRIMARY KEY` and to SQLite `INSERT OR REPLACE` — +last-write-wins, no read-then-write race, no unique-constraint collision. The +patch re-applies on every `npm ci` / `npm install` via the `postinstall` hook +(`patch-package`). Regression test: +`srv/lib/__tests__/keyv-cds-upsert.test.js` (drives the real `KeyvCDS` against an +in-memory CAP fixture, modelling the TOCTOU window deterministically). + +**Upstream:** the same read-then-insert is still present on `cds-caching` `main`. +Filed upstream as +[mikezaschka/cds-caching#44](https://github.com/mikezaschka/cds-caching/issues/44) +(issue) + +[mikezaschka/cds-caching#45](https://github.com/mikezaschka/cds-caching/pull/45) +(PR). **Drop condition:** once a published `cds-caching` release carries the +upstream fix and we adopt it, delete `patches/cds-caching+2.1.0.patch`. +**Re-verify the patch on any `cds-caching` version bump** — `patch-package` +fails the install loudly if the target file drifted, so the drift surfaces at +`npm ci`, not silently. + diff --git a/docs/superpowers/plans/2026-11-11-cds-caching-upsert-fix.md b/docs/superpowers/plans/2026-11-11-cds-caching-upsert-fix.md new file mode 100644 index 000000000..6e8317b54 --- /dev/null +++ b/docs/superpowers/plans/2026-11-11-cds-caching-upsert-fix.md @@ -0,0 +1,231 @@ +# cds-caching UPSERT Fix (#3102) Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Stop `cds-caching` from logging `error`-level unique-constraint-violation noise in PROD when two HA instances concurrently write the same cache key (notably `kg:rl:*` rate-limiter counters), by making the `KeyvCDS.set()` write path an atomic single-key UPSERT. + +**Architecture:** The defect lives in the vendored `cds-caching` plugin (`node_modules/cds-caching/lib/support/KeyvCDS.js`), whose `set()` does read-then-`create`/`update` — a TOCTOU race that collides on the `CacheStore` PK under multi-instance CF. We fix it two ways: (1) **locally and immediately** via a committed `patch-package` patch against our pinned `cds-caching@2.1.0`, replacing the racy branch with `UPSERT.into(entity).entries(...)` (last-write-wins, no collision); (2) **upstream** by filing an issue + PR against `mikezaschka/cds-caching` so the patch can eventually be dropped. The bug is confirmed still present on upstream `main`. + +**Tech Stack:** Node.js 22, `@sap/cds` (CQL `UPSERT`), `cds-caching@2.1.0`, `patch-package`, HANA (prod/hybrid) / SQLite (unit). + +**Spec:** GitHub issue [sap-tutorials/tutorials-ims#3102](https://github.com/sap-tutorials/tutorials-ims/issues/3102) + +## Global Constraints + +- `cds-caching` is pinned at exactly `"2.1.0"` in `package.json` dependencies — the patch filename and `patch-package` match this version verbatim; do **not** bump it as part of this fix. +- Node engine is pinned `22.23.4`; `patch-package` must run under it. +- No raw SQL — use CQL `UPSERT` (`@sap/cds`), consistent with the CAP constraint in CLAUDE.md. +- The `CacheStore` entity is single-PK: `key ID : String(900); value : LargeString; expiresAt : Int64;`. UPSERT semantics are only safe here because the key is a single PK column (composite-key UPSERT on HANA via CDS QL is a known gotcha — not applicable). +- The fix must remain **fail-open**: `set()` must still never throw to its caller (keep the existing `try/catch` returning normally). +- `patch-package` must re-apply automatically on `npm ci` / `npm install` (CI uses `npm ci`), via a `postinstall` hook, without breaking the existing `prepare` script. +- The MCP MTA (`tutorials-mcp`) and QA app reuse the same `srv/lib` tree but **not** `node_modules` patching concerns — no `srv-qa` cp-list change is needed (no new `srv/lib/` file). + +## Review Focus + +- **npm script collision:** the repo has `prepare` (git hooks) but no `postinstall`. Adding `postinstall: patch-package` must not clobber `prepare`; both must run on install. — covered in Task 2 (verify `npm ci` output shows both hooks firing). +- **Patch fails to apply after a future `cds-caching` bump:** `patch-package` errors (non-zero) if the target file drifted. Task 2 pins the exact `2.1.0` and documents the drop-on-upgrade path; the drift surfaces loudly at install, not silently. +- **UPSERT on SQLite (unit profile):** unit tests use in-memory SQLite with `store:"memory"` (not `cds`), so KeyvCDS is never exercised there; the patch's UPSERT path is only hit under `store:"cds"`. Task 1 adds a direct KeyvCDS test against an in-memory SQLite db to prove UPSERT works on *both* dialects and that a double-set of the same key does not throw. +- **`value`/`expiresAt` null handling:** a `set` with no TTL stores `expiresAt: null`; UPSERT must preserve that (not drop the column). Task 1 asserts a re-set with a shorter/absent TTL overwrites cleanly. +- **Concurrent set does not lose the row entirely:** after two racing sets, the key must still be readable (one winner), never left absent. Task 1 asserts the key exists post-double-set. + +--- + +### Task 1: patch-package patch — atomic UPSERT in `KeyvCDS.set()` (+ regression test) + +**Files:** +- Create: `patches/cds-caching+2.1.0.patch` (generated by `patch-package`, not hand-written) +- Modify (as patch source): `node_modules/cds-caching/lib/support/KeyvCDS.js` — the `set(key, value, ttl)` method body +- Test: `srv/lib/__tests__/keyv-cds-upsert.test.js` + +**Interfaces:** +- Consumes: nothing from earlier tasks. +- Produces: a committed `patches/cds-caching+2.1.0.patch` that Task 2 wires into install; the behavioral guarantee that `KeyvCDS.set()` is collision-free and still never throws. + +- [ ] **Step 1: Write the failing test** + +Create `srv/lib/__tests__/keyv-cds-upsert.test.js`. It instantiates the real `KeyvCDS` class from `cds-caching`, backed by an in-memory SQLite `cds.db` with the `plugin.cds_caching.CacheStore` entity deployed, and asserts the collision-safe contract: + +```js +import { describe, it, expect, beforeAll } from 'vitest'; +import cds from '@sap/cds'; +import KeyvCDS from 'cds-caching/lib/support/KeyvCDS.js'; + +describe('KeyvCDS.set is collision-safe (issue #3102)', () => { + beforeAll(async () => { + // deploy only the cache-store entity into an in-memory sqlite db + cds.model = await cds.load([require.resolve('cds-caching/db/cache-store.cds')]); + await cds.deploy(cds.model).to('sqlite::memory:'); + }); + + it('a second set of the same key does NOT throw and overwrites (last-write-wins)', async () => { + const store = new KeyvCDS({}); + await store.set('dup', { count: 1 }, 10_000); + await expect(store.set('dup', { count: 2 }, 10_000)).resolves.not.toThrow(); + expect(await store.get('dup')).toEqual({ count: 2 }); + }); + + it('two concurrent sets of a fresh key both resolve and the key is present', async () => { + const store = new KeyvCDS({}); + await Promise.all([store.set('race', { a: 1 }, 10_000), store.set('race', { a: 2 }, 10_000)]); + expect(await store.get('race')).toBeDefined(); + }); + + it('re-set with no TTL overwrites cleanly (expiresAt null)', async () => { + const store = new KeyvCDS({}); + await store.set('nt', 'first', 10_000); + await store.set('nt', 'second'); // no ttl + expect(await store.get('nt')).toBe('second'); + }); +}); +``` + +The decisive assertion is that a simulated collision does not surface. (On SQLite a true PK re-INSERT would throw `UNIQUE constraint failed`; the pre-patch read-then-create code path can hit it under interleaving. Post-patch UPSERT never does.) + +- [ ] **Step 2: Run the test against the UNPATCHED plugin to see it is not yet collision-proof** + +Run: `npm test -- srv/lib/__tests__/keyv-cds-upsert.test.js` +Expected: the concurrent-set / double-set assertions fail or emit the `KeyvCDS set error` log (pre-patch read-then-create path). If the unpatched code happens to pass serially, note it — the patch's value is proven by the HANA PK semantics; keep the test as the post-patch regression guard. + +- [ ] **Step 3: Edit `node_modules/cds-caching/lib/support/KeyvCDS.js`** + +Replace the body of `async set(key, value, ttl)` so the read-then-`create`/`update` branch becomes a single atomic UPSERT, keeping the outer `try/catch` (fail-open, no re-throw) intact: + +```js +async set(key, value, ttl) { + try { + const { db, entity } = await this._getDB(); + if (!db || !entity) return; + const expiresAt = ttl ? Date.now() + ttl : null; + // Atomic last-write-wins. CacheStore has a single-column PK (ID), + // so UPSERT compiles to HANA `UPSERT ... WITH PRIMARY KEY` and to + // SQLite `INSERT OR REPLACE` — no read-then-write race, no unique + // constraint collision under multi-instance concurrency (#3102). + await db.run(UPSERT.into(entity).entries({ ID: key, value, expiresAt })); + } catch (error) { + this.log.error(`KeyvCDS set error for key ${key}:`, error); + } +} +``` + +`UPSERT` is a CQL global provided by `@sap/cds`; `cds` is already required at the top of the file (`const cds = require('@sap/cds')`), and `UPSERT` is exposed as a global by the cds runtime. If `UPSERT` is not in scope in this module, reference it as `cds.ql.UPSERT`. Confirm which form resolves before generating the patch. + +- [ ] **Step 4: Run the test to verify it passes against the patched plugin** + +Run: `npm test -- srv/lib/__tests__/keyv-cds-upsert.test.js` +Expected: PASS, no `KeyvCDS set error` log emitted. + +- [ ] **Step 5: Generate the patch file** + +Run: `npx patch-package cds-caching` +Expected: creates `patches/cds-caching+2.1.0.patch` containing only the `set()` diff. Inspect it to confirm it touches exactly that method and nothing else. + +- [ ] **Step 6: Commit** + +```bash +git add patches/cds-caching+2.1.0.patch srv/lib/__tests__/keyv-cds-upsert.test.js +git commit -m "fix(cds-caching): atomic UPSERT in KeyvCDS.set to stop PK-collision error noise (#3102)" +``` + +--- + +### Task 2: Wire `patch-package` into install + +**Files:** +- Modify: `package.json` — add `patch-package` to `devDependencies`, add/extend a `postinstall` script. + +**Interfaces:** +- Consumes: `patches/cds-caching+2.1.0.patch` from Task 1. +- Produces: automatic patch re-application on `npm ci` / `npm install`. + +- [ ] **Step 1: Add the dev dependency** + +Run: `npm install --save-dev patch-package` +Expected: `patch-package` appears in `devDependencies`. (It also pulls its own deps; confirm the install succeeds under Node 22.) + +- [ ] **Step 2: Add the `postinstall` hook** + +In `package.json` `scripts`, add: +```json +"postinstall": "patch-package" +``` +Leave the existing `prepare` script untouched. (npm runs `postinstall` then `prepare` on a bare `npm install`; both fire.) + +- [ ] **Step 3: Verify the patch applies from a clean install** + +Run: `npm ci` +Expected: install output includes a `patch-package` line applying `cds-caching@2.1.0` cleanly; `node_modules/cds-caching/lib/support/KeyvCDS.js` contains the UPSERT edit afterward. Confirm with: `grep -c "UPSERT.into" node_modules/cds-caching/lib/support/KeyvCDS.js` → `1`. + +- [ ] **Step 4: Re-run the regression test post-`npm ci`** + +Run: `npm test -- srv/lib/__tests__/keyv-cds-upsert.test.js` +Expected: PASS (proves the patch survives a real install cycle). + +- [ ] **Step 5: Commit** + +```bash +git add package.json package-lock.json +git commit -m "build: wire patch-package postinstall for cds-caching UPSERT patch (#3102)" +``` + +--- + +### Task 3: Document the patch + upgrade path + +**Files:** +- Modify: `docs/developers/reference/cds-caching-store.md` — add a short "KeyvCDS.set UPSERT patch (#3102)" section. +- Modify: `CLAUDE.md` — one line in the cds-caching gotcha bullet noting the committed patch and that it must be re-verified on any `cds-caching` bump. + +**Interfaces:** +- Consumes: the patch + wiring from Tasks 1–2. +- Produces: operator-facing record of why the patch exists and how to drop it. + +- [ ] **Step 1: Add the reference-doc section** + +Append to `docs/developers/reference/cds-caching-store.md` a section explaining: the PROD symptom (`error`-level `unique constraint violated` on `PLUGIN_CDS_CACHING_CACHESTORE` from concurrent `kg:rl:*` writes), the root cause (read-then-insert TOCTOU in `KeyvCDS.set`), the fix (committed `patches/cds-caching+2.1.0.patch` → atomic UPSERT), the upstream issue/PR links (Task 4), and the drop condition (patch becomes unnecessary once a `cds-caching` release carrying the upstream fix is adopted — re-verify/remove the patch on any version bump). + +- [ ] **Step 2: Add the CLAUDE.md note** + +Extend the existing `cds-caching CDS-DB store` gotcha bullet with: a committed `patch-package` patch (`patches/cds-caching+2.1.0.patch`) makes `KeyvCDS.set` an atomic UPSERT to kill PROD PK-collision error noise (#3102); re-verify the patch on any `cds-caching` upgrade. + +- [ ] **Step 3: Commit** + +```bash +git add docs/developers/reference/cds-caching-store.md CLAUDE.md +git commit -m "docs(cds-caching): record KeyvCDS.set UPSERT patch + upgrade path (#3102)" +``` + +--- + +### Task 4: Upstream the fix (issue + PR to mikezaschka/cds-caching) + +**Files:** none in this repo (external repo). This task produces links recorded in Task 3's docs — run Task 4 before Task 3 Step 1 if you want the links inline, or update the doc afterward. + +**Interfaces:** +- Consumes: the diagnosis + UPSERT diff from Task 1 (the upstream patch is the same edit applied to `main`, where `set()` is byte-identical). +- Produces: an upstream issue URL and PR URL. + +- [ ] **Step 1: File the upstream issue** + +Against `mikezaschka/cds-caching`: title ≈ "KeyvCDS.set read-then-insert races on PK under multi-instance → unique-constraint errors". Body: the HA race, the `PLUGIN_CDS_CACHING_CACHESTORE` collision, that `set()` on `main` still does read-then-`create`/`update`, and the proposed atomic-UPSERT fix. Confirm no existing open issue covers it first. + +- [ ] **Step 2: Open the upstream PR** + +Fork `mikezaschka/cds-caching`, branch, apply the same `set()` UPSERT edit to `lib/support/KeyvCDS.js` on `main`, and open a PR referencing the issue. Match upstream code style (4-space indent, the existing comment tone). Include a one-line note that single-PK `CacheStore` makes UPSERT safe on HANA + SQLite + Postgres. + +- [ ] **Step 3: Record the links** + +Paste the issue + PR URLs into the Task 3 reference-doc section and the local #3102 PR description. + +--- + +### Task 5: Open the tutorials-ims PR + +**Files:** none (git/PR operation). + +- [ ] **Step 1: Push the branch and open a DEV-targeted PR** + +Per repo convention, PRs target `DEV` (not `main`). Push `worktree-fix-3102-cds-caching-upsert` and `gh pr create --base DEV`. PR body: link #3102, summarize root cause + the two-pronged fix (local patch + upstream issue/PR), link the upstream URLs, and note the patch's drop condition. + +- [ ] **Step 2: Verify the full unit suite is green in CI** + +The full `npm test` suite is hook-blocked locally; confirm the CI unit job passes on the PR before requesting review. diff --git a/package-lock.json b/package-lock.json index b40da7d93..c895dc736 100644 --- a/package-lock.json +++ b/package-lock.json @@ -79,6 +79,7 @@ "esbuild": "0.28.2", "fundamental-styles": "^0.41.7", "happy-dom": "20.14.3", + "patch-package": "8.0.1", "playwright-core": "^1.61.1", "postcss": "^8.5.16", "postcss-cli": "^11.0.1", @@ -11871,6 +11872,13 @@ "node": ">=10.0.0" } }, + "node_modules/@yarnpkg/lockfile": { + "version": "1.1.0", + "resolved": "https://registry.npmjs.org/@yarnpkg/lockfile/-/lockfile-1.1.0.tgz", + "integrity": "sha512-GpSwvyXOcOOlV70vbnzjj4fW5xW/FdUF6nQEt1ENy7m4ZCczi1+/buVUPAqmGfqznsORNFzUMjctTIp8a9tuCQ==", + "dev": true, + "license": "BSD-2-Clause" + }, "node_modules/@zxing/library": { "version": "0.21.3", "resolved": "https://registry.npmjs.org/@zxing/library/-/library-0.21.3.tgz", @@ -12729,6 +12737,25 @@ "node": ">= 0.8" } }, + "node_modules/call-bind": { + "version": "1.0.9", + "resolved": "https://registry.npmjs.org/call-bind/-/call-bind-1.0.9.tgz", + "integrity": "sha512-a/hy+pNsFUTR+Iz8TCJvXudKVLAnz/DyeSUo10I5yvFDQJBFU2s9uqQpoSrJlroHUKoKqzg+epxyP9lqFdzfBQ==", + "dev": true, + "license": "MIT", + "dependencies": { + "call-bind-apply-helpers": "^1.0.2", + "es-define-property": "^1.0.1", + "get-intrinsic": "^1.3.0", + "set-function-length": "^1.2.2" + }, + "engines": { + "node": ">= 0.4" + }, + "funding": { + "url": "https://github.com/sponsors/ljharb" + } + }, "node_modules/call-bind-apply-helpers": { "version": "1.0.2", "license": "MIT", @@ -13054,6 +13081,22 @@ "url": "https://github.com/sponsors/colinhacks" } }, + "node_modules/ci-info": { + "version": "3.9.0", + "resolved": "https://registry.npmjs.org/ci-info/-/ci-info-3.9.0.tgz", + "integrity": "sha512-NIxF55hv4nSqQswkAeiOi1r83xy8JldOFDTWiug55KBu9Jnblncd2U6ViHmYgHf01TPZS77NJBhBMKdWj9HQMQ==", + "dev": true, + "funding": [ + { + "type": "github", + "url": "https://github.com/sponsors/sibiraj-s" + } + ], + "license": "MIT", + "engines": { + "node": ">=8" + } + }, "node_modules/cjs-module-lexer": { "version": "2.3.0", "resolved": "https://registry.npmjs.org/cjs-module-lexer/-/cjs-module-lexer-2.3.0.tgz", @@ -14269,6 +14312,24 @@ "node": ">=0.10.0" } }, + "node_modules/define-data-property": { + "version": "1.1.4", + "resolved": "https://registry.npmjs.org/define-data-property/-/define-data-property-1.1.4.tgz", + "integrity": "sha512-rBMvIzlpA8v6E+SJZoo++HAYqsLrkg7MSfIinMPFhmkorw7X+dOXVJQs+QT69zGkzMyfDnIMN2Wid1+NbL3T+A==", + "dev": true, + "license": "MIT", + "dependencies": { + "es-define-property": "^1.0.0", + "es-errors": "^1.3.0", + "gopd": "^1.0.1" + }, + "engines": { + "node": ">= 0.4" + }, + "funding": { + "url": "https://github.com/sponsors/ljharb" + } + }, "node_modules/define-lazy-prop": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/define-lazy-prop/-/define-lazy-prop-2.0.0.tgz", @@ -15524,6 +15585,16 @@ "node": ">=8" } }, + "node_modules/find-yarn-workspace-root": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/find-yarn-workspace-root/-/find-yarn-workspace-root-2.0.0.tgz", + "integrity": "sha512-1IMnbjt4KzsQfnhnzNd8wUEgXZ44IzZaZmnLYx7D5FZlaHt2gW20Cri8Q+E/t5tIj4+epTBub+2Zxu/vNILzqQ==", + "dev": true, + "license": "Apache-2.0", + "dependencies": { + "micromatch": "^4.0.2" + } + }, "node_modules/findit2": { "version": "2.2.3", "resolved": "https://registry.npmjs.org/findit2/-/findit2-2.2.3.tgz", @@ -16004,6 +16075,19 @@ "node": ">=8" } }, + "node_modules/has-property-descriptors": { + "version": "1.0.2", + "resolved": "https://registry.npmjs.org/has-property-descriptors/-/has-property-descriptors-1.0.2.tgz", + "integrity": "sha512-55JNKuIW+vq4Ke1BjOTjM2YctQIvCT7GFzHwmfZPGo5wnrgkid0YQtnAleFSqumZm4az3n2BS+erby5ipJdgrg==", + "dev": true, + "license": "MIT", + "dependencies": { + "es-define-property": "^1.0.0" + }, + "funding": { + "url": "https://github.com/sponsors/ljharb" + } + }, "node_modules/has-symbols": { "version": "1.1.0", "license": "MIT", @@ -16977,6 +17061,33 @@ "integrity": "sha512-fQhoXdcvc3V28x7C7BMs4P5+kNlgUURe2jmUT1T//oBRMDrqy1QPelJimwZGo7Hg9VPV3EQV5Bnq4hbFy2vetA==", "license": "BSD-2-Clause" }, + "node_modules/json-stable-stringify": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/json-stable-stringify/-/json-stable-stringify-1.3.0.tgz", + "integrity": "sha512-qtYiSSFlwot9XHtF9bD9c7rwKjr+RecWT//ZnPvSmEjpV5mmPOCN4j8UjY5hbjNkOwZ/jQv3J6R1/pL7RwgMsg==", + "dev": true, + "license": "MIT", + "dependencies": { + "call-bind": "^1.0.8", + "call-bound": "^1.0.4", + "isarray": "^2.0.5", + "jsonify": "^0.0.1", + "object-keys": "^1.1.1" + }, + "engines": { + "node": ">= 0.4" + }, + "funding": { + "url": "https://github.com/sponsors/ljharb" + } + }, + "node_modules/json-stable-stringify/node_modules/isarray": { + "version": "2.0.5", + "resolved": "https://registry.npmjs.org/isarray/-/isarray-2.0.5.tgz", + "integrity": "sha512-xHjhDr3cNBK0BzdUJSPXZntQUx/mwMS5Rw4A7lPJ90XGAO6ISP/ePDNuo0vhqOZU+UD5JoodwCAAoZQd3FeAKw==", + "dev": true, + "license": "MIT" + }, "node_modules/jsonc-parser": { "version": "3.3.1", "resolved": "https://registry.npmjs.org/jsonc-parser/-/jsonc-parser-3.3.1.tgz", @@ -16995,6 +17106,16 @@ "graceful-fs": "^4.1.6" } }, + "node_modules/jsonify": { + "version": "0.0.1", + "resolved": "https://registry.npmjs.org/jsonify/-/jsonify-0.0.1.tgz", + "integrity": "sha512-2/Ki0GcmuqSrgFyelQq9M05y7PS0mEwuIzrf3f1fPqkVDVRvZrPZtVSMHxdgo8Aq0sxAOb/cr2aqqA3LeWHVPg==", + "dev": true, + "license": "Public Domain", + "funding": { + "url": "https://github.com/sponsors/ljharb" + } + }, "node_modules/jsonwebtoken": { "version": "9.0.3", "resolved": "https://registry.npmjs.org/jsonwebtoken/-/jsonwebtoken-9.0.3.tgz", @@ -17103,6 +17224,16 @@ "node": ">=0.10.0" } }, + "node_modules/klaw-sync": { + "version": "6.0.0", + "resolved": "https://registry.npmjs.org/klaw-sync/-/klaw-sync-6.0.0.tgz", + "integrity": "sha512-nIeuVSzdCCs6TDPTqI8w1Yre34sSq7AkZ4B3sfOBbI2CgVSB4Du4aLQijFU2+lhAFCwt9+42Hel6lQNIv6AntQ==", + "dev": true, + "license": "MIT", + "dependencies": { + "graceful-fs": "^4.1.11" + } + }, "node_modules/kuler": { "version": "2.0.0", "license": "MIT" @@ -18625,6 +18756,16 @@ "url": "https://github.com/sponsors/ljharb" } }, + "node_modules/object-keys": { + "version": "1.1.1", + "resolved": "https://registry.npmjs.org/object-keys/-/object-keys-1.1.1.tgz", + "integrity": "sha512-NuAESUOUMrlIXOfHKzD6bpPu3tYt3xvjNdRIQ+FeT0lNb4K8WR70CaDxhuNguS2XG+GjkyMwOzsN5ZktImfhLA==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">= 0.4" + } + }, "node_modules/obug": { "version": "2.1.1", "dev": true, @@ -18902,6 +19043,71 @@ "node": ">= 0.4.0" } }, + "node_modules/patch-package": { + "version": "8.0.1", + "resolved": "https://registry.npmjs.org/patch-package/-/patch-package-8.0.1.tgz", + "integrity": "sha512-VsKRIA8f5uqHQ7NGhwIna6Bx6D9s/1iXlA1hthBVBEbkq+t4kXD0HHt+rJhf/Z+Ci0F/HCB2hvn0qLdLG+Qxlw==", + "dev": true, + "license": "MIT", + "dependencies": { + "@yarnpkg/lockfile": "^1.1.0", + "chalk": "^4.1.2", + "ci-info": "^3.7.0", + "cross-spawn": "^7.0.3", + "find-yarn-workspace-root": "^2.0.0", + "fs-extra": "^10.0.0", + "json-stable-stringify": "^1.0.2", + "klaw-sync": "^6.0.0", + "minimist": "^1.2.6", + "open": "^7.4.2", + "semver": "^7.5.3", + "slash": "^2.0.0", + "tmp": "^0.2.4", + "yaml": "^2.2.2" + }, + "bin": { + "patch-package": "index.js" + }, + "engines": { + "node": ">=14", + "npm": ">5" + } + }, + "node_modules/patch-package/node_modules/fs-extra": { + "version": "10.1.0", + "resolved": "https://registry.npmjs.org/fs-extra/-/fs-extra-10.1.0.tgz", + "integrity": "sha512-oRXApq54ETRj4eMiFzGnHWGy+zo5raudjuxN0b8H7s/RU2oW0Wvsx9O0ACRN/kRq9E8Vu/ReskGB5o3ji+FzHQ==", + "dev": true, + "license": "MIT", + "dependencies": { + "graceful-fs": "^4.2.0", + "jsonfile": "^6.0.1", + "universalify": "^2.0.0" + }, + "engines": { + "node": ">=12" + } + }, + "node_modules/patch-package/node_modules/slash": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/slash/-/slash-2.0.0.tgz", + "integrity": "sha512-ZYKh3Wh2z1PpEXWr0MpSBZ0V6mZHAQfYevttO11c51CaWjGTaadiKZ+wVt1PbMlDV5qhMFslpZCemhwOK7C89A==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=6" + } + }, + "node_modules/patch-package/node_modules/tmp": { + "version": "0.2.7", + "resolved": "https://registry.npmjs.org/tmp/-/tmp-0.2.7.tgz", + "integrity": "sha512-e0votIpp4Uo2AJYSzVHV6xCcawuiez3DzqDAbrTc3YxBkplN6e+dM13ZeIcZnDg/QpSuU2zfZ3rzwY8ukEnaXw==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=14.14" + } + }, "node_modules/path-data-parser": { "version": "0.1.0", "resolved": "https://registry.npmjs.org/path-data-parser/-/path-data-parser-0.1.0.tgz", @@ -20341,6 +20547,24 @@ "version": "2.0.0", "license": "ISC" }, + "node_modules/set-function-length": { + "version": "1.2.2", + "resolved": "https://registry.npmjs.org/set-function-length/-/set-function-length-1.2.2.tgz", + "integrity": "sha512-pgRc4hJ4/sNjWCSS9AmnS40x3bNMDTknHgL5UaMBTMyJnU90EgWh1Rz+MC9eFu4BuN/UwZjKQuY/1v3rM7HMfg==", + "dev": true, + "license": "MIT", + "dependencies": { + "define-data-property": "^1.1.4", + "es-errors": "^1.3.0", + "function-bind": "^1.1.2", + "get-intrinsic": "^1.2.4", + "gopd": "^1.0.1", + "has-property-descriptors": "^1.0.2" + }, + "engines": { + "node": ">= 0.4" + } + }, "node_modules/setimmediate": { "version": "1.0.5", "resolved": "https://registry.npmjs.org/setimmediate/-/setimmediate-1.0.5.tgz", diff --git a/package.json b/package.json index 806fc2f62..565adfc55 100644 --- a/package.json +++ b/package.json @@ -89,6 +89,7 @@ "build:island-manifest": "node scripts/build-island-manifest.cjs", "check:ui5-single-copy": "node scripts/check-ui5-single-copy.cjs", "retain:assets": "node scripts/retain-asset-bundles.cjs --js-dir hugo/public/js --css-dir hugo/public/css --manifest-out hugo/public/_retained-assets.json", + "postinstall": "patch-package", "prepare": "npm run setup:git-hooks", "setup:git-hooks": "sh -c 'if [ -f scripts/install-git-hooks.sh ]; then sh scripts/install-git-hooks.sh; fi'", "static-guards": "tsx scripts/run-static-guards.ts", @@ -168,6 +169,7 @@ "esbuild": "0.28.2", "fundamental-styles": "^0.41.7", "happy-dom": "20.14.3", + "patch-package": "8.0.1", "playwright-core": "^1.61.1", "postcss": "^8.5.16", "postcss-cli": "^11.0.1", diff --git a/patches/cds-caching+2.1.0.patch b/patches/cds-caching+2.1.0.patch new file mode 100644 index 000000000..f92c8f43e --- /dev/null +++ b/patches/cds-caching+2.1.0.patch @@ -0,0 +1,22 @@ +diff --git a/node_modules/cds-caching/lib/support/KeyvCDS.js b/node_modules/cds-caching/lib/support/KeyvCDS.js +index 0000000..0000001 100644 +--- a/node_modules/cds-caching/lib/support/KeyvCDS.js ++++ b/node_modules/cds-caching/lib/support/KeyvCDS.js +@@ -63,12 +63,11 @@ + const { db, entity } = await this._getDB(); + if (!db || !entity) return; + const expiresAt = ttl ? Date.now() + ttl : null; +- const existing = await db.read(entity, key); +- if (existing) { +- await db.update(entity, key).with({ value, expiresAt }); +- } else { +- await db.create(entity, { ID: key, value, expiresAt }); +- } ++ // Atomic last-write-wins. CacheStore has a single-column PK (ID), ++ // so UPSERT compiles to HANA `UPSERT ... WITH PRIMARY KEY` and to ++ // SQLite `INSERT OR REPLACE` — no read-then-write race, no unique ++ // constraint collision under multi-instance concurrency (#3102). ++ await db.run(cds.ql.UPSERT.into(entity).entries({ ID: key, value, expiresAt })); + } catch (error) { + this.log.error(`KeyvCDS set error for key ${key}:`, error); + } diff --git a/srv/lib/__tests__/__fixtures__/keyv-cds-app/db/cache-store.cds b/srv/lib/__tests__/__fixtures__/keyv-cds-app/db/cache-store.cds new file mode 100644 index 000000000..dea39ead1 --- /dev/null +++ b/srv/lib/__tests__/__fixtures__/keyv-cds-app/db/cache-store.cds @@ -0,0 +1,10 @@ +namespace plugin.cds_caching; + +// Mirror of cds-caching/db/cache-store.cds — minimal fixture so the real +// KeyvCDS adapter can resolve `plugin.cds_caching.CacheStore` and get a +// backing table under cds.test(). Keep the shape in sync with the plugin. +entity CacheStore { + key ID : String(900); + value : LargeString; + expiresAt : Int64; +} diff --git a/srv/lib/__tests__/__fixtures__/keyv-cds-app/package.json b/srv/lib/__tests__/__fixtures__/keyv-cds-app/package.json new file mode 100644 index 000000000..d2dd267fe --- /dev/null +++ b/srv/lib/__tests__/__fixtures__/keyv-cds-app/package.json @@ -0,0 +1,9 @@ +{ + "name": "keyv-cds-fixture", + "private": true, + "cds": { + "requires": { + "db": { "kind": "sqlite", "credentials": { "url": ":memory:" } } + } + } +} diff --git a/srv/lib/__tests__/keyv-cds-upsert.test.js b/srv/lib/__tests__/keyv-cds-upsert.test.js new file mode 100644 index 000000000..5ca481589 --- /dev/null +++ b/srv/lib/__tests__/keyv-cds-upsert.test.js @@ -0,0 +1,84 @@ +// srv/lib/__tests__/keyv-cds-upsert.test.js +// +// Regression guard for issue #3102: cds-caching's KeyvCDS.set() did a +// read-then-create/update (TOCTOU). Under multi-instance CF two instances +// both read "no row", both INSERT the same CacheStore key, and the second +// INSERT violates the PK unique index — which the adapter logs at ERROR +// level (the PROD noise) and the losing write is dropped. The committed +// patches/cds-caching+2.1.0.patch replaces that branch with an atomic +// single-key UPSERT (last-write-wins): no collision, no error log, no lost +// write. +// +// A single in-process SQLite test can't naturally race two HANA instances, +// so we model the TOCTOU window deterministically: stub the adapter's +// db.read to return undefined for the key (the window where a peer's INSERT +// is not yet visible to this reader) while the row already physically +// exists. Old code then takes the INSERT branch and collides; new code +// UPSERTs cleanly. + +import { describe, it, expect, beforeAll, vi } from 'vitest'; +import { createRequire } from 'node:module'; +import cds from '@sap/cds'; + +const require = createRequire(import.meta.url); +const KeyvCDS = require('cds-caching/lib/support/KeyvCDS.js'); + +cds.test(import.meta.dirname + '/__fixtures__/keyv-cds-app'); + +describe('KeyvCDS.set is collision-safe (issue #3102)', () => { + let entity; + beforeAll(() => { + entity = cds.entities('plugin.cds_caching').CacheStore; + }); + + it('serial re-set of the same key overwrites (last-write-wins)', async () => { + const store = new KeyvCDS({}); + await store.set('dup', 'first', 10_000); + await store.set('dup', 'second', 10_000); + expect(await store.get('dup')).toBe('second'); + }); + + it('re-set with no TTL overwrites cleanly (expiresAt null)', async () => { + const store = new KeyvCDS({}); + await store.set('nt', 'a', 10_000); + await store.set('nt', 'b'); // no ttl + expect(await store.get('nt')).toBe('b'); + const row = await cds.db.run(SELECT.one.from(entity).where({ ID: 'nt' })); + expect(row.expiresAt).toBeNull(); + }); + + // The decisive test. Reproduces the multi-instance TOCTOU on one process: + // the row already exists in the DB, but this adapter's read is forced to + // miss it (the race window). Pre-patch INSERT-branch collides on the PK and + // logs 'KeyvCDS set error'; post-patch UPSERT persists the value with no + // error log. + it('a write into an already-occupied key (race window) does not collide and persists', async () => { + const key = 'kg:rl:race:win'; + // Peer instance already INSERTed the row. + await cds.db.run( + INSERT.into(entity).entries({ ID: key, value: 'peer', expiresAt: Date.now() + 10_000 }), + ); + + const store = new KeyvCDS({}); + const logSpy = vi.spyOn(store.log, 'error').mockImplementation(() => {}); + // Force the read inside set() to miss (TOCTOU window). + const realRead = cds.db.read.bind(cds.db); + const readSpy = vi.spyOn(cds.db, 'read').mockImplementation(async (ent, k) => { + if (k === key) return undefined; + return realRead(ent, k); + }); + + try { + await store.set(key, 'mine', 10_000); + } finally { + readSpy.mockRestore(); + } + + // Patched (UPSERT): no error logged, value overwritten to 'mine'. + const setErrors = logSpy.mock.calls.filter((c) => String(c[0]).includes('set error')); + expect(setErrors).toHaveLength(0); + const row = await cds.db.run(SELECT.one.from(entity).where({ ID: key })); + expect(row.value).toBe('mine'); + logSpy.mockRestore(); + }); +});