Skip to content

fix(usage-bar): evict oldest template cache entry when full#98853

Closed
zhangLei99586 wants to merge 3 commits into
openclaw:mainfrom
zhangLei99586:fix/template-cache-eviction
Closed

fix(usage-bar): evict oldest template cache entry when full#98853
zhangLei99586 wants to merge 3 commits into
openclaw:mainfrom
zhangLei99586:fix/template-cache-eviction

Conversation

@zhangLei99586

@zhangLei99586 zhangLei99586 commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Related: #98960

What Problem This Solves

Module-level fileCache grew unbounded with live fs.watch watchers.

Why This Change Was Made

Add MAX_CACHED_TEMPLATES=32, evict oldest + close watcher before insert.

User Impact

Prevents unbounded watcher/memory growth.

Evidence

proof

  • Source: MAX_CACHED_TEMPLATES=32 (line 13), eviction at fileCache.set (line 149)
  • translator.test.ts: 13/13 passed ✅

@zhangLei99586
fix(usage-bar): evict oldest entry when template cache exceeds max size
d151c3c 
Each cached template file holds a live fs.watch watcher. The module-level
Map grew without bound, accumulating watchers and memory for every distinct
file path ever loaded during the process lifetime. Add a MAX_CACHED_TEMPLATES
limit (32) and evict the oldest entry, closing its watcher, before inserting.
@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:38 AM ET / 06:38 UTC.

Summary
The PR adds a 32-entry cap to the usage-bar template file cache and evicts the oldest cached watcher before storing a new entry.

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

Reproducibility: yes. Source inspection shows each distinct valid template path can allocate an fs.watch watcher and current main stores it in fileCache with no production eviction path; I did not run a live many-template exhaustion repro in this read-only review.

Review metrics: 1 noteworthy metric.

  • Runtime cache cap: 1 hard cap added: 32 template file paths. The fixed threshold changes how many template paths remain watched before older entries are reloaded on demand, so maintainers should intentionally accept the cap.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #98960
Summary: This PR is a candidate fix for the canonical usage-template fileCache watcher-growth issue, but the stronger open PR is the better current landing path.

Members:

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

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦪 silver shellfish
Patch quality: 🧂 unranked krab
Result: blocked until stronger 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 before watcher allocation and add focused eviction coverage.
  • [P1] Add redacted terminal/live output exercising the production loader with more than the cache cap and proving watcher cleanup.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The terminal-style screenshot lists claimed live checks, but it does not show the actual command, template count, or watcher create/close output needed to verify this runtime cache behavior. 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 eviction added by this branch runs after fs.watch allocation is attempted, so under watcher/file-descriptor pressure it can still cache the new template without live refresh before freeing an old watcher.
  • [P1] The related issue remains open and there is a stronger open PR for the same root cause, so maintainers should choose whether to repair this branch or converge on that implementation.

Maintainer options:

  1. Repair this branch
    Move eviction before the new watcher allocation and add focused cache-eviction tests before reconsidering this PR for merge.
  2. Use the stronger related PR
    Keep the remaining implementation discussion on fix(usage-bar): bound template file cache to prevent unbounded watche… #98990 if maintainers prefer its pre-allocation eviction and proof path.

Next step before merge

  • [P1] The PR has a concrete code defect, insufficient contributor proof, and an overlapping stronger open PR, so maintainers should decide whether to repair this branch or consolidate on the canonical PR.

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

Review findings

  • [P2] Move eviction before opening the next watcher — src/auto-reply/usage-bar/template.ts:145-153
Review details

Best possible solution:

Land a bounded-cache implementation that evicts and closes the oldest watcher before allocating the next watcher, with focused eviction and same-key retry coverage.

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

Yes. Source inspection shows each distinct valid template path can allocate an fs.watch watcher and current main stores it in fileCache with no production eviction path; I did not run a live many-template 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 the fix should evict before watcher allocation and include focused overflow/watcher-close regression coverage.

Full review comments:

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

Overall correctness: patch is incorrect
Overall confidence: 0.84

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This targets a concrete resource-growth bug in an advanced usage-footer runtime path with limited blast radius.
  • merge-risk: 🚨 availability: Merging this patch as written can still leave a newly cached valid template without live refresh when watcher allocation fails before eviction frees an old watcher.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The terminal-style screenshot lists claimed live checks, but it does not show the actual command, template count, or watcher create/close output needed to verify this runtime cache behavior. 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:

Likely related people:

  • Peetiegonzalez: GitHub commit metadata shows this author on the native usage-template loader and watcher-backed cache work that this PR amends. (role: introduced behavior; confidence: high; commits: 505539c89929, 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 commit metadata shows this person committed the watcher-backed cache work and authored a follow-up missing-template reload fix in the same module. (role: recent area contributor and committer; confidence: high; commits: 069cb8d63687, ff6940036b43; files: src/auto-reply/usage-bar/template.ts, src/auto-reply/usage-bar/template.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: 🧂 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 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.

@clawsweeper clawsweeper Bot added the proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. label Jul 2, 2026
@zhangLei99586

zhangLei99586 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. label 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.

@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

@clawsweeper clawsweeper Bot mentioned this pull request Jul 2, 2026
Merged
@clawsweeper clawsweeper Bot removed proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Jul 2, 2026
@zhangLei99586

zhangLei99586 commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. label Jul 3, 2026
@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Superseded by #98990, merged as a152a452845.

The landed implementation is the stronger version of this fix: current main bounds the watcher-backed file cache at 64 entries, evicts and closes the oldest watcher before allocating a replacement, only evicts for a genuinely new key, and includes focused cache/watcher regression coverage. This PR instead evicts after watcher allocation, uses a different 32-entry cap, and does not protect the same-key retry path from evicting an unrelated entry.

Closing this candidate because the underlying issue is fixed on main. Thanks @zhangLei99586 for identifying and working on the resource-growth bug.

@steipete steipete closed this Jul 9, 2026
@zhangLei99586

zhangLei99586 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

@zhangLei99586

zhangLei99586 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

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: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@zhangLei99586 @steipete