fix(service-messaging): enforce ack()'s claimed-row precondition in both outbox implementations

[os-sam]

Part of #11453 — the repair half only. The card's other half (there is no cancellation expression on INotificationOutbox) is deliberately not addressed here: the only named pull for cancellation, the hard recall() on #10753, was deferred by the #11454 ruling, so widening the interface now would be capability expansion with a deferred consumer. Triage ruled the cancellation surface gets decided when recall is revived, with this card's compare-and-set analysis as input. Closing the ack workaround is deliberate and not a loss — the card itself names it a trap. Left open rather than auto-closed so a human decides whether the cancellation section still wants a card.

⛔ Draft on purpose.needs:contract-review was hung at dispatch, not after the fact: this is accept/reject movement on a declared interface member. Not to be flipped ready, auto-merged, or have that label cleared by anyone but the contract-review tier.

The defect

ack() is the dispatcher's completion callback for a row it CLAIMED, and neither implementation checked that. MemoryNotificationOutbox.ack looked the row up by id and mutated it; SqlNotificationOutbox.ack read only ['attempts'] by id. So ack(id, { success: false, suppressed: true }) on an unclaimed pending row succeeded, flipped the row to terminal suppressed, and incremented attempts — which made ack read like the missing cancel, wrong in two directions:

• It raced the dispatcher. Between a caller's list() and its ack(), claim() can take the row — claim is atomic by contract and ack was never part of that atom — so a suppression could land on a delivery already on the wire, or a dispatcher's real outcome could be overwritten by a caller that believed it was cancelling.
• It corrupted attempts. The counter feeds the retry schedule (classifyDeliveryAttempt(result, errorClass, row.attempts, …)), so a row "cancelled" this way reached its next real attempt with the backoff already advanced by an attempt that never went out.

The shape chosen: loud rejection, not loud no-op

Both implementations refuse a row that is not in_flight, throwing NotificationAckError carrying DELIVERY_NOT_ELIGIBLE. Why rejection over a no-op: a no-op leaves the caller believing the row is suppressed while it is still queued and will still be delivered — replacing "silently succeeds" with "silently does nothing", which for the ack-as-cancel caller is strictly worse than the defect. Neither outbox holds a logger, so a "loud" no-op would have needed either a new constructor dependency or a return-value widening that a void-ignoring caller drops anyway. A throw is unmissable at the call site on the first run.

Why that code, and no new one:DELIVERY_NOT_ELIGIBLE is already registered to @objectstack/service-messaging in ERROR_CODE_LEDGER, and it is already this package's refusal for "this delivery row's state does not permit the operation" — SqlHttpOutbox.redeliver raises exactly it when its own compare-and-set misses. Two spellings for one concept would be a second thing for a caller to match on. It also keeps this PR out of packages/spec, which has zero ownership in this lane: minting a code would have required a ledger entry there, and an unregistered code would have needed a row in packages/runtime's dispatcher-error vocabulary. A registered code needs neither.

⚠️For the contract reviewer: the ledger's inline gloss for DELIVERY_NOT_ELIGIBLE reads "delivery row is in a non-terminal state", which is the redeliver instance of the concept. This ack refuses pending rows and already-terminal ones, so the gloss is now narrower than its uses. Widening that one comment is a packages/spec docs edit this lane may not make — flagged rather than done.

A refused ack writes nothing

Status, attempts and error are left exactly as they were, so the row stays claimable and its backoff position stays honest. An id matching no row remains a silent no-op, unchanged and now declared on the interface: an absent row has no state to corrupt and no claim to lose.

SqlNotificationOutbox: an atomic conditional update, not a read-then-write

A read-then-write is the defect wearing a different hat, so the precondition is re-stated in the write: where: { id, status: 'in_flight' }. Per #11009 that must ride the predicate path (multi: true) — on the by-id path the driver binds only the primary key and the extra predicate is silently discarded, which is the identical trap redeliver was carrying before that card. The write therefore moves from the update op to updateMany, exactly as redeliver's did, so its options now come from a new dispatcherAckCasOptions beside the existing helper. The tenant classification is unchanged — declared global, warrant re-derived in outbox-dispatcher-scope.ts; the op moved, the warrant did not. Of the three sites on these objects, only SqlHttpOutbox.ack still writes by id.

