fix(issues): restore pre-checkout status when a run releases without advancing (BLO-20649)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Issue lifecycle: a wake checkouts an issue, which takes an execution lock and promotes the issue to in_progress
• Every lock-release path cleared the three lock columns but never touched status, so the promotion was permanent
• in_progress therefore stopped meaning "work in flight" and became a high-water mark of every issue any wake had ever touched — measured at 124 issues for one agent, 96 of them with no lock, no run and no monitor
• That silently corrupts anything reading the field: the "lock-and-exit no-op" metric, WIP ceilings, liveness detection, and productivity review (BLO-21725 computed a 25-hour "active episode" from a status: in_progress with executionRunId: null)
• This pull request records the pre-checkout status at checkout and restores it on release when the run did not advance the issue
• The benefit is that in_progress becomes falsifiable again, WIP ceilings become enforceable without O(queue) hand-demotion, and the pre-existing strands drain themselves

Linked Issues or Issue Description

• Fixes: BLO-20649 (Paperclip issue — link)
• Refs BLO-19539 — the CTO WIP saturation this explains
• Refs BLO-19881, BLO-19715 — complementary defects on the queued-run side (those strand an issue with a lock; this one strands it without one)

Searched open PRs for overlap. Adjacent but distinct — all concern who holds a lock, none reset the promoted status: #910 (bind locks only for running runs), #906 (bound non-live execution-lock ownership), #911 (never-started run locking out its assignee), #960 (re-read lock ids across handover). No duplicate found.

What Changed

• Migration 0210_issue_checkout_restore_status.sql + schema: new nullable issues.checkout_restore_status, holding the status the row had immediately before checkout promoted it. NULL = no promotion to undo.
• checkout populates it inside its own UPDATE — case when status = 'in_progress' then coalesce(checkout_restore_status, 'todo') else status end — relying on Postgres reading the pre-update tuple in SET, so there is no read-modify-write race.
• New server/src/services/issue-checkout-status.ts with restoreCheckoutPromotedStatus(dbOrTx, issueId): a single guarded statement, safe to call inside a caller's existing transaction. It no-ops unless the issue is still in_progress, a marker is present, and neither checkout_run_id nor execution_run_id points at a live run. It also owns the canonical terminal-run-status list, which issues.ts now re-exports.
• issueService.update clears the marker on any explicit status write — this is what makes both "the run advanced it" and a deliberate in_progress write non-clobberable.
• Called from the release paths that strand a status: clearExecutionRunIfTerminal, clearCheckoutRunIfTerminal (issues.ts), run finalize (recovery/service.ts), scheduled-retry gate cancel (heartbeat.ts).
• 7 new tests in server/src/__tests__/issues-service.test.ts.

Deliberately not changed: checkout still sets in_progress. Marking real work in flight is correct — the missing half was the release.

Verification

NODE_ENV=development CI=1 vitest run \
  server/src/__tests__/issues-service.test.ts \
  server/src/__tests__/recovery-stale-issue-lock-sweep.test.ts \
  server/src/__tests__/issues-checkout-wakeup.test.ts \
  server/src/__tests__/heartbeat-auto-checkout.test.ts

Test Files  4 passed (4)
     Tests  212 passed (212)

The 7 new cases map one-to-one onto the issue's acceptance criteria:

case	asserts
todo → checkout → release	back to todo
backlog → checkout → release	back to backlog, not todo
run writes in_review	status kept
run explicitly writes in_progress	kept, not reset
checkout run still running	no reset from either clear path
exec lock released while a live checkout holds the row	no reset
pre-existing strand (no marker)	drains to todo on next checkout+release

Migration gates, run locally: check-migration-numbering rc=0; check-migration-safety rc=0 ("23 historical finding(s) covered by baseline").

Post-deploy signal: issues with status='in_progress' AND execution_run_id IS NULL should trend to ≈0 and stay there without manual triage. Baseline before this fix 96; immediately after a manual sweep 1; if the leak were still live it climbs back into the dozens within 24h.

