Repository navigation
Keep audio alive in a hidden tab, and stop trusting downloads that never finished - #606
Merged
Merged
Conversation
added 5 commits
September 9, 2026 07:54
Browsers do not deliver requestAnimationFrame to a hidden tab, and a frame already pending when the tab hides is dropped rather than deferred. Both engines drove their tick straight off rAF, so switching tabs stopped the tick with no callback left to notice it. In chunkedAudioEngine that is fatal. _tick() is the only caller of _maybeSchedule(), so chunk scheduling stops with it, the LOOKAHEAD_SEC of 12 already queued plays out, and the track goes silent about fifteen seconds after the user switches away. No error, no event, nothing in the console. That is the reported symptom. audioEngine keeps sounding, because every source is scheduled up front, but it stops noticing loop ends and end-of-track until the user comes back. Adds static/js/tickLoop.js: a loop that runs on rAF while visible and falls back to setTimeout while hidden, with one document-level visibilitychange listener shared by every live loop. The listener is what supplies the callback the dropped frame would have been. Three things this gets right that a per-engine copy of the same idea does not: - dispose() deregisters. A loop left in the set is reachable from document, which outlives every engine, so the closure and every decoded buffer it captured could never be collected. That is a leak per track switch, and the buffers are the largest thing the app holds. - The cancel path follows the clock that scheduled it. A timeout id handed to cancelAnimationFrame is not an error, it just does nothing, and the loop keeps ticking after the engine believes it stopped. - One listener, not one per engine instance. Also in this commit, both found while fixing the above: - audioEngine.destroy() never cleared `playing`. destroyPlayer() tears the engine down without pausing first, so an engine destroyed mid-playback left the flag set and isPlaying() lied about a dead engine. - The statechange listener that recovers a context suspended by a backgrounded tab is removed on teardown. A caller-supplied context (the mobile UI passes one) outlives the engine and would otherwise collect a dead listener per track opened. tests/js/tick-loop.test.mjs covers the hide transition, the cancel path, dispose deregistration, and that a paused or disposed loop is never restarted. Verified it fails without the fix: the hide-transition check reports timers=0 rafs=1, the loop waiting on a frame that never comes. The two engine tests needed addEventListener on their fake contexts. A real AudioContext is an EventTarget; the fakes were incomplete stand-ins. Not covered by the e2e suite: its fixture track is 6 seconds, so a 12 second lookahead starving cannot be reproduced against it at all.
Each pip pass got a flat 20 minutes from spawn, enforced by a deadline that had no idea whether the child was making progress. The Linux CUDA runtime pass moves about 3 GB, so that budget was really an undeclared requirement for a sustained 2.5 MB/s, and nothing said so. A reporter's two runs are the whole argument. Same cudnn wheel, same machine, nothing changed between them: run 1 921s and still downloading, killed at the 1200s cap (0.77 MB/s) run 2 193s, whole pass finished ~170s inside the cap (3.66 MB/s) The second run did not fix anything. Their connection was 4.7x faster. On the first run's speed nobody ever gets there, so the cap is the bug. Replaces the fixed deadline with a stall deadline for streamed children: pip emits a Progress line continuously while bytes are moving, so a gap in those lines is the only honest signal that it is wedged. Five minutes of silence gives up; a four hour backstop still bounds a child that chatters forever but never finishes. This is the pattern the project already mandates. python-fastapi.md calls for a stall watchdog on long subprocesses and separate.py implements one for Demucs with TIMEOUT_DEMUCS_STALL. The Rust side was the one place that used a wall clock instead. Deadline is an enum rather than an extra argument so each call site says which measurement it wants. child_output_with_timeout keeps Fixed, so GPU_VERIFY_TIMEOUT and the #583 grandchild handling are untouched; child_output_streaming has exactly one caller, the pip install, and is the only thing that becomes stall-based. The timeout message now says which deadline fired. "timed out after 1200 seconds" on a download that was still moving is what sent the reporter looking for a hang that was never there. Also fixes a wrong reason string on the same path. When every candidate wheel failed to install, the run was recorded as "cuda-verify-failed" even though verify was never reached. That is the first line anyone reads in a bug report. Installs that never got that far now say "cuda-install-failed". Tests, unix-gated because they need sh: a child printing steadily for 10 seconds against a 2 second stall budget must finish untouched (this is the regression, and under the old code it is killed at 2 seconds), and a child that prints once then sleeps 300 must be given up on inside 30, with a message that says it went quiet. Verified under WSL, since #[cfg(unix)] never compiles on the Windows host.
…exists A model file that arrived truncated is never retried. torch.hub decides whether to download purely on os.path.exists, and download_url_to_file moves its result into place without comparing against Content-Length; check_hash defaults to False and beat_this does not pass it. audio-separator has the same shape, which is why a bad file there surfaces as an MD5 matching nothing. So a connection that drops mid-body and closes cleanly is promoted to a complete checkpoint, and every run afterwards finds it present and stops there. The failure is permanent and silent. beat_detect caches _model_failed for the process and falls back to librosa, which looks exactly like a machine that never had the model, so the user gets a permanently worse beat grid and nothing anywhere connects the two. A reporter has been running in that state since their install (#502): their log shows beat_this and vocal_split both failing 25 seconds into warmup, far too fast to be a download. Adds app/core/model_cache.load_or_heal: run the load, and if it fails and there was a cached artifact to blame, remove it and try exactly once more. Both load sites go through it, so a bad file heals whether it is hit during setup warmup or mid-job. Two deliberate limits: - The retry is conditional on having actually removed something. If nothing was cached the load failed for another reason, most likely offline, and a second attempt only pays the same network timeout again to reach the same answer, on the machine least able to afford it. - ImportError is re-raised untouched. The package is missing; deleting model files cannot help, and would throw away a good cache. The "falls back to lazy-download-on-first-use" promise in warmup.py's docstring was true only for an empty cache, never for a wrong one. Corrected there rather than left as a comment describing a design that does not hold. Also surfaces the result in the setup wizard. warmup_models already returned a per-model ModelWarmupStatus and setup.js threw it away, so a user whose beat model never arrived saw a clean setup and no reason to suspect anything. It now names what did not download and says it will be fetched on first use. That file has no i18n layer (verified: no import, all strings hardcoded), so these strings match the rest of it. uv.lock is untouched, so the desktop in-app updater is unaffected. tests/test_model_cache.py covers the truncated-then-healed path, that an empty cache is not retried, that ImportError leaves a good cache alone, that healing is one retry and not a loop, and that a cache directory is removed whole. The 8 failures currently in test_click_render.py and test_transpose_export.py are pre-existing on this machine (ffmpeg is not on PATH) and were confirmed failing on a clean HEAD.
The missing-models message was written to the status line, and the backend step that runs immediately after calls setStatus again and then navigates away to the backend URL. It was never on screen long enough to read. Keeps the console.warn, which does reach the log people attach to bug reports. Telling the user properly belongs in the main UI next to the feature that is degraded, not in a wizard that is about to disappear.
Every track that plays through the Web Audio engine logged one MEDIA_ELEMENT_ERROR "Empty src attribute" per stem, six lines, before the user had done anything. The elements really do have no source, and that is correct: when the engine owns playback the multitrack is built with url: null for every stem, because the engine streams the audio itself and the multitrack is only there for the lanes. The guard that was supposed to skip those tested stemsByName[name].url instead, which is the original descriptor and still holds the real URL. So it passed, attached an error listener to an element that was deliberately never given a src, and the element duly reported one. Adds useEngine to the test, mirroring the condition that nulls the URLs in the first place. Playback was never affected. The cost was that anyone reading a console, or attaching one to a bug report, saw six errors that had nothing to do with their problem. It showed up while verifying #600 and is unrelated to it.
2 tasks done
The tick-loop half of this branch came back clean. These are all in the model-cache and pip-timeout halves. The setup warning never fired. ModelWarmupStatus is #[serde(rename_all = "camelCase")] and setup.js read the Rust field names, so all four were undefined, `missing` was empty every time, and the commit that added it did nothing at all. It was verified by tests that never ran the IPC and by reading the Rust struct's fields rather than its wire format. The old-pip retry path was made worse than before. `--progress-bar raw` only exists in pip 24.1+, and the retry that drops it writes nothing to a pipe for the whole download. A stall budget measures silence, and silence is that path's normal state, so a healthy 3 GB transfer would now be killed at five minutes where it previously had twenty. That path gets a plain deadline again, generous rather than clever, because there is genuinely nothing to measure. vocal_split_artifacts named "model_data.json". audio-separator writes "vr_model_data.json" and "mdx_model_data.json". The index is listed precisely because a truncated model surfaces as an unknown MD5, which is a lookup against those files, so pointing at a name that never exists healed the checkpoint and left the cause in place. The karaoke model's lazy load had no healing. Docker has no warmup step, so that path is the only one Docker ever takes, and it was the unfixed shape of #502 for every container user. Beat detection got both of its paths; this got one. Healing now happens once per artifact per process. beat_this collapses every failure into a single ValueError, so a load broken for a reason a fresh copy cannot fix is indistinguishable from a truncated one, and it would delete and re-fetch ~100 MB on every attempt for as long as the process lived. Two comments were describing things that were not true: run_pip_install still said "bounding it at 20 minutes", and warmup.py claimed every load below went through load_or_heal when demucs and the section model do not. The latter is now stated as the deliberate choice it is, with the reason: naming another library's cache layout from a distance is exactly the mistake that produced the wrong filename above.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| import pytest | ||
|
|
||
| import app.core.model_cache as model_cache |
thcp
marked this pull request as ready for review
September 9, 2026 12:47
This was referenced Sep 9, 2026
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
1 participant
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.
Credit
@the-observer07 reported #600, forked, and pushed a branch that identified the cause correctly: requestAnimationFrame is not delivered to a hidden tab, and a pending frame is dropped rather than deferred, so the loop needs an external kick to change clocks. The diagnosis here is theirs.