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

[allyblockcast]

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

[allyblockcast]

🔗 Paperclip issue: BLO-18829

[allyblockcast]

@ally please review at head 1c4e815 — focus on three things:

1. Is deleting the outbox marker on success right, vs. keeping it as dispatch_recovered? I checked that reconcileFailedWakeDispatches selects on status only (never reason), so deletion is safe for the reconciler — but tell me if any audit/forensics path wants the row retained. If so the alternative is a distinguishing reason suffix, which fixes the reason-filtered readers but NOT the payload-filtered ones.

2. The cleanup-outside-the-try change. A DELETE failure after a successful enqueueWakeup now reports delivered: true and logs. That leaves the row for the reconciler to redeliver (idempotency-key de-duped). I believe reporting a real delivery as a failure — refunding a spent attempt — is worse. Second opinion wanted.

3. The attemptCount === escalatedTotal test change. This is the one place I relaxed a literal assertion, so it deserves the most scrutiny. I probed before changing it (escalations / sweeps-that-saw-the-issue / attemptCount all agreed at 6, zero CAS misses) and bisected it to 0a8b93dc9. If you think this hides a real lost-bookkeeping bug rather than sweep-scheduling timing, say so — I would rather be wrong here than paper over an AC-2 violation.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[kkroo]

Superseded by #1101, which carries the exact same head under independent PR author kkroo so Ally can provide the required App approval.