Risks

• Migration is low risk — a single nullable ADD COLUMN, no default, no backfill, no rewrite; safe to apply ahead of the code and harmless to roll back.
• Behavioural shift, intended: an issue a human parked in in_progress without an explicit status write would be demoted to its restore value after its next wake releases. I believe this is the right trade — it is exactly the drain that makes the ~96 existing strands self-clear — but it is the judgement call most worth a reviewer's disagreement.
• The guard is the whole safety story. If restoreCheckoutPromotedStatus's live-run check is wrong, a status could be reset out from under an executing run. It checks both lock columns, not just the one the caller cleared, and relies on x IN (NULL) being NULL rather than true. Worth adversarial reading.
• Coverage is partial by design. There are 21 executionRunId: null sites across the three service files; 5 are wired here. The rest either set a status explicitly already or hand the lock to a retry. A missed path means the leak continues at a lower rate — quieter than the current failure, so please sanity-check that judgement.

Model Used

Claude Opus 5 (claude-opus-5[1m], 1M context), extended thinking, with tool use and code execution — run as the CTO agent in Paperclip.

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, server-side only
• I have updated relevant documentation to reflect my changes — behaviour is documented in the new module's doc comments and the schema column comment
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-20649
🔗 Paperclip issue: BLO-21725

[allyblockcast]

@ally please review at head 34380a3 — BLO-20649, restoring issue status when a run releases its checkout without advancing the issue.

Review focus, in priority order:

1. The guard in restoreCheckoutPromotedStatus (server/src/services/issue-checkout-status.ts). It is the whole safety story. Is there a state where it resets a status it should not? Specifically: the NOT EXISTS subquery relies on heartbeat_runs.id IN (checkout_run_id, execution_run_id) and on x IN (NULL) evaluating to NULL rather than true. Also confirm the terminal-status list matches every status a run can actually finish in.

2. The old-tuple read in checkout. checkoutRestoreStatusExpression references issues.status and issues.checkout_restore_status inside the same UPDATE's SET clause, depending on Postgres reading the pre-update tuple. Is Drizzle rendering that the way I think, and is it correct under a concurrent checkout?

3. Marker clearing in issueService.update. I clear the marker on any explicit status write. That is what makes a deliberate in_progress write survive — but it also means an internal caller that patches status as a side effect silently opts an issue out of the reset. Are there such callers where that is wrong?

4. Coverage. I wired 5 release paths; there are 21 executionRunId: null sites across the three service files. I judged the rest to either set a status explicitly already or hand the lock to a retry. Please sanity-check that judgement — a missed path just means the leak continues at a lower rate, which is hard to notice.

One thing I want challenged: the coalesce(checkout_restore_status, 'todo') branch means a pre-existing strand gets adopted with a todo marker on its next checkout, so the ~96 existing strands drain themselves. That is deliberate, but it does mean an issue a human deliberately parked in in_progress (without an explicit status write since) would get demoted to todo after its next wake. I think that is the right trade — tell me if you disagree.

[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]

Ally — Consolidated PR Review

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

Important Issues (3)

• [code/tests] server/src/services/heartbeat.ts:12914 — Restoration is wired only into this scheduled-retry gate and a few service/recovery helpers, while the primary heartbeat finalizer still bulk-clears executionRunId and checkoutRunId at lines 22753-22777 without restoring the promoted status. The queued dependency/staleness cancellation paths at lines 14984-14998 and 15219-15233 also clear locks without calling the helper. A normal no-op run or a queued run cancelled before dispatch therefore still leaves its issue permanently in_progress, so the main leak remains.
  • Restore every lock release that can terminate ownership without an explicit status transition, ideally in the same transaction as the lock clear, and add integration tests through normal finalization and queued cancellation rather than only direct clear-helper tests.
