Skip to content

fix(gateway-lock): close fd and remove partial lock file on writeFile failure#99356

Closed
zhangqueping wants to merge 1 commit into
openclaw:mainfrom
zhangqueping:fix/issue-98958-gateway-lock-fd-leak
Closed

fix(gateway-lock): close fd and remove partial lock file on writeFile failure#99356
zhangqueping wants to merge 1 commit into
openclaw:mainfrom
zhangqueping:fix/issue-98958-gateway-lock-fd-leak

Conversation

@zhangqueping

@zhangqueping zhangqueping commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Summary

When fs.open(lockPath, "wx") succeeds but handle.writeFile() fails (e.g. disk full), the code throws GatewayLockError without closing the file handle or removing the partial lock file, causing a file descriptor leak.

Root Cause

src/infra/gateway-lock.ts:264-275 — The try block opens a handle, writes the payload, and returns. The catch block only checks error code EEXIST — any other error (including writeFile failure) skips handle.close().

What changed

Move handle to let scope and close it + remove the partial lock file in the catch block before re-throwing.

Evidence

Source inspection: fs.open() at line 264 succeeds → handle.writeFile() at 274 fails → catch block at 283 → skips close → re-throws with open fd leaked.

Risk checklist

Risk level: Minimal — 7-line change on error path only. Existing lock acquisition tests already cover error paths.

Fixes #98958

@zhangqueping
fix(usage-bar): evict oldest template cache entry when size exceeds l…
be578e3 
…imit

The module-level fileCache Map grows without bound and holds a live
fs.watch watcher for each cached template file.  Evict the oldest
entry (closing its watcher) when the cache reaches MAX_FILE_CACHE_SIZE
(64) so long-running gateways do not accumulate unused watchers.

Fixes openclaw#98960
@openclaw-barnacle openclaw-barnacle Bot added size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 3, 2026
@clawsweeper

clawsweeper Bot commented Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 3, 2026, 12:23 AM ET / 04:23 UTC.

Summary
This PR adds a 64-entry cap to the usage-bar template file cache and closes the evicted entry's fs.watch watcher before storing the new cache entry.

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

Reproducibility: yes. Source inspection of current main shows each distinct valid template path can allocate a watcher-backed cache entry with no size bound; I did not run a live watcher-exhaustion repro in this read-only review.

Review metrics: 2 noteworthy metrics.

  • Closing reference mismatch: 1 mismatched closing issue. The PR body closes the gateway-lock issue while the diff and commit message target the usage-bar cache issue.
  • Same-head duplicate: 1 open same-head PR. fix(usage-bar): evict oldest template cache entry when size exceeds limit #99358 has the same head commit and a correctly scoped usage-bar title/body, so maintainers should choose one canonical review path.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #98960
Summary: This PR's actual diff is a candidate fix for the canonical usage-template fileCache/fs.watch watcher leak, while its public title/body incorrectly reference the separate gateway-lock issue.

Members:

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

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦪 silver shellfish
Result: blocked until real behavior proof is added.

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

Rank-up moves:

  • [P1] Move eviction and watcher closure before creating the replacement watcher, then add focused eviction/watcher-close regression coverage.
  • Update the title, body, branch context, and closing references so the PR targets usage-bar: template file cache grows unbounded with live fs.watch watchers #98960.
  • [P1] Add redacted real behavior proof such as terminal output or logs from loading more than 64 template paths and observing bounded watcher behavior.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body provides source inspection but no after-fix terminal/live output, logs, recording, or linked artifact showing bounded watcher behavior in a real setup. 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] The PR body uses closing syntax for gateway-lock: file descriptor leak when writeFile fails after acquiring lock #98958 even though the diff changes usage-bar cache code, so merging can close the gateway-lock issue without fixing it.
  • [P1] Evicting after watch(path) still leaves the full-cache path vulnerable when the process is already near the watcher or file-descriptor limit.
  • [P1] Contributor real behavior proof is absent, so the bounded watcher behavior has not been shown in an after-fix real setup.

Maintainer options:

  1. Correct target and ordering before merge (recommended)
    Update the PR metadata and closing references to the usage-bar issue, then move eviction before watcher allocation with focused coverage.
  2. Continue in the same-head duplicate instead
    Maintainers can close this context-mismatched PR and continue review in fix(usage-bar): evict oldest template cache entry when size exceeds limit #99358 after proof and code ordering are fixed there.

Next step before merge

  • [P1] This needs human/author follow-up because the contributor must correct the PR context and supply real behavior proof before any merge path is safe.

Security
Cleared: The diff only changes internal TypeScript cache logic and does not add dependencies, workflows, lockfiles, scripts, secrets handling, or package-resolution changes.

