fix(cache): collect the temp files abandoned writes leave behind - #452
Merged
Conversation
DiskCache.set() writes through mkstemp then os.replace, and removes its own temp file in a finally. That covers a write that fails, but not a process that dies between the two -- a SIGKILL, a lost restart race, a power cut, all ordinary on a Pi. Nothing ever collected what was left. The temp names are ".<key>.json.<random>", and cleanup_expired_files listed only names ending in .json, so every one of them was invisible to the sweep for as long as the card had been in service. On the dev rig: 76 files, 1,050 MB, 81% of the whole cache directory, the oldest six months old. The startup sweep reported "18/8864 files deleted, 0.01 MB freed" while sitting on top of a gigabyte it could not see. They are removed after an hour. A real write holds its temp file for milliseconds, so that is far outside any in-flight write while still clearing the same day's debris, and it is deliberately not tied to the retention policies: those say how long data stays useful, and a half-written file never was. The predicate is tested harder than the sweep, because a false positive deletes real data. It matches the shape set() creates rather than just a leading dot, so a completed ".json", a stray .gitignore, and a "weather.json.bak" are all left alone -- and one test drives set() itself and asserts the names it produces are matched, so the writer and the predicate cannot drift apart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe cache cleanup now removes atomic-write temporary files older than one hour. It identifies valid temporary filenames, preserves active and completed files, updates deletion and space statistics, handles concurrent removal, and retains existing JSON expiry cleanup. ChangesOrphan temporary cache cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DiskCache
participant CacheDirectory
participant CleanupStatistics
DiskCache->>CacheDirectory: snapshot directory entries
DiskCache->>CacheDirectory: remove stale orphan temporary files
CacheDirectory-->>DiskCache: removal result or missing-file condition
DiskCache->>CleanupStatistics: record deletions, freed space, and errors
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 `@src/cache/disk_cache.py`:
- Around line 414-425: Update the orphan-temp sweep loop around
_is_orphaned_temp so each evaluated orphan temporary file increments
stats['files_scanned'], including files that are deleted. Preserve the existing
completed-.json counting behavior, and add an assertion in
test_cache_orphan_temp_sweep.py verifying a directory containing only one stale
orphan reports one scanned file alongside its deletion.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b4e7c37a-37f6-417e-bc9c-3eef9593fa8c
📒 Files selected for processing (2)
src/cache/disk_cache.pytest/test_cache_orphan_temp_sweep.py
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 12 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
files_scanned only counted completed .json files, so a sweep that removed orphans reported more deleted than it had looked at -- the summary line renders "<deleted>/<scanned>", which came out as "76/1". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
cleanup_expired_fileslisted only names ending in.json.DiskCache.set()writes throughtempfile.mkstemp(prefix=f".{basename}."), which produces.<key>.json.<random>— so every temp file left behind by an interrupted write was invisible to the sweep, permanently.Measured on the dev rig
Meanwhile the startup sweep was reporting success on top of it:
set()removes its own temp file in afinally, so these are specifically the writes where the process died betweenmkstempandos.replace— SIGKILL, a lost restart race, a power cut. All ordinary on a Pi, and each one leaks a full copy of whatever was being written.The threshold
One hour. A real write holds its temp file for milliseconds, so that is far outside any in-flight write while still clearing the same day's debris. Deliberately not tied to the retention policies — those describe how long data stays useful, and a half-written file never was.
Safety
This predicate deletes files, so it's tested harder than the sweep itself. It matches the shape
set()creates rather than merely a leading dot:weather.json.weather.json.gitignoreweather.json.bak.json.abc.json..weather.json.inflight(recent)One test drives
set()itself and asserts the names it actually produces are matched, so the writer and the predicate cannot drift apart — the failure mode that caused this bug in the first place.Tests
19 new tests in
test/test_cache_orphan_temp_sweep.py, including the 76-orphan rig scenario and a mid-sweep disappearance (benign race, must not count as an error). Verified coupled to the change: against the previousdisk_cache.pythe module doesn't even import.257 passedacross every cache-related test in the suite.Note on the rest of the 1.3 GB
Roughly 250 MB is legitimate: large season schedules under a 60-day
sports_schedulesretention, includingnhl_schedule_2025.json(19 MB) from last season, which will age out on its own. Worth revisiting whether a superseded season needs 60 days, but that's a policy question, not a bug, and not part of this PR.🤖 Generated with Claude Code
https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Summary by CodeRabbit