From e1a731beedc26959eb3aaa324de0e29a352ddac8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 01:14:37 +0000 Subject: [PATCH 1/4] fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Postgres — the production default driver — every guarded save answered 409 CONCURRENT_UPDATE, including on records nobody had ever touched. The OCC gate read `updated_at` through `String(v)`; on a Date-returning driver that is `Sun Aug 30 2026 18:19:25 GMT+0800 (China Standard Time)` — milliseconds dropped, process timezone baked in — compared as a string against the `2026-08-30T10:19:25.947Z` the client echoed back from its own GET. One instant, two spellings. SQLite returns canonical ISO text, so both sides matched by accident and development environments stayed green. Both tokens are now normalised to one representation — a canonical absolute instant — before comparison, and the `currentVersion` a 409 publishes is that same canonical instant: what the wire format documents, and the token the conflict dialog echoes back as its next If-Match. Strictly widening: when either side is not an instant the verbatim comparison still runs, so no token accepted before is refused now. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L --- .changeset/occ-version-token-instant.md | 11 + ...protocol.occ-version-token-instant.test.ts | 373 ++++++++++++++++++ packages/metadata-protocol/src/protocol.ts | 158 +++++++- 3 files changed, 529 insertions(+), 13 deletions(-) create mode 100644 .changeset/occ-version-token-instant.md create mode 100644 packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts diff --git a/.changeset/occ-version-token-instant.md b/.changeset/occ-version-token-instant.md new file mode 100644 index 0000000000..057df147d2 --- /dev/null +++ b/.changeset/occ-version-token-instant.md @@ -0,0 +1,11 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +Fix optimistic concurrency raising a false `409 CONCURRENT_UPDATE` on every guarded save against a `Date`-returning driver (Postgres, MySQL, MongoDB). + +The OCC gate read the record's `updated_at` through `String(v)`. On Postgres that value is a JS `Date`, so the comparison ran against `"Sun Aug 30 2026 18:19:25 GMT+0800 (China Standard Time)"` — milliseconds dropped, process timezone baked in — while the client echoed back the `"2026-08-30T10:19:25.947Z"` its own GET had served. One instant, two spellings, strict string compare: every guarded `PATCH` / `DELETE` conflicted, including on records nobody had ever touched, which made the Console's record-edit dialog unusable on the production default driver. SQLite stores and returns canonical ISO text, so both sides matched by accident and development environments never saw it. + +Both tokens are now normalised to one representation — a canonical absolute instant — before they are compared, and the `currentVersion` a 409 publishes is that same canonical instant, which is what `content/docs/api/wire-format.mdx` documents the field to be and what the conflict dialog echoes back as its next `If-Match`. + +The change is strictly widening: a token accepted before is still accepted (a pair whose verbatim spellings were equal still compares equal, and when either side is not an instant the verbatim comparison is what runs), so a client mid-upgrade still holding a pre-fix 409's token is not locked out. Only two spellings of the same instant change verdict — from conflict to match. Conflicts between genuinely different versions are unchanged, down to the millisecond, and the verdict no longer depends on the process `TZ`. diff --git a/packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts b/packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts new file mode 100644 index 0000000000..ce196e0227 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts @@ -0,0 +1,373 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#13382] Optimistic concurrency compares INSTANTS, not spellings. + * + * ## The defect + * + * On Postgres — the production default driver — every guarded save answered + * `409 CONCURRENT_UPDATE`, including on a record nobody had ever touched. The + * OCC gate read the record's `updated_at` through `String(v)`; on Postgres that + * value is a JS `Date`, so `String(v)` produced + * `"Sun Aug 30 2026 18:19:25 GMT+0800 (China Standard Time)"` — milliseconds + * dropped, process timezone baked in — and compared it, as a string, against + * the `"2026-08-30T10:19:25.947Z"` the client had echoed back from its own GET. + * The sharpest evidence is in the error text the card carries: `current` and + * `expected` are the SAME INSTANT (`18:19:25+08:00` == `10:19:25.947Z`) and the + * conflict detector called them a conflict. + * + * SQLite stores and returns canonical ISO text, so both sides matched by + * accident and every development environment was green. **That green is the + * camouflage, which is why these tests drive the `Date` shape explicitly.** + * + * ## What is pinned here — the property, not the spelling + * + * Not "`normaliseVersionToken` calls `toISOString()`". A future representation + * change would break such a pin while leaving the contract intact. The + * invariants are: + * + * 1. **Two spellings of one instant never conflict.** Driven from a table of + * spelling pairs, each pair naming ONE instant. + * 2. **The verdict does not depend on `process.env.TZ`** — asserted under + * three process zones, with a non-vacuity control proving the zones really + * do move the broken spelling (a pass under three identical spellings is a + * pass that means nothing). + * 3. **Different instants still conflict**, down to the millisecond. The + * repair must not be a weakening. + * 4. **The 409's `currentVersion`, echoed straight back as the next token, is + * accepted.** The live-deployment corroboration on the card reports that + * that echo is what the Console's "Overwrite anyway" sends, and it was the + * only token the broken server would take. Fixing only the comparison and + * leaving the emission as `String(updated_at)` would convert a false + * conflict into an UNRESOLVABLE one, so this is a second limb of the same + * defect, not a nicety. + * 5. **Nothing accepted before is refused now.** The change is strictly + * widening — including for a client still holding a pre-fix 409's + * `Date.toString()` token across the upgrade. + * + * ## What is deliberately NOT claimed here + * + * That the driver hands this seam a `Date` on Postgres. That is a fact about + * `driver-sql`, measured against a live PostgreSQL 16 while fixing this (both + * before and after, under three process zones), and it cannot be asserted from + * this package — `@objectstack/metadata-protocol` has no driver dependency and + * must not grow one. What these tests own is the seam's behaviour GIVEN each + * input shape a driver can produce; the shapes themselves are enumerated in + * `canonicalVersionInstant`'s docblock beside the fix. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +// The producer's OWN write-verb dispatch decisions, from +// `@objectstack/metadata-core` and never from `@objectstack/objectql` — +// objectql DEPENDS ON this package, so that import would close a cycle. A +// hand-mirrored dispatch here would be a double looser than the engine it +// stands in for (`check:engine-double-contract`). +import { + assertEngineDeleteDispatch, + assertEngineFindOnePredicate, + assertEngineUpdateDispatch, +} from '@objectstack/metadata-core'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +const SCHEMA = { + name: 'task', + fields: { + title: { name: 'title', type: 'text' }, + updated_at: { name: 'updated_at', type: 'datetime' }, + }, +}; + +/** + * A fake engine holding ONE row whose `updated_at` is whatever the case under + * test says a driver returned — a `Date`, ISO text, epoch milliseconds, or an + * opaque token. + */ +function makeProtocol(updatedAt: unknown) { + const row: Record = { id: 'rec_1', title: 'one', updated_at: updatedAt }; + const findOne = vi.fn(async (_object: string, opts: any) => { + assertEngineFindOnePredicate(_object, opts); + return String(opts?.where?.id) === 'rec_1' ? { ...row } : null; + }); + const update = vi.fn(async (_object: string, data: any, opts?: any) => { + const dispatch = assertEngineUpdateDispatch(data, opts); + if (dispatch.kind !== 'by-id') { + throw new Error(`fixture drives by-id updates only, got '${dispatch.kind}'`); + } + const fields = { ...(data as Record) }; + delete fields.id; + Object.assign(row, fields); + return { ...row }; + }); + const del = vi.fn(async (_object: string, opts?: any) => { + assertEngineDeleteDispatch(opts); + return String(opts?.where?.id) === 'rec_1'; + }); + const engine = { + registry: { getObject: (n: string) => (n === 'task' ? SCHEMA : undefined) }, + findOne, + update, + delete: del, + }; + return { p: new ObjectStackProtocolImplementation(engine as any) as any, findOne, update, del }; +} + +/** Attempt a guarded PATCH; report the verdict without letting a throw escape. */ +async function guardedPatch(updatedAt: unknown, expectedVersion: string) { + const { p, update } = makeProtocol(updatedAt); + try { + await p.updateData({ object: 'task', id: 'rec_1', data: { title: 'edited' }, expectedVersion }); + return { accepted: true as const, wrote: update.mock.calls.length }; + } catch (e: any) { + return { + accepted: false as const, + wrote: update.mock.calls.length, + code: e?.code, + status: e?.status, + currentVersion: e?.currentVersion, + message: String(e?.message ?? ''), + }; + } +} + +/** The one instant every spelling below names. */ +const INSTANT = Date.UTC(2026, 7, 30, 10, 19, 25, 947); // 2026-08-30T10:19:25.947Z +const ISO = new Date(INSTANT).toISOString(); + +// ───────────────────────────────────────────────────────────────────────────── +// 1. Two spellings of one instant never conflict +// ───────────────────────────────────────────────────────────────────────────── + +/** + * Each row: what the DRIVER put in `updated_at`, and what the CLIENT echoed + * back — two spellings of {@link INSTANT}. Every row must be accepted. + * + * The driver column is the measured input domain of the OCC seam: a `Date` on + * Postgres / MySQL / MongoDB, canonical ISO text on the SQLite family and the + * memory driver, epoch milliseconds on a pre-canonical or hand-migrated SQLite + * column (the driver's legacy-datetime repair is keyed on declared + * `Field.datetime` columns, and the engine-injected audit columns are not in + * that set, so such a value reaches this seam unrepaired). + */ +const ONE_INSTANT_TWO_SPELLINGS: ReadonlyArray<{ + why: string; + driver: unknown; + client: string; +}> = [ + { + why: 'Postgres / MySQL / MongoDB: driver returns a Date, client echoes the ISO the GET served', + driver: new Date(INSTANT), + client: ISO, + }, + { + why: 'the same, with the RFC-7232 quotes an If-Match header carries', + driver: new Date(INSTANT), + client: `"${ISO}"`, + }, + { + why: 'SQLite / Turso / sqlite-wasm / memory: canonical ISO text on both sides (unchanged)', + driver: ISO, + client: ISO, + }, + { + why: 'a client that spells the instant with a numeric offset instead of Z', + driver: new Date(INSTANT), + client: '2026-08-30T18:19:25.947+08:00', + }, + { + why: 'ISO text in the column, offset spelling from the client', + driver: ISO, + client: '2026-08-30T18:19:25.947+08:00', + }, + { + why: 'a pre-canonical SQLite column holding epoch milliseconds', + driver: INSTANT, + client: ISO, + }, + { + why: 'a microsecond rendering truncates to the millisecond the record can hold', + driver: new Date(INSTANT), + client: '2026-08-30T10:19:25.947123Z', + }, +]; + +describe('[#13382] two spellings of ONE instant are not a conflict', () => { + for (const { why, driver, client } of ONE_INSTANT_TWO_SPELLINGS) { + it(why, async () => { + const verdict = await guardedPatch(driver, client); + expect( + verdict, + `expected the guarded save to be ACCEPTED; got ${JSON.stringify(verdict)}`, + ).toMatchObject({ accepted: true }); + expect(verdict.wrote).toBe(1); + }); + } + + it('the DELETE door agrees with the PATCH door — one seam, one verdict', async () => { + // `deleteData` runs the same comparison through its own probe + // (`assertVersionMatch`), so a fix applied to one door only would leave + // guarded deletes 409-ing on Postgres for ever. + const { p, del } = makeProtocol(new Date(INSTANT)); + await p.deleteData({ object: 'task', id: 'rec_1', expectedVersion: ISO }); + expect(del).toHaveBeenCalledOnce(); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 2. The verdict does not depend on the process timezone +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#13382] the verdict is a fact about the instant, not about `TZ`', () => { + const ORIGINAL_TZ = process.env.TZ; + afterEach(() => { + if (ORIGINAL_TZ === undefined) delete process.env.TZ; + else process.env.TZ = ORIGINAL_TZ; + }); + + // The card's zone, CI's skewed zone, and UTC. Pairwise different by + // construction, which is what makes the sweep non-vacuous. + const ZONES = ['Asia/Shanghai', 'America/New_York', 'UTC'] as const; + + it('accepts the same token under three process zones, and the zones really do move the broken spelling', async () => { + const brokenSpellings = new Set(); + for (const zone of ZONES) { + process.env.TZ = zone; + const stamped = new Date(INSTANT); + // NON-VACUITY CONTROL, in the spirit of the driver matrix's + // three-way zone skew: `String(Date)` is the spelling the defect + // compared. If the zones did not move it, three green rows would + // prove nothing at all about timezone independence. + brokenSpellings.add(String(stamped)); + const verdict = await guardedPatch(stamped, ISO); + expect(verdict, `refused under TZ=${zone}: ${JSON.stringify(verdict)}`).toMatchObject({ + accepted: true, + }); + } + expect( + brokenSpellings.size, + 'the three process zones must produce three DIFFERENT `String(Date)` spellings, ' + + 'or this sweep is vacuous', + ).toBe(ZONES.length); + }); + + it('a 409 names the same instant under every process zone', async () => { + const published = new Set(); + for (const zone of ZONES) { + process.env.TZ = zone; + const verdict = await guardedPatch(new Date(INSTANT), new Date(INSTANT + 1).toISOString()); + expect(verdict).toMatchObject({ accepted: false, code: 'CONCURRENT_UPDATE' }); + published.add(String((verdict as any).currentVersion)); + } + expect( + published, + 'the published `currentVersion` drifted with the process timezone', + ).toEqual(new Set([ISO])); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 3. The repair is not a weakening +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#13382] different versions still conflict', () => { + it('one millisecond apart is a conflict, and nothing is written', async () => { + const verdict = await guardedPatch(new Date(INSTANT), new Date(INSTANT + 1).toISOString()); + expect(verdict).toMatchObject({ accepted: false, code: 'CONCURRENT_UPDATE', status: 409 }); + expect(verdict.wrote).toBe(0); + }); + + it('a stale token from an earlier version of the row is a conflict', async () => { + const verdict = await guardedPatch(new Date(INSTANT), new Date(INSTANT - 60_000).toISOString()); + expect(verdict).toMatchObject({ accepted: false, code: 'CONCURRENT_UPDATE', status: 409 }); + expect(verdict.wrote).toBe(0); + }); + + it('an opaque, non-temporal token is compared verbatim — matching accepts, differing conflicts', async () => { + // A host stamping its own version string into the column keeps exactly + // the semantics it had: the instant path is not a licence to guess. + expect(await guardedPatch('rowversion-7', 'rowversion-7')).toMatchObject({ accepted: true }); + expect(await guardedPatch('rowversion-7', 'rowversion-8')).toMatchObject({ + accepted: false, + code: 'CONCURRENT_UPDATE', + }); + }); + + it('a zone-LESS date-time is NOT reinterpreted through the process timezone', async () => { + // `Date.parse('2026-08-30 18:19:25.947')` reads LOCAL time, which would + // make the verdict depend on `TZ` — the one thing the fix must not do. + // Such a token stays opaque and is compared verbatim, so it matches the + // identical stored text and nothing else. + process.env.TZ = 'Asia/Shanghai'; + try { + expect(await guardedPatch('2026-08-30 18:19:25.947', '2026-08-30 18:19:25.947')).toMatchObject({ + accepted: true, + }); + expect(await guardedPatch(new Date(INSTANT), '2026-08-30 18:19:25.947')).toMatchObject({ + accepted: false, + code: 'CONCURRENT_UPDATE', + }); + } finally { + delete process.env.TZ; + } + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 4. The 409's own token closes the loop +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#13382] the token a 409 publishes is a token the server accepts', () => { + it('echoing `currentVersion` straight back is accepted — the "Overwrite anyway" path', async () => { + const conflict = await guardedPatch(new Date(INSTANT), new Date(INSTANT - 60_000).toISOString()); + expect(conflict).toMatchObject({ accepted: false, code: 'CONCURRENT_UPDATE' }); + const echoed = await guardedPatch(new Date(INSTANT), String((conflict as any).currentVersion)); + expect( + echoed, + 'the conflict dialog re-keys its retry to `currentVersion`; a token the server ' + + 'will not take turns a resolvable conflict into a dead end', + ).toMatchObject({ accepted: true }); + }); + + it('the published `currentVersion` is the canonical instant, matching the documented wire format', async () => { + // `content/docs/api/wire-format.mdx` documents this field as an ISO-8601 + // UTC timestamp. On a Date-returning driver it was a `Date.toString()`. + const conflict = await guardedPatch(new Date(INSTANT), new Date(INSTANT - 1).toISOString()); + expect((conflict as any).currentVersion).toBe(ISO); + expect((conflict as any).message).toContain(ISO); + }); + + it('an opaque version is published verbatim, not coerced into a timestamp', async () => { + const conflict = await guardedPatch('rowversion-7', 'rowversion-8'); + expect((conflict as any).currentVersion).toBe('rowversion-7'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 5. Strictly widening: nothing accepted before is refused now +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#13382] the change cannot refuse a token that was accepted before', () => { + it('a client still echoing a PRE-FIX 409 token (`Date.toString()`, milliseconds already lost) is accepted', async () => { + // The live-deployment report on the card says this string was the ONLY + // token the broken server would take. A client holding one across the + // upgrade must not be locked out: when either side is not an instant the + // comparison falls back to the verbatim tokens, exactly as before. + process.env.TZ = 'Asia/Shanghai'; + try { + const stamped = new Date(INSTANT); + const preFixToken = String(stamped); // what the old code published + expect(preFixToken).not.toBe(ISO); // control: it really is the other spelling + expect(await guardedPatch(stamped, preFixToken)).toMatchObject({ accepted: true }); + } finally { + delete process.env.TZ; + } + }); + + it('an empty or blank token still opts OUT of the check rather than conflicting', async () => { + expect(await guardedPatch(new Date(INSTANT), '')).toMatchObject({ accepted: true }); + expect(await guardedPatch(new Date(INSTANT), ' ')).toMatchObject({ accepted: true }); + }); + + it('a record with no `updated_at` still skips the check', async () => { + expect(await guardedPatch(undefined, ISO)).toMatchObject({ accepted: true }); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 63c6482fd6..9204c8fccd 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -1371,18 +1371,133 @@ export class ConcurrentUpdateError extends Error { } /** - * Normalises a version token for comparison. Strips RFC-7232-style quotes - * (`"…"`) that an HTTP `If-Match` header may carry, trims whitespace, and - * returns null for empty / nullish input. + * An ISO-8601 date-time that denotes an ABSOLUTE instant — it carries an + * explicit `Z` or a numeric `±HH:mm` offset, so reading it never consults the + * process timezone. + * + * Deliberately narrower than a bare `Date.parse`, which falls back to + * implementation-specific heuristics outside this shape and reads a zone-LESS + * date-time as *local* time — the one thing an OCC verdict must never depend on. + * A string this pattern rejects is not reinterpreted: it stays an opaque token + * and is compared verbatim, exactly as before. */ -function normaliseVersionToken(v: unknown): string | null { +const ABSOLUTE_ISO_INSTANT = + /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}(?::\d{2}(?:\.\d+)?)?(?:Z|[+-]\d{2}:\d{2})$/; + +/** Largest magnitude a JS time value may hold; `toISOString()` throws beyond it. */ +const MAX_TIME_VALUE = 8.64e15; + +/** + * A version token read into the two forms the OCC comparison may need. + * + * `token` is the tidied verbatim string — the whole normalisation this seam used + * to do. `instant` is the canonical absolute-instant spelling, present only when + * the token really denotes one. + */ +interface NormalisedVersion { + /** The token as compared verbatim: trimmed, RFC-7232 quotes stripped. */ + readonly token: string; + /** Canonical ISO-8601 UTC (`…Z`), or null when the token is opaque. */ + readonly instant: string | null; +} + +/** + * The canonical absolute-instant spelling of a version token, or null when the + * token does not denote an instant. + * + * The input domain is the set of things a DRIVER puts in `updated_at`, measured + * rather than listed from memory (#13382): + * + * - **JS `Date`** — `driver-sql` on Postgres and MySQL (`timestamptz` / + * `DATETIME(3)` are instants, and the driver materialises them as `Date` on + * purpose; `SqlDriver.withPostgresCalendarDayAsText` says so in as many + * words) and `driver-mongodb` (it stamps `new Date()`, and BSON round-trips + * it). Canonical form: `toISOString()`. + * - **`string`, already canonical ISO-8601 UTC with milliseconds** — + * `driver-sql` on SQLite, its `driver-turso` / `driver-sqlite-wasm` siblings, + * and `driver-memory`. Canonical form: itself, re-derived through `Date`. + * - **`number`, epoch milliseconds** — a pre-canonical or hand-migrated SQLite + * column. Reachable: the driver's legacy datetime repair is keyed on declared + * `Field.datetime` columns, and the engine-injected audit columns are not in + * that set, so such a value passes through unrepaired. Canonical form: + * `new Date(ms).toISOString()`. + * - **`null` / `undefined`** — timestamps disabled, or no such column. No + * check runs at all; handled by the caller, not here. + * - **anything else** a host stamps into the column — opaque, and compared + * verbatim rather than guessed at. + * + * Milliseconds are the resolution, because that is what a JS `Date` — and so + * every driver's materialised instant — can carry. A token with more fractional + * digits (a Postgres microsecond rendering, say) truncates to the same + * millisecond as the record it is compared against, which is the point: two + * spellings of one instant must not conflict. + */ +function canonicalVersionInstant(value: unknown, token: string): string | null { + let ms: number; + if (value instanceof Date) { + ms = value.getTime(); + } else if (typeof value === 'number') { + ms = value; + } else if (ABSOLUTE_ISO_INSTANT.test(token)) { + ms = Date.parse(token); + } else { + return null; + } + if (!Number.isFinite(ms) || Math.abs(ms) > MAX_TIME_VALUE) return null; + return new Date(ms).toISOString(); +} + +/** + * Reads a version token into the forms `assertVersionOf` compares. Strips + * RFC-7232-style quotes (`"…"`) that an HTTP `If-Match` header may carry, trims + * whitespace, and returns null for empty / nullish input — a null is the + * caller's "no token supplied", which opts out of the check. + * + * ## Why this returns two forms rather than one string (#13382) + * + * It used to return `String(v).trim()` alone, and the two sides of the OCC + * comparison do not agree on how to spell one instant. On Postgres the record's + * `updated_at` arrives as a JS `Date`, whose `String()` is + * `"Sun Aug 30 2026 18:19:25 GMT+0800 (China Standard Time)"` — **milliseconds + * dropped and the process timezone baked in** — while the client echoes back + * the `"2026-08-30T10:19:25.947Z"` the GET served it. One instant, two + * spellings, strict string compare: every guarded save on the production + * default driver answered `409 CONCURRENT_UPDATE`, on records nobody had ever + * touched. SQLite stores and returns ISO text, so both sides matched by + * accident and the defect never surfaced in development. + * + * So the comparison normalises to ONE representation before comparing — a + * canonical absolute instant — and falls back to the verbatim token when either + * side is not an instant. That fallback is what makes the change **strictly + * widening**: any pair that matched before still matches (their verbatim + * strings were equal, so they either canonicalise equally or are compared as + * strings again), so no token a client sends today starts being refused — + * including a client still echoing a pre-fix 409's `Date.toString()` value. + * Only pairs denoting the same instant in different spellings change verdict, + * from conflict to match, which is the defect. + */ +function normaliseVersionToken(v: unknown): NormalisedVersion | null { if (v === null || v === undefined) return null; - const s = String(v).trim(); - if (!s) return null; - if (s.length >= 2 && s.startsWith('"') && s.endsWith('"')) { - return s.slice(1, -1); + let token = String(v).trim(); + if (!token) return null; + if (token.length >= 2 && token.startsWith('"') && token.endsWith('"')) { + token = token.slice(1, -1); + } + return { token, instant: canonicalVersionInstant(v, token) }; +} + +/** + * Do two version tokens name the same record version? + * + * Instants are compared as instants — that is the repair. When either side is + * opaque the tokens are compared verbatim, which is what this seam has always + * done and what keeps an opaque host-stamped version working. + */ +function versionTokensAgree(current: NormalisedVersion, expected: NormalisedVersion): boolean { + if (current.instant !== null && expected.instant !== null) { + return current.instant === expected.instant; } - return s; + return current.token === expected.token; } // Lifecycle columns the engine always owns; the clone path drops them by NAME @@ -10002,6 +10117,13 @@ export class ObjectStackProtocolImplementation implements * Logging would be noisy here; OCC is opt-in and the absence of a * version column is an explicit "this object doesn't support OCC" * signal. + * - Both tokens are normalised to ONE representation before they are + * compared (#13382): a canonical absolute instant when they denote one, + * the verbatim token otherwise. Which spelling of an instant a driver + * hands back — a JS `Date` on Postgres/MySQL/Mongo, ISO text on the + * SQLite family — must not decide whether a save is a conflict. See + * `normaliseVersionToken` for the measured input domain and for why the + * change cannot refuse a token that is accepted today. */ private assertVersionOf( object: string, @@ -10012,13 +10134,23 @@ export class ObjectStackProtocolImplementation implements const expected = normaliseVersionToken(expectedVersion); if (!expected) return; if (!current) return; - const currentVersion = normaliseVersionToken((current as any).updated_at); - if (!currentVersion) return; - if (currentVersion !== expected) { + const currentToken = normaliseVersionToken((current as any).updated_at); + if (!currentToken) return; + if (!versionTokensAgree(currentToken, expected)) { + // Publish the CANONICAL spelling, not `String(updated_at)` (#13382). + // Two reasons, and the second is not optional: it is what + // `content/docs/api/wire-format.mdx` documents this field to be, and + // it is the token the client echoes back as its next `If-Match` when + // the user chooses "Overwrite anyway" — so a 409 whose + // `currentVersion` is a `Date.toString()` (milliseconds already + // dropped) hands the client a token that can never match the record + // it names, turning a resolvable conflict into an unresolvable one. + const currentVersion = currentToken.instant ?? currentToken.token; + const expectedVersionText = expected.instant ?? expected.token; throw new ConcurrentUpdateError({ currentVersion, currentRecord: current, - message: `Record ${object}/${id} was modified by another user (current version ${currentVersion}, expected ${expected})`, + message: `Record ${object}/${id} was modified by another user (current version ${currentVersion}, expected ${expectedVersionText})`, }); } } From 4295e8de827df92b797ff29fc0ac59ba326c76b9 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 01:30:47 +0000 Subject: [PATCH 2/4] chore: record the new OCC engine double in the pinned ledger (#13382) `check:engine-double-contract` retains a pin per (file, verb). The regression suite added for #13382 carries a fake engine whose delete/update/findOne route through the producer's own dispatch predicates, so the ledger has to learn about it or it never protects the file. `--write`, 3 rows added, 0 lost. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L --- scripts/engine-double-contract.pinned.json | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 3056333405..f9b75426c2 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -771,6 +771,21 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts", + "verb": "delete", + "pinned": 1 + }, + { + "file": "packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts", + "verb": "findOne", + "pinned": 1 + }, + { + "file": "packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/metadata-protocol/src/protocol.org-scoped-cold-boot-audit-live-registry.test.ts", "verb": "delete", From e27345c895a808319d8ce3c8b855df683270c916 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 02:00:14 +0000 Subject: [PATCH 3/4] docs(permissions): re-anchor the isSystem census row onto the shifted line (#13382) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `content/docs/permissions/system-context.mdx` anchors elevation reads by line number. The OCC fix added 158 lines above `stripReadonlyForInsert`, so row 21's anchor into metadata-protocol rotted: the census found an unanchored read at :1566 and an anchor at :1451 that is no longer a read site. A PURE SHIFT, verified before repairing rather than assumed — the gate refuses to guess a population change and rewrites only line rot: base :1451 and head :1566 are the byte-identical `if (context?.isSystem) return data;` (sha256 db885f75…), the `isSystem` occurrence count in the file is 9 on both trees, and :1566 is the first line of `stripReadonlyForInsert`, which is exactly what row 21 describes. Repaired with the gate's own `--fix`; one anchor rewritten, no prose touched, nothing baselined or exempted. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L --- content/docs/permissions/system-context.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 0b867113f1..4a41cbf63d 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -112,7 +112,7 @@ that silently does not happen. | 18 | **`readonly` strip bypassed — UPDATE, single row** | objectql | Get: a `readonly` field CAN be written. Lose: the protection that stops a caller seeding e.g. `approval_status` | `objectql/src/engine.ts:10581` | | 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:10743` | | 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9474` | -| 21 | **`readonly` strip bypassed — INSERT (protocol ingress)** | metadata-protocol | `isSystem` is the **only** exemption here. `preserveAudit` is deliberately not read on this path (#6640) — a non-system historical import is still stripped on create | `metadata-protocol/src/protocol.ts:1451` | +| 21 | **`readonly` strip bypassed — INSERT (protocol ingress)** | metadata-protocol | `isSystem` is the **only** exemption here. `preserveAudit` is deliberately not read on this path (#6640) — a non-system historical import is still stripped on create | `metadata-protocol/src/protocol.ts:1566` | | 22 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `objectql/src/engine.ts:9511`, `readonly-strict-errors.ts:66` | | 23 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `objectql/src/engine.ts:5603` | | 24 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `objectql/src/engine.ts:3571`, `:3573`, `:3600` | From 14c197f87ed384e24757413a2362c7687d1e94ac Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 02:11:11 +0000 Subject: [PATCH 4/4] fix(metadata-protocol): keep an empty If-Match entity-tag opting OUT of OCC (#13382) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Contract review caught a regression this PR introduced. `If-Match: ""` is empty only AFTER the RFC-7232 quotes come off, and the pre-fix seam returned the bare string, so that case handed every caller the falsy `''` and they short-circuited into "no token supplied". Wrapping the result in an object made it always truthy, so the check began running and `''` mismatched verbatim: an accept-to-refuse flip on a shipped API, falsifying this PR's own strictly- widening claim. Remedy is the conservative one — re-apply the emptiness test after the strip, so the behaviour is byte-identical to `70fe54891e`. Whether an empty entity-tag SHOULD be able to disable OCC is a contract question, filed separately; a p1 bug-fix PR does not silently install a new rejection. The claim is no longer left as prose. A new block sweeps a corpus of 8 stored shapes x 15 client tokens against the pre-fix comparison reproduced verbatim and fails on any pair that was accepted before and is refused now. Run against the unfixed tree it reports exactly the 5 `""` pairs and nothing else, so the review finding was the whole regression rather than one instance of a class. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L --- content/docs/permissions/system-context.mdx | 2 +- ...protocol.occ-version-token-instant.test.ts | 121 +++++++++++++++++- packages/metadata-protocol/src/protocol.ts | 10 ++ 3 files changed, 131 insertions(+), 2 deletions(-) diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 4a41cbf63d..2710e17429 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -112,7 +112,7 @@ that silently does not happen. | 18 | **`readonly` strip bypassed — UPDATE, single row** | objectql | Get: a `readonly` field CAN be written. Lose: the protection that stops a caller seeding e.g. `approval_status` | `objectql/src/engine.ts:10581` | | 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:10743` | | 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9474` | -| 21 | **`readonly` strip bypassed — INSERT (protocol ingress)** | metadata-protocol | `isSystem` is the **only** exemption here. `preserveAudit` is deliberately not read on this path (#6640) — a non-system historical import is still stripped on create | `metadata-protocol/src/protocol.ts:1566` | +| 21 | **`readonly` strip bypassed — INSERT (protocol ingress)** | metadata-protocol | `isSystem` is the **only** exemption here. `preserveAudit` is deliberately not read on this path (#6640) — a non-system historical import is still stripped on create | `metadata-protocol/src/protocol.ts:1576` | | 22 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `objectql/src/engine.ts:9511`, `readonly-strict-errors.ts:66` | | 23 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `objectql/src/engine.ts:5603` | | 24 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `objectql/src/engine.ts:3571`, `:3573`, `:3600` | diff --git a/packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts b/packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts index ce196e0227..26aa7d9528 100644 --- a/packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts +++ b/packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts @@ -43,7 +43,11 @@ * defect, not a nicety. * 5. **Nothing accepted before is refused now.** The change is strictly * widening — including for a client still holding a pre-fix 409's - * `Date.toString()` token across the upgrade. + * `Date.toString()` token across the upgrade. This one is not left as + * prose: the last block sweeps a corpus of (stored value, client token) + * pairs against the pre-fix comparison reproduced verbatim, because the + * prose version of this claim was believed by three readers while + * `If-Match: ""` had already flipped from accept to 409. * * ## What is deliberately NOT claimed here * @@ -367,7 +371,122 @@ describe('[#13382] the change cannot refuse a token that was accepted before', ( expect(await guardedPatch(new Date(INSTANT), ' ')).toMatchObject({ accepted: true }); }); + it('an EMPTY If-Match entity-tag — a bare pair of quotes — still opts out', async () => { + // `If-Match: ""` is empty only AFTER the RFC-7232 quotes come off, so the + // emptiness test has to run on the stripped token and not just the raw + // one. This is the shipped behaviour, not a new one: the pre-fix seam + // returned the empty STRING here and every caller short-circuited on its + // falsiness. Wrapping the result in an object made it truthy and turned + // this token from "skip the check" into a 409 — an accept-to-refuse flip + // on a live API, which is exactly what the widening guarantee forbids. + expect(await guardedPatch(new Date(INSTANT), '""')).toMatchObject({ accepted: true }); + expect(await guardedPatch(ISO, '""')).toMatchObject({ accepted: true }); + }); + + it('the DELETE door opts out on the same empty tag — it has its own call site', async () => { + // `assertVersionMatch` tests the token's truthiness itself, before it + // probes, so it can regress independently of the PATCH door. + const { p, del, findOne } = makeProtocol(new Date(INSTANT)); + await p.deleteData({ object: 'task', id: 'rec_1', expectedVersion: '""' }); + expect(del).toHaveBeenCalledOnce(); + // Opting out also means NOT paying for the probe the OCC check needs. + expect(findOne).not.toHaveBeenCalled(); + }); + it('a record with no `updated_at` still skips the check', async () => { expect(await guardedPatch(undefined, ISO)).toMatchObject({ accepted: true }); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// 6. The widening guarantee, checked against the OLD implementation rather than +// asserted in prose +// ───────────────────────────────────────────────────────────────────────────── + +/** + * The pre-fix normalisation, verbatim from `70fe54891e` — the reference this + * change promises to be a superset of. + * + * It is reproduced here rather than described because the claim "no token a + * client sends today starts being refused" is a claim ABOUT this function, and + * the `""` regression above is what a prose-only version of the claim costs: it + * read as true to three separate readers while one token had already flipped. + * A sweep over a corpus can be wrong about coverage; it cannot be wrong about + * the pairs it covers. + */ +function preFixNormalise(v: unknown): string | null { + if (v === null || v === undefined) return null; + const s = String(v).trim(); + if (!s) return null; + if (s.length >= 2 && s.startsWith('"') && s.endsWith('"')) { + return s.slice(1, -1); + } + return s; +} + +/** The pre-fix verdict for one (stored value, client token) pair. */ +function preFixAccepts(updatedAt: unknown, expectedVersion: string): boolean { + const expected = preFixNormalise(expectedVersion); + if (!expected) return true; // no token supplied -> no check + const current = preFixNormalise(updatedAt); + if (!current) return true; // no version on the record -> no check + return current === expected; +} + +describe('[#13382] every pair the OLD comparison accepted is still accepted', () => { + /** What a driver can put in the column, across the enumerated input domain. */ + const STORED: ReadonlyArray<[string, unknown]> = [ + ['Date (Postgres/MySQL/Mongo)', new Date(INSTANT)], + ['canonical ISO text (SQLite family, memory)', ISO], + ['epoch ms (pre-canonical SQLite)', INSTANT], + ['an opaque host version', 'rowversion-7'], + ['a zone-less date-time', '2026-08-30 18:19:25.947'], + ['absent', undefined], + ['null', null], + ['empty string', ''], + ]; + + /** What a client can put in `If-Match` / `expectedVersion`. */ + const TOKENS: readonly string[] = [ + ISO, + `"${ISO}"`, + '""', + '', + ' ', + '" "', + String(new Date(INSTANT)), + '2026-08-30T18:19:25.947+08:00', + '2026-08-30T10:19:25.947123Z', + '2026-08-30 18:19:25.947', + 'rowversion-7', + '"rowversion-7"', + 'rowversion-8', + String(INSTANT), + new Date(INSTANT + 1).toISOString(), + ]; + + it('over the whole corpus, the accept set only GROWS', async () => { + const regressions: string[] = []; + let acceptedBefore = 0; + let newlyAccepted = 0; + for (const [label, stored] of STORED) { + for (const token of TOKENS) { + const before = preFixAccepts(stored, token); + const after = (await guardedPatch(stored, token)).accepted; + if (before) acceptedBefore += 1; + if (before && !after) { + regressions.push(`${label} + ${JSON.stringify(token)}: accepted before, REFUSED now`); + } + if (!before && after) newlyAccepted += 1; + } + } + expect(regressions, regressions.join('\n')).toEqual([]); + // Non-vacuity: a sweep where nothing was ever accepted, or where the two + // implementations never disagree, would pass while proving nothing. + expect(acceptedBefore).toBeGreaterThan(0); + expect( + newlyAccepted, + 'the corpus must contain at least one pair this change newly accepts, or it is not exercising the repair', + ).toBeGreaterThan(0); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 9204c8fccd..6a08f3244a 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -1482,6 +1482,16 @@ function normaliseVersionToken(v: unknown): NormalisedVersion | null { if (!token) return null; if (token.length >= 2 && token.startsWith('"') && token.endsWith('"')) { token = token.slice(1, -1); + // ⚠️ The emptiness test runs on BOTH sides of the strip, and the second + // one is load-bearing: `If-Match: ""` is empty only once the RFC-7232 + // quotes come off. Before this seam returned an object it returned the + // bare string, so that case handed every caller the falsy `''` and they + // short-circuited into "no token supplied" — the opt-out was riding + // implicitly on the empty string's falsiness. An object is always + // truthy, so without this line an empty entity-tag would stop skipping + // the check and start earning a 409: an accept-to-refuse flip on a + // shipped API, which is precisely what this change promises not to do. + if (!token) return null; } return { token, instant: canonicalVersionInstant(v, token) }; }