Skip to content

feat(service-messaging): bind the claim credential in ack()'s compare-and-set - #12282

Merged
os-trump merged 1 commit into
mainfrom
claude/issue-11859-outbox-ack-claim-token
Aug 25, 2026
Merged

feat(service-messaging): bind the claim credential in ack()'s compare-and-set#12282
os-trump merged 1 commit into
mainfrom
claude/issue-11859-outbox-ack-claim-token

Conversation

@os-trump

@os-trumpos-trump commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Fixes#11859

Implements the maintainer ruling on the card (2026-08-25, option C — "the claim token rides the record; ack returns it; the cancellation surface stays deferred"): claim() / claimDigest() now declare their true return type, ClaimedDeliveryRecord[] — the same rows as before with the (claimedBy, claimedAt) pair the store stamps guaranteed present — and INotificationOutbox.ack() takes that record back instead of a bare id. Both implementations include the credential in the ack compare-and-set, so a late ack from a node whose claim was reaped and re-claimed matches nothing and is refused with the existing NotificationAckError (DELIVERY_NOT_ELIGIBLE, ADR-0112), writing nothing. The caller never supplies an identity: ownership is proven by round-tripping what claim() returned (option A's caller-supplied identity and option B's required nodeId parameter are deliberately not taken, per the ruling). The credential is the pair, not the node id alone: claimedAt distinguishes two claims by the same node, so a stale ack cannot land on its own node's later re-claim. The deferred cancellation surface (#11454) is untouched; the "deliberately no ack() spelling for stopping a pending delivery" sentence in outbox.ts stands.

Call-site population (measured on the merged ref cd2541780)

