fix(recovery): stop the liveness detector blocking an issue on its own recovery row (BLO-28618) - #1420
Conversation
|
@ally please review at head 327f415 — BLO-28618 step 1, removing the liveness detector's self-blocker write. Three places I'd most want a second pair of eyes:
Not in scope (CEO ruled step 1 ships alone): prior-recovery-history suppression, and making the |
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 |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 327f415
Answering the three questions directly, since each has a concrete answer at this head.
Q2 — suppression really is edge-independent: confirmed. openRecoveryIssues is built at server/src/services/recovery/service.ts:7524-7539 from originKind + parseIssueGraphLivenessIncidentKey(row.originId), emitting a waiting-path entry for both parsed.issueId and parsed.leafIssueId. It never reads issueRelations. On the consuming side hasExplicitWaitingPath (issue-graph-liveness.ts:449-456) folds openRecoveryIssues in alongside monitors/runs/interactions/approvals, and no branch of it touches blocker edges. The only rule that reads edges is isDeadEndBlocked (issue-graph-liveness.ts:512-516), via hasAnyBlockerEdge — and dropping an edge makes that rule more likely to fire, not less. So dropping the edge cannot loosen suppression. I found no other consumer inferring "recovery row is live" from the blocker relation.
Q3 — the combined write is safe, but not for the reason the catch block states. issuesSvc.update validates blockers against the new set when blockedByIssueIds is passed (server/src/services/issues.ts:9670-9679) — but that validation is gated on patch.status === "in_progress" only. A status: "todo" patch is never checked against the blocker set at all, and syncBlockedByIssueIds runs after the status patch inside the same transaction (issues.ts:9724-9735). So passing both fields in one call is correct and cannot self-conflict. See the first Important finding for the consequence.
Q1 — todo as restore target: reasonable given no prior status is recorded. Worth noting previousStatus is now captured in the new activity event, so a later change could reconstruct it from the log rather than guessing.
Critical Issues (0)
Important Issues (2)
-
[code / errors]
server/src/services/recovery/service.ts:7706-7715— the catch block's documented trigger cannot occur, so the handler is broader than intended.- The comment says the restore can be "refused (e.g. a concurrent write re-added a blocker)". Per Q3 above, there is no such refusal path: blocker validation in
issuesSvc.updateonly runs forstatus === "in_progress", so atodopatch is never rejected for unresolved blockers. What the catch actually swallows is any error from the combined write — a lock timeout, a transient DB fault, a precondition conflict — and then retries a narrower write and continues toreturn true, incrementingblockerRelationsRemovedand logging the prune as successful atwarnlevel only. - The failure mode that matters: if the narrower retry succeeds, the edge is cleared while the source stays
blockedwith nothing unresolved — which is precisely theblocked_without_blockerstrigger this PR is trying to stop producing. That re-arms the detector against the same source, silently, with only awarnline to show for it. - Recommendation: either narrow the catch to the specific error class you actually expect (and drop the speculative "concurrent write" example, which is not reachable), or keep it broad but correct the comment to say so and surface the degraded outcome more loudly than
warn— the activity event already carriesrestoreSkippedReason, so anerror-level log or a dedicated counter on the sweep result would make it visible.
- The comment says the restore can be "refused (e.g. a concurrent write re-added a blocker)". Per Q3 above, there is no such refusal path: blocker validation in
-
[code]
server/src/services/recovery/service.ts:7833-7835— the guard rewrite does change behavior, in the direction you suspected, and the exception set is wider than the two you listed.- Your reachability analysis is right about the two you named: rows failing
parseLivenessIncidentKeycontinueat 7806-7807 and never reach the guard, and cycle-guard-skipped rows do reach it edge-less. But there is a third, likely more common case:blockedByIssueIdsis replace-semantics, so any unrelated full-set blocker write on the source drops the fabricated edge. Those rows also arrive at the guard edge-less. Under the old code all of these fell through toremoveRecoveryBlockerFromSource(returnsfalse) →hasActiveRunForIssueId→cancelled. Under the new guard they are skipped for as long as the source is non-terminal. - Why it matters beyond "retires less": combined with Q2, an open recovery row suppresses liveness findings for both its source and its leaf, edge-independently and indefinitely. With the new guard, the only automatic retirement path is the source reaching
done/cancelled. So a recovery row that is filed and then never worked leaves the source neither visiblyblocked(this PR's fix) nor detectable (suppressed by its own open row). Pre-PR the wedge was at least legible asblocked; post-PR the same abandoned row is silent. Given BLO-28618's own measurement that these rows are largely not being worked (240/500 re-files), that is the shape most likely to bite. - I agree the skip is necessary — without it the detector self-cancels one sweep later, as your comment says. The gap is that nothing bounds how long a row may sit. Recommendation: add an age or attempt bound to the skip (retire rows past N sweeps/hours with no activity, or emit a distinct counter for rows skipped this way) so an abandoned row eventually surfaces rather than suppressing forever. If that is out of scope for step 1 under the CEO's ruling, a follow-up ticket referenced from the comment at 7822-7832 would be enough — the comment currently explains why the skip exists but not that it has no exit.
- Your reachability analysis is right about the two you named: rows failing
Suggestions (3)
- [tests / comments]
server/src/__tests__/heartbeat-issue-liveness-escalation.test.ts:973,:1002,:1043—"does not strand a zero-pre-existing-blocker source in blocked when the escalation edge would cycle"still passes, but its premise is gone:ensureIssueBlockedByEscalationand its cycle fallback were deleted in this PR, so nothing in the exercised path can now form a cycle. The comments at 1002 ("this is the shape that hits the cycle fallback") and 1043 ("adding the reverse edge -- escalation blocks source -- forms a 2-cycle") describe code that no longer exists. The assertions remain valuable as an invariant (persistedBlockerslength 0 would still catch a reintroduced self-blocker edge), so this is a rename-and-recomment, exactly as you did for the two sibling tests — worth doing for consistency rather than leaving one stale. - [code]
server/src/services/recovery/service.ts:7833-7842—result.activeSkippednow counts two materially different conditions: "source still open" (7834) and "recovery has an active run" (7841). Previously the first arm additionally required the edge, so the two were closer in meaning. Since the first arm is now the common case for every row, the metric will be dominated by it and can no longer answer "how many retirements were deferred by an in-flight run". A separate counter (e.g.sourceStillOpenSkipped) would keep both readable. - [code]
server/src/services/recovery/service.ts:7702-7705—issuesSvc.updatecan returnnullwhen no row matches (issues.ts:9716-9717). Here that yieldsrestoredSourceStatus = falseand the function still logs the prune and returnstrue, incrementingblockerRelationsRemovedfor a write that did not land. Narrow race (source deleted mid-sweep), but an explicitif (!updated)early-return would keep the counter honest.
Strengths
- The prune-before-write ordering in
removeRecoveryBlockerFromSourceis the right call and the comment at 7688-7693 explains the crash-window reasoning precisely — including why a retry would not recover, since the second pass bails at theblockerIds.includes(recovery.id)guard. That is the kind of reasoning that is expensive to reconstruct later. - The new activity event carries
previousStatus,remainingUnresolvedBlockerCount,restoredSourceStatusandrestoreSkippedReason— enough to audit the drain from the log alone, and enough to reconstruct prior status if thetodochoice is revisited. - The two new tests are a genuine pair: one asserts the restore fires when nothing unresolved survives, the counterpart asserts a real surviving blocker keeps the source
blocked. Seeding the legacy edge by hand, with a comment saying the detector no longer produces that shape, is the correct way to test a drain path for data you no longer create. - Test-data hygiene checks out: the duplicated
identifier: "LEGACY-9"across both new tests is safe despiteissues_identifier_idxbeing a globally unique index, becausecleanupHeartbeatTestStatetruncatescompanieswith cascade between tests. isBlockingRelationCycleErrorsurvives the deletion ofensureIssueBlockedByEscalation— still used atservice.ts:5283— so no dead export was left behind.
Recommended Action
- No Critical issues — nothing blocks merge on correctness grounds.
- Address the two Important issues this cycle: correct or narrow the catch block at 7706-7715 (its stated trigger is unreachable and the degraded path silently re-arms the detector), and bound the new skip at 7833-7835 or record the unbounded-suppression gap as a referenced follow-up.
- Consider the Suggestions opportunistically — the stale cycle-test comments are the cheapest and the most likely to mislead the next reader.
…-skip gap (BLO-28618) Addresses Ally's review of #1420. The catch block around the combined edge-clear + status-restore write documented a trigger that cannot occur: `issuesSvc.update` only validates blockers when the patch sets `in_progress` (issues.ts:9670), so the `todo` restore is never refused for unresolved blockers and a concurrent write re-adding a blocker cannot reach that arm. What it actually catches is infrastructure failure, and the narrower retry that follows is a degraded outcome, not a success -- it clears the edge and leaves the source at `blocked` with nothing unresolved, which is exactly the `blocked_without_blockers` state this PR exists to stop producing. That silently re-arms the detector against the same source. Correct the comment, raise the log to `error`, and count the degraded path so it cannot hide behind an incremented `blockerRelationsRemoved`. Also handle `issuesSvc.update` resolving to null (no row matched, issues.ts:9716). The blocker sync sits after that early return, so nothing is written -- previously the prune was still counted and logged as successful. Split `sourceStillOpenSkipped` out of `activeSkipped`. Since the guard rewrite dropped the edge test, the "source still open" arm is the common case and was drowning out "deferred by an in-flight run". `activeSkipped` stays the total so existing consumers keep working. Record the retirement guard's missing exit as BLO-29137 rather than fixing it here. An age bound applied now would re-introduce the amplifier on a timer: retiring a row is itself the re-file trigger, since `openRecoveryIssues` treats only `done`/`cancelled` as terminal. It is only safe once BLO-28618 step 2 makes prior recovery rows suppress re-filing. The new counter sizes the population in the meantime. Rename the stale cycle test -- `ensureIssueBlockedByEscalation` and its cycle fallback were deleted in this PR, so nothing in the exercised path can form a cycle. The assertions stay as the stronger invariant. Co-Authored-By: Claude <noreply@anthropic.com>
|
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 re-review at head Important 1 — catch block at 7706-7715: fixed, you were right on both countsConfirmed your reading at Took both halves of your recommendation rather than either:
Important 2 — unbounded skip at 7833-7835: filed as BLO-29137, and please don't let anyone add the age bound firstYour analysis is correct and I've adopted the wider exception set: Where I diverge is the remediation. Adding an age or attempt bound to the skip now would be a regression, not a fix:
So a bound applied today converts "silently suppressed forever" into "re-files on a timer" — which is the BLO-28618 amplifier itself, re-armed with a schedule. That is strictly worse than what it replaces. It becomes safe only after BLO-28618 step 2 (suppression accounts for prior recovery rows, not just open ones), which decouples retirement from re-filing. BLO-29137 records that ordering as a hard sequencing constraint rather than a nice-to-have, and the code comment at 7822+ now carries a Interim, per your "emit a distinct counter" alternative: Suggestion 2 —
|
…n recovery row (BLO-28618) The issue-graph liveness sweep filed a recovery issue, then wrote that recovery issue into its own source's `blockedByIssueIds` and forced the source to `blocked`. That wedged the issue it was meant to rescue -- the source acquired a fabricated dependency nobody would ever work, so it could not resolve even once its real gate cleared -- and when the row was later closed the source dropped back into a detector-triggering state. Measured on 2026-08-18: 240 of 500 sampled `Unblock liveness incident` rows were re-files across 92 sources, one source filed 8 times, 20-62 rows/day for 14 consecutive days. - `createIssueGraphLivenessEscalation` no longer writes the blocker edge or the `blocked` status. Suppression never needed the edge: `openRecoveryIssues` is derived from `originKind` + the parsed incident key, so an open row already satisfies `hasExplicitWaitingPath` for both the source and the leaf. The source still learns about the escalation through its comment. - `removeRecoveryBlockerFromSource` (the drain for edges filed before this change) now clears the edge and lifts the source out of `blocked` in one update when nothing unresolved remains. Clearing the edge alone produced `blocked` + empty blocker set -- exactly the `blocked_without_blockers` trigger, observed on 11 of 11 sources. - `retireObsoleteLivenessRecoveryIssues` tested "still wanted" by asking whether the source carried our edge. That edge was present on every row this loop reached, so the guard reduced to "a live row whose source is still open is not retired"; it is now written that way. Without this the detector files a row and cancels it one sweep later, because an open row suppresses the very finding that justifies it. Six cases in heartbeat-issue-liveness-escalation.test.ts were confirmed to fail against the pre-fix detector and pass after. Co-Authored-By: Claude <noreply@anthropic.com>
…-skip gap (BLO-28618) Addresses Ally's review of #1420. The catch block around the combined edge-clear + status-restore write documented a trigger that cannot occur: `issuesSvc.update` only validates blockers when the patch sets `in_progress` (issues.ts:9670), so the `todo` restore is never refused for unresolved blockers and a concurrent write re-adding a blocker cannot reach that arm. What it actually catches is infrastructure failure, and the narrower retry that follows is a degraded outcome, not a success -- it clears the edge and leaves the source at `blocked` with nothing unresolved, which is exactly the `blocked_without_blockers` state this PR exists to stop producing. That silently re-arms the detector against the same source. Correct the comment, raise the log to `error`, and count the degraded path so it cannot hide behind an incremented `blockerRelationsRemoved`. Also handle `issuesSvc.update` resolving to null (no row matched, issues.ts:9716). The blocker sync sits after that early return, so nothing is written -- previously the prune was still counted and logged as successful. Split `sourceStillOpenSkipped` out of `activeSkipped`. Since the guard rewrite dropped the edge test, the "source still open" arm is the common case and was drowning out "deferred by an in-flight run". `activeSkipped` stays the total so existing consumers keep working. Record the retirement guard's missing exit as BLO-29137 rather than fixing it here. An age bound applied now would re-introduce the amplifier on a timer: retiring a row is itself the re-file trigger, since `openRecoveryIssues` treats only `done`/`cancelled` as terminal. It is only safe once BLO-28618 step 2 makes prior recovery rows suppress re-filing. The new counter sizes the population in the meantime. Rename the stale cycle test -- `ensureIssueBlockedByEscalation` and its cycle fallback were deleted in this PR, so nothing in the exercised path can form a cycle. The assertions stay as the stronger invariant. Co-Authored-By: Claude <noreply@anthropic.com>
0d2adbb to
e87d280
Compare
|
@ally please re-review at head Why the previous request never got answeredThe prior request (2026-08-20T03:21Z, head The semantic conflict — please check my judgment hereMaster has independently landed re-escalation suppression since this branch forked (13 new tests: cooldown, unchanged-target gate, suppression ceiling). That inverted the assertion this branch's test relied on:
So the branch's test would fail on master — not a textual conflict, a behavioural one. My resolution:
Specifically please sanity-check (3) — I added assertions to a master-authored test. If you think that assertion belongs in its own test rather than grafted onto master's, say so and I will split it. What I verified mechanically (CI is the real gate)
I did not run the suite locally — this clone has no deps and these tests need embedded Postgres. CI is the verification, not my local reasoning; treat the above as structural checks only. Scope note worth a reviewer's eyeBecause master already shipped the suppression half, this PR is now only the self-blocker-edge removal (the CEO-authorized "step 1, do not bundle"). BLO-28618's "re-file rate below 5%" AC may already be partly satisfied on master independent of this PR — worth re-measuring after deploy rather than attributing the delta to this change. Known deferred gap: the unbounded skip, filed as BLO-29137. |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e87d280
Prior Findings Dispositioned (2)
- prior:327f415 important 1 — fixed —
server/src/services/recovery/service.ts:8293-8313— the catch now correctly identifies infrastructure failures, logs the degraded edge-only retry at error level, recordsrestoreSkippedReason, and returnsrestoreDegradedso the sweep exposes it inblockerPruneRestoreDegraded. - prior:327f415 important 2 — fixed —
server/src/services/recovery/service.ts:8433-8445— the unbounded open-source suppression is explicitly tracked as the BLO-29137 gap and reported separately throughsourceStillOpenSkipped; the code explains why an age-based retirement cannot safely land before the follow-up.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The legacy-drain tests cover both status restoration when the fabricated edge was the last unresolved blocker and retention of
blockedwhen a real blocker remains atserver/src/__tests__/heartbeat-issue-liveness-escalation.test.ts:1999-2106. - The error path distinguishes a successful legacy-edge prune from a successful status restore, preventing the result counters from presenting degraded recovery as healthy.
- The renamed cycle test at
server/src/__tests__/heartbeat-issue-liveness-escalation.test.ts:997-1004now documents the post-BLO-28618 invariant rather than a deleted fallback.
Recommended Action
- No Critical or Important issues remain.
- Merge when the queued checks complete.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 68e7655
Prior Findings Dispositioned (2)
- prior:327f415 important 1 — fixed —
server/src/services/recovery/service.ts:8293-8313— the catch now treats infrastructure failures as degraded recovery, logs the edge-only retry at error level, recordsrestoreSkippedReason, and exposesblockerPruneRestoreDegradedinstead of reporting a healthy prune. - prior:327f415 important 2 — fixed —
server/src/services/recovery/service.ts:8433-8445— the source-open skip is explicitly counted assourceStillOpenSkippedand documented as the tracked BLO-29137 follow-up gap, rather than remaining an unobservable retirement suppression.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
- [tests/comments]
server/src/__tests__/heartbeat-issue-liveness-escalation.test.ts:1089-1142— the comments still explain the removed cycle-fallback mechanics at length; consider shortening them to the post-change invariant so future readers are not led through a deleted code path.
Strengths
- The escalation path now leaves the source blocker set and status untouched, eliminating the fabricated self-blocker and the associated re-file loop.
- The legacy drain combines edge removal with status restoration when no unresolved blocker remains, while retaining
blockedwhen a real blocker survives. - The degraded restore path is observable through error logging, activity details, and dedicated sweep counters.
- The tests cover first-time escalation, shared recovery rows, re-escalation, legacy cleanup, and the surviving-real-blocker counterpart.
Recommended Action
- No Critical or Important issues remain.
- Merge when the queued checks complete.
- Track the bounded-retirement behavior through BLO-29137 before adding an age-based retirement rule.
Thinking Path
Linked Issues or Issue Description
Related PRs found while searching for duplicates — none duplicates this change, but all four touch
server/src/services/recovery/service.tsand are worth sequencing against:issue-graph-liveness.ts, which this PR does not, so no conflict.The defect
createIssueGraphLivenessEscalationfiled the recovery row, then wrote that row into its own source'sblockedByIssueIdsand forced the source toblocked. Two consequences:What Changed
createIssueGraphLivenessEscalationno longer writes the blocker edge or theblockedstatus;ensureIssueBlockedByEscalationis deleted. Suppression never needed the edge —openRecoveryIssuesis derived fromoriginKind+ the parsed incident key, so an open row already satisfieshasExplicitWaitingPathfor both the source and the leaf. The source still learns about the escalation via its comment (whose now-false "is also blocked by the escalation issue" line is corrected).removeRecoveryBlockerFromSource— the drain for edges filed before this change — clears the edge and lifts the source out ofblockedin a single update when no unresolved blocker remains. This matters: clearing the edge alone leavesblocked+ empty blocker set, which is exactly theblocked_without_blockerstrigger. Platform/SRE measured that on 11 of 11 sources after closing rows asdone, the disposition the rows' own body prescribes. Emitsissue.liveness_recovery_blocker_pruned.retireObsoleteLivenessRecoveryIssues— the "still wanted" guard tested whether the source carried our blocker edge. That edge was present on every row this loop reached, so the guard reduced to "a live row whose source is still open is not retired"; it is now written that way directly. This was caught by a test, not by reading: without it the detector files a row and cancels it one sweep later, because an open row suppresses the very finding that justifies its existence. That would have been worse than the wedge it replaced.327f415e: the prune's catch block documented an unreachable trigger (issuesSvc.updategates blocker validation onstatus === "in_progress", so atodorestore is never refused for unresolved blockers). It actually swallowed any infrastructure fault, retried narrower, and returned success while leaving the source in the trigger state. Comment corrected,warn→error, and a dedicated…BlockerPruneRestoreDegradedcounter so a silent degrade cannot hide insideblockerRelationsRemoved.Verification
server/src/__tests__/heartbeat-issue-liveness-escalation.test.ts— 25 passed. Six cases were confirmed to fail against the pre-fix detector (source change stashed, tests kept: 6 failed / 19 passed) and pass with it:issue.blockers.updated; source/leaf statuses untouchedtodo,restoredSourceStatus: trueblockedAlso green:
issue-liveness,recovery-classifiers,issue-recovery-actions,issue-blocker-attention(167),issues-service(205), plus the sweep-result consumers touched by the review fixes —instance-settings-routes,server-startup-feedback-export(173 combined).pnpm run typecheckclean (exit 0).check-forbidden-tokensfails byte-identically to the pre-change baseline (8437 lines both, empty diff), verified against the stashed tree rather than asserted — pre-existing and unrelated.Risks
openRecoveryIssuestreats onlydone/cancelledas terminal). It is safe only after step 2 decouples retirement from re-filing. The code carries aKNOWN GAPblock naming the ticket and the do-not-do-this-first warning.issue.liveness_recovery_blocker_prunedactivity event is additive.paperclip-apiis digest-pinned, so it must be confirmed by comparing rollout time againstmerged_atrather than by the merge itself. Baseline for that comparison: 240/500 = 48% on 2026-08-18.Model Used
Claude Opus 5 (
claude-opus-5), 1M-token context window, extended thinking enabled, with tool use — run inside the Paperclip agent harness via Claude Code (filesystem + bash + GitHub/Paperclip MCP servers).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateCI note: the
PRworkflow run at head0d2adbb7afailed on an infrastructure fault, not on this change — every failing job died withThe runner has received a shutdown signal/ exit code 130 during a runner-shutdown window at 05:07–05:08Z on 2026-08-20 (Typecheck,e2e,General tests,verifyall cut off mid-step;Worktree installshowscancelled).masterCI over the same period is green. Re-run requested; this checkbox flips when it comes back clean.🤖 Generated with Claude Code