Skip to content

fix(objectql,metadata-core,service-messaging): refuse a by-id write whose where carries predicate keys the by-id path drops (#11009) - #11147

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-11009-refuse-unhonoured-by-id-predicate
Aug 22, 2026
Merged

fix(objectql,metadata-core,service-messaging): refuse a by-id write whose where carries predicate keys the by-id path drops (#11009)#11147
os-zhuang merged 1 commit into
mainfrom
claude/issue-11009-refuse-unhonoured-by-id-predicate

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#11009

The defect, measured before the fix

A by-id update/delete routes to driver.update(object, id, …) / driver.delete(object, id, …), which bind ONLY the primary key (sql-driver.ts:5815 builds .where('id', id) + tenant scope and never reads options.where). Every other where key was silently discarded. Reproduced on origin/main @ 95437e7d2 (better-sqlite3, real ObjectQL + SqlDriver, sys_http_delivery):

  • the issue's minimal shape — update(…, { attempts: 0 }, { where: { id: 'p1', status: { $in: ['success','failed','dead'] } }, multi: false }) on a pending row — landed (attempts 7 → 0) although the predicate demanded a terminal status;
  • SqlHttpOutbox.redeliver with a guard that flips the row to in_flight inside its read→write window reset the row anyway (pending, attempts: 0, claim cleared) and reported success.

Both measurements were taken as assertions of the buggy behaviour and passed on the unfixed tree before any edit.

The fix — refuse at the engine (triage's option 2 + 3, the #5748/#6435 loud-refusal family)

The rule lands in the shared dispatch predicates (packages/metadata-core/src/engine-{update,delete}-dispatch.ts), so ObjectQL, its case tables, and every one of the pinned fake engines (check:engine-double-contract) get one verdict:

call shapebeforeafter
where: { id } — pure primary-key address, with or without multi: trueby-idby-id (unchanged — the LifecycleService guarded-reap idiom keeps its per-record cascade path; now pinned in both case tables and in engine-delete-dispatch.test.ts)
where: { id, …extra }, multi: true (id sourced from where)by-id, extra keys silently droppedmulti — the predicate path (updateMany/deleteMany) compiles EVERY where key; the compare-and-set spelling
where: { id, …extra }, no multiby-id, extra keys silently droppedrefused, naming the dropped keys (engineByIdUnhonouredPredicateMessage, one source-blind composer shared by both verbs)
scalar data.id + any extra where keys, multi or notby-id, where silently droppedrefused — the payload id outranks multi (#5748) and cannot be demoted onto the predicate path, so the unhonourable predicate is loud, never silently dropped or silently bulk-promoted

ObjectQL.update/delete now key their predicate-path branch on the ladder's verdict instead of the raw options.multi flag (otherwise a refused data.id+multi call would silently become a bulk write) and throw the dispatch's own message; the classic no-id-no-multi shapes keep ENGINE_UPDATE_REJECT_MESSAGE/ENGINE_DELETE_REJECT_MESSAGE byte-identical.

Blast-radius census (measured FIRST, per the triage order)

Scripted sweep (balanced-paren argument scan over every .update(/.delete( call with a where: bag, 4863 files) + a second sweep for data.id-sourced shapes + a multi: false grep. Every hit classified:

  • where id+extra, production: exactly one — SqlHttpOutbox.redeliver (multi: false, the measured victim; migrated in this PR). Zero others.
  • where id+extra + multi: true: zero callers anywhere (the rerouted cell of the table above ships with no occupants).
  • scalar data.id + separate where bag: zero production callers.
  • pure where: { id } + multi: true (verdict deliberately unchanged): LifecycleService guarded reap (production — its comment depends on the by-id cascade path), plugin-sharing bu-tree tests, engine-data-events.test.ts (pins the per-record event for exactly this shape). This is why "multi always wins" was rejected: it would have broken a deliberate, pinned production behaviour.
  • Flow authors reach the refusal through update_record/delete_record nodes whose filter names id plus other keys without multi — previously silently unconditional, now loud (stated in the changeset).
  • Fake engines: all pinned doubles inherit the rule through assert*Dispatch; the one test asserting the old single reject-message constant per case (service-queue/db-queue-adapter.test.ts) now quotes the predicate's own per-case message.

The guard of every censused caller is expressible on the predicate path, so the triage stop-condition was never reached, and the by-id driver path was not widened.

redeliver off the trap — both implementations

  • SqlHttpOutbox.redeliver rides the predicate path (multi: true): the terminal-status CAS is compiled into the one UPDATE, a miss writes 0 rows, and the existing read-back reports DELIVERY_NOT_ELIGIBLE instead of a false success. Tenant classification is unchanged (threaded tenantId, never bypassTenantAudit) — tenant-audit, the update half: ack is a dispatcher sweep but redeliver is request-reachable — two sites on one object with OPPOSITE classifications #10740's pins migrated to the updateMany op with their vacuity controls intact, plus a new positive control for that op.
  • MemoryHttpOutbox.redeliver re-checks terminal status after its awaited guard (bounded inline fix, declared on the issue): without it the two IHttpOutbox implementations would disagree on exactly the race this card closes.
  • outbox-dispatcher-scope.ts docstring re-anchored: multi no longer discriminates redeliver from the sweep helper; the tenant classification does.

Acceptance evidence (triage-named)

redeliver-concurrent-claim.integration.test.ts — real ObjectQL + SqlDriver on better-sqlite3: the guard window flips the row to in_flight; the reset does not land (row stays in_flight, attempts: 4, claimed_by: 'racer') and redeliver rejects DELIVERY_NOT_ELIGIBLE. Before the fix the same harness measured pending/attempts: 0/success. A still-works leg pins the unraced reset, and the memory-store leg pins the same contract.

Verification (local; CI convergence is the PM's read)

All at head 2e3d05049, through the shared verify lock:

  • Suites (all green): metadata-core 165 · objectql 4046 (228 files — dispatch conformance drives the REAL engine over both case tables, reject messages asserted byte-identical) · driver-sql 1795 · metadata-protocol 1853 (the 13 pinned fakes) · service-messaging 259 (re-run at head) · service-automation 998 · plugin-sharing 624 · plugin-webhooks 128 · service-queue 56.
  • pnpm typecheck on the four changed packages (script names echoed; wrapper verdict command-exit 0).
  • Gates, each read from its own verdict line: check:changeset-gate-self-tests · check:durability-log-level · check:objectui-changeset · check:slot-lookup · check:stack-collection-maps · check:test-source-alias · check:type-source-resolution · check-adr-0087-registration · check-changeset-no-major · check-ci-filter-parity · check-empty-changeset · check-engine-split-ratio · check-plugin-teardown-shape · docs-audit/check-affected-docs · check:query-options-erasure · check:engine-double-contract · check:where-matcher · check:nul-bytes · check:driver-conformance (prompt-named; sql-driver.ts untouched) · check:i18n · check:type-check-coverage · check:type-check-debt (after the full workspace closure build the gate itself prescribes) — all green. Gate list re-derived with node scripts/pm/dispatch-gates.mjs (no paths; 16 changed files vs merge base 95437e7d2).

Out of scope, filed

Generated by Claude Code


Generated by Claude Code

…hose where carries predicate keys the by-id path drops (#11009)
A by-id update/delete routes to driver.update/delete(object, id, ...),
which bind only the primary key — every other options.where key was
silently discarded, so a compare-and-set guard evaluated to nothing and
the write landed unconditionally. Measured on better-sqlite3 through a
real ObjectQL + SqlDriver: SqlHttpOutbox.redeliver's terminal-status
guard was inert; a row claimed in_flight between its read and its write
was reset anyway and the redelivery reported success.
The dispatch predicates (metadata-core, shared with every pinned test
double) now rule:
- a where naming a scalar id and NOTHING else stays by-id, with or
without multi (the LifecycleService guarded-reap idiom keeps its
per-record cascade path — pinned in both case tables);
- a where carrying a scalar id plus other keys WITH multi:true routes
to the predicate path (updateMany/deleteMany), which compiles every
where key — the compare-and-set spelling;
- the same shape without multi — and any data.id-sourced by-id beside
extra where keys (payload id outranks multi per #5748) — is refused
loudly, naming the dropped keys, via the shared #11009 composer.
ObjectQL.update/delete key their predicate-path branch on the ladder's
verdict (not the raw multi flag) and throw the dispatch's own message.
SqlHttpOutbox.redeliver rides the predicate path so its guard is
evaluated; MemoryHttpOutbox re-checks terminal status after its guard,
so both IHttpOutbox implementations refuse a mid-flight claim with
DELIVERY_NOT_ELIGIBLE instead of reporting success.
Fixes#11009
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

8 anchor(s) derived from 3 changed package(s); no hand-written page names any of them. ✅

What this run could not see
  • 4 changed file(s) yielded no anchor (packages/metadata-core/src/index.ts, packages/objectql/src/engine-update-dispatch.ts, packages/objectql/src/index.ts, …) — pages documenting those are invisible to this run
  • 1 anchor(s) matched too much of the corpus to be a work list: ObjectQL (symbol, 64 pages)
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 45 of 221 client-bound route-ledger rows — the other 176 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 16 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json acb4dbc09703d5c6145efb376c50ea12dfe9f41cpackageMentionDocs.

Which tree this was computed on

This run read content/docs from 665eb3344cca33d7834f9d3854c585cfb08dc635 — the merge of head 2e3d05049e47276cc59b60f3d90a7b3f6fce65a4 into base acb4dbc09703d5c6145efb376c50ea12dfe9f41c, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 665eb3344cca33d7834f9d3854c585cfb08dc635 && git checkout 665eb3344cca33d7834f9d3854c585cfb08dc635
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin acb4dbc09703d5c6145efb376c50ea12dfe9f41c 2e3d05049e47276cc59b60f3d90a7b3f6fce65a4 && git checkout -B drift-repro acb4dbc09703d5c6145efb376c50ea12dfe9f41c && git merge --no-ff 2e3d05049e47276cc59b60f3d90a7b3f6fce65a4
node scripts/docs-audit/affected-docs.mjs --json acb4dbc09703d5c6145efb376c50ea12dfe9f41c

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

@github-actions

Copy link
Copy Markdown
Contributor

⛔ merge queue 构建失败 — 先分诊,再决定要不要重排

队列构建 32602076426 红了。队列跑的是全量套件(PR 侧 CI 只跑 affected 子集),
所以失败的测试可能在本 PR 没碰过的包里 —— 那不是重排能修的。每次盲目重排都会让排在后面的所有 PR 重建一轮。

失败的 job(日志抽取,best effort):

  • Console Pin Gate — 失败步骤: Build the Console SPA at the pinned objectui SHA

    ✗ Build failed in 6.04s
    

↳ 失败原因 是判读的关键:超时Test timed out in … / Hook timed out in …)多半是负载/时序,不是本 PR 的回归;
断言AssertionError: …)才指向真实的行为改变。两者的 FAIL 行长得一模一样,只有这一行能区分。

跨 PR 相同签名(24h,按失败测试文件聚合):

  • ⚠️本次没有可用的聚合签名(日志里没有能解析出测试文件名的 FAIL 行)—— 这不是「没有同签名的其他 PR」,是这一轮没测到。跨 PR 聚合本次不可用,请手工比对其他 PR 的同类评论。
  • ⚠️ 24h 评论账本没读完(超过 5 页仍未读到窗口尽头),所以上面的「不同 PR 数」是下界,不是全量。

历史信号:

  • 本 PR 过去 24h 无队列失败记录(首次)。
  • 过去 24h 队列共有 104 个失败构建(不含本次)。

分诊清单:

  1. 失败测试在本 PR 改动的包里 → 真回归,修 PR。
  2. 失败测试与本 PR 无关 → 看上面的「跨 PR 相同签名」;已有汇总 issue ⇒ flaky/环境问题实锤,去那张 issue 上谈,修好前重排只会再烧一轮全队列。
  3. 两者都不是 → 可能与同组 PR 语义冲突;等前面的 PR 落地或失败出队后再重排一次即可,不要连续重排。

Generated by Claude Code · merge-queue-triage workflow (#4859)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-zhuang@claude