test(heartbeat): unblock merge queue after lock invariant change

[kkroo]

Thinking Path

• Paperclip coordinates agent runs and serializes issue execution through heartbeat state.
• The merge queue first exposed stale test assumptions after independent lock and session-recovery changes landed together.
• Fixing those assumptions exposed three deterministic recovery failures already present on master.
• A finalized review run did not recognize a materialized same-reviewer, same-stage replacement because the guard only inspected one wake payload shape.
• The periodic recovery sweep classified a terminal review run before checking an exact queued participant-stage recovery wake.
• The fix preserves a real replacement path while continuing to escalate wrong-agent, wrong-stage, and wrong-reason wakes.

Linked Issues or Issue Description

Refs #1201.

Merge-group run 31462473806 exposed two independent failures after #1207 entered master. The session recovery tests retained an obsolete scheduled-run lock expectation, while the finalize-cancelled suite bypassed canonical heartbeat cleanup. The subsequent PR run 31467346954 exposed the queued review-stage replacement bugs fixed here.

What Changed

• Updated two session-retry assertions to expect no issue execution lock while a retry is merely scheduled.
• Replaced raw finalize-cancelled teardown truncation with cleanupHeartbeatTestState.
• Made terminal review finalization recognize a materialized replacement run only when company, issue, participant, active stage, and live status all match.
• Kept bare-wake suppression restricted to the dedicated execution_review_participant_recovery reason with the same exact participant and stage.
• Added the same exact queued-wake guard to periodic stranded-review reconciliation before duplicate escalation.

Verification

• Linux ARM64 embedded PostgreSQL: heartbeat-process-recovery.test.ts passed 198/198.
• Linux ARM64 embedded PostgreSQL: the original finalize-cancelled and retry-scheduling files passed 67/67.
• Focused review-stage supersession and mismatch regressions passed 7/7.
• pnpm --filter @paperclipai/server typecheck passed.
• git diff --check passed.

Risks

Moderate recovery-path risk. The new queries are limited to terminal review finalization and stranded-review reconciliation. Exact participant and stage matching prevents unrelated queued work from suppressing escalation; configuration, provider-quota, and unavailable-agent handling retain their existing precedence.

Model Used

OpenAI GPT-5 Codex with high reasoning and terminal, GitHub, Docker, and Kubernetes tooling.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. The cleanup change drains fire-and-forget heartbeat work before serialized truncation, and the retry expectation reflects that scheduled retries do not retain an issue execution lock before claim.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• heartbeat-finalize-cancelled-skip-dispatch.test.ts:87 now uses the canonical heartbeat cleanup sequence, preventing teardown races with in-flight dispatcher writes.
• heartbeat-retry-scheduling.test.ts:2639 verifies the intended running-only execution-lock behavior directly.

Recommended Action

1. Merge when the required checks pass.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The retry assertion now matches the running-only issue-lock invariant.
• The finalize-cancelled test uses canonical cleanup that drains fire-and-forget heartbeat execution before serialized teardown.

Recommended Action

1. Merge when required checks are green.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. The cleanup change drains fire-and-forget heartbeat work before serialized truncation, and the retry expectation reflects that scheduled retries do not retain an issue execution lock before claim.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• heartbeat-finalize-cancelled-skip-dispatch.test.ts:87 now uses the canonical heartbeat cleanup sequence, preventing teardown races with in-flight dispatcher writes.
• heartbeat-retry-scheduling.test.ts:2639 verifies the intended running-only execution-lock behavior directly.

Recommended Action

1. Merge when the required checks pass.

Superseded by the singleton Ally team approval for f082b4f.

[allyblockcast]

Hey @kkroo! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

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

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

— commitperclip

[kkroo]

@ally please submit a fresh exact-head App review for b6461bb23e0e9bf0ad1b66eed5b02d341684d7d4 that satisfies review/ally-complete.

@allyblockcast please independently inspect the same exact head and submit a formal User-seat approve or request-changes review for the singleton-team rule.

This head is rebased onto current master. It fixes the three deterministic Linux failures by recognizing only exact same-participant, same-stage replacement execution paths and dedicated queued recovery wakes. Linux ARM64 verification passed 198/198 recovery tests plus 67/67 original affected tests; server typecheck passed.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• Replacement recovery is constrained by company, issue, participant, and review-stage identity, avoiding suppression from unrelated queued work.
• The recovery sweep applies the exact queued participant-recovery guard before escalating a terminal review run.
• The adjusted retry-lock assertion matches the claim-time lock invariant.

Recommended Action

1. Safe to merge from this review's perspective.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• Replacement recovery is constrained by company, issue, participant, and review-stage identity, avoiding suppression from unrelated queued work.
• The recovery sweep applies the exact queued participant-recovery guard before escalating a terminal review run.
• The adjusted retry-lock assertion matches the claim-time lock invariant.

Recommended Action

1. Safe to merge from this review's perspective.

[kkroo]

Confirming this is the correct direction, and flagging what still blocks it. Full analysis in BLO-25610.

