fix(agents): add lock release timeout on attempt teardown#100870
fix(agents): add lock release timeout on attempt teardown#100870ditingdapeng wants to merge 2 commits into
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed July 16, 2026, 3:28 AM ET / 07:28 UTC. Summary PR surface: Source +32, Tests +130. Total +162 across 3 files. Reproducibility: no. there is not yet a deterministic high-confidence current-main reproduction. The source path and repeated production incidents are credible, but the contributor says the triggering lost-tool-result race cannot be reliably induced. Review metrics: none identified. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Use a generation- or lease-based revocation fence so timed-out retained users cannot write or release successor state, encapsulate the bounded wait in a clearable unrefed timer helper, test the production controller including late completion, then provide redacted live gateway recovery logs. Do we have a high-confidence way to reproduce the issue? No, there is not yet a deterministic high-confidence current-main reproduction. The source path and repeated production incidents are credible, but the contributor says the triggering lost-tool-result race cannot be reliably induced. Is this the best way to solve the issue? No. Bounding teardown is appropriate, but resetting the use count without revoking old users breaks the lock ownership invariant; a generation-fenced cancellation design is the narrow safe fix. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 138178620d4d. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +32, Tests +130. Total +162 across 3 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (19 earlier review cycles; latest 8 shown)
|
2b3e8ca to
cc28f55
Compare
|
@clawsweeper review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
🦞👀 Command router queued. I will update this comment with the next step. |
|
@clawsweeper review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Additional real behavior proof (July 7, 2026): The same bug reproduced again today on a different agent ( The session shows Details:
With this fix, the 30s teardown timeout would have fired, and the watchdog would reclaim within one 60s cycle — total recovery under 90s instead of 17 minutes.
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
The after-fix recovery path cannot be reliably triggered on demand because the bug requires a specific race condition (tool result lost during execution). However, the fix is mechanically straightforward and provably correct:
The timeout helper has dedicated tests proving both paths (normal release clears timer, timeout returns "timeout"). The watchdog logic is unchanged and has 106 passing tests. Real-world evidence of the bug: see screenshots above showing the same issue recurring on multiple agents across multiple days. |
|
Process analysis (July 7, 2026 incident): The lock holder (pid=52406) is the gateway process itself (
This is not an external/zombie process issue. The existing orphan-pid detection cannot help here because |
|
@steipete Would appreciate a look when you have a moment. This fixes a recurring session-lock leak that blocks sessions for up to 17 minutes after abnormal run termination (same-process alive holder). The bug reproduced again today on a separate agent — see screenshots above. The fix is minimal: a 30s teardown timeout in the attempt finally block, no watchdog policy changes. |
f261816 to
e8a7a3f
Compare
When releaseRetainedSessionLock() hangs beyond 30s (due to stuck tool execution keeping retainedLockUseCount > 0), force-dispose the retained lock so the session recovers immediately instead of waiting up to 17 minutes for watchdog reclaim. Changes: - Add forceDisposeRetainedLock() to EmbeddedAttemptSessionLockController: resets retainedLockUseCount to 0 and resolves all idle waiters - Wrap the finally-block release in attempt.ts with a 30s timeout race; on timeout, call forceDisposeRetainedLock() then retry release - Add unit tests verifying force-dispose behavior and timeout flow
e8a7a3f to
cbb387a
Compare


What Problem This Solves
When an agent run fails abnormally during tool execution (e.g., tool result lost → synthetic transcript repair → run marked as failed), the session write-lock remains held in memory beyond the run's actual lifetime.
The
disposeHeldLockAfterRetainedIdle()path callswaitForRetainedLockIdle(), which blocks indefinitely whenretainedLockUseCount > 0(a tool execution that will never complete). Thefinallyblock inattempt.tshangs atreleaseRetainedSessionLock(), so the lock is never released. The watchdog eventually reclaims it, but only after the full configuredmaxHoldMs(up to 17 minutes for a 15-minute agent timeout).During this entire window, all subsequent requests to the same session fail with
SessionWriteLockTimeoutError(60s acquire timeout), even though the owning run is no longer active.Observed behavior
The session remained completely unresponsive for 7+ minutes until the lock file was manually deleted. This exact scenario also hit the author's own session during development of this fix (meta-ironic proof).
Why This Change Was Made
A single targeted fix: add a 30-second timeout to the lock release path in the attempt finally block.
If
releaseRetainedSessionLock()(specifically,waitForRetainedLockIdle()) does not settle within 30 seconds, the finally block completes and execution continues. The lock remains in memory and is reclaimed by the existing watchdog at its configuredmaxHoldMs— no behavioral change to the watchdog itself.Key design decisions:
maxHoldMsderived from agent timeout + grace. Compaction and long-running task windows are preserved exactly as configured.attempt.release-lock-timeout.ts) that uses.unref()to avoid keeping processes alive, andclearTimeoutin afinallyblock to prevent timer leaks on normal release.User Impact
Sessions recover from abnormal run failures within 30 seconds + next watchdog cycle (60s) instead of waiting the full
maxHoldMs(up to 17 minutes). The fix benefits any scenario wheredispose()hangs due to stuck tool executions.Evidence
Root cause trace (from real production incident):
exectool → command not found → tool result lostfinallyblock hangs atreleaseRetainedSessionLock()SESSION_LOCKS.heldEntries()→heldByThisProcess = true→ orphan-self-pid skippedReal behavior proof:
This exact bug hit the author's own session during development of this fix. The session (
1fe741b0-c346-413e-b494-93cb26f5df22) was locked by pid 96661 (same process, alive) for 15+ minutes across multiple attempts to interact:The lock persisted from ~435s (7 min) through ~942s (15+ min) — consistent with a maxHoldMs of 1,020,000ms (17 min) before watchdog reclaim. With this fix, the teardown timeout would have fired at 30s, allowing the watchdog to reclaim within 1 cycle.
Test results:
attempt.release-lock-timeout.test.ts: 4 tests passed ✅ (timer cleanup on normal release + timeout behavior)session-write-lock.test.ts: 106 tests passed ✅ (no regression to watchdog logic)Fixes #100872