attempts is incremented inside that condition and nowhere else, so the counter can only move for a row that was genuinely claimed — i.e. for a real dispatch attempt.

Because IDataEngine.update declares its return as any, a miss is detected by reading the row back — the same technique redeliver uses. The detector is the pair (status, attempts), not status alone: a retry ack's post-state ispending, the same status a refused row already had, so only the recorded attempt tells them apart. Without the read-back a lost claim would write nothing and still report success — the silent-success family this card exists to close.

The dispatcher absorbs exactly one refusal

A send slower than claimTtlMs legitimately loses its claim to the visibility-timeout reap, so the dispatcher can provoke this refusal while behaving correctly. NotificationDispatcher logs DELIVERY_NOT_ELIGIBLE and continues with the rest of the batch; any other error still propagates. Without this, one lost race would unwind the partition loop and strand every still-valid row in that batch in_flight until its own timeout expired — turning a lost race into a batch-wide delay. Named here rather than left as an incidental: it is a caller change, and it is the only one.

Measured, not assumed: the sibling HTTP outbox

assertHttpRedeliverable (http-outbox.ts:315+) uses attempts === 0 on a terminal row to tell "parked, never sent" from "sent and failed", and its docstring says that pair is reachable only because IHttpOutbox.ack increments unconditionally. That predicate cannot be reached from this diff: it reads HttpDelivery on IHttpOutbox, whose implementations (memory-http-outbox.ts, sql-http-outbox.ts) import from http-outbox.js, http-sender.js, backoff.js, audit-timestamp.js and outbox-dispatcher-scope.js — and from none of outbox.ts, memory-outbox.ts or sql-outbox.ts. The HTTP outbox is deliberately left alone; applying the same attempts change there would have made attempts === 0 reachable through ack and destroyed the discriminator. dispatcherAckOptions is likewise untouched and still serves SqlHttpOutbox.ack by id.

Premise check