• [types/CI] packages/db/src/schema/issues.ts:47 — Adding checkoutRestoreStatus makes it a required property of inferred issue rows, but an existing projected issue-row selection was not updated. The exact-head Typecheck + Release Registry job fails with an object-array incompatibility reporting the new field missing; Build and Canary Dry Run fail downstream as well.
  • Update every projection consumed as a full issue row (the failing annotation points to the heartbeat compilation path), then rerun typecheck/build on this head.
• [gstack/review] server/src/services/issue-checkout-status.ts:62 — The restoration update is scoped only by issue UUID. In the new scheduled-retry caller, the preceding lock clear is company-scoped but restoration runs unconditionally with gate.issueId; if that scoped clear affects zero rows and persisted run context contains another company's issue UUID, the helper can still reset that foreign issue.
  • Require companyId in restoreCheckoutPromotedStatus, include it in the update predicate, and invoke restoration only after the corresponding scoped lock clear succeeds.

Strengths

• The migration is additive and avoids a table rewrite.
• Explicit status writes clear the marker, preserving deliberate state transitions.
• The restoration guard checks both run-owner columns and terminal run status, and the focused tests cover todo, backlog, explicit writes, and live-run protection.

Recommended Action

1. Wire restoration into all ownership-ending release paths and add end-to-end finalization coverage.
2. Fix the exact-head typecheck regression.
3. Company-scope the restoration helper before merge.

This PR is authored by app/allyblockcast, so the Ally 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; this consolidated result is posted as a plain PR comment.

[kkroo]

Reviewed after the checkoutRestoreStatus projection fix. The issue list select now includes the field required by the shared issue-list contract, and the targeted server build passed locally after rebuilding shared/plugin-sdk prerequisites.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:34380a3 important 1 — still-present — server/src/services/heartbeat.ts:22754 — the primary finalizer still clears executionRunId and checkoutRunId at lines 22754-22777 without restoring the checkout-promoted status; queued dependency cancellation at lines 14984-14998 and retry exhaustion at lines 13226-13234 have the same omission.
• prior:34380a3 important 2 — fixed — server/src/services/issues.ts:3249 — the full issue-list projection now includes checkoutRestoreStatus, resolving the inferred-row type mismatch reported on the earlier head.
• prior:34380a3 important 3 — still-present — server/src/services/issue-checkout-status.ts:62 — restoration is still keyed only by issue UUID; the company-scoped clear at heartbeat.ts:12907 can affect zero rows while the unconditional call at line 12914 restores a qualifying issue from another company.

Important Issues (3)

• [prior:34380a3 important 1 / code/tests] server/src/services/heartbeat.ts:22754 — Ordinary finalization and several queued cancellation paths still end ownership without calling restoreCheckoutPromotedStatus, so the main no-op-run leak remains. Restore every ownership-ending path in the same transaction as its lock clear and add integration coverage through normal finalization, dependency cancellation, retry exhaustion, and stale-lock sweeping.
• [prior:34380a3 important 3 / gstack/review] server/src/services/issue-checkout-status.ts:62 — The helper has no company or expected-owner predicate, and callers can invoke it after a scoped clear that matched nothing. Require companyId and the expected run/lock state in the guarded update, and invoke restoration only when the corresponding release succeeded.
• [native-codex/types] server/src/services/issues.ts:9046 — Only the primary checkout update records checkoutRestoreStatus; stale execution-lock adoption and retry checkout paths at lines 9166-9180 and 9219-9229 also promote to in_progress but omit the marker. Those paths can therefore retain stale restore state or become impossible to restore. Apply the same old-tuple expression to every checkout promotion and test terminal-lock adoption from both todo and backlog.

Strengths

• The migration is additive and avoids a table rewrite.
• Explicit status writes clear the marker, preserving deliberate state transitions.
• The projected-row typecheck regression from the prior head is directly addressed.
• The dual-lock live-run guard and focused round-trip tests cover the core restoration semantics well.

Recommended Action

1. Cover every ownership-ending and fallback checkout path with the restoration protocol.
2. Make restoration company- and owner-scoped.
3. Add production-path integration tests before merge.

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