fix(heartbeat): terminalize queued runs detached from their issue lock (BLO-21621)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work.
• The heartbeat service owns queued runs, issue execution locks, startup recovery, and periodic liveness sweeps.
• A queued heartbeat_runs row can outlive the issue lock that originally named it because the lazy-locking model stamps issues.executionRunId at claim time, and stale-lock cleanup can later clear that pointer without cancelling the still-queued row.
• Once detached, recovery sees the stale queued row as proof of life through hasActiveExecutionPath, so the issue is skipped by sweeps that would otherwise wake fresh work.
• This pull request adds a detached queued-run reconciliation pass that only cancels rows that are both stale and no longer named by the issue's checkout/execution lock.
• The benefit is that dead queued rows stop occupying invisible liveness slots, while legitimate long-lived queued backlog remains untouched when the issue still names that run.

Linked Issues or Issue Description

Refs: BLO-21621

Root cause traced from the issue investigation and code reading:

• sweepStaleIssueLocks intentionally clears a stale pre-claim executionRunId pointer after 6 hours without cancelling the underlying queued run.
• hasActiveExecutionPath, used by reconcileStrandedAssignedIssues and blocked-dependency wake logic, treats any queued / running / scheduled_retry row referencing the issue as live work, with no staleness check.
• A queued and detached run can therefore keep an issue looking active forever even though no agent will claim that exact row.

What Changed

• Added heartbeat.reconcileDetachedQueuedRuns() to cancel stale queued runs whose issue no longer names them via checkoutRunId or executionRunId.
• Re-wakes the issue exactly once only when no other run currently owns the issue lock.
• Leaves age-old queued runs alone when they are still the recognized lock holder, preserving real backlog for agents saturated at their concurrency ceiling.
• Wires the sweep into startup recovery and the periodic scheduler immediately after sweepStaleIssueLocks.
• Added embedded Postgres coverage for terminalize-and-rewake, still-attached skip, and superseded-run no-rewake behavior.
• Added startup-test coverage/mocking for the new sweep so startup recovery keeps moving even when the sweep fails.

Verification

• pnpm --filter @paperclipai/server exec vitest run src/__tests__/server-startup-feedback-export.test.ts passed locally: 19/19.
• pnpm --filter @paperclipai/server exec vitest run src/__tests__/heartbeat-detached-queued-run-reconcile.test.ts was invoked locally; it skipped because embedded Postgres support is unavailable on this host.
• The original branch verification reported the embedded reconciliation test passing 3/3, recovery-stale-issue-lock-sweep.test.ts passing 22/22, and tsc --noEmit over server/ clean.
• Searched GitHub PRs for BLO-21621 detached queued run; only this PR matched.

Risks

Medium operational risk: the new sweep cancels queued runs that are stale and detached from their issue lock. The guard intentionally requires both conditions, so age alone cannot discard legitimate backlog. The main residual risk is a hidden producer that creates detached queued rows that are still meaningful despite the issue lock moving; those rows will now be terminalized after the stale-lock window and may trigger a fresh wake when the issue is otherwise unowned.

Model Used

Claude Sonnet 5 authored the original branch through Claude Code. Codex GPT-5 was used for CI triage, startup-test mock repair, PR description cleanup, and rerun/merge-queue handling.

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
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-21621

[allyblockcast]

🔗 Paperclip issue: BLO-21621

[allyblockcast]

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

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Verification
• Missing section: ## Risks
• Missing section: ## Model Used
• 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]

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

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Verification
• Missing section: ## Risks
• Missing section: ## Model Used
• 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

[kkroo]

Reviewed the detached queued-run reconciliation and the startup test repair. The sweeper is gated on stale + detached state, uses queued-status CAS before cancellation, avoids duplicate recovery wakes when another run owns the lock, and the focused startup regression passes locally.

[allyblockcast]

Ally — Consolidated PR Review

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

Important Issues (2)

