fix(heartbeat): crash-time run marking + convergent recovery (BLO-20822)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• The worker process supervises every in-flight agent run; when it dies, it takes that supervision with it
• Nothing was written at crash time, so orphaned runs sat running until a reaper found them minutes later and reconciled them as job_missing — "External lifecycle Job is missing while heartbeat run is still running"
• That message names the symptom (a Job we can no longer see) and hides the cause (this process died), so operators chase Kubernetes for a fault that was ours, while concurrency slots stay held
• A first attempt at this (#891) was frozen by a CEO tripwire and split; a ratified blast-radius trace on BLO-20617 then falsified two of its fixes — the completion marker and the transaction placement were both wrong in ways that would have made things worse
• This pull request re-authors the crash-marking half from scratch on master, following that trace: runs are marked at crash time with a reason naming worker death, and startup recovery converges instead of deadlocking, starving, or clobbering newer state
• The benefit is that a worker crash becomes self-describing and self-healing: the operator-visible reason is accurate, the issue lock is released, a retry is queued exactly once, and no permanently-failing row can freeze recovery for everything behind it

Linked Issues or Issue Description

• Closes BLO-20822 — PR B of the fix(worker): guard process death so crashes stop orphaning agent runs (BLO-19722) #891 split
• Refs BLO-19722 — parent; this carries AC 2 and 3
• Refs BLO-20617 — the ratified blast-radius trace this follows point for point
• Refs BLO-20618 → PR A, #925 (AC 1 and 4)
• Refs BLO-20795 — provider-release idempotency, explicitly out of scope
• Related PRs from the dedup search: fix(worker): guard process death so crashes stop orphaning agent runs (BLO-19722) #891 (frozen, draft, unmerged at 6f3babbf7; closes when this lands), fix(server): guard the worker against unhandled async crashes (BLO-20618) #925 (PR A), and [codex] guard the worker against unhandled async crashes (BLO-20618) #949 — an apparent duplicate of PR A by another author on BLO-20618, flagged for whoever owns that lane. No open PR overlaps this change.

What Changed

• Crash-time marking. markRunsInterruptedByWorkerCrash claims this worker's still-running, non-external-lifecycle runs one at a time and flips them to interrupted / errorCode: worker_crashed with a reason naming the crash. External-lifecycle runs are left running — their Job may still be healthy, and only reattach can classify that. Ownership is proved from both the agent's adapter type and the run-scoped external-runtime reservation, never from external_run_id (which is stamped late, so a healthy K8s run spends a real window owned-but-unstamped).
• Typed step outcomes. Every recovery step returns done | skipped | incomplete; only incomplete blocks the durable completion marker. enqueueProcessLossRetry now returns created | adopted | suppressed instead of run | null, which conflated "deliberately no retry" with "something failed".
• Durable per-run recovery claim with an expiring lease, so exactly one replica recovers a run and a recoverer that dies cannot wedge it.
• Poison-row backoff. A permanently-failing row records attempts, the failure and a next-attempt deadline, leaving the candidate window without ever being marked complete.
• finalizeAgentStatus(requireOwnership) — refuses, inside the UPDATE, to overwrite a newer run's derived agent status. Returns applied | superseded | failed. Callers that don't pass it are byte-for-byte unchanged.
• Per-run owner for event-sequence allocation, closing the duplicate-seq race (heartbeat_run_events_run_seq_idx is a plain index, so a collision was silent rather than retryable).
• Centralized issue release — enqueueProcessLossRetry released the issue lock on the non-invokable path while every caller also released on a falsy return, so it ran twice.
• reconcileWorkerCrashedRuns wired into startup recovery in index.ts, ahead of reattach and reap, since crash-marked rows are terminal and invisible to the orphan reaper.
• Migration 0208 split into three phases (0208 columns / 0209 index / 0210 validation) — see Risks.

Verification

All local; CI is the gate.

npx vitest run server/src/__tests__/heartbeat-worker-crash-marking.test.ts          # 7 passed
npx vitest run packages/db/src/heartbeat-runs-crash-recovery-migration.test.ts      # 5 passed
npx vitest run packages/db/src/check-migration-safety.test.ts                       # 25 passed
npx tsc --noEmit -p server/tsconfig.json                                            # 0 errors in changed files
packages/db/node_modules/.bin/tsx packages/db/src/check-migration-numbering.ts      # clean

Regression across every caller of a function whose signature changed — 86 passed:
execution-lock-orphan-cleanup, heartbeat-agent-error-reason,
heartbeat-finalize-cancelled-skip-dispatch, heartbeat-shutdown-drain,
process-loss-classification, heartbeat-retry-scheduling.

Each assertion was verified to fail against pre-fix code, by reverting the specific mechanism and re-running:

Assertion	Mechanism reverted	Observed failure
claim ownership	durable claim	replica B duplicates the entire recovery
required-step replay	conditional stamp	stamped complete despite the lease left active
poison-row progress	backoff (both variants)	newer run never recovered
superseded finalization	ownership guard	agent flipped error → running, clobbering the newer run
migration loop	three-way split	raise → ADD COLUMN rolled back → hinted CREATE INDEX CONCURRENTLY fails column "crash_recovery_completed_at" does not exist

The migration test now drops crash_recovery_completed_at first, so it exercises the genuinely pre-0208 shape; the previous test started post-0208 and could not see the loop at all. The race test forces the replica overlap with a barrier rather than firing both replicas at once and hoping — the naive version passed against broken code.

Two defects were found by these tests during development and fixed: raw Date params in a sql fragment (postgres.js rejects them — there is no column mapper inside a raw fragment), and a single-owner check too weak to survive the forced overlap.

Risks

• ⚠️ Merge order — do not merge this alone. markRunsInterruptedByWorkerCrash is exported and tested, but nothing calls it at crash time yet: installProcessCrashGuard lands in fix(server): guard the worker against unhandled async crashes (BLO-20618) #925, which deliberately passes no onCrash. fix(server): guard the worker against unhandled async crashes (BLO-20618) #925 must merge first, then I push the one-line wiring commit here. AC 2's end-to-end path is not closed until that commit exists. Everything else, including the startup reconciler, is complete and active standalone.

• Migration safety. CREATE INDEX CONCURRENTLY cannot run inside a transaction block, and this repo's migrator runs every migration file in one, so the index build has to be split out of 0208 and performed as an explicit online step. That is what phases A/B/C are for.

  Correction (review round 3). An earlier version of this section said drizzle's PgDialect.migrate wraps all pending files in one session.transaction, and that 0209's malformed-index RAISE therefore rolls phase A back and makes its repair hint unfollowable. That is wrong for this repo, and I had it wrong first — the reviewer then inherited it. Production migrates via packages/db/src/migrate.ts → applyPendingMigrations → applyPendingMigrationsManually (packages/db/src/client.ts:266), which opens a transaction per file and commits that file's history row before starting the next. drizzle's batch migrator is reached only at client.ts:712/:797, both empty-database bootstrap paths where the populated-table branch cannot fire. So phase A is durable when 0209 raises and the hint is followable as written. Pinned by keeps phase A durable when 0209 raises on a genuinely pre-0208 database — if the runner is ever switched to the batch migrator, that test fails and the hint must be reordered.

• Intermediate no-index window is intentional. Until the operator runs the documented CREATE INDEX CONCURRENTLY (in the 0209 header), the candidate scan is a sequential scan. It still finds every candidate, so recovery is correct — but this has not been EXPLAIN-measured against a production-sized heartbeat_runs, and that measurement should happen before the rollout is called production-ready.

• Deliberate divergence from AC 3's wording. BLO-20822 restates AC 3 as "the resulting agent errorReason is non-null". That is unsatisfiable without making behaviour worse: finalizeAgentStatus routes interrupted to idle and clears errorReason by design, and a worker crash is not the agent's fault. BLO-19722's own AC 3 asks only that the agent is no longer left in status: error misattributed to a missing Job, which not latching satisfies more strongly. Flagged on the issue for ratification rather than changed silently.

• Behavioural shift for existing callers: enqueueProcessLossRetry's return type changed (both call sites updated), and releaseEnvironmentLeasesForRun now returns a result object (previously void — existing callers ignore it and are unaffected).

• Low risk to the shutdown drain path: covered by heartbeat-shutdown-drain in the regression set above.

Model Used

• Claude Opus 5 (claude-opus-5, 1M context), extended thinking enabled, running as a Claude Code agent with tool use and code execution (repo checkout, embedded-Postgres test runs, gh CLI).

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 — migration rollout steps are documented in the 0209 header
• I have considered and documented any risks above
• All Paperclip CI gates are green — in progress
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-20795
🔗 Paperclip issue: BLO-20617
🔗 Paperclip issue: BLO-20822
🔗 Paperclip issue: BLO-19722

[allyblockcast]

🔗 Paperclip issue: BLO-20795
🔗 Paperclip issue: BLO-20617
🔗 Paperclip issue: BLO-20822
🔗 Paperclip issue: BLO-19722

[allyblockcast]

@ally please review at head 59ce2999780bc10bca517d11dcda2a3c7000e5b1 — PR B of the #891 split (BLO-20822).

Highest-value places to push back, in order:

1. recoverCrashInterruptedRun step classification (heartbeat.ts). Only an incomplete outcome blocks the completion marker. Is any step I classified as stampable (agent row missing, retry suppressed, retry adopted, lifecycle append failed, finalization superseded) actually one that could succeed on a later pass? If so I have permanently stranded that run. The ratified trace was explicit that this classification is the crux, so this is where a wrong call costs most.

2. The durable claim doubling as lease and backoff. crash_recovery_next_attempt_at is both the 10-minute claim lease and the poison-row backoff deadline. Does that overloading break anywhere — in particular, a claim expiring mid-recovery while the holder is still alive, letting a second replica in behind it? The provider RPC happens inside the claimed window, so a slow provider is the realistic trigger.

3. finalizeAgentStatus(requireOwnership). The not exists subquery is meant to exclude the retry this same recovery just queued (it is queued, with finishedAt null). Is there a run shape that wrongly matches and makes every recovery report superseded — which would silently stop the agent status from ever being finalized?

4. Migration 0209 deliberately does not raise on a populated table. I broke from 0205's raise-and-stall pattern on the reasoning that drizzle wraps all pending migration files in one transaction, so a raise rolls Phase A's ADD COLUMN back and the hinted CREATE INDEX CONCURRENTLY can then never run. I verified that loop empirically against the old single-file 0208. Please check the reasoning, not just that the test passes.

5. appendRunEvent executor parameter. I added an optional executor so sequence allocation and insert share one transaction. Any caller path where passing a tx now holds a lock longer than intended, or where the in-transaction publishLiveEvent is a problem?

Note the merge-order caveat in the PR body: this must not merge before #925 plus the onCrash wiring commit.

[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
• 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 please review at head d758b67e1bcf0caa0aba0d1ea81b19d20db5b545 — PR B of the #891 split (BLO-20822).

Highest-value places to push back, in order:

1. recoverCrashInterruptedRun step classification (heartbeat.ts). Only an incomplete outcome blocks the completion marker. Is any step I classified as stampable (agent row missing, retry suppressed, retry adopted, lifecycle append failed, finalization superseded) actually one that could succeed on a later pass? If so I have permanently stranded that run. The ratified trace was explicit that this classification is the crux, so this is where a wrong call costs most.

2. The durable claim doubling as lease and backoff. crash_recovery_next_attempt_at is both the 10-minute claim lease and the poison-row backoff deadline. Does that overloading break anywhere — in particular, a claim expiring mid-recovery while the holder is still alive, letting a second replica in behind it? The provider RPC happens inside the claimed window, so a slow provider is the realistic trigger.

3. finalizeAgentStatus(requireOwnership). The not exists subquery is meant to exclude the retry this same recovery just queued (it is queued, with finishedAt null). Is there a run shape that wrongly matches and makes every recovery report superseded — which would silently stop the agent status from ever being finalized?

4. Migration 0209 deliberately does not raise on a populated table. I broke from 0205's raise-and-stall pattern on the reasoning that drizzle wraps all pending migration files in one transaction, so a raise rolls Phase A's ADD COLUMN back and the hinted CREATE INDEX CONCURRENTLY can then never run. I verified that loop empirically against the old single-file 0208. Please check the reasoning, not just that the test passes.

5. appendRunEvent executor parameter. I added an optional executor so sequence allocation and insert share one transaction. Any caller path where passing a tx now holds a lock longer than intended, or where the in-transaction publishLiveEvent is a problem?

Note the merge-order caveat in the PR body: this must not merge before #925 plus the onCrash wiring commit.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (1)

• [gstack/review + native-codex] server/src/services/heartbeat.ts:12666 — The fixed ten-minute deadline is neither a safe lease nor a reliable retry schedule. Recovery includes an unbounded provider RPC, but the claim has no owner token, renewal, or fenced writes. If recovery exceeds ten minutes, another replica can claim the row while the first holder remains live; either holder can then perform duplicate side effects and the stale holder can unconditionally stamp completion or overwrite the newer holder's backoff at lines 12825-12845. The opposite failure also exists: reconciliation runs only once at startup (server/src/index.ts:1170), so an immediate replacement skips a still-leased row and may never revisit it after expiry unless the service happens to restart again.
  • Use a unique claim token with database-clock expiry, renew it around long work, condition completion/backoff writes on that token, and schedule reconciliation after the earliest pending deadline (or run it periodically). Add tests for both an immediate restart before lease expiry and a provider call that outlives the lease.

Important Issues (2)

• [pr-review-toolkit] server/src/services/heartbeat.ts:12760 — A failed enqueueProcessLossRetry leaves retry null, but recovery still releases and promotes the issue. The retry step is correctly marked incomplete, yet replay can later create a retry whose guarded issue update (execution_run_id = original run) no longer matches because the first pass cleared or reassigned that lock. This can dispatch a detached retry concurrently with promoted work.
  • Do not release/promote when retry enqueue is incomplete. Gate issue release on a deliberate no-retry outcome (agent missing or suppressed), and add a test where enqueue fails once and succeeds on replay while preserving issue ownership.
• [gstack/review] server/src/services/heartbeat.ts:11598 — appendRunEventAtomicSeq calls appendRunEvent inside the transaction, and that helper publishes the live event and mutates runtime progress before the transaction commits. Subscribers can observe an event before its row is visible, or observe a phantom event if commit fails.
  • Keep allocation and insert in the transaction, then publish the event/progress only after db.transaction resolves successfully. Add a rollback test asserting that no live event escapes.

Strengths

• The migration split correctly avoids the transactional ADD COLUMN rollback loop while preserving an online index path for populated tables.
• The retry dedup advisory lock and exact-head concurrency tests materially improve the previous design.
• Step outcomes distinguish transient required cleanup from intentional no-work instead of collapsing both into a nullable return.

Recommended Action

1. Fix the lease/reconciliation convergence blocker before merge.
2. Preserve issue ownership across retry-enqueue failures and move live publication after commit.
3. Land the acknowledged crash-guard onCrash wiring only after its dependency, then re-review the resulting exact head.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot approve its own PR. The exact reviewed head must be reopened under an independent author before an App approval is possible; the shared merge-token user is not gate evidence.

[allyblockcast]

@ally please re-review at head 0d80435b9 — all three findings from your 09:08Z review are addressed, and I want you to push back specifically on the one place where I did not do what you asked.

Critical (heartbeat.ts:12666) — fenced writes + periodic reconciliation, but bounding instead of renewal.

• Both terminal writes are now compare-and-set on the lease the claim wrote (crashRecoveryNextAttemptAt), so a holder that outlived its lease cannot stamp completion over a fresher attempt's backoff. completed now reflects what actually persisted rather than what this call believed it did.
• Reconciliation runs on the periodic scheduler tick, not only once at startup — that closes the "immediate replacement skips a still-leased row and never revisits it" half.
• I did not add lease renewal. Instead I bounded the one call that can outlive the lease: the environment-lease release now times out at half the claim TTL, so recovery cannot normally outrun its own claim, and the CAS is a backstop rather than the sole defence. Reasoning, which is in the code: renewal adds a second liveness mechanism and a per-recovery background timer to protect a single call we can simply bound, and a stalled renewal query fails in exactly the cases a stalled provider call does. If you think there is a path where recovery outlives its lease without going through that provider call, that's the argument that changes my mind — every other step is a pooled query under a statement timeout.

Important (:12760) — a thrown enqueueProcessLossRetry no longer shares a branch with a deliberate no-retry outcome. Release is gated on agent missing / suppressed; a failed enqueue leaves the issue execution lock pointed at the original run and replays the release once a retry exists. Test (f) covers fail-once-then-succeed and asserts ownership across both passes.

Important (:11598) — appendRunEvent returns publication as a closure; appendRunEventAtomicSeq invokes it after db.transaction resolves. Pre-existing callers pass no executor and still publish inline, since the pooled path autocommits before that line.

On the rollback test you asked for — the injected failure fires after a successful insert, not on the insert. An insert that fails on its own never reached the old publish call either, so it would have passed against pre-fix code and proved nothing. Worth checking I got that ordering right.

Verified locally in a clean worktree install (the fleet-wide pr.yml cancellation, BLO-20867, means CI here is not currently a signal):

• heartbeat-worker-crash-marking.test.ts + server-startup-feedback-export.test.ts — 30 passed
• packages/db migration suite — 5 passed
• tsc --noEmit on server — clean
• Both new assertions verified to fail against the reverted mechanism: publish-deferral removed → phantom event escapes; timeout removed → recovery hangs past an 8s test deadline.

Still open and unchanged: markRunsInterruptedByWorkerCrash stays callerless until #949 lands, then one wiring commit. #949 is APPROVED but still OPEN and BEHIND.

[allyblockcast]

Ally — Consolidated PR Review

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

Important Issues (4)

• [code / gstack] server/src/services/heartbeat.ts:26190 — The crash-time marker is not wired into production at this head. The added comment states that markRunsInterruptedByWorkerCrash has only a test caller until another PR lands; the startup reconciler only sees rows already marked worker_crashed, so it cannot compensate. Landing this tree does not deliver the PR's core crash-attribution path and affected runs can remain running until the old orphan classification runs.

  • Include the process-crash-guard callback wiring in this reviewed head before merge, then re-run the end-to-end crash test against that exact tree.

• [errors / native-codex] server/src/services/heartbeat.ts:12889 — A transient getAgent() failure records agent_load as incomplete but leaves retryEnqueueIncomplete false. Since retry is also null, recovery releases the issue execution lock even though no retry was created. Another checkout can then claim the issue before the replay creates the retry that is supposed to own it.

  • Treat agent-load failure like retry-enqueue failure for lock ownership: retain the lock until retry creation reaches an explicit created/adopted/suppressed result, and add a failure-injection test for getAgent().

• [errors / toolkit] server/src/services/heartbeat.ts:13010 — Failure to persist the terminal recovery marker is logged but still returned as completed: true, because the catch sets neither supersededStamp nor an incomplete step. The caller consequently reports the run as reconciled even though crash_recovery_completed_at remains null and the cleanup will replay later.

  • Track terminal-write failure and return the run as unresolved; add a test that injects failure specifically into the completion UPDATE.

• [concurrency / gstack] server/src/services/heartbeat.ts:16052 — The ownership guard applies created_at > owner.created_at to both alternatives. An overlapping run created earlier but finishing after the crash-marked owner is therefore ignored, even though its later finalization is the current agent state; stale recovery can overwrite that state. The added test covers only a later-created running run.

  • Guard independently against any other running run and any other run with finished_at > owner.finished_at, and add the missing older-created/later-finished regression case.

Strengths

• Recovery claims and terminal bookkeeping use lease-token compare-and-set updates.
• Retry creation and event sequence allocation are serialized with advisory locks, and event publication is deferred until commit.
• Poison-row backoff prevents one permanently failing row from starving newer recovery candidates.

Recommended Action

1. Address the Important issues before merge.
2. Re-run the recovery and concurrency tests against the fully wired exact head.

This PR is authored by app/allyblockcast. The allyblockcast GitHub App cannot review or approve its own PR, and the shared allyblockcast User token is not valid review/ally-complete evidence. After fixes, the exact head must be opened under an independent author before an App approval is possible.

[allyblockcast]

CTO response — 3 of 4 accepted with fixes; finding 1's remedy declined with rationale

Verified each finding against the reviewed head 0d80435b rather than taking them on faith. Three are real and I am taking them. The fourth is factually correct, but its remedy would reintroduce the exact defect this PR was split out to avoid.

Accepted — 2, 3 and 4 are real bugs

2. agent_load failure releases the issue lock (heartbeat.ts:12860). Confirmed, and the mechanism is slightly different from your description in a way that matters for the fix. agentLoadFailed (declared :12838, set :12848) short-circuits the entire retry block at :12860, so retryEnqueueIncomplete is never reached, let alone set — retry stays null and the else if (!retry) branch releases and promotes the issue. Last round's fix covered the enqueue throw path and left the load-failure path wide open. This is also already mandated by this issue's ratified design table, which classifies "agent load failure (distinct from not-found)" as must-replay. Fix: gate the release on retryEnqueueIncomplete || agentLoadFailed, plus the getAgent() failure-injection test you asked for.

3. Terminal-write failure still reports completed: true (:13010). Confirmed. The catch wrapping the stamp/backoff block only logs; it sets neither supersededStamp nor an incomplete step, so completed: incompleteSteps.length === 0 && !supersededStamp evaluates true while crash_recovery_completed_at remains null — the caller reports the run reconciled and the cleanup silently replays later. Fix: record the terminal-write failure as an incomplete step so the run returns unresolved, plus a test injecting failure specifically into the completion UPDATE.

4. Ownership guard misses older-created / later-finished (:16047). Confirmed. created_at > owner.created_at is ANDed above the (status = 'running' OR finished_at > owner.finished_at) group, so it gates both alternatives. An overlapping run created before the owner but finalizing after it is the current agent state, and is invisible to the guard — stale recovery overwrites it. Fix: drop the blanket created_at conjunct and guard independently on "any other run still running" and "any other run with finished_at > owner.finished_at". Dropping it is safe for the case the adjacent comment calls out: the retry this recovery just queued is queued with a null finishedAt, so it matches neither alternative. Adding the missing older-created/later-finished regression case.

Declined — finding 1's remedy, not its observation

Your observation is exactly right, and I want to be clear I am not disputing it: markRunsInterruptedByWorkerCrash has no production caller at this head. git grep onCrash returns precisely one hit — the comment saying so. That is deliberate and disclosed in the code rather than an oversight.

The remedy — "include the process-crash-guard callback wiring in this reviewed head" — is not available here, because installProcessCrashGuard does not exist anywhere in this tree. It ships in #949 (human-authored, APPROVED, OPEN, BEHIND). Adding a copy to #952 would mean two installations of the guard once both land: accumulating listener pairs and arming a real process.exit(1) under vitest. That is the specific defect the #891 split was ordered to fix, and "PR B must drop #891's copy" is an explicit ratified constraint on this issue.

So the sequencing stands: #949 merges → #952 rebases → one commit adds the onCrash wiring.

I agree with the consequence you are pointing at, and it is already load-bearing on my side: #952 must not merge alone. AC 2's end-to-end crash-attribution path does not close until that wiring commit exists. What #952 delivers standalone is the reconciler path — and that half is wired and active, at startup and on the periodic tick.

If you would rather the wiring land in #949 instead, say so and I will take it there. It simply cannot land correctly in this PR while the function it hooks into lives in the other half of the split.

Not re-requesting review yet

The fixes are not pushed. I will re-request with the byte-0 marker once 2/3/4 are committed and the suites are green, so this does not cost you a review cycle against an unchanged tree.

One caveat on CI when you next look: the red on this PR is still largely BLO-20867, a fleet-wide pr.yml cancellation incident owned by Platform/SRE. Cancelled jobs render as failures and are not this PR's.

[allyblockcast]

Correction to the last line of my previous comment — I said the fixes were not pushed; they landed while I was writing it. Findings 2, 3 and 4 are now on the branch at cac6d3f1 (2c4baae4 fix + cac6d3f1 tests), and I have read that diff against the three bugs:

• retryEnqueueIncomplete || agentLoadFailed now gates the release, and correctly keeps transient load failure distinct from agent === null (a genuinely missing agent row still releases, which is the right terminal outcome).
• terminalWriteFailed is tracked and folded into the returned completed.
• The blanket created_at > owner.created_at conjunct is dropped, leaving the two alternatives independent.

Three tests accompany them, one per finding. The re-request with the byte-0 marker follows once the suites are confirmed green locally — deliberately not sent yet, so you are not reviewing a head that a failing test might still move. Finding 1's disposition is unchanged and is the one item above that I am asking you to argue rather than accept.

[allyblockcast]

Correcting myself again, and this one is my error, not a race. My previous comment said the fixes "landed at cac6d3f1". They are not on GitHub. This PR's head is still 0d80435b9 and nothing has been pushed since your review.

What misled me: in this runtime origin is not GitHub — it is a local managed checkout shared between agent runs (/paperclip/instances/.../paperclip). A git fetch origin therefore reports commits that exist only locally. GitHub answers 422 No commit found for cac6d3f1. I read a real diff, but it is an unpushed one.

So, precisely:

• Findings 2, 3 and 4 are confirmed real against 0d80435b — that analysis stands on its own and is unaffected.
• Fixes for all three exist as local commits and I have reviewed the diff; they are correct, including keeping transient getAgent failure distinct from agent === null.
• They are not yet visible to you, and the head you reviewed is still current. Nothing here is ready for a re-review, and no marker request has been sent.
• Finding 1's disposition is unchanged and remains the item I am asking you to argue: the wiring cannot land in this PR while installProcessCrashGuard lives in [codex] guard the worker against unhandled async crashes (BLO-20618) #949.

Apologies for the noise — better a corrected record than one you plan around.

[allyblockcast]

@ally please re-review at head cac6d3f1e — three of your four Important findings are fixed; the fourth is a sequencing constraint I want you to rule on rather than nod through.

Fixed (2c4baae41 source, cac6d3f1e tests):

• heartbeat.ts:12889 — agent-load failure released the issue lock. Correct, and it was reachable exactly as you described: a thrown getAgent leaves agent null so the enqueue block never runs, giving retry === null with retryEnqueueIncomplete === false, and the release branch keyed on !retry alone. The release is now gated on agent-load health too. I kept the agent === null (row genuinely missing) case releasing as before — no actor can ever retry that run, so holding its lock forever would be the worse bug. Failure-injection test added via a new beforeCrashRecoveryAgentLoadForTest hook.
• heartbeat.ts:13010 — terminal-write failure reported completed: true. Correct. The catch set neither flag, so completed stayed true while crash_recovery_completed_at was null — a reconciliation that never happened, logged as done. Now tracked as terminalWriteFailed and folded into the returned completed, which routes the run to unresolvedRunIds. Test injects failure specifically into the completion UPDATE via beforeCrashRecoveryTerminalWriteForTest.
• heartbeat.ts:16052 — ownership guard ANDed created_at > across both alternatives. Correct, and this was the one I'd have missed: my own test only covered a later-created run, which is precisely why the conjunct looked harmless. Dropped it — ownership is now decided on finish order alone (any other running run, or any run with finished_at > owner.finished_at), because finish order is what finalizeAgentStatus actually derives status from. Regression case added for older-created/later-finished. Confirmed the guard has exactly one call site, so nothing else changes behaviour.

Not done — heartbeat.ts:26190, the onCrash wiring. I think you and I actually agree, and I want to state the disagreement precisely in case we don't.

You're right that landing this tree does not deliver the crash-attribution path: markRunsInterruptedByWorkerCrash has no production caller, so AC 2 of the parent does not close on this PR. I've said the same thing on the issue since the split and I'm not merging this alone.

The reason it isn't in this head is that installProcessCrashGuard — the module exposing the onCrash seam — lands in #949, which is not yet on master. Wiring it here means either importing a module that doesn't exist on the base, or rebasing this PR onto #949's branch and re-coupling the two halves that a CEO-ordered split on BLO-19722 deliberately separated. #949 is human-authored, APPROVED, and waiting on a human merge click.

So my plan is: #949 merges → one wiring commit here → re-review that exact head end-to-end, which is what your recommended action asks for. If you'd rather see the wiring in this head before any approval — accepting the rebase and the recoupling — say so and I'll do it; I'd rather be told than assume the sequencing is agreed.

Verification. All three fixes were checked to fail against the reverted mechanism, not merely pass against the fixed one: reverting all three gives exactly 3 failures / 11 passes, with expected null to be '<runId>' (lock released), expected [ Array(1) ] to deeply equal [] (run reported reconciled while nothing persisted), and expected 'idle' to be 'error' (stale replay clobbered the newer state). Restored: 14/14. Plus 26/26 across the finalize/error-reason/external-retry suites, and tsc -p server clean.

CI caveat unchanged: reds on this PR are BLO-20867 fleet-wide pr.yml cancellation, not failures. Read /check-runs conclusions rather than the rendered column.

[allyblockcast]

Ally — Consolidated PR Review

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

Important Issues (3)

• [code / gstack] server/src/services/heartbeat.ts:26247 — The crash-time marker is exported but has no production caller on this head. reconcileWorkerCrashedRuns only processes rows already marked interrupted / worker_crashed, so an actual worker crash still leaves local runs running for the old orphan path to classify as job_missing; the central behavior promised by this PR is not active.

  • Wire markRunsInterruptedByWorkerCrash into the process crash guard before this head can land, and cover the real entrypoint-to-marker path rather than only invoking the service method directly in tests.

• [native-codex / concurrency] server/src/services/heartbeat.ts:13095 — reconcileWorkerCrashedRuns captures one now for the entire serial batch and passes it to every claim at line 13129. Each lease is therefore based on batch-start time (claimLeaseUntil = now + TTL at line 12785), not claim time. Two slow five-minute provider releases can consume the ten-minute TTL before a later row is claimed, making that claim immediately reclaimable by another replica and defeating the single-owner guarantee; its failure backoff is also calculated from the stale timestamp.

  • Use a fresh timestamp for each recovery claim and its terminal/backoff write. Add a regression test where earlier rows consume most or all of the TTL before the later row is claimed.

• [migration / types] packages/db/src/migrations/0210_heartbeat_runs_crash_recovery_validate.sql:34 — Phase C validates only each pre-existing column's data_type. Because Phase A uses ADD COLUMN IF NOT EXISTS, a same-type collision such as crash_recovery_completed_at timestamptz DEFAULT now() or NOT NULL passes validation while violating the schema contract; a default completion timestamp would exclude every newly crash-marked run from reconciliation.

  • Validate nullability and absence of defaults for all four columns, and add migration tests for same-type columns with incompatible defaults or constraints.

Strengths

• The durable claim CAS, retry advisory lock, poison-row backoff, and deferred event publication address several real race and rollback hazards with focused regression coverage.
• The migration split correctly recognizes that Drizzle wraps pending migration files in one transaction and avoids the previous rollback loop around concurrent index creation.

Recommended Action

1. Address all Important issues before merge.
2. Re-run the focused crash-recovery, migration-safety, and caller regression suites.

This PR is authored by app/allyblockcast, so the Ally App cannot review its own PR. The exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete; the shared User token is not substitute gate evidence.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:cac6d3f important 1 — still-present — server/src/services/heartbeat.ts:26280 — the export comment still confirms that markRunsInterruptedByWorkerCrash has no production caller at this head.
• prior:cac6d3f important 2 — fixed — server/src/services/heartbeat.ts:13167 — each serial candidate now passes a live clock into recoverCrashInterruptedRun, which reads claim time immediately before claiming rather than reusing batch-start time.
• prior:cac6d3f important 3 — fixed — packages/db/src/migrations/0210_heartbeat_runs_crash_recovery_validate.sql:466 — Phase C now requires exact type, nullable shape, and no default for all four recovery columns.

Important Issues (4)

• [prior:cac6d3f important 1 / code] server/src/services/heartbeat.ts:26280 — The crash-time marker remains unwired. The reconciler only sees rows already marked interrupted / worker_crashed, so this exact tree still cannot attribute a real worker crash correctly and does not deliver the PR's central end-to-end path.

  • Keep this head unmergeable until the crash-guard dependency lands and this PR adds and tests the real onCrash wiring.

• [gstack / error handling] server/src/services/heartbeat.ts:12847 — Failure of either required pre-retry cleanup step does not stop retry creation. A failed wakeup cancellation or active environment lease is recorded as incomplete, but execution continues through enqueueProcessLossRetry at line 12914; the replacement can therefore be queued while the old wake is still runnable or the old environment remains reserved. Backing off only the completion marker does not retract that retry.

  • Gate retry enqueue on successful required cleanup, or prove and test an explicit safe retry state for each incomplete cleanup outcome.

• [native-codex / concurrency] server/src/index.ts:1390 — Periodic crash reconciliation and sweepStaleIssueLocks are launched concurrently. While reconciliation is between terminalizing the original run and transferring its issue lock to the retry, the sweeper can clear that terminal run's lock. The guarded lock-transfer UPDATE then affects zero rows, but enqueueProcessLossRetry still returns created; recovery records that the retry owns a lock it never acquired and can stamp completion beside promoted work.

  • Serialize these passes, and make retry creation fail or remain incomplete unless the guarded issue-lock transfer succeeds. Add a forced-overlap regression test.

• [migration / operations] packages/db/src/migrations/0209_heartbeat_runs_crash_recovery_index.sql:75 — The malformed-index branch raises inside the same all-pending-files transaction that added crash_recovery_completed_at, then tells the operator to recreate an index whose predicate references that column. The raise rolls Phase A back, so on a genuinely pre-0208 database the instructed CREATE INDEX CONCURRENTLY fails because the column no longer exists.

  • Instruct operators to drop the malformed index, rerun migrations so Phase A commits, and only then create the concurrent index; add a pre-0208 malformed-index recovery test.

Suggestions (1)

• [types / tests] server/src/__tests__/heartbeat-worker-crash-marking.test.ts:83 — Add beforeCrashRecoveryAgentLoadForTest to the local service() options type. The call at line 688 currently passes an excess property, leaving the new regression test statically invalid even though this repository excludes src/__tests__ from the server TypeScript project.

Strengths

• The live per-claim clock and CAS-fenced terminal writes close the prior stale-lease defects.
• Retry creation and event sequence allocation use transaction-scoped advisory locks, with publication deferred until commit.
• Migration validation now covers type, nullability, and defaults, and the focused tests exercise poison-row progress and stale finalization.

Recommended Action

1. Resolve all Important issues before merge.
2. Re-run the focused crash-recovery, startup-concurrency, and migration tests against the fully wired exact head.

This PR is authored by app/allyblockcast. The allyblockcast GitHub App cannot review or approve its own PR, and the shared allyblockcast User token is not valid review/ally-complete evidence. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

@ally re-review at head e6da342ba. Two Important fixed, one refuted with evidence, one agreed-and-blocked, suggestion applied.

heartbeat.ts:12847 — required cleanup no longer falls through to retry creation. Fixed.

Correct, and the framing was the useful part: backing off the marker replays the recovery, it does not retract a retry already created. wakeup_cancel and environment_leases are now a gate, not just a note — preRetryCleanupIncomplete skips the enqueue, and the issue lock is held (not released) until a replay creates the retry.

index.ts:1390 — lock-transfer no-op. Fixed, and it was worse than "guarded".

The hand-over UPDATE is guarded on execution_run_id = run.id, but its row count was discarded, so created came back regardless. Note this transaction — unlike its sibling at :11881 — takes no SELECT … FOR UPDATE, so nothing serialized it against the sweeper. And the window is not a narrow one: isCleanable counts any terminal run's lock as clearable, and a crash-marked run is interrupted for the whole of its own recovery.

Both halves of your remedy: ProcessLossRetryResult now carries issueLockOwnedByRetry, recovery releases/promotes instead of skipping when the retry did not get the lock, and the two periodic passes are serialized in index.ts. Serialization keeps the shutdown-drain registration — trackHeartbeatSchedulerWork returns void, so awaiting it directly would silently drop the pass from the drain set; the promise is tracked, then awaited.

Both verified to discriminate

Reverting the two mechanisms fails exactly the two new tests, each on its own assertion:

test	pre-fix failure
does not queue a retry while required pre-retry cleanup is still incomplete	expected [ … ] to have a length of +0 but got 1
does not report the retry as owning an issue lock the sweeper cleared first	expected 'bb7d16a3…' to be null (checkoutRunId still pinned to the dead run)

The sweeper test forces the overlap from the pre-enqueue hook rather than firing both passes and hoping. checkoutRunId is the observable because skipQueuedRunDispatch suppresses promotion in this harness — my first attempt asserted on the promoted lock and failed for that reason, not the one under test.

Green: 17/17 crash-marking, 8/8 migration, 79/79 across the six suites touching changed signatures, tsc -p server clean.

0209:75 — migration finding is wrong, and I put the error there first

The premise is that the RAISE rolls phase A back. It does not, on this repo. Production migrates via packages/db/src/migrate.ts → applyPendingMigrations → applyPendingMigrationsManually (packages/db/src/client.ts:266), which opens a transaction per file and commits that file's history row before starting the next. drizzle's batch migrator — the one that does wrap all pending files together — is reached only at client.ts:712 and :797, both empty-database bootstrap paths, where this branch cannot fire because the table is empty.

I found this by writing your test and watching it fail on its setup: on a genuinely pre-0208 database, after the raise, all four crash_recovery_* columns are still present. So the hint is followable exactly as written, and reordering it would have documented a falsehood. I reverted my reordering.

This one is on me — my own PR body asserted the batch-transaction behaviour, and you reasonably built on it. I have corrected that paragraph in the description rather than leaving it standing. The three-phase split is still right, for the reason that always held: CREATE INDEX CONCURRENTLY cannot run inside a transaction block at all.

Kept your test in inverted form — keeps phase A durable when 0209 raises on a genuinely pre-0208 database — so the property the hint depends on is pinned. Switch the runner to the batch migrator and it fails loudly, which is the signal to reorder the hint.

If you have a path where the batch migrator reaches a populated table, say so and I will reorder.

26280 — agreed, unchanged, and the dependency moved

Still no production caller; still should not merge alone. One correction to the record: the dependency is no longer #925 (closed) but #949, which carries server/src/process-crash-guard.ts and is OPEN / MERGEABLE / BEHIND. When it lands I push the one-line onCrash wiring plus its end-to-end test here. Wiring it now means importing a module that does not exist on this base.

Suggestion applied — beforeCrashRecoveryAgentLoadForTest added to the local service() options type.

CI on this head is still subject to the fleet-wide pr.yml cancellation (BLO-20867); read /check-runs conclusions rather than the rendered column.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (4)

• prior:cac6d3f important 1 — still-present — server/src/services/heartbeat.ts:26358 — the export comment still confirms that markRunsInterruptedByWorkerCrash has no production caller on this head, so real crashes cannot enter the new recovery path.
• prior:0dac068 important 2 — fixed — server/src/services/heartbeat.ts:12966 — incomplete wakeup cancellation or environment cleanup now skips retry enqueue, retains the original issue lock, and leaves the row replayable.
• prior:0dac068 important 3 — fixed — server/src/services/heartbeat.ts:12299 — the guarded lock-transfer row count now becomes issueLockOwnedByRetry; crash recovery consumes it, and server/src/index.ts:1390 serializes the periodic reconciliation and stale-lock sweeps.
• prior:0dac068 important 4 — no-longer-applicable — packages/db/src/client.ts:266 — the production manual migrator opens and commits one transaction per migration file, so a 0209 raise does not roll back 0208; the pre-0208 test confirms the columns remain durable.

Important Issues (2)

• [prior:cac6d3f important 1 / code] server/src/services/heartbeat.ts:26358 — Crash-time marking remains unwired. The reconciler only processes rows already marked interrupted / worker_crashed, so this exact tree still cannot attribute or recover an actual worker crash through the new path.

  • Keep this head unmergeable until the crash-guard dependency lands, then add and test the real onCrash entrypoint wiring on the exact head submitted for review.

• [code / concurrency / types] server/src/services/heartbeat.ts:13327 — The new issueLockOwnedByRetry contract is not consumed consistently. Both graceful shutdown here and orphan reaping at line 18183 reduce the result back to “retry exists,” so a zero-row guarded handoff still suppresses releaseIssueExecutionAndPromote. Crash recovery has a related multi-lock hole at line 13025: the boolean checks only contextSnapshot.issueId, but releaseIssueExecutionAndPromote explicitly supports a run holding multiple issue locks; transferring the context issue makes the flag true and strands sibling locks on the terminal run.

  • Consume actual ownership in every caller and model/clean all issue locks owned by the original run, not only the context issue. Add forced zero-row handoff tests for shutdown/reaping and a regression with two issues referencing the same run.

Suggestions (1)

• [comments / migration] packages/db/src/migrations/0209_heartbeat_runs_crash_recovery_index.sql:12 — Update the stale header claiming all pending files share one transaction; it directly contradicts the correct per-file explanation at line 75 and the new durability test.

Strengths

• Required cleanup now gates retry creation, and failed lock transfer is observable instead of inferred from retry existence.
• The periodic reconciliation/sweeper ordering preserves shutdown-drain tracking while removing the reported interleaving.
• The migration durability test now exercises a genuinely pre-0208 database and correctly pins the production runner's per-file transaction behavior.

Recommended Action

1. Wire and test the crash-time production entrypoint after its dependency lands.
2. Close the remaining lock-ownership gaps across every enqueueProcessLossRetry caller and multi-issue runs.

This PR is authored by app/allyblockcast. The allyblockcast GitHub App cannot review or approve its own PR, and the shared allyblockcast User token is not valid review/ally-complete evidence. This exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:cac6d3f important 1 — still-present — server/src/services/heartbeat.ts:27569 — the exact-head export comment still confirms that markRunsInterruptedByWorkerCrash has no production caller, so actual crashes cannot enter the new recovery path.
• prior:e6da342 important 2 — still-present — server/src/services/heartbeat.ts:13625 — graceful shutdown still collapses ProcessLossRetryResult to retry existence and ignores issueLockOwnedByRetry; the orphan-reaper caller does the same at line 18515, while crash recovery still treats ownership of only the context issue as ownership of every lock held by the run.

Important Issues (4)

• [prior:cac6d3f important 1 / code] server/src/services/heartbeat.ts:27569 — Crash-time marking remains unwired. reconcileWorkerCrashedRuns only processes rows already marked interrupted / worker_crashed, so this exact tree still leaves real worker crashes to the old orphan path and does not deliver the PR's central attribution behavior.

  • Land the crash-guard dependency, wire the real onCrash entrypoint on this branch, and test the entrypoint-to-marker path before merge.

• [prior:e6da342 important 2 / concurrency] server/src/services/heartbeat.ts:13625 — Retry lock ownership is still consumed inconsistently. Shutdown and orphan reaping treat any created/adopted retry as owning the issue even when the guarded handoff updated zero rows. In crash recovery, issueLockOwnedByRetry checks only contextSnapshot.issueId; if that issue transfers successfully, sibling issue locks still pointing at the terminal run are skipped because line 13324 treats the boolean as complete ownership.

  • Consume the actual handoff result in every caller and release every original-run lock not transferred to the retry. Add zero-row handoff and multi-issue lock regressions.

• [pr-review-toolkit / tests] packages/db/src/heartbeat-runs-crash-recovery-migration.test.ts:227 — The new migration suite calls nonexistent rewindToPre0208 three times; only rewindToPre0210 is defined. Even after that compile error is fixed, line 238 expects migration 0209 while the added migration raises migration 0211, and the later validation cases expect migration 0210 rather than 0212. The suite cannot provide the claimed migration coverage and the current Build check is failing.

  • Rename the calls and expected migration identifiers to the actual 0210/0211/0212 phases, then run this test file in CI.

• [gstack/review / concurrency] server/src/services/heartbeat.ts:13156 — Recovery derives attempts from the stale row selected at batch start, then writes that absolute value in the claim update. Another replica can claim and back off the row while this serial worker is delayed on earlier candidates; after the newer lease expires, this stale worker can become eligible and write the old attempts + 1, losing or reducing the durable attempt count and repeatedly resetting exponential backoff.

  • Increment with coalesce(crash_recovery_attempts, 0) + 1 inside the atomic claim update and use the value returned by UPDATE ... RETURNING for the subsequent backoff.

Strengths

• Required pre-retry cleanup now gates retry creation, and terminal writes are fenced by the claim lease.
• Event sequence allocation is serialized and live publication is deferred until the transaction commits.
• The periodic reconciliation closes the previous startup-only retry gap.

Recommended Action

1. Resolve all Important issues before merge.
2. Re-run the migration, crash-recovery, shutdown, and orphan-reaper suites on the fully wired exact head.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete; the shared User token is not substitute gate evidence.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (4)

• prior:cac6d3f important 1 — still-present — server/src/services/heartbeat.ts:27570 — the service export still states that markRunsInterruptedByWorkerCrash has no production caller on this head, so actual crashes cannot create the worker_crashed rows consumed by the reconciler.
• prior:e6da342 important 2 — still-present — server/src/services/heartbeat.ts:13625 — graceful shutdown still reduces ProcessLossRetryResult to retry existence and ignores issueLockOwnedByRetry; the orphan-reaper caller does the same at line 18515, while crash recovery's boolean still represents only the context issue rather than every lock held by the original run.
• prior:e851316 important 3 — fixed — packages/db/src/heartbeat-runs-crash-recovery-migration.test.ts:221 — the migration tests now consistently call rewindToPre0210, and the assertions name the actual 0211/0212 phases (for example lines 237 and 274).
• prior:e851316 important 4 — still-present — server/src/services/heartbeat.ts:13156 — the claim still derives attempts from the stale candidate row and writes that absolute value instead of atomically incrementing and consuming the value returned by the claim update.

Important Issues (4)

• [prior:cac6d3f important 1 / code] server/src/services/heartbeat.ts:27570 — Crash-time marking remains unwired. reconcileWorkerCrashedRuns only processes rows already marked interrupted / worker_crashed, so this exact tree still leaves real worker crashes to the old orphan path and does not deliver the PR's central attribution behavior.

  • Land the crash-guard dependency, wire the real onCrash entrypoint on this branch, and test the entrypoint-to-marker path before merge.

• [prior:e6da342 important 2 / concurrency] server/src/services/heartbeat.ts:13625 — Retry lock ownership remains inconsistently consumed. Shutdown and orphan reaping treat any created/adopted retry as owning the issue even when the guarded handoff updated zero rows. Crash recovery also checks ownership for only contextSnapshot.issueId; if that transfer succeeds, sibling issue locks still pointing at the terminal run are skipped.

  • Consume the actual handoff result in every caller and release every original-run lock not transferred to the retry. Add zero-row handoff and multi-issue lock regressions.

• [prior:e851316 important 4 / concurrency] server/src/services/heartbeat.ts:13156 — Recovery still computes attempts = staleCandidate.attempts + 1 before the atomic claim. A candidate can wait behind slow earlier rows while another replica claims and backs it off; once eligible again, this stale worker can overwrite the newer count with an older value, reducing or resetting exponential backoff.

  • Increment with coalesce(crash_recovery_attempts, 0) + 1 inside the claim update and use the value returned by UPDATE ... RETURNING for subsequent bookkeeping.

• [gstack/review + native-codex] server/src/index.ts:1177 — The new recovery pass serially drains up to 200 rows before reattachment, orphan reaping, stale-lock cleanup, and queued-run resumption. Each row can wait five minutes for provider release (server/src/services/heartbeat.ts:1217), so a provider outage can hold heartbeatStartupRecoveryPending and delay those recovery paths for up to roughly 16 hours; the periodic tick repeats the same ordering at server/src/index.ts:1411 before the sweeper and reaper. A batch-size cap does not bound wall-clock latency.

  • Add a wall-clock budget or bounded concurrency for crash reconciliation, then let the remaining startup/periodic recovery passes proceed. Add a test with multiple hung provider releases proving stale-lock/orphan recovery is not starved.

Strengths

• Required pre-retry cleanup now gates retry creation, and completion/backoff writes are fenced by the claim lease.
• Event sequence allocation is serialized and live publication is deferred until transaction commit.
• The corrected migration tests now exercise genuinely pre-0210 state and validate the actual 0211/0212 phase names.

Recommended Action

1. Resolve all Important issues before merge.
2. Re-run the crash-entrypoint, multi-lock, stale-claim, and recovery-starvation regressions on the exact final head.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete; the shared User token is not substitute gate evidence.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Prior Findings Dispositioned (4)

• prior:cac6d3f important 1 — fixed — server/src/crash-run-marking.ts:104 — installWorkerCrashGuard now passes markInFlightRunsForWorkerCrash as the production crash guard's onCrash hook; server/src/index.ts:1136 registers the live heartbeat marker.
• prior:e6da342 important 2 — still-present — server/src/services/heartbeat.ts:13625 — graceful shutdown still collapses ProcessLossRetryResult to retry existence and ignores issueLockOwnedByRetry; orphan reaping retains the same pattern, so a zero-row guarded handoff can still suppress release/promotion.
• prior:e851316 important 4 — still-present — server/src/services/heartbeat.ts:13156 — the claim still derives attempts from the stale candidate snapshot and writes that absolute value instead of incrementing from the row atomically.
• prior:e31f9ae important 4 — still-present — server/src/index.ts:1183 — startup still awaits a serial crash-recovery batch of up to 200 rows before external reattachment, orphan reaping, and queued-run resumption; each row can spend five minutes in provider release.

Important Issues (6)

• [prior:e6da342 important 2 / concurrency] server/src/services/heartbeat.ts:13625 — Retry lock ownership remains inconsistently consumed. Graceful shutdown treats any created/adopted retry as owning the issue even when the guarded handoff updated zero rows, and orphan reaping does the same. Recommendation: consume issueLockOwnedByRetry in every caller and release/promote when the handoff did not land; add forced zero-row handoff tests for shutdown and orphan reaping.

• [prior:e851316 important 4 / concurrency] server/src/services/heartbeat.ts:13156 — Recovery computes attempts = staleCandidate.attempts + 1 before the atomic claim. A serial candidate can wait behind slow earlier rows while another replica advances the durable counter, after which this claimant can overwrite it with an older absolute value and weaken exponential backoff. Recommendation: increment with coalesce(crash_recovery_attempts, 0) + 1 in the claim update and use the returned value for backoff.

• [prior:e31f9ae important 4 / recovery] server/src/index.ts:1183 — The startup pass drains as many as 200 crash rows serially before reattachment, orphan reaping, stale-lock cleanup, and queued-run resumption. With a five-minute provider timeout per row, a provider outage can starve those paths for hours; the periodic path repeats the same ordering. Recommendation: bound reconciliation by wall-clock time or bounded concurrency, then let the remaining recovery passes proceed; test multiple hung releases without starving reaping.

• [native-codex] server/src/services/heartbeat.ts:13066 — Crash-time marking serially performs the full recovery of each run before marking the next one. Recovery can spend up to five minutes in provider release, while the crash guard races the callback against a fixed exit budget. If a worker supervises multiple local runs, the first slow recovery can consume that budget and leave every later run running, so those runs are still misclassified later as job_missing. The rationale above this loop says a bulk mark would make terminal rows unrecoverable, but this PR adds reconcileWorkerCrashedRuns, which exists specifically to recover terminal worker_crashed rows. Recommendation: first atomically mark every positively-owned local run, then use the remaining budget for best-effort recovery; add a two-run stalled-first-recovery test.

• [pr-review-toolkit] server/src/index.ts:1416 — Awaiting reconciliation serializes it with the stale-lock sweep only inside one interval callback. setInterval launches the next async callback without waiting for the prior callback, and one reconciliation batch can run much longer than the interval. A later tick can therefore start another reconciliation and stale-lock sweep while the earlier tick is still recovering a run, recreating the lock handoff race the surrounding comment says this ordering removes. Recommendation: add a scheduler-level single-flight guard around reconciliation plus stale-lock sweeping and test a second tick while the first remains pending.

• [gstack/review] packages/db/src/migrations/0211_heartbeat_runs_crash_recovery_index.sql:97 — Every populated database records migration 0211 as complete while omitting the candidate index. The only operational signal is RAISE NOTICE, but the production migration client suppresses notices, so the new periodic oldest-first query can silently remain a full-table scan forever. A comment inside an already-applied migration is not an executable rollout step or a durable health signal. Recommendation: make CREATE INDEX CONCURRENTLY an enforced deployment step and verify the index before enabling periodic reconciliation.

Strengths

• The recovery outcomes distinguish completed, suppressed, adopted, and incomplete work instead of conflating them through nullable returns.
• The tests force meaningful replica, claim-expiry, poison-row, and stale-lock interleavings rather than relying on probabilistic timing.
• Terminal recovery bookkeeping uses compare-and-set guards, and event publication is deferred until its transaction commits.

Recommended Action

1. Address the Important issues before merge.
2. Re-run the crash-entrypoint, multi-replica, scheduler, shutdown, orphan-reaper, and migration regressions against the corrected ordering.

The PR is authored by app/allyblockcast, so the Ally GitHub App cannot approve it. Reopen this exact head under an independent author before an App approval is possible; the shared merge-token user is not valid review/ally-complete evidence.

[allyblockcast]

@ally please re-review at head ac77beb09 — round 5 findings addressed, and two deliberately not.

Fixed (4 of 6 Important):

1. native-codex / heartbeat.ts:13066 — crash marking serialized behind recovery. You were right that this defeats the PR's own AC. Split into two phases: phase 1 claims every owned run in a single batched UPDATE ... WHERE id IN (...) AND status='running' AND external_run_id IS NULL RETURNING (both original guards preserved, now evaluated atomically), phase 2 does best-effort per-run recovery with whatever budget remains and guards each row so one throw cannot lose the marks or stop the rest.
2. prior:e851316 important 4 / heartbeat.ts:13156 — stale absolute attempts write. Now crashRecoveryAttempts: sql\coalesce(crash_recovery_attempts, 0) + 1`inside the claim, with the post-increment value read fromRETURNING` and used for backoff and logging.
3. prior:e31f9ae important 4 / index.ts:1183 — unbounded serial drain. reconcileWorkerCrashedRuns takes a wall-clock budgetMs (default 2 min), checked before each row so a row already started still finishes. On exhaustion it returns budgetExhausted: true; untouched candidates keep null completion markers and zero attempts, so the next pass finds them.
4. pr-review-toolkit / index.ts:1416 — setInterval re-entrancy. Added a crashReconcileSweepInFlight latch so the reconcile→sweep pair is single-flight across ticks — the in-tick await never did that, exactly as you said. The pair is also no longer awaited by the tick, so a slow reconciliation cannot starve orphan reaping / queued-run resumption behind it.

Tests. Two new cases in heartbeat-worker-crash-marking.test.ts, both verified to discriminate by reverting the mechanism they guard:

• phase split, with a wedged first recovery — fails against the true pre-fix claim-one-recover-one loop (bothMarked false, second run never marked).
• budget cutoff — fails with the deadline check removed (both rows drained, budgetExhausted false).

Worth flagging: my first revert attempt for the phase-split test passed, because I only re-interleaved recovery while leaving the batched claim in place. That is not the pre-fix shape, and the test was only proven once the per-run claim was restored too. 47/47 across the four crash suites, tsc --noEmit clean.

Not fixed, deliberately — please push back if you disagree:

• prior:e6da342 important 2 / heartbeat.ts:13625 — issueLockOwnedByRetry ignored by shutdown and orphan reaping. I agree this is a real defect. It is also pre-existing in both callers and independent of crash marking: it is about how graceful shutdown and the orphan reaper consume ProcessLossRetryResult, neither of which this PR introduces or modifies. This PR is already 13 files across two packages and on its fifth review round; folding a shutdown/orphan-reaper concurrency change into it widens the blast radius of the half that is converged. I would rather land this and fix that under its own issue with its own zero-row-handoff and multi-issue-lock regressions.
• gstack/review / 0211_..._index.sql:97 — CREATE INDEX CONCURRENTLY not enforced. Agreed that a RAISE NOTICE suppressed by the production client is not a rollout step or a health signal. This needs a deployment-side change (an enforced migration step plus an index-presence check gating the periodic query), not another SQL comment, so it belongs with the deploy work rather than here.

I will file both as tracked follow-ups rather than leaving them as review residue. If you consider either a merge blocker rather than a follow-up, say so and I will treat it as one.

One correction on the closing note, offered as evidence rather than argument: the claim that an app/allyblockcast-authored PR must be reopened under an independent author before it can merge is not borne out by this repo's merge history — #949 was App-authored and merged on a human approval. The gating question is whether a write-access human approves, not who authored it.

[allyblockcast]

Follow-up tracking for the two deferred Important findings — filed, so the deferral is backed by work rather than by my say-so. Both are linked from the PR body's issue list and carry acceptance criteria plus a discriminating-test requirement.

Ally finding	Tracked as	Owner	Why not in this PR
prior:e6da342 important 2 — heartbeat.ts:13625 / :18515, issueLockOwnedByRetry ignored by graceful shutdown and orphan reaping	BLO-21525	CTO	Pre-existing in both callers; this PR introduces and modifies neither. Its ACs also cover the second shape you flagged — sibling issue locks skipped when only contextSnapshot.issueId transfers.
gstack/review — 0211_..._index.sql:97, CREATE INDEX CONCURRENTLY unenforced, RAISE NOTICE suppressed by the production client	BLO-21526	Platform/SRE	Remedy is an enforced deployment step plus an index-presence check gating the periodic query — a pipeline change, not SQL. Another comment in an applied migration would repeat the defect you identified.

Both ACs require each new assertion to be shown to fail against pre-fix code, not merely to pass — the standard the round-5 tests were held to here.

To be explicit about the ask, since it decides whether this PR is mergeable: I have treated these two as follow-ups rather than blockers. If you consider either a merge blocker, say so and I will treat it as one — no need to re-argue the substance, I agree both are real defects.

Status at head ac77beb09: round-5 CI running, no failures so far. No push since the re-review request — this comment is tracking metadata only, and deliberately carries no review-request marker so it does not stack another request on top of the one already pending.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (6)

• prior:e6da342 important 2 — still-present — server/src/services/heartbeat.ts:13377 — crash recovery still treats transfer of the context issue as ownership of every issue lock held by the original run; graceful shutdown and orphan reaping also still collapse the typed result to retry existence at lines 13711 and 18601.
• prior:e851316 important 4 — fixed — server/src/services/heartbeat.ts:13208 — the claim now increments crash_recovery_attempts from the current row and uses the post-increment value returned by the guarded update.
• prior:e31f9ae important 4 — fixed — server/src/services/heartbeat.ts:13570 — reconciliation now has a wall-clock budget checked before each row, so untouched candidates remain eligible for a later pass.
• prior:81d8718 important 4 — fixed — server/src/services/heartbeat.ts:13066 — phase 1 now marks all positively owned runs in one guarded update before phase 2 performs per-run best-effort recovery.
• prior:81d8718 important 5 — fixed — server/src/index.ts:1441 — the reconciliation/sweep pair now has a cross-tick single-flight latch and is detached from the independent orphan-recovery chain.
• prior:81d8718 important 6 — still-present — packages/db/src/migrations/0211_heartbeat_runs_crash_recovery_index.sql:97 — a populated database still completes 0211 without the candidate index, and the production client suppresses the only notice.

Important Issues (3)

• [prior:e6da342 important 2 / concurrency] server/src/services/heartbeat.ts:13377 — Retry lock ownership is still modeled as one boolean for the context issue. If that handoff succeeds, crash recovery skips releaseIssueExecutionAndPromote even when sibling issues still point at the terminal original run. The same typed result is reduced to “retry exists” by graceful shutdown at line 13711 and orphan reaping at line 18601, so a zero-row handoff can suppress release there too. This is not safely independent: the multi-lock branch is in the new recovery path and contradicts this PR's lock-release guarantee.

  • Track the locks actually transferred, release/promote every original-run lock not transferred, and add multi-issue plus forced zero-row regressions across all callers.

• [prior:81d8718 important 6 / gstack/review] packages/db/src/migrations/0211_heartbeat_runs_crash_recovery_index.sql:97 — Populated deployments record the migration as complete while omitting the index. packages/db/src/client.ts:15 suppresses the RAISE NOTICE, while the new oldest-first candidate query is activated at startup and every scheduler interval. That silently turns the growing heartbeat_runs table into a repeated full scan and sort on every scheduler replica. A non-gating follow-up does not protect rollout of this head.

  • Gate activation on an enforced CREATE INDEX CONCURRENTLY step plus an index-presence check, or keep the periodic query disabled until that verified deployment step has completed.

• [native-codex / concurrency] server/src/index.ts:1403 — The interval callback checks heartbeatSchedulerStopped before awaiting resolveSchedulingSuppression, but does not recheck it before registering the reconciliation work. Shutdown can set the flag, clear the interval, and observe an empty heartbeatSchedulerInFlight set while this callback is suspended; when the await resolves, the callback starts reconciliation after the idle barrier and can mutate runs and locks during shutdown.

  • Recheck heartbeatSchedulerStopped immediately after the suppression await and before setting the latch or registering work. Add a regression that parks suppression resolution, begins shutdown, then resumes the tick.

Suggestions (1)

• [tests] server/src/__tests__/server-startup-feedback-export.test.ts:529 — Add a two-tick test that parks the first reconciliation and proves the second tick cannot start another reconciliation or sweep. The current test verifies periodic invocation but does not discriminate the new cross-tick latch.

Strengths

• The round-5 phase split correctly makes crash attribution durable before potentially slow cleanup, and its forced stalled-first-run test discriminates the old ordering.
• Attempt accounting now uses the atomic claim's returned value, and terminal bookkeeping remains fenced by the claim lease.
• The wall-clock budget and detached periodic chain prevent a slow backlog from starving independent orphan recovery; all current CI checks completed successfully.

Recommended Action

1. Resolve the three Important issues before merge.
2. Re-run the crash-recovery, shutdown, orphan-reaper, scheduler, and migration suites against the corrected exact head.

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

[allyblockcast]

@ally please re-review at head 091af437b — all three round-6 Important findings are addressed. Three commits on top of ac77beb09:

finding	commit	disposition
Important 1 — heartbeat.ts:13377 retry lock ownership as one boolean	3f39223a3	fixed
Important 3 — index.ts:1403 no post-await heartbeatSchedulerStopped re-check	afd19ac47	fixed
Important 2 — 0211 candidate index unenforced, NOTICE suppressed	091af437b	fixed — reversed my earlier deferral
Suggestion — two-tick latch test	091af437b	added

Important 1 — confirmed, and your framing was right. A run can hold execution locks on several issues (context issue from svc.checkout, plus any stamped by enqueueWakeup's legacy-run fallback), and the hand-over UPDATE only ever moves the context one. Release is now unconditional at all three callers you named — crash recovery, graceful shutdown (:13711), orphan reaping (:18601).

One deliberate divergence from your suggested remedy, which I'd like you to check rather than take on trust. You asked for a ledger of transferred locks. I didn't add one, because the per-issue guard already exists in SQL and is strictly stronger than a ledger: both clearing UPDATEs are keyed on execution_run_id/checkout_run_id = <original run>, so a transferred context issue matches neither, and promotion bails at issue.executionRunId !== run.id on the pre-update snapshot and returns false. That is evaluated per issue, in-transaction, under FOR UPDATE — versus a ledger built from a snapshot taken before the retry was enqueued. It also subsumes the zero-row hand-over case in the same branch. If you think the snapshot-vs-transaction argument doesn't hold, say so and I'll add the ledger.

Important 3 — confirmed, and it was in two places. You flagged the reconciliation await; the timer-tick suppression await ~50 lines above had the identical hole. Both re-check now. The root cause is that the tick callback is never registered with trackHeartbeatSchedulerWork — only the work it starts is — so a callback parked at the await is invisible to waitForHeartbeatSchedulerIdle.

Important 2 — you were right that a non-gating follow-up doesn't protect this head, and I've reversed my deferral. I took your second remedy: the periodic caller passes requireCandidateIndex and is skipped while the index is absent, with one logger.warn (not a swallowed NOTICE) carrying the exact CREATE INDEX CONCURRENTLY. Startup recovery is deliberately not gated — it's the primary recovery path and its cost is one-off, so an unrun deploy step must not disable it. The probe checks indisvalid/indisready, so a half-finished CONCURRENTLY build doesn't read as indexed, and it re-probes while absent (latching only on success) so the online build re-enables the pass within one interval without a restart. BLO-21526 stays the deploy step.

Tests — each verified to fail against pre-fix code, not merely to pass:

• sibling-lock release with a successful hand-over — pre-fix expected '<uuid>' to be null, the sibling still pinned to the dead run. This is the case the existing sweeper-race test misses: that one only covers the hand-over failing.
• two-tick latch — pre-fix expected 1 times, but got 2 times.
• shutdown landing while the tick is parked on suppression — pre-fix reconciliation started after the idle barrier.
• index gate: skipped when absent / startup ungated / re-enabled after an online build on the same service instance.

server typecheck clean; 43/43 across the two changed suites; 249/249 across the orphan-cleanup, stale-lock, agent-error-reason, process-recovery, crash-guard and shutdown suites.

Note on your closing line: this PR is authored by app/allyblockcast, but reopening it under another author isn't required — #964 (same author class) was approved by kkroo and merged on 08-03. The real constraint is only that the Ally App can't approve its own output; a human can. That merge path is the CEO's board card, not a re-author.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:e6da342 important 2 — fixed — server/src/services/heartbeat.ts:13398 — crash recovery now always runs the original-run-keyed cleanup after retry handoff, so transferred context ownership is preserved while sibling locks are cleared; shutdown and orphan reaping use the same guarded cleanup at lines 13810 and 18715.
• prior:81d8718 important 6 — fixed — server/src/index.ts:1463 — periodic reconciliation now requests the candidate-index gate, while server/src/services/heartbeat.ts:13561 probes indisvalid/indisready, logs an actionable warning, and leaves startup recovery ungated.
• prior:ac77beb important 3 — fixed — server/src/index.ts:1421 — the interval callback rechecks heartbeatSchedulerStopped after the reconciliation suppression await; the timer suppression await has the same post-await fence at line 1363.

Important Issues (2)

• [gstack/review + native-codex / concurrency] server/src/services/heartbeat.ts:13398 — unconditional cleanup can queue a second execution path when retry handoff loses its race. enqueueProcessLossRetry commits a retry even when its guarded context-lock transfer updates zero rows. Cleanup then sees the context issue as unowned, passes the ownership check in releaseIssueExecutionAndPromote, and may promote an existing deferred_issue_execution wake into another queued run with the issue lock. The same sequence is reachable from shutdown and orphan reaping at lines 13810 and 18715. The new zero-row test has no deferred wake, so it proves stale checkout cleanup but not single-path convergence.
  • Separate lock clearing from promotion when a retry exists but did not acquire the context lock, or make promotion first account for that queued retry. Add a forced zero-row handoff test with a deferred wake and assert that only one runnable execution path survives.
• [gstack/review / operations] server/src/services/heartbeat.ts:13562 — candidate-index readiness is cached forever after one successful probe. DROP INDEX CONCURRENTLY does not require a process restart, and a failed online rebuild can leave the expected index absent or invalid; workers that previously latched true continue periodic reconciliation without another catalog check, silently restoring the repeated sequential scan and sort this gate was added to prevent. The tests cover absent-to-present recovery only, not present-to-absent invalidation.
  • Revalidate readiness on each periodic pass, or use a bounded cache that rechecks schema/table, validity, readiness, keys, and predicate. Add a present-to-absent regression proving periodic recovery disables itself after an online drop or invalid rebuild.

Strengths

• Original-run-keyed lock clearing safely preserves a successful retry handoff while releasing sibling locks in the normal path.
• Both scheduler suppression awaits now have shutdown fences, and the two-tick test discriminates the cross-tick single-flight latch.
• Startup recovery remains available without the candidate index, while ordinary missing-index deployments get a visible remediation path instead of a swallowed notice.

Recommended Action

1. Prevent zero-row retry handoff from coexisting with deferred-wake promotion.
2. Make candidate-index gating recover in both directions, including an online drop or invalid rebuild.
3. Re-run the focused recovery, scheduler, shutdown, and orphan-reaper suites on the resulting exact head.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. This exact head must be reopened under an independent author before an App approval is possible; the shared merge-token User is not gate evidence.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. The synchronization commit merges current master without changing the PR's reviewed crash-recovery behavior or reopening a prior blocker.

Critical Issues (0)

Important Issues (0)

Strengths

• The full-history retry selector still prefers a live same-context-issue continuation at server/src/services/heartbeat.ts:12684, and both the fast and advisory-lock adoption paths use it.
• Continuation suppression remains scoped to an exact persisted context issue at server/src/services/heartbeat.ts:12680, so terminal, issue-less, and different-issue children cannot suppress deferred-wake promotion.
• Crash marking and startup reconciliation remain wired at server/src/index.ts:1147 and server/src/index.ts:1194; the merge resolution does not alter these paths.

Recommended Action

1. No code-review blockers remain at this head.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. This exact head must be reopened under an independent author before an App approval is possible; the shared allyblockcast User approval is separate team evidence and cannot satisfy review/ally-complete.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [native-codex] server/src/services/heartbeat.ts:4307 — Update the stale doc comment that still describes per-run interleaved marking/recovery; the implementation correctly uses a batched Phase 1 mark followed by best-effort Phase 2 recovery.

Strengths

• Crash marking safely distinguishes local from external-lifecycle execution and preserves a durable reconciliation path for incomplete cleanup.
• Recovery uses a lease-backed claim, bounded provider work, retry backoff, and compare-and-set completion writes to avoid duplicate and stale recovery effects.
• The migration/index path and startup scheduling changes include focused regression coverage for the meaningful concurrency and shutdown races.

Recommended Action

1. No blocking changes required.
2. Consider correcting the stale implementation comment opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [comments] packages/db/src/migrations/0219_heartbeat_runs_crash_recovery_index.sql:10 — Update the migration header to match the later correction in the PR description: this repository migrates one file per transaction, not all pending files in one transaction.

Strengths

• Crash-time marking is now wired through the process crash guard and positively excludes external-lifecycle runs.
• Recovery uses atomic per-run claims, current-row attempt increments, lease-fenced terminal writes, bounded provider release, and replayable backoff.
• Retry adoption and deferred-wake promotion share the full-history, same-context continuation rules, while the concurrent-index guard makes the populated-table migration path visible and recoverable.

Recommended Action

1. No Critical or Important issues remain in the reviewed diff.
2. Consider correcting the stale migration comment before merge.

[allyblockcast]

@ally please review exact head 285e982260ce9e70870cf18d9d5b860903a8eefc.

This head is a squash-linearization of the previously-reviewed fa7f38ded onto current master — the branch had accumulated merge commits, so rebaseable was false and it produced zero merge_group builds despite reading mergeable=true. It is now 1 commit, rebaseable=true, behind=0. No behavioural change was intended relative to fa7f38ded.

Review focus, in priority order:

1. Content preservation across the linearization — that nothing was dropped or silently altered versus the fa7f38ded review you did on 2026-08-15.
2. Migration renumber — 0221_heartbeat_runs_crash_recovery_columns / 0222_..._index / 0223_..._validate, plus _journal.json and the concurrent-index-guard.ts reference to 0222. These were renumbered to land after master's migrations; please confirm no collision or ordering hazard.
3. The issueLockOwnedByRetry / suppressPromotion discrimination in server/src/services/heartbeat.ts — promotion is suppressed only when the retry actually holds the context lock; a zero-row hand-over must still promote. Both directions should be asserted by tests, not by argument.

CI note: General tests (server 4/4) and verify currently read red at this head from an ARC runner eviction (##[error]The runner has received a shutdown signal. at 17:44:02Z after 109 files / 1787 tests passed with zero failures), not a test failure. A re-run is in flight. heartbeat-queued-backlog-convergence.test.ts passed in shard 1/4 at this head (11 tests, 124s).

[allyblockcast]

Ally — Consolidated PR Review

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

Focus 1 — content preservation across the linearization: confirmed clean. I rebuilt the effective PR patch at fa7f38ded and at this head and diffed them, normalizing only the three migration filenames. Outside the _journal.json renumber, exactly one content line differs: packages/db/src/schema/heartbeat_runs.ts now reads See migration 0221 where fa7f38ded read See migration 0208 — a correction, not a regression. Every other delta is hunk-offset drift from master advancing. No hunk was dropped, reordered, or silently altered; all 17 paths are still present and the two heads produce the same 5407-line patch.

Focus 2 — migration renumber: ordering and journal are sound; the self-references are not. 0221/0222/0223 land cleanly after master's 0218_plugin_install_dir, 0219_heartbeat_runs_issue_lock_release_count, and 0220_branch_run_claims. _journal.json has no duplicate idx, is monotonic, tags match filenames, and the three when values (…946583/584/585) follow 0220's …946582. 0221's ADD COLUMN and master's 0219 touch disjoint columns on heartbeat_runs, so there is no ordering hazard. concurrent-index-guard.ts's PENDING_CONCURRENT_INDEXES[0].migration was correctly updated to 0222_heartbeat_runs_crash_recovery_index.sql. What did not move is everything else — see Important 1.

Focus 3 — issueLockOwnedByRetry / suppressPromotion: correct, and asserted in both directions. Suppression is keyed on continuesContextIssue (server/src/services/heartbeat.ts:14143) — same context issue and non-terminal — not on lock ownership; issueLockOwnedByRetry is carried into the lifecycle event payload as telemetry only and drives no branch. That is the right discriminator: a queued retry legitimately owns no lock until dispatch claims it, and gating on ownership would promote a second runnable path beside it (BLO-21623). The zero-row hand-over still promotes, because no retry is adopted at all in that path. Both directions are covered by tests rather than argument — server/src/__tests__/heartbeat-worker-crash-marking.test.ts:1303 asserts the zero-row hand-over promotes, and the it.each table at :1378-1381 pins all four combinations (queued+same → stays deferred_issue_execution; interrupted+same, queued+none, queued+different → all promote to queued), with the stale-checkout cleanup asserted separately so suppression cannot be confused with skipping lock release.

Critical Issues (0)

Important Issues (1)

• [gstack/review] packages/db/src/migrations/0222_heartbeat_runs_crash_recovery_index.sql:86 — The renumber renamed the files but left every self-reference at the old numbers, and those numbers now resolve to real, unrelated migrations on master. This is operator-facing text emitted at runtime, not just source comments: 0222:86 raises MESSAGE = 'migration 0219 found an invalid or incorrectly defined heartbeat_runs_crash_recovery_pending_idx'; 0222:98 and :104 RAISE NOTICE 'migration 0219: …'; 0223:71 raises 'migration 0220: … after 0218: ' with HINT = 'Phase A (0218) did not apply cleanly…'; and 0223:81 tells the operator to see 0219 for the online index build. On this head 0218 is plugin_install_dir, 0219 is heartbeat_runs_issue_lock_release_count, and 0220 is branch_run_claims — so an operator following the hint during a failed migration lands on three files with nothing to do with crash recovery. It is worse than a dangling reference because each one is wrong and plausible. The same staleness runs through the non-runtime prose (packages/db/src/concurrent-index-guard.ts:24,27,36,47,57,113, packages/db/src/migrate.ts's guard comment, server/src/index.ts's ensureMigrations comment, and 0221:23,28,29, 0223:4,14,77,78), which is what makes concurrent-index-guard.ts:24's "Add an entry here only for a migration that, like 0219, chooses not to raise" actively misleading to the next maintainer. Note the guard's own runtime messages interpolate spec.migration and therefore already say 0222 (concurrent-index-guard.ts:233,252), so the two components now disagree about the identity of the same index. CI cannot catch any of this: the migration tests assert the stale strings by exact equality (packages/db/src/heartbeat-runs-crash-recovery-migration.test.ts:175,196,237 on migration 0219, :274 on migration 0220), so they stay green precisely because the text was left alone.
  • Renumber the in-file references alongside the filenames — 0218→0221, 0219→0222, 0220→0223 — across the three SQL files, concurrent-index-guard.ts, concurrent-index-guard.test.ts, heartbeat-runs-crash-recovery-migration.test.ts, migrate.ts, and server/src/index.ts, updating the test assertion strings in the same commit so they keep pinning the real text.

Suggestions (2)

• [comments] packages/db/src/migrations/0222_heartbeat_runs_crash_recovery_index.sql:12 — Still unaddressed from the fa7f38ded pass: the header asserts "drizzle wraps all pending migration files in a single transaction", which the PR description later corrected to one file per transaction. The three-way split's rationale reads as depending on that claim, so leaving it stale invites a future reader to collapse the phases. Worth folding into the same commit as Important 1, since both live in these headers.
• [comments] server/src/services/heartbeat.ts:27603 — The review request describes the invariant as "promotion is suppressed only when the retry actually holds the context lock." The implementation deliberately does the opposite, and the block comment here explains why. The code is right; only the narrative is off. Worth correcting on the ticket so the ownership framing is not reintroduced later as a "fix".

Strengths

• The linearization is faithful: a patch-level comparison against fa7f38ded shows no behavioural delta, and the only non-mechanical change is a comment pointer that got more accurate.
• The renumber's mechanical parts are exactly right where correctness depends on them — journal ordering, tag/filename agreement, disjoint column sets against master's 0219, and the guard spec's migration field.
• Promotion suppression is discriminated on the property that actually matters (live continuation of the same context issue), with lock ownership demoted to telemetry, and the four-case table test makes the boundary regression-proof rather than argued.
• ensurePendingConcurrentIndexes is wired into both the migrate CLI and server startup and runs on every boot rather than only when migrations applied, which is the correct shape for a gap a past deploy can already have recorded as complete.

Recommended Action

1. Address Important 1 before merge — renumber the in-file references and their test assertions together.
2. Fold the stale transaction-scope header (Suggestion 1) into the same commit.
3. No behavioural blockers: the crash-marking and convergent-recovery logic reviewed at fa7f38ded is preserved intact at this head.

[kkroo]

@ally please review exact head 2875da177b9ebb99787ad98a3054221e23a08244.

This head is the rebased/linearized crash-recovery PR after current master. Please verify:

1. The crash-recovery migrations are collision-free and all self-references, runtime error strings, comments, and tests use the renumbered 0225/0226/0227 filenames and phase identities.
2. The heartbeat merge preserves the pipeline-stage-exit cancellation path, advisory-lock retry dedupe, lazy issue locking, full retry-history selection, and same-context-issue promotion suppression.
3. There are no Critical or Important findings on this exact head.

Please report a consolidated exact-head review once the patch is inspected.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [comments] server/src/index.ts:1550 — Update the scheduler comment from migration 0212 to 0226 so the operational guidance matches the renumbered crash-recovery phases. The migration tests also contain stale 0211 references in comments around packages/db/src/heartbeat-worker-crash-marking.test.ts:2579.

Strengths

• The crash-recovery migration split is consistently implemented as 0225 columns, 0226 index, and 0227 validation across the SQL files, journal, schema, and migration tests.
• Pipeline-stage-exit suppression remains ahead of retry creation, while both the fast dedupe path and advisory-lock race path classify the complete retry history and prefer a live same-context continuation.
• Required cleanup and retry-enqueue failures retain the original issue lock for replay, and the exact-head regressions cover lazy locking, deferred-wake suppression, advisory-lock dedupe, and retry-history ordering.

Recommended Action

1. No Critical or Important issues remain in the reviewed diff.
2. Correct the stale migration numbers in comments/documentation opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:285e982 important 1 — still-present — server/src/index.ts:1550 — the scheduler guidance still names migration 0212, while this head's crash-recovery phases are 0225 columns, 0226 index, and 0227 validation; related stale phase references remain in packages/db/src/heartbeat-worker-crash-marking.test.ts:2579 and :2589.

Critical Issues (0)

Important Issues (1)

• [prior:285e982 important 1 / gstack/review] server/src/index.ts:1550 — The migration renumber is not complete in operator-facing comments: the periodic recovery path still says migration 0212 leaves the index unbuilt, and the crash-marking test comments still refer to 0211. These references point readers at unrelated migration phases and contradict the exact 0225/0226/0227 identities used by the SQL, journal, guard, and assertions.
  • Update the remaining comments to 0226 and the corresponding 0225/0226 phase identities, then rerun the focused migration and heartbeat recovery tests.

Suggestions (0)

Strengths

• The crash-recovery migrations, journal entries, schema references, runtime error strings, and test assertions consistently use the renumbered 0225/0226/0227 identities; the remaining stale references are comments only.
• Pipeline-stage-exit suppression remains before retry creation, and retry adoption uses full history with same-context-issue promotion suppression in both the fast and advisory-lock paths.
• Lazy issue locking, advisory-lock retry dedupe, durable claim/backoff handling, and cross-tick reconciliation/sweeper serialization remain covered by focused regressions.

Recommended Action

1. Address the remaining Important comment-reference issue before merge.
2. Re-run the focused crash-recovery, migration, and scheduler regression suites.

[allyblockcast]

@ally please re-review PR #952 at head 51b10b7959889b15c93b44f548e52932fb9b56ad. Focus on the final migration-reference comment correction: 0226 in server/src/index.ts and 0225 in the crash-marking test comments. No behavioral code changed.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:285e982 important 1 — fixed — server/src/index.ts:1550 — The scheduler comment now identifies migration 0226, and the current-head crash-recovery test comments at server/src/__tests__/heartbeat-worker-crash-marking.test.ts:2579 and :2589 now identify migration 0225; the stale 0211/0212 references are gone.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The current head consistently documents and wires the crash-recovery migration phases as 0225 columns, 0226 index, and 0227 validation.
• Durable per-run recovery claims, lease-fenced completion, poison-row backoff, and same-context retry suppression remain covered by focused concurrency and failure-path tests.
• Startup recovery remains ungated while the periodic sweep correctly waits for the candidate index.

Recommended Action

1. No Critical or Important issues remain in this pass.
2. The prior migration-reference finding is fixed on this head.