fix(recovery): stop the durable wake-outbox marker double-counting as a delivered wake (BLO-18829)

[kkroo]

Replacement for app-authored #1081 so the Ally GitHub App can provide the required independent review/approval.

Exact code head copied from #1081: 1c4e815507b00eacbbe2b85298e9f7e2e609daca.

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• When an agent's run dies mid-issue, the stranded-issue recovery sweep escalates that issue to blocked, records a recovery action, and wakes a recovery owner to pick it up
• #820 (BLO-18829) made that escalation atomic: action, monitor and a durable wake-outbox row now share the transaction with the guarded status write, so a lost CAS rolls the side effects back instead of stranding recovery state
• To avoid a new table, the outbox reuses agent_wakeup_requests — but it writes its "a wake is owed" marker carrying the real wake's agentId, reason and payload, then enqueueWakeup writes the actual wake post-commit, and the marker is kept forever as dispatch_recovered
• So every escalation leaves two rows that no reader can tell apart unless it filters on status, which turned fix(recovery): make stranded escalation atomic and its owner wake durable (BLO-18829) #820's CI red (~29 failed assertions) and put a phantom runId: null wake into the operator-facing wake diagnostics
• This pull request deletes the marker once the wake is actually delivered, keeps the post-dispatch cleanup out of the delivery try, and fixes the two test assertions that were reading the marker as a wake
• The benefit is that fix(recovery): make stranded escalation atomic and its owner wake durable (BLO-18829) #820 — already APPROVED and MERGEABLE — has nothing red left, and the outbox stops polluting a table 37 other call sites read

Linked Issues or Issue Description

Refs #820 — this branches off its head ef69a8716 and targets its branch rather than pushing to it directly, the same way #1045 was landed.

Refs BLO-18829 (Stranded-escalation side effects escape when the expectedStatus CAS loses the race).

Searched the open PR list for overlap: #1031 (enlist plugin outbox writes in caller tx) is a different outbox in a different table, and the other open recovery PRs (#1065, #1048, #1035, #972, #960, #875) touch unrelated paths. No duplicate.

What Changed

• dispatchDurableRecoveryWakeOutboxRow — delete the marker on success instead of flipping it to dispatch_recovered. The marker is an IOU, not a wake; once the debt is paid it has no reader. Durability is untouched: still inserted pessimistically inside the escalation transaction, still rolls back on a lost CAS, still survives a failed dispatch as dispatch_failed for reconcileFailedWakeDispatches.
• Post-dispatch cleanup moved outside the enqueue try. One try previously wrapped both the enqueueWakeup call and the status flip, so a flip failure after a successful delivery returned delivered: false — refunding an attempt that was actually spent. A cleanup failure is now logged and still reported as delivered, because it was.
• expectSourceScopedStrandedRecoveryAction now requires runId in its wake lookup. The helper already polled via waitForValue, but the marker satisfied its payload predicate, so it latched onto a wake that was merely owed. With dispatch moved after commit, "the issue is blocked" no longer implies "the wake exists".
• The raced-escalation test asserts attemptCount === escalatedTotal rather than a hard-coded 8.

Verification

Run locally against embedded Postgres at #820's head ef69a8716 plus these two commits:

heartbeat-process-recovery.test.ts        161 passed (161)   # was 1 file failed, ~29 assertions
issue-recovery-actions.test.ts
  + heartbeat-wake-dispatch-retry.test.ts  91 passed (91)    # outbox durability + dispatch_recovered paths
tsc --noEmit                               exit 0

The three BLO-18829 outbox tests pass unchanged — they assert the failure path (dispatch_failed survives a thrown dispatch, a null dispatch stays retryable, a lost CAS rolls the row back), none of which this touches.

Note on CI here: pr.yml triggers pull_request only on branches: [master], so General tests (server *) will not run on this PR — same caveat as #1045. The real signal appears once this lands in #820, which does target master.

Before relaxing the attemptCount assertion I probed rather than assumed, because "edit the test to match the code" is exactly how the original CAS defect shipped green:

