Skip to content

fix(usage-bar): evict oldest template cache entry when size exceeds limit#99358

Closed
zhangqueping wants to merge 1 commit into
openclaw:mainfrom
zhangqueping:fix/issue-98960-template-cache-evict
Closed

fix(usage-bar): evict oldest template cache entry when size exceeds limit#99358
zhangqueping wants to merge 1 commit into
openclaw:mainfrom
zhangqueping:fix/issue-98960-template-cache-evict

Conversation

@zhangqueping

@zhangqueping zhangqueping commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Summary

The module-level fileCache Map in template.ts grows without bound. Each entry holds a live fs.watch watcher. Watchers are never closed and entries are never evicted — only clearUsageBarTemplateCacheForTest() closes them.

Root Cause

src/auto-reply/usage-bar/template.ts:146 — cacheTemplateFile() inserts without checking cache size.

What changed

Add MAX_FILE_CACHE_SIZE = 64 and evict the oldest entry (closing its watcher) before inserting new entries when the cache is full.

Evidence

Source inspection: fileCache.set(path, entry) at line 146 never evicts. Each new template file path creates a persistent watcher that lives for the gateway lifetime.

Risk checklist

Risk level: Low — cache eviction is LRU by insertion order (Map iteration order). A re-fetched cache-miss just reloads the file. 64 unique template paths is far above any real usage.

Fixes #98960

@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

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close as superseded: this one-file branch targets the right bug but repeats the known after-allocation eviction flaw and lacks real behavior proof, while #98990 is the stronger open canonical fix path with pre-allocation eviction, tests, and sufficient live-output proof.

Canonical path: Land the corrected bounded-cache fix through #98990 after its review findings are resolved, then close the canonical issue from the merged implementation.

So I’m closing this here and keeping the remaining discussion on #98990.

Review details

Best possible solution:

Land the corrected bounded-cache fix through #98990 after its review findings are resolved, then close the canonical issue from the merged implementation.

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

Yes, by source inspection: current main and the latest release store every valid distinct usage-template file path in fileCache and can attach an fs.watch watcher without any production eviction path. I did not run a live many-template 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 branch evicts only after trying to allocate the replacement watcher and is superseded by an open candidate that uses the safer pre-allocation ordering plus focused proof.

Security review:

Security review cleared: The diff only changes an internal TypeScript cache path and does not add dependencies, scripts, workflows, downloads, package-resolution changes, or secrets handling.

AGENTS.md: found and applied where relevant.

What I checked:

  • Current main still has the reported bug: Current main reads a template file, may allocate an fs.watch watcher, and stores the path in fileCache without any production max-size eviction path. (src/auto-reply/usage-bar/template.ts:123, e7384d5f0205)
  • This branch evicts after replacement watcher allocation: The PR head places the eviction block at lines 147-156, after watch(path, ...) has already been attempted at lines 132-142, so the full-cache path can still fail to create the replacement watcher before freeing an old one. (src/auto-reply/usage-bar/template.ts:147, be578e3c9998)
  • Canonical open PR uses the safer ordering: The stronger open candidate evicts old entries before creating the new watcher, and its branch also includes regression tests and proof artifacts for the same cache-bound issue. (src/auto-reply/usage-bar/template.ts:130, 53d196d7b491)
  • Related PR status and proof: Live PR metadata for fix(usage-bar): bound template file cache to prevent unbounded watche… #98990 shows it is open and mergeable, has proof:sufficient, and its ClawSweeper review says the PR body includes after-fix live terminal output from a real Node process plus direct watcher create/close measurement. (53d196d7b491)
  • This PR has no real behavior proof: The PR body only provides source inspection, and the Real behavior proof checks on this PR completed as failures or cancellations. (be578e3c9998)
  • Adjacent tests lack eviction coverage on main: Current adjacent tests cover loading, fallbacks, missing-file retry, invalid JSON repair, and cached serving, but not bounded eviction or watcher closure. (src/auto-reply/usage-bar/template.test.ts:59, e7384d5f0205)

Likely related people:

  • Peetiegonzalez: GitHub file history lists this author on the native templated usage footer renderer and the later watcher-backed cache performance change that created the cache lifecycle being fixed. (role: introduced behavior; confidence: high; commits: 64d0fc83364e, 069cb8d63687; files: src/auto-reply/usage-bar/template.ts, src/auto-reply/usage-bar/template.test.ts, src/auto-reply/reply/agent-runner-usage-line.ts)
  • obviyus: GitHub file history shows this person committed the watcher-backed cache work and authored several later usage-template hardening fixes in the same module. (role: recent area committer and follow-up contributor; confidence: high; commits: 069cb8d63687, ff6940036b43, afe75b3387d2; files: src/auto-reply/usage-bar/template.ts, src/auto-reply/usage-bar/template.test.ts, docs/concepts/usage-tracking.md)

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

@clawsweeper clawsweeper Bot mentioned this pull request Jul 3, 2026
Closed
@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. labels Jul 3, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 3, 2026
Merged
@zhangqueping

zhangqueping commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #98990, which uses the safer pre-allocation eviction ordering (evict before creating the new watcher, not after) and includes real-behavior proof with watcher create/close measurement.

Ref. ClawSweeper review: needs-human — superseded in root-cause cluster.

@clawsweeper clawsweeper Bot mentioned this pull request Jul 3, 2026
Closed
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: 🧂 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.

usage-bar: template file cache grows unbounded with live fs.watch watchers

1 participant

@zhangqueping