fix(heartbeat): bounded-retry stale-killed pr_review runs (BLO-18030)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• One of its load-bearing automations is the PR-review wake pipeline: a GitHub webhook creates an agent_wakeup_requests row, which dispatches an external-lifecycle Job that runs the reviewer agent
• That Job can be force-terminated by the reaper when it goes silent past EXTERNAL_LIFECYCLE_HARD_STALE_MS (45m), which sets the run terminal with errorCode: external_lifecycle_stale_killed and the wake row to status='failed'
• Nothing recovers that state: shouldScheduleAutomaticRunRetry has no branch for this errorCode, and reconcileFailedWakeDispatches only selects dispatch_failed — so the review silently never happens and no signal surfaces it
• Observed 2026-07-25 on a real PR: the wake sat queued 3h47m, was claimed, was killed at exactly 45m, and the review was simply lost
• This pull request makes a stale-killed pr_review run eligible for the existing bounded retry, but only when a GitHub probe proves no review was already posted
• The benefit is that a capacity-induced stale-kill no longer silently drops a code review, without ever risking a duplicate review

Linked Issues or Issue Description

Refs #900 — tracked internally as BLO-18030 (this repo's issue tracker is not the system of record for that work).

Since there is no public GitHub issue, describing the bug inline per CONTRIBUTING.md → "Link Issues or Describe Them In-PR":

What happened: A PR received a push. The reviewer wake was created, sat queued for 3h47m under load, was claimed, then went silent and was force-terminated by the hard-stale reaper exactly 45 minutes after claim. The review never happened, and nothing alerted.

Expected: A reviewer wake lost to infrastructure staleness should be retried, or at minimum surfaced.

Actual: The run is terminal with no retry, and the agent_wakeup_requests row sits at status='failed' forever — a status no reconciler selects.

Root cause: shouldScheduleAutomaticRunRetry (server/src/services/heartbeat.ts:754) handles job_failed, job_missing, k8s_pod_schedule_failed, pr_review_auth_expired, pr_review_output_missing, adapter_failed, process_lost — but falls through to an early return false for external_lifecycle_stale_killed. A retry path exists at heartbeat.ts:15429 (added for job-vanish recovery) but consults that same predicate, so stale-kills never reach it.

Related: #880 also touches PR-review completion evidence. No overlap in the code paths changed here, but worth a reviewer's eye if both land.

What Changed

• server/src/services/heartbeat.ts — finalizeExternalLifecycleTerminalRun now probes GitHub for reviewer evidence at the run's head on the stale-kill branch (reusing githubHasReviewerEvidenceForPr, already used on the sibling non-stale-kill branch) and records the outcome as externalLifecycleRecovery.reviewEvidenceFound.
• server/src/services/heartbeat.ts — shouldScheduleAutomaticRunRetry gains an external_lifecycle_stale_killed branch that returns true only when reviewEvidenceFound === false and the run is a pr_review context.
• server/src/__tests__/heartbeat-retry-scheduling.test.ts — 3 new cases.

The terminal verdict is unchanged: a force-terminated silent Job is still a failure. This only decides retry eligibility.

Why the probe rather than a blanket retry: unlike job_missing / k8s_pod_schedule_failed, where the pod provably never ran, a stale-killed run was running and may already have posted a review.

probe outcome	recorded	retried?
no review found	reviewEvidenceFound: false	✅ yes (pr_review only)
review found	reviewEvidenceFound: true	❌ terminal — would double-review
probe threw / no PR context	(flag absent)	❌ terminal — unproven ≠ safe

Verification

npx vitest run server/src/__tests__/heartbeat-retry-scheduling.test.ts \
               server/src/__tests__/heartbeat-process-recovery.test.ts
# → Test Files 2 passed | Tests 205 passed

npx vitest run server/src/__tests__/heartbeat-external-runtime-retry.test.ts \
               server/src/__tests__/k8s-job-liveness.test.ts \
               server/src/__tests__/run-liveness.test.ts \
               server/src/__tests__/heartbeat-external-lifecycle-concurrency-flag.test.ts
# → Test Files 4 passed | Tests 51 passed

