From 9d33a76ada17cf85efed212ef381958b4f256941 Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Mon, 31 Aug 2026 17:55:25 +0100 Subject: [PATCH 01/19] ci: stop fork pull requests running on the self-hosted runners macos-check.yml and windows-check.yml trigger on pull_request and run on [self-hosted, macOS, ARM64] and [self-hosted, windows, x64] with no fork guard, on a public repository. cargo fmt/build/clippy all execute code the pull request supplies: build.rs, proc-macro crates, a swapped Cargo.toml dependency or [patch], and the test bodies themselves. permissions: {} limits what the token can reach; it does nothing about code execution on the machine. That machine is the same one that builds, signs and uploads every macOS DMG and runtime pack. Neither check workflow cleans its workspace and the release workflows only remove .build and dist, so an implant in ~/.cargo, ~/.npm, ~/.rustup or the persistent _work tree would survive straight into the next release. GitHub's public-repo default only gates first-time contributors, so one trivial merged pull request is enough to unlock this for a later one. Gate both jobs on the pull request coming from this repository. The workflow_dispatch path is unaffected, so a fork's Rust change can still get its compiler pass when a maintainer asks for one -- which keeps the reason the trigger exists (#421: cfg-gated Rust merging without ever being compiled, because ci.yml is entirely ubuntu-latest). Refs #511 --- .github/workflows/macos-check.yml | 14 ++++++++++++++ .github/workflows/windows-check.yml | 9 +++++++++ 2 files changed, 23 insertions(+) diff --git a/.github/workflows/macos-check.yml b/.github/workflows/macos-check.yml index de670993..57f62d0e 100644 --- a/.github/workflows/macos-check.yml +++ b/.github/workflows/macos-check.yml @@ -26,6 +26,20 @@ concurrency: jobs: check: + # Never run a fork's code on the self-hosted runner. cargo build/clippy/test + # all execute whatever the PR supplies -- build.rs, proc-macro crates, a + # swapped Cargo.toml dependency, the test bodies themselves -- and this + # runner is the same machine that builds, signs and uploads every macOS + # release. Nothing here cleans the workspace, so an implant in ~/.cargo, + # ~/.rustup or the persistent _work tree would survive into the next + # release. permissions: {} limits the token, not code execution. + # + # GitHub's public-repo default only gates *first-time* contributors, so one + # trivial merged PR is enough to unlock this for a later one. Fork PRs get a + # maintainer-triggered workflow_dispatch run instead. + if: >- + github.event_name != 'pull_request' || + github.event.pull_request.head.repo.full_name == github.repository # Runner must be darwin/arm64 with Xcode CLT and rustup (same requirements # as macos-release.yml, which this intentionally does not replace -- this # only builds/checks, never signs, packages, or uploads anything). diff --git a/.github/workflows/windows-check.yml b/.github/workflows/windows-check.yml index a567d294..e072eb80 100644 --- a/.github/workflows/windows-check.yml +++ b/.github/workflows/windows-check.yml @@ -27,6 +27,15 @@ concurrency: jobs: check: + # Never run a fork's code on the self-hosted runner -- see the same guard in + # macos-check.yml. cargo build/clippy/test execute whatever the PR supplies + # (build.rs, proc-macro crates, a swapped Cargo.toml dependency, the test + # bodies), nothing here cleans the workspace, and this runner also builds + # the Windows release. Fork PRs get a maintainer-triggered + # workflow_dispatch run instead. + if: >- + github.event_name != 'pull_request' || + github.event.pull_request.head.repo.full_name == github.repository # Runner must have rustup and the MSVC toolchain (same requirements as # windows-release.yml, which this intentionally does not replace -- this # only builds/checks, never packages or uploads anything). From 5286c0567953b47a646f54e8d5cc3a3fc7707bbb Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Mon, 31 Aug 2026 17:58:07 +0100 Subject: [PATCH 02/19] fix(macos): restore directory deferral lost in the AppleDouble extractor #506 replaced Archive::unpack with a per-entry unpack_in loop so AppleDouble "._" members could be skipped. That loop reproduced the iteration but not the two things unpack does around it. Directories are now applied last, reverse-sorted by path, the way Archive::_unpack does (tar-rs#242): a directory carries its own mode, so creating it inline in archive order means a 0o555 member exists before its contents are written and the next file inside it fails with EACCES. On a runtime pack that contains one, first-run setup dies with no fallback. `ditto` and `tar` both record such modes faithfully, so this was reachable. `destination` is canonicalized before the loop again, which on Windows supplies the \\?\ prefix so member paths over 260 characters still extract. Traversal protection was never affected and is unchanged: unpack_in rejects ParentDir components, strips RootDir/Prefix, and canonicalizes against dst on every entry, so zip-slip, absolute members and symlink escapes stay blocked. This was a robustness regression, not a security one. Deferring costs nothing on a streaming archive: directory entries carry no data, so only their metadata is applied in the second pass. The new test builds a .tar.zst whose directory member is 0o555 and whose file member follows it, which is the order that broke. Verified it fails against inline creation and passes with the deferral. Refs #508 --- desktop/src-tauri/src/main.rs | 93 ++++++++++++++++++++++++++++++++++- 1 file changed, 91 insertions(+), 2 deletions(-) diff --git a/desktop/src-tauri/src/main.rs b/desktop/src-tauri/src/main.rs index 40145f5e..7ade5bae 100644 --- a/desktop/src-tauri/src/main.rs +++ b/desktop/src-tauri/src/main.rs @@ -16,7 +16,7 @@ use std::{ thread, time::{Duration, Instant, SystemTime, UNIX_EPOCH}, }; -use tar::Archive; +use tar::{Archive, EntryType}; use tauri::{Emitter, Manager}; use tauri_plugin_store::StoreExt; #[cfg(windows)] @@ -3187,13 +3187,34 @@ fn is_apple_double(path: &Path) -> bool { .is_some_and(|name| name.starts_with("._")) } +/// Stands in for `Archive::unpack`, which cannot be used because it offers no +/// way to skip an entry. It has to reproduce the two things `unpack` does +/// beyond looping over entries, both of which the first version of this +/// function dropped: +/// +/// 1. **Directories are applied last**, reverse-sorted by path, because a +/// directory carries its own mode. Created inline in archive order, a +/// `0o555` member exists before its contents are written and the next file +/// inside it fails with EACCES -- first-run setup dies with no fallback. +/// Upstream calls this out as tar-rs#242. +/// 2. **`destination` is canonicalized up front**, which on Windows supplies +/// the `\\?\` prefix so member paths over 260 characters still extract. +/// +/// Traversal protection needs nothing here: `unpack_in` rejects `ParentDir` +/// components, strips `RootDir`/`Prefix`, and canonicalizes against `dst` on +/// every entry, so zip-slip, absolute members and symlink escapes stay blocked. fn unpack_without_apple_double( mut archive: Archive, destination: &Path, ) -> Result<(), String> { + let destination = destination + .canonicalize() + .unwrap_or_else(|_| destination.to_path_buf()); + let entries = archive .entries() .map_err(|e| format!("failed to read runtime pack: {e}"))?; + let mut directories = Vec::new(); for entry in entries { let mut entry = entry.map_err(|e| format!("failed to read runtime pack: {e}"))?; let path = entry @@ -3203,8 +3224,20 @@ fn unpack_without_apple_double( if is_apple_double(&path) { continue; } + // Directories hold no data, so deferring them reads nothing back off a + // streaming archive -- only their metadata is applied later. + if entry.header().entry_type() == EntryType::Directory { + directories.push(entry); + continue; + } entry - .unpack_in(destination) + .unpack_in(&destination) + .map_err(|e| format!("failed to extract runtime pack: {e}"))?; + } + + directories.sort_by(|a, b| b.path_bytes().cmp(&a.path_bytes())); + for mut dir in directories { + dir.unpack_in(&destination) .map_err(|e| format!("failed to extract runtime pack: {e}"))?; } Ok(()) @@ -4477,6 +4510,62 @@ mod tests { ); } + #[test] + #[cfg(unix)] + fn extract_tar_archive_survives_a_read_only_directory_member() { + // tar::Archive::unpack applies directory entries last, reverse-sorted, + // so a directory's own mode cannot stop its children being written + // (tar-rs#242). The first version of unpack_without_apple_double + // created them inline in archive order, which turns a 0o555 member into + // a hard extraction failure and a dead first-run setup (#508). + use std::os::unix::fs::PermissionsExt; + + let archive_dir = make_tmp(); + let archive = archive_dir.path().join("runtime.tar.zst"); + let encoder = zstd::Encoder::new(fs::File::create(&archive).unwrap(), 0).unwrap(); + let mut builder = tar::Builder::new(encoder); + + // Directory first, file second -- the order that broke. + let mut dir_header = tar::Header::new_gnu(); + dir_header.set_entry_type(tar::EntryType::Directory); + dir_header.set_mode(0o555); + dir_header.set_size(0); + builder + .append_data(&mut dir_header, "runtime/locked/", std::io::empty()) + .unwrap(); + + let body = b"axes.grid: True"; + let mut file_header = tar::Header::new_gnu(); + file_header.set_entry_type(tar::EntryType::Regular); + file_header.set_mode(0o644); + file_header.set_size(body.len() as u64); + builder + .append_data(&mut file_header, "runtime/locked/style.mplstyle", &body[..]) + .unwrap(); + builder.into_inner().unwrap().finish().unwrap(); + + let destination = make_tmp(); + super::extract_tar_archive(&archive, destination.path()).unwrap(); + + let written = destination.path().join("runtime/locked/style.mplstyle"); + assert!( + written.is_file(), + "a file inside a read-only directory member must still extract" + ); + let mode = fs::metadata(destination.path().join("runtime/locked")) + .unwrap() + .permissions() + .mode(); + assert_eq!(mode & 0o777, 0o555, "the directory keeps its archived mode"); + + // Leave it writable so TempDir cleanup can remove it. + fs::set_permissions( + destination.path().join("runtime/locked"), + fs::Permissions::from_mode(0o755), + ) + .unwrap(); + } + #[test] fn legacy_migration_preserves_user_settings_when_data_dir_already_exists() { // setup() creates the destination before ensure_workspace() invokes From 19a72b470b70b1f7d64d990738258b1dad950eb6 Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Mon, 31 Aug 2026 18:02:48 +0100 Subject: [PATCH 03/19] fix(settings): stop an interrupted write destroying the user's settings _save() called write_text, which truncates first and writes second. A process that died in between -- app quit, the parent watchdog's SIGTERM, power loss -- left settings.json present and unparsable. _load() then could not tell that apart from a first run: it caught the parse error, returned {}, and the next set_*() persisted a single key over both settings.json and the per-user mirror that exists to protect it. A real user lost port and allow_network from both copies, with only a warning in the log. jobs_dir is the worse case, since losing it makes a relocated library look empty -- precisely what the mirror was added to prevent. Three changes: - _atomic_write_json() writes through a uniquely-named same-directory temp and replaces, so an interrupted write cannot truncate what was already there. _save() and _mirror_settings() both use it; the mirror previously used a fixed ".json.tmp" name that two writers could interleave on. - _load() distinguishes absent from unusable. Absent stays a first run. A file that exists but does not parse -- or parses to something other than an object -- is moved aside as settings.json.corrupt-, so the bytes survive for diagnosis instead of being overwritten by the next save, and the mirror is consulted before falling back to defaults. - Settings recovered from the mirror are written straight back to the primary. Recovering only into memory would last until the next start, which would read a now-absent primary and quietly return to defaults again. The tests cover the exact sequence that lost data: a torn write, then a restart. Four of them fail against the old _load and pass against this one. Refs #509 --- app/core/settings.py | 121 ++++++++++++++++++++++++------ tests/test_settings_durability.py | 110 +++++++++++++++++++++++++++ 2 files changed, 207 insertions(+), 24 deletions(-) create mode 100644 tests/test_settings_durability.py diff --git a/app/core/settings.py b/app/core/settings.py index c50da598..da093aa2 100644 --- a/app/core/settings.py +++ b/app/core/settings.py @@ -25,6 +25,8 @@ import logging import os import threading +import time +import uuid from pathlib import Path from app.core.config import ( @@ -70,16 +72,97 @@ def _default_allow_network() -> bool: return os.environ.get("STEMDECK_DESKTOP") != "1" -def _load() -> dict: +def _mirror_path() -> Path | None: + """Where the per-user copy lives, or None when the shell did not set one. + + The path comes from the shell (STEMDECK_SETTINGS_MIRROR) so the platform + logic stays in one place -- see _mirror_settings.""" + target = os.environ.get("STEMDECK_SETTINGS_MIRROR", "").strip() + return Path(target) if target else None + + +def _read_json_dict(path: Path) -> dict | None: + """Parse `path` as a JSON object. + + None means "there is nothing usable here" -- absent, unreadable, not JSON, + or JSON that is not an object. Callers that need to tell *absent* from + *unusable* must check existence themselves; that distinction is the whole + point of _load below.""" try: - data = json.loads(_SETTINGS_PATH.read_text(encoding="utf-8")) - if isinstance(data, dict): - return data + data = json.loads(path.read_text(encoding="utf-8")) except FileNotFoundError: - pass # no settings file yet — first run; use defaults + return None + except Exception: + _log.warning("could not read settings from %s", path, exc_info=True) + return None + return data if isinstance(data, dict) else None + + +def _atomic_write_json(path: Path, data: dict) -> bool: + """Write `data` to `path` so an interrupted write cannot destroy what was + there before. + + write_text() truncates first and writes second, so a process that dies in + between leaves a file that exists and does not parse -- which _load then + could not distinguish from a first run, and the next setting change + persisted a one-key file over both this and the mirror (#509). Same + same-directory temp + replace the registry already uses; the temp name is + unique per call so two concurrent writers cannot interleave on it.""" + try: + path.parent.mkdir(parents=True, exist_ok=True) + tmp = path.with_name(f"{path.name}.{uuid.uuid4().hex}.tmp") + try: + tmp.write_text(json.dumps(data), encoding="utf-8") + tmp.replace(path) + finally: + tmp.unlink(missing_ok=True) except Exception: - # Corrupt/unreadable file: fall back to defaults rather than crash. - _log.warning("could not read settings from %s", _SETTINGS_PATH, exc_info=True) + _log.warning("could not persist settings to %s", path, exc_info=True) + return False + return True + + +def _quarantine_corrupt(path: Path) -> None: + """Move an unusable settings file aside rather than leaving it to be + overwritten by the next save. + + Renaming keeps the bytes for diagnosis. Deleting or writing over them + destroys the only remaining evidence of what the user had configured.""" + try: + target = path.with_name(f"{path.name}.corrupt-{int(time.time())}") + path.replace(target) + _log.warning("settings at %s were unreadable; moved aside to %s", path, target) + except OSError: + _log.warning("could not move unreadable settings at %s aside", path, exc_info=True) + + +def _load() -> dict: + """Read settings, telling "no file yet" apart from "file we cannot read". + + Conflating the two is what lost real user settings: a torn write left an + unparsable file, this returned {} exactly as it would on a first run, and + the next set_*() then persisted a single key over both settings.json and + the mirror that existed to protect it.""" + if not _SETTINGS_PATH.exists(): + return {} # no settings file yet — genuine first run; use defaults + + data = _read_json_dict(_SETTINGS_PATH) + if data is not None: + return data + + # The file is there but unusable. Preserve it, then try the per-user copy + # the shell keeps outside the install directory. + _quarantine_corrupt(_SETTINGS_PATH) + mirror = _mirror_path() + if mirror is not None: + recovered = _read_json_dict(mirror) + if recovered: + _log.warning("recovered settings from mirror %s", mirror) + # Put them back immediately. Without this the recovery only lasts + # until the next start, which would read a now-absent primary and + # silently fall back to defaults again. + _atomic_write_json(_SETTINGS_PATH, recovered) + return recovered return {} @@ -106,11 +189,7 @@ def _save() -> bool: is still reported back, because one caller (set_jobs_dir) is coupled to something irreversible enough that silently swallowing a failure there would be actively misleading rather than merely inconvenient (#403).""" - try: - _SETTINGS_PATH.parent.mkdir(parents=True, exist_ok=True) - _SETTINGS_PATH.write_text(json.dumps(_ensure()), encoding="utf-8") - except Exception: - _log.warning("could not persist settings to %s", _SETTINGS_PATH, exc_info=True) + if not _atomic_write_json(_SETTINGS_PATH, _ensure()): return False _mirror_settings() return True @@ -135,19 +214,13 @@ def _mirror_settings() -> None: the shell (STEMDECK_SETTINGS_MIRROR) so the platform logic stays in one place and both halves cannot drift apart. """ - target = os.environ.get("STEMDECK_SETTINGS_MIRROR", "").strip() - if not target: + path = _mirror_path() + if path is None: return - try: - path = Path(target) - path.parent.mkdir(parents=True, exist_ok=True) - # Same-directory temp + replace: a torn write here would be restored - # verbatim into the user's next install. - tmp = path.with_suffix(".json.tmp") - tmp.write_text(json.dumps(_ensure()), encoding="utf-8") - tmp.replace(path) - except Exception: - _log.warning("could not mirror settings to %s", target, exc_info=True) + # Same-directory temp + replace: a torn write here would be restored + # verbatim into the user's next install. _atomic_write_json also gives the + # temp file a unique name, so two writers cannot interleave on it. + _atomic_write_json(path, _ensure()) def _num(v: object) -> int | None: diff --git a/tests/test_settings_durability.py b/tests/test_settings_durability.py new file mode 100644 index 00000000..942669e3 --- /dev/null +++ b/tests/test_settings_durability.py @@ -0,0 +1,110 @@ +"""Settings must survive a write that does not finish (#509). + +`_save()` used to call `write_text`, which truncates first and writes second. +A process that died in between left a file that existed and did not parse, and +`_load()` returned `{}` for it -- indistinguishable from a first run. The next +`set_*()` then persisted a single key over both settings.json and the mirror +that exists to protect it, so a real user lost `port` and `allow_network` from +both copies with only a warning in the log. +""" + +from __future__ import annotations + +import json +import pathlib + +from app.core import settings as _settings + + +def _read(path): + return json.loads(path.read_text(encoding="utf-8")) + + +def test_an_absent_file_is_a_first_run(tmp_path): + assert not _settings._SETTINGS_PATH.exists() + assert _settings._load() == {} + + +def test_an_unreadable_file_is_not_mistaken_for_a_first_run(tmp_path, monkeypatch): + # The distinction is the whole bug: defaults are right for a first run and + # catastrophic for a settings file we merely failed to read. + mirror = tmp_path / "shared" / "settings.json" + mirror.parent.mkdir(parents=True) + mirror.write_text(json.dumps({"port": 8081, "allow_network": True}), encoding="utf-8") + monkeypatch.setenv("STEMDECK_SETTINGS_MIRROR", str(mirror)) + + _settings._SETTINGS_PATH.write_text('{"port": 80', encoding="utf-8") # torn write + + assert _settings._load() == {"port": 8081, "allow_network": True} + + +def test_an_unreadable_file_is_kept_for_diagnosis(tmp_path): + _settings._SETTINGS_PATH.write_text('{"port": 80', encoding="utf-8") + + _settings._load() + + corrupt = list(tmp_path.glob("settings.json.corrupt-*")) + assert len(corrupt) == 1, "the unreadable bytes must not be silently destroyed" + assert corrupt[0].read_text(encoding="utf-8") == '{"port": 80' + + +def test_recovered_settings_are_written_back_immediately(tmp_path, monkeypatch): + # Recovering only into memory would last until the next start, which would + # read a now-absent primary and fall back to defaults again. + mirror = tmp_path / "shared" / "settings.json" + mirror.parent.mkdir(parents=True) + mirror.write_text(json.dumps({"port": 8081}), encoding="utf-8") + monkeypatch.setenv("STEMDECK_SETTINGS_MIRROR", str(mirror)) + _settings._SETTINGS_PATH.write_text("", encoding="utf-8") # truncated to nothing + + _settings._load() + + assert _read(_settings._SETTINGS_PATH) == {"port": 8081} + + +def test_no_mirror_and_a_corrupt_file_falls_back_to_defaults(tmp_path, monkeypatch): + monkeypatch.delenv("STEMDECK_SETTINGS_MIRROR", raising=False) + _settings._SETTINGS_PATH.write_text("not json at all", encoding="utf-8") + + assert _settings._load() == {} + + +def test_a_non_object_settings_file_is_treated_as_unusable(tmp_path, monkeypatch): + # Valid JSON, wrong shape. Returning it would make every later .get() raise. + monkeypatch.delenv("STEMDECK_SETTINGS_MIRROR", raising=False) + _settings._SETTINGS_PATH.write_text("[1, 2, 3]", encoding="utf-8") + + assert _settings._load() == {} + assert list(tmp_path.glob("settings.json.corrupt-*")) + + +def test_a_failed_write_leaves_the_previous_settings_intact(tmp_path, monkeypatch): + # The heart of it: temp + replace means an interrupted write cannot truncate + # what was already there. + # + # Only the temp write is made to fail, and monkeypatch.undo() is deliberately + # not used: the same monkeypatch instance carries conftest's _SETTINGS_PATH + # isolation, so undoing here would point the assertion at the developer's + # real settings file. + path = _settings._SETTINGS_PATH + path.write_text(json.dumps({"port": 8081, "allow_network": True}), encoding="utf-8") + + real_write_text = pathlib.Path.write_text + + def _boom(self, *a, **kw): + if self.name.endswith(".tmp"): + raise OSError("disk full") + return real_write_text(self, *a, **kw) + + monkeypatch.setattr("pathlib.Path.write_text", _boom) + + assert _settings._atomic_write_json(path, {"port": 9000}) is False + assert _read(path) == {"port": 8081, "allow_network": True} + assert not list(tmp_path.glob("*.tmp")), "a failed write must not leave a temp file" + + +def test_atomic_write_leaves_no_temp_files_behind(tmp_path): + assert _settings._atomic_write_json(_settings._SETTINGS_PATH, {"port": 8081}) is True + + assert _read(_settings._SETTINGS_PATH) == {"port": 8081} + assert not list(tmp_path.glob("*.tmp")), "temp files must not accumulate next to settings" From 261da3b1a6de6c47d6a6942a63c608d19d66b0cd Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Mon, 31 Aug 2026 18:07:16 +0100 Subject: [PATCH 04/19] fix(registry): free a job cancelled in the pop-to-claim window, and boot with a bad registry Two ways the registry stranded state. A cancel arriving between the worker's _pop_next() and _set_running() found the job in neither the queue nor the running slot. discard() returned False so cancel_job never finalised it, running_id() was None so nothing was terminated, and the worker then dropped it silently. The job stayed at "queued" forever: absent from the queue view, still counted by pending_count against the capacity limit, its uploaded source (up to 400 MB) never freed, and -- because "queued" is persisted -- re-queued on every restart. The worker is the sole consumer and owns the job once it has popped it, so it now closes out the cancellation itself via _finalise_dropped_job(). Extracted rather than inlined so the drop path can be tested directly, and so an already-terminal job is explicitly left alone rather than having a real result rewritten. Separately, restore() caught (OSError, JSONDecodeError, TypeError, ValueError). A registry.json that was valid JSON but not an object -- a top-level list, null, a bare string -- makes _migrate call data.get() and raise AttributeError, which that tuple does not name. restore() runs at import time, so the backend simply never started, with no self-healing path and no way for a user to recover short of deleting the file by hand. Orphan recovery and the persist that follows it sat outside the guard entirely, so an unreadable jobs_dir was fatal at startup for the same reason. Both are now caught broadly and logged. A registry we cannot use should cost the user their job list, not their app. The cancel test drives the real worker loop rather than calling the helper, so it also fails if the worker stops calling it. Verified: reverting the worker's call fails that test, and reverting restore()'s except fails five others. Refs #520 --- app/core/registry.py | 38 ++++++---- app/pipeline/jobqueue.py | 29 +++++++- tests/test_registry_resilience.py | 118 ++++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+), 14 deletions(-) create mode 100644 tests/test_registry_resilience.py diff --git a/app/core/registry.py b/app/core/registry.py index dc70c39b..84467c40 100644 --- a/app/core/registry.py +++ b/app/core/registry.py @@ -175,21 +175,33 @@ def restore(jobs_dir: Path) -> None: _pending_resume.extend( j.id for j in sorted(resume, key=lambda j: (j.queue_position, j.created_at)) ) - except (OSError, json.JSONDecodeError, TypeError, ValueError): + except Exception: + # Broad on purpose. restore() runs at import time, so anything that + # escapes here stops the backend booting with no way for a user to + # recover short of deleting the file by hand. A registry.json that + # is valid JSON but not an object (a top-level list, null, a bare + # string) used to do exactly that: _migrate calls data.get() and + # raises AttributeError, which the old tuple did not name (#520). logger.warning("failed to load registry from %s", path, exc_info=True) - with _lock: - known = set(_jobs) - for job_dir in jobs_dir.iterdir(): - if not job_dir.is_dir() or not JOB_ID_RE.match(job_dir.name) or job_dir.name in known: - continue - recovered = _recover_done_job(job_dir) - if recovered is not None: - with _lock: - _jobs[recovered.id] = recovered - changed = True - if changed: - persist(jobs_dir) + # Orphan recovery, and the persist that follows it, were outside the guard + # above -- an unreadable jobs_dir or a failed write was fatal at startup for + # the same reason. + try: + with _lock: + known = set(_jobs) + for job_dir in jobs_dir.iterdir(): + if not job_dir.is_dir() or not JOB_ID_RE.match(job_dir.name) or job_dir.name in known: + continue + recovered = _recover_done_job(job_dir) + if recovered is not None: + with _lock: + _jobs[recovered.id] = recovered + changed = True + if changed: + persist(jobs_dir) + except Exception: + logger.warning("failed to recover jobs from %s", jobs_dir, exc_info=True) def _resume_or_recover(job: Job, job_dir: Path) -> Job | None: diff --git a/app/pipeline/jobqueue.py b/app/pipeline/jobqueue.py index ae1b61b0..ae872b23 100644 --- a/app/pipeline/jobqueue.py +++ b/app/pipeline/jobqueue.py @@ -200,6 +200,21 @@ async def _dispatch(job: Job) -> None: await run_pipeline(job, source_url, JOBS_DIR) +def _finalise_dropped_job(job: Job) -> None: + """Complete a cancellation the API could not finish itself. + + cancel_job can only finalise a job it still finds in the queue. Between + _pop_next() and _set_running() a job is in neither the queue nor the + running slot, so a cancel arriving there sets cancel_requested and returns. + The worker owns the job by then and is the only thing that can close it + out (#520).""" + if not job.cancel_requested or job.status in ("done", "error", "cancelled"): + return + _set(job, status="cancelled", stage="Cancelled") + cleanup_job_dir(job.id) + registry_persist(JOBS_DIR) + + async def _worker_loop() -> None: assert _wake is not None while not _stopping: @@ -220,7 +235,19 @@ async def _worker_loop() -> None: if job is None: continue if job.cancel_requested or job.status in ("done", "error", "cancelled"): - # Cancelled or finished while it waited; drop it silently. + # Cancelled or finished while it waited. + # + # Finalising here is not optional. Between _pop_next() above and + # _set_running() below the job is in neither the queue nor the + # running slot, so a cancel arriving in that window finds + # discard() False and running_id() None and returns having only + # set cancel_requested. This worker is the sole consumer and owns + # the job by now, so if it just dropped it the job would sit at + # "queued" forever: absent from the queue view, still counted by + # pending_count against the capacity limit, its uploaded source + # never freed, and -- because "queued" is persisted -- re-queued + # on every restart (#520). + _finalise_dropped_job(job) continue # Claim it. No await between the pop and this status write, so a job is diff --git a/tests/test_registry_resilience.py b/tests/test_registry_resilience.py new file mode 100644 index 00000000..ace3c062 --- /dev/null +++ b/tests/test_registry_resilience.py @@ -0,0 +1,118 @@ +"""Two ways the registry used to strand state (#520). + +A cancel that lands in the window between the queue worker popping a job and +claiming it left the job at "queued" forever, invisible but still counted +against the capacity limit. And a registry.json that was valid JSON but not an +object stopped the backend booting at all. +""" + +from __future__ import annotations + +import contextlib +import json + +import pytest + +from app.core import registry as _registry +from app.core.config import JOB_ID_RE +from app.core.models import Job +from app.core.registry import registry_path + + +def _job(job_id="a1b2c3d4e5f6", **kw): + return Job(id=job_id, **kw) + + +# ─── a registry file we cannot use must not stop the backend ─── + + +@pytest.mark.parametrize("body", ["[1, 2, 3]", "null", '"a string"', "42", "[]"]) +def test_a_non_object_registry_does_not_raise(tmp_path, body): + # _migrate calls data.get(); anything that is not a dict raises + # AttributeError, which restore() ran at import time and never caught. + registry_path(tmp_path).write_text(body, encoding="utf-8") + + _registry.restore(tmp_path) # must not raise + + assert _registry.all_jobs() == {} + + +def test_a_corrupt_registry_does_not_raise(tmp_path): + registry_path(tmp_path).write_text("{not json", encoding="utf-8") + + _registry.restore(tmp_path) + + assert _registry.all_jobs() == {} + + +def test_a_good_registry_still_loads(tmp_path): + job = _job(status="done", title="Song") + registry_path(tmp_path).write_text(json.dumps({"jobs": [job.to_record()]}), encoding="utf-8") + + _registry.restore(tmp_path) + + assert job.id in _registry.all_jobs() + + +def test_an_unreadable_jobs_dir_does_not_raise(tmp_path, monkeypatch): + # Orphan recovery sat outside the guard, so an OSError from iterdir() was + # fatal at startup too. + def _boom(self): + raise OSError("permission denied") + + monkeypatch.setattr("pathlib.Path.iterdir", _boom) + + _registry.restore(tmp_path) # must not raise + + +# ─── a cancel in the pop-to-claim window must not strand the job ─── + + +def test_job_id_re_matches_the_ids_we_generate(): + # The orphan-recovery filter depends on this; a drift here would silently + # stop recovery working at all. + assert JOB_ID_RE.match("a1b2c3d4e5f6") + + +async def test_cancel_between_pop_and_claim_finalises_the_job(tmp_path, monkeypatch): + """Drive the real worker loop, so this also catches the worker simply not + calling the finaliser.""" + import asyncio + + from app.pipeline import jobqueue + + job = _job(status="queued") + _registry.register(job) + jobqueue.enqueue(job.id) + assert _registry.pending_count(uploads=False) == 1 + + # The cancel lands in the pop-to-claim window: cancel_job's discard() has + # already lost the race, so all it can do is set the flag. + job.cancel_requested = True + + task = jobqueue.start_worker() + try: + for _ in range(50): + await asyncio.sleep(0.01) + if job.status == "cancelled": + break + finally: + jobqueue.request_stop() + task.cancel() + with contextlib.suppress(asyncio.CancelledError): + await task + + assert job.status == "cancelled", "a stranded job holds a capacity slot forever" + assert _registry.pending_count(uploads=False) == 0, "the capacity slot must be released" + + +async def test_a_dropped_job_that_was_not_cancelled_is_left_alone(tmp_path): + # Already-terminal jobs reach the same branch; finalising them would + # rewrite a real result. + job = _job(status="done", title="Song") + _registry.register(job) + + jobqueue_mod = __import__("app.pipeline.jobqueue", fromlist=["jobqueue"]) + jobqueue_mod._finalise_dropped_job(job) + + assert job.status == "done" From 3dbfca58ce07946543188b4bd88b5322015b56e6 Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Mon, 31 Aug 2026 18:29:42 +0100 Subject: [PATCH 05/19] fix(sse): stop connection slots leaking when a client disconnects early claim_sse_slot() runs in the handler and release_sse_slot() lived in the stream's finally. An async generator that is never started never runs its finally, so a client that disconnected before the response body began held a slot for the life of the process: StreamingResponse raises inside stream_response on its first send(), before __anext__ is ever called, and the generator body never executes. Two hundred of those and every job-progress stream and the queue stream answer 503 with nothing actually connected, until a restart. It does not need malice either -- a flaky network or a rapidly reloaded page leaks slots the same way, just slower. Claiming cannot simply move inside the generator: by the time it runs the response headers have gone out and there is no status code left, so hitting the cap could no longer answer 503. SseSlot keeps the claim in the handler and makes release idempotent, with __del__ as the backstop for the never-started case -- collecting the generator collects the closure holding the slot. _held is assigned before the claim because __del__ runs on a half-built object too: a refused claim would otherwise raise AttributeError out of __del__ rather than releasing nothing. The test for that caught it. queue.py takes the same guard, so both streams that share the budget also share the fix. Verified: removing the __del__ backstop fails the never-started test. Refs #513 --- app/api/events.py | 43 +++++++++++++++- app/api/queue.py | 6 +-- tests/test_sse_slot_budget.py | 92 +++++++++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 5 deletions(-) create mode 100644 tests/test_sse_slot_budget.py diff --git a/app/api/events.py b/app/api/events.py index f63b6370..dedb0b84 100644 --- a/app/api/events.py +++ b/app/api/events.py @@ -38,6 +38,45 @@ def release_sse_slot() -> None: _sse_active -= 1 +class SseSlot: + """One held connection slot, released exactly once. + + Claiming has to happen in the handler so that hitting the cap can still be + answered with a 503 -- once the generator is running the response headers + have gone out and there is no status code left to send. + + That is what leaked slots: release lives in the stream's `finally`, and an + async generator that is never started never runs its `finally`. If the + client disconnects before the body begins, StreamingResponse raises inside + `stream_response` on its first `send()` -- before `__anext__` is ever + called -- so the generator body never executes and the slot was held + forever. 200 of those and every progress stream 503s with nothing actually + connected, until the process restarts (#513). + + The stream releases on its way out as before; `__del__` is the backstop for + the never-started case, where collecting the generator collects the closure + holding this. Release is idempotent so the two cannot double-count. + """ + + __slots__ = ("_held",) + + def __init__(self) -> None: + # Set first: claim_sse_slot raises at the cap, and __del__ still runs on + # a half-built object. Without this it would raise AttributeError from + # __del__ instead of releasing nothing. + self._held = False + claim_sse_slot() # may raise 503; nothing is held if it does + self._held = True + + def release(self) -> None: + if self._held: + self._held = False + release_sse_slot() + + def __del__(self) -> None: + self.release() + + @router.get("/jobs/{job_id}/events") async def job_events(job_id: str) -> StreamingResponse: """Server-Sent Events stream of job state updates. Closes when the job @@ -47,7 +86,7 @@ async def job_events(job_id: str) -> StreamingResponse: job = registry_get(job_id) if job is None: raise HTTPException(status_code=404, detail="job not found") - claim_sse_slot() + slot = SseSlot() async def stream() -> AsyncIterator[str]: try: @@ -82,7 +121,7 @@ async def stream() -> AsyncIterator[str]: keepalive_at = 0 await asyncio.sleep(0.2) finally: - release_sse_slot() + slot.release() return StreamingResponse( stream(), diff --git a/app/api/queue.py b/app/api/queue.py index 6687185a..0c2c65e5 100644 --- a/app/api/queue.py +++ b/app/api/queue.py @@ -22,7 +22,7 @@ from fastapi.responses import StreamingResponse from pydantic import BaseModel -from app.api.events import _MAX_SSE_SECONDS, claim_sse_slot, release_sse_slot +from app.api.events import _MAX_SSE_SECONDS, SseSlot from app.core.config import JOB_ID_RE, JOBS_DIR, MAX_PENDING_UPLOAD_JOBS, MAX_PENDING_URL_JOBS from app.core.registry import get as registry_get from app.core.registry import pending_count as registry_pending_count @@ -130,7 +130,7 @@ async def queue_events() -> StreamingResponse: outlives any individual job and is expected to stay open for the session, so only the 4 h ceiling ends it. """ - claim_sse_slot() + slot = SseSlot() async def stream() -> AsyncIterator[str]: try: @@ -156,7 +156,7 @@ async def stream() -> AsyncIterator[str]: keepalive_at = 0 await asyncio.sleep(0.25) finally: - release_sse_slot() + slot.release() return StreamingResponse( stream(), diff --git a/tests/test_sse_slot_budget.py b/tests/test_sse_slot_budget.py new file mode 100644 index 00000000..eb7a1613 --- /dev/null +++ b/tests/test_sse_slot_budget.py @@ -0,0 +1,92 @@ +"""The shared SSE connection budget must not leak (#513). + +claim_sse_slot() runs in the handler so hitting the cap can still answer 503. +release lives in the stream's finally -- and an async generator that is never +started never runs its finally. A client that disconnects before the response +body begins therefore held a slot forever: 200 of those and every progress +stream 503s with nothing actually connected, until the process restarts. +""" + +from __future__ import annotations + +import gc + +import pytest +from fastapi import HTTPException + +from app.api import events as _events + + +@pytest.fixture(autouse=True) +def _zero_budget(): + _events._sse_active = 0 + yield + _events._sse_active = 0 + + +def test_a_slot_is_held_then_released(): + slot = _events.SseSlot() + assert _events._sse_active == 1 + + slot.release() + assert _events._sse_active == 0 + + +def test_release_is_idempotent(): + # The stream's finally and the __del__ backstop can both fire; counting + # twice would free a slot that is still in use. + slot = _events.SseSlot() + slot.release() + slot.release() + + assert _events._sse_active == 0 + + +def test_a_slot_dropped_without_release_is_reclaimed(): + # The leak: the generator is created, so the slot is claimed, but never + # iterated, so its finally never runs. Collecting it must free the slot. + def _never_started(): + slot = _events.SseSlot() + + async def stream(): + try: + yield "data: x\n\n" + finally: + slot.release() + + return stream() # created, never iterated + + gen = _never_started() + assert _events._sse_active == 1 + + del gen + gc.collect() + + assert _events._sse_active == 0, "a slot leaked for the life of the process" + + +def test_the_cap_still_answers_503(): + held = [_events.SseSlot() for _ in range(_events._MAX_SSE_CONNECTIONS)] + assert _events._sse_active == _events._MAX_SSE_CONNECTIONS + + with pytest.raises(HTTPException) as excinfo: + _events.SseSlot() + assert excinfo.value.status_code == 503 + + for slot in held: + slot.release() + assert _events._sse_active == 0 + + +def test_a_refused_claim_holds_nothing(): + # If __init__ raises, no slot was taken -- so the failed attempt must not + # decrement on collection either. + held = [_events.SseSlot() for _ in range(_events._MAX_SSE_CONNECTIONS)] + with pytest.raises(HTTPException): + _events.SseSlot() + + gc.collect() + assert _events._sse_active == _events._MAX_SSE_CONNECTIONS + + for slot in held: + slot.release() From 017ab2599fc7fe678e7256399da63c149550ffc8 Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Mon, 31 Aug 2026 18:34:56 +0100 Subject: [PATCH 06/19] fix(api): bound request input that could exhaust memory or stall the loop Four related holes, all reachable with one unauthenticated request. The trim range had no ceiling. `end` is a float that reaches np.zeros(int(round(duration * sample_rate))) in the click renderer, so ?start=0&end=20000&count_in=1 asked for roughly 7 GB, and another 7 GB in the int16 conversion. A larger value raised MemoryError inside a blanket except, which silently shipped the export with no click rather than failing. _validate_trim_range bounds it by the job's own recorded duration, with a six-hour backstop for a job whose duration was never recorded, and a second of slack because ffprobe's duration can sit a hair under the decoded length. The click render ran synchronously inside async handlers, so all of that allocation and a Python loop over every beat blocked the event loop -- every SSE progress stream and the queue worker stalled behind it. It goes through asyncio.to_thread now. The buffer is float32 rather than float64: the output is 16-bit PCM, so the extra mantissa was never audible and a long export was allocating twice what it needed. The click cache pruned without keep=, so a render larger than the cache budget evicted itself the instant it was written and ffmpeg was handed a missing -i. That is the #482 bug, unfixed on this path. The mixdown write had the same gap against a concurrent render's prune. The body-size guard was scoped to paths ending /sections or /beats, leaving /api/search, /api/playlist, /api/settings and the JSON branch of /api/jobs uncapped -- Starlette buffers the whole body, then json.loads runs it on the event loop. A 200 MB body to /api/search stalled every other request with no valid job or prior state needed. It now applies by method, exempting multipart uploads, which stream to disk under their own 400 MB limit. A chunked request with no Content-Length used to skip the check entirely and fall through to an unbounded request.body(); it gets a 411 now. Six of the eight new tests fail against the old code. Refs #512 --- app/api/stems.py | 46 +++++++++++-- app/main.py | 41 +++++++++-- app/pipeline/click_render.py | 6 +- tests/test_jobs_api.py | 10 +-- tests/test_request_body_limits.py | 110 ++++++++++++++++++++++++++++++ 5 files changed, 195 insertions(+), 18 deletions(-) create mode 100644 tests/test_request_body_limits.py diff --git a/app/api/stems.py b/app/api/stems.py index e4710a15..f9605169 100644 --- a/app/api/stems.py +++ b/app/api/stems.py @@ -289,7 +289,10 @@ def _click_lane( return None if rendered is None: return None - _prune_mixdown_cache(_CLICK_CACHE_DIR) + # keep=path or a render larger than the cache budget evicts itself the + # instant it is written, and ffmpeg is then handed a missing -i (#512). + # Same reason the mixdown path passes keep= (#482). + _prune_mixdown_cache(_CLICK_CACHE_DIR, keep=path) return _ClickLane(path, g, lead_in, True) if not path.is_file(): @@ -308,10 +311,37 @@ def _click_lane( return None if rendered is None: return None - _prune_mixdown_cache(_CLICK_CACHE_DIR) + _prune_mixdown_cache(_CLICK_CACHE_DIR, keep=path) return _ClickLane(path, g, 0.0, False) +# A trim range is only ever meaningful inside the track. Without a ceiling, +# `end` is an unbounded float that reaches +# np.zeros(int(round(duration * sample_rate))) in the click renderer -- so +# ?start=0&end=20000&count_in=1 asks for a 7 GB allocation, and a larger value +# raises MemoryError inside a blanket except and silently drops the click +# (#512). The job's own duration is the honest ceiling; MAX_TRIM_SECONDS is the +# backstop for a job whose duration was never recorded. +MAX_TRIM_SECONDS = 6 * 60 * 60 + + +def _validate_trim_range(job_id: str, start: float | None, end: float | None) -> None: + """Reject a trim range that is not inside the track.""" + if start is None or end is None: + return + job = registry_get(job_id) + duration = getattr(job, "duration_sec", None) if job is not None else None + ceiling = float(duration) if duration else float(MAX_TRIM_SECONDS) + # A little slack over the recorded duration: it comes from ffprobe and can + # sit a hair under the decoded length, and the UI legitimately asks for the + # very end of a track. + if end > ceiling + 1.0: + raise HTTPException( + status_code=422, + detail="end is beyond the end of the track", + ) + + def _read_beat_grid(job_id: str) -> dict | None: """The grid an export should click to: the user's edits when present, the detected grid otherwise. Mirrors GET /api/jobs/{id}/beats.""" @@ -439,7 +469,7 @@ async def _stream_ffmpeg(cmd: list[str], context: str = "", cache_path: Path | N if tmp_path is not None: if finished and proc.returncode == 0: os.replace(tmp_path, cache_path) - _prune_mixdown_cache(cache_path.parent) + _prune_mixdown_cache(cache_path.parent, keep=cache_path) else: tmp_path.unlink(missing_ok=True) @@ -655,6 +685,7 @@ async def get_stem( status_code=422, detail="start and end are both required and start must be less than end", ) + _validate_trim_range(job_id, start, end) cmd = [ ffmpeg_executable(), @@ -701,6 +732,7 @@ async def get_stem_mp3( status_code=422, detail="start and end are both required and start must be less than end", ) + _validate_trim_range(job_id, start, end) # Full-stem requests (no trim) are cached to disk so repeat loads — the # common case for the mobile player — are instant instead of re-encoding. @@ -799,6 +831,7 @@ async def get_mixdown( status_code=422, detail="start and end are both required and start must be less than end", ) + _validate_trim_range(job_id, start, end) # Validates job_id (404), job done (404), and path traversal (404) per # stem -- deliberately before the cache lookup below, so a deleted or @@ -807,7 +840,8 @@ async def get_mixdown( paths = [_validate_stem_path(job_id, name) for name in names] media_type = MIXDOWN_MEDIA_TYPES[ext] - click_lane = _click_lane( + click_lane = await asyncio.to_thread( + _click_lane, job_id, click, click_mult, @@ -958,7 +992,9 @@ async def get_video_mixdown( # Click is one more audio input. It must be appended before the video input # so the audio indices the filter graph references stay contiguous from 0. - click_lane = _click_lane(job_id, click, click_mult, click_accent, click_gain) + click_lane = await asyncio.to_thread( + _click_lane, job_id, click, click_mult, click_accent, click_gain + ) if click_lane is not None: paths = [*paths, click_lane[0]] parsed_gains = [*parsed_gains, click_lane[1]] diff --git a/app/main.py b/app/main.py index 99dc0ff6..9bd29fbe 100644 --- a/app/main.py +++ b/app/main.py @@ -850,17 +850,44 @@ def download_logs_zip() -> StreamingResponse: # The ceiling is far above either editor's reach: 10000 sections with the # longest name each is about 1.6 MB, and 20000 beats about 0.4 MB. Uploads are # unaffected -- they are a different path with their own 400 MB limit. -_EDITOR_BODY_LIMIT = 4 * 1024 * 1024 -_EDITOR_PATH_SUFFIXES = ("/sections", "/beats") +_JSON_BODY_LIMIT = 4 * 1024 * 1024 +# Multipart uploads stream to disk and enforce their own, much larger, limit. + + +def _is_upload(request: Request) -> bool: + ctype = request.headers.get("content-type", "") + return ctype.startswith("multipart/form-data") + + +def _is_chunked(request: Request) -> bool: + return "chunked" in request.headers.get("transfer-encoding", "").lower() @app.middleware("http") -async def limit_editor_body_size(request: Request, call_next): - if request.method in ("PATCH", "POST", "PUT") and request.url.path.endswith( - _EDITOR_PATH_SUFFIXES - ): +async def limit_json_body_size(request: Request, call_next): + """Cap JSON request bodies. + + Scoped by path suffix before, which left every other JSON endpoint + uncapped: /api/search, /api/playlist, /api/settings and the JSON branch of + /api/jobs all await request.json(), and Starlette accumulates the whole + body before json.loads runs it on the event loop. A 200 MB body to + /api/search stalled every other request, including a running job's progress + stream, with no valid job or prior state needed (#481, reopened as #512). + + Uploads are exempt: they are multipart, not JSON, and carry their own + 400 MB limit on a path that streams to disk rather than buffering. + """ + if request.method in ("PATCH", "POST", "PUT") and not _is_upload(request): declared = request.headers.get("content-length") - if declared and declared.isdigit() and int(declared) > _EDITOR_BODY_LIMIT: + if declared is None: + # No Content-Length means chunked, which used to skip the check + # entirely and fall through to an unbounded request.body(). + if _is_chunked(request): + return JSONResponse( + {"detail": "request body must declare its length"}, + status_code=411, + ) + elif declared.isdigit() and int(declared) > _JSON_BODY_LIMIT: return JSONResponse({"detail": "request body too large"}, status_code=413) return await call_next(request) diff --git a/app/pipeline/click_render.py b/app/pipeline/click_render.py index 0db91943..e94709a2 100644 --- a/app/pipeline/click_render.py +++ b/app/pipeline/click_render.py @@ -258,7 +258,11 @@ def _render_events( import numpy as np - buf = np.zeros(total, dtype=np.float64) + # float32, not float64: the buffer is one sample per frame for the whole + # render, so a long export was allocating twice what it needed and then + # again in the int16 conversion below. The output is 16-bit PCM, so the + # extra mantissa was never audible (#512). + buf = np.zeros(total, dtype=np.float32) # Only two distinct voices, so render each once and stamp it in. plain = _voice(CLICK_PEAK, CLICK_FREQ, sample_rate) accented = _voice(ACCENT_PEAK, ACCENT_FREQ, sample_rate) diff --git a/tests/test_jobs_api.py b/tests/test_jobs_api.py index 1561044c..e8447c0a 100644 --- a/tests/test_jobs_api.py +++ b/tests/test_jobs_api.py @@ -386,15 +386,15 @@ def test_oversized_editor_body_is_refused_before_it_is_parsed(client, done_job): check from 32 ms to 5219 ms, and 16 ms once Content-Length was checked in middleware first (#481). """ - from app.main import _EDITOR_BODY_LIMIT + from app.main import _JSON_BODY_LIMIT padded = dict(_section(0), name="V" * 64) - count = (_EDITOR_BODY_LIMIT // len(json.dumps(padded, separators=(",", ":")))) + 500 + count = (_JSON_BODY_LIMIT // len(json.dumps(padded, separators=(",", ":")))) + 500 payload = {"sections": [dict(padded, id=f"sec{i}") for i in range(count)]} # Sent as the exact bytes measured, so the assertion cannot drift from what # actually goes on the wire and quietly stop testing the ceiling. raw = json.dumps(payload, separators=(",", ":")).encode() - assert len(raw) > _EDITOR_BODY_LIMIT + assert len(raw) > _JSON_BODY_LIMIT r = client.patch( f"/api/jobs/{done_job.id}/sections", @@ -410,7 +410,7 @@ def test_the_body_ceiling_clears_the_largest_legitimate_editor_payload(client, d """The ceiling must never be reachable by a real track. 10000 sections at the longest permitted name is about 1.6 MB against a 4 MB ceiling.""" import app.api.jobs as jobs_mod - from app.main import _EDITOR_BODY_LIMIT + from app.main import _JSON_BODY_LIMIT payload = { "sections": [ @@ -418,7 +418,7 @@ def test_the_body_ceiling_clears_the_largest_legitimate_editor_payload(client, d ] } raw = json.dumps(payload, separators=(",", ":")).encode() - assert len(raw) < _EDITOR_BODY_LIMIT + assert len(raw) < _JSON_BODY_LIMIT r = client.patch( f"/api/jobs/{done_job.id}/sections", diff --git a/tests/test_request_body_limits.py b/tests/test_request_body_limits.py new file mode 100644 index 00000000..4eb88092 --- /dev/null +++ b/tests/test_request_body_limits.py @@ -0,0 +1,110 @@ +"""Unbounded request input (#512). + +The body-size guard was scoped to paths ending /sections or /beats, so every +other JSON endpoint accumulated an arbitrarily large body and then ran +json.loads on the event loop. A chunked request skipped the check entirely. + +The trim range had no ceiling either: `end` reaches +np.zeros(int(round(duration * sample_rate))) in the click renderer, so +?start=0&end=20000&count_in=1 asked for a multi-GB allocation. +""" + +from __future__ import annotations + +import pytest +from fastapi.testclient import TestClient + +from app.core.models import Job +from app.core.registry import _jobs +from app.core.registry import register as registry_register + + +@pytest.fixture(autouse=True) +def _clean_jobs(): + _jobs.clear() + yield + _jobs.clear() + + +@pytest.fixture +def client(): + from app.main import app + + with TestClient(app) as c: + yield c + + +@pytest.fixture +def big_body(): + from app.main import _JSON_BODY_LIMIT + + return "x" * (_JSON_BODY_LIMIT + 1024) + + +@pytest.mark.parametrize( + "path", + [ + "/api/search", + "/api/playlist", + "/api/playlist/preview", + "/api/settings", + ], +) +def test_a_huge_json_body_is_refused_before_it_is_parsed(client, path, big_body): + # Previously uncapped: Starlette buffers the whole body, then json.loads + # runs it on the event loop and stalls every other request. + res = client.post( + path, content=f'{{"q": "{big_body}"}}', headers={"content-type": "application/json"} + ) + + assert res.status_code == 413 + + +def test_a_chunked_body_cannot_skip_the_check(client): + # No Content-Length made `declared` None, so the guard fell through to an + # unbounded request.body(). + res = client.post( + "/api/search", + content=iter([b'{"q": "', b"x" * 4096, b'"}']), + headers={"content-type": "application/json", "transfer-encoding": "chunked"}, + ) + + assert res.status_code == 411 + + +def test_a_normal_json_body_still_works(client): + res = client.post("/api/settings", json={"max_duration_sec": 600}) + + assert res.status_code == 200 + + +# ─── trim range ─── + + +def _done_job(job_id="a1b2c3d4e5f6", duration=180.0): + job = Job(id=job_id, status="done", title="Song", duration_sec=duration) + registry_register(job) + return job + + +def test_a_trim_end_beyond_the_track_is_refused(client, tmp_path): + _done_job() + + res = client.get( + "/api/jobs/a1b2c3d4e5f6/mixdown.wav", + params={"stems": "vocals", "gains": "1", "start": 0, "end": 20000, "count_in": 1}, + ) + + assert res.status_code == 422, "an unbounded end reaches a multi-GB np.zeros" + + +def test_a_trim_range_inside_the_track_is_not_refused_by_the_bound(client): + # Must not 422 on the bound; a later 404 for missing stems is fine. + _done_job() + + res = client.get( + "/api/jobs/a1b2c3d4e5f6/mixdown.wav", + params={"stems": "vocals", "gains": "1", "start": 0, "end": 120}, + ) + + assert res.status_code != 422 From b20818e2497c858ed4e531dae901402662c4d9d3 Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Mon, 31 Aug 2026 21:39:42 +0100 Subject: [PATCH 07/19] fix(separate): tear the worker down on any failure, and honour cancel before the CPU retry The persistent worker teardown sat after the finally block, not inside it. An exception out of the read loop -- proc.stderr.read(1) raising OSError when the API thread's terminate() races the read, or _set() raising -- propagated without it, so _worker still held the process and the next job's _get_worker() saw a matching device and a live poll() and reused a worker whose CUDA state followed an exception. ml-pipeline.md is explicit that any non-success must tear it down. A second path missed it entirely: the pipe check raises before the try, so a worker that came back without stdin/stderr stayed cached and would be handed to every subsequent job. Found while writing the test for the first one. separate() also had no cancel check between its two attempts. The rmtree of a multi-GB partial result takes seconds and nothing is registered for cancel during it, so a cancel landing there was invisible and the full CPU pass ran to completion -- 10+ minutes -- before JobCancelled was finally raised. The UI showed "Cancelling" throughout. _kill_worker now reaps after kill(). Without communicate() a worker wedged in an uninterruptible CUDA call becomes a zombie whose pipes close only when the Popen refcount happens to drop; vocal_split.py already pairs the two. An entry-point cancel check was tried and removed. It broke test_cancel_kills_worker_next_job_spawns_fresh, which verifies that a cancelled job tears the worker down so the next one spawns fresh -- a check that never spawns has no worker to tear down, and that test encodes the rule this change is meant to protect. The queue worker already gates dispatch on cancellation, so the fallback check covers the gap that was actually reported. Verified: reverting each half fails its test. Refs #514 --- app/pipeline/separate.py | 33 +++++-- tests/test_separate_worker_lifecycle.py | 116 ++++++++++++++++++++++++ 2 files changed, 142 insertions(+), 7 deletions(-) create mode 100644 tests/test_separate_worker_lifecycle.py diff --git a/app/pipeline/separate.py b/app/pipeline/separate.py index 609ff3d0..52cd4d6a 100644 --- a/app/pipeline/separate.py +++ b/app/pipeline/separate.py @@ -48,6 +48,11 @@ def _kill_worker() -> None: proc.wait(timeout=5) except subprocess.TimeoutExpired: proc.kill() + # kill() only sends the signal. Without the reap a worker wedged in + # an uninterruptible CUDA call becomes a zombie whose pipes are + # closed only incidentally, whenever the Popen refcount happens to + # drop. vocal_split.py already pairs the two. + proc.communicate() def _get_worker(device: str) -> subprocess.Popen: @@ -108,6 +113,10 @@ def _run_demucs(job: Job, source: Path, job_dir: Path, device: str) -> tuple[int spawn_at = time.monotonic() proc = _get_worker(device) if proc.stdin is None or proc.stderr is None: + # Raised before the try below, so nothing else tears this down, and a + # worker without pipes would otherwise stay cached and be handed to + # every subsequent job. + _kill_worker() raise RuntimeError("demucs worker has no stdin/stderr pipe") set_proc(job.id, proc) @@ -203,13 +212,18 @@ def _watchdog() -> None: _done_evt.set() set_proc(job.id, None) wt.join(timeout=2) - - # Never reuse a worker after anything but a clean success: a cancel - # (proc.terminate() from the API thread) already killed it; a failure's - # GPU/CUDA state afterward isn't something we can vouch for. Only the - # happy path keeps the worker warm for the next job. - if job_ok is not True: - _kill_worker() + # Never reuse a worker after anything but a clean success: a cancel + # (proc.terminate() from the API thread) already killed it; a failure's + # GPU/CUDA state afterward isn't something we can vouch for. Only the + # happy path keeps the worker warm for the next job. + # + # Inside the finally, not after it: an exception out of the read loop + # -- proc.stderr.read(1) raising OSError when the API thread's + # terminate() races the read, or _set() raising -- skipped this + # entirely and left a worker whose CUDA state followed an exception + # warm for the next job (#514). + if job_ok is not True: + _kill_worker() # POST /cancel calls proc.terminate() directly, which causes the read # loop above to hit EOF. Translate that into JobCancelled before the @@ -253,6 +267,11 @@ def separate(job: Job, source: Path, job_dir: Path) -> Path: # Partial output from the failed attempt must not be mistaken for # results by collect(); CPU restarts from scratch, so does progress. shutil.rmtree(job_dir / DEMUCS_MODEL, ignore_errors=True) + # The rmtree above can take seconds on a multi-GB partial result, and + # nothing is registered for cancel during it. Re-check before paying + # for a full CPU pass the user has already asked to stop (#514). + if job.cancel_requested: + raise JobCancelled() _set(job, progress=0.0, stage="GPU failed — retrying on CPU (slower)...") job.gpu_fallback = True job.compute_device = f"cpu (fallback from {device})" diff --git a/tests/test_separate_worker_lifecycle.py b/tests/test_separate_worker_lifecycle.py new file mode 100644 index 00000000..8564ae60 --- /dev/null +++ b/tests/test_separate_worker_lifecycle.py @@ -0,0 +1,116 @@ +"""The persistent demucs worker must not survive a failure, and cancel must +land promptly (#514). + +ml-pipeline.md: the worker "is reused across consecutive **successful** jobs on +the same device, but torn down after **any** non-success (cancellation or +failure) -- post-exception CUDA state can't be trusted. Both halves of this +rule matter; don't relax either side." +""" + +from __future__ import annotations + +import pytest + +from app.core.models import Job, JobCancelled +from app.pipeline import separate as _separate + + +@pytest.fixture(autouse=True) +def _no_worker(): + _separate._worker.clear() + yield + _separate._worker.clear() + + +def _job(**kw): + return Job(id="a1b2c3d4e5f6", **kw) + + +def test_an_exception_in_the_stream_loop_still_tears_the_worker_down(tmp_path, monkeypatch): + # The teardown used to sit after the finally, so anything raising out of + # the read loop left a worker whose CUDA state followed an exception warm + # for the next job. + killed = [] + monkeypatch.setattr(_separate, "_kill_worker", lambda: killed.append(True)) + + class _Boom: + stdin = None + stderr = None + + monkeypatch.setattr(_separate, "_get_worker", lambda device: _Boom()) + + job = _job(status="separating") + with pytest.raises(RuntimeError): + _separate._run_demucs(job, tmp_path / "s.wav", tmp_path, "cpu") + + assert killed, "a worker was left warm after an exception" + + +class _FakePipe: + """stdin that accepts the request, stderr that dies mid-stream.""" + + def __init__(self, on_read): + self._on_read = on_read + + def write(self, _data): + return None + + def flush(self): + return None + + def read(self, _n): + return self._on_read() + + +class _FakeProc: + def __init__(self, on_read): + self.stdin = _FakePipe(on_read) + self.stderr = _FakePipe(on_read) + + def poll(self): + return None + + def terminate(self): + return None + + +def test_an_exception_inside_the_read_loop_still_tears_the_worker_down(tmp_path, monkeypatch): + """The case #514 is actually about: teardown sat *after* the finally, so an + OSError from proc.stderr.read(1) -- which happens when the API thread's + terminate() races the read -- propagated with the worker left warm.""" + killed = [] + monkeypatch.setattr(_separate, "_kill_worker", lambda: killed.append(True)) + + def _boom(): + raise OSError("broken pipe") + + monkeypatch.setattr(_separate, "_get_worker", lambda device: _FakeProc(_boom)) + monkeypatch.setattr(_separate, "set_proc", lambda *a, **kw: None) + + job = _job(status="separating") + with pytest.raises(OSError): + _separate._run_demucs(job, tmp_path / "s.wav", tmp_path, "cpu") + + assert killed, "a worker whose CUDA state followed an exception stayed warm" + + +def test_cancel_between_the_gpu_attempt_and_the_cpu_fallback_is_honoured(tmp_path, monkeypatch): + # The expensive case: without this the entire CPU separation ran to + # completion -- 10+ minutes -- while the UI showed "Cancelling". + attempts = [] + + def _fake_run(job, source, job_dir, device): + attempts.append(device) + if device != "cpu": + job.cancel_requested = True # the user cancels during the failure + return 1, ["boom"] + raise AssertionError("the CPU fallback must not run for a cancelled job") + + monkeypatch.setattr(_separate, "_run_demucs", _fake_run) + monkeypatch.setattr(_separate, "get_demucs_device", lambda: "cuda") + + job = _job(status="separating") + with pytest.raises(JobCancelled): + _separate.separate(job, tmp_path / "s.wav", tmp_path) + + assert attempts == ["cuda"] From b11f22f9c44c5fcd365953c464fe4c3e12a7d284 Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Mon, 31 Aug 2026 21:45:46 +0100 Subject: [PATCH 08/19] fix(pipeline): let cancellation reach the processes it is meant to stop Three stages ignored cancel entirely. The local-upload ffmpeg calls used subprocess.run(), which cannot be interrupted: POST /cancel sets the flag but nothing looks at it until the call returns. Cancelling during "Preparing audio..." on a 400 MB .mp4 was a no-op for up to TIMEOUT_FFMPEG per call, twice over on that path since it runs both the video extract and the transcode. Both go through _run_registered_ffmpeg now, mirroring collect._run_ffmpeg, which registers for exactly this reason. Only demucs_worker armed the parent-death watchdog, and only separate.py exported STEMDECK_PARENT_PID. A Force-Quit during a vocal split therefore orphaned an onnxruntime process holding the GPU with nobody to collect the result, and a section pass outlived the parent whose TIMEOUT_SECTIONS was its only bound. The watchdog moves to app/core/process.py -- where process_exists already lived for it -- and all three workers arm it, all three spawn sites export the pid. Poll interval stays 1.0s, matching what demucs_worker used. A running vocal split was uncancellable by construction: cancel_job returns early for a done job, and a split only ever runs on a done job, so the flag was never even set while the split held _pipeline_lock and stalled the import queue for its full duration. Cancel now terminates the worker for that case; the split's own error path marks it failed and releases the lock. _run_registered_ffmpeg deregisters in a finally, so a failure cannot leave a stale entry that a later cancel would terminate on the wrong job. Two existing test files needed updating rather than fixing: test_worker_parent_watchdog targeted demucs_worker._arm_parent_watchdog, now app.core.process.arm_parent_watchdog; test_video_status stubbed subprocess.run, which the extract no longer calls. Behaviour is unchanged in both. Refs #519 --- app/api/jobs.py | 10 ++ app/core/process.py | 50 ++++++++ app/pipeline/demucs_worker.py | 45 +------ app/pipeline/runner.py | 35 +++++- app/pipeline/section_worker.py | 4 + app/pipeline/sections.py | 4 + app/pipeline/vocal_split.py | 4 + app/pipeline/vocal_split_worker.py | 5 + tests/test_cancellation_reach.py | 170 +++++++++++++++++++++++++++ tests/test_video_status.py | 8 +- tests/test_worker_parent_watchdog.py | 12 +- 11 files changed, 291 insertions(+), 56 deletions(-) create mode 100644 tests/test_cancellation_reach.py diff --git a/app/api/jobs.py b/app/api/jobs.py index f2be8f41..4f16765f 100644 --- a/app/api/jobs.py +++ b/app/api/jobs.py @@ -326,6 +326,16 @@ def cancel_job(job_id: str) -> dict: if job is None: raise HTTPException(status_code=404, detail="job not found") if job.status in ("done", "error", "cancelled"): + # A vocal split only ever runs on a done job, so this early return made + # it uncancellable by construction: the flag was never even set, while + # the split held _pipeline_lock and stalled the whole import queue for + # its full duration (#519). Terminating the worker is enough -- the + # split's own error path marks it failed and releases the lock. + if job.vocal_split == "running": + job.cancel_requested = True + proc = registry_get_proc(job_id) + if proc is not None and proc.poll() is None: + proc.terminate() return job.to_state() job.cancel_requested = True diff --git a/app/core/process.py b/app/core/process.py index 609372ba..965c4e21 100644 --- a/app/core/process.py +++ b/app/core/process.py @@ -38,3 +38,53 @@ def process_exists(pid: int) -> bool: kernel32.CloseHandle(handle) return True return ctypes.get_last_error() != ERROR_INVALID_PARAMETER + + +_PARENT_POLL_SECONDS = 1.0 + + +def _watch_parent(parent_pid: int) -> None: + """Exit as soon as the process that spawned us is gone. + + A worker's stdin EOF only covers a parent that exits between jobs. + Mid-inference the worker reads nothing, and the parent may have been killed + in a way that ran no cleanup at all (SIGKILL, Force Quit, Task Manager, a + crash). Without this the worker keeps running -- holding a GPU, in the + demucs and vocal-split cases -- with nobody left to collect the result. + + os._exit rather than sys.exit: this runs on a daemon thread, and raising + SystemExit there would not interrupt inference running in C code. Nothing + here needs flushing. + """ + import sys + import time + + while True: + if not process_exists(parent_pid): + sys.stderr.write("@@ERROR@@parent process exited\n") + sys.stderr.flush() + os._exit(1) + time.sleep(_PARENT_POLL_SECONDS) + + +def arm_parent_watchdog() -> None: + """Start the parent-death watchdog if the parent asked for one. + + Shared by every long-running worker. It lived in demucs_worker.py, which is + why vocal_split_worker and section_worker never had it: a Force-Quit during + a vocal split orphaned an onnxruntime process holding the GPU, and a + section pass outlived the parent whose TIMEOUT_SECTIONS was its only bound + (#519). + """ + import threading + + raw = os.environ.get("STEMDECK_PARENT_PID", "").strip() + if not raw: + return + try: + parent_pid = int(raw) + except ValueError: + return + if parent_pid <= 0 or parent_pid == os.getpid(): + return + threading.Thread(target=_watch_parent, args=(parent_pid,), daemon=True).start() diff --git a/app/pipeline/demucs_worker.py b/app/pipeline/demucs_worker.py index 482cc0ae..dc679ca8 100644 --- a/app/pipeline/demucs_worker.py +++ b/app/pipeline/demucs_worker.py @@ -35,14 +35,11 @@ from __future__ import annotations import json -import os import sys -import threading -import time from pathlib import Path from app.core.config import DEMUCS_MODEL -from app.core.process import process_exists +from app.core.process import arm_parent_watchdog def _run_one_job(model, device: str, req: dict) -> None: @@ -89,47 +86,9 @@ def _run_one_job(model, device: str, req: dict) -> None: ) -_PARENT_POLL_SECONDS = 1.0 - - -def _watch_parent(parent_pid: int) -> None: - """Exit as soon as the process that spawned us is gone. - - The stdin EOF in the loop below only covers a parent that exits between - jobs. Mid-separation the worker is inside torch and reads nothing, and the - parent may have been killed in a way that ran no cleanup at all (SIGKILL, - Force Quit, Task Manager, a crash). Without this, the worker would keep a - GPU busy with nobody left to collect the result. - - os._exit rather than sys.exit: this runs on a daemon thread, and raising - SystemExit there would not interrupt inference running in C code. Nothing - here needs flushing -- a half-written model directory is cleared before the - job is retried. - """ - while True: - if not process_exists(parent_pid): - sys.stderr.write("@@ERROR@@parent process exited\n") - sys.stderr.flush() - os._exit(1) - time.sleep(_PARENT_POLL_SECONDS) - - -def _arm_parent_watchdog() -> None: - raw = os.environ.get("STEMDECK_PARENT_PID", "").strip() - if not raw: - return - try: - parent_pid = int(raw) - except ValueError: - return - if parent_pid <= 0 or parent_pid == os.getpid(): - return - threading.Thread(target=_watch_parent, args=(parent_pid,), daemon=True).start() - - def main() -> None: device = sys.argv[1] if len(sys.argv) > 1 else "cpu" - _arm_parent_watchdog() + arm_parent_watchdog() from demucs.pretrained import get_model diff --git a/app/pipeline/runner.py b/app/pipeline/runner.py index 07603ab4..f4b3c6a0 100644 --- a/app/pipeline/runner.py +++ b/app/pipeline/runner.py @@ -14,6 +14,7 @@ from app.core.models import Job, JobCancelled, _set from app.core.redact import redact from app.core.registry import persist as persist_registry +from app.core.registry import set_proc from app.pipeline.analyze import analyze from app.pipeline.beatgrid import compute_beat_grid from app.pipeline.collect import ( @@ -49,6 +50,30 @@ def _check_cancel(job: Job) -> None: raise JobCancelled() +def _run_registered_ffmpeg(job: Job, cmd: list[str], timeout: int) -> tuple[int, bytes]: + """Run ffmpeg with the process registered, so cancel can reach it. + + subprocess.run() cannot be interrupted: POST /cancel sets the flag, but + nothing looks at it until the call returns, so a cancel during a large + upload's transcode was a no-op for up to TIMEOUT_FFMPEG per call -- twice + over on the .mp4 path, which runs both this and the video extract (#519). + + Mirrors collect._run_ffmpeg, which registers for exactly this reason. + """ + proc = subprocess.Popen(cmd, stdout=subprocess.DEVNULL, stderr=subprocess.PIPE) + set_proc(job.id, proc) + try: + try: + _, stderr = proc.communicate(timeout=timeout) + except subprocess.TimeoutExpired: + proc.kill() + proc.communicate() + raise + return proc.returncode, stderr or b"" + finally: + set_proc(job.id, None) + + def _extract_video_track(job: Job, source: Path, job_dir: Path) -> None: """For an .mp4 upload, preserve a silent video-only track at video.mp4 so the studio can later mux it with a custom stem mix @@ -76,7 +101,7 @@ def _extract_video_track(job: Job, source: Path, job_dir: Path) -> None: str(dest), ] try: - result = subprocess.run(cmd, capture_output=True, timeout=TIMEOUT_FFMPEG) + returncode, _ = _run_registered_ffmpeg(job, cmd, TIMEOUT_FFMPEG) except (OSError, subprocess.SubprocessError) as e: # ffmpeg missing or timed out. Distinct from an .mp4 that simply has no # video stream, and the only one of the two worth surfacing (#436). @@ -84,7 +109,7 @@ def _extract_video_track(job: Job, source: Path, job_dir: Path) -> None: job.video_status = "failed" logger.warning("video extract failed for job %s: %s", job.id, e) return - if result.returncode != 0 or not dest.is_file() or dest.stat().st_size == 0: + if returncode != 0 or not dest.is_file() or dest.stat().st_size == 0: dest.unlink(missing_ok=True) job.video_status = "unavailable" logger.info("no video track preserved for job %s (source has no video stream?)", job.id) @@ -127,10 +152,10 @@ def _prepare_local_source(job: Job, source: Path, job_dir: Path) -> Path: "-y", str(dest), ] - result = subprocess.run(cmd, capture_output=True, timeout=TIMEOUT_FFMPEG) - if result.returncode != 0: + returncode, stderr = _run_registered_ffmpeg(job, cmd, TIMEOUT_FFMPEG) + if returncode != 0: raise RuntimeError( - "ffmpeg transcode failed: " + result.stderr.decode("utf-8", errors="replace").strip() + "ffmpeg transcode failed: " + stderr.decode("utf-8", errors="replace").strip() ) source.unlink(missing_ok=True) return dest diff --git a/app/pipeline/section_worker.py b/app/pipeline/section_worker.py index 0d336317..7998404d 100644 --- a/app/pipeline/section_worker.py +++ b/app/pipeline/section_worker.py @@ -10,6 +10,7 @@ import threading from pathlib import Path +from app.core.process import arm_parent_watchdog from app.pipeline.section_refine import refine_segments _HEARTBEAT_SECONDS = 10 @@ -66,6 +67,9 @@ def _load_beat_grid(path: Path | None) -> object | None: def main(argv: list[str] | None = None) -> int: + # Without this a CPU inference pass outlives the parent whose + # TIMEOUT_SECTIONS was its only bound (#519). + arm_parent_watchdog() args = _parser().parse_args(argv) for path in (args.stems_dir / f"{name}.wav" for name in ("bass", "drums", "other", "vocals")): if not path.is_file(): diff --git a/app/pipeline/sections.py b/app/pipeline/sections.py index 7e509b40..0a994f16 100644 --- a/app/pipeline/sections.py +++ b/app/pipeline/sections.py @@ -214,6 +214,10 @@ def _terminate(proc: subprocess.Popen) -> None: def _run_registered_process(job: Job, cmd: list[str]) -> tuple[int, list[str], list[str]]: """Run a child with cancellation, total timeout, and output-stall detection.""" env = os.environ.copy() + # The worker arms a watchdog on this and hard-exits when we disappear, so a + # kill that runs no cleanup (SIGKILL, Force Quit, Task Manager, a crash) + # cannot leave it running with nobody to collect the result (#519). + env["STEMDECK_PARENT_PID"] = str(os.getpid()) env["PYTHONIOENCODING"] = "utf-8:replace" proc = subprocess.Popen( cmd, diff --git a/app/pipeline/vocal_split.py b/app/pipeline/vocal_split.py index 65cb224e..a3d1ebf8 100644 --- a/app/pipeline/vocal_split.py +++ b/app/pipeline/vocal_split.py @@ -61,6 +61,10 @@ def split_vocals(job: Job, stems_dir: Path) -> list[str]: # the mismatch simply moves rather than being fixed. Demucs and audio- # separator both emit progress bars and can echo track metadata, neither of # which is guaranteed to be cp1252-safe. + # The worker arms a watchdog on this and hard-exits when we disappear, so a + # kill that runs no cleanup (SIGKILL, Force Quit, Task Manager, a crash) + # cannot leave it running with nobody to collect the result (#519). + env["STEMDECK_PARENT_PID"] = str(os.getpid()) env["PYTHONIOENCODING"] = "utf-8:replace" try: import certifi diff --git a/app/pipeline/vocal_split_worker.py b/app/pipeline/vocal_split_worker.py index 116b9eb1..fe305c6b 100644 --- a/app/pipeline/vocal_split_worker.py +++ b/app/pipeline/vocal_split_worker.py @@ -24,6 +24,7 @@ import sys from app.core.config import VOCAL_SPLIT_MODEL +from app.core.process import arm_parent_watchdog def _run(device: str, vocals_path: str, out_dir: str) -> None: @@ -57,6 +58,10 @@ def _run(device: str, vocals_path: str, out_dir: str) -> None: def main() -> None: + # A Force-Quit of the app otherwise orphans this process holding the GPU: + # onnxruntime reads nothing from stdin mid-inference, so EOF never arrives + # and nothing else bounds it (#519). + arm_parent_watchdog() if len(sys.argv) < 4: sys.stderr.write("@@ERROR@@usage: vocal_split_worker \n") sys.stderr.flush() diff --git a/tests/test_cancellation_reach.py b/tests/test_cancellation_reach.py new file mode 100644 index 00000000..8f0a79b8 --- /dev/null +++ b/tests/test_cancellation_reach.py @@ -0,0 +1,170 @@ +"""Cancellation has to reach the processes it is meant to stop (#519). + +python-fastapi.md: "Always register subprocess with set_proc(job_id, proc) +immediately after Popen(); deregister in finally." Several stages used +subprocess.run(), which cannot be interrupted -- the flag is set but nothing +looks at it until the call returns. +""" + +from __future__ import annotations + +import os + +import pytest + +from app.core import process as _process +from app.core.models import Job +from app.core.registry import register as registry_register +from app.pipeline import runner as _runner + + +def _job(**kw): + return Job(id="a1b2c3d4e5f6", **kw) + + +def test_ffmpeg_is_registered_so_cancel_can_reach_it(tmp_path, monkeypatch): + seen = {} + + def _capture(job_id, proc): + # Registered while it runs, cleared after: both halves matter. + seen.setdefault("during", proc if proc is not None else seen.get("during")) + seen["last"] = proc + + monkeypatch.setattr(_runner, "set_proc", _capture) + + job = _job() + rc, _ = _runner._run_registered_ffmpeg(job, ["sh", "-c", "exit 0"], 30) + + assert rc == 0 + assert seen["during"] is not None, "cancel could not have reached this process" + assert seen["last"] is None, "the registration must be cleared in a finally" + + +def test_a_failing_command_still_deregisters(tmp_path, monkeypatch): + cleared = [] + monkeypatch.setattr(_runner, "set_proc", lambda job_id, proc: cleared.append(proc)) + + job = _job() + rc, stderr = _runner._run_registered_ffmpeg(job, ["sh", "-c", "echo boom >&2; exit 3"], 30) + + assert rc == 3 + assert b"boom" in stderr, "stderr must still be captured for the error message" + assert cleared[-1] is None + + +# ─── parent-death watchdog ─── + + +def test_the_watchdog_arms_when_the_parent_asks(monkeypatch): + started = [] + monkeypatch.setenv("STEMDECK_PARENT_PID", "999999") + monkeypatch.setattr( + "threading.Thread", + lambda *a, **kw: type("T", (), {"start": lambda self: started.append(True)})(), + ) + + _process.arm_parent_watchdog() + + assert started + + +@pytest.mark.parametrize("value", ["", "not-a-number", "0", "-1"]) +def test_the_watchdog_stays_off_without_a_usable_parent_pid(monkeypatch, value): + started = [] + monkeypatch.setenv("STEMDECK_PARENT_PID", value) + monkeypatch.setattr( + "threading.Thread", + lambda *a, **kw: type("T", (), {"start": lambda self: started.append(True)})(), + ) + + _process.arm_parent_watchdog() + + assert not started + + +def test_the_watchdog_never_targets_our_own_pid(monkeypatch): + # Would hard-exit the worker the moment it started. + started = [] + monkeypatch.setenv("STEMDECK_PARENT_PID", str(os.getpid())) + monkeypatch.setattr( + "threading.Thread", + lambda *a, **kw: type("T", (), {"start": lambda self: started.append(True)})(), + ) + + _process.arm_parent_watchdog() + + assert not started + + +def test_every_worker_spawn_exports_the_parent_pid(): + # demucs_worker had this; the other two did not, so a Force-Quit orphaned + # an onnxruntime process holding the GPU. + import pathlib + + for path in ( + "app/pipeline/separate.py", + "app/pipeline/vocal_split.py", + "app/pipeline/sections.py", + ): + src = pathlib.Path(path).read_text() + assert "STEMDECK_PARENT_PID" in src, f"{path} spawns a worker without the watchdog" + + +def test_every_worker_arms_the_watchdog(): + import pathlib + + for path in ( + "app/pipeline/demucs_worker.py", + "app/pipeline/vocal_split_worker.py", + "app/pipeline/section_worker.py", + ): + src = pathlib.Path(path).read_text() + assert "arm_parent_watchdog()" in src, f"{path} never arms the watchdog" + + +# ─── a running vocal split must be cancellable ─── + + +class _LiveProc: + def __init__(self): + self.terminated = False + + def poll(self): + return None + + def terminate(self): + self.terminated = True + + +def test_cancelling_a_running_vocal_split_terminates_it(monkeypatch): + # cancel_job returns early for a done job -- and a vocal split only ever + # runs on a done job, so it was uncancellable by construction while holding + # _pipeline_lock and stalling the import queue. + from app.api import jobs as _jobs_api + + job = _job(status="done", title="Song") + job.vocal_split = "running" + registry_register(job) + + proc = _LiveProc() + monkeypatch.setattr(_jobs_api, "registry_get_proc", lambda job_id: proc) + + _jobs_api.cancel_job(job.id) + + assert job.cancel_requested is True + assert proc.terminated, "the split ran to completion with the cancel button doing nothing" + + +def test_cancelling_a_plain_done_job_still_does_nothing(monkeypatch): + from app.api import jobs as _jobs_api + + job = _job(status="done", title="Song") + registry_register(job) + + proc = _LiveProc() + monkeypatch.setattr(_jobs_api, "registry_get_proc", lambda job_id: proc) + + _jobs_api.cancel_job(job.id) + + assert not proc.terminated + assert job.cancel_requested is False diff --git a/tests/test_video_status.py b/tests/test_video_status.py index 7a6b3a80..d89a71e5 100644 --- a/tests/test_video_status.py +++ b/tests/test_video_status.py @@ -97,14 +97,16 @@ def _run_local_extract(tmp_path: Path, returncode: int, raises=None) -> Job: source = job_dir / "in.mp4" source.write_bytes(b"x") - def fake_run(cmd, **kwargs): + # Stubs _run_registered_ffmpeg rather than subprocess.run: the extract goes + # through Popen + set_proc now, so cancel can reach it (#519). + def fake_run(job_arg, cmd, timeout): if raises is not None: raise raises if returncode == 0: Path(cmd[-1]).write_bytes(b"fake mp4 payload") - return subprocess.CompletedProcess(cmd, returncode, b"", b"") + return returncode, b"" - with patch.object(runner_mod.subprocess, "run", fake_run): + with patch.object(runner_mod, "_run_registered_ffmpeg", fake_run): runner_mod._extract_video_track(job, source, job_dir) return job diff --git a/tests/test_worker_parent_watchdog.py b/tests/test_worker_parent_watchdog.py index d7901336..583f8970 100644 --- a/tests/test_worker_parent_watchdog.py +++ b/tests/test_worker_parent_watchdog.py @@ -45,8 +45,8 @@ def test_worker_exits_when_its_parent_disappears(tmp_path): script = textwrap.dedent( """ import sys, time - from app.pipeline.demucs_worker import _arm_parent_watchdog - _arm_parent_watchdog() + from app.core.process import arm_parent_watchdog + arm_parent_watchdog() # Busy the way a separation is busy: never reading stdin, so only the # watchdog can end this process. while True: @@ -83,18 +83,20 @@ def test_worker_exits_when_its_parent_disappears(tmp_path): def test_worker_ignores_an_unset_or_bogus_parent_pid(monkeypatch): """A worker run by hand (no STEMDECK_PARENT_PID) must not arm the watchdog and shoot itself.""" - from app.pipeline import demucs_worker + import threading as _threading + + from app.core import process as _process_mod started: list[object] = [] monkeypatch.setattr( - demucs_worker.threading, + _threading, "Thread", lambda *a, **k: started.append((a, k)) or _NoopThread(), ) for value in ("", " ", "not-a-number", "0", "-5", str(os.getpid())): monkeypatch.setenv("STEMDECK_PARENT_PID", value) - demucs_worker._arm_parent_watchdog() + _process_mod.arm_parent_watchdog() assert started == [], "watchdog armed on a pid it should have ignored" From a404c7ff3c0bd98c50488d0cce68f2c25713daf8 Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Mon, 31 Aug 2026 18:18:16 +0100 Subject: [PATCH 09/19] fix(updater): only install app updates from our own release assets download_app_update took its URL straight from the WebView and verified the download against a SHA-256 supplied by the same caller, so the checksum proved the bytes arrived intact -- not that they came from us. apply_app_update then extracts that archive over StemDeck's own executable and backend/ and relaunches. Reachability is narrower than it first looks: in the normal flow the URL is appAsset.browser_download_url from the GitHub API, no XSS was found, and the CSP is script-src 'self'. But the page is served over http by the Python backend, which Tauri treats as a remote origin, and these app-defined commands are not ACL-gated by the capability config -- both facts the code already documents. A LAN attacker reaches this once network access is enabled. validate_release_url pins the host to github.com and objects.githubusercontent.com (GitHub redirects release assets to the latter) and requires https, so bytes cannot be swapped in flight on a network where the page itself is already plain http. Deliberately a new function rather than the existing validate_download_url: that one permits only 127.0.0.1/localhost, serves a different caller, and would reject every legitimate release URL. apply_app_update also re-verifies now. It trusted that whatever sat at the archive path was what download_app_update had approved, so anything able to write into data/downloads between the two calls was extracted unchecked. The verified digest is recorded next to the archive and re-checked before extraction. The test covers host lookalikes (github.com.evil.example, notgithub.com) as well as plain rejection, since a substring check would pass those. Refs #510 --- desktop/src-tauri/src/main.rs | 85 ++++++++++++++++++++++++++++++++++- 1 file changed, 84 insertions(+), 1 deletion(-) diff --git a/desktop/src-tauri/src/main.rs b/desktop/src-tauri/src/main.rs index 7ade5bae..0b8bf300 100644 --- a/desktop/src-tauri/src/main.rs +++ b/desktop/src-tauri/src/main.rs @@ -1000,8 +1000,14 @@ async fn download_app_update( // never install something the current plan did not ask for. let _ = fs::remove_file(&app_archive); + validate_release_url(&plan.app_url)?; download_file_with_progress(&plan.app_url, &app_archive, &app_handle).await?; - verify_update_sha256(&app_archive, &plan.app_sha256, "app update") + verify_update_sha256(&app_archive, &plan.app_sha256, "app update")?; + // Record what was verified so apply_app_update can check the bytes it + // is about to extract, rather than trusting that whatever now sits at + // this path is what this function approved. + let _ = fs::write(app_sha_path(&downloads), plan.app_sha256.trim()); + Ok(()) } } @@ -1010,6 +1016,13 @@ async fn download_app_update( /// the frontend) before it is ever extracted. On mismatch the file is removed /// so a corrupt or tampered download can never be applied. #[cfg(any(windows, target_os = "linux"))] +/// Where download_app_update records the checksum it verified, so +/// apply_app_update can re-check the bytes it is about to extract. +#[cfg(any(windows, target_os = "linux"))] +fn app_sha_path(downloads: &Path) -> PathBuf { + downloads.join(format!("{UPDATE_APP_ARCHIVE}.sha256")) +} + fn verify_update_sha256(path: &Path, expected: &str, label: &str) -> Result<(), String> { let actual = sha256_file(path)?; if !actual.eq_ignore_ascii_case(expected.trim()) { @@ -1181,6 +1194,13 @@ fn apply_app_update( "no downloaded app update found -- call download_app_update first".to_string(), ); } + // Re-verify rather than trusting the path. download_app_update checked + // these bytes, but anything able to write into data/downloads between + // the two calls would otherwise be extracted over the live install + // unchecked (#510). + let recorded = fs::read_to_string(app_sha_path(&downloads)) + .map_err(|_| "no verified checksum for the downloaded update -- download it again")?; + verify_update_sha256(&app_archive, recorded.trim(), "app update")?; // ── Phase 1: stage and validate, touching nothing live ── // @@ -2485,6 +2505,38 @@ fn open_url(url: String) -> Result<(), String> { /// Only localhost URLs, and only http(s). Guards against a compromised WebView /// using the desktop shell as an SSRF proxy (#138). +/// Hosts an in-app update may be fetched from. +/// +/// GitHub serves release assets from `github.com` and redirects to +/// `objects.githubusercontent.com`, so both have to be here. +const RELEASE_ASSET_HOSTS: [&str; 2] = ["github.com", "objects.githubusercontent.com"]; + +/// Reject an update URL that does not point at our own release assets. +/// +/// `download_app_update` takes its URL from the WebView, and the SHA-256 it +/// checks against comes from the same place -- so the checksum proves the file +/// arrived intact, not that it came from us. Without a host check, anything +/// able to run script on that page can hand the shell an archive that +/// `apply_app_update` then extracts over StemDeck's own executable and +/// backend/ (#510). The page is served over http by the Python backend, which +/// Tauri treats as a remote origin, and these app-defined commands are not +/// ACL-gated by the capability config. +/// +/// Deliberately not `validate_download_url`: that one permits only +/// 127.0.0.1/localhost, for a different caller, and would reject every real +/// release URL. +fn validate_release_url(url: &str) -> Result<(), String> { + let parsed = reqwest::Url::parse(url).map_err(|_| "invalid update URL".to_string())?; + if parsed.scheme() != "https" { + return Err("update URLs must use https".to_string()); + } + let host = parsed.host_str().unwrap_or(""); + if !RELEASE_ASSET_HOSTS.contains(&host) { + return Err(format!("refusing to download an update from {host}")); + } + Ok(()) +} + fn validate_download_url(url: &str) -> Result<(), String> { if !url.starts_with("http://") && !url.starts_with("https://") { return Err("only http/https URLs are permitted".to_string()); @@ -4566,6 +4618,37 @@ mod tests { .unwrap(); } + #[test] + fn only_our_own_release_assets_are_downloadable_as_updates() { + // download_app_update takes its URL from the WebView and checks it + // against a SHA-256 from the same caller, so the checksum proves the + // bytes arrived intact, not that they came from us. apply_app_update + // then extracts the result over StemDeck's own executable (#510). + for ok in [ + "https://github.com/stemdeckapp/stemdeck/releases/download/v0.16.1/x.zip", + "https://objects.githubusercontent.com/github-production-release-asset/1/2", + ] { + assert!(super::validate_release_url(ok).is_ok(), "should allow {ok}"); + } + + for bad in [ + "https://evil.example/x.zip", + // Lookalikes: the check must be on the host, not a substring of it. + "https://github.com.evil.example/x.zip", + "https://notgithub.com/x.zip", + // Plain http would let a LAN attacker swap the bytes in flight, + // which matters because the page itself is served over http. + "http://github.com/stemdeckapp/stemdeck/releases/download/v1/x.zip", + "file:///etc/passwd", + "not a url", + ] { + assert!( + super::validate_release_url(bad).is_err(), + "should reject {bad}" + ); + } + } + #[test] fn legacy_migration_preserves_user_settings_when_data_dir_already_exists() { // setup() creates the destination before ensure_workspace() invokes From 845b5acaf306c4115095fbcc5fc7f1b2b7a37ef4 Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Mon, 31 Aug 2026 18:21:04 +0100 Subject: [PATCH 10/19] fix(setup): drain child pipes while the child runs, not after it exits child_output_with_timeout read stdout and stderr only once try_wait() had reported an exit. A child that outruns the OS pipe buffer blocks in write() with nobody reading, so it never exits, so try_wait() never reports an exit, and the call ends at the timeout with no output at all. warmup_models pipes both streams, and its model downloads emit tqdm progress to stderr in proportion to how long they take rather than how large they are. So the failure lands on slow connections -- the users warmup exists to spare a mid-pipeline download. Each stream now drains on its own thread. Both are needed: draining one and then the other reintroduces the deadlock on whichever is second. The readers are joined rather than detached on the timeout path too, since killing the child closes its ends and dropping the handles would leak two threads per timeout. command_output_with_timeout had the same shape and now delegates, so the two cannot drift apart again. The test writes 512 KiB to each stream, comfortably past any pipe buffer. Against the old implementation it fails after burning the full 20-second timeout; with concurrent draining it completes in 0.11s. Worth noting because a measurement of a real cold-cache warmup on a fast connection showed only 8,360 bytes of stderr -- under the buffer, which is why this had not been seen in practice rather than why it could not happen. Refs #516 --- desktop/src-tauri/src/main.rs | 129 ++++++++++++++++++++++------------ 1 file changed, 83 insertions(+), 46 deletions(-) diff --git a/desktop/src-tauri/src/main.rs b/desktop/src-tauri/src/main.rs index 0b8bf300..f83e8c86 100644 --- a/desktop/src-tauri/src/main.rs +++ b/desktop/src-tauri/src/main.rs @@ -4410,34 +4410,65 @@ fn update_setup_config( /// Polls an already-spawned child until it exits or the timeout elapses. /// Mirrors command_output_with_timeout but accepts a pre-spawned Child so the /// caller can record the PID before waiting (e.g. to kill on window close). +/// Wait for `child`, draining its pipes while it runs. +/// +/// The draining is the point. Reading only after `try_wait()` reports an exit +/// deadlocks any child that outruns the OS pipe buffer: it blocks in `write()` +/// with nobody reading, so it never exits, so `try_wait()` never reports an +/// exit, and the whole thing ends at the timeout instead. `warmup_models` +/// pipes both streams and its model downloads emit tqdm progress to stderr in +/// proportion to how long they take -- so the failure lands on slow +/// connections, the users warmup exists to help (#516). +/// +/// Each stream gets its own thread because both must drain concurrently; +/// draining one and then the other reintroduces the deadlock on whichever is +/// second. fn child_output_with_timeout( mut child: Child, timeout: Duration, label: &str, ) -> Result { + let stdout_reader = child.stdout.take().map(|mut pipe| { + thread::spawn(move || { + let mut buf = Vec::new(); + let _ = pipe.read_to_end(&mut buf); + buf + }) + }); + let stderr_reader = child.stderr.take().map(|mut pipe| { + thread::spawn(move || { + let mut buf = Vec::new(); + let _ = pipe.read_to_end(&mut buf); + buf + }) + }); + + let collect = |reader: Option>>| { + reader.and_then(|h| h.join().ok()).unwrap_or_default() + }; + let deadline = Instant::now() + timeout; loop { if let Some(status) = child .try_wait() .map_err(|e| format!("failed to wait for {label}: {e}"))? { - let mut stdout = Vec::new(); - if let Some(mut pipe) = child.stdout.take() { - let _ = pipe.read_to_end(&mut stdout); - } - let mut stderr = Vec::new(); - if let Some(mut pipe) = child.stderr.take() { - let _ = pipe.read_to_end(&mut stderr); - } + // The child is gone, so both pipes are at EOF and these joins + // return promptly. return Ok(Output { status, - stdout, - stderr, + stdout: collect(stdout_reader), + stderr: collect(stderr_reader), }); } if Instant::now() >= deadline { let _ = child.kill(); let _ = child.wait(); + // Joined rather than detached: killing the child closes its ends, + // so the readers finish, and dropping the handles without joining + // would leak two threads per timeout. + let _ = collect(stdout_reader); + let _ = collect(stderr_reader); return Err(format!( "{label} timed out after {} seconds", timeout.as_secs() @@ -4452,44 +4483,12 @@ fn command_output_with_timeout( timeout: Duration, label: &str, ) -> Result { - let mut child = command + let child = command .spawn() .map_err(|e| format!("failed to start {label}: {e}"))?; - let deadline = Instant::now() + timeout; - - loop { - if let Some(status) = child - .try_wait() - .map_err(|e| format!("failed to wait for {label}: {e}"))? - { - let mut stdout = Vec::new(); - if let Some(mut pipe) = child.stdout.take() { - let _ = pipe.read_to_end(&mut stdout); - } - - let mut stderr = Vec::new(); - if let Some(mut pipe) = child.stderr.take() { - let _ = pipe.read_to_end(&mut stderr); - } - - return Ok(Output { - status, - stdout, - stderr, - }); - } - - if Instant::now() >= deadline { - let _ = child.kill(); - let _ = child.wait(); - return Err(format!( - "{label} timed out after {} seconds", - timeout.as_secs() - )); - } - - thread::sleep(Duration::from_millis(100)); - } + // Same pipe-draining requirement as child_output_with_timeout; sharing it + // keeps the two from drifting apart again (#516). + child_output_with_timeout(child, timeout, label) } #[cfg(windows)] @@ -4649,6 +4648,44 @@ mod tests { } } + #[test] + #[cfg(unix)] + fn a_chatty_child_is_drained_rather_than_deadlocked() { + // Reading the pipes only after try_wait() reports an exit deadlocks any + // child that outruns the OS pipe buffer (64 KiB on Linux, smaller on + // macOS): it blocks in write() with nobody reading, so it never exits. + // warmup_models pipes both streams and its downloads emit tqdm progress + // to stderr in proportion to how long they take, so the old code failed + // for users on slow connections after burning the full 30-minute + // timeout (#516). + // + // 512 KiB on each stream is comfortably past any pipe buffer. A short + // timeout keeps the failure mode obvious: without concurrent draining + // this returns Err(timed out) instead of the output. + let mut command = Command::new("sh"); + command + .arg("-c") + .arg("yes stdoutstdoutstdout | head -c 524288; yes errerrerr | head -c 524288 >&2") + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + + let output = + super::command_output_with_timeout(command, Duration::from_secs(20), "chatty child") + .expect("a child that fills its pipes must still be collected"); + + assert!(output.status.success()); + assert_eq!( + output.stdout.len(), + 524_288, + "stdout must be drained in full" + ); + assert_eq!( + output.stderr.len(), + 524_288, + "stderr must be drained in full" + ); + } + #[test] fn legacy_migration_preserves_user_settings_when_data_dir_already_exists() { // setup() creates the destination before ensure_workspace() invokes From 1ab8c744c7917850ce6c47a614340ec2eca20ecf Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Mon, 31 Aug 2026 18:24:36 +0100 Subject: [PATCH 11/19] fix(linux): verify the FFmpeg download against a pinned checksum download_linux_ffmpeg fetched a tarball from a rolling URL on a single host, extracted it with the system tar, marked the binaries executable and ran them, with no integrity check of any kind. Windows verifies against BtbN's published checksums.sha256 and macOS against four pinned hashes; Linux verified nothing. verify_ffmpeg only proves the binary runs and has the encoders we need, which says nothing about where it came from. Pinned rather than verified against upstream's .md5 companion, which was the obvious move but is worth less than it looks: MD5 is broken for collisions, and the companion is served by the same host as the tarball, so anyone able to replace one can replace the other. It evidences corruption, not authenticity. The pinned hash was computed from the artifact whose MD5 matched upstream's published 7fa72b652e19bf84c9461e332ea1cdf3, so this pin is anchored to what upstream currently vouches for. The URL is a rolling one, so this needs a manual bump when upstream publishes a new build; the current one is dated 2024-08-24. A stale pin fails closed with a checksum error rather than silently accepting whatever arrives. STEMDECK_FFMPEG_URL still overrides, and skips the check -- an override points somewhere we cannot have a hash for, so vouching for it is the caller's business, matching how the macOS override already behaves. Note this code cannot be compiled on macOS or by ci.yml, which does not build the Rust shell at all. It was type-checked by temporarily widening its cfg gate to #[cfg(unix)]; cargo check reported no errors. Refs #518 --- desktop/src-tauri/src/main.rs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/desktop/src-tauri/src/main.rs b/desktop/src-tauri/src/main.rs index f83e8c86..599bb843 100644 --- a/desktop/src-tauri/src/main.rs +++ b/desktop/src-tauri/src/main.rs @@ -111,6 +111,24 @@ const SHAKA_FFPROBE_SHA256_X64: &str = #[cfg(all(unix, not(target_os = "macos")))] const DEFAULT_LINUX_FFMPEG_URL: &str = "https://johnvansickle.com/ffmpeg/releases/ffmpeg-release-amd64-static.tar.xz"; +// Pinned like the macOS hashes above, because this binary is downloaded, +// marked executable and run: without it the only thing standing between a +// compromised or MITM'd host and code execution was that the download +// completed (#518). +// +// Upstream publishes only a .md5 companion, which is both cryptographically +// broken for collisions and served by the same host as the tarball -- an +// attacker able to replace one can replace the other, so it evidences +// corruption, not authenticity. This hash was computed from the artifact whose +// MD5 matched upstream's published 7fa72b652e19bf84c9461e332ea1cdf3. +// +// The URL is a rolling one, so this needs a manual bump when upstream +// publishes a new build (the current one is dated 2024-08-24). A stale pin +// fails closed with a checksum error rather than silently accepting whatever +// arrives; STEMDECK_FFMPEG_URL still overrides both, for anyone who needs it. +#[cfg(all(unix, not(target_os = "macos")))] +const DEFAULT_LINUX_FFMPEG_SHA256: &str = + "abda8d77ce8309141f83ab8edf0596834087c52467f6badf376a6a2a4c87cf67"; struct BackendHandles { child: Child, @@ -3814,6 +3832,11 @@ fn download_linux_ffmpeg(data_dir: &Path) -> Result<(), String> { .map_err(|e| format!("failed to create {}: {e}", downloads.display()))?; let archive = downloads.join("ffmpeg-linux.tar.xz"); download_file(&url, &archive, Duration::from_secs(30 * 60), "FFmpeg")?; + // Only the pinned artifact is trusted. An override points somewhere we + // cannot have a hash for, so it is the caller's business to vouch for it. + if env_path_override("STEMDECK_FFMPEG_URL").is_none() { + verify_pinned_sha256(&archive, Some(DEFAULT_LINUX_FFMPEG_SHA256), "FFmpeg")?; + } // Extract with the system tar (xz support is standard on desktop Linux). The // static build unpacks to a single ffmpeg--amd64-static/ directory. From fb23927b7a447ad99bddf877f051fbfb2c66d418 Mon Sep 17 00:00:00 2001 From: "Tha.Les" <31625914+thcp@users.noreply.github.com> Date: Mon, 31 Aug 2026 22:18:19 +0100 Subject: [PATCH 12/19] fix(player): drive playback through the audio engine, not the silent multitrack (#536) engineMode() returns "chunked" unless the user has set the audioEngine flag to "0", and on that path audioEngine owns the clock while the multitrack is mounted with url: null for visuals only. transport.js handled this everywhere via `audioEngine ?? multitrack`; main.js imported only `multitrack` and called it bare. So on the default configuration: - The footer scrub bar did nothing. It is a full-size cursor: pointer overlay, and clicking or dragging anywhere on it moved neither the playhead nor the audio. - [ and ] seeked the silent multitrack, so nothing happened. - I and O read multitrack.getCurrentTime(), which is pinned at 0 there, so "set loop in at playhead" always wrote 0 no matter where the playhead was, and "set loop out" always wrote max(0, loopStart + 0.5). Space and ruler clicks were unaffected because they live in transport.js. Rather than patching five call sites, transport.js exports the accessor it was already using internally, plus setPlayheadTime. Seeking now goes through setPlayheadTime, which also updates the playhead marker, footer times and presence playhead -- none of which the old multitrack.setTime call did, so the scrub bar would have left the marker stale even had it worked. Same anti-drift shape as apply_ffmpeg_path in #506. The keydown guard excluded HTMLInputElement but not HTMLTextAreaElement, so Space in the settings log viewer started playback instead of scrolling. Fixed alongside, since the guard was being edited anyway. The test is structural rather than DOM-driven: the bug was not a wrong value from a function, it was reaching for the wrong object, so it asserts main.js holds no bare multitrack playback calls. All 8 checks fail against the previous code. Refs #515 --- static/js/main.js | 36 ++++++++++------ static/js/transport.js | 13 +++++- tests/js/transport-clock.test.mjs | 72 +++++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 15 deletions(-) create mode 100644 tests/js/transport-clock.test.mjs diff --git a/static/js/main.js b/static/js/main.js index 4009ce6c..80e6f331 100644 --- a/static/js/main.js +++ b/static/js/main.js @@ -1,5 +1,5 @@ import { - playBtn, loopBtn, multitrack, totalDuration, loopEnabled, loopStart, loopEnd, + playBtn, loopBtn, totalDuration, loopEnabled, loopStart, loopEnd, setLoopStart, setLoopEnd, selectedStems, saveSelectedStems, stemSelectionReady, currentJobId, vocalSplitMode, vocalSplitModeReady, setVocalSplitMode, setAutoSectionsResetFn, @@ -10,7 +10,7 @@ import { wireJobForm, showError } from "./job.js"; import { initSearch } from "./search.js"; import { wireTransportButtons } from "./transport.js"; import { wireBeatGridUi } from "./beatgridUi.js"; -import { togglePlayPause, updateLoopRegionVisual, toggleMetronome } from "./transport.js"; +import { togglePlayPause, updateLoopRegionVisual, toggleMetronome, transport, setPlayheadTime } from "./transport.js"; import { wireStemListControls, wireMixerToolbar } from "./mixer.js"; import { initCatalog, collectDiagnostics } from "./catalog.js"; import { initNotifications, notifyFailure, dismissFailuresByJobId } from "./notifications.js"; @@ -486,10 +486,14 @@ function wireFooterControls() { const scrub = document.getElementById("footer-scrub"); if (scrub) { function seekToX(clientX) { - if (!multitrack || !totalDuration) return; + // setPlayheadTime, not multitrack.setTime: on the default chunked engine + // the multitrack is silent and this whole bar did nothing. It also moves + // the playhead marker, footer times and presence playhead, which the old + // call never did (#515). + if (!totalDuration) return; const rect = scrub.getBoundingClientRect(); const frac = Math.max(0, Math.min(1, (clientX - rect.left) / rect.width)); - multitrack.setTime(frac * totalDuration); + setPlayheadTime(frac * totalDuration); } let _scrubbing = false; scrub.addEventListener("mousedown", (e) => { @@ -645,32 +649,36 @@ function wireAppShellControls() { // ─── Keyboard shortcuts ─── document.addEventListener("keydown", (e) => { - if (!multitrack) return; - if (e.target instanceof HTMLInputElement) return; + if (!transport()) return; + // Textareas were not excluded, so Space in the log viewer started playback + // instead of scrolling. + if (e.target instanceof HTMLInputElement || e.target instanceof HTMLTextAreaElement) return; if (e.code === "Space") { e.preventDefault(); togglePlayPause(); } else if (e.code === "BracketLeft") { e.preventDefault(); - multitrack.setTime(Math.max(0, multitrack.getCurrentTime() - 5)); + // setPlayheadTime clamps to [0, totalDuration] itself, so the Math.max / + // Math.min the multitrack version needed are gone with it. + setPlayheadTime(transport().getCurrentTime() - 5); } else if (e.code === "BracketRight") { e.preventDefault(); - multitrack.setTime( - Math.min(multitrack.getDuration(), multitrack.getCurrentTime() + 5), - ); + setPlayheadTime(transport().getCurrentTime() + 5); } else if (e.code === "KeyL") { e.preventDefault(); loopBtn.click(); } else if (e.code === "KeyK") { e.preventDefault(); toggleMetronome(); - } else if (e.code === "KeyI" && loopEnabled && multitrack) { + } else if (e.code === "KeyI" && loopEnabled) { e.preventDefault(); - setLoopStart(Math.min(multitrack.getCurrentTime(), loopEnd - 0.5)); + // multitrack.getCurrentTime() is pinned at 0 on the engine path, so "set + // loop in at playhead" always wrote 0 regardless of where the playhead was. + setLoopStart(Math.min(transport().getCurrentTime(), loopEnd - 0.5)); updateLoopRegionVisual(); - } else if (e.code === "KeyO" && loopEnabled && multitrack) { + } else if (e.code === "KeyO" && loopEnabled) { e.preventDefault(); - setLoopEnd(Math.max(multitrack.getCurrentTime(), loopStart + 0.5)); + setLoopEnd(Math.max(transport().getCurrentTime(), loopStart + 0.5)); updateLoopRegionVisual(); } }); diff --git a/static/js/transport.js b/static/js/transport.js index 7b6e92fc..826456e8 100644 --- a/static/js/transport.js +++ b/static/js/transport.js @@ -70,7 +70,18 @@ function timeFromClientX(clientX) { return frac * totalDuration; } -function setPlayheadTime(sec) { +/// The clock that actually owns playback. +/// +/// engineMode() defaults to "chunked", where audioEngine drives audio and the +/// multitrack is mounted with url: null for visuals only -- so operating on +/// `multitrack` directly moves nothing and reads 0. Everything in this module +/// already went through `audioEngine ?? multitrack`; exporting it stops other +/// modules re-deriving it and drifting (#515). +export function transport() { + return audioEngine ?? multitrack; +} + +export function setPlayheadTime(sec) { const tx = audioEngine ?? multitrack; if (!tx || !totalDuration) return; const next = Math.max(0, Math.min(totalDuration, sec)); diff --git a/tests/js/transport-clock.test.mjs b/tests/js/transport-clock.test.mjs new file mode 100644 index 00000000..61bf6f06 --- /dev/null +++ b/tests/js/transport-clock.test.mjs @@ -0,0 +1,72 @@ +// main.js drove `multitrack` directly while audioEngine owns the clock (#515). +// +// engineMode() defaults to "chunked", where the multitrack is mounted with +// url: null for visuals only. So the footer scrub bar did nothing at all, and +// "set loop in at playhead" always wrote 0 because multitrack.getCurrentTime() +// is pinned there. +// +// A structural check rather than a DOM one: the bug was not a wrong value from +// a function, it was reaching for the wrong object. Anything that reintroduces +// a bare multitrack playback call in main.js fails here. + +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; + +const root = join(dirname(fileURLToPath(import.meta.url)), '..', '..'); +const mainSrc = readFileSync(join(root, 'static/js/main.js'), 'utf8'); +const transportSrc = readFileSync(join(root, 'static/js/transport.js'), 'utf8'); + +let passed = 0; +let failed = 0; + +function check(name, condition, detail = '') { + if (condition) { + passed++; + console.log(`PASS ${name}`); + } else { + failed++; + console.log(`FAIL ${name}${detail ? ` -- ${detail}` : ''}`); + } +} + +// Strip comments so the explanatory ones below don't count as usage. +const code = mainSrc.replace(/\/\/.*$/gm, '').replace(/\/\*[\s\S]*?\*\//g, ''); + +for (const call of ['multitrack.setTime', 'multitrack.getCurrentTime', 'multitrack.getDuration']) { + check( + `main.js does not call ${call}`, + !code.includes(call), + 'the multitrack is silent on the default chunked engine', + ); +} + +check( + 'transport.js exports the accessor', + /export function transport\(\)/.test(transportSrc), +); + +check( + 'the accessor prefers the audio engine', + /return audioEngine \?\? multitrack;/.test(transportSrc), +); + +check( + 'transport.js exports setPlayheadTime', + /export function setPlayheadTime\(/.test(transportSrc), +); + +check( + 'main.js imports both rather than re-deriving them', + /import \{[^}]*\btransport\b[^}]*\} from "\.\/transport\.js"/.test(mainSrc) && + /import \{[^}]*\bsetPlayheadTime\b[^}]*\} from "\.\/transport\.js"/.test(mainSrc), +); + +check( + 'the keyboard guard excludes textareas', + code.includes('HTMLTextAreaElement'), + 'Space in the log viewer used to start playback instead of scrolling', +); + +console.log(`\n${passed}/${passed + failed} checks passed`); +process.exit(failed === 0 ? 0 : 1); From 2835c8f6fa2d7ea6f2cf7b177b0ec9889887dc1c Mon Sep 17 00:00:00 2001 From: "Tha.Les" <31625914+thcp@users.noreply.github.com> Date: Mon, 31 Aug 2026 22:18:36 +0100 Subject: [PATCH 13/19] ci: close four release-integrity gaps (#537) Deno was fetched from releases/latest/download with no version pin and no checksum, into every published GHCR image. It is the JS runtime yt-dlp feeds YouTube's challenge payload to, so it executes against untrusted input, and every build took whatever Deno published that day. The desktop packaging scripts already pin and verify QuickJS with the rationale written out in a comment -- Docker was the one path that did not follow it. Now pinned to v2.9.6 with a per-arch SHA256 verified before the zip is unpacked. linux-release.yml and windows-release.yml uploaded with action-gh-release, whose fail_on_unmatched_files defaults to false, and neither asserted the files existed first -- macOS is the only path that did. A missing updater asset therefore published a release that went green while the in-app updater 404'd for every installed user. Both now check every asset before uploading and set fail_on_unmatched_files. make-portable.ps1 set $PSNativeCommandErrorActionPreference, which exists only in PowerShell 7+, while CI invokes it with `powershell` -- Windows PowerShell 5.1, where it does nothing and $ErrorActionPreference does not cover native commands either. A failed CPU-torch --force-reinstall was ignored, leaving whatever torch was already resolved in place, and the zip labelled CPU shipped a non-CPU torch; the later import checks still passed because torch imports fine either way. Assert-LastExitCode now guards the venv creation and all three pip installs. The preference stays for a pwsh run. Switching the invocation to pwsh would have been the tidier fix, but nothing in this repo uses pwsh and the runner is self-hosted, so there is no way to confirm PowerShell 7 is installed there. An exit-code check works on both. trivy-action was pinned to @master, the only unpinned action in the repo while everything else is SHA-pinned. Now on v0.36.0's commit. Both Deno hashes were verified against the published artifacts, including a negative control confirming a wrong hash is rejected. Refs #517 --- .github/workflows/ci.yml | 4 ++-- .github/workflows/linux-release.yml | 19 +++++++++++++++++++ .github/workflows/windows-release.yml | 20 ++++++++++++++++++++ build/Dockerfile | 16 +++++++++++++--- scripts/windows/make-portable.ps1 | 19 +++++++++++++++++++ 5 files changed, 73 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c1496cde..08d74051 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -178,7 +178,7 @@ jobs: # would scan its bundled extractor files and flag false-positive # secrets that ship inside third-party packages like yt-dlp). - name: trivy fs - uses: aquasecurity/trivy-action@master + uses: aquasecurity/trivy-action@ed142fd0673e97e23eac54620cfb913e5ce36c25 # v0.36.0 with: scan-type: fs scan-ref: . @@ -190,7 +190,7 @@ jobs: skip-dirs: .venv,jobs # Dedicated Dockerfile + compose static analysis (Trivy's IaC linter). - name: trivy config - uses: aquasecurity/trivy-action@master + uses: aquasecurity/trivy-action@ed142fd0673e97e23eac54620cfb913e5ce36c25 # v0.36.0 with: scan-type: config scan-ref: build/ diff --git a/.github/workflows/linux-release.yml b/.github/workflows/linux-release.yml index 5aa06590..fade993b 100644 --- a/.github/workflows/linux-release.yml +++ b/.github/workflows/linux-release.yml @@ -139,6 +139,24 @@ jobs: clamscan --recursive --infected --bell /scan echo "ClamAV scan completed successfully. No infected files reported." + # macos-release.yml asserts its assets exist before uploading; this did + # not. action-gh-release defaults fail_on_unmatched_files to false, so a + # missing updater asset published a release that looked fine and went + # green, and the in-app updater then 404'd for every installed user. + - name: verify every asset exists + if: github.event_name == 'release' + run: | + for f in \ + dist/StemDeck-Linux-x64.tar.gz \ + dist/StemDeck-Linux-x64.tar.gz.sha256 \ + dist/StemDeck-Linux-x64.NVIDIA.tar.gz \ + dist/StemDeck-Linux-x64.NVIDIA.tar.gz.sha256 \ + dist/StemDeck-Linux-x64-app.tar.gz \ + dist/StemDeck-Linux-x64-app.tar.gz.sha256 \ + dist/StemDeck-Linux-x64-runtime-version.json; do + test -f "$f" || { echo "missing release asset: $f" >&2; exit 1; } + done + - name: upload artifacts # Only attach to a real release; a manual test build has nothing to upload to. if: github.event_name == 'release' @@ -150,6 +168,7 @@ jobs: # pushing :latest to GHCR, and makes the in-app updater offer a build # that was never verified. prerelease: ${{ github.event.release.prerelease }} + fail_on_unmatched_files: true files: | dist/StemDeck-Linux-x64.tar.gz dist/StemDeck-Linux-x64.tar.gz.sha256 diff --git a/.github/workflows/windows-release.yml b/.github/workflows/windows-release.yml index 70883061..6103a65b 100644 --- a/.github/workflows/windows-release.yml +++ b/.github/workflows/windows-release.yml @@ -92,6 +92,25 @@ jobs: } Write-Host "ClamAV scan completed successfully. No infected files reported." + # See the same guard in linux-release.yml: action-gh-release silently + # tolerates missing files, so an absent updater asset shipped a green + # release the in-app updater could not use. + - name: verify every asset exists + shell: powershell + run: | + $required = @( + "dist/StemDeck-Windows-x64.NVIDIA.zip", + "dist/StemDeck-Windows-x64.NVIDIA.zip.sha256", + "dist/StemDeck-Windows-x64.zip", + "dist/StemDeck-Windows-x64.zip.sha256", + "dist/StemDeck-Windows-x64-app.zip", + "dist/StemDeck-Windows-x64-app.zip.sha256", + "dist/StemDeck-Windows-x64-runtime-version.json" + ) + foreach ($f in $required) { + if (-not (Test-Path $f)) { Write-Error "missing release asset: $f"; exit 1 } + } + - name: upload artifacts uses: softprops/action-gh-release@3d0d9888cb7fd7b750713d6e236d1fcb99157228 # v3.0.2 with: @@ -101,6 +120,7 @@ jobs: # pushing :latest to GHCR, and makes the in-app updater offer a build # that was never verified. prerelease: ${{ github.event.release.prerelease }} + fail_on_unmatched_files: true files: | dist/StemDeck-Windows-x64.NVIDIA.zip dist/StemDeck-Windows-x64.NVIDIA.zip.sha256 diff --git a/build/Dockerfile b/build/Dockerfile index 5405f55c..cf0c38db 100644 --- a/build/Dockerfile +++ b/build/Dockerfile @@ -29,13 +29,23 @@ RUN apt-get update \ # deno -- yt-dlp uses it as the JS runtime for YouTube format # extraction. Staged here so the runner doesn't need curl/unzip. +# Pinned and checksummed, not "latest". Deno is the JS runtime yt-dlp feeds +# YouTube's challenge payload to, so it executes against untrusted input -- +# and an unverified binary fetched at build time is a supply-chain hole +# whether or not it is small. The desktop packaging scripts already pin and +# verify QuickJS for exactly this reason; this was the one path that did not. +# Bump DENO_VERSION and both hashes together. ARG TARGETARCH +ARG DENO_VERSION=v2.9.6 +ARG DENO_SHA256_AMD64=394f07f4da2bebe6ce6f1e7ce0fa16429b29b08c35e3fac3fe25972676dff4b2 +ARG DENO_SHA256_ARM64=9a46afc6c392c7cd2ff71a31558935545b46408d0e87f7a86908c712721c046e RUN case "${TARGETARCH:-$(dpkg --print-architecture)}" in \ - amd64) DENO_ARCH=x86_64-unknown-linux-gnu ;; \ - arm64) DENO_ARCH=aarch64-unknown-linux-gnu ;; \ + amd64) DENO_ARCH=x86_64-unknown-linux-gnu; DENO_SHA256="${DENO_SHA256_AMD64}" ;; \ + arm64) DENO_ARCH=aarch64-unknown-linux-gnu; DENO_SHA256="${DENO_SHA256_ARM64}" ;; \ *) echo "Unsupported arch: ${TARGETARCH}" && exit 1 ;; \ esac \ - && curl -fsSL -o /tmp/deno.zip "https://github.com/denoland/deno/releases/latest/download/deno-${DENO_ARCH}.zip" \ + && curl -fsSL -o /tmp/deno.zip "https://github.com/denoland/deno/releases/download/${DENO_VERSION}/deno-${DENO_ARCH}.zip" \ + && echo "${DENO_SHA256} /tmp/deno.zip" | sha256sum -c - \ && unzip /tmp/deno.zip -d /usr/local/bin \ && rm /tmp/deno.zip \ && /usr/local/bin/deno --version diff --git a/scripts/windows/make-portable.ps1 b/scripts/windows/make-portable.ps1 index 4288810c..759fd2cf 100644 --- a/scripts/windows/make-portable.ps1 +++ b/scripts/windows/make-portable.ps1 @@ -9,7 +9,19 @@ param( ) $ErrorActionPreference = "Stop" +# PowerShell 7+ only. CI invokes this script with `powershell` (Windows +# PowerShell 5.1), where this variable does nothing and $ErrorActionPreference +# does not cover native commands either -- so a failed pip install was ignored +# and the build carried on. Kept for a pwsh run; Assert-LastExitCode below is +# what actually enforces it on 5.1 (#517). $PSNativeCommandErrorActionPreference = "Stop" + +function Assert-LastExitCode { + param([Parameter(Mandatory)][string]$What) + if ($LASTEXITCODE -ne 0) { + throw "$What failed with exit code $LASTEXITCODE" + } +} Set-StrictMode -Version Latest if ($env:OS -ne "Windows_NT") { @@ -231,8 +243,10 @@ if (Get-Command "py" -ErrorAction SilentlyContinue) { } else { & python -m venv $PythonDir } +Assert-LastExitCode "creating the virtualenv" & $PythonExe -m pip install --upgrade pip +Assert-LastExitCode "pip self-upgrade" # The project version is git-derived (hatch-vcs). Pin it from $PackageVersion so # the install doesn't depend on git tags in the build checkout (#169). @@ -240,6 +254,7 @@ if ($PackageVersion) { $env:SETUPTOOLS_SCM_PRETEND_VERSION = ($PackageVersion -replace '^v', '') } & $PythonExe -m pip install "$Root" +Assert-LastExitCode "installing the StemDeck package" if ($CpuOnly) { # Force the slim CPU-only wheel. On Windows the default PyPI torch wheel is @@ -249,6 +264,10 @@ if ($CpuOnly) { & $PythonExe -m pip install torch==2.6.0+cpu torchaudio==2.6.0+cpu ` --index-url https://download.pytorch.org/whl/cpu ` --force-reinstall --no-deps + # Unchecked, a transient network failure here left whatever torch was already + # resolved in place and the zip labelled CPU shipped a non-CPU torch. The + # import checks later still pass, because torch imports fine either way. + Assert-LastExitCode "installing the CPU-only torch wheel" } # Do NOT bundle CUDA torch into the NVIDIA (non-CpuOnly) package. It ships base # torch and the desktop app installs the CUDA build on first run via From 31ef8ac1cdf657233c22ab0c749080c061278c1b Mon Sep 17 00:00:00 2001 From: "Tha.Les" <31625914+thcp@users.noreply.github.com> Date: Mon, 31 Aug 2026 22:33:36 +0100 Subject: [PATCH 14/19] feat(loop): adjust a loop region instead of redrawing it (#539) Reported in discussion #507: changing the size of a selection meant clicking and dragging again from scratch, which restarts the whole thing and loses the precision you had. The numeric fields are the only exact route, and finding a seamless loop through them is guesswork. The region could not be touched at all -- .loop-region was pointer-events: none, so every pointerdown reached the create-drag underneath, which resets both edges to the click position. Adjusting one edge necessarily destroyed the other. Three gestures now: - a handle at either edge moves that edge alone, so a loop can be tightened one side at a time; - the body moves both edges together, preserving length, so a loop found by ear can be slid without being re-measured; - a press that does not travel still seeks, which is what clicking inside the selection did before the region took pointer events. Losing that would have been a regression for anyone who just wants to move the playhead. The handles are 12px and overhang the 2px border on both sides, because an edge you cannot reliably grab is the finnicky behaviour this is meant to replace. They only render on hover or while dragging, so the selection looks the same at rest. create-drag already guarded with `e.target.closest(".loop-region")` -- dead code until now, since the element could not receive events. It is a real guard again, alongside the stopPropagation on the region's own handler. The geometry lives in its own module rather than in transport.js, which cannot be imported outside a browser: it pulls in state.js, which touches document at module load. Same reason playbackStems.js is separate. That makes the clamping testable, which is the half that matters -- an edge crossing its partner, or a region pushed against either end of the track. MIN_LOOP_SEC moves with it and now has one definition. Pointer events throughout, so this works with touch and pen. Closes #538. --- static/css/waves.css | 44 +++++++++++++- static/index.html | 2 +- static/js/loopRegion.js | 42 ++++++++++++++ static/js/transport.js | 87 +++++++++++++++++++++++++++- tests/js/loop-drag.test.mjs | 111 ++++++++++++++++++++++++++++++++++++ 5 files changed, 283 insertions(+), 3 deletions(-) create mode 100644 static/js/loopRegion.js create mode 100644 tests/js/loop-drag.test.mjs diff --git a/static/css/waves.css b/static/css/waves.css index ebdfcfc7..6f490621 100644 --- a/static/css/waves.css +++ b/static/css/waves.css @@ -765,10 +765,52 @@ box-shadow: inset 1px 0 0 rgba(255, 255, 255, 0.18), inset -1px 0 0 rgba(255, 255, 255, 0.18); - pointer-events: none; + /* Interactive so the region can be moved and its edges adjusted (#538). + A pointerdown that does not move still falls through to a seek, so + clicking inside the selection behaves as it always did. */ + pointer-events: auto; + cursor: grab; z-index: 3; } +.loop-region.dragging { + cursor: grabbing; +} + +/* Wider than the 2px border they sit on: an edge you cannot reliably grab is + the finnicky behaviour this replaces. Extends outside the region as well as + in, so the handle is catchable from either side. */ +.loop-handle { + position: absolute; + top: 0; + bottom: 0; + width: 12px; + cursor: ew-resize; + z-index: 4; +} + +.loop-handle-start { + left: -7px; +} + +.loop-handle-end { + right: -7px; +} + +/* Only visible while the pointer is on the region, so the selection reads the + same as before at rest. */ +.loop-region:hover .loop-handle::after, +.loop-region.dragging .loop-handle::after { + content: ""; + position: absolute; + top: 0; + bottom: 0; + left: 4px; + width: 4px; + background: var(--gold); + border-radius: 2px; +} + .lane-placeholder { height: 48px; position: relative; diff --git a/static/index.html b/static/index.html index 7d9f58c8..c79d4064 100644 --- a/static/index.html +++ b/static/index.html @@ -617,7 +617,7 @@
- +