• [gstack/review] server/src/services/heartbeat.ts:17555 — The bounded scan can permanently starve detached runs behind old candidates that are intentionally skipped. The query always selects the oldest 100 stale queued rows, while attached runs, terminal/missing-issue runs, and malformed/no-issue rows remain queued and retain their position. Once 100 such rows exist, every pass scans the same set and never reaches later detached rows, so the failure this sweep is meant to repair can remain permanent.
  • Filter candidates to issue-scoped, nonterminal, detached rows in SQL, or paginate with a cursor until the pass has examined/terminalized the requested number without repeatedly starting from the same skipped rows. Add coverage with more than limit permanently skipped rows preceding a repairable row.
• [native-codex] server/src/services/heartbeat.ts:17655 — Recovery can reattach another stale detached run instead of producing a clean execution when an issue has multiple orphaned queued rows. After the first row is cancelled, enqueueWakeup takes the issue lock and its legacy-run lookup selects another queued row for the same issue, stamps that old row into executionRunId, and coalesces/defers the recovery onto it. The loop then sees that second candidate as attached and deliberately leaves it queued indefinitely, while recovered may still count the returned existing run.
  • Reconcile all stale detached rows for an issue under one lock before enqueueing, or make this recovery enqueue exclude the candidate set from legacy-run adoption. Add a two-orphan test that asserts both old rows are terminal and the surviving run is newly created.

Strengths

• The queued-status compare-and-swap prevents the sweep from cancelling a row that a dispatcher has already claimed.
• The tests cover the single-orphan happy path, an attached backlog holder, and a newer live lock owner.
• Startup and periodic failures are isolated so reconciliation cannot stop the rest of heartbeat recovery.

Recommended Action

1. Address the two Important issues before merge.
2. Add the starvation and multiple-orphan regression cases described above.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot approve its own PR. Reopen this exact head 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: 1298a0e

Prior Findings Dispositioned (2)

• prior:a586c2a important 1 — still-present — server/src/services/heartbeat.ts:17587 — The current query still limits the oldest stale queued rows before validating issue scope or detachment, while skipped rows remain eligible for every subsequent pass.
• prior:a586c2a important 2 — still-present — server/src/services/heartbeat.ts:17671 — Recovery still calls enqueueWakeup after each individual cancellation, allowing its legacy-run lookup to adopt another stale detached row for the same issue.

Important Issues (2)

• [prior:a586c2a important 1 / gstack/review] server/src/services/heartbeat.ts:17587 — The bounded scan can permanently starve repairable detached runs. The query always selects the oldest 100 stale queued rows, but malformed rows, missing or terminal issue rows, and intentionally attached rows are only skipped and remain at the front of every later pass. Once 100 such rows exist, later detached rows are never examined. Filter issue-scoped detached candidates in SQL or paginate beyond skipped rows, and add a regression with more than limit permanent skips before one repairable row.
• [prior:a586c2a important 2 / native-codex] server/src/services/heartbeat.ts:17671 — Multiple stale detached rows for one unlocked issue can still defeat recovery. After cancelling the first row, enqueueWakeup can select another queued orphan through its legacy-run fallback and stamp it into executionRunId; the reconciliation loop then treats that row as attached and preserves it indefinitely. Reconcile all stale detached rows for an issue before enqueueing, or exclude the candidate set from legacy adoption, and add a two-orphan test asserting both old rows are terminal and the survivor is newly created.

Strengths

• The queued-status compare-and-swap prevents cancellation after a dispatcher has claimed a run.
• Issue lookup is company-scoped and validates the context UUID.
• Startup and periodic sweep failures are isolated from the remaining recovery pipeline.

Recommended Action

1. Address both carried-forward Important issues before merge.
2. Add the starvation and multiple-orphan regression cases described above.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head 1298a0e90e375b22daee8e51c4c711d8c5d998a1 must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Addressed both carried-forward Important findings

Pushed a follow-up commit addressing the two issues from Ally's re-review (head 1298a0e90) that were flagged as still-present after the prior test-only commit:

gstack/review (heartbeat.ts:17587) — detachment is now a JOIN condition evaluated in SQL against issues (issues.executionRunId IS DISTINCT FROM heartbeatRuns.id, same for checkoutRunId, plus a nonterminal-issue filter), not an application-side skip after an age-ordered scan. Attached/terminal/malformed rows never occupy a slot in the bounded result set at all now, so they can't permanently block a repairable row behind them regardless of limit.

