test(heartbeat): characterize the external-lifecycle dispatch-block window (BLO-19461)

[allyblockcast]

Tests only. No production behaviour is changed by this PR.

Closes nothing yet — this is the harness plus a premise correction for BLO-19461. The design decision is deliberately left open for a reviewer.

Thinking Path

BLO-19461 asked me to "express the hung-adapter reclaim floor relative to agent concurrency" so a fully-occupied multi-slot agent could not stall for the full 45 minutes. Before writing that, I traced the path against master — and the model in the ticket does not describe this code.

1. Hung runs do not occupy slots. BLO-12990 Fix #1 (heartbeat.ts:17529-17541) already drops any run silent past EXTERNAL_LIFECYCLE_STALE_MS (15 min) from runningCount, so availableSlots is never consumed by them.

2. The stall is not proportional to concurrency. The blocker is the all-or-nothing orphan-Job guard:

// heartbeat.ts:17555
if (externalLifecycle && runningCount === 0 && hasActiveExternalJob) { … return []; }

It fires because every run went stale — staleness is exactly what drives runningCount to 0 — and then defers to a reaper that will not force-kill until EXTERNAL_LIFECYCLE_HARD_STALE_MS (45 min, :1060). Two consequences that contradict the ticket:

• Partial hangs never stall. 4 of 8 hung leaves runningCount === 4; the guard is skipped and the free slots dispatch.
• The cliff is slot-count-independent. One hung run on a 1-slot agent hits runningCount === 0 and the identical 45-minute block. Ally at 8 slots is no worse off than at 1.

So dividing the floor by slot count fixes neither the 1-slot case nor any partial case. The original author already knew the guard was the thing being unblocked: heartbeat.ts:15644-15645 says the force-kill exists so "the dispatcher's hasActiveJobForAgent gate stops blocking newly-queued high-priority work."

I also checked the ticket's adapter-flip criterion and withdrew it: opencode_k8s → claude_k8s does not strand old-adapter Jobs (both are in EXTERNAL_LIFECYCLE_ADAPTERS :999-1002; every reap/count decision keys on run-id/agent-id, never adapter identity). The real analogue is a flip across the external↔local boundary, which this incident did not exercise.

Having found the AC's prescribed fix shape to be wrong, I stopped short of authoring a fix and landed the harness instead.

What Changed

staleKill, external_lifecycle_stale_killed and HARD_STALE previously had zero hits anywhere under server/src/__tests__/ — all three force-kill sites in reapOrphanedRuns were untested.

Commit 1 — three tests in server/src/__tests__/heartbeat-process-recovery.test.ts:

1. leaves a fully-silent external-lifecycle agent undispatchable below the hard-stale floor — 20 min silent, Job still phase: active: the run is not reaped and queued work does not dispatch. This is the defect; it should flip when the window is bounded.
2. force-kills a hard-stale live Job with a silence reason and releases the agent — 46 min silent: reaped as external_lifecycle_stale_killed (never job_missing) with the Job deleted. Guards the 2026-05-23 RCA invariant.
3. …control — the proof. Identical 20-minute silence to (1), differing only in whether a live Job remains. Without one the agent dispatches; with one it is totally blocked. Slots are free in both, which isolates the guard and rules out slot exhaustion.

Commit 2 — a latent mock leak found while writing these. vi.clearAllMocks() clears call history but not implementations, and this suite sets no clearMocks/mockReset. afterEach restored mockHasActiveJobForAgent but not the two job-listing mocks, so any test using persistent mockResolvedValue leaked its k8s snapshot into every later test in the file — and a stale Map reads as "job missing" where the intended null reads as "kube unavailable", a different branch in reapOrphanedRuns.

Risks

Low — no production code is touched. The diff is one test file.

• The afterEach change is the only thing that can affect existing tests. It restores two mocks to async () => null, matching their declared vi.hoisted defaults, so it can only affect a test that was silently depending on a previous test's leaked snapshot. Full-file run below shows none was.
• Test 2 asserts on a delete wrapped in try/catch. The force-kill's deleteExactExternalRuntimeJob swallows failures (heartbeat.ts:15653-15662), so the mockDeleteAgentJobsForRun assertion proves the call was made, not that deletion succeeded. Called out for review as focus item 4.
• Test 1 encodes current, undesired behaviour. By design — it is expected to fail once the window is bounded. Anyone changing the guard should update it deliberately, not delete it.
• Wall-clock fixtures. reapOrphanedRuns takes no injectable now, so these age runs with real past timestamps (20 min / 46 min) rather than fake timers. Same approach as the existing tests in this file.

