fix(recovery): stop stranding recovery actions on backlog issues (BLO-25907) - #1345
Conversation
1 similar comment
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
…-25907) An assigned issue in `backlog` carrying an active recovery action was selected by no sweep: `reconcileStrandedAssignedIssues` filters to todo/in_progress/in_review and `reconcileStrandedRecoveryWakeBackstop` filtered to `blocked`. The action stayed active and the issue stayed silent — BLO-16074 itself sat that way for 27 days with `updatedAt == createdAt`. Widen the backstop to select `backlog`, but fold rather than wake it: `backlog` means "deliberately not dispatchable", so re-delivering an owner wake would contradict the park. Resolving the action keeps the active set honest without claiming a backlog issue is schedulable. Selecting it in the sweep rather than relying only on a write-time fold in `classifySourceRecoveryRevalidation` is what heals rows already parked — that classifier runs only when something writes to the issue, and the failure mode is that nothing does. The classifier gets a `backlog` branch too, so new parks retire at the moment of the park. Both sweeps' status filters are now exported constants whose union is asserted to cover every non-terminal `IssueStatus`, so adding a status without routing it to a sweep fails the suite instead of silently reopening the hole. Co-Authored-By: Claude <noreply@anthropic.com>
251ff9e to
dc14833
Compare
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally please review at head This PR was opened 2026-08-12 and received no review on either surface, then went Review focus:
Verification on the rebased tree: |
Thinking Path
Linked Issues or Issue Description
Closes BLO-25907 — the last open gap (gap 3) of BLO-16074.
Related PRs found in the duplicate search (all touch
server/src/services/recovery/service.tsorissue-recovery-actions.test.ts, all disjoint in intent — none of them widen a candidate filter or add abacklogbranch):What Changed
server/src/services/recovery/service.ts— backstop candidate filter widenedblocked→["blocked", "backlog"], with a fold branch at the top of the candidate loop that resolves backlog rowscancelled/cancelledwith a resolution note namingbacklog, counted asbacklogParkedResolved. Placed ahead of the owner/cause/cooldown gates deliberately: those gates decide whether a wake is worth attempting, and none of them apply to an action that will never be serviced at all.server/src/routes/issues.ts—backlogbranch added toclassifySourceRecoveryRevalidation, placed after the human-owner check so a human park keeps its more specific note. This retires the action at the moment of the park.RECOVERY_SWEEP_COVERED_ISSUE_STATUSESis their union — so the invariant test asserts the real filters rather than a copy of them.status: "blocked"; it now logs the candidate's real status.Why fold-plus-sweep, not fold alone
The issue offered three options. This takes option 3 (fold), but folded from a sweep as well as from the write path — not option 3 alone.
Option 3 as written only fires when something writes to the issue and trips
classifySourceRecoveryRevalidation. The failure mode is precisely that nothing writes to it, so a write-triggered fold cannot heal the already-stuck population. AC #2 is also explicitly an invariant over sweep candidate queries, so a sweep has to be able to selectbacklog.It just must not wake on it — that is what option 1 got wrong.
backlogmeans "deliberately not dispatchable"; re-delivering an owner wake would contradict the status.Verification
Red-first, per the acceptance criteria — each row was shown to fail against master's behavior before passing with the change:
blocked)checked: 0, backlogParkedResolved: 0, action staysactive["blocked"]expected [ 'backlog' ] to deeply equal []issue-recovery-actions.test.ts(rebased onfc5354a6→dc1483376)services/recovery/*+stranded-blocked-issue-reconcilerheartbeat-process-recovery.test.tstsc --noEmitThe second row matters: it proves the invariant test isn't vacuous — it names
backlogas the uncovered status when the filter is master's.The 80/80 is the load-bearing number after the rebase: 78 from this branch plus 2 from master's new
describe("scheduler-side failure heartbeat for routine executions")block (BLO-21395), which landed in the same trailing region of the test file. Both sides of that conflict execute, which is what makes the resolution verified rather than merely non-conflicting.An earlier run saw
heartbeat-process-recovery.test.tsfail one test (awaitForValue(..., 8_000)poll that took 13,775 ms). It does not reproduce on the rebased tree — 202/202 above. It was load, not this diff.Risks
backlogissue now resolves ascancelledinstead of remainingactive. That is the intended semantics — it makes the active set honest — but anything countingactiverecovery actions will see the number drop as the backlog population folds.backlogissue, so this cannot cause surprise runs or spend.classifySourceRecoveryRevalidationbacklogbranch must follow the human-owner check.recovery/service.tsmeans whichever lands second will need a rebase; the changes are in different functions, so no semantic conflict is expected.Model Used
Claude Opus 5 (
claude-opus-5), 1M context window, extended thinking enabled, with tool use and code execution via Claude Code.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatebacklogbehavior is documented in a comment on the changed function, per the issue's third acceptance criterionreviewgate🤖 Generated with Claude Code