fix: allow agents to clear orphaned execution locks - #8792
Conversation
Greptile SummaryThis PR adds TTL-aware dead-owner detection to the issue execution lock system and wires it into checkout acquisition, stale-lock adoption, the recovery sweeper, and a new guarded force-release route for the assigned agent. The approach is well-structured: a single
Confidence Score: 4/5Safe to merge with the two minor comments addressed; the locking and TTL logic is correct and well-tested. The sweeper's inline TTL check diverges from
Important Files Changed
Reviews (6): Last reviewed commit: "Merge master into orphaned execution loc..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| monitorNextCheckAt: sql<Date | null>`case | ||
| when ${issues.monitorNextCheckAt} is null | ||
| and ${issues.monitorLastTriggeredAt} is null | ||
| and ${issues.monitorAttemptCount} = 0 | ||
| then ${nextCheckAt.toISOString()}::timestamptz | ||
| else ${issues.monitorNextCheckAt} | ||
| end`, | ||
| monitorWakeRequestedAt: null, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The CASE condition only sets monitorNextCheckAt when ALL three guards are true: monitorNextCheckAt IS NULL AND monitorLastTriggeredAt IS NULL AND monitorAttemptCount = 0. Since release() never clears the monitor fields (monitorLastTriggeredAt, monitorAttemptCount), any issue that has been through even one monitoring cycle will fall into the ELSE branch — preserving the current monitorNextCheckAt, which may be NULL after the previous monitor run completed and cleared it. The result is that lock acquisition on any previously-monitored issue silently skips scheduling the TTL reaper, leaving orphaned locks on that issue class with no monitor-driven cleanup trigger.
Concrete case: issue is assigned → monitor fires → lock is released → issue is reassigned and the worker dies → monitorLastTriggeredAt is non-null from cycle 1 → CASE falls to ELSE → monitorNextCheckAt remains NULL → monitor is never scheduled for the new orphaned lock.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/services/issues.ts
Line: 431-440
Comment:
**Monitor check not re-scheduled on subsequent lock acquisitions**
The CASE condition only sets `monitorNextCheckAt` when ALL three guards are true: `monitorNextCheckAt IS NULL AND monitorLastTriggeredAt IS NULL AND monitorAttemptCount = 0`. Since `release()` never clears the monitor fields (`monitorLastTriggeredAt`, `monitorAttemptCount`), any issue that has been through even one monitoring cycle will fall into the `ELSE` branch — preserving the current `monitorNextCheckAt`, which may be `NULL` after the previous monitor run completed and cleared it. The result is that lock acquisition on any previously-monitored issue silently skips scheduling the TTL reaper, leaving orphaned locks on that issue class with no monitor-driven cleanup trigger.
Concrete case: issue is assigned → monitor fires → lock is released → issue is reassigned and the worker dies → `monitorLastTriggeredAt` is non-null from cycle 1 → CASE falls to ELSE → `monitorNextCheckAt` remains `NULL` → monitor is never scheduled for the new orphaned lock.
How can I resolve this? If you propose a fix, please make it concise.
c2485ee to
b338e90
Compare
| if (options.force && (existing.checkoutRunId || existing.executionRunId)) { | ||
| const [checkoutDead, executionDead] = await Promise.all([ | ||
| existing.checkoutRunId ? isTerminalOrMissingHeartbeatRun(existing.checkoutRunId, tx) : Promise.resolve(true), | ||
| existing.executionRunId ? isTerminalOrMissingHeartbeatRun(existing.executionRunId, tx) : Promise.resolve(true), | ||
| ]); | ||
| if (!checkoutDead || !executionDead) { | ||
| throw conflict("Issue run ownership conflict", { | ||
| issueId: existing.id, | ||
| status: existing.status, | ||
| assigneeAgentId: existing.assigneeAgentId, | ||
| checkoutRunId: existing.checkoutRunId, | ||
| executionRunId: existing.executionRunId, | ||
| actorAgentId: actorAgentId ?? null, | ||
| actorRunId: actorRunId ?? null, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The force-release path reads checkoutRunId/executionRunId liveness via isTerminalOrMissingHeartbeatRun without first acquiring a SELECT … FOR UPDATE lock on those heartbeat run rows. Every other liveness-gated path in this file (clearExecutionRunIfTerminal, clearCheckoutRunIfTerminal, adoptStaleCheckoutRun) locks the run row before reading it. Without that lock, a concurrent heartbeat posting a fresh updatedAt between the TTL check and the issue UPDATE can make a "running but stale-looking" owner appear dead: the force-release proceeds and clears its lock even though the run is now demonstrably live. Terminal status is monotonic so there is no race there, but TTL-staleness is ephemeral and makes the window meaningful. Add await tx.execute(sql\select … for update`)for each run ID before callingisTerminalOrMissingHeartbeatRun, mirroring the pattern used in clearCheckoutRunIfTerminal` (lines 4349–4361).
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/services/issues.ts
Line: 6218-6233
Comment:
**Force-release TOCTOU: heartbeat run rows not locked before TTL read**
The force-release path reads `checkoutRunId`/`executionRunId` liveness via `isTerminalOrMissingHeartbeatRun` without first acquiring a `SELECT … FOR UPDATE` lock on those heartbeat run rows. Every other liveness-gated path in this file (`clearExecutionRunIfTerminal`, `clearCheckoutRunIfTerminal`, `adoptStaleCheckoutRun`) locks the run row before reading it. Without that lock, a concurrent heartbeat posting a fresh `updatedAt` between the TTL check and the issue UPDATE can make a "running but stale-looking" owner appear dead: the force-release proceeds and clears its lock even though the run is now demonstrably live. Terminal status is monotonic so there is no race there, but TTL-staleness is ephemeral and makes the window meaningful. Add `await tx.execute(sql\`select … for update\`)` for each run ID before calling `isTerminalOrMissingHeartbeatRun`, mirroring the pattern used in `clearCheckoutRunIfTerminal` (lines 4349–4361).
How can I resolve this? If you propose a fix, please make it concise.
b338e90 to
50f9477
Compare
![superagent-security[bot]](https://avatars.githubusercontent.com/in/3287076?s=60&v=4){kind=small}
| @@ -6182,6 +6215,23 @@ export function issueService(db: Db) { | |||
| }); | |||
There was a problem hiding this comment.
Force-release checks run liveness without SELECT … FOR UPDATE, unlike all other liveness-gated paths in the file.
Acquire row-level locks on heartbeat run rows before the TTL liveness check, mirroring clearCheckoutRunIfTerminal.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="server/src/services/issues.ts">
<violation number="1" location="server/src/services/issues.ts:6215">
<priority>P2</priority>
<title>Missing FOR UPDATE lock on heartbeat runs in force-release path</title>
<evidence>The new options.force block in release() calls isTerminalOrMissingHeartbeatRun for checkoutRunId and executionRunId without first locking the referenced heartbeat run rows via SELECT ... FOR UPDATE. Every other liveness-gated path in the same file acquires such a lock before reading run status. Because updatedAt-based TTL staleness is ephemeral, a concurrent heartbeat update between the liveness check and the issue UPDATE can make an actively-running owner appear dead, allowing the force-release to clear its lock.</evidence>
<recommendation>Add a SELECT ... FOR UPDATE lock on each referenced heartbeat run row before calling isTerminalOrMissingHeartbeatRun, matching the pattern used in clearCheckoutRunIfTerminal.</recommendation>
</violation>
</file>
superagent-security
Bot
added
the
pr:flagged
Co-Authored-By: Paperclip <noreply@paperclip.ing>
50f9477 to
35dfd06
Compare
| const run = runById.get(runId); | ||
| if (!run) return true; // missing run row → no real claim | ||
| if (TERMINAL_HEARTBEAT_RUN_STATUSES.has(run.status)) return true; | ||
| return run.updatedAt.getTime() <= Date.now() - ISSUE_EXECUTION_LOCK_TTL_MS; |
There was a problem hiding this comment.
Sweeper uses stale heartbeat snapshot for TTL check, creating a race window where live runs can be incorrectly cleared.
Re-verify heartbeat updatedAt inside the update transaction or add a freshness guard to the WHERE clause.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="server/src/services/recovery/service.ts">
<violation number="1" location="server/src/services/recovery/service.ts:4421">
<priority>P2</priority>
<title>TOCTOU race in recovery sweeper TTL-stale lock detection</title>
<evidence>The recovery sweeper reads heartbeat run updatedAt values in a bulk query outside the per-issue update loop. It then uses this stale snapshot to decide if a run is TTL-stale via `run.updatedAt.getTime() <= Date.now() - ISSUE_EXECUTION_LOCK_TTL_MS`. A run that posts a fresh heartbeat after the bulk read but before its issue row is updated will incorrectly pass the TTL check and have its lock swept, even though it is now demonstrably live. The UPDATE's WHERE clause only guards on issue-side columns (checkoutRunId, executionRunId) and does not verify the heartbeat run is still TTL-stale.</evidence>
<recommendation>Re-verify the heartbeat run's updatedAt inside the per-issue update transaction or add an updatedAt freshness guard to the UPDATE WHERE clause.</recommendation>
</violation>
</file>
Pass `-q -` to hermes so the CLI reads the query from stdin instead of receiving it as a command-line argument. The previous invocation hermes chat -q "<full_prompt>" exposed the entire rendered prompt — task body, wake context, issue comments, agent instructions, and API guidance — to any local process that can read /proc/<pid>/cmdline or run `ps aux` on the same host. The fix passes the sentinel `-` and delivers the actual prompt via the `stdin` option of runChildProcess, matching the pattern already used by the claude-local adapter. Adds execute.argv-security.test.ts with 5 focused regression tests. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…ion-lock-release # Conflicts: # server/src/services/heartbeat.ts # server/src/services/issues.ts # server/src/services/recovery/service.ts
This reverts commit 81dd616.
2 participants
Thinking Path
Linked Issues or Issue Description
Refs #2533
Refs #6007
Refs #6399
Refs #7314
Refs #8502
Bug fix: issue execution locks can remain stuck after an agent run dies without writing a terminal heartbeat status.
What happened:
Expected behavior:
What Changed
Verification
QA review requested:
Risks
Model Used
OpenAI Codex GPT-5.5 through the Paperclip local agent harness, with repository file editing, shell command execution, and local verification tools enabled.
Checklist