fix(suggest-compact): clean up old counter temp files - #2159
Conversation
claude-tool-count-<sessionId> files were written into the OS temp dir on every hook run and never removed, accumulating one orphan per session indefinitely. Sweep stale counter files at the top of main() before opening the active counter. Retention is env-tunable via COMPACT_STATE_TTL_DAYS (default 14 days); invalid values fall back to the default. The active session's counter file is preserved unconditionally even if its mtime is past the cutoff. Failures during the sweep are swallowed to preserve the always-exit-0 hook contract. Adds 7 regression tests covering the sweep, env-var validation, and the always-exit-0 invariant under a populated temp dir. Fixes affaan-m#2156
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds configurable retention (COMPACT_STATE_TTL_DAYS, default 14) and a cleanupOldCounters routine to sweep and remove stale ChangesCounter Cleanup and Retention
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/hooks/suggest-compact.js`:
- Line 78: The TTL cutoff comparison is off-by-one: in the loop that checks file
age using stats.mtimeMs and cutoffMs (line with "if (stats.mtimeMs > cutoffMs)
continue;") change the comparison to use >= so files exactly at the cutoff are
treated as within retention (i.e. use if (stats.mtimeMs >= cutoffMs)
continue;)—update the condition where stats.mtimeMs and cutoffMs are compared to
enforce "older than" semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ade602ab-9d20-4cc4-b9ec-90a4d643b8c5
📒 Files selected for processing (2)
scripts/hooks/suggest-compact.jstests/hooks/suggest-compact.test.js
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/hooks/suggest-compact.js">
<violation number="1" location="scripts/hooks/suggest-compact.js:113">
P2: Cleanup scans the full OS temp directory synchronously on every PreToolUse invocation, adding potentially unbounded blocking I/O before each tool call.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| // Sweep stale counter files (concern 1 of #2156). Cheap, swallows errors, | ||
| // skips the active session's file. See cleanupOldCounters for details. | ||
| cleanupOldCounters(tempDir, getCounterRetentionDays(), counterFile); |
There was a problem hiding this comment.
P2: Cleanup scans the full OS temp directory synchronously on every PreToolUse invocation, adding potentially unbounded blocking I/O before each tool call.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/hooks/suggest-compact.js, line 113:
<comment>Cleanup scans the full OS temp directory synchronously on every PreToolUse invocation, adding potentially unbounded blocking I/O before each tool call.</comment>
<file context>
@@ -43,7 +105,13 @@ async function main() {
+
+ // Sweep stale counter files (concern 1 of #2156). Cheap, swallows errors,
+ // skips the active session's file. See cleanupOldCounters for details.
+ cleanupOldCounters(tempDir, getCounterRetentionDays(), counterFile);
+
const rawThreshold = parseInt(process.env.COMPACT_THRESHOLD || '50', 10);
</file context>
| cleanupOldCounters(tempDir, getCounterRetentionDays(), counterFile); | |
| + // Only sweep occasionally to avoid blocking every tool call; use a sentinel file | |
| + // to track the last sweep time without scanning the full temp dir each run. | |
| + const sweepMarker = path.join(tempDir, `${COUNTER_FILE_PREFIX}.last-sweep`); | |
| + let shouldSweep = false; | |
| + try { | |
| + const sweepStats = fs.statSync(sweepMarker); | |
| + shouldSweep = Date.now() - sweepStats.mtimeMs > 24 * 60 * 60 * 1000; // once per day | |
| + } catch { | |
| + shouldSweep = true; // no marker yet | |
| + } | |
| + if (shouldSweep) { | |
| + cleanupOldCounters(tempDir, getCounterRetentionDays(), counterFile); | |
| + try { fs.writeFileSync(sweepMarker, ''); } catch { /* swallow */ } | |
| + } |
There was a problem hiding this comment.
Thanks. Considered but not adopting: the sentinel-file approach trades one orphan class for another (the marker itself, plus extra writes per invocation), and the original concern (#2156) was specifically about temp files being created on every PreToolUse and never cleaned. On a typical OS temp dir the readdirSync + statSync sweep runs in well under the hook's <200ms budget, and cleanupOldCounters swallows all I/O errors so a slow filesystem cannot block tool execution. If profiling later shows real overhead, gating the sweep by frequency (e.g. once per process) would be cheaper than a sentinel and keeps the cleanup self-contained.
The cleanup sweep used `mtimeMs > cutoffMs` to short-circuit, which matched files whose mtime sits exactly on the cutoff boundary and deleted them. The cleanupOldCounters docstring promises only files *older than* retentionDays are removed; a file at age == retentionDays is not older than retentionDays, so it must survive. Switch the comparison to `>=` so only strictly older files fall through to deletion. Add a regression test that pins boundary-aged files (mtimeMs sitting just past the projected cutoff) are preserved. Refs affaan-m#2156
* fix(suggest-compact): clean up old counter temp files claude-tool-count-<sessionId> files were written into the OS temp dir on every hook run and never removed, accumulating one orphan per session indefinitely. Sweep stale counter files at the top of main() before opening the active counter. Retention is env-tunable via COMPACT_STATE_TTL_DAYS (default 14 days); invalid values fall back to the default. The active session's counter file is preserved unconditionally even if its mtime is past the cutoff. Failures during the sweep are swallowed to preserve the always-exit-0 hook contract. Adds 7 regression tests covering the sweep, env-var validation, and the always-exit-0 invariant under a populated temp dir. Fixes affaan-m#2156 * fix(suggest-compact): preserve counter files at the TTL cutoff boundary The cleanup sweep used `mtimeMs > cutoffMs` to short-circuit, which matched files whose mtime sits exactly on the cutoff boundary and deleted them. The cleanupOldCounters docstring promises only files *older than* retentionDays are removed; a file at age == retentionDays is not older than retentionDays, so it must survive. Switch the comparison to `>=` so only strictly older files fall through to deletion. Add a regression test that pins boundary-aged files (mtimeMs sitting just past the projected cutoff) are preserved. Refs affaan-m#2156
2 participants
Summary
claude-tool-count-<sessionId>files written byscripts/hooks/suggest-compact.jswere never removed, accumulating one orphan per session in the OS temp dir indefinitely (concern 1 of suggest-compact: counter temp files never cleaned up, and count resets every /compact #2156).cleanupOldCounters()that sweeps stale counter files (older thanCOMPACT_STATE_TTL_DAYSdays, default 14) at the top ofmain()before the active counter is opened. The active session's counter file is preserved unconditionally; sweep failures are swallowed to keep the always-exit 0hook contract intact.COMPACT_THRESHOLDparser: zero, negative, and non-numeric values fall back to the default./compactbecausesession_idrotates) changes user-visible counter semantics and is better as a separate PR.Verification
New test cases pin the contract:
COMPACT_STATE_TTL_DAYSenv varCOMPACT_STATE_TTL_DAYS(0,-5,abc)claude-tool-count-prefix)Fixes #2156
Summary by cubic
Clean up stale
claude-tool-count-<sessionId>temp files to prevent unbounded buildup. Adds a TTL-based sweep (default 14 days) that runs before opening the active counter and preserves files at the TTL boundary; keeps the hook’s exit-0 behavior. Fixes #2156.COMPACT_STATE_TTL_DAYS; invalid values fall back to 14 days.Written for commit 1e0d835. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests