fix(dispatch): unify queued-run claimability into isRunClaimable (BLO-22091) - #1276
Conversation
…-22091) Five review rounds on BLO-21792 each found dispatch and claimQueuedRun disagreeing about which queued rows are actually claimable, because claimability was re-derived independently at four sites. Round 5 (leak 5) was fixed inline on PR #1022 via isEffectivelyDependencyReadyForDispatch, but that only unified the dependency-blocker screen across the three priority lanes and dispatchRank's ready computation - claimQueuedRun's own gate still re-derived the same boolean locally, and the terminal-issue / k8s-isolation-retry screens were still three independent, only-by-convention copies across the lanes. This introduces isRunClaimable as the one predicate every priority lane's eligibility test consumes for all three claim-time screens (terminal status, isolation-retry deferral, dependency-readiness). claimQueuedRun's blocker gate now calls the same isEffectivelyDependencyReadyForDispatch function isRunClaimable is built on, instead of re-deriving the blocker-count/interaction-wake boolean inline. dispatchRank's own ready computation gets a narrower sibling, isDispatchRankReady, rather than the full isRunClaimable: folding the terminal-issue screen in there would have reintroduced an unbounded wait for a *different* exemption - evaluateQueuedRunStaleness's resumeIntent/ wake-comment reopen path, which the main scan already prunes for correctly upstream. isDispatchRankReady closes the concrete gap that existed instead: an isolation-retry-deferred row that was otherwise dependency-ready read `ready: true` at the rank site (missing screen 2 entirely), so once it aged past the 6h absolute floor it ranked at the very front of the queue. Because such a row is critical-priority it was already marked as emergency-lane work, so the claim loop's refusal handler aborted the whole dispatch pass on it rather than falling through to the next candidate - a self-healing but real one-pass stall on every affected agent, every pass, for as long as the row stayed deferred. New test in heartbeat-dispatch-priority-sort.test.ts asserts a "resume_critical_lane" reschedule never fires for this scenario - checked at the point the regression actually happens (a reschedule event fired synchronously by the scheduling call), not after a wait, since the existing dispatchDeferredRunIdsByAgent retry-exclusion self-heals the outcome within one extra pass and would otherwise mask the bug from an eventual-consistency assertion. Confirmed failing against pre-fix source and passing post-fix. Both existing dispatch-priority-sort and queued-backlog-convergence suites pass unmodified in intent (42/42). Co-Authored-By: Paperclip <noreply@paperclip.ing>
1 similar comment
![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: b878848
Looks good. The shared claimability checks consistently keep isolation-retry-deferred work out of dispatch ranking while retaining the resume-intent terminal-status exception.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
isRunClaimableconsolidates the three lane eligibility checks without altering their terminal-status behavior.isDispatchRankReadynarrowly adds the missing isolation-retry screen while preserving the resume-intent-aware staleness handling.- The regression test asserts the first-pass reschedule behavior, which distinguishes the previous false-ready ranking from eventual self-healing.
Recommended Action
- Merge after the required CI checks pass.
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: 39c944b
Looks good. The claimability consolidation prevents isolation-retry-deferred rows from receiving the absolute starvation rank while preserving the existing terminal-status and interaction-wake behavior.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
isRunClaimablecentralizes the lane eligibility screens and the ranking path separately preserves the intentional terminal-status exemption.- The regression test observes synchronous continuation scheduling, distinguishing the faulty first-pass refusal from eventual self-healing dispatch.
- The claim path continues to reject isolation-deferred runs before side effects, while fresh critical work can be selected directly.
Recommended Action
- Merge after the required CI checks pass.
Production deploy stuck 38h+ on this PR's fix — needs @eyad-hussein or @MohamedElmdaryThis PR merged as Current state of the
Ask: either approve the pending deployment (understanding it targets a stale commit — a fresh dispatch against current master would be needed regardless), or explicitly decline/reschedule so this stops silently blocking. Tracked on Paperclip at BLO-26633. |
Correction: approve run 31656073701, not 31615580214Following up on the comment above, which named the wrong run. Verified ancestry via
I have dispatched a superseding run targeting current master, which is a strict superset of both:
Blast radius, so the click is informed — this is a large batch and that is the reviewer's call, not mine:
Ask (unchanged, only the target changed): I deliberately did not cancel 31615580214, so it remains as a fallback if you would rather ship the smaller batch first. Tracked at BLO-26718 and BLO-26633. |
Correction: old run cancelled — it was head-of-line blocking the new oneCorrecting my comment above, where I said I was leaving 31615580214 alive as a fallback. I've cancelled it, because keeping it alive meant the corrected run could never reach the approval gate. The group: deploy-${{ vars.PAPERCLIP_HELM_RELEASE || 'paperclip' }}-${{ vars.PAPERCLIP_NAMESPACE || 'paperclip' }}
cancel-in-progress: falseA run parked Nothing was lost: Net effect — one unambiguous, actually-clickable ask: approve 31656073701 ( Mechanism and the suggested durable fix are written up on BLO-26718. |
Thinking Path
Linked Issues or Issue Description
Fixes: BLO-22091 — "Redesign the dispatch anti-starvation guarantee around one shared claimability predicate — five consecutive leaks say the mechanism, not the patches, is wrong"
Related: BLO-21792 (the five-round series this escalated from), PR #1022 (round 5's interim fix), BLO-19881 (fleet-wide starvation this unblocks)
What Changed
isRunClaimable(contextSnapshot, issue, readiness, now)— the single predicate for "is this queued run's issue actually claimable", covering the same three screensclaimQueuedRunapplies to a routine queued run: non-terminal issue, not k8s-isolation-retry-deferred, and dependency-ready-or-interaction-wake-exempt (via the existingisEffectivelyDependencyReadyForDispatch).foundReadyCritical), Lane B (foundReadyRecovery), and Lane C (foundReadyAbsolute) all now callisRunClaimableinstead of each re-deriving the same three-screen combination inline. Lane C's doc comment (previously asserting a property the code didn't fully have) is rewritten to match what the code now actually does.claimQueuedRun's dependency-blocker gate now callsisEffectivelyDependencyReadyForDispatchdirectly — the same functionisRunClaimableis built on — instead of manually re-derivingunresolvedBlockerCount > 0 && !allowsIssueInteractionWake(...).isDispatchRankReady(contextSnapshot, readiness, now)— a deliberately narrower sibling used only at thedispatchRankinvocation site, covering isolation-retry-deferral + dependency-readiness but not terminal-status. See "Risks" below for why folding the fullisRunClaimablein there would have been a regression, not a fix.dispatchRank'sreadycomputation previously calledisEffectivelyDependencyReadyForDispatchalone (no isolation-retry screen at all). An isolation-retry-deferred, dependency-ready, critical-priority row aged pastSTARVATION_ABSOLUTE_ESCALATION_MS(6h) therefore ranked at the very front of the queue every pass. Because lane A had already marked it as emergency-lane work,claimQueuedRun's refusal of it aborted the whole dispatch pass (scheduleEmergencyContinuationForStillQueuedRun) instead of falling through to the next candidate — stranding a fresh critical arrival ranked right behind it for a full extra round-trip, every single pass, for as long as the row stayed deferred.heartbeat-dispatch-priority-sort.test.tsfor the above.Verification
pnpm run typecheck— clean.npx vitest run src/__tests__/heartbeat-dispatch-priority-sort.test.ts src/__tests__/heartbeat-queued-backlog-convergence.test.ts— 42/42 pass (both suites named in the issue's AC fix(adapter-utils): CAS-retry on concurrent SSH workspace restores #4, "pass unmodified in intent").does not let an aged isolation-retry-deferred critical row strand a fresh critical arrival via a false-ready absolute-floor rank (BLO-22091). Confirmed failing against pre-fix source (git stashon justheartbeat.ts) —criticalLaneRescheduleshas length 1, proving the pass aborted and rescheduled instead of dispatching directly — and passing post-fix (length 0). The assertion is checked at the momentscheduleDetachedDispatchPass's test hook fires synchronously, not after a wait: the pre-existingdispatchDeferredRunIdsByAgentretry-exclusion self-heals the eventual dispatch outcome within one extra pass, so an eventual-consistency assertion would pass on both sides of the fix and hide the regression — this is the same determinism finding from Ally's fix(dispatch): bound worst-case queue wait for every priority tier (BLO-21792) #1022 review (finding fix(test): restore upstream agent-permissions expectations dropped during v513 merge #2), applied here to a different test in the same family.continues the bounded scan when a pass claims fewer runs than it has slots— failed once when run in the full combined-file batch and passed both in isolation and on a full re-run; not touched by this diff).Why this design cannot leak a sixth time in the same shape
Leaks 2-5 were all one shape: the set of rows a dispatch-side screen treats as eligible is not the set the claim path actually accepts, because each site had its own hand-written copy of the rule. That's now closed for the two predicates that exist:
isRunClaimableis consumed, not re-implemented, everywhere it applies. Lane A, Lane B, and Lane C now call the exact same function with the exact same three checks. A future change to any screen — e.g., adding a fourth check — is one edit that every lane picks up automatically; the previous failure mode (edit one copy, forget the other three) becomes structurally impossible because there are no other copies.claimQueuedRun's blocker gate consumes the shared sub-predicate, not a re-derived boolean.isEffectivelyDependencyReadyForDispatchis the one function that answers "is this row dependency-ready-or-exempt", called by both the claim path andisRunClaimable. They cannot disagree on this screen because they call the same code, not because someone remembered to keep two implementations in sync.isDispatchRankReadyomitting the terminal-status screen — is named, documented, and provably safe rather than silently divergent.claimQueuedRun's terminal-status handling (evaluateQueuedRunStaleness) has a broader,resumeIntent-aware exemption for reopening a done/cancelled issue via comment — a different feature (reopen semantics) than dispatch fairness, re-checked against the live row rather than a batch snapshot. Applying the lanes' unconditional terminal check at the rank site would silently override that upstream, resume-intent-aware pruning and strand a legitimately-claimable reopened row at rank12+forever (that branch never ages — seedispatchRank's own comment on why a row that "cannot be claimed" must not escalate). So the split is not incidental:isRunClaimableis for the three lanes, where terminal exclusion was already unconditional pre-fix (no behavior change);isDispatchRankReadyis for the one site downstream of upstream resume-aware pruning, where re-checking terminal status would be wrong, not merely redundant. Each sibling's docstring names the other and states exactly why the split exists, so a future reviewer sees the reasoning in the code, not just in this PR description.Together: every site that must agree, does — by construction, not convention — and the one site that must legitimately differ says so in its own name and docstring rather than reimplementing a fourth copy that quietly drifts.
Risks
dispatchRank'sreadycomputation — this is the only site whose actual runtime behavior changes (it now excludes isolation-retry-deferred rows from ranking eligibility, where previously it didn't check that screen at all). I traced this carefully: folding in the fullisRunClaimable(including the terminal check) at that site would have been a regression for theresumeIntent-exempted terminal-issue edge case (see "Why this cannot leak" v513 test-fallout cleanup batch 2: codex-local SSH dispatch + company-portability mock/expectations #3 above) — confirmed by tracingdispatchRank's!readybranch, which never ages. I deliberately used the narrowerisDispatchRankReadysibling instead.isEffectivelyDependencyReadyForDispatchsince fix(dispatch): bound worst-case queue wait for every priority tier (BLO-21792) #1022's round-5 fix) and for the terminal-status screen (each lane already excluded terminal issues unconditionally before this PR;isRunClaimablejust consolidates three copies of that same rule into one).claimQueuedRun's blocker gate is a pure refactor:readiness?.isDependencyReadyis constructed exactly asunresolvedBlockerCount === 0(verified inserver/src/services/issues.ts), soisEffectivelyDependencyReadyForDispatch(context, readiness)is behaviorally identical to the old inline check for every input.startNextQueuedRunForAgent/claimQueuedRunand covered by the existing 42-case integration suite plus one new case.Model Used
Claude Sonnet 5 (
claude-sonnet-5[1m], 1M context), extended reasoning, with tool use (Read/Edit/Bash) to read the live source, trace the dispatch/claim code paths directly, runtsc --noEmitand the embedded-Postgres vitest suites, and verify the new test against both pre-fix and post-fix source viagit stash.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template