From 4dded527acb0039755157276b7e24b2a62046460 Mon Sep 17 00:00:00 2001 From: Thales <> Date: Tue, 3 Nov 2026 18:04:22 +0000 Subject: [PATCH] fix(desktop): a portable folder never reads an installed app's settings or library A fresh portable folder copied %LOCALAPPDATA%\StemDeck\settings.json from an installed StemDeck in ensure_workspace. That file can name the installed app's library, so the portable run opened it, and its factory reset or auto-delete sweep would delete those songs. A portable package also adopted ~\Documents\StemDeck\jobs as its default library whenever it had entries, and copied Documents\StemDeck\user-data.json into its own jobs folder. A portable package now starts from its own data folder only, with its own defaults, or the library the user picks in Settings. It never resolves Documents. Installed builds keep ~/Documents/StemDeck/jobs as before. The settings mirror (STEMDECK_SETTINGS_MIRROR) only existed to feed the portable seed. A portable package already had none, and for an installed build it was the same file as settings.json, so its write and its corrupt file recovery did nothing. It is removed from the shell and the backend. A corrupt settings.json is still moved aside for diagnosis. --- README.md | 2 +- app/core/settings.py | 70 +------- desktop/src-tauri/src/main.rs | 238 ++++++++++++-------------- packaging/windows/README-WINDOWS.txt | 8 +- tests/acceptance/README.md | 22 +-- tests/acceptance/acceptance.spec.mjs | 25 +-- tests/acceptance/harness.mjs | 21 +-- tests/acceptance/upgrade-fixture.mjs | 17 +- tests/fixtures/upgrade-0.20.0/make.py | 4 +- tests/test_max_duration_setting.py | 42 +++++ tests/test_settings_durability.py | 38 +--- tests/test_settings_mirror.py | 150 ---------------- tests/test_upgrade_defaults.py | 1 - 13 files changed, 206 insertions(+), 432 deletions(-) create mode 100644 tests/test_max_duration_setting.py delete mode 100644 tests/test_settings_mirror.py diff --git a/README.md b/README.md index 54e1ad33..975349db 100644 --- a/README.md +++ b/README.md @@ -104,7 +104,7 @@ The first launch downloads the runtime and the model. Apple Silicon separates on **Docker and Unraid.** See [Build from source](docs/build-from-source.md#docker) for the `docker run` line, GPU passthrough and the Unraid volume mapping. -**Updates.** The desktop app tells you when a new version is out. On Windows and Linux it usually installs the update for you. A version that changes the Python packages StemDeck runs on is a full download instead, and 0.21.0 is one: about 300 MB on Windows and 420 MB on Linux, once. On Windows, unzip it over your current StemDeck folder. The zip has no files inside `data`, so your songs, models and settings there stay as they are. Unzipped into a new folder, StemDeck starts without the songs from the old `data` folder and downloads FFmpeg and the model again. On macOS, download the new DMG. +**Updates.** The desktop app tells you when a new version is out. On Windows and Linux it usually installs the update for you. A version that changes the Python packages StemDeck runs on is a full download instead, and 0.21.0 is one: about 300 MB on Windows and 420 MB on Linux, once. On Windows, unzip it over your current StemDeck folder. The zip has no files inside `data`, so your songs, models and settings there stay as they are. A portable StemDeck no longer opens a library in `Documents\StemDeck\jobs` on its own: if yours is there, choose that folder once in Settings, StemData location. Unzipped into a new folder, StemDeck starts with default settings, without the songs from the old `data` folder, and downloads FFmpeg and the model again. On macOS, download the new DMG. diff --git a/app/core/settings.py b/app/core/settings.py index 0a9d29b6..b88ae4f9 100644 --- a/app/core/settings.py +++ b/app/core/settings.py @@ -83,15 +83,6 @@ def _default_allow_network() -> bool: return os.environ.get("STEMDECK_DESKTOP") != "1" -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. @@ -133,7 +124,7 @@ def _atomic_write_json(path: Path, data: dict) -> bool: 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 + persisted a one-key file over it (#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. @@ -189,8 +180,7 @@ def _load() -> dict: 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.""" + the next set_*() then persisted a single key over it.""" if not _SETTINGS_PATH.exists(): return {} # no settings file yet — genuine first run; use defaults @@ -198,19 +188,8 @@ def _load() -> dict: 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. + # The file is there but unusable. Preserve it for diagnosis. _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 {} @@ -218,15 +197,6 @@ def _ensure() -> dict: global _state if _state is None: _state = _load() - # Seed the per-user copy from settings that already exist. Mirroring - # only on _save() would protect nobody who configured StemDeck before - # this shipped and never opens Settings again -- their next install - # would still start empty. Safe against recursion (_state is assigned - # first) and against clobbering: an empty dict means a genuine first - # run, and overwriting a good copy with it is exactly the data loss - # this whole mechanism exists to prevent. - if _state: - _mirror_settings() return _state @@ -237,39 +207,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).""" - if not _atomic_write_json(_SETTINGS_PATH, _ensure()): - return False - _mirror_settings() - return True - - -def _mirror_settings() -> None: - """Keep a per-user copy of settings.json outside the install directory. - - A Windows portable package keeps its data in `/data` (#399), so - settings.json lives *inside the install*. Upgrading by extracting the new - zip to a fresh folder therefore started that install with no settings at - all: the stems location, port, compute device, quality and language were - all silently back to defaults, and a relocated library looked empty. - - The desktop shell restores from this copy -- `ensure_workspace` seeds a - fresh portable install from it before the backend ever reads - settings.json. A portable package itself writes nothing outside its own - folder, so the shell gives it no mirror; the copy it reads is the one an - installed StemDeck keeps. - - Best-effort by definition: this is a redundant copy, and failing to write - it must never fail the setting the user just changed. The path comes from - the shell (STEMDECK_SETTINGS_MIRROR) so the platform logic stays in one - place and both halves cannot drift apart. - """ - path = _mirror_path() - if path is None: - return - # 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()) + return _atomic_write_json(_SETTINGS_PATH, _ensure()) def _num(v: object) -> int | None: diff --git a/desktop/src-tauri/src/main.rs b/desktop/src-tauri/src/main.rs index 14b9b413..8e4c11e4 100644 --- a/desktop/src-tauri/src/main.rs +++ b/desktop/src-tauri/src/main.rs @@ -672,9 +672,32 @@ fn main() { /// ~/Documents/StemDeck/jobs, since the backend's own ensure_runtime_dirs /// (app/core/config.py) already mkdirs whichever JOBS_DIR actually wins that /// precedence -- this path only needs to exist when it is the one in use. -fn documents_stemdeck_dir(app: &tauri::AppHandle) -> Result { - let documents = app.path().document_dir().map_err(|e| e.to_string())?; - Ok(documents.join("StemDeck")) +/// +/// None for a portable package (see installed_documents_dir), or when the OS +/// has no Documents folder. +fn documents_stemdeck_dir(app: &tauri::AppHandle) -> Option { + let documents = || app.path().document_dir().ok().map(|d| d.join("StemDeck")); + match app_root() { + Ok(root) => installed_documents_dir(&root, documents), + Err(_) => documents(), + } +} + +/// `documents` for an installed app, None for a portable package. +/// +/// ~/Documents/StemDeck is where an installed StemDeck keeps its library. A +/// portable package keeps its own in data/jobs, or in the folder the user picks +/// in Settings, so it never looks there: a portable run that adopted that +/// folder opened another install's songs, and its reset or sweep deleted them. +fn installed_documents_dir( + root: &Path, + documents: impl FnOnce() -> Option, +) -> Option { + if is_portable_package(root) { + None + } else { + documents() + } } /// The stems/jobs folder as it exists right now: the backend's own @@ -720,7 +743,7 @@ fn documents_store_path(app: &tauri::AppHandle) -> Result { .map_err(|e| format!("failed to create {}: {e}", jobs_dir.display()))?; let new_path = jobs_dir.join("user-data.json"); if !new_path.is_file() { - if let Ok(old_path) = documents_stemdeck_dir(app).map(|d| d.join("user-data.json")) { + if let Some(old_path) = documents_stemdeck_dir(app).map(|d| d.join("user-data.json")) { if old_path.is_file() && old_path != new_path { let _ = fs::copy(&old_path, &new_path); } @@ -729,14 +752,6 @@ fn documents_store_path(app: &tauri::AppHandle) -> Result { Ok(new_path) } -/// True if `path` exists and contains at least one entry. Used to tell an -/// already-in-use default folder apart from one nothing has ever written to. -fn directory_has_entries(path: &Path) -> bool { - fs::read_dir(path) - .map(|mut entries| entries.next().is_some()) - .unwrap_or(false) -} - /// The DEFAULT stems folder. Does NOT create it -- see documents_stemdeck_dir /// for why. /// @@ -746,45 +761,18 @@ fn directory_has_entries(path: &Path) -> bool { /// is the one that has to move the library when it changes, including /// creating whichever path wins (app/core/config.py's ensure_runtime_dirs). /// -/// Two candidates, resolved in this order: -/// -/// 1. ~/Documents/StemDeck/jobs, if it already has anything in it. Every -/// install before this default existed used this path, so an existing -/// user's real library lives there without any explicit `jobs_dir` in -/// settings.json to record it -- it was simply "the default." Checking -/// disk content directly (rather than writing a one-time migration flag -/// into settings.json, which only the backend otherwise writes) keeps this -/// self-contained: nothing to persist, no other-process race, and it stays -/// correct on every future launch for as long as that folder holds data. -/// 2. Otherwise, for the Windows portable package, local_data_dir()/jobs -- -/// i.e. next to data/cache and data/models inside the package itself, -/// rather than leaving a footprint in Documents. Non-portable installs -/// (installer builds, macOS, Linux) keep candidate 1 either way: the -/// original Documents rationale (visible in Finder/Explorer, eligible for -/// OneDrive/iCloud backup, survives reinstalls) still applies to them. +/// ~/Documents/StemDeck/jobs for installer builds, macOS and Linux: visible in +/// Finder/Explorer, eligible for OneDrive/iCloud backup, and it survives +/// reinstalls. local_data_dir()/jobs for the Windows portable package, next to +/// data/cache and data/models inside the package itself, and wherever the OS +/// has no Documents folder. fn documents_dir_for_jobs(app: &tauri::AppHandle) -> PathBuf { - let legacy_default = match documents_stemdeck_dir(app) { - Ok(dir) => dir.join("jobs"), - Err(_) => { - return local_data_dir() - .map(|d| d.join("jobs")) - .unwrap_or_else(|_| PathBuf::from("jobs")); - } - }; - - if directory_has_entries(&legacy_default) { - return legacy_default; - } - - if let Ok(root) = app_root() { - if is_portable_package(&root) { - if let Ok(data_dir) = local_data_dir() { - return data_dir.join("jobs"); - } - } + match documents_stemdeck_dir(app) { + Some(dir) => dir.join("jobs"), + None => local_data_dir() + .map(|d| d.join("jobs")) + .unwrap_or_else(|_| PathBuf::from("jobs")), } - - legacy_default } /// Native folder picker for the stems location. Returns None when the user @@ -1762,58 +1750,11 @@ fn apply_app_update_blocking( } } -/// The per-user directory holding the settings copy that survives reinstalling. -/// -/// A portable package keeps its data in `/data` (#399), which means -/// settings.json lives *inside the install*. `ensure_workspace` seeds a freshly -/// extracted package from the copy here, which it only ever reads: a portable -/// package writes nothing outside its own folder, so `settings_mirror` gives -/// it no copy to keep. -/// -/// Deliberately the OS-standard data dir, i.e. exactly what `local_data_dir` -/// returns for a NON-portable install, so the two layouts share one location -/// and an install that switches between them keeps its settings either way. -fn shared_settings_dir() -> Option { - #[cfg(windows)] - { - env::var("LOCALAPPDATA") - .ok() - .map(|base| PathBuf::from(base).join("StemDeck")) - } - #[cfg(target_os = "macos")] - { - env::var("HOME").ok().map(|home| { - PathBuf::from(home) - .join("Library") - .join("Application Support") - .join("StemDeck") - }) - } - #[cfg(all(unix, not(target_os = "macos")))] - { - if let Ok(xdg) = env::var("XDG_DATA_HOME") { - return Some(PathBuf::from(xdg).join("stemdeck")); - } - env::var("HOME").ok().map(|home| { - PathBuf::from(home) - .join(".local") - .join("share") - .join("stemdeck") - }) - } -} - -/// Where the backend keeps its per-user copy of settings.json -/// (STEMDECK_SETTINGS_MIRROR), or None for a portable package, which writes -/// nothing outside its own folder. -fn settings_mirror(root: &Path) -> Option { - if is_portable_package(root) { - return None; - } - shared_settings_dir().map(|dir| dir.join("settings.json")) -} - /// Creates required data directories and runs any pending data migrations. +/// +/// A portable package starts from its own data folder only. It never takes the +/// settings an installed StemDeck keeps in the OS data dir: they name that +/// install's library, and a portable run must not open, reset or sweep it. #[tauri::command] fn ensure_workspace() -> Result<(), String> { log_setup_step("ensure-workspace"); @@ -1837,15 +1778,6 @@ fn ensure_workspace() -> Result<(), String> { } } - #[cfg(windows)] - if is_portable_package(&root) { - // Portable data moved from %LocalAppData% to /data in #399. A - // freshly extracted package has an empty data directory, so carry the - // user's prior choices forward before the backend reads settings.json. - if let Some(shared) = shared_settings_dir() { - migrate_persisted_files(&shared, &data, &["settings.json"])?; - } - } migrate_legacy_data(&root, &data)?; fs::create_dir_all(&data).map_err(|e| format!("failed to create data dir: {e}"))?; for dir in ["cache", "downloads", "ffmpeg", "jobs", "logs", "models"] { @@ -2373,8 +2305,7 @@ fn spawn_backend( cmd.env("PYTHONHOME", pythonhome); } - // Jobs (stem audio files) live in ~/Documents/StemDeck/jobs/ so the user's - // library is visible in Finder, backed up by iCloud, and survives app reinstalls. + // The default jobs (stem audio) folder: see documents_dir_for_jobs. let jobs_dir = documents_dir_for_jobs(app_handle); cmd.current_dir(&backend_dir) @@ -2384,7 +2315,6 @@ fn spawn_backend( // Reported back by /api/settings as network_listening, so the // switch shows what is listening rather than what was saved. .env("STEMDECK_BIND_HOST", bind_host) - .envs(settings_mirror(&root).map(|path| ("STEMDECK_SETTINGS_MIRROR", path))) .env("STEMDECK_PARENT_PID", std::process::id().to_string()) // How the backend proves it is ours when it answers /api/health. // The environment is the only channel that survives the Windows @@ -11400,37 +11330,79 @@ configuration: --cc=clang\n"; } #[test] - fn only_an_installed_app_mirrors_its_settings_to_the_profile() { - // B25b: the backend of a portable package kept a copy of settings.json - // in %LOCALAPPDATA%\StemDeck. - let root = make_tmp(); - assert_eq!( - super::settings_mirror(root.path()), - super::shared_settings_dir().map(|dir| dir.join("settings.json")) + fn a_new_portable_folder_never_takes_an_installed_apps_settings() { + // The settings an installed StemDeck keeps in %LOCALAPPDATA%\StemDeck + // name that install's library. A portable run that started from them + // opened that library, and a reset or a sweep there deleted its songs. + // ensure_workspace reads LOCALAPPDATA and STEMDECK_ROOT, so it runs in + // a child test process with its own environment. Only Windows ships a + // portable package, so only there can this catch a regression. + let tmp = make_tmp(); + let root = tmp.path().join("StemDeck"); + let data = root.join("data"); + let installed = tmp.path().join("LocalAppData"); + fs::create_dir_all(installed.join("StemDeck")).unwrap(); + fs::write( + installed.join("StemDeck").join("settings.json"), + br#"{"jobs_dir":"D:\\StemDeck\\jobs"}"#, + ) + .unwrap(); + fs::create_dir_all(&root).unwrap(); + fs::write(root.join("portable.txt"), "").unwrap(); + + let status = Command::new(env::current_exe().unwrap()) + .args(["--exact", "tests::ensure_workspace_in_child", "--ignored"]) + .env("STEMDECK_WORKSPACE_CHILD", "1") + .env("STEMDECK_ROOT", &root) + .env("STEMDECK_DATA_DIR", &data) + .env("LOCALAPPDATA", &installed) + .stdout(Stdio::null()) + .status() + .unwrap(); + + assert!(status.success(), "ensure_workspace failed in the child"); + assert!(data.join("config.json").is_file(), "the workspace was made"); + assert!( + !data.join("settings.json").exists(), + "a portable folder copied the installed app's settings" ); - fs::write(root.path().join("portable.txt"), "").unwrap(); - assert_eq!(super::settings_mirror(root.path()), None); } #[test] - fn directory_has_entries_true_for_a_populated_dir() { - let dir = make_tmp(); - fs::write(dir.path().join("registry.json"), "{}").unwrap(); - assert!(super::directory_has_entries(dir.path())); + fn a_portable_package_never_looks_in_documents() { + let root = make_tmp(); + fs::write(root.path().join("portable.txt"), "").unwrap(); + let found = super::installed_documents_dir(root.path(), || { + panic!("a portable package looked in Documents") + }); + assert_eq!(found, None); } #[test] - fn directory_has_entries_false_for_an_empty_dir() { - let dir = make_tmp(); - assert!(!super::directory_has_entries(dir.path())); + fn an_installed_app_keeps_its_library_in_documents() { + let root = make_tmp(); + let documents = PathBuf::from("Documents").join("StemDeck"); + assert_eq!( + super::installed_documents_dir(root.path(), || Some(documents.clone())), + Some(documents) + ); } #[test] - fn directory_has_entries_false_for_a_missing_dir() { - let dir = make_tmp(); - assert!(!super::directory_has_entries( - &dir.path().join("does-not-exist") - )); + #[ignore = "run by a_new_portable_folder_never_takes_an_installed_apps_settings"] + fn ensure_workspace_in_child() { + // Anything but the environment the parent test sets up is someone's + // `cargo test -- --ignored` in their real environment, where + // ensure_workspace would migrate into their real data folder. + let Some(root) = env::var_os("STEMDECK_ROOT").map(PathBuf::from) else { + return; + }; + let isolated = env::var_os("STEMDECK_WORKSPACE_CHILD").is_some() + && super::is_portable_package(&root) + && env::var_os("STEMDECK_DATA_DIR").map(PathBuf::from) == Some(root.join("data")); + if isolated { + super::ensure_workspace().unwrap(); + } } #[test] diff --git a/packaging/windows/README-WINDOWS.txt b/packaging/windows/README-WINDOWS.txt index 04a7c234..37611976 100644 --- a/packaging/windows/README-WINDOWS.txt +++ b/packaging/windows/README-WINDOWS.txt @@ -38,8 +38,12 @@ Notes (Prefetch), folders you picked in its file dialogs, and the firewall rule if you let it reach your network. The WebView2 runtime that draws the window is part of Windows, not of this folder. -- A new portable folder starts with the settings of an installed StemDeck on - this PC, if there is one. It only reads them. +- A new portable folder starts with its own default settings and keeps its + library in data/jobs/. It never reads the settings or the library of an + installed StemDeck on this PC. To use a library you already have, choose its + folder in Settings, StemData location. That includes a library an older + version opened in Documents/StemDeck/jobs without you choosing it: choose + that folder once, before you import a new song. - To install a new version by hand, extract it over this folder. The zip has no files inside data/, so your library, models and settings stay. Extracted into a new folder, StemDeck starts without the songs in this folder's data/jobs/. diff --git a/tests/acceptance/README.md b/tests/acceptance/README.md index fde57e0a..ba3fdae5 100644 --- a/tests/acceptance/README.md +++ b/tests/acceptance/README.md @@ -57,11 +57,11 @@ in `acceptance.spec.mjs`, which runs them in the order they can, not by id. UPG-01 and UPG-02 start the build a third time, last, on a 0.20.0 library: `tests/fixtures/upgrade-0.20.0`, written by the 0.20.0 code (`make.py` there), copied to `data\acceptance-upg` with WAV stems added. The build gets -the package's data folder without its `settings.json`, and a `LOCALAPPDATA` -holding the old settings copy that names the old library, which is how a new -version unzipped into a new folder finds them. UPG-01 is skipped when +the package's data folder with the old `settings.json` that names the old +library, which is how a new version unzipped over the old folder, or installed +by the in-app update, finds them. UPG-01 is skipped when `Documents\StemDeck\jobs` exists, since a lost StemData location would open -that folder. +that folder in a build before 0.21.0. The line by line timing checks: @@ -102,13 +102,13 @@ report replaces it with `[key]` should it ever appear. downloaded runtime archives, so later runs are fast. - Starts `StemDeck.exe` with `LOCALAPPDATA` pointed at `%TEMP%\stemdeck-acceptance\LocalAppData` (emptied every run), which also - holds the WebView2 profile. A portable build is not isolated by default: it - restores settings from `%LOCALAPPDATA%\StemDeck` and mirrors changes back - there. With the redirect, the real `%LOCALAPPDATA%\StemDeck` and the real - library are never read or written; the report says whether the real - settings file stayed byte for byte the same. -- Refuses to run if `Documents\StemDeck\user-data.json` exists, since the - build would copy that library layout into the test run. + holds the WebView2 profile. A portable build before 0.21.0 was not isolated + by default: it restored settings from `%LOCALAPPDATA%\StemDeck` and mirrored + changes back there. With the redirect, the real `%LOCALAPPDATA%\StemDeck` and + the real library are never read or written, whichever build is under test; + the report says whether the real settings file stayed byte for byte the same. +- Refuses to run if `Documents\StemDeck\user-data.json` exists, since a build + before 0.21.0 would copy that library layout into the test run. - Closes the app it started, and its backend, at the end, also after a failure. It never looks at any other StemDeck process. diff --git a/tests/acceptance/acceptance.spec.mjs b/tests/acceptance/acceptance.spec.mjs index bafd11d1..07f2c8cb 100644 --- a/tests/acceptance/acceptance.spec.mjs +++ b/tests/acceptance/acceptance.spec.mjs @@ -1717,23 +1717,24 @@ test(title("E6"), async ({ app }, testInfo) => { // // The library is tests/fixtures/upgrade-0.20.0, written by the 0.20.0 code // itself (make.py there), copied to data\acceptance-upg (git-ignored) with real -// WAV stems added. An upgrader unzips the new version into a new folder: its -// data folder has no settings.json, and the shell brings the old settings in -// from %LOCALAPPDATA%\StemDeck\settings.json, which names the old library. So -// the build starts with STEMDECK_DATA_DIR set to the package's own data folder -// (the throwaway the harness resets, holding the runtime that is slow to -// download) minus its settings.json, and LOCALAPPDATA set to the fixture's. -// The user's real %LOCALAPPDATA% and library are never named. +// WAV stems added. An upgrader unzips the new version over the old folder, or +// the app updates itself: the data folder keeps the old settings.json, which +// names the old library. (Unzipped into a new folder, a portable package starts +// with its own defaults: it never reads settings from outside itself.) So the +// build starts with STEMDECK_DATA_DIR set to the package's own data folder (the +// throwaway the harness resets, holding the runtime that is slow to download) +// holding the 0.20.0 settings.json. The user's real %LOCALAPPDATA% and library +// are never named. const UPG_ROOT = path.join(REPO_ROOT, "data", "acceptance-upg"); // Steps: with 0.20.0's library and settings in place, start the new version -// from a new folder. Expect: the same songs, folders and settings show; +// over the old folder. Expect: the same songs, folders and settings show; // Settings > Library > StemData location is the old folder. test(title("UPG-01"), async ({ app }, testInfo) => { test.skip(!desktop(), "Server target: an upgrade starts the desktop build on an old library."); - // If the old StemData location were lost, the shell's default would be - // Documents\StemDeck\jobs when that folder has anything in it, and that may + // If the old StemData location were lost, a build before 0.21.0 defaulted to + // Documents\StemDeck\jobs when that folder had anything in it, and that may // be a real library. Refuse rather than find out by opening it. const docsJobs = path.join(documentsDir(), "StemDeck", "jobs"); test.skip(fs.existsSync(docsJobs), `${docsJobs} exists: a failed upgrade would open it. Move it aside for the run.`); @@ -1741,9 +1742,9 @@ test(title("UPG-01"), async ({ app }, testInfo) => { await app.browser.close().catch(() => {}); await stopRecordedApp(); const upg = prepareUpgradeLibrary(UPG_ROOT); - fs.rmSync(path.join(DATA_DIR, "settings.json"), { force: true }); + fs.writeFileSync(path.join(DATA_DIR, "settings.json"), JSON.stringify(upg.settings)); writeState({ ...readState(), upgrade: { ready: false } }); - await launchApp({ label: "upgrade", localAppData: upg.localAppData, env: { STEMDECK_DATA_DIR: DATA_DIR } }); + await launchApp({ label: "upgrade", env: { STEMDECK_DATA_DIR: DATA_DIR } }); const { browser, page } = await connect(chromium, { timeout: 5 * 60_000 }); watchNetwork(page); try { diff --git a/tests/acceptance/harness.mjs b/tests/acceptance/harness.mjs index 830a3d5f..be1b7787 100644 --- a/tests/acceptance/harness.mjs +++ b/tests/acceptance/harness.mjs @@ -9,14 +9,14 @@ // server an already running backend (a dev server), driven in // Playwright's own Chromium. Nothing is reset or stopped. // -// Isolation of the desktop build is the point of most of this file. A portable -// package is not isolated by default: ensure_workspace restores settings.json -// (and with it jobs_dir) from %LOCALAPPDATA%\StemDeck, and the backend mirrors -// every setting change back there (STEMDECK_SETTINGS_MIRROR). So the build is -// started with LOCALAPPDATA pointed at a throwaway folder, its own data folder -// is given a settings.json naming its own jobs folder, and the WebView2 -// profile is put in the throwaway folder too. The user's real -// %LOCALAPPDATA%\StemDeck and library are never read or written. +// Isolation of the desktop build is the point of most of this file. Builds +// before 0.21.0 were not isolated by default: ensure_workspace restored +// settings.json (and with it jobs_dir) from %LOCALAPPDATA%\StemDeck, and the +// backend mirrored every setting change back there. So the build is started +// with LOCALAPPDATA pointed at a throwaway folder, its own data folder is +// given a settings.json naming its own jobs folder, and the WebView2 profile +// is put in the throwaway folder too. The user's real %LOCALAPPDATA%\StemDeck +// and library are never read or written, whichever build is under test. import { spawn, spawnSync } from "node:child_process"; import fs from "node:fs"; @@ -90,8 +90,9 @@ export function resetPackageState() { // Documents\StemDeck\jobs: possibly a real library. Refuse. throw new Error(`${PACKAGE_DIR} is not a portable package (no portable.txt).`); } - // The desktop store is copied from Documents\StemDeck\user-data.json into a - // new jobs folder once (documents_store_path). That file would be the user's + // A portable build before 0.21.0 copied the desktop store from + // Documents\StemDeck\user-data.json into a new jobs folder once + // (documents_store_path). That file would be the user's // real library layout, so refuse rather than import it into the test run. const docs = documentsDir(); if (docs && fs.existsSync(path.join(docs, "StemDeck", "user-data.json"))) { diff --git a/tests/acceptance/upgrade-fixture.mjs b/tests/acceptance/upgrade-fixture.mjs index 6aaaa385..599057e3 100644 --- a/tests/acceptance/upgrade-fixture.mjs +++ b/tests/acceptance/upgrade-fixture.mjs @@ -2,8 +2,8 @@ // // tests/fixtures/upgrade-0.20.0 holds what the 0.20.0 code wrote (make.py runs // the tagged code): registry.json, the song's metadata.json, the desktop -// library store user-data.json, and the settings.json the desktop shell keeps -// in %LOCALAPPDATA%\StemDeck. This copies it into a throwaway folder under the +// library store user-data.json, and the settings.json the package keeps in its +// data folder. This copies it into a throwaway folder under the // repository's git-ignored data\ and adds what the repository does not carry: // the stems, as real PCM16 WAVs, and their peaks.json, both where 0.20.0's // pipeline put them. @@ -11,9 +11,9 @@ // Layout of the copy, under `root`: // // library\ the old StemData location (jobs folder) -// LocalAppData\StemDeck\settings.json the 0.20.0 settings, naming library\ // -// Nothing here reads or writes outside `root`. +// The 0.20.0 settings, naming library\, are returned for the test to put in +// the package's data folder. Nothing here reads or writes outside `root`. import fs from "node:fs"; import path from "node:path"; @@ -67,8 +67,8 @@ function peaks(points = 400) { } /** - * Copy the fixture into `root` (emptied first). Returns the paths the test - * needs: the library folder and the LOCALAPPDATA to start the build with. + * Copy the fixture into `root` (emptied first). Returns the library folder and + * the 0.20.0 settings that name it. */ export function prepareUpgradeLibrary(root) { const resolved = path.resolve(root); @@ -78,7 +78,6 @@ export function prepareUpgradeLibrary(root) { } fs.rmSync(resolved, { recursive: true, force: true, maxRetries: 5, retryDelay: 500 }); const library = path.join(resolved, "library"); - const localAppData = path.join(resolved, "LocalAppData"); fs.cpSync(path.join(FIXTURE_DIR, "library"), library, { recursive: true }); const stems = path.join(library, UPG_JOB_ID, "stems"); @@ -89,7 +88,5 @@ export function prepareUpgradeLibrary(root) { fs.writeFileSync(path.join(library, UPG_JOB_ID, "source.wav"), wav(220)); const settings = { ...oldSettings(), jobs_dir: library }; - fs.mkdirSync(path.join(localAppData, "StemDeck"), { recursive: true }); - fs.writeFileSync(path.join(localAppData, "StemDeck", "settings.json"), JSON.stringify(settings)); - return { root: resolved, library, localAppData, settings }; + return { root: resolved, library, settings }; } diff --git a/tests/fixtures/upgrade-0.20.0/make.py b/tests/fixtures/upgrade-0.20.0/make.py index 85e54f80..f9bacb10 100644 --- a/tests/fixtures/upgrade-0.20.0/make.py +++ b/tests/fixtures/upgrade-0.20.0/make.py @@ -7,8 +7,8 @@ - registry.json comes from 0.20.0's registry.persist, so every field of the 0.20.0 Job record is there with its 0.20.0 default; - metadata.json from 0.20.0's runner._write_metadata; -- settings.json from 0.20.0's settings setters (the mirror the desktop shell - restores into a fresh package); +- settings.json from 0.20.0's settings setters (what the package keeps in its + data folder); - user-data.json is the desktop library store as 0.20.0's catalog.js wrote it (STORAGE_KEY "stemdeck.folders", version 2), through tauri-plugin-store, which keeps one JSON object keyed by store key. diff --git a/tests/test_max_duration_setting.py b/tests/test_max_duration_setting.py new file mode 100644 index 00000000..e0663f58 --- /dev/null +++ b/tests/test_max_duration_setting.py @@ -0,0 +1,42 @@ +"""The longest track accepted for processing (max_duration_sec).""" + +from __future__ import annotations + +from app.core import settings as _settings + + +def test_the_duration_ceiling_is_published_not_duplicated(): + """The UI used to keep its own copy of the max-track-length ceiling. It + went stale at 20 minutes against a real ceiling of 60, so typing 60 was + silently clamped to 20 and the field snapped back with no explanation. + + The cure was to stop having a second copy, so the bound has to stay in the + payload for the client to read. + """ + from fastapi.testclient import TestClient + + from app.core.settings import DURATION_MAX_SEC, DURATION_MIN_SEC + from app.main import app + + with TestClient(app) as c: + body = c.get("/api/settings").json() + assert body["max_duration_max_sec"] == DURATION_MAX_SEC + assert body["max_duration_min_sec"] == DURATION_MIN_SEC + + +def test_a_value_at_the_ceiling_is_kept(tmp_path, monkeypatch): + """60 minutes is a legal setting. It was reachable through the API all + along; only the client refused to send it.""" + path = tmp_path / "settings.json" + monkeypatch.setattr(_settings, "_SETTINGS_PATH", path) + monkeypatch.setattr(_settings, "_state", None) + assert _settings.set_max_duration_sec(3600) == 3600 + assert _settings.get_max_duration_sec() == 3600 + + +def test_beyond_the_ceiling_still_clamps(tmp_path, monkeypatch): + path = tmp_path / "settings.json" + monkeypatch.setattr(_settings, "_SETTINGS_PATH", path) + monkeypatch.setattr(_settings, "_state", None) + assert _settings.set_max_duration_sec(99 * 60) == 3600 + assert _settings.set_max_duration_sec(1) == 60 diff --git a/tests/test_settings_durability.py b/tests/test_settings_durability.py index ad51dd34..06332903 100644 --- a/tests/test_settings_durability.py +++ b/tests/test_settings_durability.py @@ -3,9 +3,8 @@ `_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. +`set_*()` then persisted a single key over it, so a real user lost `port` and +`allow_network` with only a warning in the log. """ from __future__ import annotations @@ -26,19 +25,6 @@ def test_an_absent_file_is_a_first_run(tmp_path): 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") @@ -49,30 +35,14 @@ def test_an_unreadable_file_is_kept_for_diagnosis(tmp_path): 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) +def test_a_corrupt_file_falls_back_to_defaults(tmp_path): _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): +def test_a_non_object_settings_file_is_treated_as_unusable(tmp_path): # 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() == {} diff --git a/tests/test_settings_mirror.py b/tests/test_settings_mirror.py deleted file mode 100644 index d57e6ccb..00000000 --- a/tests/test_settings_mirror.py +++ /dev/null @@ -1,150 +0,0 @@ -"""The per-user settings copy that survives extracting a new package (#421). - -A Windows portable package keeps its data in `/data`, so settings.json -lives inside the install directory. Upgrading by extracting the new zip to a -fresh folder started that install with no settings at all -- stems location, -port, compute device, quality and language silently back to defaults, and a -relocated library looking empty. - -The desktop shell already restored from a per-user copy in `ensure_workspace`; -nothing had written it since the data directory moved. These cover the write -half. -""" - -from __future__ import annotations - -import json - -from app.core import settings as _settings - - -def test_settings_are_mirrored_when_the_shell_asks_for_it(tmp_path, monkeypatch): - mirror = tmp_path / "shared" / "settings.json" - monkeypatch.setenv("STEMDECK_SETTINGS_MIRROR", str(mirror)) - - _settings.set_max_duration_sec(11 * 60) - - assert mirror.is_file(), "a fresh install has nothing to restore from without this" - assert json.loads(mirror.read_text(encoding="utf-8"))["max_duration_sec"] == 11 * 60 - - -def test_the_mirror_tracks_later_changes(tmp_path, monkeypatch): - mirror = tmp_path / "shared" / "settings.json" - monkeypatch.setenv("STEMDECK_SETTINGS_MIRROR", str(mirror)) - - _settings.set_max_duration_sec(5 * 60) - _settings.set_max_duration_sec(9 * 60) - - # Restoring a stale copy would quietly hand back a setting the user had - # already changed, which is the same class of bug as losing it. - assert json.loads(mirror.read_text(encoding="utf-8"))["max_duration_sec"] == 9 * 60 - - -def test_no_mirror_is_written_when_the_shell_does_not_ask(tmp_path, monkeypatch): - # Docker and a source checkout keep their data outside the install already, - # so there is nothing to preserve and nothing should be created. - monkeypatch.delenv("STEMDECK_SETTINGS_MIRROR", raising=False) - stray = tmp_path / "shared" - - _settings.set_max_duration_sec(7 * 60) - - assert not stray.exists() - - -def test_a_failing_mirror_never_fails_the_setting(tmp_path, monkeypatch): - # The mirror is a redundant copy. If it cannot be written -- read-only disk, - # a path the user cannot reach -- the setting the user just changed must - # still be saved and reported as saved. - blocker = tmp_path / "not-a-dir" - blocker.write_text("", encoding="utf-8") - monkeypatch.setenv("STEMDECK_SETTINGS_MIRROR", str(blocker / "nested" / "settings.json")) - - assert _settings.set_max_duration_sec(6 * 60) is not None - assert _settings.get_max_duration_sec() == 6 * 60 - assert ( - json.loads(_settings._SETTINGS_PATH.read_text(encoding="utf-8"))["max_duration_sec"] - == 6 * 60 - ) - - -def test_existing_settings_are_mirrored_without_waiting_for_a_change(tmp_path, monkeypatch): - # Someone who relocated their stems folder in an earlier release and never - # opens Settings again would never trigger a save, so mirroring on save - # alone would leave them exposed on their next install. - _settings._SETTINGS_PATH.write_text( - json.dumps({"jobs_dir": str(tmp_path / "MyStems")}), encoding="utf-8" - ) - _settings._state = None # a fresh process, reading what is already on disk - mirror = tmp_path / "shared" / "settings.json" - monkeypatch.setenv("STEMDECK_SETTINGS_MIRROR", str(mirror)) - - _settings.get_jobs_dir() - - assert mirror.is_file() - assert json.loads(mirror.read_text(encoding="utf-8"))["jobs_dir"] == str(tmp_path / "MyStems") - - -def test_a_first_run_never_clobbers_an_existing_mirror(tmp_path, monkeypatch): - # The restore is what seeds a new install, and it happens before the backend - # starts. If a run with no settings of its own overwrote the copy with its - # defaults, it would destroy the very thing being preserved. - mirror = tmp_path / "shared" / "settings.json" - mirror.parent.mkdir(parents=True) - mirror.write_text(json.dumps({"jobs_dir": str(tmp_path / "MyStems")}), encoding="utf-8") - monkeypatch.setenv("STEMDECK_SETTINGS_MIRROR", str(mirror)) - _settings._state = None - assert not _settings._SETTINGS_PATH.exists() - - _settings.get_jobs_dir() - - assert json.loads(mirror.read_text(encoding="utf-8"))["jobs_dir"] == str(tmp_path / "MyStems") - - -def test_mirror_holds_the_relocated_stems_folder(tmp_path, monkeypatch): - # The reported symptom: a new install went back to the default jobs folder - # because jobs_dir only existed inside the old install directory. - mirror = tmp_path / "shared" / "settings.json" - monkeypatch.setenv("STEMDECK_SETTINGS_MIRROR", str(mirror)) - chosen = tmp_path / "MyStems" - chosen.mkdir() - - _settings.set_jobs_dir(str(chosen)) - - assert json.loads(mirror.read_text(encoding="utf-8"))["jobs_dir"] == str(chosen) - - -def test_the_duration_ceiling_is_published_not_duplicated(): - """The UI used to keep its own copy of the max-track-length ceiling. It - went stale at 20 minutes against a real ceiling of 60, so typing 60 was - silently clamped to 20 and the field snapped back with no explanation. - - The cure was to stop having a second copy, so the bound has to stay in the - payload for the client to read. - """ - from fastapi.testclient import TestClient - - from app.core.settings import DURATION_MAX_SEC, DURATION_MIN_SEC - from app.main import app - - with TestClient(app) as c: - body = c.get("/api/settings").json() - assert body["max_duration_max_sec"] == DURATION_MAX_SEC - assert body["max_duration_min_sec"] == DURATION_MIN_SEC - - -def test_a_value_at_the_ceiling_is_kept(tmp_path, monkeypatch): - """60 minutes is a legal setting. It was reachable through the API all - along; only the client refused to send it.""" - mirror = tmp_path / "settings.json" - monkeypatch.setattr(_settings, "_SETTINGS_PATH", mirror) - monkeypatch.setattr(_settings, "_state", None) - assert _settings.set_max_duration_sec(3600) == 3600 - assert _settings.get_max_duration_sec() == 3600 - - -def test_beyond_the_ceiling_still_clamps(tmp_path, monkeypatch): - mirror = tmp_path / "settings.json" - monkeypatch.setattr(_settings, "_SETTINGS_PATH", mirror) - monkeypatch.setattr(_settings, "_state", None) - assert _settings.set_max_duration_sec(99 * 60) == 3600 - assert _settings.set_max_duration_sec(1) == 60 diff --git a/tests/test_upgrade_defaults.py b/tests/test_upgrade_defaults.py index 055c70d8..7b20a2b1 100644 --- a/tests/test_upgrade_defaults.py +++ b/tests/test_upgrade_defaults.py @@ -45,7 +45,6 @@ def _no_deployment_overrides(monkeypatch): "STEMDECK_STEM_FORMAT", "STEMDECK_STEM_MP3_KBPS", "STEMDECK_AUTO_LYRICS", - "STEMDECK_SETTINGS_MIRROR", ): monkeypatch.delenv(var, raising=False) # One test registers a job; conftest does not clear the registry.