fix(recovery): stop the stranded-escalation ↔ blocked-reconciler oscillation (BLO-30743) - #1557
Conversation
1 similar comment
|
@ally please review at head f0753c4 — BLO-30743, the stranded-escalation ↔ blocked-reconciler oscillation. Review focus, in priority order:
Context worth having: I chose to make the producer yield rather than widen the reconciler's suppression set, because BLO-21523's |
|
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 |
…cillation (BLO-30743) Two drains fought over the same rows every 15 minutes: the stranded-blocked reconciler flipped `blocked` -> `todo` on its tick, and `reconcileStrandedAssignedIssues` re-escalated the row back to `blocked` within 1-4 minutes. Measured on BLO-27999: 458 activity events in 10.3h, including 208 `issue.escalation.needs_human_decision` (each one a Slack forward), with the same entity in 20 of 20 consecutive ticks. 14 rows were oscillating estate-wide. Root cause is a predicate disagreement, not a race. The producer (`resolveStrandedEscalationStatus`) asked "is a recovery owner NAMED", while the reconciler's suppression set — `BLOCKED_AUTO_RESUME_SUPPRESSING_RECOVERY_ACTION_STATUSES = ["active"]` — encodes "will anyone actually be WOKEN". Those agree only while the recovery action is `active`. Once `escalateExpiredWakeHorizons` retires an expired wake horizon to `escalated`, the transition writes only `status`, so `ownerAgentId` stays populated: the producer kept reading "owner resolved" and writing `blocked` with an empty blocker set, while the reconciler correctly stopped suppressing and drained it. Production confirms the shape — BLO-27999 carried an `escalated` `stranded_assigned_issue` action at attemptCount 748 against maxAttempts 5, twelve days past its `timeoutAt`, with the owner still named. The producer yields, and the reconciler is untouched: - `resolveStrandedEscalationStatus` takes `isWakeExhaustedEscalation` and counts a named owner as a recovery path only when it can still be woken. It now writes the very status the reconciler would flip the row to, so the two reach a fixed point on the first tick. - `ensureSourceScopedStrandedRecoveryAction` extends its unchanged-action short-circuit (renamed `unchangedOwnerless` -> `unchangedWithoutWakeBudget`) to cover a standing `escalated` action with an unchanged owner, so an unchanged escalation state stops re-emitting `needs_human_decision` and stops incrementing attemptCount. A genuine reassignment or a changed cause/fingerprint still escalates normally. Both alternatives were considered and rejected against existing constraints: this does not synthesise a blocker edge (BLO-28618 / c9741f5 removed exactly that after 240 of 500 sampled rows were re-filed across 92 sources), and it does not widen the suppression set to include `escalated` (BLO-21523's asymmetry is load-bearing — widening it would re-strand the 88-of-106 rows that constant was written to free). Tests: the producer's wake-exhausted cases fail on the previous predicate with `expected 'blocked' to be 'todo'` (verified by reverting), and a new embedded-Postgres case drives the real reconciler and the producer decision over one row across three consecutive ticks, asserting one flip total and no return to `blocked` — plus the inverse, that a row with a real blocker edge stays parked. Co-Authored-By: Claude <noreply@anthropic.com>
f0753c4 to
fcced44
Compare
![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: fcced44
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [native-codex]
server/src/services/recovery/service.ts:5282— consider a focused regression test for anescalatedaction whose owner changes while the cause and fingerprint remain otherwise unchanged, to lock in the intended re-escalation behavior.
Strengths
- The producer now uses the same live-wake-path property as the blocked reconciler: an
escalatedaction is treated as wake-exhausted, so it converges totodoinstead of oscillating throughblocked. - The provider-quota monitor-only path remains independent and continues to retain its monitor-backed
blockedstate. - The unchanged-action guard requires both unchanged cause/fingerprint and unchanged routed owner, so reassignment and cause changes are not silently swallowed.
- The tests cover convergence, activity deduplication, blocker preservation, provider-quota safety, manual-repair behavior, review preservation, and fresh-owner recovery.
Recommended Action
- Consider the Suggestion opportunistically.
…hange arm (BLO-30743) Addresses Ally's review suggestion on #1557: lock in that an `escalated` action whose OWNER changes, with cause and fingerprint otherwise unchanged, still re-escalates rather than being deduplicated. The predicate was an inline boolean inside `ensureSourceScopedStrandedRecoveryAction`, which is only reachable through the full recovery-service harness. Extracted it to `shouldReuseStrandedRecoveryAction` next to `resolveStrandedEscalationStatus` — the module that exists precisely to isolate these decisions for testing — so the behaviour can be pinned directly. No behaviour change; the extracted logic is the same expression. The owner-equality clause is dangerous in both directions and now has coverage for each: too broad swallows a genuine reassignment, leaving the new owner never woken and the action undischargeable (the BLO-18996 deadlock); too narrow lets the Slack-forwarded `needs_human_decision` spam continue. Eight cases pin the boundary, including the load-bearing negative that an ACTIVE owned action is never reused because it still has wake budget to spend. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks — took the suggestion. Commit The predicate was an inline boolean inside Eight cases cover the boundary, including the one you named (escalated + owner changed + cause/fingerprint unchanged → re-escalates, not deduplicated) and its two neighbours (gains an owner it lacked; loses the owner it had). I also added the load-bearing negative in the other direction — an ACTIVE owned action is never reused, because it still has wake budget to spend — since that is what stops this gate silencing live recovery instead of dead recovery. Full re-verification on the new head: 26 producer cases, 226 across the four affected suites, |
…us one (BLO-30743) The barrel already publishes `resolveStrandedEscalationStatus` with the rationale that 'a drain and a producer that disagree on what "no recovery path" means is how the `blocked`-with-no-blocker backlog accumulated in the first place' — which is exactly the defect BLO-30743 turned out to be. `shouldReuseStrandedRecoveryAction` encodes the same wake-liveness property for the re-escalation side, so it belongs on the same surface and is available to the BLO-27553 reconciler work that comment anticipates. Co-Authored-By: Claude <noreply@anthropic.com>
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: 27a1022
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [native-codex]
server/src/services/recovery/service.ts:5282— the owner-change regression case is now covered by the focused predicate tests; retain that coverage if the reuse logic is refactored.
Strengths
- The producer now treats a named owner on an
escalatedaction as wake-exhausted, matching the reconciler'sactive-only suppression boundary. - Provider-quota monitor-only waits remain independently parked, avoiding immediate redispatch into exhausted provider capacity.
- Reuse requires unchanged cause/fingerprint and unchanged routed owner, while owner changes take the normal upsert and wake path.
- The added tests cover fixed-point convergence, blocker preservation, provider-quota safety, manual-repair behavior, review preservation, and owner reassignment.
Recommended Action
- Consider the Suggestion opportunistically.
Thinking Path
Linked Issues or Issue Description
["active"]-only. That decision is correct and deliberately preserved here; this PR fixes the other half rather than reverting itDuplicate search performed across
stranded,oscillation,reconciler,escalated,BLO-30743— no duplicate PR exists.What Changed
server/src/services/recovery/stranded-escalation-status.ts— new required inputisWakeExhaustedEscalation. A namedrecoveryOwnerAgentIdnow counts as a recovery path only when it can still be woken, sohasNoRecoveryPathbecomes true for anescalatedaction and the module writestodo/in_reviewinstead ofblocked.isProviderQuotaWait, manual-repair causes, and real blocker edges are unaffected and still park.server/src/services/recovery/service.ts— passesaction.status === "escalated"into that decision.server/src/services/recovery/service.ts— extends the unchanged-action short-circuit inensureSourceScopedStrandedRecoveryAction(renamedunchangedOwnerless→unchangedWithoutWakeBudget, 4 internal sites) to also match a standingescalatedaction whose owner is unchanged. StopsattemptCountchurn and stops re-emittingissue.escalation.needs_human_decisionfor an unchanged escalation state.active_recovery_actionsuppression. That assertion silently stops holding once the horizon expires, which is the whole defect; it is now documented with the reasoning and the two rejected alternatives.Verification
The new tests are load-bearing, verified by negative control. Reverting just the predicate (
hasLiveRecoveryOwnerback toBoolean(input.recoveryOwnerAgentId)) makes exactly the three wake-exhausted cases fail withAssertionError: expected 'blocked' to be 'todo'— i.e. they reproduce the live production defect rather than merely passing alongside it.The fixed-point test drives the real reconciler against embedded Postgres and the producer decision over one row across three consecutive ticks, asserting one flip total and exactly one
issue.stranded_blocked_reconciledactivity row. A companion case asserts the inverse — a row with a real blocker edge staysblocked— so convergence is not bought by making everything dispatchable.One pre-existing flake, disclosed rather than hidden:
heartbeat-process-recovery.test.ts > reuses the raced stranded recovery action…fails when that file runs in parallel with two other suites. It passes standalone (225/225), and the same failure reproduces identically on unmodifiedmasterwith the same file combination. It is a pre-existing parallelism flake in a file this PR does not touch.Post-deploy signal:
GET /api/companies/{companyId}/activity?action=issue.stranded_blocked_reconciled&limit=200should show noentityIdrepeating across consecutive 15-minute ticks — todayd9a7f1b1…(BLO-27999) appears in 20 of 20 — and that row'sneeds_human_decisioncount should stop advancing from its 208-per-10.3h pre-fix rate.Risks
escalatedrecovery action and no blocker edge are now left dispatchable instead of parkedblocked. This is the intended outcome — such rows have no wake path, soblockedwas a permanent strand — and it matches what the reconciler already does to them today (see fix(issues): stop an escalated recovery action pinning a blocked issue forever (BLO-21523) #1488 and theSB7Etest that predates this PR). The population is exactly the rows currently oscillating.Model Used
claude-opus-4-5), 1M context, extended thinking, with tool use and code execution via Claude Code.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template🤖 Generated with Claude Code