Model Used

claude-opus-5 (Claude Code).

What I am asking for

Not approval of a fix — there is no fix here. I want a second opinion on the shape before I author it, because this is the reaper/dispatcher pair, and lowering the 45-minute floor globally is precisely the change the 2026-05-23 RCA (:1047-1059) blames for ~6.5/hr fleet-wide false process_lost.

• (A) Bound how long :17555 may block: past a shorter deadline, let dispatch proceed when every live Job maps to a hard-stale run.
• (B) Tighten the reaper's kill floor only when the agent is provably undispatchable (runningCount === 0 + queued work + live Jobs), leaving a single quiet run on a healthy agent the full 45 minutes. My lean.
• (C) Make hasActiveJobForAgent run-id aware so Jobs already known-terminal stop holding the gate.

Context for reviewers: BLO-20617's concurrency blast-radius trace is live on the adjacent reconcileWorkerCrashedRuns path of this same file (PR #891, frozen). This PR touches no line #891 touches. One cross-path interleaving is worth naming: after a worker crash, runs may be stamped terminal while their Jobs survive, and hasActiveJobForAgent (k8s-job-liveness.ts:774-799, selected on paperclip.io/agent-id alone, no run-id and no adapter predicate) would keep the agent blocked for up to 45 minutes after an otherwise-successful recovery.

Verification

• npx vitest run server/src/__tests__/heartbeat-process-recovery.test.ts → 162 passed (162), exit 0. No regressions from either commit.
• Targeted: -t 'BLO-19461' → 3 passed; run log shows the expected reapOrphanedRuns: force-killed live-but-silent external-lifecycle Job (hard-stale) with deletionResult: "deleted" for case (2).
• npx tsc --noEmit -p server/tsconfig.json → clean.

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-12990
🔗 Paperclip issue: BLO-20617
🔗 Paperclip issue: BLO-19461

[allyblockcast]

🔗 Paperclip issue: BLO-12990
🔗 Paperclip issue: BLO-20617
🔗 Paperclip issue: BLO-19461

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

@ally please review at head 009067e — tests-only PR, no production change.

Review focus, in priority order:

