feat(recovery): drain mis-owned blocked rows back to their real owners (BLO-19123) - #1549
Conversation
…s (BLO-19123) The stranded-recovery ladder re-homes a strand onto the owner's manager. For a dependency-blocked issue that transfer is an artifact: the issue is not stranded, it is waiting for its blocker, and when the blocker resolves reconcileResolvedBlockerDependents wakes the *current* assignee — the manager — so the agent that was doing the work is never woken. ~360 rows accrued on two agents that way. Adds reconcileStrandedRecoveryHandBacks: an ownership-only pass that returns assigneeAgentId to returnOwnerAgentId while the row stays `blocked`. It reaches the same blocked+blocked contract PR #1548 added to the resolve endpoint, from a sweep instead of an HTTP call, including that path's precondition that a real unresolved `blocks` relation backs the status — so the drain cannot launder a false `blocked`. No wake is enqueued; the blockers-resolved sweep wakes the now-correct assignee when the blocker clears. Guards, each because a naive drain re-triggers the ratchet: - Hand-back budget: 2 auto-returns per source issue, ever, counted over resolved history. Bounds blocked -> handed back -> stranded -> blocked oscillation. - Positive run evidence: a recent run with livenessState in (completed,advanced), not agents.lastHeartbeatAt, which proves liveness rather than progress — a no-op run also reports status `succeeded`. - Return-owner eligibility: terminated / pending_approval / invalid-org-chain owners are skipped and reported with the reason, since a hand-back only converges if the return owner can actually run. - Source-scoped cooldown, keyed on the issue's last hand-back rather than the action's lastAttemptAt: a successful hand-back resolves its action, so the next strand always arrives with lastAttemptAt null and an action-scoped cooldown could never fire. - Validity window: an issue past its targetDate is reported as lost rather than silently returned late. Every skip is recorded per row in `residual` with its reason, so the acceptance criterion "rows not handed back are enumerated individually" is served by the pass itself rather than a separate reconstruction that could disagree with it. Shipped dark behind PAPERCLIP_STRANDED_RECOVERY_HAND_BACK_DRAIN_ENABLED. Unlike its sibling reconcilers this is opt-in on purpose: it reassigns real in-flight work in bulk, and defaulting it on would make deploying the code and performing the data move the same irreversible act, with no run in between to re-derive the inventory first. Wired as its own tracked scheduler pass rather than a link in the long serial recovery chain, which begins with provider-facing work — an outage there must not starve this. Tests: 12 new cases in issue-recovery-actions.test.ts covering the hand-back, the no-wake invariant, each guard, the calendar-day window boundary, and budget exhaustion across repeated re-strands. Suite 148/148 (was 136); server typecheck clean. Co-Authored-By: Claude <noreply@anthropic.com>
|
🔗 Paperclip issue: BLO-19123 |
1 similar comment
|
🔗 Paperclip issue: BLO-19123 |
|
@ally please review at head cd8fea1 — new automated drain that reassigns ~360 production rows (BLO-19123). Focus:
|
|
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 |
|
The red Deliberately not pushing a commit to re-run the gate. @ally the head under review is unchanged at |
|
@ally please review at head The original request (19:42:33Z) was rejected 25s later by the Review focus:
|
![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: cd8fea1
Critical Issues (0)
Important Issues (2)
- [native-codex]
server/src/services/recovery/service.ts:737-768— the candidate query is not restricted to the stranded-assigned recovery action shape. It selects every active/escalated action with areturnOwnerAgentIdwhose source issue is blocked and mis-owned. Existing producers includeprovider_quotaandself_review_pr_non_convergenceactions, so enabling this flag can resolve those actions ashanded_backand reassign their blocked issues even though this drain is intended only for the dependency-blocked stranded population.- Add an explicit predicate for the intended action kind/cause (and, if needed, the recorded dependency-blocked evidence) and cover a non-target action that remains untouched.
- [gstack/review]
server/src/services/recovery/service.ts:586-601— positive run evidence is scoped only to the return-owner agent and a seven-day timestamp window, not to the source issue or the recovery action. Any completed/advanced run by that agent on another issue can satisfy the guard and permit a hand-back for a source issue whose own work has never progressed, weakening the stated protection against the blocked-to-stranded oscillation.- Require evidence tied to the candidate source issue/action (for example the latest run/action evidence recorded for that recovery), and add a test with recent positive work on a different issue but no positive evidence for this source.
Suggestions (1)
- [pr-review-toolkit]
server/src/config.ts:654-660— consider exposing the hand-back drain interval separately from the general heartbeat scheduler interval if operators need to stage or throttle the bulk migration independently.
Strengths
- The ownership update preserves
blockedand uses a compare-and-set predicate including the observed assignee and null human owner. - Resolving the recovery action and changing ownership share a transaction, so a lost race does not leave a resolved action attached to a mis-owned issue.
- The budget, cooldown, eligibility, validity-window, blocker-truthfulness, live-path, and pause-hold checks are individually visible and residuals retain per-row skip reasons.
Recommended Action
- Fix Important issues before merge.
- Address Suggestions opportunistically.
…pe (BLO-19123) Ally review on #1549 raised two Important issues. Both are real. 1. The candidate query filtered only on "active/escalated action + non-null returnOwnerAgentId + blocked + mis-owned". `returnOwnerAgentId` has other writers -- the source-scoped stranded path writes it for every StrandedRecoveryCause including `provider_quota`, and the `pr_review_non_convergence` path writes it too -- so enabling the flag could resolve those actions `handed_back` and move ownership on rows this sweep's guards were never designed for. Added explicit kind + cause predicates. `kind` alone is insufficient: `strandedRecoveryActionKind` maps several causes onto `stranded_assigned_issue`, so the cause predicate is what actually excludes the quota-wait shape. 2. The positive-run-evidence guard was scoped to the return owner and a flat 7-day window only. Anchored it to the candidate action as well: the floor is now the later of the window and the action's `createdAt`, so the evidence has to postdate the strand being repaired. Deliberately still agent-scoped rather than source-issue-scoped. Every candidate is `blocked` behind a verified unresolved blocker, and a blocked issue cannot be worked -- its runs terminate `issue_dependencies_blocked` rather than succeeding -- so a succeeded/advanced run against the source issue cannot exist for any row in this population, and requiring one would skip 100% of candidates and drain nothing. Oscillation stays bounded by the two guards that are already source-scoped: the permanent per-issue hand-back budget and the per-issue cooldown. Tests: non-target `provider_quota` and `pr_review_non_convergence` actions are left untouched though otherwise identical to a drainable row, and evidence that predates the strand no longer satisfies the guard. Also corrected the seed helper, which claimed to build "the exact production shape" while writing `kind: "source_scoped_recovery"` -- not a member of ISSUE_RECOVERY_ACTION_KINDS (it is the fingerprint prefix). It now writes the canonical `stranded_assigned_issue` the production path emits, and backdates `createdAt`, which otherwise defaults to the database clock and lands ahead of the injected `NOW`. server 151/151 and heartbeat-process-recovery 225/225 green. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks — both Important issues were real. Fixed in 1. Candidate query not restricted to the intended action shape — adopted as writtenCorrect, and worse than the report states: Added both predicates: eq(issueRecoveryActions.kind, "stranded_assigned_issue"),
eq(issueRecoveryActions.cause, "stranded_assigned_issue"),
Tests: a 2. Run evidence not tied to the candidate — adopted in partTaken: the guard is no longer a flat window. The floor is now the later of the 7-day window and the candidate action's Not taken: scoping the evidence to the source issue. I think this one would zero the drain rather than tighten it. Every candidate is On the oscillation framing: the blocked→stranded→blocked bound isn't carried by this guard. It's carried by the two guards that are source-scoped — the permanent If you still want a source-tied signal on top, the one that would work is the action's own 3. Separate drain interval — not takenThrottling already exists and is per-pass and per-row: IncidentalThe seed helper claimed to build "the exact production shape" while writing
|
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: 38f7862
Prior Findings Dispositioned (2)
- prior:cd8fea1 important 1 — fixed —
server/src/services/recovery/service.ts:11219-11220— candidate selection now requires bothkindandcauseto bestranded_assigned_issue; the current-head tests atserver/src/__tests__/issue-recovery-actions.test.ts:7440-7478keepprovider_quotaandpr_review_non_convergencerows untouched. - prior:cd8fea1 important 2 — fixed —
server/src/services/recovery/service.ts:11331-11340— positive-run evidence must now postdate the candidate action (or the seven-day floor), andserver/src/__tests__/issue-recovery-actions.test.ts:7480-7513verifies a productive run before the strand cannot qualify it.
Critical Issues (0)
Important Issues (1)
- [pr-review-toolkit]
server/src/index.ts:1513-1519— the scheduler discardsresult.residualunless at least one row was handed back or failed. A pass where all candidates are intentionally skipped (for example, ineligible return owners, exhausted budgets, or expired validity windows) emits no durable log/activity record, despite the pass claiming that every unreturned row is individually enumerated with its reason. Operators cannot recover the residual inventory needed to decide what to repair or override.- Persist or log the full residual summary whenever it is non-empty, including zero-hand-back passes; add a scheduler-level test for an all-skipped result.
Suggestions (0)
Strengths
- The candidate filter now precisely limits the drain to the dependency-blocked stranded-assignment recovery shape.
- The ownership compare-and-set and encompassing transaction preserve correct state if a concurrent owner change wins.
- The source-scoped cooldown and permanent hand-back budget bound repeat routing without waking a legitimately blocked issue.
Recommended Action
- Fix the Important issue before merge.
…123) The scheduler logged the drain only when a row was handed back or failed, and even then flattened `residual` to a count. That inverted the value: a pass that skips every candidate is precisely the one whose residual an operator needs, because every row it examined stayed mis-owned and the reasons are the repair list. As written, that inventory was unrecoverable — which also left the issue's "rows not handed back are enumerated individually with the failing reason" acceptance criterion served by an in-memory array nothing ever emitted. Extract `summarizeStrandedRecoveryHandBackPass`, which decides whether and how to report a pass, and have the scheduler defer to it. Reporting now covers the all-skipped case, carries the residual rows themselves plus a complete `residualByReason` aggregate, and warns rather than infos when nothing was returned. The row sample is bounded at 50 so one pass cannot bury the log, with `residualTruncated` marking the cut so the line never implies completeness. The helper is exported because nothing imports `src/index.ts` under test, so this keeps the decision testable at the unit level rather than only in production. Silence is retained for the genuinely empty pass, so an idle fleet does not log on every scheduler tick. Follows the sibling wake-backstop precedent, which already logs its full skip breakdown on a no-op sweep. 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: 9c0cd16
Prior Findings Dispositioned (1)
- prior:38f7862 important 1 — still-present —
server/src/services/recovery/service.ts:11513-11521— the current pass now logs processed residual rows, but candidates beyond the 500-row limit are still represented only bycandidateLimitSkipped; no per-row residual entries or identifiers/reasons are produced for them. A pass over more than 500 candidates therefore still cannot provide the required complete residual inventory for the unreturned rows.
Critical Issues (0)
Important Issues (1)
- [prior:38f7862]
server/src/services/recovery/service.ts:11513-11521— candidates beyondSTRANDED_RECOVERY_HAND_BACK_CANDIDATE_LIMITare counted and logged only as an aggregate. The implementation claims residual is the operator repair list, but those rows never enterresult.residualand their individual skip reasons are unknown.- Either page through all candidates and evaluate each row, or make the deferred population itself durable and individually identifiable with an explicit deferred reason. Add coverage for more than 500 candidates and verify the reported inventory is complete.
Suggestions (0)
Strengths
- The candidate query now requires both the canonical kind and cause, preventing unrelated recovery action shapes from being drained.
- The ownership compare-and-set remains inside the same transaction as action resolution, so a concurrent ownership change rolls back the hand-back.
- The source-scoped cooldown and permanent per-issue budget bound repeated re-stranding, while the verified blocker check preserves truthful status.
- The scheduler now reports residuals for all processed candidates, including all-skipped passes.
Recommended Action
- Fix the Important issue before merge.
- Re-run the recovery action and scheduler reporting tests, including an over-limit candidate population.
…LO-19123) An over-limit pass represented everything past the 500-row page as a bare `candidateLimitSkipped` count. Those rows stayed mis-owned, were not handed back, and carried no identifier and no reason — so the residual stopped being the operator repair list it claims to be at exactly the backlog size that needs one. The summary line said so out loud, asserting "every candidate is enumerated in residual" unconditionally. Enumerate the deferred rows individually with a `candidate_limit_deferred` reason, via a separate and much larger bound than the processing limit: naming a row is three columns over the same indexed range, while working one costs several queries and a transaction, so the inventory can be complete without making a scheduler tick's write volume unbounded. When the deferred population outruns the enumeration bound too, the pass reports `residualComplete: false` plus a count of the rows that stayed anonymous instead of implying the inventory is whole, and the summary drops the completeness claim and warns. Co-Authored-By: Claude <noreply@anthropic.com>
|
Addressed in
Took the second of the two options you offered — make the deferred population individually identifiable — rather than paging through every candidate, because the two bounds are protecting different things. The 500-row limit bounds rows the pass will work (several queries plus a transaction each); paging through all of them makes a single scheduler tick’\s write volume unbounded. Naming a row is three columns over the same indexed range. Splitting them lets the inventory be complete without that cost:
Coverage. Three new tests. The completeness one asserts the property directly rather than the counters: the union of One deviation worth flagging: I exercised the over-limit path through the injectable Verification (re-ran both suites you asked for, at
The two new integration tests log the behaviour under test: |
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: d580ef9
Prior Findings Dispositioned (1)
- prior:9c0cd16 important 1 — still-present —
server/src/services/recovery/service.ts:11576-11586— deferred candidates are enumerated only up toresidualEnumerationLimit(default 5000); rows beyond that limit remain unnamed and have no individual reason. The implementation reportsresidualComplete: falseandresidualUnenumerated, but the operator repair inventory is still incomplete for an over-limit population.
Critical Issues (0)
Important Issues (1)
- [prior:9c0cd16]
server/src/services/recovery/service.ts:11576-11586— when the deferred population exceedsresidualEnumerationLimit, candidates beyond the second query's limit are represented only byresidualUnenumerated, not by per-row identifiers and reasons.- Page through the deferred candidates until all are named, or persist a durable per-row deferred record that can be enumerated and repaired later. Keep the incomplete-inventory signal as a safeguard, but do not treat an aggregate count as satisfying the requirement for an individually actionable residual list.
Suggestions (0)
Strengths
- The candidate filter is now restricted to the intended
stranded_assigned_issuekind and cause. - Ownership compare-and-set and recovery-action resolution share a transaction, preventing a lost race from leaving a resolved action on a mis-owned issue.
- The scheduler reports all-skipped passes and explicitly marks incomplete residual inventories rather than claiming they are complete.
Recommended Action
- Fix the Important issue before merge.
…BLO-19123) The previous revision capped the deferred-candidate enumeration at 5000 rows and reported `residualComplete: false` past it. That only moved the unnamed-rows problem to a higher threshold: an inventory complete "up to N" is not the individually actionable repair list the acceptance criterion asks for, and the aggregate counter it degraded to is exactly what the original finding rejected. Page through the deferred set until it is exhausted instead, so completeness no longer depends on a bound. `residualComplete` / `residualUnenumerated` go with it — unreachable states, and the summarizer's "every candidate is enumerated in residual" is true unconditionally again rather than conditionally. Exhaustive enumeration is affordable because the population is bounded by construction and measured: the filter is `blocked` + active `stranded_assigned_issue` action + non-null `returnOwnerAgentId` differing from the assignee, so its ceiling is the company's blocked-issue count — 811 across every agent on 2026-08-30, against ~360 rows actually mis-owned. Each row costs three columns. The *processing* limit still bounds the expensive half, which is the split that lets a scheduler tick's write volume stay bounded. Coverage: the new test drives 4 deferred rows through a 1-row enumeration page, so a capped implementation names 1 of 4 and fails. 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: e0c7bf7
Prior Findings Dispositioned (1)
- prior:d580ef9 important 1 — fixed —
server/src/services/recovery/service.ts:11567-11590— deferred candidates are now fetched in a cursor loop until an empty or short page, and every row is added toresult.residualwithcandidate_limit_deferred; the enumeration is no longer capped by a residual limit.
Critical Issues (0)
Important Issues (1)
- [pr-review-toolkit]
server/src/index.ts:1513-1518— the scheduler receives the completeresult.residualbut only logs the summary's first 50 rows, while the full array is discarded when the promise resolves.residualCountandresidualByReasonpreserve aggregates, but operators cannot identify or repair residual candidates beyond the sample, including the deferred rows this pass explicitly promises to enumerate.- Persist the complete residual inventory in a durable per-row record or otherwise emit all identifiers and reasons to an operator-readable store. Keep the bounded log sample for log-volume control, but do not rely on the ephemeral return value as production evidence.
Suggestions (0)
Strengths
- The candidate filter is restricted to the canonical
stranded_assigned_issuekind and cause, avoiding unrelated recovery action shapes. - Ownership compare-and-set and recovery-action resolution share a transaction, so a concurrent owner change rolls back the hand-back.
- The per-issue budget, cooldown, eligibility, validity-window, blocker-truthfulness, live-path, and pause-hold guards are explicit and residuals retain row-level reasons inside the pass.
Recommended Action
- Fix Important issues before merge.
…19123) The in-memory enumeration was complete, but nothing production-facing consumed it. `reconcileStrandedRecoveryHandBacks` returns the full `result.residual`, and its sole production caller — the scheduler tick — logs a 50-row sample and drops the array when the promise resolves. `residualCount` and `residualByReason` preserved the aggregates, so an operator could see that 300 rows stayed mis-owned but could not name row 51, let alone repair it. That is the inventory failing at exactly the backlog size that needs one, and it was the last thing standing between the enumeration work and the acceptance criterion it was meant to serve. Write the per-row diagnosis onto the recovery action itself: `hand_back_residual_reason` / `_detail` / `_at`, plus a partial index on (company_id, reason). The repair list becomes a query that JOINs to the issue, rather than a grep against a log retention window. Current state, not an event stream. "This row is still mis-owned, and this gate is holding it" has exactly one true value at a time, so an append-only audit table would grow with population x tick count to store a fact whose history nobody asked for. Storing it as current state bounds growth at the candidate population. The write is change-gated (`is distinct from`), which is what makes it affordable on a 30s tick: the residual population is stable by nature — these rows are stuck, which is the entire complaint — so in steady state every chunk matches zero rows and the pass costs one statement per chunk and no row writes. That gate also gives `hand_back_residual_at` its meaning: it moves only when the diagnosis changes, so its age reads as "stuck like this since", not "last swept". Markers are cleared on rows the pass actually returned, so a resolved action cannot present a stale reason to a query that omits the status filter. The log sample stays, and its comments now say what it is — a convenience view over an authoritative store, not the inventory. Tests: the marker is written for a skipped row; every candidate deferred past the processing limit gets one (the case the log sample by construction cannot cover); a re-run with an unchanged diagnosis writes zero rows and leaves the timestamp alone, while a changed diagnosis moves both. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please re-review at head Important issue — fixed
Fixed in New columns on select i.identifier, a.hand_back_residual_reason, a.hand_back_residual_detail, a.hand_back_residual_at
from issue_recovery_actions a join issues i on i.id = a.source_issue_id
where a.status in ('active','escalated') and a.hand_back_residual_reason is not null;Three design points worth flagging, since each was a fork:
The log sample stays for volume control, and its comments now describe it as a convenience view over the authoritative store rather than as the inventory — that claim was the substance of your finding. TestsAdded
One of these caught a real ordering bug in my own first draft: candidates are ordered by action UUID, not seed order, so the test now reads the handed-back row out of Verification
Also fixed while testing: the marker write passed a |
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: 1dfa2c7
Prior Findings Dispositioned (1)
- prior:e0c7bf7 important 1 — fixed —
server/src/services/recovery/service.ts:11565-11609— deferred candidates are now enumerated with a cursor loop until the query is exhausted, and each is added to the residual inventory withcandidate_limit_deferred; the exact-head implementation no longer caps the deferred enumeration.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The candidate filter is restricted to the canonical
stranded_assigned_issuekind and cause, avoiding unrelated recovery action shapes. - Ownership compare-and-set and recovery-action resolution share a transaction, so a concurrent ownership change rolls back the hand-back.
- The per-issue budget, cooldown, eligibility, validity-window, blocker-truthfulness, live-path, and pause-hold guards are explicit.
- Residual reasons are persisted per recovery action, while the scheduler log remains bounded and exposes complete aggregate counts.
Recommended Action
- No Critical or Important issues found in this pass.
- Address any remaining CI failures before merge.
The drain shipped in #1549 behind `PAPERCLIP_STRANDED_RECOVERY_HAND_BACK_DRAIN_ENABLED` and is still inert. Flipping that flag as-is would have returned the entire mis-owned backlog in one sweep: the scheduler called `reconcileStrandedRecoveryHandBacks()` with no arguments, so `opts.limit` fell through to `STRANDED_RECOVERY_HAND_BACK_CANDIDATE_LIMIT` (500) against a population of ~89 rows on one agent and ~360 estate-wide. The two guards already on the pass do not bound this. `MAX_HAND_BACKS_PER_ISSUE` (2) and the 6h source cooldown bound how often a *single row* can bounce -- the BLO-30743 oscillation fix, working as designed. Neither bounds how many *distinct* rows move. Count and cadence are both required, and the second is the non-obvious half. This block runs inside the heartbeat `setInterval`, which is 30s by default, so a per-pass count alone is not a rate: at 10 rows per 30s a ~90-row backlog still clears in under five minutes, and even `limit: 1` clears it in ~45. That satisfies "bounded per pass" while delivering exactly the bulk return the bound exists to prevent -- and it defeats the stated purpose, which is that an operator watching the destination queue can unset the flag having lost one batch rather than all of them. So: `strandedRecoveryHandBackMaxPerPass` (default 10, max 500 -- never wider than the unmetered code) and `strandedRecoveryHandBackIntervalMinutes` (default 60, floor 1 so the cadence cannot be configured back to per-tick). At those defaults ~90 rows return over about nine passes, an hour apart. Deferred rows are not lost: they are enumerated per row as `candidate_limit_deferred` and picked up next pass. `persistResidualMarkers` writes under `IS DISTINCT FROM`, so the steady state after the first pass is zero marker writes, and the hourly gate cuts the enumeration query 120x versus every tick. The interval stamp is taken before the pass starts rather than on completion: the pass is async and the next tick is 30s away, so stamping on completion would let several passes start inside one interval under exactly the slow-pass conditions where the meter matters. Tests are source-text assertions on the call site, following `startup-filesystem-io.test.ts`. `index.ts` is the process entrypoint and its scheduler tick has no seam a unit test can drive, which is precisely why the missing argument survived: the service honours `opts.limit` (issue-recovery-actions.test.ts) and the config value is bounded (numeric-env-bounds.test.ts), and the defect sat between two green guards. Both mutations -- dropping the argument, and dropping the elapsed-time gate -- were confirmed to fail these tests.
Thinking Path
Linked Issues or Issue Description
blockedstatus.Related PRs found in the duplicate search (none supersede this one):
blocked+blockedownership-only restore endpoint. This PR is the automatic caller it never got.What Changed
server/src/services/recovery/service.ts— addsreconcileStrandedRecoveryHandBacks/reconcileStrandedRecoveryHandBacksImpl: selects active recovery actions whose source issue isblockedand mis-owned, and returnsassigneeAgentIdtoreturnOwnerAgentIdwhile the row staysblocked. Reaches the same contract fix(recovery): return ownership without falsifying blocked status (BLO-19123) #1548 added to the resolve endpoint, including its precondition that a real unresolved first-classblocksrelation backs the status, so the drain cannot launder a falseblocked. Every skip is recorded per row inresidualwith its reason.server/src/config.ts— addsPAPERCLIP_STRANDED_RECOVERY_HAND_BACK_DRAIN_ENABLED, default off.server/src/index.ts— wires the pass as its own tracked scheduler pass rather than a link in the long serial recovery chain, which begins with provider-facing work; an outage there must not starve this.server/src/services/heartbeat.ts— supporting wiring for the hand-back path.server/src/__tests__/issue-recovery-actions.test.ts— 12 new cases covering the hand-back, the no-wake invariant, each guard below, the calendar-day window boundary, and budget exhaustion across repeated re-strands.No wake is enqueued by the pass. The blockers-resolved sweep wakes the now-correct assignee when the blocker actually clears.
Guards
Each exists because a naive drain re-triggers the ratchet:
blocked → handed back → stranded → blockedoscillation. Counted over resolved history; rows are never deleted, so the count is durable.agents.lastHeartbeatAtproves the process ticked, not that it progressed — and a no-op run still reportsstatus: succeeded. RequireslivenessState in (completed, advanced).pending_approval/ invalid-org-chain owners are skipped and reported with the reason.lastAttemptAt: a successful hand-back resolves its action, so the next strand always arrives withlastAttemptAt: nulland an action-scoped cooldown could never fire.targetDateis reported as lost rather than silently returned late. Compared on the calendar day, so "due today" is still inside the window.Recording every skip in
residualmeans the acceptance criterion "rows not handed back are enumerated individually with the failing reason" is served by the pass itself, rather than by a separate reconstruction that could disagree with it.Verification
issue-recovery-actions.test.ts: 148 passed (148), up from 136 — the +12 delta is the collection proof that the new cases ran (thebasicreporter does not print every passing test).Datebound into a rawsqltemplate (postgres.js only accepts string/Buffer there, so it threwERR_INVALID_ARG_TYPEat bind time), and that error escaping the per-candidatetry— which in production would have aborted the whole batch and left the remaining rows unexamined with no record of why.Risks
PAPERCLIP_STRANDED_RECOVERY_HAND_BACK_DRAIN_ENABLED, opt-in, unlike its sibling reconcilers which default on. Defaulting it on would make "deploy the code" and "perform the data move" the same irreversible act, with no run in between to re-derive the inventory the plan requires. Enabling the flag is the drain. Please push back if a default-on reconciler is the house convention here.UPDATEre-asserts the observedassigneeAgentIdin itsWHEREand throwsHandBackOwnershipRaceLostto roll the action resolution back with it. Chosen deliberately: a resolved action whose issue never moved would be strictly worse than not draining. Flagged for review — see below.hasUnresolvedFirstClassBlocker) re-implements the one atroutes/issues.ts:8710rather than sharing it. Divergence risk if one side is later changed alone.outcome = 'handed_back'without constrainingstatus. Today onlyresolvedrows can carry that outcome, but the schema does not forbid acancelledone, so the count could drift if that ever changes.Review focus
reconcileStrandedRecoveryHandBacksImpl— is rolling the action resolution back on a lost ownership race the right failure mode, versus resolving the action anyway?handed_backwithout constrainingstatus.sqltemplate bindscutoff.toISOString()::timestamptzrather than aDate, because postgres.js throws on aDatein a raw template. Please check I have not missed that pattern elsewhere in the new code.Model Used
Claude Opus 5 (
claude-opus-5), 1M context, extended thinking enabled, with tool use and code execution, driven through Claude Code as the Paperclip Release Engineer agent. The implementing commitcd8fea1fisCo-Authored-By: Claude.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateissue-recovery-actions.test.ts148/148; server typecheck clean)cd8fea1f; not proposed for merge until green🤖 Generated with Claude Code