Skip to content

fix(service-messaging): an HTTP ack binds the claim credential, so a reaped claim's late ack cannot overwrite the live re-claim - #17641

Merged
hotlong merged 5 commits into
mainfrom
claude/issue-17634-http-ack-claim-credential
Sep 11, 2026
Merged

fix(service-messaging): an HTTP ack binds the claim credential, so a reaped claim's late ack cannot overwrite the live re-claim#17641
hotlong merged 5 commits into
mainfrom
claude/issue-17634-http-ack-claim-credential

Conversation

@hotlong

@hotlong hotlong commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Closes #17634

A late ack from a claim the visibility-timeout reap had taken back overwrote the live re-claim on sys_http_delivery: IHttpOutbox.ack(id, result) carried no claim credential, and both stores wrote by id. This PR gives the HTTP outbox the protection the notification outbox has had since #11859 — no new design: the (claimedBy, claimedAt) credential round-tripped from claim(), a compare-and-set that re-states ownership in the write, a refusal that writes nothing, and the dispatcher's ack refused, claim no longer held warn.

Baseline red — the defect reproduces on origin/main

Base 8f751cdc5, which contains #17632's merge a9096af48 (git merge-base --is-ancestor a9096af48 HEAD → exit 0). The reproduction test was committed alone as f945fa864git diff --stat 8f751cdc5 f945fa864 -- . ':!**/*.test.ts' prints nothing — and run there:

$ pnpm --filter @objectstack/service-messaging exec vitest run --maxWorkers=2 src/http-outbox-ack-claim-ownership.integration.test.ts
 × 'MemoryHttpOutbox' — HttpDispatcher acks with the claim it holds (#17634) > replays the card: A's late ack leaves B's live re-claim alone, and B's own ack lands
 × 'SqlHttpOutbox' — HttpDispatcher acks with the claim it holds (#17634) > replays the card: A's late ack leaves B's live re-claim alone, and B's own ack lands
AssertionError: expected 'dead:-:-:1:410' to be 'in_flight:node-b:1060001:0:-' // Object.is equality
Tests  2 failed | 2 passed (4)

The sequence is the card's, driven through two real HttpDispatchers sharing one store — injected clock, gated fetches, no hand-poked rows. Node A claims at T0; A's POST moves the clock past claimTtlMs and runs node B's tick, which reaps and re-claims the row and starts POSTing (held open); A's POST answers 410 and A acks. On base the row read dead, unclaimed, 1 attempt, response 410 — while B was still sending. The two green legs are the negative controls (the same dispatchers without the reap: A's ack lands). Reproduced on MemoryHttpOutbox, and on SqlHttpOutbox over a real ObjectQL + SqlDriver (better-sqlite3 :memory:), the harness http-dispatcher-idle-cost.integration.test.ts uses.

The compatible form — design and reasoning

IHttpOutbox.ack gains an optional third parameter:

ack(id: string, result: HttpAckResult, claimed?: HttpClaimCredential)   // resolves to void, as before

HttpClaimCredential is { claimedBy: string; claimedAt: number } — exactly the pair the notification outbox's ClaimedDeliveryRecord guarantees, with the same meaning: the pair identifies one CLAIM, not one node, and ownership is proven by handing back what claim() returned (the dispatcher passes the claimed row itself). Three forms were weighed against the three kinds of existing code:

form existing IHttpOutbox implementation existing caller subclass overriding a built-in store's ack()
optional third parameter (chosen) compiles; ignores the extra argument; behaves as before gets the by-id write it always got keeps receiving every dispatcher ack; gains the check by forwarding the argument
new optional method (ackClaimed?) + deprecated ack — the reap? pattern from #17632 compiles; the dispatcher probes and falls back to ack unchanged silently stops seeing dispatcher acks — the dispatcher calls the new method instead
record-typed first parameter (overload or union) an overload taking the record is not assignable from an (id: string, …) implementation; a union compiles but hands that implementation an object at run time unchanged same as the implementation column

The subclass column is not hypothetical: TickRecordingOutbox in http-dispatcher-idle-backoff.test.ts overrides MemoryHttpOutbox.ack() to record retry schedules. Under the new-method form its retry legs would have lost their readings; under the chosen form it keeps working unchanged, and this PR makes it forward the credential (one line) so its dispatcher acks keep the check.

For the same reason claim() keeps declaring HttpDelivery[] instead of a narrowed claimed-record type: narrowing a built-in store's declared return type stops a subclass override that declares the old one from compiling (TS2416 — that test double overrides claim() too). SqlHttpOutbox.claim() does now stamp the credential explicitly on its results, in the values its claiming UPDATE wrote, as SqlNotificationOutbox.claim() does.

The price, stated: omitting claimed still compiles, so the checked path is not type-enforced for a caller. The one production caller is HttpDispatcher, pinned by the dispatcher legs and by ablation A below. The two-argument arity is documented as deprecated (it checks no ownership); retiring it, and making the parameter required, is next-major work.

Changeset: minor, not declared breaking — nothing an implementer or caller wrote has to change.

Acceptance, item by item

1. IHttpOutbox.ack carries the claim credential, aligned with ClaimedDeliveryRecord, in a compatible form. As above. New exports: HttpClaimCredential, HttpAckError.

2. SqlHttpOutbox.ack writes only on id + claimed_by + claimed_at + status = 'in_flight'; no match ⇒ nothing written, logged at warn in the notification side's words. Handed the credential, SqlHttpOutbox.ack takes SqlNotificationOutbox.ack's shape: two deterministic refusals read before any write (row not in_flight; row claimed under another credential); the same two tests re-stated in the conditional write through dispatcherAckCasOptions(id, 'in_flight', claimedBy, claimedAt), i.e. where: { id, status: 'in_flight', claimed_by, claimed_at } on the predicate path (updateMany — the by-id path discards every predicate but the id, #11009); and a (status, attempts) read-back that reports a write which matched nothing. A refusal throws HttpAckError with DELIVERY_NOT_ELIGIBLE — the code this package already raises for a delivery row in the wrong state, so no new code — and writes nothing. HttpDispatcher absorbs exactly that code and logs http-dispatcher: ack refused, claim no longer held (the notification dispatcher's line is notification-dispatcher: ack refused, claim no longer held), then carries on with the rest of its batch.

  • The write predicate is pinned on a real dispatcher tick: delivery-update-tenant-audit.integration.test.ts now reads the HTTP ack on its updateMany spy and requires, for both organizations' rows, a scalar where.id + status: 'in_flight' + claimed_by: 'n1', bypassTenantAudit: true, no tenantId, zero by-id update calls on the object and no audit line — then the updateMany positive control fires.
  • A refusal leaves the row untouched: every store leg below pins the row fingerprint after the refusal.

3. MemoryHttpOutbox checks the same. The same two tests (single-threaded, so each is atomic with the mutation), the same error, and the same messages — one helper both stores call.

4. HttpDispatcher passes the credential its own claim returned. ackAttempt() hands the claimed row back as the credential. Pinned by the dispatcher legs; see ablation A.

5. Tests, both stores. src/http-outbox-ack-claim-ownership.integration.test.ts, describe.each over MemoryHttpOutbox and SqlHttpOutbox, 16 tests:

  • through the dispatcher — the card replay (after A's late ack the row still reads in_flight:node-b:1060001:0 with no response code, with exactly one warn naming node-a and the row; B's ack then records success, 1 attempt, B's 200); the negative control (no reap: A's ack lands dead, 410, no warn); and an outbox whose ack reads (id, result) only still gets its attempt recorded.
  • through the store contract — the card replay with explicit claim() instants; its negative control; the same node's own re-claim refusing its stale ack (claimedAt is the discriminator); a claim reaped and not re-claimed refusing a retry-shaped ack, whose post-state would also read pending, so only attempts could expose a write that landed; and a credential missing a member refused before anything is written. Refusals assert the error identity (name: 'HttpAckError', code: 'DELIVERY_NOT_ELIGIBLE'), never a bare throw.

Ablation A — one-off, trap-restored

On the committed fix (06215b074) the dispatcher stopped passing the credential: outbox.ack(row.id, result, row as HttpClaimCredential) became outbox.ack(row.id, result) (on disk: removed text 1 → 0, injected text 0 → 1). The tests import the source by relative path, so no dist rebuild is in the resolution path.

 × 'MemoryHttpOutbox' — HttpDispatcher acks with the claim it holds (#17634) > replays the card: …
 × 'SqlHttpOutbox' — HttpDispatcher acks with the claim it holds (#17634) > replays the card: …
AssertionError: expected 'dead:-:-:1:410' to be 'in_flight:node-b:1060001:0:-' // Object.is equality
Tests  2 failed | 14 passed (16)

Restored with git checkout HEAD -- PATH: blob f8e5db21f before and after, and git diff HEAD on the path is empty. The store legs stayed green under the mutation — they pin the stores, the dispatcher legs pin the dispatcher.

Docs and census

  • content/docs/automation/webhooks.mdx §12 gains a failure-mode row: a send that outlasts the claim TTL while another dispatcher re-claims the row.
  • The conditional write is one more write call site in sql-http-outbox.ts (4 → 5 update sites), so the tenant-audit census moves 222 → 223. node scripts/tenant-audit-census.mjs --write regenerated content/docs/permissions/tenant-audit-census.mdx and docs/audits/2026-08-tenant-audit-write-call-sites.counts.md, and the eight hand-written prose figures the gate named moved with it (unreadable 66 → 67, the over-claim 83 → 84, undecidable 73 → 74, elevation-undecidable 100 → 101, population 222 → 223).

Contract

No packages/spec/src/** in the diff, and nothing in packages/spec changes what it accepts or rejects. The contract change is on service-messaging's own runtime interface IHttpOutbox: one optional parameter, plus two exports.

Verification — head b373361f6

origin/main was merged in at 240704622 (8 incoming commits, none touching this diff's paths, lockfile unchanged); every reading below is on the final head.

  • pnpm --filter @objectstack/service-messaging build → exit 0, check-dts-emitted: 2/2
  • pnpm --filter @objectstack/service-messaging typecheck → exit 0; tsc --listFiles includes all three touched test files
  • pnpm --filter @objectstack/service-messaging test → 40 files, 423 tests passed (the 16 new ones included)
  • dependency closure pnpm turbo run build --filter='@objectstack/service-messaging^...' --concurrency=2 → 15/15
  • ⚠️ scripts/pm/os-verify-lock.sh ran in declared UNLOCKED mode on this host (no usable flock), so nothing was serialized.
  • derived gate families: node scripts/pm/dispatch-gates.mjs --commands → 94 commands, every one run on b373361f6 with its exit code captured before any pipe; node scripts/pm/dispatch-gates.mjs --ran94 derived famil(ies) accounted for — 93 run, 1 NOT-MEASURED (1 DERIVED from a recorded exit 3). 92 exited 0, among them:
    • check-tenant-audit-census: OK -- 223 write call sites certified (149 decidable; 9 tenancy-enabled sites PROVABLY carry no tenant context, 32 more unreadable), 23 prose figures held to the census.
    • check-adr-0087-registration: this PR adds no declared-breaking changeset (1 non-breaking changeset(s) seen).
    • check-doc-authoringsibling-package prose ids hold the baseline … no growth. On the first sweep it caught the tracker id this PR had put into two HttpAckError message strings; b373361f6 takes it out of the strings (the JSDoc keeps it).
    • check-dispatcher-error-vocabulary: OK, check-engine-double-contract: OK, check:nul-bytes, check:cross-package-test-inputs, check:published-files, check:docs (222 generated files in sync), check-changeset-no-major, check-empty-changeset.
    • check:skill-examples and check:i18n first refused on unbuilt prerequisites (packages/client-react/dist; the workspace CLI and its extract closure). After the targeted closure the gates name — pnpm exec turbo run build --concurrency=2 over the CLI, its 9 extract packages and @objectstack/client-react..., 58 tasks — both ran green: 258 prose examples type-check across 3 surface(s) and check-i18n-bundles: OK (9 package(s) — all bundles in sync, no undeclared authoring keys). That build also let check:type-check-debt measure: --re-measure: OK — 5 ledger entr(ies) re-measured …, none above its recorded number.
  • NOT MEASURED: check:dual-build-cjs-loads — exit 3, PREREQUISITE NOT MET: it reads the dist/ of every package (hono, account, setup, studio, client, cloud-connection and 46 more), a workspace-wide build this seat does not run. CI measures it.
  • Red, not introduced here: check:merge-driver — exit 1: its self-test fails the case "a gate whose RUNNER is not installed refuses the same way" and that case's diagnosis leg. Control leg: the same command in a detached worktree at origin/main 76c9fab30, on this host, exits 1 with the identical failing cases. This diff touches none of scripts/git-env.mjs, scripts/git-merge-regen.mjs, scripts/check-regen-pending.mjs or .gitattributes. Left for CI; reported to the dispatching PM.
  • CI on this PR was still pending when this body was written; not waited on.

Acceptance notes

  • reported to the dispatching PM for filing, not fixed here: pnpm check:merge-driver self-test exits 1 on this host at origin/main 76c9fab30 itself — the case "a gate whose RUNNER is not installed refuses the same way" and its diagnosis leg. Not touched by this diff.
  • noted, not filed: packages/spec/src/api/error-code-ledger.zod.ts — the prose under DELIVERY_NOT_ELIGIBLE names its throw surfaces per surface (IHttpOutbox.redeliver, INotificationOutbox.ack); IHttpOutbox.ack handed a credential (HttpAckError) is now a third. Left unedited to keep packages/spec out of this diff; the code is already registered under @objectstack/service-messaging, so no gate reads that prose. Picked up by: the next PR that edits that ledger entry; none scheduled.
  • noted, not filed: the two-argument IHttpOutbox.ack arity stays unchecked by design, for compatibility. Retiring it and making claimed required is next-major work. Picked up by: none scheduled.
  • noted, not filed: the (status, attempts) read-back that reports a missed conditional write is blind to one interleaving — the row reaped, re-claimed and acked by another node to the same status with the same attempt count, all between this ack's read and its write. SqlNotificationOutbox.ack has the identical detector; IDataEngine.update declares its return as any, so the row is the only answer available. Not reproduced. Picked up by: none.
  • noted, not filed: HttpDispatcher still calls onAttempt after a refused ack (the attempt did go on the wire) — unchanged from before, and onAttempt has no production subscriber in this repo.
  • noted, not filed: the header of delivery-update-tenant-audit.integration.test.ts still describes the redeliver assertions as a spy on SqlDriver.update; they moved to updateMany in A compare-and-set where on a by-id update is silently inert — the extra predicate keys never reach the driver, and SqlHttpOutbox.redeliver's status guard is one of them #11009. Pre-existing prose drift.

Generated by Claude Code

@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 11, 2026
@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-messaging, touching 20 documentable anchor(s). ⚠️ 2 changed file(s) yielded no anchor (packages/services/service-messaging/src/index.ts, packages/services/service-messaging/src/outbox-dispatcher-scope.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

4 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/api/environment-routing.mdx (via HttpDispatcher (symbol, a top-level class))
  • content/docs/automation/webhooks.mdx (via HttpDispatcher (symbol, a top-level class), DELIVERY_NOT_ELIGIBLE (literal, a string literal in ack; a string literal in ackAttempt; a string literal in assertHttpClaimCredential; a string literal in constructor), claimed_at (literal, a string literal in ack), claimed_by (literal, a string literal in ack), in_flight (literal, a string literal in ack; a string literal in httpAckNotClaimedMessage))
  • content/docs/kernel/cluster.mdx (via HttpDispatcher (symbol, a top-level class), claimed_by (literal, a string literal in ack))
  • content/docs/plugins/packages.mdx (via HttpDispatcher (symbol, a top-level class))

1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v17/17-2.mdx (via IHttpOutbox (symbol, a top-level interface), SqlHttpOutbox (symbol, a top-level class), DELIVERY_NOT_ELIGIBLE (literal, a string literal in ack; a string literal in ackAttempt; a string literal in assertHttpClaimCredential; a string literal in constructor))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 2 changed file(s) yielded no anchor (packages/services/service-messaging/src/index.ts, packages/services/service-messaging/src/outbox-dispatcher-scope.ts) — pages documenting those are invisible to this run
  • 3 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 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 bc2bf01c8a30f0caed922b33d21ab2dac2bba274packageMentionDocs.

Which tree this was computed on

This run read content/docs from a61412bea9f99e1d336588a55287492a122cd965 — the merge of head b373361f64acc867de530d39b762d8772a7e5c1e into base bc2bf01c8a30f0caed922b33d21ab2dac2bba274, 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 a61412bea9f99e1d336588a55287492a122cd965 && git checkout a61412bea9f99e1d336588a55287492a122cd965
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin bc2bf01c8a30f0caed922b33d21ab2dac2bba274 b373361f64acc867de530d39b762d8772a7e5c1e && git checkout -B drift-repro bc2bf01c8a30f0caed922b33d21ab2dac2bba274 && git merge --no-ff b373361f64acc867de530d39b762d8772a7e5c1e

node scripts/docs-audit/affected-docs.mjs --json bc2bf01c8a30f0caed922b33d21ab2dac2bba274

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs bc2bf01c8a30f0caed922b33d21ab2dac2bba274 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@hotlong

hotlong commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

PM 复核(Session c5c0ce54):通过,等 CI

复核对象:head b373361f6,共 13 个文件。不涉及 packages/spec/src/** 和受治理面;changeset 为 minor,未声明 breaking。

逐项核对:

  1. 兼容形态。 IHttpOutbox.ack 只加了一个可选的第三个参数,旧的两参数实现依然可以赋值。claim() 仍声明返回 HttpDelivery[],子类覆盖不受影响。
  2. 与先例一致。 读取段的两个拒绝条件(不是 in_flight、凭证不符)、经 dispatcherAckCasOptions 的条件写、(status, attempts) 回读,与 main 上的 SqlNotificationOutbox.ackINotificationOutbox has no cancellation, and ack() on an unclaimed pending row silently succeeds in both implementations #11453INotificationOutbox.ack() carries no nodeId, so its compare-and-set can prove a claim exists but not whose #11859)逐行同构。claim() 也同样把 claimedBy: opts.nodeId, claimedAt: now 显式写回返回的行。
  3. 方言风险已排除。 读取段对 claimed_at 用的是严格相等。我对照了两个对象定义:sys_http_deliverysys_notification_deliveryclaimed_at 都是 Field.number,与通知 outbox 条件相同,而通知 outbox 的同一套比较在托管环境的远程 Turso 上能正常把投递写成终态。因此不会出现这种新风险:某个方言把这一列读回成字符串,导致每次 ack 都被拒、投递无限重发。
  4. 调度器的吸收范围。 ackAttempt 只吸收 DELIVERY_NOT_ELIGIBLE,存储故障照常抛出。没有 fetch 实现的分支也改走了 ackAttempt
  5. 基线红与消融。 修复前,新测试在内存和 SQL 两个存储上各红 1 例;去掉调度器传递凭证后同样是 2 例红;恢复后 16/16 通过。

范围外,不阻塞:

下一步:CI 全部跑完且无失败,就转 ready 并入合并队列(以 timeline 出现 added_to_merge_queue 为准)。出现失败则按日志退回席位修复。

@hotlong
hotlong marked this pull request as ready for review September 11, 2026 07:56
@hotlong
hotlong enabled auto-merge September 11, 2026 07:56
@hotlong
hotlong added this pull request to the merge queue Sep 11, 2026
Merged via the queue into main with commit 4be4e04 Sep 11, 2026
42 checks passed
@hotlong
hotlong deleted the claude/issue-17634-http-ack-claim-credential branch September 11, 2026 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

1 participant