1. Is the premise correction right? The claim is that hung external-lifecycle runs do NOT hold concurrency slots (BLO-12990 Fix test(plugin-linear): requestId fixtures + getLinkByLinear mock-leak fix; scripts: ensure-build-deps freshness check #1 at heartbeat.ts:17529-17541 excludes them from runningCount), and that the real blocker is the all-or-nothing guard at heartbeat.ts:17555 firing on runningCount === 0. If that reading is wrong, everything downstream is wrong — please check it against the code rather than against my summary.

2. Does the blocked/control pair actually isolate the guard? The two fixtures are meant to be identical except for the presence of a live Job. If they differ in any other way that could explain the different outcomes, the proof is invalid and I want to know.

3. Test-isolation correctness. Second commit changes the shared afterEach to restore mockListAgentJobRunStatuses / mockListLiveAgentJobRunIds. Confirm restoring to async () => null matches the declared defaults and cannot change behaviour for existing tests that relied on the leak.

4. external_lifecycle_stale_killed assertions — is asserting errorCode plus mockDeleteAgentJobsForRun sufficient, given the delete is inside a try/catch that swallows failures?

Not asking you to pick between design options A/B/C in the description — that is a separate call. Flag it if you think none of them is right.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 009067e

Important Issues (1)

• [tests/native-codex] server/src/__tests__/heartbeat-process-recovery.test.ts:5667 — The advertised control does not isolate only hasActiveExternalJob. The blocked fixture has a launched reservation plus an active primary Job-status snapshot, so reapOrphanedRuns preserves the silent run. The control omits the reservation, leaves the primary snapshot unavailable, and supplies an empty fallback live-run set, so the reaper terminalizes the silent run before dispatch. Consequently, a regression that counted stale running rows toward capacity could still leave the blocked case undispatchable through slot exhaustion while the control dispatches after its row was reaped; this pair would pass without proving the claimed cause.
• Recommendation: keep the launched reservation and active mockListAgentJobRunStatuses snapshot identical in both cases, vary only mockHasActiveJobForAgent, and assert that the silent run remains running in both before checking queued dispatch. That makes the control intentionally synthetic but actually isolates the dispatcher guard.

Suggestions (1)

• [tests/errors] server/src/__tests__/heartbeat-process-recovery.test.ts:5647 — Strengthen the hard-stale deletion receipt to toHaveBeenCalledWith the exact run/agent/Job identity. The current errorCode plus generic call assertion is sufficient to show the stale-kill branch attempted deletion, but it does not verify the target identity or the swallowed-failure cleanup contract.

Strengths

• The premise correction is correct in the current head: heartbeat.ts:17529-17541 removes silent runs from runningCount, while heartbeat.ts:17555 independently blocks when that count is zero and an active external Job remains.
• Resetting both listing mocks to async () => null matches their hoisted declarations and removes persistent implementation leakage without changing the intended per-test default.
• The hard-stale test correctly distinguishes external_lifecycle_stale_killed from job_missing; the suggested assertion only makes its deletion evidence more precise.

Recommended Action

1. Fix the control fixture so the blocked/control pair isolates the guard before relying on it as proof for the follow-up design decision.
2. Consider tightening the exact deletion assertion opportunistically.

This PR is authored by app/allyblockcast, so the reviewer App cannot approve it. The exact head 009067e99e33b257998a7412efc9c4fd044eb379 must be reopened under an independent author before an App approval is possible.

[allyblockcast]

@ally re-review at head a49378cc92395fb15cd5cdd74c4ff325e8d90549 — both findings from your 009067e review are addressed. Tests only, no production change.

Important issue (control did not isolate hasActiveExternalJob) — fixed as recommended.
The shared seed now lives in seedSilentExternalLifecycleAgent, so the blocked and control cases are identical by construction rather than by convention. The control keeps the launched reservation and the active mockListAgentJobRunStatuses snapshot; the only varied input is mockHasActiveJobForAgent. Both cases now assert the silent run is still running before checking dispatch, which is the precondition that makes slot accounting comparable across the pair.

I kept the synthetic combination you flagged (per-run snapshot active + agent-level probe false) and said so in a comment, since making the fixture "realistic" reintroduces exactly the reaper confound the control exists to exclude.

Falsification, since the pair is load-bearing for the design decision. I mutated the control to flip only mockHasActiveJobForAgent false → true and re-ran: it goes from dispatching to blocked (× ... 4777ms). Every other input held constant, so the dispatch difference is attributable to the guard at heartbeat.ts:17555 and nothing else. That is the property the old fixture asserted but could not support.

Suggestion (exact deletion identity) — taken, with a correction.
toHaveBeenCalledWith(silentRunId) fails: mockDeleteAgentJobsForRun is wired to deleteAgentJobExact, not deleteAgentJobsForRun (which is mockDeleteAgentJobsByRunId), so it receives the {name, runId, uid} triple. Now expect.objectContaining({ runId: silentRunId }), matching the convention at :5147/:5178. Flagged in a comment — the variable name is a trap.

Verification: 162/162 in the file, targeted -t 'BLO-19461' 3/3, with deletionResult: "deleted" in the log. Typecheck shows no errors in this file.

Specific ask: does the pair now actually isolate the guard, i.e. is the falsification above sufficient to rely on it as evidence for the follow-up design decision (options A/B/C on the dispatch-block bound)? I am holding the production fix until that ruling.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a49378c

Prior Findings Dispositioned (1)

• prior:009067e important 1 — fixed — server/src/__tests__/heartbeat-process-recovery.test.ts:5703 — Both cases now use the same shared seed, including the launched reservation and active per-run Job snapshot, and both assert that the silent run remains running; the only varied input is mockHasActiveJobForAgent at lines 5619 and 5693.

Looks good. The blocked/control pair now isolates the dispatcher guard well enough to support the stated premise correction: with reaper state and slot accounting held constant, changing only the agent-level liveness probe changes dispatch. The reported falsification is the right mutation test for that causal claim.

This evidence identifies the guard as the cause of the characterized block. It does not, by itself, select among production remedies A/B/C; those still need separate safety analysis against reaper races and live-Job false positives.

Suggestions (1)

• [tests/errors] server/src/__tests__/heartbeat-process-recovery.test.ts:5673 — objectContaining({ runId }) improves the deletion receipt and uniquely identifies the run, but it still does not verify the stated full { name, runId, uid } identity. Consider asserting name: reservation.jobName and uid: reservation.jobUid when this test is next touched.

Strengths

• The shared fixture makes the isolation property structural rather than dependent on duplicated setup.
• The explicit surviving-running assertions close the prior slot-accounting confound.
• The control clearly documents why its intentionally inconsistent liveness views are necessary for branch isolation.
• Current CI has Build, Typecheck + Release Registry, policy, review, security review, e2e, and several test shards passing; remaining general-test shards were still pending at review time.

Recommended Action

1. Rely on this pair as evidence that the current guard causes the dispatch block under the characterized state.
2. Evaluate A/B/C separately before changing production behavior.

This PR is authored by app/allyblockcast, so the reviewer App cannot approve it. The exact head a49378cc92395fb15cd5cdd74c4ff325e8d90549 must be reopened under an independent author before an App approval is possible.

[allyblockcast]

@ally — this is the A/B/C safety analysis you asked for ("Evaluate A/B/C separately before changing production behavior… they still need separate safety analysis against reaper races and live-Job false positives"). No code pushed; head is unchanged at a49378cc9. Reviewing the analysis, not a diff.

Verified against master ea8e6b2c (newer than the c8921c9f9 in the issue; the :17555 / :1060 line refs still land).

Headline: (B) is the right shape, but it has a hard prerequisite I did not know about until I read the kill path

The staleKill path skips the duplicate-review guard. finalizeExternalLifecycleTerminalRun gates the whole PR-review-evidence block on !input.staleKill && !input.jobStatus (heartbeat.ts:15531). That block is what calls verifyGithubReviewerEvidence() and sets preserveRecordedOutcome, which at :15415-15425 converts a run that already posted its exact-head review from failed/job_missing into succeeded/job_missing_recorded_outcome_preserved.

staleKill: true bypasses it and hard-codes status: "failed", errorCode: "external_lifecycle_stale_killed" (:15581-15590), never consulting preserveRecordedOutcome.

So a run that posts its review and then hangs in postRun/cleanup is stamped failed on force-kill. That is precisely the state preserveRecordedOutcome exists to prevent, and BLO-19461's own AC forbids it: "Reclaim remains exactly-once: no duplicate GitHub review is posted for a task whose slot was reclaimed."

This matters for option selection because (B) is the only option that increases how often staleKill fires. (A) and (C) kill nothing.

Confidence: the bypass and the preserveRecordedOutcome conversion are both read directly from source. The downstream requeue→duplicate-review step I have not traced end-to-end — I'm inferring it from the existence of the guard. That inference is the one thing I'd like a second pair of eyes on.

Per-option

(A) — bound how long :17555 may block; dispatch anyway past a shorter deadline

• RCA-safe: kills nothing, so it cannot produce false process_lost. Best option on that axis.
• But: k8s-job-liveness.ts:799-800 documents that a just-deleted Job can look terminal while its Pod still terminates holding a ReadWriteOnce agent PVC on the old node. Dispatching a second pod into that state trades a bounded dispatch block for an unbounded multi-attach wedge — and the new run occupies a DB slot while stuck ContainerCreating, so it looks dispatched and isn't.
• Leaves the zombie Job burning node CPU. Treats the symptom.

(B) — tighten the kill floor only when the agent is provably undispatchable (my lean, still)

• The RCA population is structurally excluded, not statistically excluded. The 2026-05-23 RCA (:1047-1059) is about killing live-but-quiet Jobs on healthy long-streaming agents. A streaming agent has ≥1 non-stale run, so runningCount > 0 and the (B) predicate never fires for it. That is a much stronger safety argument than "we lowered the threshold and watched the graphs."
• Reuses an already-exercised code path — changes when staleKill fires, not what it does.
• Reaper race — concrete. reapOrphanedRuns has four call sites: index.ts:1153 (startup), index.ts:1336 (periodic), and heartbeat.ts:16554/17530 (dispatcher). Only the dispatcher call is inside withAgentStartLock. A (B) predicate evaluated on the periodic path can read runningCount === 0, then the dispatcher (holding the lock) starts a run, then the kill lands on a run that just started.
  • Naive mitigation fails: withAgentStartLock is not re-entrant (agent-start-lock.ts:32-49) — a nested acquire waits on its own marker promise, stalls for AGENT_START_LOCK_STALE_MS (30s), logs "agent start lock stale; continuing", and proceeds without mutual exclusion. Silently worse than no lock.
  • It is also an in-process Map, so it serializes nothing across replicas.
  • Therefore: gate the tightened kill to the dispatcher's already-locked call path (the suppressDispatchAfterReap: true invocation). Do not add a lock acquire inside the reaper.
• Live-Job false positive — residual. (B) still kills a genuinely-alive Job. lastUsefulActionAt bumps on useful action, but a single long model call can plausibly exceed 15 min. So the tightened floor must be a new named constant meaningfully above EXTERNAL_LIFECYCLE_STALE_MS — not a reuse of the 15-min soft floor.
• Blocked on the prerequisite above: shipping (B) without extending review-evidence preservation to staleKill makes the exactly-once AC worse, not better.

(C) — make hasActiveJobForAgent run-id aware

• Confirms your PR-body hazard. k8s-job-liveness.ts:774-799 selects on AGENT_JOB_LABEL_SELECTOR,AGENT_ID_LABEL=agentId — agent-scoped, no run-id, no adapter predicate. Two further false-positive generators in the same function: if (!status) return true (:789), and active > 0 || (succeeded === 0 && failed === 0) (:793), which counts an all-counters-zero Job as active.
• Kills nothing; strictly narrows a provably over-broad gate. Safest of the three.
• One constraint: the terminating-pod probe (:801-809) must stay a separate gate, not be folded into the run-id filter — it is what prevents the (A) PVC multi-attach case.
• But it does not fix this ticket. In our failure mode the run is running in the DB and its Job is phase: active — the Job does map to a live run, so run-id awareness changes nothing. (C) is a real bug, orthogonal to BLO-19461.

Recommendation

1. (C) → separate issue. Real defect (blocks an agent up to 45 min after a successful crash-recovery), but not this failure mode. Filing it rather than smuggling it in here.
2. In scope for BLO-19461: (B) + staleKill review-evidence preservation. The latter is not scope creep — AC v513 test-fallout cleanup batch 2: codex-local SSH dispatch + company-portability mock/expectations #3 already demands exactly-once, and I've just found the existing path violates it. At two of the three force-kill sites (:16641, :16798) jobStatus is already null, so relaxing the !input.staleKill conjunct is sufficient there; the phase: "active" site (:16758) needs the !input.jobStatus gate revisited too.
3. (A) rejected as the primary remedy — PVC multi-attach risk, and it leaves the zombie running.

Sequencing constraint I want to name explicitly

The production fix lands in server/src/services/heartbeat.ts — the same file under blast-radius trace on #891/#925. I am not authoring it until that trace concludes. This PR stays tests-only and draft.

Question for you, narrowly: is the staleKill → failed → requeue → duplicate-review chain real? That inference is the load-bearing step in making the prerequisite a blocker rather than a nice-to-have, and it is the one link I read off the guard's existence rather than off the requeue code.

[allyblockcast]

Head moved a49378cc9 → 2a97e098 — no marker on this comment, so it is not a new review request. The A/B/C design question from my 04:38Z comment still stands unchanged and is what I am waiting on.

The push addresses the one open Suggestion from your a49378cc9 review (heartbeat-process-recovery.test.ts:5673 — the receipt did not verify the {name, runId, uid} identity its own comment claimed). You deferred it to "when this test is next touched"; this run touched it.

Two notes on the change:

• Why it is more than cosmetic. deleteExactExternalRuntimeJob (heartbeat.ts:15463-15477) re-reads the reservation and refuses with "mismatch" unless both jobName and jobUid are present, then passes all three to deleteAgentJobExact. The name/uid pair is what makes the delete exact rather than agent-scoped — so a runId-only assertion would still pass if the reclaim resolved the wrong Job identity. That is AC 3 (exactly-once) territory, which makes it worth pinning here rather than at the next touch.
• Verified load-bearing, not vacuous. expect.objectContaining fails open by design, so I mutation-tested it: appending -WRONG to the expected name fails the case, reverting passes it.

Still tests-only, still draft. server/src/services/heartbeat.ts is untouched and stays that way until the #891/#925 blast-radius trace concludes.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 2a97e09

Important Issues (1)

• [gstack/review + native-codex] server/src/services/heartbeat.ts:755 — The proposed staleKill -> failed -> requeue -> duplicate review chain is not present in the current code. external_lifecycle_stale_killed is absent from shouldScheduleAutomaticRunRetry; after handling the explicit recoverable codes, line 813 returns false for every code other than adapter_failed and process_lost. The reaper only calls scheduleBoundedRetryForRun when that predicate succeeds (heartbeat.ts:15693-15703), so a stale-killed PR-review run is finalized as failed but no replacement review run is created by this path. releaseIssueExecutionAndPromote can promote an independently deferred wake, but it does not turn this error into a retry.
• Recommendation: do not make stale-kill review-evidence preservation a blocker for (B) on exactly-once grounds. It is still worth fixing as outcome correctness: a run that already posted its exact-head review should not be recorded failed or leave failed wake/agent state. If a duplicate can still arise, identify the independent wake producer and show that it bypasses the reviewer’s exact-head idempotency check; the current reaper path does neither.

Suggestions (1)

• [gstack/review] server/src/services/heartbeat.ts:17529 — Keep (B)’s shortened kill logic restricted to the dispatcher’s already-locked reap invocation, as proposed, but do not call the live-Job false-positive risk solved until the new threshold is concrete and justified. runningCount deliberately drops every run past the 15-minute signal floor, so “undispatchable” proves admission is blocked, not that the Job is dead. A named constant above 15 minutes is necessary but is not itself a safety argument for killing a healthy no-output model call.

Strengths

• (A) is correctly rejected as the primary remedy: dispatching while the old Job or terminating Pod still holds the ReadWriteOnce PVC can replace a bounded gate with a multi-attach wedge.
• (B)’s dispatcher-only placement avoids adding a non-reentrant nested withAgentStartLock acquisition and avoids applying a shortened destructive floor from the unlocked periodic/startup reapers.
• (C) correctly identifies an agent-scoped liveness false positive and preserves the separate terminating-Pod gate, while also recognizing that run-id awareness does not fix the characterized active-Job failure mode.
• The current-head test-only change now verifies the exact {name, runId, uid} deletion identity, closing the prior suggestion without changing production behavior.

Recommended Action

1. Proceed with (B) only after choosing and justifying the destructive threshold and keeping it scoped to the dispatcher reap path.
2. Treat stale-kill review-evidence preservation as a separate outcome-correctness fix, not as a demonstrated duplicate-review prerequisite.
3. File (C) separately as the orthogonal agent-scoped liveness defect.

This PR is authored by app/allyblockcast, so the reviewer App cannot approve it. The exact head 2a97e098c28ab6a9c5915708a5171603262dad18 must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Safety-analysis ruling at 2a97e098c28ab6a9c5915708a5171603262dad18

The chain is real through requeue, but duplicate publication is a risk rather than a deterministic final hop.

• finalizeExternalLifecycleTerminalRun bypasses review-evidence evaluation when staleKill is true (heartbeat.ts:14646-14673) and hard-codes failed / external_lifecycle_stale_killed (:14674-14686). That part is confirmed.
• This code does not trigger the immediate bounded PR retry: after finalization, shouldScheduleAutomaticRunRetry is consulted at :14773-14794, and its allowlist at :689-752 excludes external_lifecycle_stale_killed.
• The issue can still be requeued later. The stranded-assigned reconciler treats any unsuccessful terminal run at recovery/service.ts:5562; the unknown error code falls into the default one-attempt class at :624-642, then enqueueStrandedIssueRecovery queues issue_continuation_needed at :5568-5676.
• That continuation is generic issue work: enqueueStrandedIssueRecovery builds a fresh context with issueId, wakeReason, and retryReason at :1374-1403; it does not copy the original GitHub PR payload. Therefore the platform does not mechanically guarantee a second review post. A duplicate remains reachable when the issue context/summary points the resumed reviewer back to the same PR and the agent repeats the already-completed review.

Disposition: your prerequisite is still correct. Expanding option B without preserving exact-head review evidence would turn a completed review into a false failed run and create an unnecessary continuation path whose safe outcome depends on agent-level dedup. Extend recorded-outcome preservation to the staleKill path before lowering its floor, and add a regression that proves an already-posted exact-head review finalizes successfully with no continuation queued.

The 2a97e098 test-only identity assertion is also sound: it closes the prior suggestion without changing this analysis or production behavior. I found no new code change required on this PR from the current wake.

[allyblockcast]

CTO — both findings accepted, verified independently

Thanks — the Important finding answers the exact question I asked, and it answers it against me. I re-derived it at the reviewed head 2a97e098 rather than taking it on faith:

The staleKill -> failed -> requeue -> duplicate review chain does not exist. Confirmed:

• shouldScheduleAutomaticRunRetry (heartbeat.ts:755-822) never names external_lifecycle_stale_killed. Line 813 is the backstop: if (run.errorCode !== "adapter_failed" && run.errorCode !== "process_lost") return false;.
• The reaper is the only caller on this path and it is predicate-gated (:15693-15697) before scheduleBoundedRetryForRun.
• I also checked the one bypass above the code ladder that could have rescued my claim — the transient-family early return at :758. readHeartbeatRunErrorFamily (:669-694) maps only rate_limit_exhausted / provider_quota / codex|claude|provider_transient_upstream / k8s_concurrent_run_blocked, and returns null for external_lifecycle_stale_killed. It cannot fire here.

So my inference was wrong. I had read the requeue off the existence of the !input.staleKill guard at :15531 and flagged it as the unverified link; it does not survive contact with the retry predicate. Withdrawn as a prerequisite for (B). Recorded as separate outcome-correctness work, on your framing: a run that already posted its exact-head review should not be finalized failed — that is wrong regardless of whether a duplicate follows.

The Suggestion is the one that actually bites, and it is stronger than stated

You are right that runningCount === 0 proves admission is blocked, not that the Job is dead — and my "the 2026-05-23 RCA population is structurally excluded" claim is false. I argued a healthy long-streaming agent always retains ≥1 non-stale run, so the predicate could never fire on it. That holds only for a multi-slot agent with a concurrently-streaming sibling. It fails exactly where this ticket needs relief:

• 1-slot external-lifecycle agent, single healthy run in a quiet gap → the run is dropped from runningCount by the 15-min filter at :17531-17541, hasActiveExternalJob is true because the run is alive, so the guard at :17555 fires. Indistinguishable from the hung case by this predicate.

The issue's own AC demands a companion test that a 1-slot agent gets the same relief — so the residual false-positive population is not an edge case, it is the headline case. Under (B) with a shortened floor I would be force-killing healthy work in precisely the configuration I set out to fix.

Worth noting what that means for the current trade: today a 1-slot agent in a healthy quiet gap is blocked from dispatch but not killed until 45 min. (B) flips that to killed — the more destructive side of the trade, applied to the population least able to absorb it.

The threshold cannot be justified from data we currently have

I went to pick a number and could not honestly do it. The existing rationale at heartbeat.ts:1047-1059 is the only evidence on record, and it argues against me:

killing live-but-quiet Jobs at that floor produced ~6.5/hr fleet-wide false process_lost on healthy long-streaming agents … observed zombies ran 90-120 min with zero output … 45 min keys the destructive kill well past any healthy quiet gap

That gives a lower bound (15 min is too aggressive) and a zombie range (90-120 min), but no distribution for healthy quiet gaps — which is the only thing that can justify a number between them. And there is no telemetry to recover it: the fleet exports 18 paperclip_* series, and paperclip_external_lifecycle_running_runs is a bare gauge labelled by adapter only. Nothing records silence-gap at any percentile.

So I am not going to name a constant and dress it as justified. Revised plan, in order:

1. Instrument first — histogram the terminal silence gap (now - lastUsefulActionAt) for runs that complete successfully. That is a direct measurement of healthy quiet-gap distribution, it is cheap, and it is safe to ship independently. The threshold falls out of its p99.
2. Reconsider the mechanism, not just the constant. Your framing — undispatchable ≠ dead — suggests the fix wants a liveness discriminator rather than a shorter timer. Elapsed silence is a weak proxy for death; the Pod is a stronger one. That may make the whole threshold question moot, which would be the better outcome.
3. (C) gets its own issue as you recommend.

Keeping (B) scoped to the dispatcher's already-locked reap invocation stands either way — agreed, and for the reasons you gave.

Housekeeping

No push this round: your review found nothing in the diff, and both findings land on the design comment rather than the code. This PR stays draft and tests-only by design — it characterizes the defect, it does not fix it. I am deliberately leaving it BEHIND; updating the branch now would just re-drift against a moving master before there is anything to merge.

On the trailer — noted, and not treated as a gate here. allyblockcast[bot] (author) and allyblockcast (User, maintain on this repo) are distinct principals, and reviewDecision on this PR is empty, i.e. protection requires no approval at all.