Skip to content

fix(gateway-lock): close fd when writeFile fails after acquiring lock#98848

Closed
zhangLei99586 wants to merge 3 commits into
openclaw:mainfrom
zhangLei99586:fix/gateway-lock-fd-leak
Closed

fix(gateway-lock): close fd when writeFile fails after acquiring lock#98848
zhangLei99586 wants to merge 3 commits into
openclaw:mainfrom
zhangLei99586:fix/gateway-lock-fd-leak

Conversation

@zhangLei99586

@zhangLei99586 zhangLei99586 commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Related: #98958

What Problem This Solves

fs.open(lockPath, 'wx') acquired the lock but if writeFile() failed (disk full, etc), the code threw without closing the handle or removing the partial lock file, leaking an fd.

Why This Change Was Made

Inner try-catch closes the handle (handle.close()) and removes the partial lock file (fs.rm) before re-throwing GatewayLockError.

User Impact

No user-visible change. Prevents fd leak on disk errors during gateway lock acquisition.

Evidence

Failure-injection regression test with hashed lock path:

gateway-lock.test.ts — Test Files 1 passed, Tests 17 passed
✓ closes handle and removes lock file on writeFile rejection after open (6ms)
  → fs.open → fakeHandle with writeFile that rejects ENOSPC
  → GatewayLockError thrown
  → closeCalled === true (handle closed before throw)
  → fs.access(lockPath) rejects (partial lock removed)
  • 17/17 tests passed ✅ — new test asserts the hashed lock path (SHA256-based), same as production
  • Same cleanup pattern already validated in src/plugin-sdk/file-lock.test.ts:158

@zhangLei99586
fix(gateway-lock): close fd when writeFile fails after acquiring lock
60dd632 
When fs.open(lockPath, 'wx') succeeded but writeFile() failed
(e.g. disk full), the code threw without closing the file handle,
leaking a file descriptor. Wrap the write in an inner try-catch
that closes the handle and removes the partial lock file.
@clawsweeper

clawsweeper Bot commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 3, 2026, 2:37 AM ET / 06:37 UTC.

Summary
The PR wraps gateway lock payload writing after fs.open("wx") with cleanup that closes the handle and removes the lock file on write failure, plus a focused regression test.

PR surface: Source +8, Tests +23. Total +31 across 2 files.

Reproducibility: yes. Source-level reproduction is high confidence: make fs.open(lockPath, "wx") succeed and the returned FileHandle.writeFile reject, and current main has no close or removal path before throwing GatewayLockError; I did not run a mutating repro because this review is read-only.

Review metrics: 1 noteworthy metric.

Stored data model
Persistent data-model change detected: serialized state: src/infra/gateway-lock.test.ts, serialized state: src/infra/gateway-lock.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #98958
Summary: This PR is a candidate fix for the canonical gateway-lock post-open write-failure fd-leak issue; another open PR targets the same root cause but is not a safe superseding close target without maintainer selection.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
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:

  • [P1] Add redacted terminal output or logs from a small real-process fault-injection run showing writeFile rejection closes the handle, removes the lock file, and leaves fd count stable.
  • Coordinate with maintainers on whether this PR or Fix/issue 98958 gateway lock fd leak #99291 should be the single landing path.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body shows mocked regression-test output, but no after-fix terminal output, logs, recording, or linked artifact from a real process demonstrates the fd cleanup path. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Contributor proof is still only mocked regression-test output; no after-fix real process or terminal proof on this branch shows stable fd count and partial-lock cleanup.
  • [P1] Fix/issue 98958 gateway lock fd leak #99291 is another open same-root-cause candidate, so maintainers should land one gateway-lock cleanup path and avoid duplicate merges.

Maintainer options:

  1. Decide the mitigation before merge
    Land this narrow acquireGatewayLock cleanup and regression test after real behavior proof is added, then close the canonical issue and duplicate candidate path once one fix merges.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] Human review is needed for the real-behavior proof gate and duplicate-candidate selection; there is no narrow automated code defect in this branch.

Security
Cleared: The diff only changes local gateway lock cleanup and a test; it adds no dependencies, workflows, lockfiles, scripts, secrets handling, or package-resolution changes.

Review details

Best possible solution:

Land this narrow acquireGatewayLock cleanup and regression test after real behavior proof is added, then close the canonical issue and duplicate candidate path once one fix merges.

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

Yes. Source-level reproduction is high confidence: make fs.open(lockPath, "wx") succeed and the returned FileHandle.writeFile reject, and current main has no close or removal path before throwing GatewayLockError; I did not run a mutating repro because this review is read-only.

Is this the best way to solve the issue?

Yes for the code shape: cleanup belongs inside acquireGatewayLock because callers only receive a release handle after payload writing succeeds. The branch still needs real behavior proof before merge.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body shows mocked regression-test output, but no after-fix terminal output, logs, recording, or linked artifact from a real process demonstrates the fd cleanup path. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • remove status: 🛠️ actively grinding: Current PR status label is status: 📣 needs proof.