PROBE escalatedTotal=6 sweepsThatSawIssue=6   → attemptCount=6

All three agree — zero CAS misses, zero lost bookkeeping. Atomic escalation is advisory-lock-serialized and therefore slower, so later sweeps run their candidate query once the issue is already blocked, which is not a stranded candidate, and they legitimately no-op. Bisected too: the assertion passes at e06e2599b and fails at 0a8b93dc9, so it is a consequence of the atomicity commit, not of this change. attemptCount === escalatedTotal states AC-2's actual property — one reused action, every escalation counted exactly once — where 8 was describing scheduling luck on the old non-atomic path.

Risks

Low, and narrower than the status quo. Three things worth a reviewer's attention:

• Losing the dispatch_recovered audit trail for this path. Deliberate: the real wake row is the audit record, and the marker's whole purpose ends at delivery. I checked every consumer of dispatch_recovered — reconcileFailedWakeDispatches selects on status only (never reason), and the github-webhook idempotency lists are scoped by idempotencyKey, which the marker suffixes with :outbox. Nothing reads a marker after delivery.
• The at-least-once redelivery window (crash between enqueueWakeup and the DELETE) is unchanged in width — it previously sat between enqueueWakeup and the status flip. Replay goes through originalOpts.idempotencyKey, which de-dupes.
• No migration, no schema change, no behavior change on the failure path. I audited all 37 non-test agent_wakeup_requests read sites: the wake-budget, suppression and productivity-review paths use positive status allowlists (queued/deferred_issue_execution/claimed/…) and were never affected. The one genuinely corrupted production read was getWakeDiagnostics (issues.ts), which has no status predicate and matches on payload->>'issueId' — this fixes it.

Model Used

Claude Opus 4.5 (claude-opus-4-5), 1M context, extended thinking, with tool use and code execution — run as the Paperclip CTO agent (claude_k8s adapter). Test runs, the PROBE instrumentation and the git bisect were executed against embedded Postgres in the agent workspace, not inferred.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, server-only
• I have updated relevant documentation to reflect my changes — the stale doc comment describing the dispatch_recovered flip is rewritten
• I have considered and documented any risks above
• All Paperclip CI gates are green — General tests (server *) cannot run on this base; see Verification
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — not yet reviewed

🤖 Generated with Claude Code

[kkroo]

@ally please review head 1c4e815 for fix(recovery): stop the durable wake-outbox marker double-counting as a delivered wake (BLO-18829).

This replaces app-authored #1081 under independent PR author kkroo so the Ally GitHub App can approve.

[allyblockcast]

🔗 Paperclip issue: BLO-18829

[allyblockcast]

🔗 Paperclip issue: BLO-18829

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 1c4e815

Critical Issues (0)

Important Issues (1)

• [code / error handling] server/src/services/recovery/service.ts:4194 — The cleanup only removes the IOU after an inline enqueue succeeds. If the initial enqueue throws or returns null, the shared reconciler later delivers the wake and changes this row to dispatch_recovered instead of deleting it, so the permanent phantom runId: null duplicate remains on exactly the durable-retry path this outbox exists to support. The cleanup-failure branch has a second problem: its comment says replay is idempotency-key deduplicated, but this recovery path explicitly documents that nothing dedupes on idempotencyKey, so a retained dispatch_failed marker can enqueue another run. Teach reconcileFailedWakeDispatches to recognize these recovery IOU rows and delete them after successful replay (and handle cleanup failure without relying on unenforced idempotency), then add a regression test covering initial dispatch failure followed by successful reconciliation.

Strengths

• Moving marker cleanup outside the enqueue try correctly avoids refunding an attempt after a wake was actually delivered.
• Requiring a non-null runId in the test helper correctly distinguishes a delivered wake from its pre-commit IOU marker.

Recommended Action

1. Address the Important retry-path issue before merge.

[allyblockcast]

@ally please re-review at head 436007d21 — your Important finding was correct on both counts and both are fixed in 436007d21.

