fix: honour the requested stem set; cache every stem the model produced (#10) - #11
Merged
Merged
Conversation
…ed (#10) A 6-stem request on audio already separated into 4 returned the cached 4 — instantly, with no error. The caller asked for guitar and piano, got neither, and the response looked like a fast success. Silent and fast is the worst combination: nothing distinguishes a stale answer from an authoritative one. The cause was NOT the on-disk cache. `_check_cache` correctly requires every requested stem to be present. It was `_enqueue_job`, which short-circuits on the in-memory jobs table keyed by job_id = (audio, model) — no stem set — and returned the completed job's stems regardless of what had been asked for. Two changes: 1. Reuse a completed job only when it COVERS the request. A superset request now re-separates instead of quietly returning a short answer; a subset request is still a hit and returns only what was asked for. Jobs from before this fix (which carry `stems` but no `stems_all`) fall back to `stems`, so nothing is needlessly recomputed. An in-flight job with a smaller stem set is no longer joined — completing without the caller's stems is the same silent loss, merely delayed — and says so instead. 2. Cache EVERY stem the model produced, not just the caller's subset. Both workers already computed them all and then discarded the extras: bs_roformer_sw always emits six, and a 4-stem request threw guitar and piano away. So the *next* request for those stems paid a full ~2 min GPU inference for output we had already made and deleted. Now a 4-then-6 sequence is a genuine cache hit rather than a re-run, which is also what makes (1) cheap. Jobs now carry `stems_all` (everything produced) and `missing` (requested stems this model cannot produce — a caller asking htdemucs for `guitar` should be told, not handed a short dict and left to wonder). 7 regression tests, AST-extracted like test_cache_cleanup so they run without the torch/whisperx import chain. Verified they FAIL against pre-fix server.py rather than merely passing against the new one. Signed-off-by: topkoa <topkoa@gmail.com>
tests/test_cache_cleanup.py opened requirements.txt (and server.py, and the service file) with no encoding, so it used the platform default: UTF-8 on Linux, cp1252 on Windows. My earlier PR (#7) put an em-dash in a requirements.txt comment. That is all it takes: UnicodeDecodeError: 'charmap' codec can't decode byte 0x8f in position 184 CI stayed green the whole time, because CI is Linux. The tests have simply been broken for every Windows contributor since — which is the worst way for a test to fail: invisibly, and only for other people. Fixed all four reads in the file. Caught while adding the #10 regression tests. Signed-off-by: topkoa <topkoa@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughStem-aware cache reuse now checks completed and in-flight coverage, records requested and produced stems, caches all model outputs, handles case-insensitive cache filenames, preserves conflict status codes, and adds regression tests for coverage behavior and UTF-8 source loading. ChangesStem cache correctness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant EnqueueJob
participant JobCache
participant SeparationRunner
Client->>EnqueueJob: request audio, model, and stems
EnqueueJob->>JobCache: inspect existing job coverage
alt completed job covers request
JobCache-->>EnqueueJob: cached stem set
EnqueueJob-->>Client: requested stem subset
else no covering job
EnqueueJob->>SeparationRunner: run model separation
SeparationRunner->>JobCache: store all produced stems
JobCache-->>EnqueueJob: requested stems and missing list
EnqueueJob-->>Client: separation result
end
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness bug where /separate could return a previously-completed job’s stems from the in-memory jobs table even when the caller requested a different stem set, leading to silent missing stems (notably for superset requests like adding guitar,piano). It also improves cache efficiency by persisting all stems the model produced, enabling later subset/superset requests to benefit from prior work.
Changes:
- Update
_enqueue_jobto only reuse a completed/in-flight job when its stem coverage satisfies the current request; otherwise re-separate or return an explicit error for in-flight underspecified jobs. - Cache all produced stems in
_run_demucsand_run_roformer(stems_all) while returning only the requested subset (stems), and trackmissingstems explicitly. - Add regression tests (AST-extracted) to validate stem-set coverage logic and fix some test file-reading portability issues (explicit UTF-8).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
server.py |
Adds stem-set coverage checks for job reuse; caches all produced stems and tracks missing stems. |
tests/test_stem_set_cache.py |
New regression tests covering superset/subset/exact stem-set behavior and in-flight job joining rules. |
tests/test_cache_cleanup.py |
Makes test file reads deterministic across platforms by explicitly using UTF-8. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Two findings, one root cause: I lowercased the new cache keys but left the old lookups
using the caller's spelling. A half-migration.
1. The completed-job path checked coverage case-INSENSITIVELY, then looked values up by the
lowercased name. A job from before this fix stores `stems` keyed by the caller's ORIGINAL
casing ({"Vocals": ...}), so the check passed and the lookup found nothing: `cached: true`
with an EMPTY stems dict. A confident, instant, empty answer is worse than the bug this PR
set out to fix. Keys are now normalized before both the check and the lookup, and the
response echoes the caller's spelling.
2. _check_cache probed the caller's spelling verbatim, but the workers now write LOWERCASE
filenames — so a mixed-case `stems=` request would miss a cache entry that exists and
silently re-separate. And after a restart the in-memory jobs table is empty, so the disk
cache is the ONLY path that can find it. It now probes the lowercase name first (what we
write today), then the caller's own casing (what pre-fix entries were written with).
5 more tests (31 total).
One of the new tests initially failed on Windows only, which is worth recording: NTFS is
case-insensitive, so `(dir/"vocals.flac").exists()` is True even when the file is
`Vocals.flac` — the lowercase probe matches and we emit a lowercase URL, whereas on Linux it
misses and we emit the original. Both resolve correctly on their own platform. The assertion
was encoding the developer's OS into the test, which is exactly the class of bug that had
these tests passing in CI while broken on Windows (see the utf-8 commit).
Signed-off-by: topkoa <topkoa@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@server.py`:
- Around line 1703-1706: Update the cache-hit handling around the existing
status checks at the referenced branches to inspect the recorded missing stems
before scheduling inference. When every requested stem is known missing, return
the explicit missing-result/error response without launching either runner;
continue processing requests with genuinely incomplete or legacy cache entries,
and preserve normal reuse for available stems.
- Around line 1714-1716: Make the completed-cache miss path around the uncovered
completed job atomic: synchronize the capacity check, replacement of
jobs[job_id] with a processing job, and capacity reservation as one transition.
Ensure concurrent superset requests cannot both pass active_count, overwrite the
job, and start duplicate separations; subsequent callers must observe the
replacement as in flight.
🪄 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 Plus
Run ID: 31a833d6-403f-438e-933e-6e68f7003db2
📒 Files selected for processing (3)
server.pytests/test_cache_cleanup.pytests/test_stem_set_cache.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_stem_set_cache.py (1)
187-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate AST-extraction scaffolding.
_load_check_cachere-implements the same parse/extract/exec pattern as_load_enqueue_job. Worth extracting a shared helper (e.g._load_server_function(name, extra_ns)) to avoid maintaining two copies of this scaffolding.♻️ Sketch of a shared helper
+def _load_server_function(name, extra_ns=None): + tree = ast.parse(SERVER_PY.read_text(encoding="utf-8")) + node = next(n for n in ast.iter_child_nodes(tree) + if isinstance(n, ast.FunctionDef) and n.name == name) + mod = ast.Module(body=[node], type_ignores=[]) + ast.copy_location(mod, node) + ns = dict(extra_ns or {}) + exec(compile(ast.unparse(mod), "<test>", "exec"), ns) + return ns[name] + + def _load_check_cache(cache_dir): - tree = ast.parse(SERVER_PY.read_text(encoding="utf-8")) - node = next(n for n in ast.iter_child_nodes(tree) - if isinstance(n, ast.FunctionDef) and n.name == "_check_cache") - mod = ast.Module(body=[node], type_ignores=[]) - ast.copy_location(mod, node) - ns = { - "_cache_entry_path": lambda job_id: Path(cache_dir), - "_remember_cache_entry": lambda job_id: None, - } - exec(compile(ast.unparse(mod), "<test>", "exec"), ns) - return ns["_check_cache"] + return _load_server_function("_check_cache", { + "_cache_entry_path": lambda job_id: Path(cache_dir), + "_remember_cache_entry": lambda job_id: None, + })🤖 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_stem_set_cache.py` around lines 187 - 200, Extract the shared AST parse, function lookup, module construction, compilation, and execution logic from _load_check_cache and _load_enqueue_job into a helper such as _load_server_function(name, extra_ns). Update both loaders to call it with their function name and namespace overrides, preserving their current behavior.
🤖 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_stem_set_cache.py`:
- Around line 187-200: Extract the shared AST parse, function lookup, module
construction, compilation, and execution logic from _load_check_cache and
_load_enqueue_job into a helper such as _load_server_function(name, extra_ns).
Update both loaders to call it with their function name and namespace overrides,
preserving their current behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d4e03fcc-66f9-43eb-a887-d49d5a3cb522
📒 Files selected for processing (2)
server.pytests/test_stem_set_cache.py
🚧 Files skipped from review as they are similar to previous changes (1)
- server.py
…job race Four findings. Two of them are bugs my own fix for #10 introduced, and one is nasty. 1. UNBOUNDED RECOMPUTE (the bad one). Ask htdemucs (4-stem) for `guitar`: the job completes with guitar in `missing`. The next identical request finds `wanted` uncovered, falls through, and separates again — ~2 minutes of GPU for output that CANNOT contain guitar, because neither runner passes stem_list to the subprocess: the work is byte-for-byte identical every time. Every request pays full inference, forever. A client polling for a stem the model cannot produce is an unbounded GPU spend, and the fix for the original bug is precisely what created it. Now: if everything we lack is KNOWN-MISSING for this model, serve what exists and report what doesn't (`cached: true` + `missing`). Only a genuinely incomplete entry — a narrower previous run, or a pre-fix job that recorded no `missing` — re-separates. That distinction is what keeps the original fix intact. 2. DUPLICATE SEPARATION RACE. Two concurrent superset requests both observed the stale completed job, both passed the (non-reserving) capacity check, and both started a job for the same job_id — duplicating GPU-minutes and racing to overwrite each other's result. The decision, the capacity check and the installation of the replacement `processing` entry now happen under ONE jobs_lock hold, so the second caller sees it in flight and attaches. The thread is still started outside the lock. 3. LEGACY IN-FLIGHT JOBS. A job started by an older server has no `stem_list`, and I treated "unknown" as "covers everything" — so a superset request would attach to it and complete without the caller's stems. That reintroduces the exact silent loss this PR exists to stop, during a deploy window, where it is hardest to notice. Unknown now refuses and asks the caller to retry. 5 more tests (35 total), including the recompute loop and the duplicate-start race. Signed-off-by: topkoa <topkoa@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
server.py:1637
_check_cache()returns{}(and calls_remember_cache_entry) whenstem_listis empty becauselen(stems_found) == len(stem_list)is true. Callers treat{}as falsy and proceed as a cache miss anyway, so this is an inconsistent “hit” that can perturb cache eviction order. Consider treating an empty stem list as an immediate miss (None).
stems_found = {}
for stem_name in stem_list:
# Probe the LOWERCASE filename first (what the workers now write), then the caller's
# own casing (what pre-fix cache entries were written with). Probing only the
# caller's spelling would miss a cache entry that exists — a mixed-case `stems=`
# request would silently re-separate, and after a restart the in-memory jobs table
# is empty, so this is the ONLY path that can find it.
lower = stem_name.strip().lower()
for candidate in (lower, stem_name):
hit = None
for ext in (".mp3", ".wav", ".flac"):
p = cache_path / f"{candidate}{ext}"
if p.exists():
# The key echoes what the caller asked for; the URL points at the file
# that actually exists on disk.
hit = f"/download/{job_id}/{candidate}{ext}"
break
if hit:
stems_found[stem_name] = hit
break
if len(stems_found) == len(stem_list):
_remember_cache_entry(job_id)
return stems_found
The route mapped ANY {"error": ...} from _enqueue_job to HTTP 503. 503 means "no capacity,
back off and retry" — and clients act on that: feedBack's own splitter retries 503 with
exponential backoff.
The two conflicts this PR introduced are not capacity problems:
* a separation for this audio is already running with a SMALLER stem set;
* ...or with an UNKNOWN one (a job from an earlier server version).
Backing off does not fix either, and a client that sees 503 cannot tell them apart from a
genuinely saturated server — so it retries a condition retrying cannot help, and its
capacity-pressure signal is polluted by conflicts.
Both now return 409 Conflict; the route honours a `status_code` from the decision and keeps
503 as the default, so real capacity exhaustion is unchanged (a client SHOULD back off
there, which is precisely what 503 tells it to do).
2 tests (37 total): conflicts are 409, capacity exhaustion is still 503.
Signed-off-by: topkoa <topkoa@gmail.com>
The MAX_CONCURRENT gate read active_count under active_lock but did not increment it, and the workers incremented only once they had started. Two near-simultaneous enqueues (for different job_ids) could therefore both pass the gate before either worker got that far, and the server would briefly run MAX_CONCURRENT+1 separations. On a small GPU that isn't a queue, it's an OOM kill mid-job — the container dies and restarts, which reads like a crash rather than a capacity limit. (Pre-existing, not introduced here, but it's the same function and the same lock.) The gate now RESERVES: check-and-increment happen together, under the lock, before the worker is started. The runners no longer increment — they only release, in their `finally` — so the count can never lag behind reality. A failed thread start now releases the reservation explicitly. It is the runner's `finally` that frees a slot, and if the thread never starts, that never runs: the slot would leak permanently and the server would wedge one separation earlier, for good. 2 tests (39 total): MAX_CONCURRENT=1 admits exactly one separation, and a worker that cannot start does not leak its slot. Signed-off-by: topkoa <topkoa@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_stem_set_cache.py (2)
46-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider suppressing/documenting the
exec/compilepattern for lint hygiene.Both Ruff (S102, error) and ast-grep (no-exec/no-compile, CWE-94) flag the
exec(compile(...))call. In practice this only executes code parsed from the repo's ownserver.py, not attacker-controlled input, so the real risk is low — but the "error" severity on Ruff means this could fail a lint gate in CI. Since the AST-extraction approach is a deliberate design choice (avoiding heavyserver.pyimports like torch), consider adding an inline# noqa: S102/ suppression with a short rationale comment so the intent is explicit and CI stays green.Suggested inline suppression
+ # Safe: parses/execs only this repo's own server.py, never external input. + # noqa: S102 -- exec is required to isolate `_enqueue_job` without importing + # server.py's heavy module-level deps (torch, audio libs, etc.). exec(compile(ast.unparse(mod), "<test>", "exec"), ns)🤖 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_stem_set_cache.py` around lines 46 - 60, Add an inline lint suppression and brief rationale at the exec(compile(...)) call in the test setup, documenting that it intentionally executes AST-transformed code from the repository’s server.py to avoid importing heavy dependencies. Ensure the suppression covers Ruff S102 and the repository’s no-exec/no-compile checks without changing the test behavior.Source: Linters/SAST tools
349-363: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffTest validates eager reservation, not actual thread-safety under contention.
test_concurrent_enqueues_cannot_exceed_max_concurrentcallsenqueuetwice sequentially, so it confirms the slot is reserved before the worker thread starts, but it never exercisesactive_lockunder real concurrent access (e.g., two threads racing to check-then-incrementactive_count). A genuine race in the lock usage wouldn't be caught here.Given the difficulty of writing a reliable, non-flaky true-concurrency test, this is optional — the current test still meaningfully covers the eager-reservation regression this PR fixes.
🤖 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_stem_set_cache.py` around lines 349 - 363, The existing test only verifies sequential eager reservation, not contention on active_lock. Optionally update test_concurrent_enqueues_cannot_exceed_max_concurrent to coordinate two concurrent enqueue calls with synchronization primitives so they race through the reservation path, then assert exactly one succeeds, active_count remains 1, and only one worker starts; keep the test deterministic and non-flaky.
🤖 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_stem_set_cache.py`:
- Around line 46-60: Add an inline lint suppression and brief rationale at the
exec(compile(...)) call in the test setup, documenting that it intentionally
executes AST-transformed code from the repository’s server.py to avoid importing
heavy dependencies. Ensure the suppression covers Ruff S102 and the repository’s
no-exec/no-compile checks without changing the test behavior.
- Around line 349-363: The existing test only verifies sequential eager
reservation, not contention on active_lock. Optionally update
test_concurrent_enqueues_cannot_exceed_max_concurrent to coordinate two
concurrent enqueue calls with synchronization primitives so they race through
the reservation path, then assert exactly one succeeds, active_count remains 1,
and only one worker starts; keep the test deterministic and non-flaky.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 31f559da-0ef1-4ccb-94f0-a605e150a82c
📒 Files selected for processing (2)
server.pytests/test_stem_set_cache.py
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
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.
Fixes #10.
The bug
The caller asked for guitar and piano, got neither, and the response looked like a fast success. Nothing distinguishes a stale answer from an authoritative one — silent and fast is the worst combination.
The cause — not where the issue guessed
It is not the on-disk cache.
_check_cacheis correct: it requires every requested stem to be present and misses otherwise.It's
_enqueue_job. It short-circuits on the in-memory jobs table, keyed byjob_id = (audio_hash, model)— which deliberately does not include the stem set — and returned the completed job's stems regardless of what was asked for:The fix
1. Reuse a completed job only if it covers the request.
stemsbut nostems_all) fall back tostems, so nothing is needlessly recomputed2. Cache every stem the model produced. This one is free performance. Both workers already computed all of them and then threw the extras away:
bs_roformer_swalways emits six. A 4-stem request computed guitar and piano and deleted them — so the next request for those stems paid a full ~2 minute GPU inference for output we had already made. Now a 4-then-6 sequence is a genuine cache hit, which is also what makes fix (1) cheap rather than expensive.Jobs now carry
stems_all(everything produced) andmissing(requested stems this model cannot produce — a caller asking htdemucs forguitarshould be told, not handed a short dict and left to wonder).Tests
7 regression tests, AST-extracted like
test_cache_cleanupso they run without the torch/whisperx import chain.Verified they fail against the pre-fix
server.py, not merely pass against the new one — a regression test that doesn't bite is decoration:Impact on feedBack
The stem-splitter plugin always requests the same 6 stems, so it doesn't hit this in normal use. It surfaced while validating the Docker sidecar by hand — and it would bite anyone integrating against the API directly, or anyone who tests with
curlfirst and then wonders why the app "lost" guitar and piano.Summary by CodeRabbit
Bug Fixes
Tests