cd server && npx tsc --noEmit -p tsconfig.json   # → clean

New tests cover the positive gate, the double-review guard (found / probe-errored / absent-flag), and the non-pr_review leak guard including malformed snapshots.

Confirmed non-vacuous: reverting only the new predicate branch and re-running fails the positive test (Tests 1 failed | 2 passed) — the two negative tests pass either way by construction, which is expected.

Risks

• Double review — the primary risk, and the reason for the probe gate. Retry requires a definitive reviewEvidenceFound === false; a found review, a thrown probe, and an absent flag all stay terminal. Absence of proof is deliberately not treated as proof of absence.
• Retry leak — bounded by the existing isPrReviewRetryContext gate, matching the k8s_concurrent_run_blocked / job_missing precedent. Non-PR contexts stay terminal. Covered by an explicit test.
• Extra GitHub API call on the reaper path — one githubHasReviewerEvidenceForPr call per stale-kill finalize. Stale-kills are rare; the call is wrapped in try/catch and a failure degrades safely to "stay terminal". Flagged for reviewer judgement.
• No migration, no schema change, no UI change. externalLifecycleRecovery is an existing free-form JSON blob in resultJson; the new key is additive and read defensively via parseObject.
• Not covered here: rows that legitimately stay terminal are still not alertable — githubReviewRequestDeadLetterUnresolved counts only dispatch_failed_exhausted, not failed. Deliberately filed as a separate follow-up rather than widened into this PR.

Model Used

Claude Opus 5 (claude-opus-5), 1M context, extended thinking, with tool use and code execution — running as a Paperclip agent (claude_k8s adapter). All code, tests, and verification commands in this PR were produced and executed by the model.

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 — n/a; behavior is documented in-code at both changed sites
• I have considered and documented any risks above
• All Paperclip CI gates are green
• 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-18030
🔗 Paperclip issue: BLO-18953

[allyblockcast]

🔗 Paperclip issue: BLO-18030
🔗 Paperclip issue: BLO-18953

[allyblockcast]

@ally please review at head 9763b278.

Focus:

1. Double-review safety. The retry gate keys on externalLifecycleRecovery.reviewEvidenceFound === false. Confirm a probe error, a found review, and an absent flag all stay terminal — absence must never read as "safe to retry".
2. Guard placement. The new external_lifecycle_stale_killed branch sits immediately above the adapter_failed/process_lost early-return in shouldScheduleAutomaticRunRetry. Confirm it does not shadow or bypass any guard below it.
3. Probe cost/failure mode. githubHasReviewerEvidenceForPr now runs on every stale-kill finalize, not just pr_review ones. Is the extra API call acceptable on the reaper path, and is the try/catch swallow correct there?
4. Retry-leak surface. Non-pr contexts must stay terminal; verify the isPrReviewRetryContext gate is not weakened by the new branch.