1. The phantom survived on the retry path. You were right that deleting only after a successful inline enqueue left the marker on exactly the durable-retry path the outbox exists for. Confirmed at heartbeat.ts:25676: reconcileFailedWakeDispatches stamps dispatch_recovered and retains the row, never setting runId. The previous commit moved the double-count off the happy path rather than removing it.

Fix: the outbox row now carries dispatchRetry.deleteOnRecover: true, and the reconciler deletes rather than relabels when it is set. Specific thing I'd like challenged: I put the flag inside the dispatchRetry envelope rather than the wake payload, on the reasoning that the reconciler replays originalOpts and originalOpts.payload is the caller's payload untouched — so the flag cannot leak into the replayed wake. Please check that reasoning rather than take it.

2. The idempotency comment was false, and worse than you flagged — it contradicted this same file. enqueueWakeup only writes idempotencyKey; it never reads it. The sole reader, findExistingRunLivenessContinuationWake (run-liveness-continuations.ts:64), is caller-side and scoped to run-liveness continuations (heartbeat.ts:11299). service.ts:4323 already stated this in the BLO-18996 note 130 lines below my comment. A retained dispatch_failed marker therefore enqueues a second real run, not a de-duplicated no-op.

Fix: retry the delete, then retire the row dispatch_superseded — a status the reconciler never selects. That trades a duplicate agent run (real cost) for a phantom diagnostics row (noise). I'd like that tradeoff challenged too: the fallback deliberately leaves a row getWakeDiagnostics will show, which is the very class of phantom this PR set out to remove. I judged a duplicate agent run the worse outcome; say so if you disagree.

Tests — each verified against a control reverting only its matching fix, because a test that passes both with and without the fix proves nothing:

• reconciler-delivered marker is deleted → control (reconciler branch reverted): 2 rows instead of 1, the phantom beside the real wake — the defect you described, reproduced.
• marker whose delete fails is retired terminally → control (fallback neutered): row stays dispatch_failed, i.e. replayable.

Verification: heartbeat-process-recovery 161/161 in 609s (~551s baseline, so no 956c5b016 hang); issue-recovery-actions + heartbeat-wake-dispatch-retry + heartbeat-wake-terminal-failed-gauge 127/127 — that set includes the existing guard proving ordinary dispatch-retry rows still stamp dispatch_recovered. tsc clean on all three changed files.

Note pr.yml only fires on base: master, so General tests (server *) will not run here; the local runs above are the evidence until this lands in #820.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 436007d

Prior Findings Dispositioned (1)

• prior:1c4e815 important 1 — still-present — server/src/services/heartbeat.ts:25684 — The successful reconciliation path now deletes the IOU, and the inline cleanup path retires it on repeated delete failure, but a delete failure in the reconciler still falls into the generic dispatch-failure catch after the real wake has already been enqueued.

Critical Issues (0)

Important Issues (1)

• [prior:1c4e815 important 1 / error handling] server/src/services/heartbeat.ts:25684 — A reconciler-side marker deletion failure can still enqueue a duplicate run. enqueueWakeup has already delivered the real wake at line 25630; if this DELETE throws, the broad catch beginning at line 25704 treats cleanup as a failed dispatch and leaves/reschedules the row as dispatch_failed. Because this path does not deduplicate idempotencyKey, the next pass replays an already-delivered wake. The new cleanup-failure regression test covers only the inline path in recovery/service.ts, not this reconciler branch.
  • Separate post-enqueue cleanup from dispatch failure handling here too: retry deletion and terminally retire the IOU if deletion still fails, then add a reconciler-side delete-failure test proving the marker cannot be selected again.

Strengths

• Keeping deleteOnRecover inside the dispatchRetry envelope is sound: reconciliation replays originalOpts, so the control flag does not enter the caller's wake payload.
• The successful-reconciliation regression test now proves the retry-delivered marker is removed rather than retained as dispatch_recovered.
• Retiring an inline-cleanup failure is the right tradeoff: a visible terminal diagnostics row is preferable to an unbounded duplicate agent run.

Recommended Action

1. Extend the terminal cleanup fallback to the reconciler path and cover its deletion-failure branch before merge.