fix(heartbeat): cap external-lifecycle dispatch to one run (BLO-13176 follow-on) - #578
Merged
Merged
Conversation
… follow-on) Follow-on to #576. With the pre-adapter reaper fixed, BLO-12825 still could not complete: its runs kept dying "before external adapter invocation" — but NOT from the reaper. Root cause is over-dispatch. An external-lifecycle (k8s Job) agent can only run ONE Job at a time — the `runningCount>0` and `hasActiveJobForAgent` gates in startNextQueuedRunForAgent reject any second dispatch while one is active. But on an IDLE agent (runningCount 0), `availableSlots = maxConcurrentRuns` (e.g. 3), so the claim loop claimed and `executeRun`'d up to 3 queued runs (one per distinct issue) CONCURRENTLY. Only the first to reach Job creation won the single slot; the losers sat pre-adapter with no Job and were correctly reaped as process_lost. The race is first-lease-wins, not priority-ordered — so on 2026-07-02 the `critical` BLO-12825 repeatedly lost the slot to a sibling that leased ~5s earlier (confirmed in prod: run 49ca8e50 leased 17:32:11, never got a Job, reaped 17:36:17, while a normal-priority sibling that leased 17:32:06 got the Job and ran fine). Fix: cap external-lifecycle dispatch to a single run (`hasExternalLifecycle(adapterType) ? 1 : maxConcurrentRuns`). This (a) stops leasing work we cannot immediately give a Job — no more doomed surplus pre-adapter runs — and (b) gives the one slot to the top of the existing priority sort, so a critical issue wins instead of the fastest leaser. Local (child-process) adapters are unaffected and keep full concurrency. Test: new case — an idle opencode_k8s agent with maxConcurrentRuns=3 and three queued distinct-issue runs (critical is the newest, so createdAt/first-lease would NOT pick it) claims exactly ONE run, and it is the critical one; the other two stay queued. Full heartbeat-process-recovery + dispatch-priority suites green (128/128). Server typecheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Hey @kkroo! 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 |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (degraded — nested CLI unavailable in this k8s runtime; reasoning applied directly against /tmp/pr.diff).
Looks good — small, well-scoped fix with strong test coverage. No Critical or Important findings.
Suggestions (2)
- [native-codex]
.githubPR-template quality gate — thecommitperclipbot flagged this PR as missing required description sections (## Thinking Path,## What Changed,## Verification,## Risks,## Model Used) and the dedup-search checkbox. ThereviewCI check is currently failing because of this, independent of the code change itself — worth fixing before merge so the gate goes green. - [pr-review-toolkit:comment-analyzer]
server/src/services/heartbeat.ts:11341-11354— the added comment is long (14 lines) but earns its length: it documents a genuinely non-obvious race (first-lease-wins vs. priority-ordered dispatch) and the two invariants the existing gates already provide. No change requested, just flagging it was reviewed rather than skipped.
Strengths
- The fix is minimal and precisely targeted:
effectiveMaxConcurrentRunsonly changes behavior forhasExternalLifecycleadapters, leaving local child-process adapters at full concurrency. - Verified against the surrounding code (
server/src/services/heartbeat.ts:11403-11467) thatprioritizedRunsis priority-sorted before theavailableSlotscap is applied in the claim loop, so capping to 1 correctly hands the single slot to the top of the existing priority sort rather than an arbitrary run — this matches the PR's stated intent. - New test (
heartbeat-process-recovery.test.ts:1839-1961) is a good regression test: it seeds three distinct-issue queued runs with the critical one seeded newest specifically to prove the priority sort — not the createdAt tie-break — is what wins the slot, and asserts bothclaimed.length === 1and the two losers stayqueuedrather than being claimed-and-doomed. normalizeMaxConcurrentRunsclamps toHEARTBEAT_POLICY_MAX_CONCURRENT_MIN, so there's no risk of this new cap silently overriding an intentional "0 = disabled" policy — confirmed there's no such value in the valid range.- CI is green except the unrelated PR-template gate above (Build, both test suites, typecheck, e2e, security-review, policy all pass).
Recommended Action
- Fill in the missing PR description sections to unblock the
reviewCI gate. - Merge — no code-level blockers.
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-on to #576. With the pre-adapter reaper fixed, BLO-12825 still could not complete — its runs kept dying
"before external adapter invocation", but not from the reaper. Root cause: over-dispatch.An external-lifecycle (k8s Job) agent can only run one Job at a time — the
runningCount>0andhasActiveJobForAgentgates instartNextQueuedRunForAgentreject any second dispatch while one is active. But on an idle agent (runningCount 0),availableSlots = maxConcurrentRuns(e.g. 3), so the claim loop claimed andexecuteRun'd up to 3 queued runs (one per distinct issue) concurrently. Only the first to reach Job creation won the single slot; the losers sat pre-adapter with no Job and were (correctly) reaped asprocess_lost. The race is first-lease-wins, not priority-ordered.Confirmed in prod (2026-07-02)
criticalrun49ca8e50leased 17:32:11, never got a k8s Job, reaped 17:36:17 "before external adapter invocation".b1c7a483) that leased 17:32:06 — 5 s earlier — got the Job and ran fine (8 min+).So a
criticalissue kept losing the single slot to whatever leased a few seconds sooner.Fix
Cap external-lifecycle dispatch to a single run:
This (a) stops leasing work we can't immediately give a Job — no more doomed surplus pre-adapter runs — and (b) gives the one slot to the top of the existing priority sort, so a critical issue wins instead of the fastest leaser. Local (child-process) adapters are unaffected and keep full concurrency. The steady-state
runningCount>0/hasActiveJobForAgentgates already enforced effective-concurrency-1 for subsequent dispatches; this just makes the initial idle-agent burst consistent with them.Test
New case in
heartbeat-process-recovery.test.ts: an idleopencode_k8sagent withmaxConcurrentRuns=3and three queued distinct-issue runs (critical is the newest, so the createdAt tie-break / first-lease-wins would not pick it) claims exactly one run — and it's the critical one; the other two stayqueued.Full
heartbeat-process-recovery+heartbeat-dispatch-priority-sortsuites green: 128/128. Server typecheck clean.🤖 Generated with Claude Code