You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
fix(service-messaging): enforce ack()'s claimed-row precondition in both outbox implementations (#11453) (#11858)
`ack()` is the dispatcher's completion callback for a row it CLAIMED, and
neither implementation checked that, so `ack(id, { success: false,
suppressed: true })` on an unclaimed `pending` row succeeded — flipping the row
terminal and recording an attempt that never went on the wire. That made `ack`
read like the cancellation primitive this interface deliberately does not have,
and it raced `claim()` (atomic by contract; `ack` was never part of that atom).
Both implementations now refuse a row that is not `in_flight`, with
`NotificationAckError` / `DELIVERY_NOT_ELIGIBLE` — this package's already
registered ADR-0112 code, the same refusal `SqlHttpOutbox.redeliver` raises
when its own compare-and-set misses. A refused ack writes nothing.
`SqlNotificationOutbox` does it as an atomic conditional update, not a
read-then-write: the precondition is re-stated in the write, which per #11009
must ride the predicate path (the by-id path silently discards it). `attempts`
increments inside that condition and nowhere else, so it can only move for a
row that was genuinely claimed.
The sibling HTTP outbox is untouched: `assertHttpRedeliverable` depends on
`IHttpOutbox.ack` incrementing unconditionally, so `attempts === 0` on a
terminal row still means "parked, never sent".
Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
`INotificationOutbox.ack()` enforces its declared precondition — the row must be claimed — in both implementations, and `attempts` moves only for a real dispatch attempt
6
+
7
+
`ack()` is the dispatcher's completion callback for a row it CLAIMED, and
8
+
neither implementation checked that. `MemoryNotificationOutbox.ack` looked the
9
+
row up by id and mutated it; `SqlNotificationOutbox.ack` read only `attempts`
10
+
by id. So `ack(id, { success: false, suppressed: true })` on an unclaimed
11
+
`pending` row succeeded, flipped the row to terminal `suppressed`, and
12
+
incremented `attempts` — which made `ack` read like the cancellation primitive
13
+
this interface deliberately does not have.
14
+
15
+
That was a trap in two directions. It **raced the dispatcher**: between a
16
+
caller's `list()` and its `ack()`, `claim()` could take the row — `claim` is
17
+
atomic by contract and `ack` was never part of that atom — so a suppression
18
+
could land on a delivery already on the wire, or a dispatcher's real outcome
19
+
could be overwritten by a caller that thought it was cancelling. And it
20
+
**corrupted `attempts`**: the counter feeds the retry schedule
21
+
(`classifyDeliveryAttempt(result, errorClass, row.attempts, …)`), so a row
22
+
"cancelled" this way arrived at its next real attempt with the backoff already
23
+
advanced by an attempt that never went out.
24
+
25
+
Both implementations now refuse an ack on a row that is not `in_flight`,
26
+
throwing `NotificationAckError` with this package's already-registered
27
+
ADR-0112 code `DELIVERY_NOT_ELIGIBLE` — the same refusal
28
+
`SqlHttpOutbox.redeliver` raises when its own compare-and-set misses. A refused
29
+
ack writes **nothing**: status, `attempts` and `error` are left exactly as they
30
+
were, so the row stays claimable and its backoff position stays honest. An id
31
+
matching no row remains a silent no-op — an absent row has no state to corrupt
32
+
and no claim to lose.
33
+
34
+
`SqlNotificationOutbox` does it as an **atomic conditional update** rather than
35
+
a read-then-write, because a read cannot hold a row still and a read-then-write
36
+
is the same defect wearing a different hat. The precondition is re-stated in
37
+
the write (`where: { id, status: 'in_flight' }`), which — per #11009 — must
38
+
ride the predicate path: on the by-id path the driver binds only the primary
39
+
key and the extra predicate is silently discarded. `attempts` is incremented
40
+
inside that condition and nowhere else, so the counter can only move for a row
41
+
that was genuinely claimed. A conditional write that matches nothing is
42
+
reported rather than passed off as success.
43
+
44
+
`NotificationDispatcher` absorbs exactly one refusal — `DELIVERY_NOT_ELIGIBLE`
45
+
— logs it and continues with the rest of the batch, because a send slower than
46
+
`claimTtlMs` legitimately loses its claim to the visibility-timeout reap, and
47
+
letting that unwind the partition loop would strand every still-valid row in
48
+
the batch `in_flight` until its own timeout expired. Any other error still
49
+
propagates.
50
+
51
+
The sibling HTTP outbox is deliberately untouched: `assertHttpRedeliverable`
52
+
depends on `IHttpOutbox.ack` incrementing `attempts` unconditionally, so that
53
+
`attempts === 0` on a terminal row still means "parked, never sent".
0 commit comments