fix(session): add fallback lock file cleanup on session write-lock release by immortal-autumn · Pull Request #91332 · openclaw/openclaw

immortal-autumn · 2026-06-08T05:16:46Z

Problem

When sidecar-lock's removeLockIfUnchanged fails because the lock file snapshot doesn't match (content/inode changed between acquisition and release), the session write-lock file persists on disk permanently. This blocks all subsequent messages to that session with SessionWriteLockTimeoutError until the gateway is restarted.

Observed in production (OpenClaw 2026.6.1):

04:35:55 chat.abort ✓ 87ms
04:35:55 embedded abort settle timed out (timeoutMs=2000)
04:35:55 lock file created: pid=<gateway> maxHoldMs=1020000
04:37:14 SessionWriteLockTimeoutError ageMs=79210  (×2 lanes)
04:38:25 SessionWriteLockTimeoutError ageMs=149601 (×2 lanes)

See #91327 for full root cause analysis.

Fix

Add a fallback cleanup in acquireSessionWriteLock in session-write-lock.ts: after the inner sidecar-lock release() completes, check if the .lock file still exists on disk and no reentrant holder remains. If both conditions hold and the file's pid field matches the current process, force-remove it.

const innerRelease = lock.release;
const release = async () => {
    await innerRelease();
    // Fallback cleanup: if sidecar-lock's removeLockIfUnchanged failed
    // due to snapshot mismatch, force-remove the lock file if no
    // reentrant holder remains and the file still matches our pid.
    try {
        if (!sessionLockHeldByThisProcess(normalizedSessionFile)) {
            const payload = await readLockPayload(lockPath);
            if (payload?.pid === process.pid) {
                await fs.rm(lockPath, { force: true });
            }
        }
    } catch {
        // Best-effort fallback on filesystem errors.
    }
};
return { release };

Why this is safe

Reentrancy guard: sessionLockHeldByThisProcess() prevents removing the lock file while other reentrant holders still expect it to exist.
pid check: Only force-removes if the lock file's pid matches process.pid. If another process replaced the lock file, it won't match → we don't touch it.
Best-effort: Falls back silently on filesystem errors.
Non-breaking: The inner release() still runs as before. This is purely additive cleanup.

Files changed

src/agents/session-write-lock.ts: +18 lines, 1 changed

Real behavior proof

Setup:

OS: Oracle Linux 9.7 (aarch64)
Node.js: v22.22.3
Fork rebased onto openclaw/main tip — previously was 4,466 commits behind, now cleanly atop current upstream

Verified before push:

Rebase integrity: clean rebase, zero conflicts
Diff confirms only intended change:

$ git diff upstream/main --stat
 src/agents/session-write-lock.ts | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

Reentrancy fix (v2): First iteration broke expectLockRemovedOnlyAfterFinalRelease — the fallback removed the lock file during the first release in a reentrant pair. Added sessionLockHeldByThisProcess() guard so the fallback only triggers when all holders have released.
Scope check — readLockPayload, lockPath, fs, process.pid, normalizedSessionFile, and sessionLockHeldByThisProcess are all in scope at the fix site.

CI: Full test suite running.

Not tested (requires production environment):

Reproduction of the exact sidecar-lock snapshot mismatch race condition
Multi-process lock contention with the fallback path triggered
Long-running gateway with stale lock accumulation

clawsweeper · 2026-06-08T05:18:43Z

Codex review: needs real behavior proof before merge. Reviewed June 30, 2026, 3:35 PM ET / 19:35 UTC.

Summary
This PR wraps acquireSessionWriteLock release with best-effort PID-matched .lock file removal after the underlying fs-safe release completes.

PR surface: Source +17. Total +17 across 1 file.

Reproducibility: yes. at source level. fs-safe deletes the held entry before async snapshot cleanup, and the PR then awaits a payload read after its no-holder check, leaving a same-process reacquire window before PID-only path deletion.

Review metrics: 1 noteworthy metric.

Fallback regression coverage: 0 tests added. The PR changes session-lock release cleanup semantics but adds no coverage for the snapshot-mismatch fallback or same-process reacquire race.

