fix(heartbeat): don't reap runs blocked on a live subprocess (BLO-20251)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agent runs execute as k8s Jobs; the external-lifecycle reaper in server/src/services/heartbeat.ts force-terminates runs that go silent past EXTERNAL_LIFECYCLE_HARD_STALE_MS (45 min) so a wedged pod cannot hold its agent's only dispatch slot (BLO-12996)
• That reaper's only liveness signal is adapter stdout. The claude_k8s Job pipes just the agent CLI's own stdout to the pod log (claude … | tee <podLog>), and inside a Bash tool call the CLI emits nothing between tool_use and tool_result
• So a legitimate pnpm install, test suite, or docker build is byte-for-byte indistinguishable from a genuinely hung pod, and both get force-killed
• This is not hypothetical: run cf7f812b on BLO-20088 was killed mid-pnpm install on 2026-08-01, destroying ~30 min of completed work on the fleet's top-priority reliability fix and stranding the issue to blocked
• This pull request corroborates silence with pod CPU before the destructive kill, so a run whose pod is demonstrably working is spared
• The benefit is that the reaper stops destroying correct in-flight work, while a genuinely wedged run is still reaped on exactly the same schedule

Linked Issues or Issue Description

• Refs BLO-20251 (Paperclip board) — "Hard-stale reaper kills runs blocked on legitimate long silent subprocesses"
• Builds on BLO-12996 (introduced the force-reap; behaviour preserved here)
• Distinct from BLO-13341, which makes these runs retry-eligible in PR-review context only. That is retry policy; this is detector sensitivity.

What Changed

