Clear stale checkoutRunId on run finalization and add backstop sweeper (#6008)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The issue subsystem holds per-row lock columns (`checkoutRunId`, `executionRunId`, `executionAgentNameKey`, `executionLockedAt`) that gate checkout, ownership, and release > - When a heartbeat run terminates, `releaseIssueExecutionAndPromote` clears the execution-lock columns but stale checkout locks could remain attached to dead runs in edge paths > - The original fix closed the finalization, checkout, release, and sweeper paths, but PR CI exposed one more process-loss retry path where a queued retry advanced `executionRunId` while leaving `checkoutRunId` pinned to the failed run > - This pull request closes the asymmetry: terminal-run cleanup and process-loss retry recovery release dead checkout locks while preserving live execution ownership > - The benefit is permanent, automatic self-heal of stale lock columns and fewer false checkout 409s requiring board intervention > - Related upstream issue: #6007 ## Linked Issues or Issue Description Refs #6007. Duplicate/related PR search performed on 2026-06-10 with query `checkoutRunId process loss retry stale checkout lock repo:paperclipai/paperclip`. Related PRs found and reviewed for overlap: - #7727 `fix(heartbeat): atomically advance checkoutRunId on process-loss retry` - #7707 `test: cover same-agent stale checkout adoption` - #3068 `fix: clear checkoutRunId when releasing issue execution lock` ## What Changed - `server/src/services/heartbeat.ts` `releaseIssueExecutionAndPromote`: extend the per-issue update to also null `checkoutRunId` when it matches the terminating run id. WHERE clause scoped to `executionRunId = run.id OR checkoutRunId = run.id` for idempotence. - `server/src/services/heartbeat.ts` process-loss retry: when queuing the retry run, move `executionRunId` to the retry and clear the failed run's `checkoutRunId` so the dead run no longer owns checkout. - `server/src/services/issues.ts`: add `clearCheckoutRunIfTerminal` helper, symmetric to `clearExecutionRunIfTerminal`. No assignee/status precondition. Wired into `checkout`, `assertCheckoutOwner`, and `release`. Exported on the issue service. - `server/src/services/recovery/service.ts`: add `sweepStaleIssueLocks`. Scans `issues` where `checkoutRunId IS NOT NULL OR executionRunId IS NOT NULL`, joins each referenced run, and clears all lock columns on issues whose referenced runs are all terminal or missing. Emits one `issue.stale_lock_cleared` activity log row per cleared issue. - `server/src/services/heartbeat.ts`: re-export the sweeper on the heartbeat facade. - `server/src/index.ts`: invoke `sweepStaleIssueLocks` in both the startup recovery sequence and the periodic heartbeat timer chain. - Tests: route-level coverage of the new self-heal path on the next checkout attempt, service-level sweeper coverage, and heartbeat recovery assertions that terminal process-loss cleanup releases `checkoutRunId`. ## Verification ```bash pnpm --filter @paperclipai/server typecheck pnpm --filter @paperclipai/server exec vitest run \ src/__tests__/recovery-stale-issue-lock-sweep.test.ts \ src/__tests__/issue-stale-execution-lock-routes.test.ts NODE_ENV=test pnpm exec vitest run src/__tests__/heartbeat-process-recovery.test.ts -t "queues exactly one retry when the recorded local pid is dead|does not block paused-tree work when immediate continuation recovery is suppressed by the hold" NODE_ENV=test pnpm exec vitest run src/__tests__/heartbeat-process-recovery.test.ts ``` All listed local checks pass. The new and updated tests cover: - Run termination clears `checkoutRunId` when it points at the terminating run. - Process-loss retry clears the failed run's `checkoutRunId` while assigning `executionRunId` to the queued retry. - A different agent calling `POST /api/issues/:id/checkout` on an issue whose prior owner died self-heals via `clearCheckoutRunIfTerminal` and succeeds. - Sweeper clears stale lock columns for issues whose run row is terminal. - Sweeper leaves issues alone while the referenced run is still running. - Sweeper leaves issues alone when `executionRunId` is still running even if `checkoutRunId` is terminal. - Sweeper is idempotent; second pass clears nothing. Manual reproduction of the original bug shape: 1. Create an issue assigned to agent A, set `status='in_progress'`, `checkoutRunId=R1`, `executionRunId=null`, where `heartbeat_runs.status = 'failed'` for `R1`. 2. Reassign to agent B and move to `status='todo'`. 3. Before this PR: agent B `POST /checkout` returns `409 Issue checkout conflict` indefinitely. After this PR: succeeds, lock columns rewritten to agent B's current run id. ## Risks - Low. All clears are scoped by run id, so they only fire when the lock column unambiguously points at the terminating or terminal run. No schema change. No migration. No API surface change. - Behavioral shift: an issue that previously stayed `in_progress` with a dead `checkoutRunId` after run termination now self-heals. Downstream code that reads stale `checkoutRunId` as a proxy for recent run history should already be reading `executionRunId` or the `heartbeat_runs` table. - Sweeper cost: one indexed scan per recovery tick over rows where `checkoutRunId IS NOT NULL OR executionRunId IS NOT NULL` plus a single batched `heartbeatRuns` lookup per candidate. Negligible at expected cardinality; further bounded by the existing recovery cadence. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. This is a bug fix, not a feature. No roadmap overlap. ## Model Used - Claude (Anthropic), model ID `claude-opus-4-7`, extended-thinking off, tool use enabled. - OpenAI Codex, GPT-5-based coding agent, tool use enabled, used for the follow-up process-loss retry fix and PR body update. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] If this change affects the UI, I have included before/after screenshots - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] 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 --------- Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: Dotta <bippadotta@protonmail.com>
This commit is contained in:
committed by
GitHub
parent
c297ba2a80
commit
f3db7b88ea
@@ -1,7 +1,7 @@
|
||||
# Execution Semantics
|
||||
|
||||
Status: Current implementation guide
|
||||
Date: 2026-06-08
|
||||
Date: 2026-06-10
|
||||
Audience: Product and engineering
|
||||
|
||||
This document explains how Paperclip interprets issue assignment, issue status, execution runs, wakeups, parent/sub-issue structure, and blocker relationships.
|
||||
@@ -121,6 +121,17 @@ These are related but not identical:
|
||||
|
||||
Paperclip already clears stale execution locks and can adopt some stale checkout locks when the original run is gone.
|
||||
|
||||
The active-lock lifecycle is part of the checkout contract:
|
||||
|
||||
- a run owns `checkoutRunId` only while that run is non-terminal
|
||||
- when a run reaches `succeeded`, `failed`, `cancelled`, or `timed_out`, finalization must compare-and-clear lock columns that still point at that run
|
||||
- finalization must not clear a lock already reacquired by a successor run
|
||||
- process-loss retry handoff must not leave `checkoutRunId` pinned to the failed run when `executionRunId` moves to the retry run
|
||||
- checkout and checkout-owner checks may self-heal lock columns that point at terminal or missing runs before evaluating conflicts
|
||||
- the recovery sweeper may clear rows whose checkout and execution locks all point at terminal or missing runs
|
||||
|
||||
Stale-lock recovery is crash recovery, not a retry loop. Paperclip must not clear or adopt locks held by non-terminal runs. After stale cleanup, a checkout `409` should mean a real live owner, status/assignee mismatch, unresolved blocker, or active gate still prevents checkout. Agents must treat that `409` as an ownership conflict and stop rather than retrying the same checkout.
|
||||
|
||||
## 6. Parent/Sub-Issue vs Blockers
|
||||
|
||||
Paperclip uses two different relationships for different jobs.
|
||||
|
||||
Reference in New Issue
Block a user