fix: force-release session lock on dispose to prevent orphan locks#90419
fix: force-release session lock on dispose to prevent orphan locks#90419longkunbetter wants to merge 1 commit into
Conversation
openclaw-barnacle
Bot
added
agents
|
Codex review: needs real behavior proof before merge. Reviewed June 21, 2026, 10:48 AM ET / 14:48 UTC. Summary PR surface: Source +17. Total +17 across 1 file. Reproducibility: yes. at source level: current main and v2026.6.9 return early when waitForRetainedLockIdle() is false in the release/dispose paths, matching the reported active-write lock leak. I did not run the timing-sensitive GUI rapid-switching reproduction in this read-only review. Review metrics: 1 noteworthy metric.
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:
Next step before merge
Security Review findings
Review detailsBest possible solution: Defer the same fence-preserving graceful release work until the retained write drains, add focused active-write abort/dispose regression coverage, and include redacted after-fix proof from the rapid session-switching flow or an equivalent real-component harness. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main and v2026.6.9 return early when waitForRetainedLockIdle() is false in the release/dispose paths, matching the reported active-write lock leak. I did not run the timing-sensitive GUI rapid-switching reproduction in this read-only review. Is this the best way to solve the issue? No, not as-is. Deferred release is the right shape for preserving write serialization, but this implementation needs to preserve the existing fence handoff rather than only calling release after idle. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 0ad48dad2c47. Label changesLabel justifications:
Evidence reviewedPR surface: Source +17. Total +17 across 1 file. View PR surface stats
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
|
|
Nice catch on the teardown leak. One thing I wanted to sanity-check: in the |
clawsweeper
Bot
added
rating: 🧂 unranked krab
When disposeHeldLockAfterRetainedIdle or releaseHeldLockWithFence are called from within the active retained-write context (waitForRetainedLockIdle returns false), register a deferred release callback on retainedLockIdleWaiters instead of silently returning. This preserves write serialization (no concurrent writes from force-release) while ensuring the lock is eventually released when the retained-use counter drains to zero. Previously the early return leaked the lock in memory and on disk, causing subsequent sub-agents to time out with "session file locked" errors when the web GUI rapidly switches sessions.
4a3de67 to
881a33b
Compare
Reproduction AnalysisThanks for the thorough review. I've updated the fix to use deferred release via Trigger scenarioThe orphan-lock bug is triggered by rapid session switching in the web GUI:
EvidenceThe actual error from the reporter's environment:
Updated fixBoth if (!(await waitForRetainedLockIdle())) {
const pending = heldLock;
retainedLockIdleWaiters.add(() => {
if (heldLock === pending) heldLock = undefined;
pending?.release().catch(() => {});
});
return;
}This preserves the write serialization invariant (lock released AFTER write |
clawsweeper
Bot
added
P2
clawsweeper
Bot
removed
rating: 🧂 unranked krab
openclaw-barnacle
Bot
added
the
triage: needs-pr-context
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Thanks for this contribution. This is superseded by the narrower retained-lock cleanup work that has now landed in #96100 (merge commit 0a042f6): The remaining dispose/stuck-retained-user case is still a distinct follow-up and is tracked by #95833 / #95854, which should be reviewed against the post-#96100 code. Closing this PR to avoid landing an older overlapping implementation. |
3 participants
Problem
When an embedded attempt (sub-agent) is torn down via
dispose(), thedisposeHeldLockAfterRetainedIdle()function silently returns withoutreleasing the session write lock if a write operation is still in progress
(
retainedLockUseCount > 0withactiveWriteLockstore active).The same silent-return issue exists in
releaseHeldLockWithFence(), usedby
releaseHeldLockForAbort().This leaves orphaned
.jsonl.lockfiles on disk that block subsequentsub-agents from writing to the same session file. Because the lock's PID
matches the still-alive gateway process and the in-memory
state.heldmap still tracks the entry, the contention handler does not reclaim it
(
heldByThisProcess=true→shouldTreatAsOrphanSelfLockreturns false).The lock is neither reclaimed nor released until the watchdog force-releases
it after
maxHoldMs(default 5 min), but the watchdog only checks in-memoryentries and the file lock can persist indefinitely in some scenarios.
Result: new sub-agents get a 60-second timeout with "session file locked"
errors.
Fix
In both
disposeHeldLockAfterRetainedIdle()andreleaseHeldLockWithFence(), whenwaitForRetainedLockIdle()returnsfalse, force-release the lock (heldLock = undefined; lock.release())instead of silently returning. Teardown and abort paths must never leak a
lock.
Files changed
src/agents/embedded-agent-runner/run/attempt.session-lock.ts— twofunctions modified, 16 lines added.