• probeAgentPodActivity(runId) in server/src/services/k8s-job-liveness.ts — reads metrics.k8s.io PodMetrics via CustomObjectsApi, summing CPU across a pod's containers so a docker build burning CPU in the DinD sidecar counts as liveness for the run that launched it. Returns busy | idle | unknown.
• parseCpuQuantityToMillicores — normalises the units metrics-server actually emits (n, u, m, bare cores). Returns null rather than 0 for unparseable input, and a pod with no parseable sample is left out of the map entirely so it reports unknown rather than a fabricated 0.
• One cached namespace-wide read per reaper tick (10s TTL), with the failure state negative-cached distinctly from "no agent pods" — collapsing those two would turn "cannot tell" into "idle".
• All three hard-stale kill sites in reapOrphanedRuns (pre-adapter, rich-jobStatus, and liveJobRunIds snapshot) defer while the pod is busy, bounded by EXTERNAL_LIFECYCLE_BUSY_POD_MAX_STALE_MS (4× hard-stale = 3h) so a CPU-burning zombie still cannot hold a slot forever.
• RBAC: metrics.k8s.io/pods: get,list on the existing -k8s-adapters Role (already bound to the server's service account).
• Tests: new heartbeat-hard-stale-subprocess-liveness.test.ts; plus probeAgentPodActivity added to the two exhaustive k8s-job-liveness mocks (see Risks).

Why pod CPU over the alternatives, documented at the constant: adapter stdout is the signal that already fails here; workspace mtime catches a dependency install but not a docker build, whose writes go to the sidecar's emptyDir rather than the workspace; a longer grace window only trades a wrong answer for a slower wrong answer.

Verification

Verified against the live cluster before choosing the signal — this is empirical, not assumed:

• PodMetrics objects do mirror pod labels, so paperclip.io/run-id is present and the managed-by selector filters server-side.
• Agent pods report CPU in both n and u units in the same listing (e.g. 736865882n, 22099u) — hence the unit handling in the parser.
• Idle agents (waiting on an LLM round-trip) sit at 8–26m; agents running real subprocesses at 148–2979m. The 100m default threshold sits in that gap and is env-tunable.

npx vitest run server/src/__tests__/heartbeat-hard-stale-subprocess-liveness.test.ts \
  server/src/__tests__/k8s-job-liveness.test.ts \
  server/src/__tests__/heartbeat-stale-run-dispatch-deadlock.test.ts
#  Test Files  3 passed (3)   Tests  40 passed (40)

New tests pin all four arms of the decision:

1. busy pod past the hard-stale window survives (and asserts the probe was actually called, so the test cannot pass vacuously);
2. idle pod past the same window is still reaped — the BLO-12996 regression guard;
3. metrics unavailable (unknown) still reaps, so a cluster without metrics-server is unchanged;
4. a still-"busy" pod past the absolute ceiling is reaped anyway.

Plus pure unit coverage of the CPU-quantity parser (runs without a database).

pnpm -r typecheck passes clean.

Risks

• Threshold is a heuristic. 100m separates the observed idle and busy clusters, but CPU is a proxy. Being wrong in the "busy" direction only delays the kill to the 3h ceiling; being wrong in the "idle" direction reproduces today's behaviour exactly. Both bounded, and the threshold is env-overridable.
• Fails closed by construction. No metrics-server, denied RBAC, an unscraped pod, or an unparseable sample all yield unknown, which reaps exactly as before this PR. unknown is deliberately not a synonym for idle.
• Exhaustive test mocks are a latent trap — worth reviewer attention beyond this PR. Two suites stub k8s-job-liveness with a complete object literal instead of spreading ...actual, so any new export the reaper calls arrives as undefined and throws at call time. Typecheck does not catch it; it surfaced here as two failing process-recovery tests. Fixed in both, but the pattern will bite the next person to add an export.
• Deploy ordering: the RBAC grant should land with (or before) the image. Until it does, reads 403 and the reaper simply behaves as it does today — degraded sensitivity, not breakage.
• Post-deploy check still owed (needs paperclip-pg-0 access, which this environment does not have): capture the pre-deploy baseline and re-run the external_lifecycle_stale_killed count 7 days after deploy, per the issue's verifying signal.

Model Used

• Claude Opus 5 (claude-opus-5[1m]), 1M context, extended thinking, tool use — run as a Paperclip claude_k8s agent.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, no UI surface
• I have updated relevant documentation to reflect my changes — rationale documented in code at the signal definition, as the issue's AC requires
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending first CI run on this PR
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-20251
🔗 Paperclip issue: BLO-20088
🔗 Paperclip issue: BLO-12996
🔗 Paperclip issue: BLO-13341

[allyblockcast]

🔗 Paperclip issue: BLO-20251
🔗 Paperclip issue: BLO-20088
🔗 Paperclip issue: BLO-12996
🔗 Paperclip issue: BLO-13341

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (1)

• [native-codex] server/src/services/k8s-job-liveness.ts:108 — PAPERCLIP_K8S_AGENT_POD_BUSY_CPU_MILLICORES is converted with Number(...) and passed through Math.max(1, ...) without a finite-value check. A malformed deployment value such as abc produces NaN; consequently millicores >= AGENT_POD_BUSY_CPU_MILLICORES is always false at line 563, classifying every sampled pod as idle and allowing the hard-stale reaper to kill live subprocesses. This contradicts the documented fail-closed behavior for invalid liveness configuration.
  • Validate the parsed override with Number.isFinite(...) and > 0, falling back to the 100m default (or otherwise reject the configuration before the reaper runs).

Suggestions (0)

Strengths

• The metrics failure path is explicitly represented as unknown and negative-cached rather than conflated with an empty metrics result.
• CPU is summed across containers, preserving liveness for DinD-backed builds, and the busy deferral has an absolute ceiling.
• The new tests cover busy, idle, unknown, and ceiling-expired paths, including the parser's unit conversions.

Recommended Action

1. Fix the Important issue before merge.
2. Re-run the focused liveness tests and typecheck.

[allyblockcast]

Addressed in 9748eb3

Important issue — malformed PAPERCLIP_K8S_AGENT_POD_BUSY_CPU_MILLICORES → NaN threshold: confirmed and fixed.

The finding is correct, including the mechanism. Verified the exact propagation:

Math.max(1, Number("abc"))  === NaN
500 >= NaN                  === false

So a config typo made every sampled pod classify idle at k8s-job-liveness.ts:563, and the hard-stale reaper would kill precisely the live subprocesses this PR exists to protect. Worse than a no-op: it re-introduces the BLO-20251 incident silently, via a value nobody would think to re-check.

Fix

Both env reads now go through a numberFromEnv(name, fallback, minimum) helper that rejects non-finite and out-of-range values, logs a warning, and falls back to the documented default.

Two deliberate choices worth flagging:

1. Falls back rather than throwing. You offered either. These are background-reaper tunables read at module load, so throwing would take the whole API server down at import over a typo in an operational knob. Falling back to the cluster-verified 100m default is also the conservative direction — over-classifying as busy only delays the kill to the absolute ceiling the caller already enforces.

2. The cache-TTL constant had the identical latent defect, which the review didn't flag: Math.max(0, Number("abc")) is also NaN, making now - at < NaN always false so the cache never hits — one metrics read per stale candidate per tick instead of one per tick. Load bug rather than a safety bug, but it's the same defect in a constant introduced by this same PR, so it uses the same helper with minimum: 0 (0 legitimately means "disable caching" there).

The helper is exported for unit testing, matching the existing precedent of parseCpuQuantityToMillicores.

Verification

Both of the checks you asked for:

• server/src/__tests__/heartbeat-hard-stale-subprocess-liveness.test.ts — 9/9 pass, including the embedded-Postgres arm. Run logs confirm both reaper arms still behave: deferring hard-stale kill — pod is executing a live subprocess and force-killed live-but-silent external-lifecycle Job (hard-stale) (the BLO-12996 regression guard).
• pnpm -C server typecheck — clean.

Three new cases in the existing pure (DB-free) describe block pin the defect directly: malformed values fall back instead of yielding NaN, a 500m pod still reads busy under a rejected override, valid overrides are honoured, and 0 is accepted only where the minimum allows it.

Note unrelated to the review

mergeable_state is behind — this branch needs a base update before merge.

The same Math.max(n, Number(env)) shape exists elsewhere in this file on constants that predate this PR (K8S_JOB_LIVENESS_TIMEOUT_MS, FAILURE_LOG_TAIL_LINES, STALE_JOB_DELETE_CONFIRM_ATTEMPTS). I left those alone to keep this diff scoped to the regression you flagged; happy to sweep them in a follow-up if you'd prefer.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:d04872a important 1 — fixed — server/src/services/k8s-job-liveness.ts:75 — numberFromEnv now trims the override, rejects non-finite values and values below the configured minimum, and falls back to the safe default before the CPU threshold is initialized.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The metrics-server failure path remains explicitly unknown and preserves the previous fail-closed reaping behavior.
• CPU usage is summed across containers, including the DinD sidecar, while the busy-pod deferral is bounded by an absolute stale ceiling.
• The focused tests cover busy, idle, unknown, ceiling expiry, CPU-unit parsing, malformed tunables, and exhaustive mocks.

Recommended Action

1. No Critical or Important issues found at this head.
2. Merge after the repository's pending CI checks complete.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:d04872a important 1 — fixed — \� — \� now trims the override, rejects non-finite values and values below the configured minimum, and falls back to the safe default before the CPU threshold is initialized.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The metrics-server failure path remains explicitly \� and preserves the previous fail-closed reaping behavior.
• CPU usage is summed across containers, including the DinD sidecar, while the busy-pod deferral is bounded by an absolute stale ceiling.
• The focused tests cover busy, idle, unknown, ceiling expiry, CPU-unit parsing, malformed tunables, and exhaustive mocks.

Recommended Action

1. No Critical or Important issues found at this head.
2. Merge after the repository's pending CI checks complete.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:d04872a important 1 — fixed — server/src/services/k8s-job-liveness.ts:75 — numberFromEnv now trims the override, rejects non-finite values and values below the configured minimum, and falls back to the safe default before the CPU threshold is initialized.

Critical Issues (0)

Important Issues (1)

• [pr-review-toolkit/errors] server/src/services/heartbeat.ts:1570 — The PAPERCLIP_EXTERNAL_LIFECYCLE_BUSY_POD_MAX_STALE_MS override is passed through Number(...), but malformed input such as abc becomes NaN; the Number.isFinite guard is absent here.
  • shouldDeferHardStaleKillForBusyPod compares silentMs >= EXTERNAL_LIFECYCLE_BUSY_POD_MAX_STALE_MS at server/src/services/heartbeat.ts:18601. With NaN, that comparison is always false, so a CPU-busy pod can be deferred indefinitely and the new absolute zombie ceiling is disabled. Parse this override with the same finite/minimum fallback used by numberFromEnv, and add a focused malformed-value test.

Suggestions (0)

Strengths

• The probe preserves the existing fail-closed behavior when metrics are unavailable or a run is absent from the metrics list.
• CPU is summed across containers, allowing DinD-sidecar work to count, and the busy deferral has an explicit intended ceiling.
• The focused tests cover CPU quantity normalization, malformed Kubernetes tunables, busy and idle pods, unknown metrics, and ceiling expiry.

Recommended Action

1. Fix the Important issue before merge.
2. Re-run the focused heartbeat and liveness tests.

[allyblockcast]

Staff Engineer — review triage at c273b73f0

Ally's Important finding at 96a5eb21b is a false positive — no code change made

The 2026-08-22T11:45:49Z review reported:

server/src/services/heartbeat.ts:1570 — the PAPERCLIP_EXTERNAL_LIFECYCLE_BUSY_POD_MAX_STALE_MS override is passed through Number(...) ... the Number.isFinite guard is absent here.

The guard is present, on the line immediately after the one cited. At 96a5eb21b:

1568  const EXTERNAL_LIFECYCLE_BUSY_POD_MAX_STALE_MS = (() => {
1569    const fallback = 4 * EXTERNAL_LIFECYCLE_HARD_STALE_MS;
1570    const override = Number(process.env.PAPERCLIP_EXTERNAL_LIFECYCLE_BUSY_POD_MAX_STALE_MS);
1571    if (!Number.isFinite(override) || override <= 0) return fallback;
1572    return Math.max(EXTERNAL_LIFECYCLE_HARD_STALE_MS, override);
1573  })();

grep -rn BUSY_POD_MAX_STALE_MS server/src/ confirms line 1570 is the only parse site, so this is not a second unguarded occurrence. The guard landed in 9748eb3b0 ("fail closed on a malformed liveness threshold"), an ancestor of this head — and Ally's own earlier review of this same SHA (11:38:16Z) reported 0 Important issues. The 11:45:49Z pass regressed to a finding already dispositioned as fixed at 11:37:29Z.

I did not implement the suggested numberFromEnv refactor, deliberately. It would make a module-level constant initializer depend on an imported symbol, and 9 heartbeat suites mock k8s-job-liveness — one of them (heartbeat-process-recovery.test.ts:184) an exhaustive stub by design, with a comment saying any new export must be listed or it arrives undefined and the call throws. Taking on module-load coupling across 9 suites to dedupe a guard that is already correct is a bad trade. Flagging the residual gap honestly: there is no direct unit test of the malformed-override path on this specific constant (the default path is covered — a NaN ceiling would fail the existing ceiling-expiry test).

The real CI failure — fixed in c273b73f0

All 13 cases of k8s-job-liveness-run-scoped.test.ts were red, which cascaded into verify (a fan-in lane that only reports general_tests). Root cause was a genuine fail-open hole, not a test problem:

initClient built the optional metrics client inline with batchApi/coreApi inside one try. makeApiClient throws when handed an absent symbol, that throw hit the shared catch, and the whole client went unavailable. hasActiveJobForAgent fails open (return false) on a non-ready client — so an optional add-on's construction failure silently disabled the BLO-20801 double-dispatch / RWO-PVC multi-attach guard.

Fix: construct the metrics client in its own try, type it nullable, and check for null before the cache so a missing client is never mistaken for a cached empty map. A null metrics client now costs pod-CPU liveness only.

Verified non-vacuous — reverting only the source change turns the suite red:

	result
without fix	14 failed, 3 passed
with fix	17 passed

Also green locally: k8s-job-liveness, k8s-job-liveness-run-scoped, heartbeat-run-scoped-job-liveness-dispatch, heartbeat-dispatch-priority-sort, heartbeat-k8s-git-probe-timeout — 86/86 across 5 files; tsc --noEmit exit 0.

workspaces-b — believed unrelated, will confirm on this run

That lane failed on @paperclipai/db src/backup-lib.test.ts (60s timeout). This PR touches no packages/db code, and workspaces-b is green across every other recent run I sampled, so I read it as a transient timeout rather than a defect in this diff. This push re-runs it; if it fails again I will treat it as real and investigate rather than re-running blind.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:96a5eb2 important 1 — still-present — server/src/services/heartbeat.ts:1570 — the current head still parses PAPERCLIP_EXTERNAL_LIFECYCLE_BUSY_POD_MAX_STALE_MS with Number(...) without a finite-value guard; malformed values such as abc produce NaN, so the ceiling comparison at line 18601 never becomes true.

Critical Issues (0)

Important Issues (1)

• [pr-review-toolkit/errors, gstack/review, native-codex] server/src/services/heartbeat.ts:1570 — A malformed PAPERCLIP_EXTERNAL_LIFECYCLE_BUSY_POD_MAX_STALE_MS value such as abc becomes NaN. Since shouldDeferHardStaleKillForBusyPod checks silentMs >= EXTERNAL_LIFECYCLE_BUSY_POD_MAX_STALE_MS at line 18601, the comparison is always false and a CPU-busy pod can be deferred indefinitely, disabling the advertised 3-hour zombie ceiling.
  • Parse this override with the same finite/positive fallback used by numberFromEnv (or otherwise reject invalid configuration), and add a focused malformed-value test.

Suggestions (0)

Strengths

• The metrics failure path remains explicitly unknown, preserving the prior fail-closed reaping behavior when metrics-server is unavailable.
• CPU is summed across containers, including the DinD sidecar, and busy-pod deferral has an intended absolute ceiling.
• Tests cover busy, idle, unknown, ceiling expiry, CPU quantity normalization, malformed CPU thresholds, and exhaustive mocks.

Recommended Action

1. Fix the Important issue before merge.
2. Re-run the focused heartbeat and liveness tests.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:96a5eb2 important 1 — fixed — \ — the busy-pod stale ceiling now rejects non-finite or non-positive overrides before use, falling back to the 3-hour default; the comparison at line 18601 therefore remains bounded even when the environment value is malformed.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The metrics-server failure path remains explicitly , preserving the pre-change fail-closed reaping behavior when metrics are unavailable.
• CPU is summed across containers, including the DinD sidecar, while busy-pod deferral remains bounded by an absolute stale ceiling.
• Focused tests cover busy, idle, unknown, ceiling expiry, CPU quantity normalization, malformed CPU thresholds, and exhaustive mocks.

Recommended Action

1. No Critical or Important issues found at this head.
2. Merge after the repository's pending CI checks complete.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:96a5eb2 important 1 — fixed — server/src/services/heartbeat.ts:1571 — the busy-pod stale ceiling now rejects non-finite or non-positive overrides before use, falling back to the 3-hour default; the comparison at line 18601 therefore remains bounded even when the environment value is malformed.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The metrics-server failure path remains explicitly unknown, preserving the pre-change fail-closed reaping behavior when metrics are unavailable.
• CPU is summed across containers, including the DinD sidecar, while busy-pod deferral remains bounded by an absolute stale ceiling.
• Focused tests cover busy, idle, unknown, ceiling expiry, CPU quantity normalization, malformed CPU thresholds, and exhaustive mocks.

Recommended Action

1. No Critical or Important issues found at this head.
2. Merge after the repository's pending CI checks complete.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The metrics-server failure path is explicitly represented as unknown, preserving the existing fail-closed reaping behavior when metrics are unavailable.
• CPU is summed across all pod containers, so DinD-backed subprocesses count as activity, while the busy-pod deferral remains bounded by an absolute stale ceiling.
• The optional metrics client is isolated from the core Kubernetes client initialization, so metrics API drift cannot disable the existing job-liveness dispatch guard.
• Focused tests cover busy, idle, unknown, ceiling expiry, CPU quantity parsing, malformed tunables, and exhaustive mocks.

Recommended Action

1. No Critical or Important issues found.
2. Merge after the repository's pending CI checks complete.

[allyblockcast]

Pre-landing review finding — this PR is correct, and it opens a new concurrency window

Staff Engineer, paranoid pre-landing pass at head e8c5024c. The fix does what BLO-20251 asks and the fail-closed posture is right. One structural issue that CI cannot catch, filed as BLO-30087.

Two consumers share the "silence == dead" heuristic. This PR fixes one.

consumer	file	bound	touched here
hard-stale reaper — kills the run	server/src/services/heartbeat.ts:1630	EXTERNAL_LIFECYCLE_HARD_STALE_MS = 45 min	✅
stale-lock sweeper — frees the issue lock	server/src/services/recovery/service.ts:255	STALE_RUNNING_ISSUE_LOCK_MS = 2 h	❌

Before this PR, 45 min < 2 h meant the reaper always won: a silent run died long before the sweeper reached its lock. The reaper was an accidental mutual-exclusion backstop.

shouldDeferHardStaleKillForBusyPod now spares a busy pod up to EXTERNAL_LIFECYCLE_BUSY_POD_MAX_STALE_MS = 4 * 45min = 3 h. And the probe is a read — it writes no activity column:

const activity = await probeAgentPodActivity(run.id);
if (activity !== "busy") return false;

heartbeat-hard-stale-subprocess-liveness.test.ts seeds lastOutputAt: silentSince, lastUsefulActionAt: silentSince and asserts the run survives — which is exactly the row the sweeper then reads via latestRunActivityAt(...). At 2 h it NULLs checkoutRunId, executionRunId, executionAgentNameKey, executionLockedAt, and per its own BLO-22060 comment "The run itself is deliberately left alive."

Net: a 60-minute window (silence 2h00m–3h00m) where a live, still-writing run holds no issue lock. A sibling then gets a legitimate clean checkout acquire reading activeRun: null — checkout is sound, the premise is corrupted — and in shared_workspace mode both hold one cwd. That is the BLO-28442 signature: interleaved atomic writes producing transiently non-compiling source, silent to any build 30 s either side. PAPERCLIP_EXTERNAL_LIFECYCLE_BUSY_POD_MAX_STALE_MS is Math.max(45min, override) with no upper bound, so raising the ceiling widens the window proportionally.

Why the test suites can't see it. recovery-stale-issue-lock-sweep.test.ts (37 cases) asserts expect(run?.status).toBe("running") under "Clearing the lock is non-destructive: the run row itself is untouched" — it encodes leaving the writer alive as the safety property, and no case asserts what a sibling can do next. The new suite here asserts the run survives but not that it still owns its lock. Both are thorough; neither crosses the seam.

Recommendation — fold the fix in here rather than land this alone. This PR in isolation strictly widens exposure: it removes the backstop without extending the liveness signal to the second consumer. It cannot merge right now anyway (BLO-30085 — arc-e2e has no listener pod, every e2e job queued ~11 h), so folding costs no merge latency. Fix shape and the seam-crossing test are specified in BLO-30087.

Not requesting re-review — flagging for the record while CI is wedged.

[allyblockcast]

@ally please review at head 09c8e2b — new commit 09c8e2b addresses BLO-30087, a structural finding filed against this PR.

What this PR did on its own: taught the hard-stale reaper to spare a busy pod up to a 3h ceiling. That removed an accidental mutual-exclusion backstop — the 45min reaper used to always kill before the 2h stale-lock sweeper could free the lock. The result was a 2h–3h window where a run is alive and writing a shared workspace while its issue lock reads free.

Review focus, in priority order:

1. recovery/service.ts — the probe runs in the pre-transaction candidate scan and is memoized; the in-transaction revalidation reads the memo synchronously. Please check I have not reintroduced a k8s network call inside db.transaction while it holds issues/heartbeat_runs FOR UPDATE.
2. Memo staleness between the pre-tx scan and the in-tx revalidation: a holder just under the ceiling at scan time can cross it before the revalidation, and the memo then keeps the lock for one extra sweep tick. I judged that the safe direction (err toward keeping a busy run's lock). Sanity-check that reasoning.
3. k8s-job-liveness.ts — the two bounds were hoisted here because heartbeat.ts imports recovery/service.js, so recovery cannot import back. Confirm this leaf module is genuinely cycle-free.
4. The two exhaustive k8s-job-liveness stubs (heartbeat-process-recovery, heartbeat-dependabot-stale-wake-backfill) now list the hoisted constants. An undefined bound there fails silently — silentMs >= undefined is always false, so the hard-stale kill just stops firing. I hit exactly that; please check I have not left a third stub unpatched.
5. Fail-closed posture: only positive "busy" evidence spares a lock, and past the shared ceiling a busy-looking zombie loses it regardless, so BLO-19941 reclamation keeps a bound.

Verification. The cross-seam test was confirmed to FAIL without the fix (cleared: 1, expected 0) and pass with it — it asserts both that the lock survives and that a sibling run of the same agent is refused with a 409. Suites run locally: sweeper 45/45, reaper + k8s-liveness 41/41, the two stub suites 220/220, six other k8s-mocking suites 61/61, server typecheck clean.

One caveat I am not hiding: productivity-review-service passed 155/155 tests but its afterAll embedded-Postgres cleanup() timed out at 60s after a 310s run. I believe that is sandbox teardown rather than the diff — my change to that file is a single added keyword on a constant declaration — but CI is the arbiter, not me.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (1)

• [native-codex] server/src/services/recovery/service.ts:10745 — the in-transaction stale-lock revalidation trusts busySparedByRunId without rechecking the busy-pod absolute ceiling. If the transaction waits past AGENT_POD_BUSY_MAX_STALE_MS after the pre-transaction probe, this branch still returns false and preserves the issue lock, while isBusySparedRunningHolder and the hard-stale reaper explicitly require busy holders past that ceiling to be reclaimed.
  • Recompute the current silence age in currentRunningLockSilent and only honor the memoized busy result while it remains below AGENT_POD_BUSY_MAX_STALE_MS; otherwise return true and clear the lock. Add a regression test that advances the clock or otherwise makes the transaction revalidation cross the ceiling.

Suggestions (0)

Strengths

• The CPU probe fails closed when metrics are unavailable and sums usage across containers.
• The PR adds regression coverage for the reaper/sweeper lock-ownership seam and keeps the stale-lock behavior bounded in the normal path.

Recommended Action

1. Address the Important issue before merge.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:09c8e2b important 1 — still-present — server/src/services/recovery/service.ts:10745 — the in-transaction stale-lock revalidation still returns false from busySparedByRunId without rechecking the current silence age against AGENT_POD_BUSY_MAX_STALE_MS. If the transaction waits past the busy-pod ceiling after the pre-transaction probe, the issue lock remains preserved indefinitely, unlike the pre-transaction check and hard-stale reaper, which reclaim busy holders after that ceiling.

Critical Issues (0)

Important Issues (1)

• [native-codex] prior:09c8e2b important 1 server/src/services/recovery/service.ts:10745 — the in-transaction stale-lock revalidation trusts a memoized busy result after the busy-pod absolute ceiling may have elapsed. A CPU-burning zombie can therefore retain its issue lock beyond the advertised reclamation bound.
  • Recompute silentMs in currentRunningLockSilent and only honor the memoized busy result while it is below AGENT_POD_BUSY_MAX_STALE_MS; otherwise return true. Add a regression test that crosses the ceiling during transaction revalidation.

Suggestions (0)

Strengths

• The CPU probe fails closed when metrics are unavailable and sums usage across containers.
• The PR adds regression coverage for reaper and stale-lock behavior while preserving the bounded hard-stale fallback.

Recommended Action

1. Address the Important issue before merge.
2. Re-run the stale-lock and hard-stale liveness tests after adding the ceiling check.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:09c8e2b important 1 — still-present — server/src/services/recovery/service.ts:10745 — the in-transaction stale-lock revalidation still returns false from the memoized busySparedByRunId result without checking whether the current silence age has crossed AGENT_POD_BUSY_MAX_STALE_MS. The current head therefore can preserve a busy-looking holder's issue lock beyond the shared reclamation ceiling if the transaction begins before that boundary and revalidates after it.

Critical Issues (0)

Important Issues (1)

• [pr-review-toolkit/errors, gstack/review, native-codex] prior:09c8e2b important 1 server/src/services/recovery/service.ts:10745 — The currentRunningLockSilent revalidation trusts a pre-transaction memoized busy result after the busy-pod absolute ceiling may have elapsed. This can leave an issue lock held by a CPU-burning zombie beyond the advertised bound, allowing the same-agent sibling checkout to proceed only after the lock is incorrectly cleared later.
  • Recompute the current silence age in currentRunningLockSilent and honor the memoized busy result only while it remains below AGENT_POD_BUSY_MAX_STALE_MS; otherwise return true. Add a regression test that crosses the ceiling between the pre-transaction scan and in-transaction revalidation.

Suggestions (0)

Strengths

• The pod-CPU probe is performed during the pre-transaction candidate scan, while the in-transaction path reads only the memo and performs no Kubernetes network call under row locks.
• The shared silence bounds are hoisted into the leaf k8s-job-liveness module, whose imports are limited to Kubernetes client, logger, and redaction dependencies, avoiding a recovery/heartbeat cycle.
• Both exhaustive liveness mocks include the probe and hoisted bounds, and the cross-seam tests cover lock preservation, sibling checkout refusal, and ceiling expiry.

Recommended Action

1. Address the Important issue before merge.
2. Re-run the stale-lock seam and focused liveness tests after adding the ceiling revalidation.

[allyblockcast]

⚠️ Do not enqueue yet — this PR is now green, and merging it alone widens a concurrency window

Status check as of 2026-08-25T16:57Z, head 09c8e2b085114d5be02554216a64fd99bf9bd5f4: mergeStateStatus: CLEAN, 21 checks with zero failures and zero pending — including e2e, which passed at job 97808807403 (12:54:30Z → 13:34:49Z) now that the arc-e2e listener has recovered.

That is the problem. This PR was previously held back by the arc-e2e outage, which was acting as an accidental brake. It is now one command from landing, and landing it on its own ships a regression that BLO-30087 documents in detail.

The short version

There are two consumers of one "silence == dead" heuristic, and this PR fixes only one:

consumer	file	bound	touched here?
hard-stale reaper (kills the run)	server/src/services/heartbeat.ts:1630	EXTERNAL_LIFECYCLE_HARD_STALE_MS = 45 min	yes
stale-lock sweeper (frees the lock)	server/src/services/recovery/service.ts:255	STALE_RUNNING_ISSUE_LOCK_MS = 2 h	no

Before this PR, 45 min < 2 h meant the reaper always won: a silent run was killed long before the sweeper could reach its lock, so "live run holding no lock" was structurally unreachable. shouldDeferHardStaleKillForBusyPod removes that backstop by sparing a demonstrably busy pod up to EXTERNAL_LIFECYCLE_BUSY_POD_MAX_STALE_MS = 3 h — while the probe is a read that writes no activity column, so lastOutputAt / lastUsefulActionAt stay frozen at the original silence timestamp. The sweeper reads exactly those frozen columns.

Net: a ~60-minute window at silence 2h00m–3h00m where a run is alive and actively writing a shared workspace while its issue lock reads free. A sibling then gets a legitimate clean acquire (isNull(issues.executionRunId) is genuinely satisfied), and in shared_workspace mode both runs hold the same cwd. The PAPERCLIP_EXTERNAL_LIFECYCLE_BUSY_POD_MAX_STALE_MS override has no upper bound, so raising the ceiling widens the window proportionally.

The sweeper's own safety argument is explicitly DB-scoped — releaseIssueExecutionAndPromote is guarded by eq(executionRunId, run.id), which protects the issue row. Nothing protects the filesystem.

Why CI green here is not reassurance

Both test suites are individually thorough and both test their component in isolation. recovery-stale-issue-lock-sweep.test.ts asserts expect(run?.status).toBe("running") under the comment "Clearing the lock is non-destructive: the run row itself is untouched" — it encodes leaving the writer alive as the safety property. The new heartbeat-hard-stale-subprocess-liveness.test.ts asserts the run survives, but not that it still owns its lock. No test crosses the seam, so a fully green board is exactly what this failure mode looks like.

Ask

Fold the sweeper-side fix into this PR before enqueueing — per BLO-30087's acceptance criteria, minimally:

1. A constant-drift assertion pinning STALE_RUNNING_ISSUE_LOCK_MS >= EXTERNAL_LIFECYCLE_BUSY_POD_MAX_STALE_MS (or deriving both from one source), so the 45 min / 2 h / 3 h relationship cannot silently invert again.
2. A cross-seam test: seed a running holder at 2 h 30 m silence with probeAgentPodActivity → "busy", run sweepStaleIssueLocks(), and assert both that issues.executionRunId is unchanged and that a sibling checkout receives 409 rather than a clean acquire. Asserting only the first reproduces the blind spot.
3. Regression guard: same fixture with activity "idle" and "unknown" still clears the lock on today's schedule, preserving BLO-19941 / BLO-22060 behaviour and issueLockReleaseCount accounting.

Degradation stays safe: with metrics-server absent every probe returns "unknown" and sweeper behaviour is byte-for-byte pre-change, matching the fail-closed posture this PR already adopts for the reaper.

Note this repo merges through a merge queue (a direct merge returns 405 Changes must be made through the merge queue), so enqueueing is the point of no return — there is no post-enqueue review step that would catch this.

Flagging rather than pushing to this branch directly, since I don't own the in-flight work here.

[allyblockcast]

Disposition of the recurring Important finding (recovery/service.ts:10745)

Raised at head 09c8e2b three times (12:49, 12:57, 15:53), escalating from "preserves the issue lock" to "remains preserved indefinitely". I investigated it as the PR author and it is not reachable. No behaviour change in this push; I added the guard that was actually missing.

Why the branch cannot be entered with true

Being spared and reaching the transaction are mutually exclusive:

executionLockExpired = isPreClaimLockExpired(...) || runningLockSilent
runningLockSilent    = isRunningLockSilent(...) && !isBusySparedRunningHolder(...)

service.ts:10645 — if (!isCleanable(issue.executionRunId) && !executionLockExpired) continue; — a running holder is never isCleanable, so a spared one hits continue before db.transaction is opened. A running holder therefore only reaches the transaction when isBusySparedRunningHolder returned false, which means busySparedByRunId is false or absent exactly when currentRunningLockSilent reads it. The === true arm is defensive, not load-bearing.

Two corrections to the finding as written:

• "indefinitely" is wrong regardless. busySparedByRunId is declared at 10565, inside sweepStaleIssueLocks() (10374) — it is per-invocation. Even if the branch were reachable, the next sweep re-probes against a fresh silentMs past the ceiling and clears. Exposure would be one sweep cycle, not unbounded.
• I did find the narrower thing the finding gestures at: in the pre-transaction path (10574-10575) the memo read precedes the ceiling gate, so a run holding locks on several issues that crosses AGENT_POD_BUSY_MAX_STALE_MS mid-loop keeps them for the remainder of that invocation. Self-correcting on the next sweep, never reaches the flagged line, and not worth code churn on a green PR — recorded on BLO-30087 rather than fixed here.

What I changed instead

I first wrote the regression test the review asked for — crossing the ceiling between the probe and the in-transaction revalidation. It failed, because the spared holder never enters the transaction, so the hook never fires. That failure is the proof above.

The real gap is that this guarantee lives in the ordering of two guards ~150 lines apart and nothing enforced it. Reorder the candidate filter and the branch stops being a no-op and becomes the bug it was mistaken for. So this push adds that guard (test-only, recovery-stale-issue-lock-sweep.test.ts):

• a busy, in-band holder is asserted not to reach the transaction and to keep every lock column;
• a control holder in the same band with no busy evidence is asserted to reach the transaction and be cleared — so the negative assertion cannot pass vacuously.

Mutation-checked: dropping the busy spare from the candidate filter makes the new case fail (expected [ …(2) ] to not include '<busy issueId>'); on unmodified code the suite is 46/46 green locally against embedded Postgres.

I am the author and cannot approve my own PR — flagging for Release Engineer once CI is green at 8d13b6f8. Note mergeable_state is behind; it needs a rebase before merge.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:09c8e2b important 1 — still-present — server/src/services/recovery/service.ts:10745 — the in-transaction stale-lock revalidation still returns false from the memoized busySparedByRunId result without checking whether the current silence age has crossed AGENT_POD_BUSY_MAX_STALE_MS. A CPU-burning zombie can therefore retain its issue lock beyond the shared reclamation ceiling if the transaction revalidates after that boundary.

Critical Issues (0)

Important Issues (1)

• [pr-review-toolkit/errors, gstack/review, native-codex] prior:09c8e2b important 1 server/src/services/recovery/service.ts:10745 — The currentRunningLockSilent revalidation trusts a pre-transaction memoized busy result after the busy-pod absolute ceiling may have elapsed. This can leave an issue lock held by a CPU-burning zombie beyond the advertised bound, allowing a sibling checkout to proceed only after the lock is incorrectly cleared later.
  • Recompute the current silence age in currentRunningLockSilent and honor the memoized busy result only while it remains below AGENT_POD_BUSY_MAX_STALE_MS; otherwise return true. Add a regression test that crosses the ceiling between the pre-transaction scan and in-transaction revalidation.

Suggestions (0)

Strengths

• The CPU probe fails closed when metrics are unavailable and sums usage across containers.
• The PR adds regression coverage for the reaper and stale-lock seam while preserving bounded hard-stale fallback behavior.

Recommended Action

1. Address the Important issue before merge.
2. Re-run the stale-lock seam and focused liveness tests after adding the ceiling revalidation.

[allyblockcast]

Disposition: the service.ts:10745 Important finding is unreachable (4th consecutive head)

Re-verified against the current head 8d13b6f8 after the 4th at-head review repeated it. Recording the full control-flow proof so this doesn't need re-deriving a fifth time.

Claim under review: currentRunningLockSilent (:10745) trusts busySparedByRunId.get(runId) === true without re-checking AGENT_POD_BUSY_MAX_STALE_MS, so a CPU-burning zombie could retain its lock past the ceiling — escalated in an earlier pass to "indefinitely".

Why it cannot happen. For a running execution holder, reaching :10745 requires reaching db.transaction at :10636. But:

1. runningLockSilent = isRunningLockSilent(...) && !(await isBusySparedRunningHolder(...)) (:10592-10598). If the run is busy-spared, runningLockSilent is false.
2. isCleanable() is false forever for running (:238), and isPreClaimLockExpired doesn't apply. So executionLockExpired is false, and :10634 — if (!isCleanable(executionRunId) && !executionLockExpired) continue; — skips the issue before the transaction is opened.
3. Therefore a running holder enters the transaction only when isBusySparedRunningHolder returned false for that runId.
4. That function returns false either from the memo/compute path — which writes busySparedByRunId[runId] = false — or via the early return at :10573 (silentMs < STALE_RUNNING_ISSUE_LOCK_MS). The early return is impossible here: isRunningLockSilent is evaluated first (left of &&), shares the identical runningLockStaleBasis, and calls Date.now() earlier, so if it passed >= 2h the later silentMs is only larger.
5. Identity is guaranteed, not assumed: the transaction bails at the top with if (currentIssue.executionRunId !== issue.executionRunId) return LOCK_CHANGED_UNDER_SWEEP;.

So at :10745 the memo is always false for that runId — the === true arm is defensive, never load-bearing.

"Indefinitely" is wrong independently of the above: busySparedByRunId is declared at :10565, inside sweepStaleIssueLocks(). It is per-invocation. The next sweep starts with an empty memo and re-probes against a fresh silentMs. Worst case would be one sweep cycle, not unbounded.

Evidence, not just reading. In an earlier pass I wrote the exact regression test the review asks for. It failed — the beforeStaleIssueLockSweepClearForTest hook never fired, because a spared holder never enters the transaction. That failure is the disproof. I reverted the behavioural change rather than add unreachable code, and instead landed 8d13b6f8 (test-only, +76) pinning the invariant, with a control holder in the same silence band and no busy evidence that must be cleared — so the negative assertion cannot pass vacuously. Mutation-checked: removing the busy spare from the candidate filter makes it fail.

What is real, and is now tracked. There is a genuine quirk nearby, and it is not the flagged line: at :10574-10575 the memo read precedes the ceiling gate at :10579, so a run holding locks on multiple issues that crosses the 3h ceiling mid-loop keeps them for the remainder of that one invocation. Bounded by one sweep interval, self-correcting, fails closed without metrics-server. Split out as BLO-30245 with acceptance criteria and a mutation-checked test plan rather than folded in here — this PR is green-pending at 8d13b6f8 with e2e finally running after the BLO-30085 ARC outage, and a synchronize push would cancel it.

Status: 0 failing checks at this head. Not self-merging — I authored this PR; it goes to Release Engineer, and needs a rebase first (mergeable_state: behind).

[allyblockcast]

Correction — my previous comment was wrong, and the blocking review finding is unreachable

Two things to clear up, one of them mine.

1. Retracting my own comment above

My comment at 16:57:35Z claimed the stale-lock sweeper in server/src/services/recovery/service.ts was not touched by this PR, and asked for the BLO-30087 fix to be folded in before enqueueing.

That was already done when I wrote it. Commit 09c8e2b — "fix(recovery): stale-lock sweeper must honour the busy-pod liveness signal" — landed at 12:44:22Z, four hours and thirteen minutes before my comment, and touches exactly the file I said was untouched (recovery/service.ts +73/−2, recovery-stale-issue-lock-sweep.test.ts +174/−2). The comparison table in that comment describes the head before this PR's last commit. All three things I asked for — the busy-pod consult, the cross-seam sibling-checkout test, the constant-drift assertion — were present on the branch as I was asking for them.

The mechanism of the error is worth naming, since it is cheap to repeat: the analysis was carried over from head e8c5024c and posted against head 09c8e2b without re-reading the diff. Ignore that comment. The sequencing concern it raises is real but is already satisfied by this PR.

2. The blocking Important finding is real in form, unreachable in fact

Ally has now raised the same finding four times (12:49, 12:57, 15:53 at 09c8e2b, and 17:16 at 8d13b6f), each time as "still-present": that currentRunningLockSilent reads the busySparedByRunId memo without re-deriving AGENT_POD_BUSY_MAX_STALE_MS, so a zombie could hold its lock "beyond the advertised bound".

The code shape is exactly as described. The failure mode does not exist, for two independent reasons.

It cannot be entered. Being spared and reaching the transaction are mutually exclusive:

executionLockExpired = isPreClaimLockExpired(...) || runningLockSilent
runningLockSilent    = isRunningLockSilent(...) && !isBusySparedRunningHolder(...)

A busy spare makes runningLockSilent false, hence executionLockExpired false, so a non-cleanable running holder hits continue at recovery/service.ts:10634 before db.transaction ever opens. The memo is false or absent exactly when the branch runs. I measured this rather than reasoning about it: seeding a holder at 2h30m silence with the probe forced to "busy", beforeStaleIssueLockSweepClearForTest never fires for that issue.

And even granting reachability, the impact is not the one stated. busySparedByRunId is a fresh Map per sweepStaleIssueLocks() invocation (declared inside the function), and the sweep re-runs every heartbeatSchedulerIntervalMs — 30s by default (config.ts:523, floor 10s). The next pass recomputes silentMs >= AGENT_POD_BUSY_MAX_STALE_MS, spares nothing, and clears. The worst case would be one sweep interval of extra retention in a race that requires the transaction to straddle the 3h boundary — not "indefinitely", and not "beyond the advertised bound".

8d13b6f pins this with a negative assertion plus a non-vacuous control, and mutation-checks it (dropping the busy spare from the candidate filter makes it fail). That is the right fix for a finding of this shape: the guarantee previously lived in the ordering of two guards ~150 lines apart with nothing enforcing it, and reordering them would have turned the no-op into the bug it was mistaken for. I verified that suite independently: 46/46 green, and tsc --noEmit -p server/tsconfig.json clean.

I also built the change Ally asked for — recompute silentMs and honour the memo only below the ceiling — and then dropped it. It is unfalsifiable by construction: the regression test Ally specifies ("crosses the ceiling between the pre-transaction scan and in-transaction revalidation") cannot pass, because the revalidation is never entered. Writing one to close the finding would have meant asserting behaviour the code cannot exhibit. A defensive recheck is defensible as hedging against future drift, but 8d13b6f's test covers that drift directly and more honestly, so the behaviour change is not worth carrying.

Recommendation: this finding should not block merge. It is a static-shape observation without a reachability step, and it has survived four rounds because each round re-derives it from the same code rather than from the new test.

Current state

• Head 8d13b6f, CI in progress (9 pending / 9 success / 1 skipped at time of writing). e2e passed at the prior head now that the arc-e2e listener has recovered.
• mergeable_state: behind — needs a rebase on master before enqueueing.
• Not re-requesting review: a fifth pass on unchanged logic would re-emit the same finding.

[allyblockcast]

Ready to enqueue — do not rebase

Status at head 8d13b6f8: 19/19 checks green (including e2e, which completed once the BLO-30085 ARC listener outage cleared), mergeable: true, mergeable_state: clean.

One correction to the record, since my earlier note on this PR said "rebase first": mergeable_state has flipped behind → clean, but the branch is still behind_by: 15 against master@dc2bfaa4. That is not a gap. This repo lands through a GitHub merge queue — pr.yml carries merge_group: types: [checks_requested] and documents maximumEntriesToBuild=1 — so the queue builds this PR merged onto the current master tip and runs the full suite on that merge-group head before it lands. Three such builds succeeded today (pr-1491, pr-1495, and pr-1494 → dc2bfaa4). The merged result is gated by the queue, not by this PR's head checks.

So: enqueue, don't rebase. A synchronize push buys nothing the queue doesn't already do, and it would invalidate the at-head review — producing a 5th consecutive repeat of the service.ts:10745 finding already disproved in #1465 (comment).

Pre-diagnosed merge-group failure vector

Recording this so nobody re-derives it if the queue build goes red. Master's 15 new commits concurrently modified two of this PR's source files:

• server/src/services/heartbeat.ts — master +72/-4, this PR +64/-1. No hunk overlap (master's hunks sit at @24829–@32486, this PR's at @106–@20276), and git reports no conflict. But master's 0a835e31 classify dependency-blocked timer parks and 249acced recover expired capacity retries (PEN-2190) act on an overlapping population with this PR's shouldDeferHardStaleKillForBusyPod — both reason about "runs that look dead". Textual non-overlap does not rule out behavioural interaction. If a merge-group build fails, server/src/__tests__/heartbeat-hard-stale-subprocess-liveness.test.ts is the first place to look.
• server/src/services/productivity-review.ts — a one-line visibility change to NON_LIVE_EXECUTION_SILENCE_MS here vs master's 8afd2b4c emit one monitor suppression per wait. Different regions, no interaction expected.

This is exactly the case the merge queue exists for, so I am not treating it as a blocker — just as a pre-labelled suspect.

Handing off to Release Engineer to enqueue. Not self-merging: I authored this PR.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:09c8e2b important 1 — no-longer-applicable — server/src/services/recovery/service.ts:10634 — a busy-spared running holder makes runningLockSilent false before the candidate filter; because a running holder is non-cleanable, this guard skips it before the transaction and before currentRunningLockSilent can read the memo. A stale memo therefore cannot preserve that holder beyond the busy ceiling through the flagged branch.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The reaper and stale-lock sweeper now share the pod-CPU liveness signal while keeping Kubernetes reads outside the database transaction.
• Metrics failures remain unknown, preserving the prior fail-closed reaping and lock-clearing behavior.
• The current tests cover busy, idle, unknown, ceiling, malformed tunables, exhaustive mocks, and the candidate-filter reachability invariant.

Recommended Action

1. No Critical or Important issues found at this exact head.
2. Merge after the repository merge-queue checks complete.

[kkroo]

Ally exact-head review reports no Critical, Important, or Suggestion findings. Approving review coverage only; merge remains subject to required checks and branch protection.

[allyblockcast]

Root cause of the 37 silent merge-queue ejections: the queue's REBASE conflicts, while mergeStateStatus reports on a MERGE

#1465 was ejected again at 2026-08-28T12:49:26Z — the 37th — and this time was not re-added. It is now out of the queue, and the queue is empty. Over ~13h it has never once produced a merge_group build; every gh-readonly-queue/master/pr-* ref belongs to some other PR.

I reproduced the ejection locally. It is not flakiness, not capacity, and not this PR's tests.

The mechanism

merge_method: REBASE. The queue must replay this branch's commits onto master tip. That rebase fails:

$ git rebase origin/master     # onto aae23297, branch is ahead 10 / behind 108
Rebasing (5/7)
CONFLICT (content): Merge conflict in scripts/general-server-shard-durations.json
error: could not apply 06384a3d... chore(ci): record the new subprocess-liveness suite's shard duration (BLO-20251)

The conflict is only in the $comment prose key. Master's 64e4b5dc ("make shard manifest refresh reliable", 2026-08-24) rewrote $comment and added a durable $notes key; this PR's 06384a3d rewrote the same $comment differently. The actual data — heartbeat-hard-stale-subprocess-liveness.test.ts: 22961 — does not conflict at all.

GitHub ejects on this with no stated reason: the timeline events carry commit_id: null and no body, and github-merge-queue[bot] posts nothing. That is why 37 ejections produced zero diagnostic signal.

Why the PR reads permanently green while being permanently un-rebaseable

mergeable: MERGEABLE / mergeStateStatus: CLEAN are computed for a merge. A merge of master into this branch does not conflict. A rebase does. Those two answers have diverged here, and only the second one governs landing.

This also explains why merging master in never helped, though it was the right instinct:

• e8c5024c is a merge of master that did resolve this exact file.
• git rebase drops merge commits and replays only the non-merge ones — 10 commits in, Rebasing (1/7)…(7/7) out, all 3 merge commits discarded.
• So every conflict resolution carried in a merge commit is thrown away, and the raw conflicting 06384a3d is replayed against a newer master each time.

Merging master in again will not fix this. It cannot.

The fix (verified end-to-end, not pushed)

Rebase onto master, keeping master's $comment + $notes and this branch's duration entry. I ran it: one conflict, trivial resolution, remaining commits replay clean, result is valid JSON with 444 suites and the new entry present.

21c4329e test(recovery): pin that a spared busy holder never reaches the sweep transaction
7250cc84 fix(recovery): stale-lock sweeper must honour the busy-pod liveness signal
507881f7 chore(ci): record the new subprocess-liveness suite's shard duration (BLO-20251)
ef904d81 fix(k8s): isolate optional metrics client so it cannot fail open (BLO-20251)
69d71034 fix(heartbeat): fail closed on a malformed liveness threshold (BLO-20251)
8c0e7051 test(heartbeat): list probeAgentPodActivity in exhaustive k8s mocks (BLO-20251)
2eab15b2 fix(heartbeat): don't reap runs blocked on a live subprocess (BLO-20251)

Why I have not pushed it

This needs a force-push, and @kkroo's APPROVED at head is the only human approval on it. I cannot read this repo's stale-dismissal setting (branches/master/protection → 403; the ruleset carries only merge_queue), so I cannot rule out that a force-push dismisses that approval and converts a mechanical problem into a wait on a human. The standing instruction on this PR is also explicitly no push, no rebase, no dequeue.

Three of the four reasons behind that instruction are now dead — there is no queue position left to lose, no CI in flight to cancel, and "the queue rebases onto master itself so a manual rebase buys nothing" is exactly the premise this disproves. The fourth (the approval) is real and unverifiable by me, so I am asking rather than acting.

@kkroo — re-adding to the queue will fail again, deterministically, until the rebase happens. 37 for 37. If you're happy for the branch history to be rewritten, say so (or push the rebase yourself) and I'll take it from there.

[allyblockcast]

@ally please re-review at head 0693e464e1b8e96e760a6df96d1505517393242d.

What changed and why. The branch was rebased onto master@aae23297 and force-pushed under an explicit board authorization (approval f9376b19, approved by kkroo 2026-08-28T14:34:12Z). This reverses the earlier "enqueue, don't rebase" guidance on purpose: the queue's merge_method is REBASE, and replaying 06384a3d conflicted with master's 64e4b5dc in the $comment prose key of scripts/general-server-shard-durations.json. That conflict is what silently ejected this PR 37 times with commit_id: null and no diagnostic. Rebasing locally is the only thing that clears it.

The rebase is content-neutral. I verified the rebased tree is byte-identical to merging the previously-reviewed head into master (git diff master..rebased == git diff master..(master+9fe2ca7d), no delta). The only conflict resolution was prose: master's $comment + $notes kept, this branch's duration entry kept — 444 suites, manifest valid, check-shard-manifest-freshness.mjs exit 0. kkroo's approval carried forward to the new head (stale=false).

On the recurring recovery/service.ts:10745 Important finding. Please don't re-raise it as blocking. It has been raised on four consecutive heads and was disproved in detail; the residual narrow case is tracked separately as BLO-30245 (todo, low, Chief Architect). It is deliberately out of scope here. The review/ally-comment gate currently reads carried_finding purely because the force-push moved the head and nothing attests 0693e464 yet — an attestation of this head is exactly what clears it.

Focus: confirm the rebase preserved the BLO-30087 fix (recovery/service.ts busy-pod consult, the cross-seam sibling-checkout 409 assertion, and the constant-drift assertion) intact at this head. Locally: 72/72 tests pass across the three affected suites, tsc --noEmit clean.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• Pod CPU is summed across containers, so DinD-backed subprocesses count as activity, while the busy-pod deferral has an absolute stale ceiling.
• Metrics failures, missing samples, and malformed quantities remain unknown rather than being treated as idle, preserving fail-closed reaping behavior.
• The optional metrics client is isolated from core Kubernetes client initialization, so metrics API drift cannot disable the existing dispatch guard.
• The reaper and stale-lock sweeper share the liveness bounds, and tests cover the reaper/sweeper lock-ownership seam and sibling checkout safety.

Recommended Action

1. No Critical or Important issues found at this exact head.
2. Merge after the repository's pending CI checks complete.