Review findings

  • [P2] Evict before opening the next watcher — src/auto-reply/usage-bar/template.ts:149
Review details

Best possible solution:

Keep one correctly scoped usage-bar cache fix that evicts before watcher allocation, includes focused regression coverage, supplies real behavior proof, and closes #98960 rather than the gateway-lock issue.

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

Yes. Source inspection of current main shows each distinct valid template path can allocate a watcher-backed cache entry with no size bound; I did not run a live watcher-exhaustion repro in this read-only review.

Is this the best way to solve the issue?

No. The cache module is the right boundary, but this implementation should evict before calling fs.watch and the PR metadata/proof must match the actual usage-bar fix.

Full review comments:

  • [P2] Evict before opening the next watcher — src/auto-reply/usage-bar/template.ts:149
    This eviction block runs after watch(path, ...) has already tried to allocate the replacement watcher. When the cache is full and the process is near the watcher/file-descriptor limit, the new watcher can fail before an old one is closed, leaving the loaded template without live refresh even though freeing one existing watcher first would have worked.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.88

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P2: This is a normal-priority resource-leak fix in an advanced usage-footer runtime path, but the PR has bounded correctness, proof, and metadata blockers.
  • add merge-risk: 🚨 other: Merging as-is can close the wrong GitHub issue because the PR body points to the gateway-lock tracker while the diff changes usage-bar cache code.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • 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 provides source inspection but no after-fix terminal/live output, logs, recording, or linked artifact showing bounded watcher behavior in a real setup. 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.

Label justifications:

  • P2: This is a normal-priority resource-leak fix in an advanced usage-footer runtime path, but the PR has bounded correctness, proof, and metadata blockers.
  • merge-risk: 🚨 other: Merging as-is can close the wrong GitHub issue because the PR body points to the gateway-lock tracker while the diff changes usage-bar cache code.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • 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 provides source inspection but no after-fix terminal/live output, logs, recording, or linked artifact showing bounded watcher behavior in a real setup. 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 +11. Total +11 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 11 0 +11
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 11 0 +11

What I checked:

  • Current main has the unbounded watcher-backed cache: cacheTemplateFile can create an fs.watch watcher for each valid template path and then stores every path in fileCache with no max size or eviction path. (src/auto-reply/usage-bar/template.ts:123, e7384d5f0205)
  • Runtime caller reaches the cache only for full usage footers: resolveResponseUsageLine calls loadUsageBarTemplate for /usage full rendering when reply usage state exists, so the affected runtime surface is the advanced usage footer path. (src/auto-reply/reply/agent-runner-usage-line.ts:98, e7384d5f0205)
  • Adjacent tests lack cache eviction coverage: Existing template tests cover loading, fallback, retry, and cached serving, but no full-cache eviction or watcher-close behavior. (src/auto-reply/usage-bar/template.test.ts:59, e7384d5f0205)
  • PR patch evicts after watcher allocation: The added eviction block is placed after the try block that calls watch(path), so a full-cache path near the watcher/file-descriptor limit can still fail before an old watcher is freed. (src/auto-reply/usage-bar/template.ts:149, be578e3c9998)
  • PR metadata targets the wrong issue: Live PR data shows the title/body and closing reference describe the gateway-lock issue, while the changed file and commit message target the usage-bar template cache issue. (be578e3c9998)
  • Latest release still has the unbounded cache: v2026.6.11 contains the same watcher-backed fileCache.set path without eviction, so this PR is not obsolete by the latest shipped release. (src/auto-reply/usage-bar/template.ts:123, e085fa1a3ffd)

Likely related people:

  • Peetiegonzalez: GitHub commit metadata identifies this handle as the author of the performance change that introduced the watcher-backed usage-template cache this PR modifies. (role: introduced behavior; confidence: high; commits: 069cb8d63687, ba7275e95974; files: src/auto-reply/usage-bar/template.ts, src/auto-reply/usage-bar/template.test.ts)
  • obviyus: GitHub metadata shows this handle committed the watcher-backed cache change and authored follow-up template handling fixes in the same module. (role: recent area committer and follow-up contributor; confidence: high; commits: 069cb8d63687, ff6940036b43; files: src/auto-reply/usage-bar/template.ts, src/auto-reply/usage-bar/template.test.ts, docs/concepts/usage-tracking.md)
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: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Jul 3, 2026
@zhangqueping

zhangqueping commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Closing — this PR was created with incorrect metadata (title/body reference gateway-lock #98958 but the diff is the usage-bar template change). The same commit is already reviewed under #99358 (now closed as superseded by #98990).

This was referenced Jul 3, 2026
Closed
Merged
Merged
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

merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

gateway-lock: file descriptor leak when writeFile fails after acquiring lock

1 participant

@zhangqueping