From b6f658ef4f421e0f6fb7f9bae7104e0f6914ee7e Mon Sep 17 00:00:00 2001 From: topkoa Date: Mon, 13 Jul 2026 22:09:10 -0400 Subject: [PATCH 1/3] fix: the 24h cache sweeper was deleting the 700 MB roformer checkpoint every day MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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>-`, 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 --- server.py | 26 +++++++++++++++-- tests/test_cache_cleanup.py | 56 +++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/server.py b/server.py index 8678717..d5d1137 100644 --- a/server.py +++ b/server.py @@ -84,8 +84,26 @@ def _parse_cache_max_completed_jobs(value: str | None) -> int: )) MAX_CONCURRENT = 2 CACHE_TTL = os.environ.get("CACHE_TTL", "24h") -# Directories under CACHE_DIR that hold model weights (never auto-deleted) -_PRESERVED_CACHE_DIRS = frozenset({"torch", "huggingface", "locale"}) +# Directories under CACHE_DIR that hold model weights (never auto-deleted). +# +# This list was a BLACKLIST, and a blacklist is the wrong shape for this job: it has to +# enumerate everything that must survive, so anything it forgets gets deleted. It forgot +# `_roformer-models`, and the 24h sweeper duly deleted the 700 MB BS-Roformer-SW checkpoint +# every single day — users re-downloaded it on the next start, forever, with no error and no +# clue why. (Reported at got-feedBack/feedBack-plugin-stem-splitter, seen in the wild.) +# +# The real fix is _CACHE_ENTRY_RE below: only delete directories that LOOK like stem-cache +# entries. The list stays as a second line of defence. +_PRESERVED_CACHE_DIRS = frozenset({"torch", "huggingface", "locale", "_roformer-models"}) + +# What a stem-cache directory is named: _job_id_for() -> f"{audio_hash}-{slug}", where the +# hash is sha256[:16] (hex) and the slug is a sanitized model name. +# +# The sweeper only deletes things matching THIS. Anything else under CACHE_DIR — a model dir +# we added, a model dir some future version adds, a stray file a user dropped in — is left +# alone. Deleting only what we recognize is the only version of this that stays correct as +# the cache dir grows new neighbours. +_CACHE_ENTRY_RE = re.compile(r"^[0-9a-f]{16}-[A-Za-z0-9._-]+$") def _parse_ttl(ttl_str: str) -> int | None: @@ -2212,6 +2230,10 @@ def _cache_cleanup_loop() -> None: continue if entry.name in _PRESERVED_CACHE_DIRS: continue + if not _CACHE_ENTRY_RE.fullmatch(entry.name): + # Not a stem-cache entry -> not ours to delete. Model weights, and + # anything else that comes to live under CACHE_DIR, land here. + continue try: mtime = entry.stat().st_mtime age = now - mtime diff --git a/tests/test_cache_cleanup.py b/tests/test_cache_cleanup.py index 1dbfd96..fbd822b 100644 --- a/tests/test_cache_cleanup.py +++ b/tests/test_cache_cleanup.py @@ -276,3 +276,59 @@ def test_sleep_at_end_of_loop_not_beginning(self): f"time.sleep should be the LAST statement in the while loop, " f"but last statement is: {last_stmt_text[:100]}" ) + +# ── the sweeper must not eat model weights ────────────────────────────────── + +class TestSweeperOnlyDeletesStemCaches: + """The 24h TTL sweeper deleted the 700 MB BS-Roformer-SW checkpoint every day. + + Model weights live under CACHE_DIR alongside the stem caches, and the sweeper protected + them with a BLACKLIST (`torch`, `huggingface`, `locale`). It forgot `_roformer-models`. + So every 24 hours the checkpoint was deleted, the next start re-downloaded 700 MB, and + nothing anywhere said why. Seen in the wild. + + A blacklist is the wrong shape here: it must enumerate everything that has to survive, so + whatever it forgets gets destroyed — including any model dir a future version adds. The + sweeper now deletes only names that LOOK like stem-cache entries. + """ + + SERVER_SRC = SERVER_PY.read_text(encoding="utf-8") + + 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") + + def test_a_real_stem_cache_entry_is_deletable(self): + # _job_id_for() -> f"{sha256[:16]}-{model-slug}" + assert self._entry_re().fullmatch("7a819518228bca22-bs-roformer-sw") + assert self._entry_re().fullmatch("deadbeefdeadbeef-htdemucs-ft") + + def test_the_roformer_model_dir_is_NOT_deletable(self): + """The actual bug. This directory holds a 700 MB checkpoint.""" + assert not self._entry_re().fullmatch("_roformer-models") + + def test_the_other_model_dirs_are_not_deletable(self): + for name in ("torch", "huggingface", "locale"): + assert not self._entry_re().fullmatch(name) + + def test_an_unknown_future_model_dir_is_not_deletable(self): + """The point of a whitelist: a model dir nobody has added yet must survive too. Under + the old blacklist it would have been silently deleted after 24h.""" + for name in ("_mdx-models", "whisper-models", "some-new-cache", "openvino"): + assert not self._entry_re().fullmatch(name), name + + def test_roformer_dir_is_also_on_the_preserve_list(self): + # Belt as well as braces: even if the pattern check were bypassed, the name is listed. + assert '"_roformer-models"' in self.SERVER_SRC + + def test_the_sweeper_actually_consults_the_pattern(self): + # A constant nobody calls fixes nothing. + src = self.SERVER_SRC + start = src.index("def _cache_cleanup_loop") + body = src[start:start + 2000] + assert "_CACHE_ENTRY_RE" in body, "the cleanup loop must check the pattern" + From 851ad7cc49c3a980f46e9360783d12aadd8fb849 Mon Sep 17 00:00:00 2001 From: topkoa Date: Mon, 13 Jul 2026 22:39:18 -0400 Subject: [PATCH 2/3] fix(review): tighten the sweeper pattern; make its tests test the code, not the text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * _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 --- server.py | 6 +++- tests/test_cache_cleanup.py | 67 +++++++++++++++++++++++++++---------- 2 files changed, 54 insertions(+), 19 deletions(-) diff --git a/server.py b/server.py index d5d1137..ab0530d 100644 --- a/server.py +++ b/server.py @@ -103,7 +103,11 @@ def _parse_cache_max_completed_jobs(value: str | None) -> int: # we added, a model dir some future version adds, a stray file a user dropped in — is left # alone. Deleting only what we recognize is the only version of this that stays correct as # the cache dir grows new neighbours. -_CACHE_ENTRY_RE = re.compile(r"^[0-9a-f]{16}-[A-Za-z0-9._-]+$") +# Exactly what _job_id_for() emits: sha256[:16] (lowercase hex) + "-" + a slug that is +# re.sub(r"[^A-Za-z0-9]+", "-", model).strip("-").lower() — so lowercase, digits and +# hyphens, nothing else. Matching more loosely than we emit only widens the set of +# directories we are willing to delete, which is the opposite of the point. +_CACHE_ENTRY_RE = re.compile(r"^[0-9a-f]{16}-[a-z0-9-]+$") def _parse_ttl(ttl_str: str) -> int | None: diff --git a/tests/test_cache_cleanup.py b/tests/test_cache_cleanup.py index fbd822b..1cc1c93 100644 --- a/tests/test_cache_cleanup.py +++ b/tests/test_cache_cleanup.py @@ -290,18 +290,39 @@ class TestSweeperOnlyDeletesStemCaches: A blacklist is the wrong shape here: it must enumerate everything that has to survive, so whatever it forgets gets destroyed — including any model dir a future version adds. The sweeper now deletes only names that LOOK like stem-cache entries. + + Everything below is read out of server.py with AST. Parsing the source (rather than + grepping it, or exec'ing a line of it) means these tests keep testing the CODE and not the + text: they don't pass because a name appears in a comment, and they don't break when a + line moves. """ - SERVER_SRC = SERVER_PY.read_text(encoding="utf-8") + TREE = ast.parse(SERVER_PY.read_text(encoding="utf-8")) - 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") + def _literal(self, name): + """The value assigned to a module-level constant.""" + for node in ast.iter_child_nodes(self.TREE): + if isinstance(node, ast.Assign): + for tgt in node.targets: + if isinstance(tgt, ast.Name) and tgt.id == name: + return node.value + raise AssertionError(f"{name} not found in server.py") + def _entry_re(self): + # _CACHE_ENTRY_RE = re.compile(r"...") -> compile the literal pattern itself. + call = self._literal("_CACHE_ENTRY_RE") + assert isinstance(call, ast.Call), "_CACHE_ENTRY_RE should be a re.compile(...) call" + pattern = ast.literal_eval(call.args[0]) + return re.compile(pattern) + + def _preserved(self): + # frozenset({...}) -> the real set, not a substring of the file. + node = self._literal("_PRESERVED_CACHE_DIRS") + if isinstance(node, ast.Call): # frozenset({...}) + node = node.args[0] + return set(ast.literal_eval(node)) + + # ── what may be deleted ─────────────────────────────────────────────────── def test_a_real_stem_cache_entry_is_deletable(self): # _job_id_for() -> f"{sha256[:16]}-{model-slug}" assert self._entry_re().fullmatch("7a819518228bca22-bs-roformer-sw") @@ -312,8 +333,8 @@ def test_the_roformer_model_dir_is_NOT_deletable(self): assert not self._entry_re().fullmatch("_roformer-models") def test_the_other_model_dirs_are_not_deletable(self): - for name in ("torch", "huggingface", "locale"): - assert not self._entry_re().fullmatch(name) + for name in ("torch", "huggingface", "locale", "hub"): + assert not self._entry_re().fullmatch(name), name def test_an_unknown_future_model_dir_is_not_deletable(self): """The point of a whitelist: a model dir nobody has added yet must survive too. Under @@ -321,14 +342,24 @@ def test_an_unknown_future_model_dir_is_not_deletable(self): for name in ("_mdx-models", "whisper-models", "some-new-cache", "openvino"): assert not self._entry_re().fullmatch(name), name + def test_the_pattern_is_no_looser_than_what_we_emit(self): + """Matching more loosely than _job_id_for() emits only widens the set of directories + we are willing to delete — the opposite of the point.""" + for name in ("DEADBEEFDEADBEEF-model", # hash is lowercase hex + "deadbeefdeadbeef-Model_X.1", # slug is lowercased + sanitized + "some.model.cache"): + assert not self._entry_re().fullmatch(name), name + + # ── the safeguards are real, not textual ────────────────────────────────── def test_roformer_dir_is_also_on_the_preserve_list(self): - # Belt as well as braces: even if the pattern check were bypassed, the name is listed. - assert '"_roformer-models"' in self.SERVER_SRC + # Membership in the actual set — not "the string appears somewhere in the file", + # which would still pass if the name were only left behind in a comment. + assert "_roformer-models" in self._preserved() def test_the_sweeper_actually_consults_the_pattern(self): - # A constant nobody calls fixes nothing. - src = self.SERVER_SRC - start = src.index("def _cache_cleanup_loop") - body = src[start:start + 2000] - assert "_CACHE_ENTRY_RE" in body, "the cleanup loop must check the pattern" - + """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 From b5d4eb96b5b1c1c5d83911ea57d4e98ba8d07300 Mon Sep 17 00:00:00 2001 From: topkoa Date: Mon, 13 Jul 2026 22:48:40 -0400 Subject: [PATCH 3/3] docs+test: describe what the sweeper now does; assert re.compile specifically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * _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 --- server.py | 16 +++++++++++++--- tests/test_cache_cleanup.py | 6 ++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/server.py b/server.py index ab0530d..dd6cf58 100644 --- a/server.py +++ b/server.py @@ -2215,9 +2215,19 @@ def _run_warmup() -> None: def _cache_cleanup_loop() -> None: """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 and deletes a directory only if it BOTH: + + * is not in _PRESERVED_CACHE_DIRS (torch, huggingface, locale, _roformer-models), and + * matches _CACHE_ENTRY_RE — i.e. it looks like something _job_id_for() produced. + + The second condition is the load-bearing one. This used to be a preserve-list alone, which + is a blacklist: it has to enumerate everything that must survive, so whatever it forgets + gets destroyed. It forgot `_roformer-models`, and duly deleted the 700 MB BS-Roformer-SW + checkpoint every 24 hours — users re-downloaded it on the next start, forever, with no + error and no clue why. Deleting only what we RECOGNIZE is the only version of this that + stays correct as the cache dir gains new neighbours. + Runs every 10 minutes. """ if CACHE_TTL_SECONDS is None: diff --git a/tests/test_cache_cleanup.py b/tests/test_cache_cleanup.py index 1cc1c93..e995ee8 100644 --- a/tests/test_cache_cleanup.py +++ b/tests/test_cache_cleanup.py @@ -311,7 +311,13 @@ def _literal(self, name): def _entry_re(self): # _CACHE_ENTRY_RE = re.compile(r"...") -> compile the literal pattern itself. call = self._literal("_CACHE_ENTRY_RE") + # Assert it is specifically a re.compile(...) call, not merely "a call". Otherwise a + # refactor to some other callable with the same first-argument shape would slip + # through, and this helper would compile whatever that call's first argument happened + # to be — testing a pattern the sweeper does not use. assert isinstance(call, ast.Call), "_CACHE_ENTRY_RE should be a re.compile(...) call" + assert ast.unparse(call.func) == "re.compile", ( + f"_CACHE_ENTRY_RE should be built by re.compile, got {ast.unparse(call.func)}") pattern = ast.literal_eval(call.args[0]) return re.compile(pattern)