diff --git a/.changeset/by-id-unhonoured-predicate-refusal.md b/.changeset/by-id-unhonoured-predicate-refusal.md new file mode 100644 index 0000000000..8da00ee070 --- /dev/null +++ b/.changeset/by-id-unhonoured-predicate-refusal.md @@ -0,0 +1,21 @@ +--- +"@objectstack/metadata-core": minor +"@objectstack/objectql": minor +"@objectstack/service-messaging": minor +--- + +**BREAKING (accept-set tightening)**: a by-id `update`/`delete` whose `options.where` carries predicate keys beyond `id` is now refused loudly instead of silently dropping the predicate (#11009). + +The by-id dispatch routes to `driver.update(object, id, …)` / `driver.delete(object, id, …)`, which bind ONLY the primary key — every other `where` key was discarded with no diagnostic. A compare-and-set written as `{ where: { id, status: { $in: [...] } }, multi: false }` therefore evaluated to nothing and the write landed unconditionally, reading exactly like a working conditional write. Measured on `better-sqlite3` through a real `ObjectQL` + `SqlDriver`: `SqlHttpOutbox.redeliver`'s terminal-status guard was inert, so a delivery row claimed `in_flight` mid-redeliver was reset anyway and the redelivery reported success while the in-flight attempt kept running. + +What changes, per call shape (`resolveEngineUpdateDispatch` / `resolveEngineDeleteDispatch`, so every pinned test double inherits the same verdicts): + +- A `where` naming a scalar `id` **and nothing else** is unchanged — by-id, with or without `multi: true` (the `LifecycleService` guarded-reap idiom keeps its per-record cascade path). +- A `where` carrying a scalar `id` **plus other keys**, with a declared `multi: true` (id sourced from `where`): now routes to the **predicate path** (`driver.updateMany` / `driver.deleteMany`), which compiles EVERY `where` key — the compare-and-set spelling. Previously this dispatched by-id and dropped the extra keys. +- The same shape **without** `multi: true` — and any by-id call via a scalar `data.id` beside extra `where` keys, `multi` or not (the payload id outranks `multi` per #5748 and cannot be demoted onto the predicate path): now **throws**, naming the keys the by-id path would have dropped. Previously it succeeded with the condition ignored. + +A caller hitting the new refusal decides which of two things it meant, and each is a one-line edit at the call site: declare the predicate path (`multi: true`) so the full `where` is honoured and the result is the matched count, or drop the extra `where` keys to keep an unconditional single-row write. Flow authors reach this through `update_record` / `delete_record` nodes whose `filter` names `id` plus other keys without declaring `multi` — those configs were silently unconditional before and refuse loudly now. + +`SqlHttpOutbox.redeliver` itself now rides the predicate path, and `MemoryHttpOutbox.redeliver` re-checks terminal status after its guard, so both `IHttpOutbox` implementations agree: a row claimed between redeliver's read and its write is NOT reset, and `redeliver` reports `DELIVERY_NOT_ELIGIBLE` instead of success. + + diff --git a/packages/metadata-core/src/engine-delete-dispatch.ts b/packages/metadata-core/src/engine-delete-dispatch.ts index 606726d298..1ffa07034c 100644 --- a/packages/metadata-core/src/engine-delete-dispatch.ts +++ b/packages/metadata-core/src/engine-delete-dispatch.ts @@ -54,7 +54,9 @@ * `objectql -> metadata-core` and `metadata-protocol -> metadata-core` both * pre-date this change, and this package's own dependencies are * `{ @objectstack/spec, zod }` — no `objectql`, so no new edge and no new cycle. - * This module importing nothing at all is what makes that free. + * This module importing nothing from outside its own package is what makes + * that free (its one sibling import, the #11009 refusal shared with the + * update twin, adds no package edge). * * `@objectstack/objectql` re-exports every symbol below from its original path, * so the 24+ call sites already pinned to it, and the public API, are unchanged. @@ -72,6 +74,16 @@ * - otherwise → **`reject`**. The call names neither one row nor a bulk * intent, and the engine throws rather than guessing. * + * **[#11009] One overriding clause on the by-id arm** (byte-for-byte the + * update twin's, minus the payload door `delete` does not have): the by-id + * route binds ONLY the primary key, so a `where` carrying any key besides + * `id` is a predicate the by-id path would silently discard. Such a call is + * never dispatched `by-id` — with `multi` truthy it is `multi` (the full + * `where`, scalar id included, rides the AST to `driver.deleteMany` — the + * compare-and-set spelling), otherwise it is `reject`, naming the dropped + * keys. A PURE-id `where` (`{ where: { id } }`) is untouched: it stays + * `by-id` even beside `multi: true` (LifecycleService's guarded-reap idiom). + * * Two halves of that first line are load-bearing, and a hand-written double * drops one or the other — which is the whole argument for importing this * instead of copying it: @@ -112,6 +124,11 @@ * @see scripts/check-engine-double-contract.mjs — the gate that keeps doubles on it. */ +import { + engineByIdUnhonouredPredicateMessage, + unhonouredByIdPredicateKeys, +} from './engine-dispatch-unhonoured-predicate.js'; + /** The message `delete()` throws when a call identifies neither one row nor a bulk intent. */ export const ENGINE_DELETE_REJECT_MESSAGE = 'Delete requires an ID or options.multi=true'; @@ -177,7 +194,32 @@ export function resolveEngineDeleteDispatch( // `!== undefined`, so a falsy scalar id (`0`, `''`) is not an identifying // call and falls down the same ladder as a non-scalar one. See header // point 2, and the twin's point 3 (objectstack#5747 / objectstack#5748). - if (id) return { kind: 'by-id', id }; + if (id) { + // [#11009] The by-id route binds ONLY the primary key, so a `where` + // carrying any key besides `id` is a predicate the by-id path would + // silently discard (the guard evaluated to nothing — the shape the update + // twin measured on `SqlHttpOutbox.redeliver`). Byte-for-byte the twin's + // rule, minus the payload arm `delete` does not have: + // + // - `multi` truthy → `multi`: a declared predicate call; the full + // `where` (scalar id included, as an equality term) rides the AST to + // `driver.deleteMany`. The compare-and-set spelling. ⚠️ A PURE-id + // `where` never reaches this branch and stays by-id even under + // `multi: true` — LifecycleService's guarded reap depends on that for + // per-record cascade handling, and the predicate path deliberately + // trades cascade for an honoured predicate. + // - otherwise → `reject`, loudly naming the keys that would have been + // dropped. + const unhonoured = unhonouredByIdPredicateKeys(options?.where); + if (unhonoured.length > 0) { + if (options?.multi) return { kind: 'multi' }; + return { + kind: 'reject', + message: engineByIdUnhonouredPredicateMessage('Delete', unhonoured), + }; + } + return { kind: 'by-id', id }; + } if (options?.multi) return { kind: 'multi' }; return { kind: 'reject', message: ENGINE_DELETE_REJECT_MESSAGE }; } @@ -225,10 +267,18 @@ export interface EngineDeleteDispatchCase { export const ENGINE_DELETE_DISPATCH_CASES: readonly EngineDeleteDispatchCase[] = [ { what: 'scalar string id', options: { where: { id: 'rec_1' } }, expect: 'by-id' }, { what: 'scalar number id', options: { where: { id: 42 } }, expect: 'by-id' }, - { what: 'scalar id alongside other predicates', options: { where: { id: 'rec_1', tenant: 't1' } }, expect: 'by-id' }, + // [#11009] A PURE-id `where` stays by-id even under a declared `multi` — + // there is no predicate the by-id path could drop, and LifecycleService's + // guarded reap relies on this shape for per-record cascade handling + // (`engine-data-events.test.ts` pins the event contract of the same shape). + { what: 'scalar where.id with multi:true and NOTHING else in where — still one by-id delete (#11009)', options: { where: { id: 'rec_1' }, multi: true }, expect: 'by-id' }, { what: 'multi with a predicate', options: { where: { rule_id: 'r1' }, multi: true }, expect: 'multi' }, { what: 'multi with no predicate at all', options: { multi: true }, expect: 'multi' }, { what: 'multi alongside an $in id set', options: { where: { id: { $in: ['a', 'b'] } }, multi: true }, expect: 'multi' }, + // [#11009] The compare-and-set spelling: a scalar `where.id` beside real + // predicate keys WITH a declared `multi` is a predicate call — every key + // rides the AST to `driver.deleteMany`, so the condition is honoured. + { what: 'scalar where.id + extra predicate keys + multi:true — the predicate path honours ALL of it (#11009)', options: { where: { id: 'rec_1', status: 'stale' }, multi: true }, expect: 'multi' }, // ── The FALSY scalars (objectstack#5747). `0` and `''` are scalars, so // `scalarDeleteId` returns them — but the engine's `if (input.id)` is a // truthiness test, so neither identifies a row. With a declared bulk @@ -253,4 +303,10 @@ export const ENGINE_DELETE_DISPATCH_CASES: readonly EngineDeleteDispatchCase[] = { what: 'empty where, no multi', options: { where: {} }, expect: 'reject' }, { what: 'no options at all', options: undefined, expect: 'reject' }, { what: 'multi explicitly false with a predicate', options: { where: { rule_id: 'r1' }, multi: false }, expect: 'reject' }, + // ── [#11009] The unhonoured-predicate refusals — the delete twin of the + // update-side cases. Each used to dispatch `by-id` and silently DISCARD + // every `where` key other than `id`; now the refusal names the dropped + // keys and prescribes the predicate path (`multi: true`). + { what: 'scalar where.id alongside other predicates, NO multi — the guard would be silently dropped (#11009)', options: { where: { id: 'rec_1', tenant: 't1' } }, expect: 'reject' }, + { what: 'scalar where.id + a CAS operator predicate, multi explicitly false (#11009)', options: { where: { id: 'rec_1', status: { $in: ['done'] } }, multi: false }, expect: 'reject' }, ]; diff --git a/packages/metadata-core/src/engine-dispatch-unhonoured-predicate.ts b/packages/metadata-core/src/engine-dispatch-unhonoured-predicate.ts new file mode 100644 index 0000000000..b780e33cfd --- /dev/null +++ b/packages/metadata-core/src/engine-dispatch-unhonoured-predicate.ts @@ -0,0 +1,77 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The shared half of the objectstack#11009 refusal, imported by BOTH write + * dispatch modules (`engine-update-dispatch.ts` / `engine-delete-dispatch.ts`) + * so the two verbs cannot drift apart on what an unhonoured by-id predicate + * is, or on the words that refuse one. + * + * ## The defect this refuses + * + * A by-id dispatch routes to `driver.update(object, id, …)` / + * `driver.delete(object, id, …)` — entry points that bind ONLY the primary + * key (plus tenant scope). Every other key the caller wrote into + * `options.where` is silently discarded there: a compare-and-set guard + * (`{ where: { id, status: { $in: [...] } } }`) evaluates to nothing, the + * write lands unconditionally, and nothing reports that the declared + * condition never ran. Measured on `better-sqlite3` through a real + * `ObjectQL` + `SqlDriver` (objectstack#11009); the concrete victim was + * `SqlHttpOutbox.redeliver`'s terminal-status guard, which this repo's + * dispatcher could race and overwrite mid-flight while `redeliver` reported + * success. + * + * The refusal is the same answer this dispatch family has ruled twice before + * (objectstack#5748's operator-object `data.id`, objectstack#6435), and the + * same one `ENGINE_UPDATE_OPTION_KEYS` gives unknown option keys (#4371): a + * declaration the engine will not honour is refused at authoring time, never + * evaluated to nothing. + * + * ## What is NOT refused + * + * - A pure primary-key `where` (`{ where: { id } }`), with or without + * `multi` — the by-id path honours it in full. `multi: true` beside a + * pure-id `where` stays a BY-ID call: `LifecycleService`'s guarded reap + * relies on exactly that shape for per-record cascade handling, and + * `engine-data-events.test.ts` pins it. + * - A `where` carrying extra keys WITH a declared `multi: true` (id sourced + * from `where`) — that is a real predicate call, routed to the predicate + * path (`driver.updateMany` / `driver.deleteMany`), where `applyFilters` + * compiles EVERY key. This is the compare-and-set spelling the refusal + * message prescribes. + */ + +/** + * The `options.where` keys a by-id dispatch would silently discard: every own + * enumerable key other than `id`. + * + * `null`-valued keys COUNT, deliberately — unlike the options bag (where + * `null` is a withdrawal, #4371), a `null` inside `where` is a real predicate + * (`status IS NULL`), so dropping it loses declared intent. + */ +export function unhonouredByIdPredicateKeys(where: unknown): string[] { + if (!where || typeof where !== 'object') return []; + return Object.keys(where as Record).filter((k) => k !== 'id'); +} + +/** + * The message a by-id dispatch carrying unhonoured predicate keys is refused + * with — one composer for both verbs, and deliberately blind to WHICH source + * supplied the id (`data.id` or `where.id`): the #5748 symmetry property says + * the same call shape verdicts alike through either door, so the words must + * too. + */ +export function engineByIdUnhonouredPredicateMessage( + verb: 'Update' | 'Delete', + keys: readonly string[], +): string { + const list = keys.map((k) => `'${k}'`).join(', '); + return ( + `${verb} names one row by primary key, but options.where also carries ` + + `${keys.length > 1 ? 'predicate keys' : 'the predicate key'} ${list}. The by-id path binds ONLY ` + + `the id — the driver never evaluates the remaining predicate — so the call would succeed with ` + + `the declared condition silently ignored (#11009). For a conditional (compare-and-set) write, ` + + `declare the predicate path, which honours EVERY where key: { where: { id, ${keys.join(', ')} }, ` + + `multi: true } writes at most the one row matching ALL predicates and reports the matched count. ` + + `For an unconditional single-row write, drop the extra where keys.` + ); +} diff --git a/packages/metadata-core/src/engine-update-dispatch.ts b/packages/metadata-core/src/engine-update-dispatch.ts index 5284f06966..0eeba2ee9e 100644 --- a/packages/metadata-core/src/engine-update-dispatch.ts +++ b/packages/metadata-core/src/engine-update-dispatch.ts @@ -54,6 +54,22 @@ * - otherwise → **`reject`**. The call names neither one row nor a bulk * intent, and the engine throws rather than rewriting every row it can see. * + * **[#11009] One overriding clause on the two by-id arms:** the by-id route + * binds ONLY the primary key, so a `where` carrying any key besides `id` is a + * predicate the by-id path would silently discard. Such a call is never + * dispatched `by-id`: + * + * - id sourced from `where`, `multi` truthy → **`multi`** — a declared + * predicate call; the full `where` (scalar id included, as an equality + * term) rides the AST to `driver.updateMany`. The compare-and-set spelling. + * - anything else (no `multi`; or the id came from `data.id`, which outranks + * `multi` per #5748 and cannot be demoted onto the predicate path) → + * **`reject`**, naming the keys that would have been dropped. + * + * A PURE-id `where` (`{ where: { id } }`) is untouched by this clause: it + * stays `by-id` even beside `multi: true` (LifecycleService's guarded-reap + * idiom — pinned in the case-set below). + * * Three things about that list are load-bearing and easy to get wrong when * copying it by hand — which is the whole argument for importing it instead: * @@ -102,6 +118,11 @@ * @see scripts/check-engine-double-contract.mjs — the gate that keeps doubles on both. */ +import { + engineByIdUnhonouredPredicateMessage, + unhonouredByIdPredicateKeys, +} from './engine-dispatch-unhonoured-predicate.js'; + /** The message `update()` throws when a call identifies neither one row nor a bulk intent. */ export const ENGINE_UPDATE_REJECT_MESSAGE = 'Update requires an ID or options.multi=true'; @@ -195,14 +216,43 @@ export function resolveEngineUpdateDispatch( // parked in the payload is not an id and does not shadow the ladder below // it. `data.id` stays UNGUARDED so a missing payload is the producer's // `TypeError`, not a kinder verdict. - let id: unknown = asScalarId(data.id); + const payloadId = asScalarId(data.id); + let id: unknown = payloadId; if (!id) { const fromWhere = scalarUpdateId(options); if (fromWhere !== undefined) id = fromWhere; } // The engine branches on `if (hookContext.input.id)` — truthiness, so a // falsy scalar id is not an identifying call. See header point 3. - if (id) return { kind: 'by-id', id }; + if (id) { + // [#11009] A `where` carrying more than the primary key is a REAL + // predicate, and the by-id path does not evaluate predicates — it binds + // only the id, so every extra key used to be silently discarded (a + // compare-and-set guard that evaluated to nothing). Two honest verdicts + // replace that silent drop: + // + // - the caller ALSO declared `multi: true`, and the id came from `where` + // → `multi`: a declared predicate call, routed to the predicate path + // where `applyFilters` compiles every key (a scalar `where.id` is an + // ordinary equality term there). This is the compare-and-set spelling. + // ⚠️ A PURE-id `where` (`{ where: { id } }`) never reaches this branch + // and stays by-id even under `multi: true` — LifecycleService's guarded + // reap depends on that for per-record cascade handling. + // - otherwise → `reject`, loudly naming the keys the by-id path would + // have dropped. A truthy scalar `data.id` lands here even with + // `multi: true`: the payload id outranks `multi` (#5748), and silently + // demoting the row address into a bulk write would be the same class + // of dropped declaration this refusal exists to prevent. + const unhonoured = unhonouredByIdPredicateKeys(options?.where); + if (unhonoured.length > 0) { + if (!payloadId && options?.multi) return { kind: 'multi' }; + return { + kind: 'reject', + message: engineByIdUnhonouredPredicateMessage('Update', unhonoured), + }; + } + return { kind: 'by-id', id }; + } if (options?.multi) return { kind: 'multi' }; return { kind: 'reject', message: ENGINE_UPDATE_REJECT_MESSAGE }; } @@ -268,12 +318,15 @@ export const ENGINE_UPDATE_DISPATCH_CASES: readonly EngineUpdateDispatchCase[] = // ── by-id via `where`. { what: 'scalar string where.id', data: { title: 'x' }, options: { where: { id: 'rec_1' } }, expect: 'by-id' }, { what: 'scalar number where.id', data: { title: 'x' }, options: { where: { id: 42 } }, expect: 'by-id' }, - { what: 'scalar where.id alongside other predicates', data: { title: 'x' }, options: { where: { id: 'rec_1', tenant: 't1' } }, expect: 'by-id' }, + // [#11009] A PURE-id `where` stays by-id even under a declared `multi` — + // there is no predicate the by-id path could drop, and LifecycleService's + // guarded reap relies on this shape taking the per-record path. + { what: 'scalar where.id with multi:true and NOTHING else in where — still one by-id write (#11009)', data: { title: 'x' }, options: { where: { id: 'rec_1' }, multi: true }, expect: 'by-id', expectId: 'rec_1' }, // ── by-id via the PAYLOAD. A SCALAR `data.id` still outranks `where` and // `multi` alike — that is the common, legal `update(o, { id, …fields })` // spelling and objectstack#5748 left it exactly as it was. { what: 'id carried in the data payload, no where at all', data: { id: 'rec_1', title: 'x' }, options: undefined, expect: 'by-id', expectId: 'rec_1' }, - { what: 'a SCALAR data.id still wins over an explicit multi:true', data: { id: 'rec_1', title: 'x' }, options: { where: { tenant: 't1' }, multi: true }, expect: 'by-id', expectId: 'rec_1' }, + { what: 'a SCALAR data.id still wins over an explicit multi:true', data: { id: 'rec_1', title: 'x' }, options: { multi: true }, expect: 'by-id', expectId: 'rec_1' }, { what: 'a SCALAR data.id still wins over a scalar where.id', data: { id: 'rec_1', title: 'x' }, options: { where: { id: 'rec_2' } }, expect: 'by-id', expectId: 'rec_1' }, // ── The payload's scalar test (objectstack#5748). A non-scalar `data.id` // names no row, so it stops shadowing everything under it: the decision @@ -285,6 +338,11 @@ export const ENGINE_UPDATE_DISPATCH_CASES: readonly EngineUpdateDispatchCase[] = { what: 'multi with a predicate', data: { title: 'x' }, options: { where: { tenant: 't1' }, multi: true }, expect: 'multi' }, { what: 'multi with no predicate at all', data: { title: 'x' }, options: { multi: true }, expect: 'multi' }, { what: 'multi alongside an $in id set', data: { title: 'x' }, options: { where: { id: { $in: ['a', 'b'] } }, multi: true }, expect: 'multi' }, + // [#11009] The compare-and-set spelling: a scalar `where.id` beside real + // predicate keys WITH a declared `multi` is a predicate call — every key + // (the id included, as an equality term) rides the AST to + // `driver.updateMany`, so the declared condition is honoured in full. + { what: 'scalar where.id + extra predicate keys + multi:true — the predicate path honours ALL of it (#11009)', data: { title: 'x' }, options: { where: { id: 'rec_1', status: 'draft' }, multi: true }, expect: 'multi' }, { what: 'multi with a FALSY data.id (0 does not identify a row)', data: { id: 0, title: 'x' }, options: { multi: true }, expect: 'multi' }, { what: 'operator object in data.id WITH multi:true — the declared bulk intent is honoured (#5748)', data: { id: { $in: ['a', 'b'] }, title: 'x' }, options: { multi: true }, expect: 'multi' }, { what: 'array data.id with multi:true', data: { id: ['a', 'b'], title: 'x' }, options: { multi: true }, expect: 'multi' }, @@ -305,4 +363,18 @@ export const ENGINE_UPDATE_DISPATCH_CASES: readonly EngineUpdateDispatchCase[] = { what: 'operator object in data.id, multi explicitly false', data: { id: { $in: ['a', 'b'] }, title: 'x' }, options: { multi: false }, expect: 'reject' }, { what: 'array data.id, no multi', data: { id: ['a', 'b'], title: 'x' }, options: undefined, expect: 'reject' }, { what: 'null data.id, no multi', data: { id: null, title: 'x' }, options: undefined, expect: 'reject' }, + // ── [#11009] The unhonoured-predicate refusals. Each of these used to + // dispatch `by-id` and silently DISCARD every `where` key other than + // `id` — a compare-and-set guard that evaluated to nothing, reading + // exactly like a working conditional write. Now they are loud: the + // refusal names the dropped keys and prescribes the predicate path + // (`multi: true`), which honours the full `where`. + { what: 'scalar where.id alongside other predicates, NO multi — the guard would be silently dropped (#11009)', data: { title: 'x' }, options: { where: { id: 'rec_1', tenant: 't1' } }, expect: 'reject' }, + { what: 'scalar where.id + a CAS operator predicate, multi explicitly false (#11009 — the redeliver shape)', data: { title: 'x' }, options: { where: { id: 'rec_1', status: { $in: ['done'] } }, multi: false }, expect: 'reject' }, + { what: 'scalar data.id + extra where predicate, no multi — same drop through the payload door (#11009)', data: { id: 'rec_1', title: 'x' }, options: { where: { tenant: 't1' } }, expect: 'reject' }, + // The payload id outranks `multi` (#5748), so a declared `multi: true` + // cannot re-route it onto the predicate path — and the unhonourable + // predicate is REFUSED rather than silently dropped (the pre-#11009 + // behaviour) or silently promoted to a bulk write. + { what: 'scalar data.id + extra where predicate + multi:true — refused, the payload id cannot take the predicate path (#11009)', data: { id: 'rec_1', title: 'x' }, options: { where: { tenant: 't1' }, multi: true }, expect: 'reject' }, ]; diff --git a/packages/metadata-core/src/index.ts b/packages/metadata-core/src/index.ts index 6735fd2a68..78e7a5be8d 100644 --- a/packages/metadata-core/src/index.ts +++ b/packages/metadata-core/src/index.ts @@ -26,6 +26,9 @@ export * from './objects/index.js'; // See `scripts/check-engine-double-contract.mjs` — the gate over the doubles. export * from './engine-delete-dispatch.js'; export * from './engine-update-dispatch.js'; +// [#11009] The refusal both write dispatches share: a by-id call whose +// `where` carries keys the by-id path would silently discard. +export * from './engine-dispatch-unhonoured-predicate.js'; // [#4513] The audit-family GOVERNANCE table (#4447) and its normalizer, sunk // here for the same reason and by the same criterion as the two dispatch diff --git a/packages/objectql/src/engine-delete-dispatch.test.ts b/packages/objectql/src/engine-delete-dispatch.test.ts index ee68f6d565..893dac9329 100644 --- a/packages/objectql/src/engine-delete-dispatch.test.ts +++ b/packages/objectql/src/engine-delete-dispatch.test.ts @@ -25,6 +25,7 @@ import { assertEngineDeleteDispatch, scalarDeleteId, } from './engine-delete-dispatch.js'; +import { engineByIdUnhonouredPredicateMessage } from './engine-update-dispatch.js'; /** Records which driver entry point the engine chose, if any. */ function makeRecordingDriver() { @@ -71,10 +72,21 @@ async function makeEngine() { /** What the real engine actually did with this options bag. */ async function observeEngine(options: unknown): Promise<'by-id' | 'multi' | 'reject'> { const { engine, calls } = await makeEngine(); + // [#11009] `reject` no longer has one spelling — the unhonoured-predicate + // refusal composes its message from the dropped keys, so a throw counts as + // the dispatch's verdict only when byte-identical to what the predicate + // says THIS call refuses with. Anything else still rethrows. + const predicted = resolveEngineDeleteDispatch(options as Parameters[0]); try { await engine.delete('task', options as any); } catch (e) { - if ((e as Error).message === ENGINE_DELETE_REJECT_MESSAGE) return 'reject'; + const message = (e as Error).message; + if ( + message === ENGINE_DELETE_REJECT_MESSAGE || + (predicted.kind === 'reject' && message === predicted.message) + ) { + return 'reject'; + } throw e; } if (calls.length !== 1) { @@ -139,6 +151,38 @@ describe('engine delete dispatch — the shared predicate IS the engine (#4550)' expect(assertEngineDeleteDispatch({ where: { id: 0 }, multi: true })).toEqual({ kind: 'multi' }); }); + // ── [#11009] The unhonoured-predicate refusal — the delete twin of the + // update-side pins, and deliberately the SAME shared message composer, so + // a reader can see the two verbs refuse symmetrically. + it('a by-id delete carrying where keys beyond id is refused with the shared message', async () => { + const message = engineByIdUnhonouredPredicateMessage('Delete', ['status']); + expect(resolveEngineDeleteDispatch({ where: { id: 'p1', status: { $in: ['done'] } } })) + .toEqual({ kind: 'reject', message }); + expect(() => assertEngineDeleteDispatch({ where: { id: 'p1', status: { $in: ['done'] } } })) + .toThrow(message); + // With a DECLARED multi the same call is a predicate delete — every where + // key (the scalar id included) rides the AST to `driver.deleteMany`. + const { engine, calls } = await makeEngine(); + await engine.delete('task', { where: { id: 'p1', status: 'stale' }, multi: true } as any); + expect(calls).toHaveLength(1); + expect(calls[0].fn).toBe('deleteMany'); + expect(calls[0].arg).toMatchObject({ object: 'task', where: { id: 'p1', status: 'stale' } }); + }); + + it('a PURE-id where stays by-id even under multi:true — the LifecycleService reap idiom (#11009)', async () => { + // No predicate exists for the by-id path to drop, and the guarded reap + // depends on this shape taking the per-record path (cascade handling, + // per-record events — pinned from the event side in + // `engine-data-events.test.ts`). + expect(resolveEngineDeleteDispatch({ where: { id: 'rec_1' }, multi: true })) + .toEqual({ kind: 'by-id', id: 'rec_1' }); + const { engine, calls } = await makeEngine(); + await engine.delete('task', { where: { id: 'rec_1' }, multi: true } as any); + expect(calls).toHaveLength(1); + expect(calls[0].fn).toBe('delete'); + expect(calls[0].arg).toBe('rec_1'); + }); + it('a falsy-id `multi` delete is still SCOPED by the caller where, not a whole-table purge', () => { // `multi` is the honest verdict for `{ where: { id: 0 }, multi: true }`, // and the reason it is safe is that the caller's own predicate still diff --git a/packages/objectql/src/engine-update-dispatch.test.ts b/packages/objectql/src/engine-update-dispatch.test.ts index 5b2a5fbec1..8eab7a7f30 100644 --- a/packages/objectql/src/engine-update-dispatch.test.ts +++ b/packages/objectql/src/engine-update-dispatch.test.ts @@ -24,6 +24,8 @@ import { resolveEngineUpdateDispatch, assertEngineUpdateDispatch, scalarUpdateId, + engineByIdUnhonouredPredicateMessage, + unhonouredByIdPredicateKeys, } from './engine-update-dispatch.js'; /** Records which driver entry point the engine chose, if any. */ @@ -85,12 +87,28 @@ async function makeEngine() { async function observeEngine( data: unknown, options: unknown, -): Promise<{ kind: 'by-id' | 'multi' | 'reject'; boundId?: unknown }> { +): Promise<{ kind: 'by-id' | 'multi' | 'reject'; boundId?: unknown; message?: string }> { const { engine, calls } = await makeEngine(); + // [#11009] `reject` no longer has ONE spelling: the unhonoured-predicate + // refusal composes its message from the dropped keys. A throw counts as the + // engine's `reject` verdict only when it is byte-identical to what the + // PREDICATE says this exact call refuses with — anything else (validation, + // not-found, a driver error) still rethrows, so no unrelated failure can + // impersonate a dispatch refusal. + const predicted = resolveEngineUpdateDispatch( + data as Parameters[0], + options as Parameters[1], + ); try { await engine.update('task', data as any, options as any); } catch (e) { - if ((e as Error).message === ENGINE_UPDATE_REJECT_MESSAGE) return { kind: 'reject' }; + const message = (e as Error).message; + if ( + message === ENGINE_UPDATE_REJECT_MESSAGE || + (predicted.kind === 'reject' && message === predicted.message) + ) { + return { kind: 'reject', message }; + } throw e; } if (calls.length !== 1) { @@ -121,6 +139,13 @@ describe('engine update dispatch — the shared predicate IS the engine (#5480)' expect(predicted.kind, 'predicate').toBe(c.expect); const observed = await observeEngine(c.data, c.options); expect(observed.kind, 'real ObjectQL.update').toBe(c.expect); + if (c.expect === 'reject') { + // Both halves must refuse with the SAME words — the refusal text is + // part of the contract a pinned double reproduces (#11009). + expect(observed.message, 'real engine reject message').toBe( + (predicted as { message?: string }).message, + ); + } if ('expectId' in c) { // Both halves must bind the SAME id, and it must be the declared one — // see `EngineUpdateDispatchCase.expectId` for why the verdict alone is @@ -233,8 +258,20 @@ describe('engine update dispatch — the shared predicate IS the engine (#5480)' // The asymmetry #5748 removed, stated as the property that replaced it. // `where` carries a second key so the `where`-side call is never the // "empty where" reject for an unrelated reason. + // + // [#11009] The property survives with ONE carve-out, pinned separately + // below: a truthy scalar id beside a `multi: true` and an extra `where` + // key. There the two doors legitimately diverge — the `where`-sourced id + // yields to the declared bulk intent (predicate path), while the payload + // id outranks `multi` (#5748's own ruling) and the unhonourable predicate + // is refused. Every OTHER (value × rest) pair still verdicts alike, and + // the shared refusal message is deliberately source-blind so even the + // rejects agree word for word. for (const value of [{ $in: ['a', 'b'] }, { $ne: 'a' }, ['a', 'b'], null, 'rec_1', 42, 0, '']) { - for (const rest of [{}, { multi: true }]) { + for (const rest of [{}, { multi: true }] as Array>) { + const isDivergentPair = + rest.multi === true && (typeof value === 'string' || typeof value === 'number') && Boolean(value); + if (isDivergentPair) continue; const viaPayload = resolveEngineUpdateDispatch({ id: value, title: 'x' }, { where: { tenant: 't1' }, ...rest }); const viaWhere = resolveEngineUpdateDispatch({ title: 'x' }, { where: { tenant: 't1', id: value }, ...rest }); expect(viaPayload, `${JSON.stringify({ value, rest })}`).toEqual(viaWhere); @@ -242,6 +279,38 @@ describe('engine update dispatch — the shared predicate IS the engine (#5480)' } }); + // ── [#11009] The carve-out from the symmetry loop above, stated positively. + it('truthy scalar id + extra where key + multi:true — where.id takes the predicate path, data.id refuses', () => { + // The `where` door: the caller declared a bulk/predicate intent and the id + // is PART of the predicate — every key (id included) rides the AST. + expect(resolveEngineUpdateDispatch({ title: 'x' }, { where: { tenant: 't1', id: 'rec_1' }, multi: true })) + .toEqual({ kind: 'multi' }); + // The payload door: `data.id` outranks `multi` (#5748), so the call is a + // single-row write whose extra predicate CANNOT be honoured — refused + // loudly, never silently dropped (pre-#11009) or silently bulk-promoted. + expect(resolveEngineUpdateDispatch({ id: 'rec_1', title: 'x' }, { where: { tenant: 't1' }, multi: true })) + .toEqual({ kind: 'reject', message: engineByIdUnhonouredPredicateMessage('Update', ['tenant']) }); + }); + + // ── [#11009] The refusal itself, quoted and bounded. + it('a by-id update carrying where keys beyond id is refused with the shared message', async () => { + const message = engineByIdUnhonouredPredicateMessage('Update', ['status']); + expect(resolveEngineUpdateDispatch({ title: 'x' }, { where: { id: 'p1', status: { $in: ['done'] } } })) + .toEqual({ kind: 'reject', message }); + expect(() => assertEngineUpdateDispatch({ title: 'x' }, { where: { id: 'p1', status: { $in: ['done'] } } })) + .toThrow(message); + // …and nothing reached the driver: a refused compare-and-set writes NOTHING, + // which is the entire point — the old behaviour wrote unconditionally. + expect(await observeDriverCalls({ title: 'x' }, { where: { id: 'p1', status: { $in: ['done'] } } })).toEqual([]); + // A `null`-valued extra key is a REAL predicate (`IS NULL`), not a + // withdrawal — it refuses too. + expect(resolveEngineUpdateDispatch({ title: 'x' }, { where: { id: 'p1', error: null } }).kind).toBe('reject'); + // The keys helper reports exactly the droppable keys, `id` excluded. + expect(unhonouredByIdPredicateKeys({ id: 'p1', status: 'done', error: null })).toEqual(['status', 'error']); + expect(unhonouredByIdPredicateKeys({ id: 'p1' })).toEqual([]); + expect(unhonouredByIdPredicateKeys(undefined)).toEqual([]); + }); + it('branches on TRUTHINESS, so a falsy scalar id does not identify a row', () => { expect(resolveEngineUpdateDispatch({ title: 'x' }, { where: { id: 0 } }).kind).toBe('reject'); expect(resolveEngineUpdateDispatch({ title: 'x' }, { where: { id: '' } }).kind).toBe('reject'); diff --git a/packages/objectql/src/engine-update-dispatch.ts b/packages/objectql/src/engine-update-dispatch.ts index d3fd11562d..ed012397df 100644 --- a/packages/objectql/src/engine-update-dispatch.ts +++ b/packages/objectql/src/engine-update-dispatch.ts @@ -21,6 +21,11 @@ export { resolveEngineUpdateDispatch, assertEngineUpdateDispatch, ENGINE_UPDATE_DISPATCH_CASES, + // [#11009] The unhonoured-by-id-predicate refusal, shared by BOTH write + // dispatches (the delete twin imports the same two symbols); re-exported + // once, here, so `objectql` callers can quote the refusal verbatim. + engineByIdUnhonouredPredicateMessage, + unhonouredByIdPredicateKeys, } from '@objectstack/metadata-core'; export type { diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 40da9584dd..782ebf7414 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -9537,7 +9537,14 @@ export class ObjectQL implements IObjectQLEngine { // Keyed on the SAME falsy-`id` test the #2982 AST seed above uses, so // seed, ladder and branch cannot disagree. const isByIdWrite = Boolean(id); - const isPredicatePath = !isByIdWrite && Boolean(options?.multi) && typeof driver.updateMany === 'function'; + // [#11009] Keyed on the LADDER's verdict, not on `options?.multi`: the + // dispatch can now return `reject` for a call that carries a truthy + // `multi` (a scalar `data.id` beside an unhonourable `where` + // predicate), and branching on the raw flag would silently promote + // that refusal into a bulk write. For every pre-#11009 input the two + // spellings agree (`kind === 'multi'` ⇔ `options.multi` truthy when + // not by-id), so this narrows nothing else. + const isPredicatePath = !isByIdWrite && dispatch.kind === 'multi' && typeof driver.updateMany === 'function'; if (!isByIdWrite && !isPredicatePath) { // [#5480] The `reject` verdict of resolveEngineUpdateDispatch. It // used to be re-asked AFTER the before phase, because a hook could @@ -9545,7 +9552,15 @@ export class ObjectQL implements IObjectQLEngine { // the ladder resolved first there is no such conversion, so the // refusal lands where it costs least — before any handler runs and // before anything is read. - throw new Error(ENGINE_UPDATE_REJECT_MESSAGE); + // + // [#11009] The dispatch's OWN message: the unhonoured-predicate + // refusal names the `where` keys the by-id path would have + // dropped, while the classic no-id-no-multi shape keeps + // `ENGINE_UPDATE_REJECT_MESSAGE` verbatim. The fallback arm covers + // the one non-reject way in here — a `multi` verdict against a + // driver with no `updateMany`, which stays the generic refusal it + // has always been. + throw new Error(dispatch.kind === 'reject' ? dispatch.message : ENGINE_UPDATE_REJECT_MESSAGE); } // [#6966] The ladder verdict, stated on the contract. Bound HERE and @@ -11159,12 +11174,20 @@ export class ObjectQL implements IObjectQLEngine { // update()'s twin for the full reasoning. Keyed on the SAME falsy-`id` // test the #2982 AST seed above uses. const isByIdDelete = Boolean(id); - const isPredicatePath = !isByIdDelete && Boolean(options?.multi) && typeof driver.deleteMany === 'function'; + // [#11009] Keyed on the LADDER's verdict, not on `options?.multi` — the + // update twin says why; for every pre-#11009 input the two spellings + // agree. + const isPredicatePath = !isByIdDelete && dispatch.kind === 'multi' && typeof driver.deleteMany === 'function'; if (!isByIdDelete && !isPredicatePath) { // [#4550] The `reject` verdict of resolveEngineDeleteDispatch. It used // to be re-asked after the before phase because a hook could still bind // the id; with the ladder resolved first there is no such conversion. - throw new Error(ENGINE_DELETE_REJECT_MESSAGE); + // + // [#11009] The dispatch's OWN message — the unhonoured-predicate + // refusal names the dropped `where` keys; the classic shape keeps + // `ENGINE_DELETE_REJECT_MESSAGE`; the fallback arm is a `multi` + // verdict against a driver with no `deleteMany`. + throw new Error(dispatch.kind === 'reject' ? dispatch.message : ENGINE_DELETE_REJECT_MESSAGE); } // [#6966] See update()'s twin — same rule, same single binding point. diff --git a/packages/objectql/src/index.ts b/packages/objectql/src/index.ts index cdce3e2951..a76adadd18 100644 --- a/packages/objectql/src/index.ts +++ b/packages/objectql/src/index.ts @@ -123,6 +123,9 @@ export { scalarUpdateId, ENGINE_UPDATE_REJECT_MESSAGE, ENGINE_UPDATE_DISPATCH_CASES, + // [#11009] Shared by both write dispatches — see the shim's note. + engineByIdUnhonouredPredicateMessage, + unhonouredByIdPredicateKeys, } from './engine-update-dispatch.js'; export type { EngineUpdateDispatch, diff --git a/packages/services/service-messaging/src/delivery-update-tenant-audit.integration.test.ts b/packages/services/service-messaging/src/delivery-update-tenant-audit.integration.test.ts index 14d1ed1050..0a2fa910f1 100644 --- a/packages/services/service-messaging/src/delivery-update-tenant-audit.integration.test.ts +++ b/packages/services/service-messaging/src/delivery-update-tenant-audit.integration.test.ts @@ -11,6 +11,13 @@ * | `SqlHttpOutbox.ack` | dispatcher tick | global sweep | * | `SqlHttpOutbox.redeliver` | POST /api/v1/… | request-contextual | * + * [#11009] `redeliver`'s reset now rides the PREDICATE path (`multi: true`, + * `driver.updateMany`) so its terminal-status compare-and-set is actually + * evaluated — the by-id path silently discarded it. Its audit op is therefore + * `updateMany`, and its spy below records `SqlDriver.updateMany`; the + * tenant-classification contract this file pins (#10740) is unchanged — + * threaded `tenantId`, never `bypassTenantAudit`. + * * The `ack` pair is declared global (`dispatcherAckOptions`, warrant in * `outbox-dispatcher-scope.ts`). `redeliver` is NOT: it is served to any * authenticated user, so it threads the caller's tenant instead. ⛔ A @@ -67,11 +74,17 @@ let driver: SqlDriver; let warns: Array<{ msg: string; meta: any }>; /** Every `options` bag that reached `SqlDriver.update` — the `update` op only. */ let driverUpdates: Array<{ object: string; id: unknown; options: any }>; +/** Every `options` bag that reached `SqlDriver.updateMany` — `redeliver`'s op since #11009. */ +let driverUpdateManys: Array<{ object: string; where: unknown; options: any }>; /** The audit line for the SINGLE-RECORD op, matched on object + op. */ const auditedUpdate = (object: string): boolean => warns.some((w) => w.msg.includes(`[tenant-audit] update on tenant-scoped object "${object}"`)); +/** The audit line for the PREDICATE op — `redeliver`'s write since #11009. */ +const auditedUpdateMany = (object: string): boolean => + warns.some((w) => w.msg.includes(`[tenant-audit] updateMany on tenant-scoped object "${object}"`)); + beforeEach(async () => { // Read LIVE by `isMultiTenantMode()` (#5262), so this really arms the gate. process.env.OS_TENANCY_POSTURE = 'isolated'; @@ -84,6 +97,7 @@ beforeEach(async () => { }); warns = []; driverUpdates = []; + driverUpdateManys = []; (driver as any).logger = { warn: (msg: string, meta: any) => warns.push({ msg, meta }) }; // Spy on the driver's by-id UPDATE — the method that calls @@ -96,6 +110,14 @@ beforeEach(async () => { driverUpdates.push({ object, id, options }); return realUpdate(object, id, data, options); }; + // [#11009] The same reading for the predicate op: `redeliver`'s reset now + // reaches the driver through `updateMany`, and the options bag IT received + // is the only truthful record of what that write was scoped by. + const realUpdateMany = (driver as any).updateMany.bind(driver); + (driver as any).updateMany = async (object: string, query: any, data: any, options: any) => { + driverUpdateManys.push({ object, where: query?.where, options }); + return realUpdateMany(object, query, data, options); + }; engine = new ObjectQL(); engine.registerDriver(driver, true); @@ -137,6 +159,23 @@ async function controlUnscopedUpdate(object: string, existingId: string): Promis ).toBe(true); } +/** + * The positive control for the `updateMany` op — `redeliver`'s op since + * #11009. An unscoped predicate write with no bypass MUST produce the + * `updateMany` audit line, or the silence assertions on that op are vacuous. + * Run AFTER the assertion it guards (one warning per object+op key). + */ +async function controlUnscopedUpdateMany(object: string, existingId: string): Promise { + // Scalar id + a second predicate key + multi — the #11009 predicate-path + // spelling, exactly the shape `redeliver` writes. + await engine.update(object, { attempts: 98 }, { where: { id: existingId, attempts: { $gte: 0 } }, multi: true } as any); + expect( + auditedUpdateMany(object), + `positive control failed: an unscoped predicate update on ${object} produced no ` + + '[tenant-audit] updateMany line, so the silence assertions on that op are vacuous', + ).toBe(true); +} + function okFetch(): { impl: FetchImpl; calls: string[] } { const calls: string[] = []; const impl: FetchImpl = async (url) => { @@ -264,8 +303,13 @@ describe('redeliver — the request-reachable site is SCOPED, never bypassed', ( const replayed = await outbox.redeliver('h_a', { tenantId: 'org_a' }); expect(replayed.status).toBe('pending'); - const writes = driverUpdates.filter((u) => u.object === SYS_HTTP_DELIVERY); + // [#11009] The reset rides the predicate path now, so the truthful + // record of its scoping is the options `SqlDriver.updateMany` + // received — and the write must carry its full compare-and-set + // predicate, id AND terminal status. + const writes = driverUpdateManys.filter((u) => u.object === SYS_HTTP_DELIVERY); expect(writes).toHaveLength(1); + expect(writes[0].where).toMatchObject({ id: 'h_a', status: { $in: ['success', 'failed', 'dead'] } }); // ⛔ The forbidden implementation, named: a bypass here would silence // the audit for an authenticated user's unscoped write. expect(writes[0].options?.bypassTenantAudit).toBeUndefined(); @@ -273,9 +317,9 @@ describe('redeliver — the request-reachable site is SCOPED, never bypassed', ( expect(writes[0].options?.tenantId).toBe('org_a'); // The line is absent BECAUSE the write is scoped — the two assertions // above are what make this one mean something. - expect(auditedUpdate(SYS_HTTP_DELIVERY)).toBe(false); + expect(auditedUpdateMany(SYS_HTTP_DELIVERY)).toBe(false); - await controlUnscopedUpdate(SYS_HTTP_DELIVERY, 'h_a'); + await controlUnscopedUpdateMany(SYS_HTTP_DELIVERY, 'h_a'); }); it('refuses a cross-tenant redeliver — the row is not found, not merely forbidden', async () => { @@ -294,6 +338,7 @@ describe('redeliver — the request-reachable site is SCOPED, never bypassed', ( const [row] = (await engine.find(SYS_HTTP_DELIVERY, { where: {} })) as any[]; expect(`${row.status}:${row.attempts}`).toBe('dead:3'); expect(driverUpdates.filter((u) => u.object === SYS_HTTP_DELIVERY)).toHaveLength(0); + expect(driverUpdateManys.filter((u) => u.object === SYS_HTTP_DELIVERY)).toHaveLength(0); }); it('still works: an in-tenant redeliver succeeds while a foreign one does not', async () => { @@ -324,9 +369,13 @@ describe('redeliver — the request-reachable site is SCOPED, never bypassed', ( const outbox = new SqlHttpOutbox(engine as any, { partitionCount: 1 }); await outbox.redeliver('h_a', { tenantId: undefined }); - const writes = driverUpdates.filter((u) => u.object === SYS_HTTP_DELIVERY); + // [#11009] `redeliver`'s op is `updateMany` now; the audit line moves + // with it. The dispatcher sweeps on this object carry + // `bypassTenantAudit` and return before the throttle, so they cannot + // have consumed this op's one warning. + const writes = driverUpdateManys.filter((u) => u.object === SYS_HTTP_DELIVERY); expect(writes).toHaveLength(1); expect(writes[0].options?.bypassTenantAudit).toBeUndefined(); - expect(auditedUpdate(SYS_HTTP_DELIVERY)).toBe(true); + expect(auditedUpdateMany(SYS_HTTP_DELIVERY)).toBe(true); }); }); diff --git a/packages/services/service-messaging/src/memory-http-outbox.ts b/packages/services/service-messaging/src/memory-http-outbox.ts index 1b3a0d2106..6a8c9737bc 100644 --- a/packages/services/service-messaging/src/memory-http-outbox.ts +++ b/packages/services/service-messaging/src/memory-http-outbox.ts @@ -176,6 +176,20 @@ export class MemoryHttpOutbox implements IHttpOutbox { // [#8069] Refuse BEFORE any mutation — a refused redelivery must leave // the row byte-identical, including its `dead` status and its reason. await assertRedeliverAllowed({ ...row }, options.guard); + // [#11009] The compare-and-set half of the check-then-act, mirroring + // `SqlHttpOutbox.redeliver`'s predicate-path write: the guard above is + // awaited, so a dispatcher tick can claim this row `in_flight` between + // the read and this mutation. A row no longer terminal is NOT reset — + // the same refusal (and the same code) the SQL store reports when its + // predicate write matches zero rows. Without this, the two + // implementations of one `IHttpOutbox.redeliver` contract would + // disagree on exactly the race the contract exists to close. + if (row.status !== 'success' && row.status !== 'failed' && row.status !== 'dead') { + throw new HttpRedeliverError( + `Delivery row '${id}' state changed during redeliver`, + 'DELIVERY_NOT_ELIGIBLE', + ); + } const now = Date.now(); row.status = 'pending'; row.attempts = 0; diff --git a/packages/services/service-messaging/src/outbox-dispatcher-scope.ts b/packages/services/service-messaging/src/outbox-dispatcher-scope.ts index c788d780af..ca21e8788c 100644 --- a/packages/services/service-messaging/src/outbox-dispatcher-scope.ts +++ b/packages/services/service-messaging/src/outbox-dispatcher-scope.ts @@ -54,6 +54,14 @@ import type { EngineUpdateOptions } from '@objectstack/spec/data'; * sites, and `SqlHttpOutbox.redeliver` — request-reachable — carries a * threaded tenant and no bypass at all. * + * ⛔ [#11009] `redeliver` is now ALSO a `multi: true` write (its terminal- + * status compare-and-set must ride the predicate path to be evaluated at + * all), which makes it TYPE-compatible with this helper — and still the one + * write on these objects that must never use it: this helper's warrant is + * "no request context exists", and `redeliver` is precisely the site that + * has one. `multi` stopped discriminating the two; the classification — + * threaded tenant vs declared-global bypass — is the line that still does. + * * @param where Predicate identifying the rows this sweep claims or reaps. */ export function dispatcherSweepOptions( diff --git a/packages/services/service-messaging/src/redeliver-concurrent-claim.integration.test.ts b/packages/services/service-messaging/src/redeliver-concurrent-claim.integration.test.ts new file mode 100644 index 0000000000..559e9f3deb --- /dev/null +++ b/packages/services/service-messaging/src/redeliver-concurrent-claim.integration.test.ts @@ -0,0 +1,169 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #11009 — `redeliver`'s terminal-status compare-and-set must actually hold + * against a concurrent claim. + * + * ## The measured defect this file pins the fix for + * + * `redeliver` is a check-then-act: read the row, refuse a non-terminal one, + * then reset it — re-stating the terminal requirement IN the write + * (`where: { id, status: { $in: ['success','failed','dead'] } }`) so a row + * that changed underneath is not reset. On `multi: false` that predicate + * dispatched BY-ID, and the by-id driver path binds only the primary key — + * the status guard was silently discarded. Measured on `origin/main` + * @ `95437e7d2` (better-sqlite3, real `ObjectQL` + `SqlDriver`, this exact + * harness): a row claimed `in_flight` between `redeliver`'s read and its + * write was reset to `pending, attempts: 0` anyway, and `redeliver` reported + * SUCCESS — while the in-flight attempt kept running, so the delivery could + * go out twice with the attempt counter reading 0. + * + * The fix has two halves, both asserted here: + * - the engine now REFUSES the by-id spelling outright + * (`engine-update-dispatch.ts`, #11009 — nothing is written), and + * - `redeliver` rides the predicate path (`multi: true`), whose write + * compiles EVERY `where` key — so the concurrent-claim window closes: the + * reset misses, and `redeliver` reports refusal, not success. + * + * ## How the race is made deterministic + * + * `RedeliverOptions.guard` is awaited BETWEEN the read and the write — the + * exact window the `HttpDispatcher` tick can claim a `pending` row into + * `in_flight`. The guard here performs the claim flip itself, so the test + * stands in the window rather than hoping to hit it. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { ObjectQL, engineByIdUnhonouredPredicateMessage } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { SqlHttpOutbox } from './sql-http-outbox.js'; +import { MemoryHttpOutbox } from './memory-http-outbox.js'; +import { HttpDelivery, SYS_HTTP_DELIVERY } from './objects/http-delivery.object.js'; + +let engine: ObjectQL; +let driver: SqlDriver; + +beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + engine = new ObjectQL(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(HttpDelivery as any, '@objectstack/service-messaging'); + await engine.syncSchemas(); +}); + +afterEach(async () => { + try { await engine?.destroy(); } catch { /* noop */ } +}); + +async function seedRow(id: string, over: Record = {}): Promise { + const now = new Date(); + await engine.insert(SYS_HTTP_DELIVERY, { + id, source: 'webhook', ref_id: id, dedup_key: id, + url: 'https://receiver.example/hook', method: 'POST', payload_json: '{}', + partition_key: 0, status: 'pending', attempts: 0, + created_at: now, updated_at: now, ...over, + } as any); +} + +async function readRow(id: string): Promise { + return engine.findOne(SYS_HTTP_DELIVERY, { where: { id } }); +} + +describe('#11009 — the by-id CAS spelling is refused, nothing is written', () => { + it("the issue's minimal shape now throws the unhonoured-predicate refusal instead of landing", async () => { + await seedRow('p1', { status: 'pending', attempts: 7 }); + // Pre-fix this call SUCCEEDED and reset attempts to 0 although the + // predicate demanded a terminal status (measured — see the header). + await expect(engine.update( + SYS_HTTP_DELIVERY, + { attempts: 0 }, + { where: { id: 'p1', status: { $in: ['success', 'failed', 'dead'] } }, multi: false } as any, + )).rejects.toThrow(engineByIdUnhonouredPredicateMessage('Update', ['status'])); + // …and the refusal wrote NOTHING: the silent unconditional write was + // the defect, so the row must be byte-identical. + const row = await readRow('p1'); + expect(`${row.status}:${row.attempts}`).toBe('pending:7'); + }); + + it('the delete twin refuses the same shape symmetrically', async () => { + await seedRow('p2', { status: 'pending', attempts: 1 }); + await expect(engine.delete( + SYS_HTTP_DELIVERY, + { where: { id: 'p2', status: { $in: ['dead'] } } } as any, + )).rejects.toThrow(engineByIdUnhonouredPredicateMessage('Delete', ['status'])); + expect(await readRow('p2')).toBeTruthy(); + }); +}); + +describe('#11009 — redeliver vs a concurrent claim (the triage acceptance evidence)', () => { + it('a row claimed in_flight between read and write is NOT reset, and redeliver reports refusal', async () => { + await seedRow('d1', { status: 'dead', attempts: 3, error: 'receiver down' }); + const outbox = new SqlHttpOutbox(engine as any, { partitionCount: 1 }); + + // The guard runs in the read→write window; flip the row exactly there, + // as the dispatcher's claim tick would. + const claimedAt = Date.now(); + await expect(outbox.redeliver('d1', { + tenantId: undefined, + guard: async () => { + await engine.update( + SYS_HTTP_DELIVERY, + { status: 'in_flight', claimed_by: 'racer', claimed_at: claimedAt, attempts: 4 }, + { where: { id: 'd1' } } as any, + ); + }, + })).rejects.toMatchObject({ + name: 'HttpRedeliverError', + code: 'DELIVERY_NOT_ELIGIBLE', + }); + + // The reset did NOT land: the in-flight claim survives untouched. + // Pre-fix (measured): status=pending, attempts=0, claimed_by=null — + // and redeliver resolved successfully. + const row = await readRow('d1'); + expect(row.status).toBe('in_flight'); + expect(row.attempts).toBe(4); + expect(row.claimed_by).toBe('racer'); + }); + + it('still works: an unraced terminal row is reset and redeliver succeeds', async () => { + // The still-works leg — a refusal that refused everything would score + // green on the race test alone. + await seedRow('d2', { status: 'dead', attempts: 5, error: 'receiver down' }); + const outbox = new SqlHttpOutbox(engine as any, { partitionCount: 1 }); + const replayed = await outbox.redeliver('d2', { tenantId: undefined }); + expect(`${replayed.id}:${replayed.status}:${replayed.attempts}`).toBe('d2:pending:0'); + const row = await readRow('d2'); + expect(`${row.status}:${row.attempts}:${row.error ?? ''}`).toBe('pending:0:'); + }); +}); + +describe('#11009 — MemoryHttpOutbox honours the same contract (one interface, one verdict)', () => { + it('a row claimed in_flight during the guard window is NOT reset, and redeliver reports refusal', async () => { + const outbox = new MemoryHttpOutbox(); + const id = await outbox.enqueue({ source: 'webhook', refId: 'm1', dedupKey: 'm1', url: 'https://x', payload: {} }); + // Drive it terminal the way the store itself does. + await outbox.claim({ nodeId: 'n1', limit: 1, claimTtlMs: 60_000 }); + await outbox.ack(id, { success: false, dead: true, error: 'receiver down', durationMs: 1 }); + + await expect(outbox.redeliver(id, { + tenantId: undefined, + guard: async () => { + // A competing writer flips the row inside the read→write + // window. `list()` returns copies and `claim()` only takes + // `pending` rows, so the flip goes through the store's own + // map — the same stand-in for "another actor wrote the row" + // the SQL leg above expresses through the engine. + (outbox as unknown as { rows: Map }).rows.get(id)!.status = 'in_flight'; + }, + })).rejects.toMatchObject({ name: 'HttpRedeliverError', code: 'DELIVERY_NOT_ELIGIBLE' }); + + const [row] = await outbox.list(); + expect(row.status).toBe('in_flight'); + }); +}); diff --git a/packages/services/service-messaging/src/sql-http-outbox.ts b/packages/services/service-messaging/src/sql-http-outbox.ts index 41c5a0b96e..93676e22aa 100644 --- a/packages/services/service-messaging/src/sql-http-outbox.ts +++ b/packages/services/service-messaging/src/sql-http-outbox.ts @@ -377,6 +377,19 @@ export class SqlHttpOutbox implements IHttpOutbox { // [#8069] Every refusal runs BEFORE the reset UPDATE — a refused // redelivery leaves the row exactly as it was, `dead` reason included. await assertRedeliverAllowed(this.toDelivery(current), options.guard); + // [#11009] `multi: true`, deliberately, and it is the compare-and-set + // half of this method's check-then-act: the status predicate re-states + // the terminal requirement AT THE WRITE so a row that changed under us + // (the dispatcher's tick claims `pending` rows into `in_flight` + // continuously) is NOT reset. On the by-id path (`multi: false`) this + // exact predicate was silently discarded — `driver.update` binds only + // the id — so the guard evaluated to nothing and a mid-flight claim + // was overwritten while redeliver reported success; the engine now + // REFUSES that spelling outright. The predicate path + // (`driver.updateMany`) compiles every `where` key, `id` equality + // included, so the reset lands only if the row is still terminal. + // A miss writes 0 rows and the read-back below reports the refusal + // (`DELIVERY_NOT_ELIGIBLE`) instead of a false success. await this.engine.update( this.objectName, { @@ -390,7 +403,7 @@ export class SqlHttpOutbox implements IHttpOutbox { response_body: null, error: null, }, - { where: { id, status: { $in: ['success', 'failed', 'dead'] } }, multi: false, ...scope }, + { where: { id, status: { $in: ['success', 'failed', 'dead'] } }, multi: true, ...scope }, ); const after = (await this.engine.findOne(this.objectName, { where: { id }, diff --git a/packages/services/service-queue/src/db-queue-adapter.test.ts b/packages/services/service-queue/src/db-queue-adapter.test.ts index e602cfecf5..00d3434e5f 100644 --- a/packages/services/service-queue/src/db-queue-adapter.test.ts +++ b/packages/services/service-queue/src/db-queue-adapter.test.ts @@ -5,6 +5,7 @@ import { ENGINE_DELETE_DISPATCH_CASES, ENGINE_DELETE_REJECT_MESSAGE, assertEngineDeleteDispatch, + resolveEngineDeleteDispatch, } from '@objectstack/objectql'; import { DbQueueAdapter } from './db-queue-adapter.js'; @@ -276,7 +277,13 @@ describe('makeFakeEngine().delete conforms to ObjectQL.delete (#4550)', () => { if (c.expect === 'reject') { // Not merely "throws": the same message a real server answers with, so // the fake's rejection surface cannot drift from the producer's. - await expect(call).rejects.toThrow(ENGINE_DELETE_REJECT_MESSAGE); + // [#11009] `reject` no longer has one spelling — the unhonoured- + // predicate refusal composes its message from the dropped keys — so + // the expected words are read from the PREDICATE itself, per case, + // instead of from the one classic constant. + const predicted = resolveEngineDeleteDispatch(c.options); + if (predicted.kind !== 'reject') throw new Error(`case-set drift: ${_what} is not a reject`); + await expect(call).rejects.toThrow(predicted.message); // …and a refused call must not have deleted anything on its way out. expect(engine.tables.get('sys_job_queue')).toHaveLength(1); return;