[bug] Delivery outcome writes are unguarded — a slow attempt can resurrect a delivered event, bury a dead one, and stall the attempt counter

[serge-ivo]

claimDelivery (added deliberately, with a good comment explaining why) stops two sweeps from
starting the same delivery. But the two writes that END an attempt are unguarded, so a slow
attempt that outlives its claim can still corrupt the row after another attempt has finished with
it.

The gap

The claim is a CAS that pushes next_attempt_at forward by CLAIM_HOLD_MS (5 min):

UPDATE … SET next_attempt_at = ?2 WHERE id = ?1 AND status = 'pending' AND next_attempt_at IS ?4

Both terminal writes then key on id alone — no status guard, no claim identity:

-- markDelivered (:161)
UPDATE … SET status='delivered', attempts = attempts + 1, … WHERE id = ?1

-- markAttemptFailed (:176)
UPDATE … SET status='pending', attempts = ?2, next_attempt_at = ?3, … WHERE id = ?1

attempts is also read from the row the sweep SELECTed (connections.ts:391 passes
row.attempts), so it is a stale absolute value, not an increment.

Three consequences

1. A delivered event is resurrected and delivered again.

• t=0 sweep A claims D, starts the consumer (run_pipeline → Workflow). The attempt stalls.
• t=5m the hold expires, D is due, sweep B claims and succeeds → markDelivered.
• t=6m attempt A finally throws → markAttemptFailed → status='pending', due in ~1 min.
• t=7m sweep C delivers it a third time.

This is precisely what the module promises it prevents: "the consumer's irreversible work
(creating a site, spending tokens) can't happen twice." site-builder builds and bills twice;
site-deploy deploys twice.

2. A dead-lettered delivery is silently marked delivered. Reverse order — the failing attempt
dead-letters D, then the slow successful attempt calls markDelivered on it. The row leaves
status='dead', so it vanishes from ?status=dead and from replay. A real failure disappears.

3. attempts can stall, so MAX_ATTEMPTS is never reached. Two overlapping attempts both write
attempts = <same stale value> + 1. A delivery that keeps overlapping never advances past N+1 and
therefore never dead-letters — it retries forever, which is the opposite of the bounded
at-least-once contract.

Suggested shape

The codebase already solves this exact class three times, all with a guarded terminal write:

• setWorkCardProgress — guarded on status = 'running' so a late progress write can't resurrect
  a finished card.
• releaseSessionDriver — scoped to driver_id so a late release can't free someone else's claim.
• AgentDO.alarm's saveJob — re-reads and matches startedAt before writing.

Give the claim an identity (a claim token column, or CAS on the exact next_attempt_at the
claimer wrote) and have BOTH terminal writes require it plus status = 'pending'. Make attempts
a relative increment (attempts = attempts + 1) rather than a stale absolute.

Verification

• A late markAttemptFailed against a row already delivered changes nothing (0 rows).
• A late markDelivered against a row already dead changes nothing.
• Two overlapping attempts increment attempts twice, so the dead-letter bound still holds.
• Mutation-check: remove the guard and each test must fail — otherwise it isn't reproducing it.

Note on scope

The 5-minute re-execution window itself (consequence 1's first half) looks intentional — the
comment says a died-mid-flight attempt "simply becomes due again", trading possible re-execution
for recoverability. That trade is defensible and is not what this issue asks to change. The
corruption caused by the late writer is not part of that trade.

[serge-ivo]

Shipped in fa7b00e.

Verified: true. Both terminal writes keyed on id alone, and connections.ts passed row.attempts as a stale absolute into attempts = ?2. (One detail differed: markDelivered already used attempts = attempts + 1; only the failure path wrote the absolute. The stall you describe is real either way, since it's the failure path that gates dead-lettering.)

What shipped — the claim gets an identity, no migration

The hold claimDelivery writes is the claim token. Any completed attempt clears or rewrites next_attempt_at, so presenting a stale hold is precisely "someone else already finished with this row" — no new column needed.

• claimDelivery returns DeliveryClaim {id, hold} (or null) instead of a boolean; enqueueDelivery returns the same for the inline attempt, since its dueAt is that row's next_attempt_at.
• Both terminal writes require status = 'pending' AND next_attempt_at IS <hold>.
• attempts is now a relative attempts + 1 on every branch.
• markAttemptFailed gained a "stale" outcome; attemptDelivery returns it and logs a connection.delivery_stale warn — a delivery slow enough to outlive a 5-minute hold is worth seeing, not silently discarding. runDueDeliveries counts it.
• planNextAttempt(attemptsSpent, now) extracted as the pure decision.

Tests — lib/connection-outcome-guard.test.ts, executed against real SQLite (the 0058 migration read off disk), not SQL-string matching. None of your three consequences are visible in statement text; only running the statements shows them. One test per consequence, plus the normal single-attempt and retry paths so the guard can't be satisfied by simply refusing every write. Mutation-checked: loosening the guard to id IS NOT and restoring an absolute attempts turns exactly those 3 red.

Scope respected: the 5-minute re-execution window itself is untouched — a died-mid-flight attempt still simply becomes due again. Only the late writer's ability to corrupt the row is closed.

Left open: if a claim goes stale after the consumer's action already succeeded, the work has happened twice — the guard stops the row being corrupted, not the double execution. That's the trade the module already documents, and it's now at least visible in the trace as connection.delivery_stale rather than invisible.