Derived by scanning packages/ examples/ apps/ for INotificationOutbox / SqlNotificationOutbox / MemoryNotificationOutbox references plus every .ack( call on those types.

Production ack call sites (1):

  • packages/services/service-messaging/src/dispatcher.tsackAttempt (the only production caller; both the normal and the digest path funnel through it). Now passes the claimed record.

Test ack call sites, updated in this PR (8 calls in 2 files):

  • src/outbox-ack-precondition.integration.test.ts — 7 calls (all now round-trip claimed records; the ack-as-cancel pin hands back a list() row, which carries no credential and stays refused).
  • src/sql-outbox-audit-columns.test.ts — 1 call (fake row now carries the credential the record round-trips).

Consumers of the claim/record types, measured and compile-compatible with no edit (the narrowing is a supertype-to-subtype return change; none call ack directly):

  • service-messaging: dispatcher.test.ts, digest.test.ts, messaging-service.test.ts, delivery-claim-tenant-audit.integration.test.ts, delivery-update-tenant-audit.integration.test.ts (drives ack through the dispatcher; its CAS-shape filter still matches), messaging-service.ts (enqueue only), messaging-service-plugin.ts (constructs the outbox), index.ts (re-exports; now also exports ClaimedDeliveryRecord).
  • service-automation: builtin/notify-delivery-outcome.integration.test.ts, builtin/notify-organization-stamp.integration.test.ts — reach the types via MemoryNotificationOutbox + dispatcher; re-run green against the rebuilt dist (2 files, 9 tests).
  • packages/spec/src/api/error-code-ledger.zod.ts mentions INotificationOutbox.ack in a prose comment only — not touched (this lane's red line); flagged for the spec seat in the dev report.

The HTTP outbox family (IHttpOutbox, SqlHttpOutbox, MemoryHttpOutbox) is a different interface and is untouched.

The replay test

src/outbox-ack-claim-ownership.integration.test.ts, one table over both backends (memory + real engine: ObjectQL + SqlDriver on better-sqlite3 :memory:), driving every step through the public contract with explicit now values — no hand-set claimed_by:

  1. The card's four-step replay: claim on node A → the claim outruns claimTtlMs → node B's claim() reaps and re-claims → A's late ack() is refused by identity (NotificationAckError + DELIVERY_NOT_ELIGIBLE), and the row still belongs to B's claim with B's attempt intact (the asserted fingerprint reads status in_flight, holder node-b at the re-claim instant t1, zero attempts, no error), and B's own ack then lands (status success, one attempt).
  2. Negative control (same sequence minus the reap): B's claim inside the TTL takes nothing — proven, [] — and A's ack must succeed. An implementation that refuses every ack fails here.
  3. The credential is the claim, not the node: A's own later re-claim of its reaped row refuses A's first-claim ack — pins claimedAt in the predicate.

Dissolution verification (direction predicted in writing before running): dropping ownership from the predicate (CAS where-clause + both backends' ownership checks; mutation proven on disk by marker/absence greps before running) turned exactly the predicted legs red — 4 failed (promise resolved "undefined" instead of rejecting, both refusal legs on both backends) while both negative controls and the entire #11453 precondition suite stayed green (12 passed). Restored via git checkout HEAD -- under an EXIT/INT/TERM trap; git status --porcelain clean (disk == index == HEAD), pins re-run green 6/6.

Reverse verification of the narrowed signature against the rebuilt dist: a scratch consumer in service-automation calling the old spelling fails compilation — error TS2345: Argument of type 'string' is not assignable to parameter of type 'ClaimedDeliveryRecord' — while the round-trip spelling compiles (positive control, exit 0).

Verification (union at 8deb984b2, the final commit)

  • pnpm --filter @objectstack/service-messaging typecheck — exit 0.
  • pnpm --filter @objectstack/service-messaging exec vitest run — 29 files, 295 tests, all passed.
  • pnpm --filter @objectstack/service-automation exec vitest run src/builtin/notify-delivery-outcome.integration.test.ts src/builtin/notify-organization-stamp.integration.test.ts — 2 files, 9 tests, all passed.
  • Derived gates (scripts/pm/dispatch-gates.mjs, no paths), all exit 0 locally: check:nul-bytes, check:changeset-gate-self-tests, check:objectui-changeset, check:published-files, check:slot-lookup, check:test-source-alias, check:type-source-resolution, check-adr-0087-registration (accepts the no-migration-prescription disposition), check-changeset-no-major, check-empty-changeset, check-plugin-teardown-shape, release-rehearsal-clone --self-test, docs-audit/check-affected-docs; convention-triggered: check:query-options-erasure, check:type-check-coverage, check:type-check-debt (32 ledger entries re-measured, none above its recorded number), check:engine-double-contract, check:cross-package-test-inputs, check:where-matcher, check:i18n, check:i18n-stale-fill. docs-audit/check-drift-comment is PR-context-only and runs in CI.

Changeset

@objectstack/service-messagingminor with an explicit BREAKING note (launch-window convention; never major): the one breaking edit for a host is at ack call sites, which hand the whole record back where they previously handed record.id — the caller already holds it, since ack completes a claim and the record is what claim() returned. Reads of claim results do not change. No stored row changes shape; sys_notification_delivery is byte-identical before and after. ADR-0087 disposition: not-required (no-migration-prescription), stated in the changeset.

Severity, honestly

The multi-node population is unmeasurable from inside this repo (#8501 — no membership view), and single-node deployments cannot hit the window at all. The contract tightening is cheap and correct regardless; that is the whole justification.

Review state: draft + needs:contract-review (Clause-② yes — this changes a declared interface's accept behaviour). This PR stays gated; no ready flip, no auto-merge.

Generated by Claude Code

…-and-set (#11859)
INotificationOutbox.ack() takes back the claimed record; claim()/claimDigest()
declare ClaimedDeliveryRecord[] (the (claimedBy, claimedAt) pair the store
stamps, guaranteed present). The ack predicate now includes ownership, so a
late ack from a node whose claim was reaped and re-claimed matches nothing —
refused with the existing NotificationAckError DELIVERY_NOT_ELIGIBLE, writing
nothing. The caller never supplies an identity: ownership is proven by
round-tripping what claim() returned.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UQgPSniH1GFM9ZDeGyuGUa
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/automation/webhooks.mdx(via claimed_at (literal), claimed_by (literal), in_flight (literal))
  • content/docs/kernel/cluster.mdx(via claimed_by (literal))
What this run could not see
  • 1 changed file(s) yielded no anchor (packages/services/service-messaging/src/index.ts) — pages documenting those are invisible to this run
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 45 of 222 client-bound route-ledger rows — the other 177 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 177: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 107 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 — 4 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 6274a1aea55305cb85549aeb9dabef8e0e831565packageMentionDocs.

Which tree this was computed on

This run read content/docs from 79f9ded95d7a579ab29f379256c66558274e4c77 — the merge of head 8deb984b295b073eacf35b3fbbb5a31759110e4a into base 6274a1aea55305cb85549aeb9dabef8e0e831565, 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 79f9ded95d7a579ab29f379256c66558274e4c77 && git checkout 79f9ded95d7a579ab29f379256c66558274e4c77
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 6274a1aea55305cb85549aeb9dabef8e0e831565 8deb984b295b073eacf35b3fbbb5a31759110e4a && git checkout -B drift-repro 6274a1aea55305cb85549aeb9dabef8e0e831565 && git merge --no-ff 8deb984b295b073eacf35b3fbbb5a31759110e4a
node scripts/docs-audit/affected-docs.mjs --json 6274a1aea55305cb85549aeb9dabef8e0e831565

⚠️ 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 6274a1aea55305cb85549aeb9dabef8e0e831565 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-trumpClaude

Copy link
Copy Markdown
CollaboratorAuthor

Contract-review verdict: PASS (#11859)

Reviewed by the contract-review chain at CONTRACT_REVIEW_TIER. Tier fuse, machine reading this sub-round: get_session.external_metadata.last_served_model = claude-fable-5.

Reviewed: the full diff (all 10 files), the changeset, and the ruling record on #11859.

Conformance to ruling C (maintainer 2026-08-25, 「其他同意」 — the claim token rides the record; ack returns it; the cancellation surface stays deferred):

  • The credential is round-tripped, never supplied: claim()/claimDigest() declare ClaimedDeliveryRecord[] and stamp the (claimedBy, claimedAt) pair they just wrote; ack() takes the record back. Option A's caller-supplied identity and option B's required nodeId are demonstrably absent from the surface — the caller never learns its own node id. The pair (not the node id alone) is the credential, so a node's stale ack loses even to its own later re-claim.
  • The ownership test lives where it holds under the race: re-stated inside the conditional write (dispatcherAckCasOptions now requires claimed_by + claimed_at, so no caller can reach the CAS without the credential), with the read-half check kept only as the deterministic refusal. Both implementations enforce it identically; the runtime credential check also refuses JS callers and casts (list()-row pin) before any IO.
  • The ruled replay test is the four-step sequence driven through the public contract on both backends (the SQL leg on a real engine), asserting the refusal identity (NotificationAckError + DELIVERY_NOT_ELIGIBLE), what the refusal did NOT touch (row still B's claim, attempts intact), and that B's own ack then lands. The negative control (same sequence, no reap ⇒ A's ack succeeds) closes the refuses-everything hole; the claimedAt leg exceeds what the dispatch asked for.
  • Dissolution verification red exactly as predicted (4 failed, both controls + the INotificationOutbox has no cancellation, and ack() on an unclaimed pending row silently succeeds in both implementations #11453 suite green); reverse verification of the narrowed signature via a scratch consumer (TS2345) with a compiling positive control.
  • The spec red line held: error-code-ledger.zod.ts's prose mention is untouched and flagged for the spec seat. The deferred cancellation surface is untouched. Changeset minor + BREAKING with the one-line caller fix; ADR-0087 catch-all disposition argued honestly.

Landing completed by the chain in the same stroke per the maintainer's standing instruction (2026-08-25, verbatim: 「审核通过你应该直接负责合并吧,还要等原始的项目经理吗」): clearing needs:contract-review on this PR and #11859, flipping ready, arming the queue — CI green lands it.


Generated by Claude Code

@os-trump
os-trump marked this pull request as ready for review August 25, 2026 16:41
@os-trump
os-trump added this pull request to the merge queueAug 25, 2026
Merged via the queue into main with commit d9cf78eAug 25, 2026
42 of 43 checks passed
@os-trump
os-trump deleted the claude/issue-11859-outbox-ack-claim-token branch August 25, 2026 17:06
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

Development

Successfully merging this pull request may close these issues.

INotificationOutbox.ack() carries no nodeId, so its compare-and-set can prove a claim exists but not whose

2 participants

@os-trump@claude