fix: the 24h cache sweeper deletes the 700 MB roformer checkpoint every day - #13
Conversation
…t every day
Reported from a live install: restart the app and the demucs server re-downloads
BS-Roformer-SW.ckpt (~700 MB). Every time. The weights were on disk yesterday and are gone
today, with no error and nothing in the UI to explain it.
Cause. Model weights live under CACHE_DIR, alongside the stem caches that the 24h TTL sweeper
is there to reap. The sweeper protected them with a BLACKLIST:
_PRESERVED_CACHE_DIRS = frozenset({"torch", "huggingface", "locale"})
`_roformer-models` is not in it. So the sweeper deleted the checkpoint every 24 hours, and
the next start re-fetched it. That also explains why the OTHER weights survived (whisperx and
CREPE live under huggingface/ and torch/, which are listed) — only the roformer checkpoint
went missing, which makes it look like a roformer bug rather than a sweeper one.
Timeline on the reporting machine matched exactly: checkpoint downloaded ~7/12 morning, the
server ran continuously, swept its own model dir once it crossed 24h, and re-downloaded on
the next start.
Fix. A blacklist is the wrong shape for this: it has to enumerate everything that must
survive, so whatever it forgets gets destroyed — including any model dir a FUTURE version
adds, silently, a day later. The sweeper now deletes only directories that look like stem
cache entries (`<16 hex>-<model-slug>`, which is exactly what _job_id_for() produces).
Anything else under CACHE_DIR is left alone. `_roformer-models` is added to the preserve list
as well, as a second line of defence.
6 tests, including "an unknown future model dir must survive" — the case the blacklist could
never have handled.
Signed-off-by: topkoa <topkoa@gmail.com>
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe cache sweeper now deletes only directories matching the stem-cache naming pattern, preserves ChangesCache cleanup hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server.py (1)
2213-2218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring is now stale relative to the new guard logic.
It still says the loop "skips preserved directories (torch, huggingface, locale)" — omitting
_roformer-modelsand the new_CACHE_ENTRY_REname-shape guard that is actually the fix for this bug.📝 Suggested docstring update
"""Background daemon thread: periodically delete expired stem cache dirs. - Walks CACHE_DIR, skips preserved directories (torch, huggingface, locale), - and deletes any stem cache directory whose mtime exceeds CACHE_TTL. + Walks CACHE_DIR, skips preserved model directories (torch, huggingface, + locale, _roformer-models) and any entry not matching _CACHE_ENTRY_RE, + then deletes any stem cache directory whose mtime exceeds CACHE_TTL. Runs every 10 minutes. """🤖 Prompt for 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. In `@server.py` around lines 2213 - 2218, Update the background daemon thread docstring to document all current deletion guards: preserved torch, huggingface, and locale directories, the preserved _roformer-models directory, and the _CACHE_ENTRY_RE name-shape check before deleting stem cache directories.tests/test_cache_cleanup.py (1)
297-303: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid
execfor extracting the pattern; parse the string literal instead.Static analysis flags
exec(line, ns)here (Ruff S102 / ast-grep no-exec, CWE-94). The source is trusted (server.py in this repo), so exploitability is minimal, but it's easy to avoid entirely by extracting the pattern string and compiling it directly.🔧 Exec-free alternative
def _entry_re(self): - ns = {"re": re} - for line in self.SERVER_SRC.splitlines(): - if line.startswith("_CACHE_ENTRY_RE"): - exec(line, ns) - return ns["_CACHE_ENTRY_RE"] - raise AssertionError("_CACHE_ENTRY_RE not found in server.py") + m = re.search(r'_CACHE_ENTRY_RE\s*=\s*re\.compile\((.+)\)', self.SERVER_SRC) + if not m: + raise AssertionError("_CACHE_ENTRY_RE not found in server.py") + return re.compile(ast.literal_eval(m.group(1)))(requires
import astat the top of the file)🤖 Prompt for 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. In `@tests/test_cache_cleanup.py` around lines 297 - 303, Update the test helper `_entry_re` to remove `exec(line, ns)` and parse the `_CACHE_ENTRY_RE` assignment with `ast`, extracting its string literal and compiling it directly. Add the required `ast` import, while preserving the existing source scan and assertion when the assignment is absent.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@server.py`:
- Around line 2213-2218: Update the background daemon thread docstring to
document all current deletion guards: preserved torch, huggingface, and locale
directories, the preserved _roformer-models directory, and the _CACHE_ENTRY_RE
name-shape check before deleting stem cache directories.
In `@tests/test_cache_cleanup.py`:
- Around line 297-303: Update the test helper `_entry_re` to remove `exec(line,
ns)` and parse the `_CACHE_ENTRY_RE` assignment with `ast`, extracting its
string literal and compiling it directly. Add the required `ast` import, while
preserving the existing source scan and assertion when the assignment is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f1aee3c7-3d81-4534-bb76-a8eaf925e43a
📒 Files selected for processing (2)
server.pytests/test_cache_cleanup.py
There was a problem hiding this comment.
Pull request overview
Fixes an issue where the 24h cache cleanup thread could delete large model-weight directories under CACHE_DIR (notably _roformer-models), causing repeated re-downloads. The PR changes the cleanup logic to delete only directories that match the stem-cache naming scheme and adds regression tests to ensure model directories (including unknown future ones) are never swept.
Changes:
- Add
_CACHE_ENTRY_REand gate deletion in_cache_cleanup_loop()on a strict stem-cache name pattern. - Add
_roformer-modelsto_PRESERVED_CACHE_DIRSas an additional safeguard. - Add a dedicated test suite asserting only stem-cache-shaped entries are deletable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
server.py |
Restricts sweeper deletions to stem-cache-shaped directories and preserves _roformer-models. |
tests/test_cache_cleanup.py |
Adds regression tests to prevent model-weight directories (including future/unknown ones) from being deleted. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…e, not the text
* _CACHE_ENTRY_RE was broader than what _job_id_for() actually emits — it allowed uppercase,
'_' and '.'. Matching more loosely than we emit only widens the set of directories we are
willing to delete, which is the exact opposite of the point of a whitelist. Tightened to
`^[0-9a-f]{16}-[a-z0-9-]+$`: lowercase hex hash, hyphen, sanitized lowercase slug. Nothing
else.
* The tests were brittle in three ways, all fair hits:
- exec()'d a raw source line to get the regex. Breaks if the assignment is reformatted,
and executes whatever that line happens to contain.
- asserted `'"_roformer-models"' in SERVER_SRC` — which would still pass if the name were
deleted from the preserve list and left behind in a comment. A test that passes on the
bug is worse than no test.
- searched a 2000-CHARACTER WINDOW of the cleanup loop for the pattern name. Add a comment
near the top of that function and the test fails for no reason.
All three now use AST, which this suite already does: read the constants as literals, and
assert the cleanup loop holds a real Name reference to them. The tests now test the code
rather than the text — they don't pass because a string appears in a comment, and they
don't break because a line moved.
+2 tests (the pattern is no looser than what we emit; the loop references the preserve list
too).
Signed-off-by: topkoa <topkoa@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_cache_cleanup.py (1)
359-365: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWeak assertion: only checks that names are referenced, not that they gate deletion.
test_the_sweeper_actually_consults_the_patternwalks the function body for anyast.Namenode matching_CACHE_ENTRY_RE/_PRESERVED_CACHE_DIRS. This would pass even if the reference were incidental (e.g. logged, or used outside a guard) rather than actually controlling thecontinue/skip logic in the loop. It's a reasonable proxy given the stated intent of avoiding textual/exec-based brittleness, but doesn't fully prove the safeguard is load-bearing.♻️ Optional: assert the names are used as guard conditions
def test_the_sweeper_actually_consults_the_pattern(self): """A constant nobody reads fixes nothing. Assert the cleanup loop REFERENCES it.""" loop = next(n for n in ast.walk(self.TREE) if isinstance(n, ast.FunctionDef) and n.name == "_cache_cleanup_loop") - names = {n.id for n in ast.walk(loop) if isinstance(n, ast.Name)} - assert "_CACHE_ENTRY_RE" in names, "_cache_cleanup_loop must check the pattern" - assert "_PRESERVED_CACHE_DIRS" in names + # Require the names to appear inside an `if` test, not just anywhere in the body. + guard_names = { + n.id + for if_node in ast.walk(loop) if isinstance(if_node, ast.If) + for n in ast.walk(if_node.test) if isinstance(n, ast.Name) + } + assert "_CACHE_ENTRY_RE" in guard_names, "_cache_cleanup_loop must gate on the pattern" + assert "_PRESERVED_CACHE_DIRS" in guard_names🤖 Prompt for 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. In `@tests/test_cache_cleanup.py` around lines 359 - 365, Strengthen test_the_sweeper_actually_consults_the_pattern so _CACHE_ENTRY_RE and _PRESERVED_CACHE_DIRS must appear in conditional guard expressions within _cache_cleanup_loop, rather than merely anywhere in the AST. Inspect the relevant ast.If or loop-control condition nodes and assert each name participates in logic that leads to skipping/continuing before deletion.
🤖 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.
Nitpick comments:
In `@tests/test_cache_cleanup.py`:
- Around line 359-365: Strengthen test_the_sweeper_actually_consults_the_pattern
so _CACHE_ENTRY_RE and _PRESERVED_CACHE_DIRS must appear in conditional guard
expressions within _cache_cleanup_loop, rather than merely anywhere in the AST.
Inspect the relevant ast.If or loop-control condition nodes and assert each name
participates in logic that leads to skipping/continuing before deletion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f41791f3-cc85-4465-a1e6-b9b60ac6ac2f
📒 Files selected for processing (2)
server.pytests/test_cache_cleanup.py
🚧 Files skipped from review as they are similar to previous changes (1)
- server.py
…ifically * _cache_cleanup_loop's docstring still described the OLD behaviour — "skips preserved directories, deletes any stem cache directory" — which is exactly the blacklist that caused the bug. A docstring that describes the version of the code that was broken is worse than no docstring: the next maintainer trusts it. It now states both conditions and says which one is load-bearing, and why. * The test asserted _CACHE_ENTRY_RE is "a Call", not that it is a re.compile call. A refactor to any other callable with the same first-argument shape would have slipped through, and the helper would have compiled whatever that call's first argument happened to be — testing a pattern the sweeper does not use. Now asserts the target is re.compile. Signed-off-by: topkoa <topkoa@gmail.com>
Symptom
Start the app, and the demucs server re-downloads
BS-Roformer-SW.ckpt(~700 MB). Every day. The weights were on disk yesterday; today they're gone. No error, nothing in the UI, no clue why.Reported from a live install and reproduced from the timeline: checkpoint downloaded on 7/12, server ran continuously, checkpoint gone by 7/13, re-downloading on the next start.
Cause
Model weights live under
CACHE_DIR— the same directory the 24-hour TTL sweeper exists to reap stale stem caches from. It protected the weights with a blacklist:_roformer-modelsisn't in it. So the sweeper deleted the checkpoint every 24 hours and the next start silently re-fetched 700 MB.This also explains why it looked like a roformer problem rather than a sweeper problem: whisperx and CREPE live under
huggingface/andtorch/, which are listed — so every other model survived and only the roformer checkpoint kept vanishing.Fix
A blacklist is the wrong shape here. It must enumerate everything that has to survive, so whatever it forgets gets destroyed — including any model directory a future version adds, silently, a day later. That's not a bug you find; it's a bug that finds you.
The sweeper now deletes only what it recognizes: directories matching the stem-cache name shape (
<16 hex>-<model-slug>, exactly what_job_id_for()produces). Everything else underCACHE_DIRis left alone._roformer-modelsis also added to the preserve list, as a second line of defence.Tests
6, including "an unknown future model dir must survive" — the case a blacklist could never have handled, and the reason this is a whitelist now.
Impact
Every user of the managed local server (and of the container) has been re-downloading 700 MB a day. It's invisible: the download happens at startup, the model still works afterwards, and nothing reports it.
Summary by CodeRabbit
Bug Fixes
Tests