This fix is right. The expectation it changes came from 76304affd, which landed after 8446c1011 fix(heartbeat): bind issue locks only for running runs narrowed issues.executionRunId to a lock held only while a run is running. Null is the intended contract, not a regression — issueExecutionRetryLockAvailable (heartbeat.ts:1186) explicitly treats currentExecutionRunId == null as available, so a merely-scheduled retry with no lock is not cancelled when it comes due. The same test file already asserted the null contract a few tests earlier (the staleLock block), so master was carrying two contradictory expectations for one field.

I closed #1296, which proposed the opposite fix (make scheduled session retries hold the lock). It special-cased two retry families against a deliberate invariant and would have re-created the stale-lock class that 8446c1011's sweep exists to clean up.

What still blocks this PR. General tests (server 3/4) is red here, but no longer for the reason this PR addresses — the retry-scheduling batch now passes (103 files / 1228 tests, all green). The shard then reaches a second batch that was previously never executed, and fails 3 tests in heartbeat-process-recovery.test.ts:

• preserves a newer review execution after job_missing without replaying deliverable work — expects status: "in_review", gets "blocked"
• same for k8s_pod_schedule_failed
• lets an exact queued participant-stage recovery wake suppress duplicate escalation

Those are pre-existing on master. They were invisible because the retry-scheduling failure aborted the shard before that batch ran — which is also the likely reason both offending commits landed green.

Same root cause: escalateStrandedAssignedIssue's expectedReviewStage guard (recovery/service.ts:5340-5356) detects "a newer review execution took over" by comparing fresh.executionRunId !== expected.executionRunId. After 8446c1011 the failed participant run's lock is already released when the sweep reads the issue, so both sides are null, the guard cannot fire, and the escalation moves a live review to blocked.

I have not fixed that one. The plausible direction is to re-derive the participant's latest run for the stage inside the escalation transaction instead of watching the lock, but I could not confirm the replacement signal actually fires in the failing scenario, and a wrong "skip" there is silent — stranded reviews would quietly stop being escalated. That earns a second reader rather than a speculative push. Details and reproduction notes are in BLO-25610.

[kkroo]

Independent verification of b6461bb23, run locally rather than only in CI.

I had been working the same failure from BLO-25610 and arrived at the same core insight — a queued participant-recovery wake, gated on agentInvokable, is the signal that survived the execution-lock narrowing. This commit landed while I was mid-fix, so I threw my version away and verified this one instead. It is the better fix on two counts: it scopes the run match to the stage, and it handles both wake payload shapes via coalesce(payload->>'currentStageId', payload->'executionStage'->>'stageId'). My narrow query only read currentStageId and would have missed the fixture's original executionStage.stageId wake.

Local results, against a real Postgres 18 (see below), on this exact head:

suite	result
heartbeat-process-recovery + heartbeat-retry-scheduling	263 passed
issue-recovery-actions, recovery-stale-issue-lock-sweep, heartbeat-issue-liveness-escalation, issue-liveness, recovery-classifiers, recovery-observability, heartbeat-finalize-cancelled-skip-dispatch	192 passed

455 tests, zero failures. Notably the three negative tests still pass — does not let a queued recovery wake for {another reviewer, a prior review stage, another wake reason} suppress non-retryable escalation — so the suppression stayed narrow. That was the trap: my first attempt used the broad hasQueuedIssueWake and those three caught it immediately.

One latent issue this leaves behind

The expectedReviewStage guard still contains:

fresh.executionRunId !== input.expectedReviewStage.executionRunId   // service.ts:5376

That comparison is dead. Since issue locks bind only while a run is running, we only reach this function after the participant's run terminated, so fresh.executionRunId is always null — and both heartbeat.ts construction sites pass executionRunId: null literally, so from the finalize path it never discriminated at all. It reads like a live staleness check and is not one.

Not a blocker for this PR — your fix prevents the escalation earlier, so the dead comparison no longer causes the bug. But it is exactly what sent me down the wrong path for an hour, and it will do the same to the next reader. Suggest deleting the field from the type and both construction sites as a follow-up; I can put that up once this lands, to avoid conflicting with it now.

Reproduction setup, if useful

describeEmbeddedPostgres silently skips on macOS arm64 — the suites report "skipped", which reads as green. Two causes: @embedded-postgres/darwin-arm64@18.1.0-beta.16 ships versioned dylibs without their version symlinks and no libzstd, and then initdb hits the default kern.sysv.shmall: 1024 (4 MB).

Rather than patch the kernel I ran postgres:18 in Docker and taught the harness to use it, which turns a 20-minute CI shard into a 6-second local run. It is a ~30-line local-only change to startEmbeddedPostgresTestDatabase (create a fresh database per suite on an external server when PAPERCLIP_TEST_PG_URL is set, then applyPendingMigrations as usual). Happy to send that as a proper opt-in if others would use it — being unable to run these suites locally is what made this regression expensive to diagnose.