fix(cache): one cleanup thread per cache directory, not per manager - #453
Conversation
The display process ran three cleanup threads over one directory:
14:22:59.954 display_controller (the real manager)
14:22:59.973 startup validation, run 1 (discarded)
14:23:01.055 startup validation, run 2 (discarded)
Two of those managers existed only to read a directory path.
StartupValidator._validate_cache_directory built a whole CacheManager to
call get_cache_dir(), and validation runs twice -- once before the
plugin manager exists and again after. Each construction also probes
writability by writing and deleting .writetest on the card.
The discarded ones never went away. cleanup_loop closes over `self`, so
the thread keeps its manager alive: two objects that could never be
collected, waking every 24 hours to re-scan the same 9,000-file
directory. Nothing stopped them either -- stop_cleanup_thread had no
callers anywhere in the tree.
Two changes. The validator now takes the CacheManager the application
actually uses, which is also the more correct thing to validate; when
no caller supplies one it still builds its own, but stops the thread
afterwards. And CacheManager now tracks which directory it is sweeping,
so the second manager over a directory skips starting a thread at all.
That is the right granularity regardless of call sites: the sweep lists
a directory and deletes from it, so a second thread only duplicates the
scan. Ownership is released on stop, so a survivor can take over rather
than leaving the directory permanently unclaimed by a dead owner.
Measured directly, three managers over one directory: 3 threads before,
1 after.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 4 |
| 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.
|
@coderabbitai review |
|
1 participant
What
The display process ran three cleanup threads over one cache directory:
Two of those
CacheManagers existed only to read a directory path.StartupValidator._validate_cache_directorybuilt an entire one to callget_cache_dir(), and validation runs twice —display_controller.py:93before the plugin manager exists, and:261again after. Each construction also probes writability by writing and deleting.writeteston the card.Why they never went away
cleanup_loopis a closure overself, so the thread keeps its manager alive. Those two discarded objects could never be garbage collected, and woke every 24 hours to re-scan the same 9,000-file directory. Nothing stopped them either —stop_cleanup_threadhad zero callers anywhere in the tree.The fix
1. The validator uses the cache manager it's given. Also the more correct thing to validate: it was reporting on a cache the application doesn't use. When no caller supplies one it still builds its own — but stops the thread afterwards instead of abandoning it.
2.
CacheManagertracks which directory it's sweeping. A second manager over the same directory skips starting a thread. That's the right granularity independent of call sites: the sweep lists a directory and deletes from it, so a second thread only duplicates the scan. Ownership is released on stop, so a survivor can take over rather than leaving the directory permanently unclaimed by a dead owner.Measured
Tests
9 new tests in
test/test_cache_cleanup_thread_ownership.py: one manager starts one thread, three still start one, the first owns it, a survivor can take over, stopping a non-owner doesn't unclaim the directory, separate directories still get separate threads, twelve constructions leak nothing, and both validator paths (given a manager / building its own) leave no thread behind.2775 passed, 60 skippedacross the suite. The one failure,test_install_lowmem.py::TestDiskBackedTmpdir, is pre-existing onmainand environment-dependent — this rig's/tmpis already disk-backed.Scope
Deliberately not a singleton.
CacheManager.__init__takes no arguments, so a process-wide instance would be natural and would also collapse the duplicated memory tier and_write_digestsmap — but it touches every call site and changes test isolation, so it belongs in its own PR if wanted.Also left alone:
background_data_service.get_sport_cache_key()builds a full manager to computef"{sport}_{date_str}", which uses no instance state. I checked the callers — all four sports plugins passcache_keyexplicitly, so that branch never runs in production. A footgun worth a one-line fix, but not a live leak, and not this PR.Honest scope note
This is a thread leak and a startup-tidiness fix, not an SD-wear or framerate win like #269 and #452. Two leaked threads and two redundant
.writetestwrites per boot.🤖 Generated with Claude Code
https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5