native-codex (heartbeat.ts:17671) — before calling enqueueWakeup for an issue, the sweep now cancels every stale detached queued row for that issue (re-queried fresh, scoped by issueId), not just the one row the bounded scan happened to select first. This closes the window where cancelling one orphan and then waking let enqueueWakeup's legacy-run lookup adopt a sibling orphan as though it were live.

Added the two regressions the review asked for:

• a >limit permanently-attached-rows starvation case (5 attached backlog issues + 1 repairable row behind them, limit: 3) — proves the repairable row is still reached
• a two-orphan case for one issue — proves both terminalize and exactly one fresh run survives, with the issue lock pointed at the survivor

All 5 tests in heartbeat-detached-queued-run-reconcile.test.ts pass (including the pre-existing 3, with the "still attached" case's expectation updated to reflect SQL-level filtering rather than app-level skip-counting). Also re-ran recovery-stale-issue-lock-sweep.test.ts (51/51) to confirm no regression to the sibling sweep. tsc --noEmit clean.

Note: this PR had been added to the merge queue (position 32, enqueued 21:11:17Z) based on the stale pre-re-review approval. I dequeued it before pushing this fix, since merging at that point would have shipped both known, reviewer-flagged bugs. Re-queue once CI is green and review clears.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:a586c2a important 1 — fixed — server/src/services/heartbeat.ts:17617 — Candidate detachment and terminal-issue filtering now execute in SQL before the age ordering and LIMIT, so permanently attached rows no longer consume the bounded candidate prefix.
• prior:a586c2a important 2 — still-present — server/src/services/heartbeat.ts:17664 — The quiescent sibling case is fixed, but cancellation still performs only a run-status CAS without locking the issue or revalidating detachment. A concurrent enqueueWakeup can adopt a selected queued sibling under the issue lock before this update cancels it.

Critical Issues (1)

• [pr-review-toolkit / gstack/review / native-codex] server/src/services/heartbeat.ts:17572 — The predicate treats an old queued run with both issue lock pointers NULL as detached, but normal enqueue deliberately leaves executionRunId unset until claim (server/src/services/heartbeat.ts:25244). Legitimate issue backlog waiting longer than the stale threshold, including work delayed by sustained agent concurrency saturation, therefore qualifies for cancellation even though it never detached from a prior lock. The attached-backlog tests manually stamp executionRunId, so they do not exercise the normal lazy-lock queue shape.
  • Require positive durable evidence that the queued run previously owned the released lock, such as persisted lock lineage or a stale-lock-sweep marker. Do not infer detachment from null pointers plus age.

Important Issues (2)

• [prior:a586c2a important 2 / native-codex] server/src/services/heartbeat.ts:17664 — Sibling cancellation races with enqueueWakeup's legacy-run adoption. After the sibling query, a concurrent wake can lock the issue and stamp one selected queued row into executionRunId; the status-only CAS then cancels that newly attached row. The fresh issue read sees a non-null pointer and skips recovery, leaving the issue locked to a cancelled run.
  • Lock the issue row and revalidate detachment while cancelling, ideally making sibling cancellation and recovery creation part of the same issue-lock transaction used by wake adoption. Add a deterministic concurrency regression around legacy adoption.
• [gstack/review] server/src/services/heartbeat.ts:17664 — Terminalization is committed before wakeup status, lifecycle event, fresh issue read, and recovery enqueue. If any later operation throws, the catch records a failure after the old rows are already cancelled; they no longer qualify on the next pass, so no durable path guarantees that recovery will be retried.
  • Atomically persist terminalization with durable recovery intent, then emit nonessential events best-effort and dispatch that intent idempotently. Add fault-injection coverage after cancellation but before enqueueWakeup.

Strengths

• SQL-side candidate filtering resolves the bounded-prefix starvation problem.
• The new two-orphan regression covers sequential sibling adoption and verifies one replacement run.
• Startup and periodic callers isolate sweep failures from the rest of heartbeat recovery.

Recommended Action

1. Preserve legitimate lazy-lock backlog by requiring positive prior-lock evidence.
2. Serialize cancellation with issue-lock adoption and make recovery durable across partial failures.
3. Add the lazy-lock, adoption-race, and post-cancellation fault regressions above.

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.