diff --git a/app/core/settings.py b/app/core/settings.py index 0b6c3aa..18da525 100644 --- a/app/core/settings.py +++ b/app/core/settings.py @@ -81,6 +81,15 @@ 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 @@ -94,10 +103,45 @@ def _save() -> bool: try: _SETTINGS_PATH.parent.mkdir(parents=True, exist_ok=True) _SETTINGS_PATH.write_text(json.dumps(_ensure()), encoding="utf-8") - return True except Exception: _log.warning("could not persist settings to %s", _SETTINGS_PATH, exc_info=True) 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 already restores from this copy -- `ensure_workspace` + seeds a fresh portable install from it before the backend ever reads + settings.json. Only the write half was missing, because #399 moved the + data directory and nothing took over writing the old location. + + 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. + """ + target = os.environ.get("STEMDECK_SETTINGS_MIRROR", "").strip() + if not target: + 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) def _num(v: object) -> int | None: diff --git a/desktop/src-tauri/src/main.rs b/desktop/src-tauri/src/main.rs index 3b7d36a..2760638 100644 --- a/desktop/src-tauri/src/main.rs +++ b/desktop/src-tauri/src/main.rs @@ -113,6 +113,7 @@ struct BackendHandles { url: String, } +#[derive(Default)] struct BackendStateInner { handles: Option, /// True while start_backend is executing; prevents concurrent starts (#145). @@ -139,18 +140,6 @@ struct BackendStateInner { /// process. const MAX_PENDING_SAVES: usize = 16; -impl Default for BackendStateInner { - fn default() -> Self { - BackendStateInner { - handles: None, - starting: false, - setup_child_pid: None, - pending_saves: HashMap::new(), - next_save_token: 0, - } - } -} - struct BackendState { inner: Mutex, } @@ -423,16 +412,16 @@ fn main() { ]) .build(tauri::generate_context!()) .expect("failed to build StemDeck desktop app") - .run(|app_handle, event| match event { - tauri::RunEvent::WindowEvent { + .run(|app_handle, event| { + if let tauri::RunEvent::WindowEvent { event: tauri::WindowEvent::CloseRequested { .. }, .. - } => { + } = event + { let state = app_handle.state::(); stop_backend(&state); app_handle.exit(0); } - _ => {} }); } @@ -1284,6 +1273,48 @@ fn apply_app_update( } } +/// 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*. Upgrading by extracting the new +/// zip to a fresh folder therefore lost every setting the user had changed: +/// stems location, port, compute device, quality, language. `ensure_workspace` +/// already restores from here, but nothing wrote it after #399 moved the data +/// directory — the backend mirrors to it now, via STEMDECK_SETTINGS_MIRROR. +/// +/// 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") + }) + } +} + /// Creates required data directories and runs any pending data migrations. #[tauri::command] fn ensure_workspace() -> Result<(), String> { @@ -1301,7 +1332,16 @@ fn ensure_workspace() -> Result<(), String> { } } - migrate_legacy_data(&root, &data); + #[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"] { fs::create_dir_all(data.join(dir)) @@ -1498,6 +1538,14 @@ fn start_backend( .env("STEMDECK_DATA_DIR", &data_dir) .env("STEMDECK_DEFAULT_JOBS_DIR", &jobs_dir) .env("STEMDECK_DESKTOP", "1") + // Where the backend keeps the per-user copy of settings.json that + // survives extracting a new package into a fresh folder. Computed + // here so the write half and ensure_workspace's restore half can + // never point at different places. + .envs( + shared_settings_dir() + .map(|dir| ("STEMDECK_SETTINGS_MIRROR", dir.join("settings.json"))), + ) .env("STEMDECK_PARENT_PID", std::process::id().to_string()) .env("PYTHONUNBUFFERED", "1") .env("XDG_CACHE_HOME", data_dir.join("cache")) @@ -1917,12 +1965,7 @@ fn parse_cuda_version(smi_output: &str) -> Option { for line in smi_output.lines() { if let Some(pos) = line.find("CUDA Version:") { let rest = &line[pos + "CUDA Version:".len()..]; - let v = rest - .trim() - .split_whitespace() - .next()? - .trim_matches('|') - .trim(); + let v = rest.split_whitespace().next()?.trim_matches('|').trim(); if !v.is_empty() && v != "N/A" { return Some(v.to_string()); } @@ -2567,7 +2610,7 @@ async fn save_audio_file( } fn stop_backend(state: &BackendState) { - let (handles, setup_child_pid) = match state.inner.lock() { + let (handles, _setup_child_pid) = match state.inner.lock() { Ok(mut guard) => (guard.handles.take(), guard.setup_child_pid.take()), Err(_) => return, }; @@ -2575,7 +2618,7 @@ fn stop_backend(state: &BackendState) { // Kill any in-progress setup-time subprocess (pip install, model warmup) // so it doesn't corrupt the venv/cache if the window is closed mid-setup (#140). #[cfg(unix)] - if let Some(pid) = setup_child_pid { + if let Some(pid) = _setup_child_pid { // SAFETY: pid was stored immediately after spawn; we send SIGTERM best-effort. unsafe { libc::kill(pid as libc::pid_t, libc::SIGTERM) }; } @@ -2671,30 +2714,86 @@ fn append_to_setup_log(data_dir: &Path, msg: &str) { /// One-time migration: move legacy data/models/jobs/ffmpeg from the install /// directory into the new per-user data directory on the user's first launch -/// after upgrading to a version that uses local_data_dir(). -fn migrate_legacy_data(root: &Path, data_dir: &Path) { +/// after upgrading to a version that uses local_data_dir(). User-owned settings +/// are copied as well so reinstalling cannot silently restore defaults. +fn migrate_legacy_data(root: &Path, data_dir: &Path) -> Result<(), String> { let old = root.join("data"); - // Only migrate if the old location exists and the new one doesn't yet. - if !old.is_dir() || data_dir.exists() { - return; + if !old.is_dir() || old == data_dir { + return Ok(()); } + let _ = fs::create_dir_all(data_dir); for name in ["models", "jobs", "ffmpeg", "logs", "cache"] { let src = old.join(name); - if src.is_dir() { + let destination = data_dir.join(name); + if src.is_dir() && !destination.exists() { // rename is a cheap move on the same volume; ignore errors silently // so a cross-volume failure doesn't block startup. - let _ = fs::rename(&src, data_dir.join(name)); + let _ = fs::rename(&src, destination); } } // NOTE: deliberately does NOT migrate `cpu-only` -- the marker is only // trusted in the app root (see is_cpu_only_package); carrying it into the // shared data dir poisoned later NVIDIA installs (#247). - { - let src = old.join("config.json"); - if src.exists() { - let _ = fs::copy(&src, data_dir.join("config.json")); + migrate_persisted_files(&old, data_dir, &["config.json", "settings.json"]) +} + +/// Copy persisted choices from an older data directory without ever replacing +/// state already written at the destination. +fn migrate_persisted_files( + source_dir: &Path, + data_dir: &Path, + names: &[&str], +) -> Result<(), String> { + if source_dir == data_dir || !source_dir.is_dir() { + return Ok(()); + } + fs::create_dir_all(data_dir) + .map_err(|e| format!("failed to prepare settings migration: {e}"))?; + for name in names { + let source = source_dir.join(name); + let destination = data_dir.join(name); + if source.is_file() && !destination.exists() { + let nonce = SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|duration| duration.as_nanos()) + .unwrap_or(0); + let temporary = data_dir.join(format!( + ".{name}.migrate.{}.{nonce}.tmp", + std::process::id() + )); + let result = (|| -> Result<(), String> { + let mut input = fs::File::open(&source) + .map_err(|e| format!("failed to read existing {name}: {e}"))?; + let mut output = fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(&temporary) + .map_err(|e| format!("failed to stage existing {name}: {e}"))?; + std::io::copy(&mut input, &mut output) + .map_err(|e| format!("failed to copy existing {name}: {e}"))?; + output + .flush() + .map_err(|e| format!("failed to flush existing {name}: {e}"))?; + output + .sync_all() + .map_err(|e| format!("failed to sync existing {name}: {e}"))?; + drop(output); + + // Another process may have completed migration while this copy + // was staged. Its destination wins; never replace it. + if destination.exists() { + return Ok(()); + } + fs::rename(&temporary, &destination) + .map_err(|e| format!("failed to preserve existing {name}: {e}")) + })(); + if temporary.exists() { + let _ = fs::remove_file(&temporary); + } + result?; } } + Ok(()) } fn runtime_dir(data_dir: &Path) -> PathBuf { @@ -3355,7 +3454,7 @@ fn ensure_ffmpeg(data_dir: &Path) -> Result { let portable = ffmpeg_path(data_dir).ok_or_else(|| "failed to resolve FFmpeg path".to_string())?; verify_ffmpeg(&portable)?; - return Ok(portable); + Ok(portable) } #[cfg(target_os = "macos")] @@ -4090,6 +4189,87 @@ mod tests { tempfile::tempdir().expect("failed to create temp dir") } + #[test] + fn legacy_migration_preserves_user_settings_when_data_dir_already_exists() { + // setup() creates the destination before ensure_workspace() invokes + // migration, which used to make migration return without copying any + // user state at all. + let root = make_tmp(); + let destination_parent = make_tmp(); + let destination = destination_parent.path().join("StemDeck"); + fs::create_dir_all(&destination).unwrap(); + fs::create_dir_all(root.path().join("data")).unwrap(); + let settings = br#"{"jobs_dir":"D:\\Audio\\StemDeck","separation_quality":"best"}"#; + fs::write(root.path().join("data/settings.json"), settings).unwrap(); + fs::write( + root.path().join("data/config.json"), + br#"{"torchDevice":"cuda"}"#, + ) + .unwrap(); + + super::migrate_legacy_data(root.path(), &destination).unwrap(); + + assert_eq!( + fs::read(destination.join("settings.json")).unwrap(), + settings + ); + assert!(destination.join("config.json").is_file()); + assert!(root.path().join("data/settings.json").is_file()); + assert!( + fs::read_dir(&destination).unwrap().all(|entry| !entry + .unwrap() + .file_name() + .to_string_lossy() + .contains(".migrate.")), + "successful migration must not leave staging files" + ); + } + + #[test] + fn legacy_migration_never_overwrites_newer_user_settings() { + let root = make_tmp(); + let destination_parent = make_tmp(); + let destination = destination_parent.path().join("StemDeck"); + fs::create_dir_all(root.path().join("data")).unwrap(); + fs::create_dir_all(&destination).unwrap(); + fs::write( + root.path().join("data/settings.json"), + br#"{"jobs_dir":"D:\\Old"}"#, + ) + .unwrap(); + let current = br#"{"jobs_dir":"E:\\Current"}"#; + fs::write(destination.join("settings.json"), current).unwrap(); + + super::migrate_legacy_data(root.path(), &destination).unwrap(); + + assert_eq!( + fs::read(destination.join("settings.json")).unwrap(), + current + ); + } + + #[test] + fn portable_migration_carries_preferences_but_not_install_readiness() { + let source = make_tmp(); + let destination_parent = make_tmp(); + let destination = destination_parent.path().join("data"); + fs::write( + source.path().join("settings.json"), + br#"{"jobs_dir":"D:\\Audio"}"#, + ) + .unwrap(); + fs::write( + source.path().join("config.json"), + br#"{"modelReady":true,"ffmpegReady":true}"#, + ) + .unwrap(); + + super::migrate_persisted_files(source.path(), &destination, &["settings.json"]).unwrap(); + + assert!(destination.join("settings.json").is_file()); + assert!(!destination.join("config.json").exists()); + } + // ── stale app-data cleanup (#356) ──────────────────────────────────────── fn seed_downloads(dir: &std::path::Path, names: &[(&str, usize)]) { diff --git a/templates/stemdeck.xml b/templates/stemdeck.xml index d63c6b5..dff9c39 100644 --- a/templates/stemdeck.xml +++ b/templates/stemdeck.xml @@ -1,7 +1,7 @@ StemDeck - ghcr.io/stemdeckapp/stemdeck:0.14.0 + ghcr.io/stemdeckapp/stemdeck:0.14.1 https://github.com/stemdeckapp/stemdeck/pkgs/container/stemdeck bridge sh diff --git a/tests/test_settings_mirror.py b/tests/test_settings_mirror.py new file mode 100644 index 0000000..55f0b82 --- /dev/null +++ b/tests/test_settings_mirror.py @@ -0,0 +1,113 @@ +"""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)