Label justifications:

  • P2: This is a bounded core gateway-runtime fd-leak fix with limited blast radius and an active narrow patch.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body shows mocked regression-test output, but no after-fix terminal output, logs, recording, or linked artifact from a real process demonstrates the fd cleanup path. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +8, Tests +23. Total +31 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 25 17 +8
Tests 1 23 0 +23
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 48 17 +31

What I checked:

  • Repository policy read: Root AGENTS.md was read fully; it requires whole-path PR review, best-fix assessment, proof review, and likely-owner history for this surface. (AGENTS.md:1, c785196287d0)
  • Current main leak path: Current main opens the lock file, writes the payload, and only closes the handle from the returned release callback; a writeFile rejection reaches the outer non-EEXIST error path without closing the already-opened handle or removing the partial lock file. (src/infra/gateway-lock.ts:264, c785196287d0)
  • PR runtime fix: The PR head adds an inner try/catch after fs.open that closes the acquired handle, removes the lock path with force: true, and rethrows so the existing GatewayLockError wrapping remains intact. (src/infra/gateway-lock.ts:264, 99a5ecacba52)
  • PR regression coverage: The added test mocks fs.open to return a handle whose writeFile rejects, then asserts the handle close path ran and the computed hashed lock path is absent. (src/infra/gateway-lock.test.ts:380, 99a5ecacba52)
  • Caller boundary: The gateway run loop receives a releasable lock only after acquireGatewayLock returns, so acquisition-time cleanup cannot be delegated to callers. (src/cli/gateway-cli/run-loop.ts:125, c785196287d0)
  • Sibling cleanup invariant: The plugin SDK file-lock sibling already tests that an opened lock handle is closed when owner-payload writing fails, supporting the same invariant for gateway locks. (src/plugin-sdk/file-lock.test.ts:158, c785196287d0)

Likely related people:

  • steipete: Git history links Peter Steinberger to the original gateway singleton lock and multiple later gateway-lock recovery/test changes in the same files. (role: feature owner and recent area contributor; confidence: high; commits: 05a254746eae, e6383a2c13e5, 32a6eae576e5; files: src/infra/gateway-lock.ts, src/infra/gateway-lock.test.ts, src/cli/gateway-cli/run-loop.ts)
  • Derek YU: Recent merged PID-recycling and startup-progress hardening touched acquireGatewayLock, its tests, and the gateway run loop. (role: recent gateway-lock contributor; confidence: medium; commits: 5f6e3499f321; files: src/infra/gateway-lock.ts, src/infra/gateway-lock.test.ts, src/cli/gateway-cli/run-loop.ts)
  • vincentkoc: Git history links Vincent Koc to gateway-lock hash-path changes and the latest release commit that still carries the affected code. (role: adjacent infra contributor and release carrier; confidence: medium; commits: c20d519e0576, e085fa1a3ffd; files: src/infra/gateway-lock.ts, src/infra/gateway-lock.test.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.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jul 2, 2026
@zhangLei99586

zhangLei99586 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@zhangLei99586

zhangLei99586 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot mentioned this pull request Jul 2, 2026
Closed
lwy-2 added a commit to lwy-2/openclaw that referenced this pull request Jul 2, 2026
@lwy-2
revert: drop gateway-lock changes (covered by openclaw#98848) [skip ci]
c0dde23
@zhangLei99586

zhangLei99586 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot mentioned this pull request Jul 2, 2026
Closed
@openclaw-barnacle openclaw-barnacle Bot added triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. and removed triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 2, 2026
@zhangLei99586

zhangLei99586 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@zhangLei99586
test(gateway-lock): add regression test for writeFile failure after open
366a50c 
Prove that when fs.open succeeds but writeFile rejects (e.g. disk
full), the handle is closed and the partial lock file is removed
before throwing GatewayLockError. This covers the distinct path not
tested by the existing EACCES-on-open-only case.
@zhangLei99586
zhangLei99586 force-pushed the fix/gateway-lock-fd-leak branch from 301d5a4 to 366a50c Compare July 2, 2026 14:20
@zhangLei99586

zhangLei99586 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@zhangLei99586
test(gateway-lock): assert hashed lock path in writeFile failure test
99a5eca 
The test previously hardcoded lockPath as gateway.lock, but the actual
lock path uses SHA256 hash. Use resolveLockPath(env).lockPath so the
cleanup assertion matches the real acquireGatewayLock behavior.
@zhangLei99586

zhangLei99586 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 2, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 3, 2026
Merged
@zhangLei99586

zhangLei99586 commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. labels Jul 3, 2026
@steipete

steipete commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Superseded by #99291, which landed as 36dd9ee3 and closed #98958.

The landed fix keeps cleanup inside acquireGatewayLock, removes a partial lock file as well as closing the handle, preserves the original write failure, and passed exact-head CI plus real-process FD/reacquisition proof. Thanks @zhangLei99586 for independently fixing the leak.

@steipete steipete closed this Jul 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@zhangLei99586 @steipete