The card asked whether any live caller depends on ack-as-cancel today. None exists. Repo-wide, .ack( has zero callers outside packages/services/service-messaging/src, and inside it every call site is a dispatcher completion. The only producer of suppressed: true anywhere is backoff.ts:56 — classifyDeliveryAttempt classifying an invalid_recipient send outcome, i.e. a genuine attempt on a claimed row, which the contract test pins as still reaching suppressed. No compatibility shim was added, because nothing needs one.

Verification

Test-first: the contract test was written and run on an unmodified tree with the failure signature predicted in writing beforehand. Predicted and observed identically — AssertionError: promise resolved "undefined" instead of rejecting, 6 failed | 4 passed (10), the two still-works legs passing on both backends as the positive control that the harness really drives an ack.

outbox-ack-precondition.integration.test.ts is one table over both backends, because the precondition is a property of the interface and the two drifting apart is the failure it prevents. The SQL leg runs on a real ObjectQL + SqlDriver (better-sqlite3 :memory:, the #5704 ruled test backend) rather than a fake, because a fake engine cannot refuse a write. Assertions are identities, not counts: each names the row, its terminal state and its attempts value, and each refusal asserts the error identity (name + code) rather than that something was thrown.

Ablation, per implementation, restoring source from HEAD and verifying with git hash-object against the HEAD blob (both non-empty and matching):

leg	mutation	result
A	memory precondition removed	3 failed | 7 passed — the 3 memory refusal cases, SQL untouched
B	SQL guard + CAS + read-back reverted to the pre-fix by-id write	3 failed | 7 passed — the 3 SQL refusal cases, memory untouched

Each leg reds only its own backend's three cases, so neither implementation's pass is borrowed from the other. The mutation was confirmed on disk each way (anchored greps for both the injected marker and the removed text) before the run, and the script carried a restore trap so a mid-mutation kill could not leave the tree poisoned. No rebuild is needed for these legs and that is measured, not assumed: the contract test imports its subjects by same-package relative specifier, so vitest runs src/ — visible in this branch's own history, where behaviour changed between runs with no build in between.

Gates, all at 0b765a45, each read from the gate's own verdict line with the exit code captured before any pipe:

• check:driver-memory-census — OK — every declaration is ledgered … Nothing here invests in the driver (#5499 freeze). Run explicitly: the derivation reports it unreachable by construction, so it is in no path-derived union.
• check:type-check-debt — narrowing measured, not declared: @objectstack/service-messaging is absent from both DEBT and TEST_DEBT (grep exit 1, 0 hits, against a positive control of @objectstack/core and @objectstack/plugin-approvals), and the package's tsconfig.json includes src, where its tests live — so the ledger's "tests hidden from tsc" trap does not apply and the package typecheck really does cover the new test file.
• check:type-check-coverage — OK — 65/78 workspace packages type-checked.
• check:engine-double-contract — OK — 401 pinned, 133 in the DEBT ledger, 2 exempt (this diff edits a fake engine double).
• check:cross-package-test-inputs — OK: 16 package(s) read outside themselves, all declared (the new test reads across packages).
• check:where-matcher — 296 matcher(s) discovered … 0 silently-wrong … none new.
• check:test-source-alias, check:type-source-resolution, check:published-files, check:slot-lookup, check:query-options-erasure, check:nul-bytes, check:changeset-gate-self-tests, check:objectui-changeset, plugin-teardown-shape, empty-changeset, changeset-no-major, adr-0087-registration, release-rehearsal-clone self-test — all exit 0.
• check:i18n — first run returned PREREQUISITE NOT MET — the workspace CLI is not built, which is not measured, never a pass; after building the CLI it returns a real OK (9 package(s) — all bundles in sync, no undeclared authoring keys).

Package suite: 28 passed (28) files, 286 passed (286) tests, and pnpm --filter @objectstack/service-messaging typecheck clean (script name echoed in the output, so it is not a zero-match silent pass).

The gate list was re-derived from the actual changeset with scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack rather than taken from the dispatch, which is how the test-kind gates above (engine-double-contract, cross-package-test-inputs, where-matcher, query-options-erasure, i18n) were found — none were named at dispatch.

Follow-ups filed, not smuggled in

• ack(id, result) carries no nodeId, so the compare-and-set can verify that a claim exists but not whose. A row reaped and re-claimed by another node in the read→write window still matches status = 'in_flight'. Closing that needs a signature change — a contract decision, not a repair.
• The dispatcher's "channel not registered" branch acks a claimed row with no send attempted, so it still records an attempt for a delivery that never reached the wire — the notification-side twin of what IHttpOutbox.recordUndeliverable exists for. Nothing on this side reads that value the way assertHttpRedeliverable does, so closing it today would mean declaring a discriminator with no consumer.

---

Generated by Claude Code

[github-actions]

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-messaging, touching 10 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.

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

• content/docs/automation/webhooks.mdx(via in_flight (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: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

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 589758d22ccacf9cc56b5bc8a9f9766cb7e2a93a → packageMentionDocs.

Which tree this was computed on

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

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

[os-sam]

docs-drift answered — no prose goes false, and the one row it listed is a shared-vocabulary collision between the two outboxes

domain:services PM seat (session session_01APWX2AwT3a4xDcjPCe8bk4). Measured on origin/main. PR stays draft; needs:contract-review untouched.

The one page named: content/docs/automation/webhooks.mdx, via the in_flightliteral

That page is about the HTTP outbox, measured rather than assumed:

grep over the page	hits
sys_http_delivery / "http outbox" / IHttpOutbox	20
INotificationOutbox / "notification outbox" / sys_notification	0

Every in_flight occurrence belongs to sys_http_delivery's lifecycle — the schema table (:205), the dispatch diagram (:244), the claim SQL (:402–:405), and the crash-recovery table (:614).

⇒ This PR deliberately leaves IHttpOutbox alone, and proves it by import graph rather than by intent: assertHttpRedeliverable is unreachable from this diff, and dispatcherAckOptions still serves SqlHttpOutbox.ack by id. Nothing on that page changes.

⭐ The row is a true match on its own rule and a false one on relevance: the two sibling outboxes share their vocabulary — in_flight, attempts, and even DELIVERY_NOT_ELIGIBLE. That is not a bot defect; it is the corpus being genuinely ambiguous at the literal level. ⚠️ And it is the same resemblance that made this PR's blast-radius measurement load-bearing: the two subsystems look alike enough that a careless attempts change would have crossed over and destroyed assertHttpRedeliverable's attempts === 0 discriminator.

⚠️ The bot's declared blind spot — checked, and it is where the interesting thing was

It flagged packages/services/service-messaging/src/index.ts as yielding no anchor. That file carries the new exports, so I searched the docs for them directly:

• NotificationAckError → zero hits in content/docs.
• DELIVERY_NOT_ELIGIBLE → three hits, and none of them goes false:
  • references/api/error-code-ledger.mdx:239 and references/api/contract.mdx:135 — bare bullets in a code list, no gloss, and both files carry ⚠️ AUTO-GENERATED — DO NOT EDIT. Run build-docs.ts to regenerate.
  • automation/webhooks.mdx:552–556 — the HTTP redeliver endpoint's409 refusal list, i.e. the SqlHttpOutbox.redeliver instance of the concept, which this PR does not touch.

⇒ The narrow-gloss concern this PR raises for the contract reviewer — that the ledger's inline gloss ("delivery row is in a non-terminal state") is now narrower than its uses, since this ack refuses pending rows and already-terminal ones — is confined to the in-repo ledger comment. It has not propagated into any hand-written published page. Correctly flagged rather than done: widening it is a packages/spec edit and this lane has zero ownership there.

⇒ No docs work outstanding on this PR. Recorded so the contract reviewer does not re-derive it and a later reader does not read the bot's row as unaddressed.

---

Generated by Claude Code

[os-sam]

Landing — contract review CLEARED (checked which, not assumed), CI green

domain:services seat (session session_01APWX2AwT3a4xDcjPCe8bk4). Flipping ready and arming auto-merge; it holds for green.

The label-absence test, run rather than skipped

needs:contract-review came off #11453. That absence has two meanings and only one is safe:

PASS + label absent = CLEARED. No PASS + label absent = DROPPED (#10778) — re-hang nothing, ask.

Checked: contract review PASS at 22:39:33Z, PR #11858 at head 0b765a45, tier verified by fuse (get_session.external_metadata.last_served_model = claude-fable-5 against dispatch-gates.mjs:3070 read fresh from origin/main), label cleared by the reviewer in the same stroke. ⇒ CLEARED. The head is unchanged since the PASS, so there is no delta to re-assess.

CI: 31 checks — 29 success, 2 skipped, 0 non-green, including Type Check · debt ledger and Lint & Repo Gates, and all six Test Core shards.

⭐ The open question is ruled — and it lands on this seat, not on this PR

The reviewer ruled B on the DELIVERY_NOT_ELIGIBLE reuse: the reuse itself is right ("one concept … on two delivery surfaces, exactly what ADR-0112's closed set exists for"), and option C's second near-synonym code is rejected for the report's own vocabulary-sprawl reason. What remains is that the ledger's inline gloss (error-code-ledger.zod.ts:511, the redeliver-flavoured wording) is now narrower than the code's uses — this ack refuses pending rows and already-terminal ones.

⇒ "a one-line spec-lane docs catch-up, for the dispatching seat to card" — that is mine, and I am carding it rather than carrying it.⚠️packages/spec has zero ownership in this lane, so it is filed for the spec lane and not dispatched from here. ⛔ Not folded into this PR: it was flagged rather than done, which was the right call.

Card disposition — Part of, and #11453 does not close on this merge

The reviewer confirms the spelling: "no cancel member (part 1 stays the card's open half — Part of, not Fixes, is correct)." The card's part 1 — that INotificationOutbox has no cancellation expression at all — is untouched here, deliberately: the only named pull for it (the hard recall() on #10753) was deferred by the #11454 ruling, so widening the interface now would be capability expansion with a deferred consumer.

⇒ After merge I will set #11453's remaining half to its real state rather than letting a half-answered card sit as pm:dispatched. ⛔ A card closed on half its question is invisible residue; a card left dispatched after its dispatched half landed is the same failure wearing the other hat.

What made this one land clean

Two things the reviewer singled out that were measured rather than argued:

• The CAS rides the predicate path (multi: true via dispatcherAckCasOptions), "correctly dodging the 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 by-id predicate-discard trap that redeliver once carried" — a landmine avoided by reading the sibling's history rather than by luck.
• The read-back detector keys on the (status, attempts) pair, because a retry ack's post-state ispending — the same status a refused row already had. Status alone would have been a detector that cannot detect.

And the residue was filed, not buried: #11859 and #11860, both by the implementer, both searched-first.

---

Generated by Claude Code