[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: ## Verification
• Missing section: ## Risks
• Missing section: ## Model Used
• No linked issue or inline issue description found — either tag an existing issue with Fixes #NNN / Closes #NNN / Refs #NNN, or describe the underlying issue inline in the PR body following one of our issue templates (https://github.com/paperclipai/paperclip/tree/master/.github/ISSUE_TEMPLATE). See CONTRIBUTING.md → "Link Issues or Describe Them In-PR".
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

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

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (1)

• [gstack/review + native-codex] server/src/services/heartbeat.ts:15323 — The negative GitHub probe runs while the stale Job is still live. That Job can post after the probe returns found: false but before deletion, and deletion can also fail or return mismatch; in both cases the persisted reviewEvidenceFound: false still reaches retry scheduling. This violates the double-review invariant. Claim and quiesce the exact Job before probing, and authorize retry only when quiescence succeeded and the subsequent exact-head probe returned found: false.

Important Issues (2)

• [pr-review-toolkit/errors] server/src/services/heartbeat.ts:15326 — Passing a nullable headSha is not fail-closed for this caller. If the helper's fallback PR-head fetch fails, githubHasReviewerEvidenceForPr can skip comment evidence and return { found: false } rather than { error }; a comment-mode review may therefore exist while this path records definitive absence and retries. Require an exact head for stale-kill recovery, or make failure to resolve it return an error/unproven result.
• [pr-review-toolkit/code] server/src/services/heartbeat.ts:823 — This new safety gate is below the unconditional transient-recovery return at line 757. Because stale-kill finalization preserves the prior resultJson, any retained errorFamily can make shouldScheduleAutomaticRunRetry return true before checking reviewEvidenceFound or PR context, including when evidence is present or the probe failed. Handle external_lifecycle_stale_killed before generic transient families, and add tests combining the stale-kill code with transient metadata.

Strengths

• The explicit reviewEvidenceFound !== false check correctly treats missing and positive evidence as terminal within the new branch.
• The PR-context predicate keeps ordinary non-PR snapshots terminal when no earlier retry contract bypasses it.
• The added unit cases clearly document the intended retry contract and malformed-snapshot behavior.

Recommended Action

1. Quiesce the original Job before establishing reusable negative evidence.
2. Make unresolved head/probe state terminal and move the stale-kill gate ahead of generic retry contracts.
3. Add integration coverage for deletion failure, a late review between probe and kill, missing-head fetch failure, and stale-kill results carrying transient metadata.

[kkroo]

/ally review

[kkroo]

/test
/ally review

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:9763b27 critical 1 — still-present — server/src/services/heartbeat.ts:15541 — The negative GitHub probe still runs before the winning terminal claim and exact Job deletion; retry scheduling at line 15739 does not require terminalJobQuiesced, so a late review or failed deletion can coexist with persisted reviewEvidenceFound: false.
• prior:9763b27 important 1 — still-present — server/src/services/heartbeat.ts:15543 — The stale-kill path still accepts a PR context with no exact headSha, passes the nullable value to the probe at line 15548, and treats found: false as definitive absence.
• prior:9763b27 important 2 — still-present — server/src/services/heartbeat.ts:758 — Generic transient recovery still returns true before the stale-kill gate, while stale-kill finalization preserves prior top-level resultJson metadata at line 15649.

Critical Issues (1)

• [prior:9763b27 critical 1; gstack/review + native-codex] server/src/services/heartbeat.ts:15541 — The original reviewer Job can still post after the negative probe and before deletion, and deletion can fail or return mismatch; either path leaves negative evidence that authorizes a duplicate retry. Claim and quiesce the exact Job before probing, and authorize retry only when quiescence succeeded and the subsequent exact-head probe returned found: false.

Important Issues (2)

• [prior:9763b27 important 1; pr-review-toolkit/errors] server/src/services/heartbeat.ts:15543 — Nullable headSha cannot establish exact-head absence. Fail closed unless the persisted run has an exact head, or make failure to resolve that head return an unavailable/error result that cannot set reviewEvidenceFound: false.
• [prior:9763b27 important 2; pr-review-toolkit/code] server/src/services/heartbeat.ts:758 — A retained errorFamily such as transient_upstream, rate_limit_exhausted, or provider_quota bypasses every new stale-kill safety check, including positive/absent review evidence and PR-context validation. Handle external_lifecycle_stale_killed before generic transient contracts and add mixed-metadata tests.

Strengths

• The dedicated stale-kill branch correctly treats missing or positive evidence as terminal when no earlier retry contract bypasses it.
• The predicate tests clearly cover positive, found-review, absent-proof, malformed-context, and non-PR cases.
• The exact Job deletion helper requires persisted name and UID, and the terminal CAS prevents two reapers from deleting the same Job.

Recommended Action

1. Quiesce the original Job before establishing reusable negative evidence.
2. Require an exact persisted head for the probe and move the stale-kill gate ahead of generic retry contracts.
3. Add integration coverage for deletion failure, a late review between probe and kill, missing-head resolution, and stale-kill results carrying transient metadata.

This PR is authored by app/allyblockcast, so the reviewer App cannot approve it. The exact head must be reopened under an independent author before an App approval is possible; the shared User token is not a substitute for the App gate.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:9763b27 critical 1 — still-present — server/src/services/heartbeat.ts:15553 — The negative GitHub probe still runs before the terminal claim and exact Job deletion; retry scheduling at line 15750 does not require terminalJobQuiesced, so a late review or failed deletion can coexist with persisted reviewEvidenceFound: false.
• prior:9763b27 important 1 — still-present — server/src/services/heartbeat.ts:15555 — The stale-kill path still accepts a PR context with no exact headSha, passes the nullable value to the helper at line 15560, and can treat the current PR head's found: false result as evidence about a different wake head.
• prior:9763b27 important 2 — still-present — server/src/services/heartbeat.ts:758 — Generic transient recovery still returns true before the stale-kill gate, while stale-kill finalization preserves prior top-level resultJson metadata at line 15661.

Critical Issues (1)

• [prior:9763b27 critical 1; gstack/review + native-codex] server/src/services/heartbeat.ts:15553 — The original reviewer Job can post after the negative probe and before deletion, and deletion can fail, return mismatch, or remain asynchronously terminating. Each path leaves negative evidence that authorizes a duplicate retry at line 15750. Claim and confirm quiescence of the exact Job before probing, and authorize retry only when quiescence succeeded and a subsequent exact-head probe returned found: false.

Important Issues (2)

• [prior:9763b27 important 1; pr-review-toolkit/errors] server/src/services/heartbeat.ts:15555 — Nullable headSha cannot establish absence at the run's exact head. The helper falls back to the PR's current head, which may have advanced since the wake. Require the persisted exact wake head for stale-kill recovery, or treat unresolved head state as unavailable and terminal.
• [prior:9763b27 important 2; pr-review-toolkit/code] server/src/services/heartbeat.ts:758 — A retained errorFamily such as transient_upstream, rate_limit_exhausted, or provider_quota bypasses every stale-kill safety check, including positive or absent review evidence and PR-context validation. Handle external_lifecycle_stale_killed before generic transient contracts and add mixed-metadata tests.

Strengths

• The dedicated stale-kill predicate correctly treats missing or positive evidence as terminal when no earlier retry contract bypasses it.
• The predicate tests clearly cover positive, found-review, absent-proof, malformed-context, and non-PR cases.
• Exact Job deletion requires persisted name and UID, and the terminal status CAS prevents overlapping reapers from both winning finalization.

Recommended Action

1. Quiesce the original Job before establishing reusable negative evidence.
2. Require an exact persisted wake head and move the stale-kill gate ahead of generic retry contracts.
3. Add integration coverage for deletion failure, late publication, missing head, and stale-kill results carrying transient metadata.

This PR is authored by app/allyblockcast, so the reviewer App cannot approve its own PR. The exact head must be reopened under an independent author before an App approval is possible; the shared User token is not gate evidence and is not a substitute.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:9763b27 critical 1 — still-present — server/src/services/heartbeat.ts:15955 — The GitHub evidence probe still runs before the terminal CAS and exact Job deletion; retry scheduling at line 16146 does not require terminalJobQuiesced, so a late review or unsuccessful deletion can coexist with persisted reviewEvidenceFound: false.
• prior:9763b27 important 1 — still-present — server/src/services/heartbeat.ts:15958 — The stale-kill path still passes nullable prReview.headSha; githubHasReviewerEvidenceForPr falls back to the PR's current head, so a negative result need not describe the stale run's exact wake head.
• prior:9763b27 important 2 — still-present — server/src/services/heartbeat.ts:878 — readTransientRecoveryContractFromRun still returns true before the stale-kill branch, while finalization preserves prior top-level resultJson metadata at line 16055.

Critical Issues (1)

• [prior:9763b27 critical 1; gstack/review + native-codex] server/src/services/heartbeat.ts:15955 — The original reviewer Job can post after the negative probe and before deletion, and deletion can fail or return mismatch; either path leaves negative evidence that authorizes a duplicate retry at line 16146. Claim and quiesce the exact Job before probing, and require confirmed quiescence plus a subsequent exact-head found: false result before scheduling retry.

Important Issues (2)

• [prior:9763b27 important 1; pr-review-toolkit/errors] server/src/services/heartbeat.ts:15958 — Nullable headSha cannot prove absence at the stale run's exact revision. Require the persisted wake head for this recovery path; if it is missing or invalid, keep the run terminal rather than falling back to the PR's current head.
• [prior:9763b27 important 2; pr-review-toolkit/code] server/src/services/heartbeat.ts:878 — A retained transient_upstream, rate_limit_exhausted, or provider_quota family bypasses all stale-kill evidence and PR-context checks. Handle external_lifecycle_stale_killed before generic transient recovery, and add mixed-metadata tests proving positive, absent, and failed evidence remain terminal.

Strengths

• The dedicated stale-kill branch treats missing or positive evidence as terminal when no earlier retry contract bypasses it.
• The added predicate tests clearly document the intended positive, fail-closed, malformed-context, and non-PR behavior.
• The terminal CAS and exact-name/UID deletion reduce overlapping-reaper cleanup races.

Recommended Action

1. Quiesce the original Job before establishing reusable negative evidence.
2. Require the exact persisted wake head and move the stale-kill gate ahead of generic transient contracts.
3. Add integration coverage for deletion failure, late publication, missing head, and stale-kill results carrying transient metadata.

This PR is authored by app/allyblockcast, so the reviewer App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible; the shared User token is not gate evidence and is not a substitute.

[allyblockcast]

@ally please re-review at head 864f4b5e — this addresses all three findings from your review of 9763b278 (review 4834483992).

Note the branch was rebased twice since that review (9763b278 → 79a102c9 → 6107dadc) and the fixes were never actually pushed until now — 6107dadc was still the original pre-fix implementation, so if you diffed it against your last review you would have found all three findings still open. 864f4b5e is the first head that contains them.

Critical — probe ran while the killed Job was still live. The probe sat at the top of finalizeExternalLifecycleTerminalRun, before the terminal CAS claim and before the Job delete. deleteAgentJobExact uses propagationPolicy: "Background", so a "deleted" result only means the API accepted the delete while the pod drains; "mismatch" and a thrown delete leave the Job untouched. In all three states the agent could post after a negative probe, and that negative was persisted as proof-of-no-review. Probe now runs after claim + delete, gated on confirmStaleKilledJobQuiesced (polls the exact Job and every pod labelled with the runId until neither is active-or-terminating). Unconfirmed quiescence records no flag → terminal.

Important 1 — nullable head not fail-closed. probeStaleKillReviewEvidence now requires the wake's exact head; an unresolved head returns unproven (null), never false.

Important 2 — gate below the transient-recovery return. Hoisted above readTransientRecoveryContractFromRun, so a retained errorFamily can no longer return true before the evidence gate runs.

Specific things worth your scepticism:

1. Because BLO-13176 requires claim-before-delete, the probe result can't ride the CAS write — it's persisted in a follow-up db.update. Please check that finalizedRun/resultJson are correctly refreshed before the shouldScheduleAutomaticRunRetry call below, and whether that second write needs its own CAS.
2. confirmStaleKilledJobQuiesced treats a null from listManagedAgentPods() / readAgentJobRunStatusByName() (kube API unavailable) as not quiesced. Intended fail-closed, but please confirm it can't strand a legitimate retry permanently.
3. Quiescence bound is 3 attempts × 2s. If a pod's grace period routinely exceeds ~6s, this silently degrades to "never retries" rather than being unsafe — flag if that bound looks wrong.

Verification: heartbeat-retry-scheduling.test.ts 57/57 pass; @paperclipai/server typecheck clean. The Important-2 regression test was confirmed to fail against the pre-fix ordding (expected true to be false) before the fix was restored, so it is not a vacuous test. The Critical and Important-1 fixes have unit coverage for the exact-head guard only — there is no existing harness that exercises the staleKill finalize path end-to-end (it needs embedded postgres + kube mocks), so the quiescence ordering itself is not covered by an automated test. Calling that out rather than implying full coverage.

[kkroo]

Approved: stale-kill PR-review retry now fails closed unless exact-head review evidence is proven after job quiescence, with regression coverage for the prior race and transient-family bypass.