Stored data model
Persistent data-model change detected: serialized state: src/agents/session-write-lock.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

[P2] Make fallback deletion ownership-safe and add a regression for same-process reacquire during awaited cleanup.
[P1] Add redacted runtime logs, terminal output, copied live output, or a recording showing after-fix snapshot-mismatch cleanup in a real OpenClaw setup.

Proof guidance:

[P1] Needs real behavior proof before merge: The PR provides CI/code-review evidence but no after-fix real runtime proof; the contributor should add redacted logs, terminal output, copied live output, or a recording and then update the PR body for re-review.

Risk before merge

[P2] The fallback can remove an active same-process session lock because the no-holder check happens before an awaited payload read, while a later same-process acquire can create a fresh lock with the same PID.
[P1] The PR still lacks after-fix real behavior proof for snapshot-mismatch cleanup, multi-process contention, or long-running gateway cleanup.
[P1] The broader session write-lock delivery problem remains tracked in [Bug]: Session write-lock timeouts block subagent delivery lanes #86538, so this narrow cleanup should not be treated as the canonical full fix.

Maintainer options:

Make fallback deletion ownership-safe (recommended)
Move the no-holder check to immediately before unlinking or use snapshot/ownership-token-aware deletion, then cover same-process reacquire during fallback cleanup.
Accept a proof override after code safety
Maintainers may explicitly accept source-level proof instead of live gateway proof only after the active-lock deletion race is fixed and covered.
Defer to canonical lock work
If maintainers prefer a broader run-aware lock ownership fix, pause or close this narrow fallback PR in favor of the canonical session-lock tracker.

Next step before merge

[P1] Human review is needed because external real behavior proof is absent and maintainers need to decide whether to repair this branch or defer to the canonical session-lock work.

Security
Cleared: No concrete security or supply-chain concern was found; the diff only changes session lock release logic and does not touch workflows, dependencies, secrets, or package execution paths.

Review findings

[P1] Recheck ownership immediately before unlinking — src/agents/session-write-lock.ts:998-1000

Review details

Best possible solution:

Make any release-side fallback cleanup ownership-safe at the deletion point or snapshot/token-aware, add a focused regression for same-process reacquire during cleanup, and require redacted runtime proof or explicit maintainer proof override before merge.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level. fs-safe deletes the held entry before async snapshot cleanup, and the PR then awaits a payload read after its no-holder check, leaving a same-process reacquire window before PID-only path deletion.

Is this the best way to solve the issue?

No. The PR is a plausible mitigation, but the best fix must preserve mutual exclusion with a deletion-point ownership check or snapshot/token-aware cleanup plus focused regression coverage and proof.

Full review comments:

[P1] Recheck ownership immediately before unlinking — src/agents/session-write-lock.ts:998-1000
The no-holder guard runs before an awaited readLockPayload. After innerRelease() removes the held entry, another same-process acquire can create a fresh lock while this release is awaiting; because that fresh lock also has pid === process.pid, the fallback can unlink an active lock and break mutual exclusion.
Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 5a89484eb31d.

Label changes

Label justifications:

P1: The PR targets session write-lock behavior that can block real agent/channel workflows and the current patch can regress the same session-state boundary.
merge-risk: 🚨 session-state: Merging the fallback as written could unlink a fresh same-process session lock and allow concurrent session writes.
rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR provides CI/code-review evidence but no after-fix real runtime proof; the contributor should add redacted logs, terminal output, copied live output, or a recording and then update the PR body for re-review.

Evidence reviewed

PR surface:

Source +17. Total +17 across 1 file.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	18	1	+17
Tests	0	0	0	0
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	1	18	1	+17

What I checked:

Repository policy applied: Root AGENTS.md and scoped src/agents/AGENTS.md were read fully; the review applied the repository guidance that session-state fallback behavior is compatibility-sensitive and needs deep source/dependency proof. (AGENTS.md:7, 5a89484eb31d)
Current main has no release-side fallback: Current main acquires through SESSION_LOCKS.acquire and returns the dependency lock.release directly, so this PR is not already implemented on main. (src/agents/session-write-lock.ts:929, 5a89484eb31d)
PR deletion check is separated from deletion by an await: The PR checks sessionLockHeldByThisProcess, then awaits readLockPayload, then removes by path when the payload PID matches the current process. (src/agents/session-write-lock.ts:998, 9851714a9b6a)
Dependency intentionally avoids deleting changed lock files: @openclaw/[email protected] returns false when the current lock snapshot no longer matches the observed snapshot, explicitly to avoid deleting a fresh holder by path. (openclaw/fs-safe/src/sidecar-lock.ts:145, cb91a5e42b30)
Dependency release removes held state before async cleanup: releaseHeldLock deletes the process-local held entry before awaiting removeLockIfUnchanged, leaving a same-process reacquire window while cleanup is still running. (openclaw/fs-safe/src/sidecar-lock.ts:274, cb91a5e42b30)
Existing tests protect reentrant/fresh-lock ownership: Adjacent tests require reentrant locks and old release handles not to remove a lock still held by a later same-process holder. (src/agents/session-write-lock.test.ts:21, 5a89484eb31d)

Likely related people:

steipete: Recent GitHub history shows repeated changes to stale-lock reporting, transient stale retry behavior, cleanup policy, and adjacent session-lock tests. (role: session-lock policy contributor; confidence: high; commits: 210adf1d1189, 7ca77124fea0, 0b86decf948d; files: src/agents/session-write-lock.ts, src/agents/session-write-lock.test.ts)
njuboy11: Authored the current maxHoldMs stale-detection work that established part of the acquire-time session-lock reclaim policy. (role: feature-history contributor; confidence: high; commits: a1eb765f0a32; files: src/agents/session-write-lock.ts, src/agents/session-write-lock.test.ts)
openperf: Recent history touched owner-process argument handling in this module, and the same author has an open adjacent maxHoldMs reclaim PR. (role: recent adjacent contributor; confidence: medium; commits: c430fcde1c42, 25db525072d7; files: src/agents/session-write-lock.ts, src/agents/session-write-lock.test.ts)
vincentkoc: Recent current-main history includes a runtime guard refactor touching the same session-lock module. (role: recent area contributor; confidence: medium; commits: d1ea170c9b55; files: src/agents/session-write-lock.ts)
giodl73-repo: Recent current-main history added doctor exposure for session-lock findings in the same module area. (role: recent diagnostics contributor; confidence: medium; commits: 704fc3504392; files: src/agents/session-write-lock.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

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

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8ada369b4f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-06-08T05:19:02Z

+          const payload = await readLockPayload(lockPath);
+          if (payload?.pid === process.pid) {
+            await fs.rm(lockPath, { force: true });


Preserve active reentrant locks on release

When the same process holds a reentrant session write lock (for example allowReentrant: true, or the watchdog test's old release handle after a new in-process lock is acquired), innerRelease() can legitimately leave the .lock file in place because another in-process holder still owns it. This fallback then sees the current PID and removes that active lock file anyway, so another writer can acquire the session while the second holder still believes it is protected; the existing expectLockRemovedOnlyAfterFinalRelease coverage in src/agents/session-write-lock.test.ts exercises exactly this contract but cannot pass with this unconditional cleanup.

Useful? React with 👍 / 👎.

When sidecar-lock's removeLockIfUnchanged fails due to snapshot mismatch (e.g. lock file content modified between acquire and release), the session write-lock file persists on disk, blocking all subsequent messages to that session with SessionWriteLockTimeoutError. This adds a fallback cleanup in acquireSessionWriteLock: after the inner release, check if the lock file still exists and its pid matches the current process. If so, force-remove it. Fixes session write lock leak observed when embedded abort settle times out during cleanup, leaving the lock file orphaned on disk with maxHoldMs=1020000 (17 min) and no automatic recovery. Closes openclaw#91327

The original fallback unconditionally force-removed the lock file whenever pid matched, which broke reentrant lock semantics — the first release in a reentrant pair would remove the file while another holder still expected it to exist. Now checks sessionLockHeldByThisProcess before the fallback cleanup. If any reentrant holder remains, the lock file is preserved as sidecar-lock intends.

immortal-autumn · 2026-06-12T04:34:17Z

Real behavior proof — maintainer judgement requested

The "Real behavior proof" CI gate is asking for runtime evidence from a live OpenClaw setup. I want to be transparent about what has been verified and what requires maintainer judgement:

✅ Verified

Test suite passes: The CI checks-node-agentic-agents-core-runner (670 tests, including the reentrant lock test expectLockRemovedOnlyAfterFinalRelease with 3 sub-cases) now passes after the v2 fix that added the sessionLockHeldByThisProcess() reentrancy guard.
Code review: The fallback is purely additive — it wraps the existing lock.release() and runs only when all holders have released (!sessionLockHeldByThisProcess(...)), the lock file still exists on disk, and its pid matches the current process. All dependencies (readLockPayload, lockPath, fs, normalizedSessionFile) are in scope and already used in the same function.
Rebase clean: Fork was 4,466 commits behind upstream; rebased onto current openclaw/main tip with zero conflicts.

❌ Cannot reproduce in test environment

The original bug (sidecar-lock snapshot mismatch leaving orphan .lock files — see #91327) is a production race condition triggered by embedded abort settle timed out during chat abort. It requires a running gateway with active session processing to reproduce. A unit test mock of the sidecar-lock snapshot mismatch path is possible but would require exposing internal sidecar-lock testing hooks.

🔧 Request

Given that this fix is defensive (no existing code path is broken), test-verified for the reentrancy edge case, and the original bug requires a production gateway to reproduce, I respectfully request maintainer review on whether a proof: override is appropriate here.

If a more concrete runtime verification is preferred, I am happy to set up a local gateway instance with this patch and capture logs — just let me know the preferred format.

openclaw-barnacle · 2026-07-16T04:29:17Z

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

clawsweeper · 2026-07-17T05:18:19Z

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix(session): add fallback lock file cleanup on session write-lock release This is item 1/1 in the current shard. Shard 0/2.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

openclaw-barnacle Bot added agents Agent runtime and tooling size: XS triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 8, 2026

chatgpt-codex-connector Bot reviewed Jun 8, 2026

View reviewed changes

immortal-autumn mentioned this pull request Jun 8, 2026

Session write lock leaked on embedded abort settle timeout #91327

Closed

immortal-autumn force-pushed the main branch from 8ada369 to 39cb851 Compare June 12, 2026 04:24

clawsweeper Bot removed the rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. label Jun 12, 2026

clawsweeper Bot added rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 12, 2026

zhanxingxin1998 mentioned this pull request Jun 16, 2026

fix(session): guard fallback lock cleanup with reentrancy check #93557

Closed

clawsweeper Bot mentioned this pull request Jun 19, 2026

fix(agents): reclaim session write-locks held past the holder's own maxHoldMs #89673

Closed

2 tasks

openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jul 16, 2026

openclaw-barnacle Bot added triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. and removed stale Marked as stale due to inactivity labels Jul 16, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(session): add fallback lock file cleanup on session write-lock release#91332

fix(session): add fallback lock file cleanup on session write-lock release#91332
immortal-autumn wants to merge 2 commits into
openclaw:mainfrom
immortal-autumn:main

immortal-autumn commented Jun 8, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented Jun 8, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Jun 8, 2026

Uh oh!

immortal-autumn commented Jun 12, 2026

Uh oh!

openclaw-barnacle Bot commented Jul 16, 2026

Uh oh!

clawsweeper Bot commented Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

immortal-autumn commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Why this is safe

Files changed

Real behavior proof

Uh oh!

clawsweeper Bot commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

immortal-autumn commented Jun 12, 2026

Real behavior proof — maintainer judgement requested

✅ Verified

❌ Cannot reproduce in test environment

🔧 Request

Uh oh!

openclaw-barnacle Bot commented Jul 16, 2026

Uh oh!

clawsweeper Bot commented Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

immortal-autumn commented Jun 8, 2026 •

edited

Loading

clawsweeper Bot commented Jun 8, 2026 •

edited

Loading