feat: "Check for update" for the managed server source - #15
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a click-triggered managed-server update flow, API routes, server source refresh and restart logic, model presence reporting, corrected warmup status labels, update tests, and 0.3.3 release metadata. ChangesManaged server lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsUI
participant routes
participant demucs_server
participant GitHub
SettingsUI->>routes: Request update check
routes->>demucs_server: check_update()
demucs_server->>GitHub: Resolve configured ref
GitHub-->>demucs_server: Return commit
SettingsUI->>routes: Submit update
routes->>demucs_server: update_server()
demucs_server->>demucs_server: Refresh source and verify install
demucs_server-->>routes: Return status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Folded in: the chips were lying about downloadsWhile testing the update path, the reporter restarted the app and reported "it's still downloading". It wasn't. Nothing crossed the wire: The status was lying. The server marks a model _set_warmup_state("bs_roformer_sw", "downloading") # ...then loads from cacheSo a few-second RAM load is displayed as a download. The user saw "downloading" against models whose weights were sitting on disk and concluded they'd been thrown away again. They were right to believe the UI; the UI was wrong. What made it so convincing is that it sat on top of two bugs that genuinely were deleting weights (the 24h cache sweeper, and the warmup gate that only checked the roformer checkpoint). The status had been telling the truth right up until it wasn't — so the one time it lied, it was completely credible.
Done plugin-side deliberately: fixing the label in Verified against the reporting machine: |
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 `@demucs_server.py`:
- Around line 550-588: Update update_server so every exception after stopping a
previously running server triggers a restart attempt before propagating the
original failure. Wrap the download, launcher/driver regeneration, metadata, and
success-flow operations in exception handling, and in the failure path call
start_server with warmup=models_downloaded(config_dir) and the existing progress
callback when was_running is true; preserve the original exception even if
recovery also fails.
🪄 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: 883866ba-2234-4ea7-8f38-99169a650f52
📒 Files selected for processing (6)
CHANGELOG.mddemucs_server.pyplugin.jsonroutes.pysettings.htmltests/test_models_downloaded.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
04062fb to
949fc1a
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds an explicit “Check for update” flow for the plugin-managed local demucs server so existing installs can refresh only the server source (not multi‑GB deps/models), and improves the status UI to better distinguish true downloads from warmup loading cached weights.
Changes:
- Add Settings UI + backend routes to check GitHub for a newer managed-server revision and update the installed server source on demand.
- Improve model readiness reporting: all-or-nothing warmup gating, torch hub cache migration, and clearer warmup chip labeling (“loading” vs “downloading”).
- Add unit tests covering models-downloaded gating and torch cache migration behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_models_downloaded.py | Adds tests for warmup gating, missing-model reporting, and torch cache migration idempotency. |
| settings.html | Adds “Check for update” button + client flow; refines warmup chip labeling using models_present. |
| routes.py | Adds /server/check_update and /server/update endpoints to trigger update operations. |
| plugin.json | Bumps plugin version to 0.3.3. |
| demucs_server.py | Implements update check/update workflow; tightens model presence checks; migrates TORCH_HOME layout; adds models_present to status. |
| CHANGELOG.md | Documents the 0.3.3 update feature and status-chip improvements. |
Comments suppressed due to low confidence (1)
demucs_server.py:1637
models_present.whisperxis currently true when the faster-whisper snapshot exists, but/health.warmup.whisperx(per upstream contract) represents “ASR + en aligner”. This can make the UI relabel an actual network download (aligner missing, state isdownloading) as “loading” just because the ASR weights are present.
else:
os.kill(pid, sig)
try:
_sig(signal.SIGTERM)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Two findings on #15. 1. update_server() stopped a RUNNING server and then ran download_source() / write_launcher() / patch_driver_scripts() with no exception handling. A network drop mid-download, a half-extracted archive, or a locked file on Windows would propagate straight out — skipping the restart entirely. The user clicks a button meant to FIX something and ends up with a server that was healthy before the click and is now down, over a network blip. Strictly worse than not clicking. The fetch is now wrapped: on failure the server is restarted as it was, and the error is re-raised so the UI still reports it. Whatever is on disk is startable — the launcher and the driver bootstrap are regenerated from our own templates on every start. 2. "Did the revision change?" compared 8-character sha prefixes. Two commits can share one, and reporting "already up to date" for an update that actually happened is a lie the user has no way to check. Compares full shas now; still displays short ones. Verified END TO END against the reporting machine's live install — the path I said I had not exercised: BEFORE source 94a1c30f (Jul 7) · sweeper fix in server.py: False · server running stop -> fetch -> re-bootstrap drivers -> restart AFTER source 02bd7cda (current) · sweeper fix: True · drivers bootstrapped: True 15s, nothing large fetched That install had been silently running a July 7 server: no diffq fix, no crash-loop fix, no stem-cache fix, and the cache sweeper that deletes 1 GB of model weights every night. One click now brings it current, which is the entire reason this PR exists. Signed-off-by: topkoa <topkoa@gmail.com>
949fc1a to
f020b88
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
…the docstring promised Two findings on #15. 1. update_server() discarded the port and restarted on DEFAULT_PORT. Anyone running the server on a configured port would have it moved to 7865 by an update — or collide with whatever is already there — leaving a working setup down or unreachable, after clicking a button meant to help. My live test could not have caught this: I use the default port. The route now passes the configured port/device/model through, and both restart paths (success and the failure rollback) use them. 2. The docstring claimed "if requirements.txt changes, verify_install() will say so" — and update_server() never called verify_install(). A promise made only to the reader. Now it really runs, after the source refresh and BEFORE the restart. A newer revision can need a dependency the installed pylibs tree lacks; if the imports no longer hold we say so in one sentence ("click Install server + models to refresh them") and do NOT restart. A server that cannot import its own dependencies would just crash-loop, and "it keeps restarting" is a far worse message than "the update needs new dependencies". Re-verified live: fetch -> re-bootstrap drivers -> verify imports -> "already up to date", 8s, no restart (it wasn't running), no reinstall needed. Signed-off-by: topkoa <topkoa@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
routes.py (1)
1013-1058: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a lifespan hook for this shutdown cleanup.
@app.on_event("shutdown")is deprecated in FastAPI/Starlette; if this plugin can access the app factory, move this into alifespanwrapper. If it only receives an already-builtapp, keep this handler as the practical fallback.🤖 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 `@routes.py` around lines 1013 - 1058, Move the managed-server cleanup from the deprecated _stop_managed_server shutdown event into the application factory’s lifespan wrapper, running the existing bounded stop logic during lifespan shutdown. Preserve the state-file check, three-second join, watchdog fallback, and exception logging; if this plugin only receives an already-built app and cannot configure lifespan, retain _stop_managed_server as the fallback.
🤖 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 `@demucs_server.py`:
- Around line 582-607: Update the network-failure return in check_update() to
include the same "unknown": not have field as the successful return, preserving
the signal for installs without a recorded commit across all installed-result
paths.
- Around line 1893-1904: Update server_status() to compute the model-presence
checks once and reuse those results when constructing both models_downloaded and
models_present. Keep models_downloaded() unchanged for its other callers, and
preserve the existing per-model status values while eliminating the duplicate
filesystem scans in this polling path.
---
Nitpick comments:
In `@routes.py`:
- Around line 1013-1058: Move the managed-server cleanup from the deprecated
_stop_managed_server shutdown event into the application factory’s lifespan
wrapper, running the existing bounded stop logic during lifespan shutdown.
Preserve the state-file check, three-second join, watchdog fallback, and
exception logging; if this plugin only receives an already-built app and cannot
configure lifespan, retain _stop_managed_server as the fallback.
🪄 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: ff2928fa-90bc-404e-8401-8c460223fc55
📒 Files selected for processing (5)
CHANGELOG.mddemucs_server.pyplugin.jsonroutes.pysettings.html
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin.json
Two findings on #15. 1. update_server() stopped a RUNNING server and then ran download_source() / write_launcher() / patch_driver_scripts() with no exception handling. A network drop mid-download, a half-extracted archive, or a locked file on Windows would propagate straight out — skipping the restart entirely. The user clicks a button meant to FIX something and ends up with a server that was healthy before the click and is now down, over a network blip. Strictly worse than not clicking. The fetch is now wrapped: on failure the server is restarted as it was, and the error is re-raised so the UI still reports it. Whatever is on disk is startable — the launcher and the driver bootstrap are regenerated from our own templates on every start. 2. "Did the revision change?" compared 8-character sha prefixes. Two commits can share one, and reporting "already up to date" for an update that actually happened is a lie the user has no way to check. Compares full shas now; still displays short ones. Verified END TO END against the reporting machine's live install — the path I said I had not exercised: BEFORE source 94a1c30f (Jul 7) · sweeper fix in server.py: False · server running stop -> fetch -> re-bootstrap drivers -> restart AFTER source 02bd7cda (current) · sweeper fix: True · drivers bootstrapped: True 15s, nothing large fetched That install had been silently running a July 7 server: no diffq fix, no crash-loop fix, no stem-cache fix, and the cache sweeper that deletes 1 GB of model weights every night. One click now brings it current, which is the entire reason this PR exists. Signed-off-by: topkoa <topkoa@gmail.com>
…the docstring promised Two findings on #15. 1. update_server() discarded the port and restarted on DEFAULT_PORT. Anyone running the server on a configured port would have it moved to 7865 by an update — or collide with whatever is already there — leaving a working setup down or unreachable, after clicking a button meant to help. My live test could not have caught this: I use the default port. The route now passes the configured port/device/model through, and both restart paths (success and the failure rollback) use them. 2. The docstring claimed "if requirements.txt changes, verify_install() will say so" — and update_server() never called verify_install(). A promise made only to the reader. Now it really runs, after the source refresh and BEFORE the restart. A newer revision can need a dependency the installed pylibs tree lacks; if the imports no longer hold we say so in one sentence ("click Install server + models to refresh them") and do NOT restart. A server that cannot import its own dependencies would just crash-loop, and "it keeps restarting" is a far worse message than "the update needs new dependencies". Re-verified live: fetch -> re-bootstrap drivers -> verify imports -> "already up to date", 8s, no restart (it wasn't running), no reinstall needed. Signed-off-by: topkoa <topkoa@gmail.com>
…onotonic server_status() is the poll endpoint — hit every few seconds, and required to stay offline and cheap. It called models_downloaded() (which is exactly the AND of the three presence checks) and then called all three AGAIN for models_present, so every check ran twice per poll, including _has_whisper()'s rglob() over the whole faster-whisper snapshot tree. Walk once and derive the flag from the dict, which also makes the two fields agree by construction instead of by two independent walks happening to concur. update_server() emitted 0.86 for the dependency check and then 0.85 for the result, so the bar visibly ran backwards. check_update()'s GitHub-unreachable branch omitted `unknown`, handing callers a different shape on one path than on every other. Tests: update_server was orchestration with no coverage — which is why review, not use, caught the dropped port (my own live test runs on the default port, so it could not have failed). Now locked: monotonic progress, restart on the CONFIGURED port/device/model, a failed update puts the server back rather than leaving it down, deps that no longer import don't get restarted, and the cache is walked once. Counting only calls for our own cache dir — the routes tests leave a status poller on a daemon thread that calls the same mocks. All three found by Copilot and CodeRabbit on #15. Signed-off-by: topkoa <topkoa@gmail.com>
10a68e8 to
c5134a8
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
Two findings on #15. 1. update_server() stopped a RUNNING server and then ran download_source() / write_launcher() / patch_driver_scripts() with no exception handling. A network drop mid-download, a half-extracted archive, or a locked file on Windows would propagate straight out — skipping the restart entirely. The user clicks a button meant to FIX something and ends up with a server that was healthy before the click and is now down, over a network blip. Strictly worse than not clicking. The fetch is now wrapped: on failure the server is restarted as it was, and the error is re-raised so the UI still reports it. Whatever is on disk is startable — the launcher and the driver bootstrap are regenerated from our own templates on every start. 2. "Did the revision change?" compared 8-character sha prefixes. Two commits can share one, and reporting "already up to date" for an update that actually happened is a lie the user has no way to check. Compares full shas now; still displays short ones. Verified END TO END against the reporting machine's live install — the path I said I had not exercised: BEFORE source 94a1c30f (Jul 7) · sweeper fix in server.py: False · server running stop -> fetch -> re-bootstrap drivers -> restart AFTER source 02bd7cda (current) · sweeper fix: True · drivers bootstrapped: True 15s, nothing large fetched That install had been silently running a July 7 server: no diffq fix, no crash-loop fix, no stem-cache fix, and the cache sweeper that deletes 1 GB of model weights every night. One click now brings it current, which is the entire reason this PR exists. Signed-off-by: topkoa <topkoa@gmail.com>
…the docstring promised Two findings on #15. 1. update_server() discarded the port and restarted on DEFAULT_PORT. Anyone running the server on a configured port would have it moved to 7865 by an update — or collide with whatever is already there — leaving a working setup down or unreachable, after clicking a button meant to help. My live test could not have caught this: I use the default port. The route now passes the configured port/device/model through, and both restart paths (success and the failure rollback) use them. 2. The docstring claimed "if requirements.txt changes, verify_install() will say so" — and update_server() never called verify_install(). A promise made only to the reader. Now it really runs, after the source refresh and BEFORE the restart. A newer revision can need a dependency the installed pylibs tree lacks; if the imports no longer hold we say so in one sentence ("click Install server + models to refresh them") and do NOT restart. A server that cannot import its own dependencies would just crash-loop, and "it keeps restarting" is a far worse message than "the update needs new dependencies". Re-verified live: fetch -> re-bootstrap drivers -> verify imports -> "already up to date", 8s, no restart (it wasn't running), no reinstall needed. Signed-off-by: topkoa <topkoa@gmail.com>
…onotonic server_status() is the poll endpoint — hit every few seconds, and required to stay offline and cheap. It called models_downloaded() (which is exactly the AND of the three presence checks) and then called all three AGAIN for models_present, so every check ran twice per poll, including _has_whisper()'s rglob() over the whole faster-whisper snapshot tree. Walk once and derive the flag from the dict, which also makes the two fields agree by construction instead of by two independent walks happening to concur. update_server() emitted 0.86 for the dependency check and then 0.85 for the result, so the bar visibly ran backwards. check_update()'s GitHub-unreachable branch omitted `unknown`, handing callers a different shape on one path than on every other. Tests: update_server was orchestration with no coverage — which is why review, not use, caught the dropped port (my own live test runs on the default port, so it could not have failed). Now locked: monotonic progress, restart on the CONFIGURED port/device/model, a failed update puts the server back rather than leaving it down, deps that no longer import don't get restarted, and the cache is walked once. Counting only calls for our own cache dir — the routes tests leave a status poller on a daemon thread that calls the same mocks. All three found by Copilot and CodeRabbit on #15. Signed-off-by: topkoa <topkoa@gmail.com>
…ef in one place Four from review, all real: - update_server() forwarded progress_cb straight into download_source() and start_server(). Those are whole operations with their own 0→1 progress, so they emit ~0.02 first — the bar leapt back to 2% right after we reported 20%, three times per update. Now scaled into slices via the _scaled() that already existed for setup_server. (My first attempt added a SECOND _scaled with (lo, hi) semantics; the existing one is (base, span) and shadowed it, which the monotonic test caught — the duplicate is gone.) - The restart used the port from settings while the comment claimed it used the one the server was actually running on. Those disagree exactly when the user edits the port without restarting — and putting the server back somewhere other than where we found it is the failure this argument was threaded through to prevent. It now reads the live port from is_running() and falls back to the configured one. - check_update() trimmed the ref; update_server() passed the raw settings value through. Same input, two behaviours: a ref with stray whitespace would report an update available and then fail to apply it. One _norm_ref() now, used by both. - The test helper stopped its patches in start order. A test that patches the same attribute twice has the second patch capture the first MOCK as its original, so unwinding forwards restores a MagicMock onto the module and leaks it into every test that runs after. LIFO now, with a test asserting the module is intact afterwards. The progress stubs used to emit nothing, which is why the monotonic test passed while the bar was visibly jumping backwards: they now emit their own 0→1 like the real calls do. Verified it FAILS against verbatim forwarding. Found by Copilot and CodeRabbit on #15. Signed-off-by: topkoa <topkoa@gmail.com>
- The "loading vs downloading" chip fix didn't work for the model it mattered most for. The server reports the default separator under the warmup key `demucs` (an alias for bs_roformer_sw — see _model_ready), while models_present keys it bs_roformer_sw, so present[k] missed and the chip stayed amber for a 700 MB file sitting right there. That is the exact bug this change exists to fix, reintroduced through a key mismatch. Mapped now, with unmapped keys falling back to themselves rather than guessing. - If the update failed AND the restore failed, only the log heard about it. The user's last message said "restarting the server as it was", so silence leaves them believing the old server is still up when it is actually down — the one state they cannot see. It now says so. - _norm_ref()'s docstring claimed to be the one place a ref is normalized while download_source() did its own. Made the claim true rather than weakening it. - RUF001: the multiplication sign in a test message is now an ASCII x. Found by Copilot and CodeRabbit on #15. Signed-off-by: topkoa <topkoa@gmail.com>
The model was threaded through Update and nowhere else. So a user who changed the split model got that model on a restart-after-update and DEFAULT_MODEL on a plain Start — the same server warming a different model depending on which button they pressed, and a split whose behaviour depends on how the server happened to come up. Prepare models had it worse: the explicit multi-GB fetch would download the weights for a model the user isn't using. _server_opts() now returns the whole triple and Start, Update, Install, Prepare models and autostart all take it from there. The point isn't the four call sites I fixed; it's that the next lifecycle path can't forget one. Tests drive the real routes over a real settings file (a mock spanning only setup() is gone by the time the button is pressed) and fail if Start drops the model. Found by Copilot on #15. Signed-off-by: topkoa <topkoa@gmail.com>
de1eb78 to
0791e42
Compare
The UI seeds a percentage for the step it is about to take (10% "Checking", 20% "Updating"), and then the server's own stream legitimately opens lower — update_server() emits 0.05 for "Stopping the server" when one is running. The bar jumped forward and then back, which reads as the operation having restarted. setSrvBar() now clamps: within an operation the bar only moves forward. `reset` marks the two cases where moving down is correct — a new operation starting, and a terminal state (Done / Failed / Busy / Up to date). The reconnect snapshot resets too: it is authoritative for whatever op is live now, which may not be the one whose floor we were carrying. Enforcing it in the setter rather than hand-tuning the seeds is the difference between fixing this and fixing it again the next time a step is inserted — which is exactly what happened on the backend side of this same bar two rounds ago. Found by Copilot on #15. Signed-off-by: topkoa <topkoa@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_update_server.py (1)
229-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd route-level coverage for
/server/updateand/server/install.This class's docstring claims Start, Update, Install, Prepare models and autostart all now share
_server_opts(), but only/server/startand/server/prepare_modelsare actually driven throughTestClienthere./server/updateis only exercised at theupdate_server()function level via_Stubbed(bypassingroutes.py's_server_opts()wiring entirely), and/server/installisn't exercised at the route level at all. A regression in how_server_opts()'s output is threaded into those two routes (e.g. theportdefault gap flagged inroutes.py) would go undetected by this suite.✅ Suggested additional test
def test_update_uses_the_configured_port_device_and_model(self): seen = {} settings = dict(SETTINGS, remote_model="htdemucs", local_server_port=9001, local_server_device="cuda") with tempfile.TemporaryDirectory() as td: client = self._routes(td, settings) with mock.patch.object(demucs_server_mod, "update_server", side_effect=lambda cfg, **kw: seen.update(kw) or {}): client.post(f"{P}/server/update") _settle(seen) self.assertEqual(seen.get("model"), "htdemucs") self.assertEqual(seen.get("port"), 9001) self.assertEqual(seen.get("device"), "cuda")🤖 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_update_server.py` around lines 229 - 277, Add route-level tests in EveryLifecyclePathRunsTheSameServer for /server/update and /server/install, driving both through TestClient and patching the corresponding demucs_server_mod functions. Assert each receives the configured model, local_server_port, and local_server_device from _routes settings, preserving the existing async settling pattern and covering _server_opts() wiring.
🤖 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 `@demucs_server.py`:
- Around line 732-791: The verify_install failure path leaves the newly
downloaded source in place without preserving the previous installation. Update
the surrounding update flow to snapshot the existing src_dir(config_dir) before
download_source, and restore that snapshot when dependency verification fails
before returning the needs_reinstall status. Remove the snapshot only after
verification succeeds, while preserving the existing status flags and
non-restart behavior.
In `@routes.py`:
- Around line 663-675: Update _server_opts() so the port returned by
_as_port(s.get("local_server_port")) falls back to demucs_server.DEFAULT_PORT
when the persisted value is invalid or absent, ensuring all lifecycle paths
receive a valid default port instead of None.
---
Nitpick comments:
In `@tests/test_update_server.py`:
- Around line 229-277: Add route-level tests in
EveryLifecyclePathRunsTheSameServer for /server/update and /server/install,
driving both through TestClient and patching the corresponding demucs_server_mod
functions. Assert each receives the configured model, local_server_port, and
local_server_device from _routes settings, preserving the existing async
settling pattern and covering _server_opts() wiring.
🪄 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: 22fc3c85-7e45-4970-bc60-00e1142b476d
📒 Files selected for processing (6)
CHANGELOG.mddemucs_server.pyplugin.jsonroutes.pysettings.htmltests/test_update_server.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
There was a problem hiding this comment.
♻️ Duplicate comments (1)
demucs_server.py (1)
777-791: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftVerify-install failure still leaves the overwritten source unrolled-back (unresolved from a prior review).
download_source()(Line 740) has already overwrittensrc_dir(config_dir)in place by the timeverify_install()runs here. If verification fails, this branch reportsneeds_reinstalland returns — but nothing restores the previous, working source files, and (per the docstring at lines 715-718) the server is deliberately not restarted. Ifwas_runningwas true, a healthy running server is now stopped with its on-disk source replaced by one that can't import its own dependencies, recoverable only via a full "Install server + models" reinstall. This is the same gap flagged in a past review round on this function that doesn't show a corresponding fix in the current diff (unlike every sibling comment in that thread, which has a "✅ Confirmed as addressed" follow-up).🔧 Sketch: snapshot the source files before overwrite, restore them here
try: _emit(progress_cb, "Fetching the latest server source…", 0.2, "Updating") + sdir = src_dir(config_dir) + backup = {f.name: f.read_bytes() for f in sdir.iterdir() if f.is_file()} download_source(config_dir, ref=ref, progress_cb=_scaled(progress_cb, 0.2, 0.55)) write_launcher(config_dir) patched = patch_driver_scripts(config_dir) if patched: _emit(progress_cb, f"Re-bootstrapped {', '.join(patched)}.", 0.8, "Updating") except Exception as e: ... raise try: _emit(progress_cb, "Checking the updated server's dependencies…", 0.86, "Updating") verify_install(config_dir, progress_cb=None) except Exception as e: + for name, data in backup.items(): + (sdir / name).write_bytes(data) + write_launcher(config_dir) + patch_driver_scripts(config_dir) _emit(progress_cb, f"The updated server needs dependencies this install doesn't have ({e}). " - f"Click 'Install server + models' to refresh them. Not restarting, because a " - f"server that can't import its own deps would only crash-loop.", + f"Rolled back to the previous source. Click 'Install server + models' if you " + f"still want this update.", 1.0, "Needs reinstall") + if was_running: + try: + start_server(config_dir, port=port, device=device, model=model, + warmup=models_downloaded(config_dir), progress_cb=None) + except Exception as restart_error: + log.warning("stem_splitter: rollback restart failed: %s", restart_error) st = server_status(config_dir) st["updated"] = True st["needs_reinstall"] = True return stWould appreciate confirming whether
setup_server()(not in this diff) fully recovers this state today, since that governs how urgent the rollback is versus just documenting the risk.#!/bin/bash rg -n "def setup_server|def src_dir|def write_launcher|def patch_driver_scripts" -A 15 demucs_server.py🤖 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 `@demucs_server.py` around lines 777 - 791, Update the source-update flow around download_source() and the verify_install() failure handler to snapshot the existing src_dir(config_dir) contents before overwriting them, then restore that snapshot when verification fails. Preserve the existing needs_reinstall status and non-restart behavior, while ensuring a previously working source remains on disk for running or stopped servers.
🤖 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.
Duplicate comments:
In `@demucs_server.py`:
- Around line 777-791: Update the source-update flow around download_source()
and the verify_install() failure handler to snapshot the existing
src_dir(config_dir) contents before overwriting them, then restore that snapshot
when verification fails. Preserve the existing needs_reinstall status and
non-restart behavior, while ensuring a previously working source remains on disk
for running or stopped servers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bb58a956-e2d0-4d01-82f2-036f87e93652
📒 Files selected for processing (6)
CHANGELOG.mddemucs_server.pyplugin.jsonroutes.pysettings.htmltests/test_update_server.py
🚧 Files skipped from review as they are similar to previous changes (3)
- plugin.json
- CHANGELOG.md
- routes.py
… the model we started - download_source() overwrites the source tree IN PLACE, so by the time verify_install() found that the new revision needs a dependency this install doesn't have, the old working source was already gone. The user was left with a stopped server and the only source on disk being one that cannot run — strictly worse than never clicking, and not recoverable from the button they clicked. The tree is now snapshotted first (a few hundred KB), restored on failure, and the server restarted, so a failed update costs nothing but time. A failed FETCH rolls back too: a half-extracted tree is the same trap. - server_status() asked /health whether DEFAULT_MODEL was warm, but the lifecycle paths now start the server with the CONFIGURED model. A user on a non-default model would warm it fully and models_ready would stay false forever — the UI never saying "Warm · ready to split" while polling a server that is, in fact, ready. Fallout from threading the model through in the previous commit; the route now passes the same model it starts with. Found by CodeRabbit and Copilot on #15. Signed-off-by: topkoa <topkoa@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@demucs_server.py`:
- Around line 707-708: Update the snapshot and rollback flow so an in-place
update aborts before downloading when snapshot creation returns None or
otherwise fails. In each failure path around _restore_source(), delete the
backup only after successful restoration; retain it when restoration fails so
recovery remains possible.
- Around line 700-718: Update the source snapshot and rollback flow around
_snapshot_source, download_source, and the restore logic to include
demucs-server/source.json alongside the source tree. Restore the metadata
whenever the source snapshot is restored, and add a regression assertion
verifying that source.json’s recorded commit matches the rolled-back source
rather than the rejected update.
- Line 862: Update the successful update-response path to call server_status
with the active model, matching the verification-failure path. Apply the same
change to the additional referenced response path so non-default model updates
report the correct models_ready state.
🪄 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: b7dee348-9ddd-47ab-bdf6-06e40e700bd8
📒 Files selected for processing (3)
demucs_server.pyroutes.pytests/test_update_server.py
🚧 Files skipped from review as they are similar to previous changes (1)
- routes.py
…t a way back Four follow-ups, all fallout from the rollback and model-threading commits: - source.json was not in the snapshot. download_source() rewrites it, so restoring the tree without it left the recorded commit pointing at the revision we had just REJECTED — check_update() would then report "up to date" and stop offering the update, running the old source under a new name and putting the fix the user was reaching for out of reach of the button that exists to reach it. The snapshot now covers the install (tree + recorded commit), and a test fails if only the tree comes back. - A snapshot that could not be taken let the update proceed anyway, which is the exact failure the snapshot exists to prevent. It now refuses: nothing has been touched at that point (the server is still running), so refusing costs only the click. - Both failure paths deleted the backup even when the restore FAILED — destroying the only intact copy of a working install the user had left. The backup now survives a failed restore. - server_status() reported models_ready for DEFAULT_MODEL on the success path (and in start/setup/prepare), while the lifecycle paths start the CONFIGURED model. Every path that knows the model now says which one it means; the two that can't (stop, uninstall) still default. Found by CodeRabbit and Copilot on #15. Signed-off-by: topkoa <topkoa@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
The gap
The server's
server.pyis downloaded at install time and never touched again. So a bug fixed upstream cannot reach anyone who has already installed. The only route was: uninstall, then re-download several GB of wheels — to pick up a one-line change. Nobody does that. In practice the fix never lands at all.This is not hypothetical. On the machine that reported the daily ~1 GB re-download:
Their server predates the
diffqbuild fix (#7), the crash-loop fix (#9), the AGPL license (#8), and the stem-cache fix (#11). They had no way to know, and no proportionate way to fix it.It also blocks the fix for the bug they actually reported: got-feedBack/feedBack-demucs-server#13 stops the 24h sweeper deleting the 700 MB roformer checkpoint — and that fix lives in
server.py, so without this button it would reach nobody who already installed.What it does
Check for update → resolves the pinned ref to a commit, compares with what's on disk, and updates only if there's something newer.
The update re-fetches the source only (a few hundred KB):
run_demucs.py/run_roformer.py, so without this the server comes back up unable to import its own dependencies;pylibs/and the model cache alone, so nothing is re-downloaded.Two steps, on purpose
server_status()is polled every few seconds and must stay offline and cheap — putting a network call there would be a quiet regression.Merge order
There are three plugin PRs open, each with a single version bump (one per PR):
Merging in that order keeps the numbers honest; any other order and I'll rebase.
Summary by CodeRabbit