From 262f759ed34d6ea6d7ed41529a7f827baa98a459 Mon Sep 17 00:00:00 2001 From: Afonso Jorge Ramos Date: Thu, 17 Sep 2026 01:40:24 +0200 Subject: [PATCH 1/3] fix(daemon): prevent service loss and console flashes after apply (#3948) --- docs/daemon-apply-verification.md | 128 ++++++++++++++++++ docs/windows-update-verification.md | 5 + rust/crates/daemon/src/routes.rs | 58 +++++++- rust/crates/daemon/src/server.rs | 5 +- rust/crates/daemon/src/update_job.rs | 6 +- rust/crates/daemon/src/watcher.rs | 108 ++++++++++----- rust/crates/spicetify/src/commands/apply.rs | 36 +++-- rust/crates/spicetify/src/commands/mod.rs | 10 +- .../crates/spicetify/src/commands/protocol.rs | 25 ++-- .../spicetify/src/hooks/version_detect.rs | 3 +- rust/crates/spicetify/src/platform/windows.rs | 4 +- rust/crates/spicetify/src/process.rs | 32 ++++- rust/crates/tui/src/app/input.rs | 6 +- 13 files changed, 358 insertions(+), 68 deletions(-) create mode 100644 docs/daemon-apply-verification.md diff --git a/docs/daemon-apply-verification.md b/docs/daemon-apply-verification.md new file mode 100644 index 0000000000..a1b1a7401d --- /dev/null +++ b/docs/daemon-apply-verification.md @@ -0,0 +1,128 @@ +# Daemon-owned Apply verification, 2026-09-16 + +Applying from the client could stop the daemon permanently and disable its +autostart entry. Hide Window Controls then reported that the service was +unavailable. Restarting the daemon recovered that module without reopening +Spotify. + +The RPC handler ran synchronous commands on the daemon's single async thread. +Apply's request to its own health endpoint timed out, so Apply treated the +daemon's version as unknown, unregistered it, and killed its own process. +Daemon-owned Apply could also register `spicetify-daemon` as the URL handler, +although that executable does not implement CLI protocol commands. + +RPC commands now execute on blocking workers. Explicit Apply modes keep daemon +maintenance and URL registration in the foreground CLI. RPC Apply, watcher +repairs, and the update transaction preserve their owning daemon. Blocking +workers are no longer limited to one, so a watcher waiting for Spotify to exit +does not prevent RPC commands from reaching the operation guard. + +The watcher could also keep waiting after another Apply had consumed the stock +archive. On Windows, each process check launched `tasklist.exe` with a visible +console. Process sampling confirmed daemon-owned `tasklist.exe` starts about +2.2 seconds apart, each followed by an `OpenConsole.exe` start. The watcher now +stops waiting when repair is no longer pending. Windows process and PowerShell +helpers use `CREATE_NO_WINDOW` so legitimate background checks stay hidden too. + +## Automated checks + +- A regression holds the Apply file lock while dispatching a real Apply RPC. + The old handler stalled the async runtime for five seconds and failed. The + fixed handler kept it responsive and passed in 0.06 seconds. The fixture + refuses a foreign apply before any real Spotify operation. +- `cargo +1.95.0 test --workspace --locked --features daemon/native-window-controls-tests`: + 144 passed, three existing tests requiring real bundles or registry downloads ignored. +- A watcher regression consumes the archive while Spotify remains running and + verifies that polling stops and the operation lock remains available. +- A Windows child-process test verifies that a background PowerShell helper has + no console while preserving its output and nonzero exit status. +- `cargo +1.95.0 clippy --workspace --locked -- -D warnings`: passed. + A pre-existing TUI Backspace match required a behavior-preserving lint fix. +- An additional `--all-targets` Clippy scan found existing test-only warnings + outside this regression. That broader scan is not the repository CI command + and is not reported as passing. +- Matching release CLI/daemon binaries were built with the current payload. + +## Windows live run + +The patched pair was installed in the normal local installation directory, +with backups retained. Both still identify as 3.0.0-beta.17; this is a local +build, not a published release. The existing daemon was explicitly restarted +before testing because its version alone cannot distinguish local builds. + +An authenticated `spicetify:0:apply` request completed against Spotify desktop +1.3.0.277. This used the same RPC as the client but was sent by a diagnostic +script, **not clicked in the UI**. + +- All 109 concurrent health requests succeeded; maximum latency was 17 ms. +- Daemon PID 28012 survived the operation, with monotonically increasing uptime. +- Apply finished and automatically launched Spotify, which exposed a window + titled `Spotify Free`. +- Autostart stayed enabled and the URL handler still targeted `spicetify.exe`. +- No daemon restart or registration mutation appeared in the Apply log. +- Spotify updates remained blocked. + +Evidence is retained locally under the workspace's +`scratchpad/daemon-owned-apply/`. It is not a release fixture. + +### Follow-up after the console fix + +The matching CLI and daemon were rebuilt and installed, then the same diagnostic +RPC Apply was repeated. All 84 health requests succeeded with a maximum latency +of 28 ms. Daemon PID 28800 survived; autostart, CLI URL registration, and update +protection remained intact. + +A 45-second process sample spanning Apply and the period after it recorded the +expected daemon-owned helpers during Apply and no new `OpenConsole.exe` process. +There were no recurring `tasklist.exe` starts after completion. This run's +watcher saw the already-applied client and skipped repair; the cancellation of +an existing wait is covered by the regression test, not this live timing. +The latest RPC result, Apply log, and helper-process sample are retained in the +same local evidence directory. This is process-level verification, not a native +visual pass. + +## End-user coverage and remaining limits + +Before installing this fix, native Computer Use verified the profile-menu +settings route, Manager's six loaded modules, playback, elapsed-time rendering, +and recovery of hidden window controls after restarting the stopped daemon. + +The native bridge subsequently became unavailable in the current host session. +Both a fresh connection and a session reset returned `native pipe unavailable`. +Consequently the fixed build's automatic launch was verified as a process and +window, not visually inspected. The Windows UI Apply action and the first +patched renderer after an actual Spotify version update still need a native +end-user pass. The earlier [Windows update report](windows-update-verification.md) +remains accurate; this run does not clear its first-boot limitation or enable +Windows Update & Apply in release builds. + + +## macOS visible Store Apply, 2026-09-17 + +A combined local build at `115d010` included this fix, GraphQL discovery and +protocol signing (#3951), and launchd reconciliation (#3952). Matching CLI and +daemon binaries were installed in the normal installation directory after +backing up both binaries and Spotify. This remains a local build reporting +3.0.0-beta.17, not a published release. + +The fixture used the published, checksum-verified stdlib 1.11.3 artifact, +installed and applied through the CLI. From the visible Module Store, **Update +all** staged stdlib 1.12.0. **Apply stdlib update** opened the restart warning. +**Cancel** preserved the staged update. Reopening the confirmation and clicking +**Apply and restart** restarted Spotify 1.3.0.277. The Home view and Module Store +rendered with the existing theme, the Apply banner cleared, and the manifest +and installed module link both returned to stdlib 1.12.0. + +Daemon PID 3060 survived the operation with increasing uptime and an unchanged +launch-agent plist. The concurrent health sample recorded no failures or uptime +resets. Both watchers remained active. The watcher observed the temporary stock +archive during Apply and cancelled its pending repair once that archive was +consumed. The normal macOS URL handoff, `open spicetify:0:apply`, subsequently +completed another Apply and restart with the same daemon PID. The registered +applet passed strict code-signature verification. + +Fixture preparation and the URL handoff used CLI commands; the Store update, +cancellation, confirmation, restart, and returned views were exercised through +the native UI. This run does not test a Spotify version upgrade, Windows native +UI behavior, or browser confirmation prompts for custom URL schemes. Evidence +is retained under `scratchpad/final-reconciliation/` in the local workspace. diff --git a/docs/windows-update-verification.md b/docs/windows-update-verification.md index 6e8e5b4387..4e317e060d 100644 --- a/docs/windows-update-verification.md +++ b/docs/windows-update-verification.md @@ -76,6 +76,11 @@ These files are local diagnostic artifacts, not release fixtures. ## Remaining verification +The later [daemon Apply verification](daemon-apply-verification.md) diagnoses +and fixes a separate self-shutdown path seen during an ordinary in-client +Apply. Its authenticated RPC run passed, but it does not replace the missing +first-boot visual check below. + Repeat an actual version update with reliable first-boot observation and without a diagnostic restart. Check loaded modules through the normal UI before calling the job's user outcome complete. Microsoft Store installations, Linux, diff --git a/rust/crates/daemon/src/routes.rs b/rust/crates/daemon/src/routes.rs index 0797a20d1a..7b92615fbd 100644 --- a/rust/crates/daemon/src/routes.rs +++ b/rust/crates/daemon/src/routes.rs @@ -203,8 +203,7 @@ async fn handle_ws(mut socket: WebSocket, state: Arc) { while let Some(Ok(msg)) = socket.next().await { if let Message::Text(text) = msg { tracing::info!("{}", spicetify::fl!("rpc-received", msg = text.as_str())); - let ctx = state.ctx.load(); - match protocol::handle(&ctx, &text) { + match dispatch_rpc(state.ctx.load_full(), text.to_string()).await { Ok(res) if !res.is_empty() => { if let Err(e) = socket.send(Message::Text(res.into())).await { tracing::warn!(error = %e, "failed to send ws message"); @@ -226,6 +225,16 @@ async fn handle_ws(mut socket: WebSocket, state: Arc) { } } +async fn dispatch_rpc( + ctx: Arc, + text: String, +) -> anyhow::Result { + tokio::task::spawn_blocking(move || { + protocol::handle(&ctx, &text, spicetify::commands::apply::ApplyMode::Daemon) + }) + .await? +} + // A cross-origin POST is sent even when the browser refuses to let the page // read the reply, so without a token any page the user visits could stop the // daemon and silently disable auto re-apply. @@ -246,6 +255,51 @@ async fn shutdown_handler( mod tests { use super::*; + #[tokio::test] + async fn apply_rpc_does_not_block_the_server_while_waiting_for_the_apply_lock() + -> anyhow::Result<()> { + use spicetify::context::{AppContext, Config}; + use std::fs::OpenOptions; + + let nonce = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH)?.as_nanos(); + let root = + std::env::temp_dir().join(format!("spicetify-rpc-{}-{nonce}", std::process::id())); + // Foreign apply artifacts make the command fail before it can touch Spotify. + std::fs::create_dir_all(root.join("Apps/xpui"))?; + let ctx = Arc::new(AppContext::from_config( + root.clone(), + &Config { + spotify_exec: Some(root.join("Spotify")), + spotify_data_dir: Some(root.clone()), + offline_bnk_dir: Some(root.clone()), + ..Config::default() + }, + )?); + let lock = OpenOptions::new() + .create(true) + .truncate(false) + .write(true) + .open(root.join("spicetify-apply.lock"))?; + lock.lock()?; + let (release, held) = std::sync::mpsc::channel(); + // A bounded external release keeps a regression from hanging the test runtime. + let holder = std::thread::spawn(move || { + let _ = held.recv_timeout(Duration::from_secs(5)); + drop(lock); + }); + let rpc = tokio::spawn(dispatch_rpc(ctx, "spicetify:0:apply".to_string())); + let started = std::time::Instant::now(); + tokio::time::sleep(Duration::from_millis(50)).await; + let responsive = started.elapsed() < Duration::from_secs(2) && !rpc.is_finished(); + let _ = release.send(()); + let result = rpc.await?; + holder.join().expect("lock holder exits"); + std::fs::remove_dir_all(root)?; + assert!(result.is_err(), "fixture must refuse a foreign apply"); + assert!(responsive, "the server must keep polling while Apply waits on disk"); + Ok(()) + } + fn headers(protocols: &str) -> HeaderMap { let mut h = HeaderMap::new(); let _ = diff --git a/rust/crates/daemon/src/server.rs b/rust/crates/daemon/src/server.rs index 8d008c76a2..e6bcfc3394 100644 --- a/rust/crates/daemon/src/server.rs +++ b/rust/crates/daemon/src/server.rs @@ -40,10 +40,7 @@ pub fn run() -> anyhow::Result<()> { fn start(ctx: AppContext) -> anyhow::Result<()> { let _lock = acquire_instance_lock(&ctx.config_root)?; - let runtime = tokio::runtime::Builder::new_current_thread() - .enable_all() - .max_blocking_threads(1) - .build()?; + let runtime = tokio::runtime::Builder::new_current_thread().enable_all().build()?; runtime.block_on(async move { let shared = Arc::new(SharedContext::new(ctx)); let shutdown = Arc::new(tokio::sync::Notify::new()); diff --git a/rust/crates/daemon/src/update_job.rs b/rust/crates/daemon/src/update_job.rs index 961cc81123..6778b1e1d0 100644 --- a/rust/crates/daemon/src/update_job.rs +++ b/rust/crates/daemon/src/update_job.rs @@ -637,7 +637,11 @@ impl Supervisor { ); return; }; - if let Err(e) = spicetify::commands::apply::run(ctx, guard, false) { + if let Err(e) = spicetify::commands::apply::run( + ctx, + guard, + spicetify::commands::apply::ApplyMode::Daemon, + ) { self.secure_failure( FailureCode::ApplyFailed, &format!("Spicetify apply failed after Spotify updated: {e}"), diff --git a/rust/crates/daemon/src/watcher.rs b/rust/crates/daemon/src/watcher.rs index 6d7cb991ea..e28501229b 100644 --- a/rust/crates/daemon/src/watcher.rs +++ b/rust/crates/daemon/src/watcher.rs @@ -104,10 +104,17 @@ fn auto_apply(ctx: &AppContext, nth: u32) { nth, "auto-apply triggered by a Spotify update; waiting for pending package operations" ); - let guard = match wait_for_idle_guard(&ctx.config_root, CLIENT_EXIT_CEILING, || { - spicetify::lifecycle::is_running(ctx) - }) { - Ok(guard) => guard, + let guard = match wait_for_idle_guard( + &ctx.config_root, + CLIENT_EXIT_CEILING, + || ctx.spotify_apps_path().join("xpui.spa").is_file(), + || spicetify::lifecycle::is_running(ctx), + ) { + Ok(Some(guard)) => guard, + Ok(None) => { + tracing::info!("stock xpui.spa is no longer present; cancelling pending auto-apply"); + return; + } Err(e) => { tracing::warn!(error = %e, "auto-apply could not acquire an idle client; run `spicetify apply` when convenient"); return; @@ -117,7 +124,7 @@ fn auto_apply(ctx: &AppContext, nth: u32) { tracing::info!("stock xpui.spa is no longer present; skipping auto-apply"); return; } - if let Err(e) = commands::apply::run(ctx, &guard, false) { + if let Err(e) = commands::apply::run(ctx, &guard, commands::apply::ApplyMode::Daemon) { tracing::warn!(error = %e, "auto-apply failed"); } } @@ -125,11 +132,15 @@ fn auto_apply(ctx: &AppContext, nth: u32) { fn wait_for_idle_guard( config_root: &std::path::Path, timeout: Duration, + mut repair_pending: impl FnMut() -> bool, mut is_running: impl FnMut() -> bool, -) -> anyhow::Result { +) -> anyhow::Result> { let deadline = std::time::Instant::now() + timeout; let mut waited = false; loop { + if !repair_pending() { + return Ok(None); + } let remaining = deadline.saturating_duration_since(std::time::Instant::now()); if remaining.is_zero() { anyhow::bail!( @@ -151,7 +162,7 @@ fn wait_for_idle_guard( config_root, remaining.min(Duration::from_secs(2)), ) { - Ok(guard) if !is_running() => return Ok(guard), + Ok(guard) if !is_running() => return Ok(Some(guard)), // Release the guard and resume the polite wait if Spotify relaunched. Ok(_) => waited = true, Err(error) if commands::guard::is_contention(&error) => {} @@ -299,15 +310,20 @@ mod tests { let root = scratch("contention")?; let mut competing = Some(commands::guard::try_acquire(&root)?); let mut checks = 0; - let result = wait_for_idle_guard(&root, Duration::from_secs(10), || { - checks += 1; - if checks == 2 { - drop(competing.take()); - } - false - }); + let result = wait_for_idle_guard( + &root, + Duration::from_secs(10), + || true, + || { + checks += 1; + if checks == 2 { + drop(competing.take()); + } + false + }, + ); drop(competing); - drop(result?); + drop(result?.expect("repair remains pending")); assert!(checks >= 3, "retry must recheck the client before and after acquiring"); std::fs::remove_dir_all(root)?; Ok(()) @@ -317,20 +333,25 @@ mod tests { fn auto_apply_resumes_waiting_when_spotify_relaunches() -> anyhow::Result<()> { let root = scratch("relaunch")?; let mut checks = 0; - let result = wait_for_idle_guard(&root, Duration::from_secs(10), || { - checks += 1; - match checks { - // Idle before locking, but restarted by the post-lock check. - 2 => true, - 3 => { - // The polite wait must not keep package operations locked. - drop(commands::guard::try_acquire(&root).expect("guard released")); - false + let result = wait_for_idle_guard( + &root, + Duration::from_secs(10), + || true, + || { + checks += 1; + match checks { + // Idle before locking, but restarted by the post-lock check. + 2 => true, + 3 => { + // The polite wait must not keep package operations locked. + drop(commands::guard::try_acquire(&root).expect("guard released")); + false + } + _ => false, } - _ => false, - } - }); - drop(result?); + }, + ); + drop(result?.expect("repair remains pending")); assert!(checks >= 5, "a restart must resume waiting, not abandon the repair"); std::fs::remove_dir_all(root)?; Ok(()) @@ -340,19 +361,42 @@ mod tests { fn auto_apply_wait_has_one_deadline_and_preserves_filesystem_errors() -> anyhow::Result<()> { let root = scratch("deadline")?; let guard = commands::guard::try_acquire(&root)?; - let error = wait_for_idle_guard(&root, Duration::from_millis(20), || false) - .expect_err("the operation guard is still held"); + let error = wait_for_idle_guard(&root, Duration::from_millis(20), || true, || false) + .expect_err("operation remains locked"); assert!(error.to_string().contains("still busy")); drop(guard); let file = root.join("not-a-directory"); std::fs::write(&file, "sentinel")?; - let error = wait_for_idle_guard(&file, Duration::from_secs(60), || false) - .expect_err("the config root is a file"); + let error = wait_for_idle_guard(&file, Duration::from_mins(1), || true, || false) + .expect_err("lock path is not a directory"); assert!(error.downcast_ref::().is_some()); std::fs::remove_dir_all(root)?; Ok(()) } + #[test] + fn auto_apply_stops_polling_when_another_apply_consumes_the_archive() -> anyhow::Result<()> { + let root = scratch("completed-elsewhere")?; + let archive = root.join("xpui.spa"); + std::fs::write(&archive, "stock")?; + let mut process_checks = 0; + let result = wait_for_idle_guard( + &root, + Duration::from_secs(10), + || archive.is_file(), + || { + process_checks += 1; + std::fs::remove_file(&archive).expect("another apply consumes the archive"); + true + }, + )?; + assert!(result.is_none(), "completed repair must not wait for Spotify to exit"); + assert_eq!(process_checks, 1, "polling must stop even though Spotify is still running"); + drop(commands::guard::try_acquire(&root)?); + std::fs::remove_dir_all(root)?; + Ok(()) + } + #[expect(clippy::unnecessary_wraps, reason = "matches the channel's item type")] fn event() -> notify::Result { Ok(Event { kind: EventKind::Modify(notify::event::ModifyKind::Any), ..Event::default() }) diff --git a/rust/crates/spicetify/src/commands/apply.rs b/rust/crates/spicetify/src/commands/apply.rs index 3f24fd55b4..59c83bf9c2 100644 --- a/rust/crates/spicetify/src/commands/apply.rs +++ b/rust/crates/spicetify/src/commands/apply.rs @@ -9,6 +9,15 @@ use crate::{fl, util}; const APPLY_LOCK_FILE: &str = "spicetify-apply.lock"; +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ApplyMode { + Cli { + no_cache: bool, + }, + /// Preserve the daemon executing this apply and the CLI's URL registration. + Daemon, +} + // The foreground CLI and daemon both enter this command and mutate the same // xpui.spa, backup and xpui.tmp paths. Keep one persistent lock file: deleting // it on drop could let a third process lock a new inode while a waiter still @@ -44,9 +53,9 @@ fn fs_err<'a>(doing: &'a str, path: &'a Path) -> impl FnOnce(std::io::Error) -> pub fn run( ctx: &AppContext, _operation_guard: &super::guard::DisruptiveOperationGuard, - no_cache: bool, + mode: ApplyMode, ) -> Result<()> { - run_inner(ctx, no_cache, true) + run_inner(ctx, mode, true) } /// Prepare an isolated installation without stopping or launching the active client. @@ -57,10 +66,11 @@ pub(crate) fn prepare( ) -> Result<()> { // Updater changes can stop Spotify too; defer those until activation. let staging = AppContext { block_spotify_updates: Some(false), ..ctx.clone() }; - run_inner(&staging, false, false) + run_inner(&staging, ApplyMode::Cli { no_cache: false }, false) } -fn run_inner(ctx: &AppContext, no_cache: bool, activate: bool) -> Result<()> { +fn run_inner(ctx: &AppContext, mode: ApplyMode, activate: bool) -> Result<()> { + let no_cache = matches!(mode, ApplyMode::Cli { no_cache: true }); let _apply_lock = acquire_apply_lock(&ctx.config_root)?; let dest_apps = ctx.dest_apps_path(); let spa = ctx.spotify_apps_path().join("xpui.spa"); @@ -195,15 +205,24 @@ fn run_inner(ctx: &AppContext, no_cache: bool, activate: bool) -> Result<()> { super::updates::finalize_app_signature(ctx)?; if activate { - ensure_daemon(ctx); - crate::lifecycle::start(ctx)?; - crate::platform::register_url_scheme(); + activate_client(ctx, mode)?; } tracing::info!("{}", fl!("applied-patches")); Ok(()) } +fn activate_client(ctx: &AppContext, mode: ApplyMode) -> Result<()> { + if matches!(mode, ApplyMode::Cli { .. }) { + ensure_daemon(ctx); + } + crate::lifecycle::start(ctx)?; + if matches!(mode, ApplyMode::Cli { .. }) { + crate::platform::register_url_scheme(); + } + Ok(()) +} + fn detect_supported_spotify_version(ctx: &AppContext) -> Result> { match crate::hooks::version_detect::detect_spotify_version(ctx) { Ok(version) if !crate::hooks::version_detect::spotify_supported(&version) => { @@ -871,7 +890,8 @@ mod tests { let apply = std::thread::spawn(move || { let guard = super::super::guard::try_acquire(&ctx.config_root) .expect("synthetic apply owns the disruptive-operation guard"); - tx.send(run(&ctx, &guard, false)).expect("test receiver remains available"); + tx.send(run(&ctx, &guard, ApplyMode::Cli { no_cache: false })) + .expect("test receiver remains available"); }); assert!( rx.recv_timeout(std::time::Duration::from_millis(100)).is_err(), diff --git a/rust/crates/spicetify/src/commands/mod.rs b/rust/crates/spicetify/src/commands/mod.rs index 9b0a1f9517..d236b9de4f 100644 --- a/rust/crates/spicetify/src/commands/mod.rs +++ b/rust/crates/spicetify/src/commands/mod.rs @@ -71,7 +71,7 @@ pub fn dispatch(cmd: &Command, ctx: &AppContext) -> Result<()> { match cmd { Command::Apply { no_cache } => { let guard = guard::try_acquire(&ctx.config_root)?; - apply::run(ctx, &guard, *no_cache) + apply::run(ctx, &guard, apply::ApplyMode::Cli { no_cache: *no_cache }) } Command::Config(action) => match action { ConfigAction::Show => config::run(ctx), @@ -170,8 +170,12 @@ mod tests { "fast-delete", "fast-remove", ] { - let error = protocol::handle(&ctx, &format!("spicetify:0:{action}?id=module%401")) - .expect_err("competing protocol mutation"); + let error = protocol::handle( + &ctx, + &format!("spicetify:0:{action}?id=module%401"), + apply::ApplyMode::Daemon, + ) + .expect_err("competing protocol mutation"); assert!(error.to_string().contains("already in progress"), "{error}"); } let error = diff --git a/rust/crates/spicetify/src/commands/protocol.rs b/rust/crates/spicetify/src/commands/protocol.rs index 78f4e8299f..24cbfd6af2 100644 --- a/rust/crates/spicetify/src/commands/protocol.rs +++ b/rust/crates/spicetify/src/commands/protocol.rs @@ -2,13 +2,14 @@ use std::borrow::Cow; use url::Url; +use super::apply::ApplyMode; use crate::context::AppContext; use crate::error::Result; use crate::fl; use crate::module::{self, ModulePaths, Store}; pub(crate) fn run(ctx: &AppContext, uri: &str) -> Result<()> { - let response = handle(ctx, uri)?; + let response = handle(ctx, uri, ApplyMode::Cli { no_cache: false })?; if !response.is_empty() { let outbound = format!("spotify:app:rpc:{response}"); launch_uri(&outbound)?; @@ -16,7 +17,7 @@ pub(crate) fn run(ctx: &AppContext, uri: &str) -> Result<()> { Ok(()) } -pub fn handle(ctx: &AppContext, uri: &str) -> Result { +pub fn handle(ctx: &AppContext, uri: &str, apply_mode: ApplyMode) -> Result { let u = Url::parse(uri).map_err(|_| anyhow::anyhow!(fl!("proxy-invalid-url")))?; if u.scheme() != "spicetify" { return Err(anyhow::anyhow!(fl!("unsupported-scheme"))); @@ -30,7 +31,7 @@ pub fn handle(ctx: &AppContext, uri: &str) -> Result { let prefix = format!("spicetify:{module_id}:"); let action = ProtocolAction::parse(action) .ok_or_else(|| anyhow::anyhow!(fl!("protocol-error", err = "unknown action")))?; - perform(ctx, action, &u)?; + perform(ctx, action, &u, apply_mode)?; if module_id == "0" { return Ok(String::new()); @@ -76,7 +77,12 @@ impl ProtocolAction { } } -fn perform(ctx: &AppContext, action: ProtocolAction, uri: &Url) -> Result<()> { +fn perform( + ctx: &AppContext, + action: ProtocolAction, + uri: &Url, + apply_mode: ApplyMode, +) -> Result<()> { let _guard = match action { ProtocolAction::Apply | ProtocolAction::BlockUpdates | ProtocolAction::UnblockUpdates => { None @@ -176,7 +182,7 @@ fn perform(ctx: &AppContext, action: ProtocolAction, uri: &Url) -> Result<()> { // fire-and-forget rather than waiting on a response. ProtocolAction::Apply => { let guard = super::guard::try_acquire(&ctx.config_root)?; - super::apply::run(ctx, &guard, false) + super::apply::run(ctx, &guard, apply_mode) } ProtocolAction::BlockUpdates => { let _guard = super::guard::try_acquire(&ctx.config_root)?; @@ -284,7 +290,8 @@ mod tests { for raw in ["../victim@1", "/victim@1", "module@..", "../victim@"] { let mut uri = Url::parse(&format!("spicetify:0:{action}"))?; let _ = uri.query_pairs_mut().append_pair("id", raw); - let error = handle(&ctx, uri.as_str()).expect_err("unsafe IDs must be refused"); + let error = handle(&ctx, uri.as_str(), ApplyMode::Daemon) + .expect_err("unsafe IDs must be refused"); assert!(error.to_string().contains("invalid store id"), "{action} {raw}: {error}"); assert!(!root.join("modules").exists(), "validation must precede vault mutation"); } @@ -311,7 +318,8 @@ mod tests { let version = if action == "enable" { "" } else { "1" }; let mut uri = Url::parse(&format!("spicetify:0:{action}"))?; let _ = uri.query_pairs_mut().append_pair("id", &format!("{module}@{version}")); - let error = handle(&ctx, uri.as_str()).expect_err("the Store must be protected"); + let error = handle(&ctx, uri.as_str(), ApplyMode::Daemon) + .expect_err("the Store must be protected"); assert!(error.to_string().contains("cannot be uninstalled"), "{error}"); } } @@ -319,7 +327,8 @@ mod tests { for action in ["add", "fast-install", "fast-enable"] { let mut uri = Url::parse(&format!("spicetify:0:{action}"))?; let _ = uri.query_pairs_mut().append_pair("id", &format!("{module}@1")); - let error = handle(&ctx, uri.as_str()).expect_err("unverified system install"); + let error = handle(&ctx, uri.as_str(), ApplyMode::Daemon) + .expect_err("unverified system install"); assert!(error.to_string().contains("registry-verified"), "{error}"); } } diff --git a/rust/crates/spicetify/src/hooks/version_detect.rs b/rust/crates/spicetify/src/hooks/version_detect.rs index 5b01fd8e36..403d1bfbe1 100644 --- a/rust/crates/spicetify/src/hooks/version_detect.rs +++ b/rust/crates/spicetify/src/hooks/version_detect.rs @@ -1,4 +1,5 @@ use std::path::Path; +#[cfg(not(windows))] use std::process::Command; use std::sync::LazyLock; @@ -122,7 +123,7 @@ fn detect_version(exec_path: &Path) -> Result { let ps_script = format!("(Get-Item -LiteralPath '{}').VersionInfo.ProductVersion", exec_path.display()); - let output = Command::new("powershell.exe") + let output = crate::process::background_command("powershell.exe") .args(["-NoProfile", "-NonInteractive", "-Command", &ps_script]) .output() .map_err(|e| anyhow::anyhow!("failed to run powershell: {e}"))?; diff --git a/rust/crates/spicetify/src/platform/windows.rs b/rust/crates/spicetify/src/platform/windows.rs index 295a6100c8..255bae1b49 100644 --- a/rust/crates/spicetify/src/platform/windows.rs +++ b/rust/crates/spicetify/src/platform/windows.rs @@ -1,5 +1,4 @@ use std::path::{Path, PathBuf}; -use std::process::Command; use std::sync::LazyLock; use tracing; @@ -14,9 +13,10 @@ struct SpotifyPackage { } static SPOTIFY_PACKAGE: LazyLock> = LazyLock::new(|| { - let output = match Command::new("powershell") + let output = match crate::process::background_command("powershell") .args([ "-NoProfile", + "-NonInteractive", "-Command", "$p=Get-AppxPackage -Name 'SpotifyAB.SpotifyMusic'; if($p){$p.InstallLocation; \ $p.PackageFamilyName}", diff --git a/rust/crates/spicetify/src/process.rs b/rust/crates/spicetify/src/process.rs index a769310c64..0a570f99da 100644 --- a/rust/crates/spicetify/src/process.rs +++ b/rust/crates/spicetify/src/process.rs @@ -3,6 +3,16 @@ use std::process::{Command, Stdio}; use crate::context::AppContext; use crate::error::Result; +#[cfg(windows)] +pub(crate) fn background_command(program: impl AsRef) -> Command { + use std::os::windows::process::CommandExt; + use windows::Win32::System::Threading::CREATE_NO_WINDOW; + + let mut command = Command::new(program); + let _ = command.creation_flags(CREATE_NO_WINDOW.0); + command +} + pub(crate) fn process_running(name: &str) -> bool { #[cfg(target_os = "linux")] { @@ -26,7 +36,7 @@ pub(crate) fn process_running(name: &str) -> bool { } #[cfg(windows)] { - Command::new("tasklist") + background_command("tasklist") .args(["/FI", &format!("ImageName eq {name}"), "/NH"]) .stdout(Stdio::piped()) .stderr(Stdio::null()) @@ -87,7 +97,7 @@ pub(crate) fn kill_image(name: &str) { } #[cfg(windows)] { - match Command::new("taskkill") + match background_command("taskkill") .args(["/F", "/IM", name]) .stdout(Stdio::null()) .stderr(Stdio::null()) @@ -163,7 +173,7 @@ fn spawn_windows(ctx: &AppContext) -> Result<()> { let ps_cmd = format!("& \"{}\" --app-directory=\"{}\"", appx_exe.display(), dest_apps.display()); - let child = Command::new("powershell.exe") + let child = background_command("powershell.exe") .args(["-NoProfile", "-NonInteractive", "-Command", &ps_cmd]) .stdin(Stdio::null()) .stdout(Stdio::null()) @@ -245,6 +255,22 @@ pub fn force_kill_spotify(ctx: &AppContext) { kill_image(image); } +#[cfg(all(test, windows))] +mod windows_tests { + #[test] + fn background_helpers_have_no_console_and_preserve_output_and_status() { + let output = super::background_command("powershell.exe") + .args([ + "-NoProfile", "-NonInteractive", "-Command", + r#"Add-Type 'using System; using System.Runtime.InteropServices; public class ConsoleProbe { [DllImport("kernel32.dll")] public static extern IntPtr GetConsoleWindow(); }'; [ConsoleProbe]::GetConsoleWindow().ToInt64(); exit 7"#, + ]) + .output() + .expect("run background console probe"); + assert_eq!(output.status.code(), Some(7), "child exit status is preserved: {output:?}"); + assert_eq!(String::from_utf8_lossy(&output.stdout).trim(), "0", "no console is allocated"); + } +} + #[cfg(all(test, target_os = "macos"))] mod tests { use super::macos_bundle; diff --git a/rust/crates/tui/src/app/input.rs b/rust/crates/tui/src/app/input.rs index eda760db30..4d0b65a1ff 100644 --- a/rust/crates/tui/src/app/input.rs +++ b/rust/crates/tui/src/app/input.rs @@ -167,10 +167,8 @@ impl TuiApp { self.input = None; self.run_command(cmd, &label); } - KeyCode::Backspace => { - if input.buffer.pop().is_none() { - tracing::debug!("backspace pressed with empty input buffer"); - } + KeyCode::Backspace if input.buffer.pop().is_none() => { + tracing::debug!("backspace pressed with empty input buffer"); } KeyCode::Char(c) if key.modifiers.is_empty() || key.modifiers == KeyModifiers::SHIFT => From 5794259b23c7f227bf340529d56097fe89bbe798 Mon Sep 17 00:00:00 2001 From: Afonso Jorge Ramos Date: Thu, 17 Sep 2026 01:40:46 +0200 Subject: [PATCH 2/3] fix(v3): restore GraphQL definitions and macOS app handoff (#3951) --- rust/crates/spicetify/src/platform/macos.rs | 54 +++++- .../webpack/graphql-definitions.js | 56 ++++++ .../webpack/graphql-definitions.test.mts | 161 ++++++++++++++++++ .../webpack/spicetify-bindings.js | 2 + 4 files changed, 272 insertions(+), 1 deletion(-) create mode 100644 src/jsHelper/spicetifyWrapper/webpack/graphql-definitions.js create mode 100644 src/jsHelper/spicetifyWrapper/webpack/graphql-definitions.test.mts diff --git a/rust/crates/spicetify/src/platform/macos.rs b/rust/crates/spicetify/src/platform/macos.rs index 66ad69ef36..7cffb753fd 100644 --- a/rust/crates/spicetify/src/platform/macos.rs +++ b/rust/crates/spicetify/src/platform/macos.rs @@ -129,12 +129,24 @@ fn install_protocol_handler() -> crate::error::Result { tracing::debug!(error = %e, "could not remove the temporary applescript"); } - declare_url_scheme(&bundle.join("Contents").join("Info.plist"))?; + finalize_protocol_bundle(&bundle)?; run(LSREGISTER, &["-f".as_ref(), bundle.as_os_str()])?; Ok(bundle) } +fn finalize_protocol_bundle(bundle: &Path) -> crate::error::Result<()> { + declare_url_scheme(&bundle.join("Contents").join("Info.plist"))?; + run( + "/usr/bin/codesign", + &["--force".as_ref(), "--sign".as_ref(), "-".as_ref(), bundle.as_os_str()], + )?; + run( + "/usr/bin/codesign", + &["--verify".as_ref(), "--deep".as_ref(), "--strict".as_ref(), bundle.as_os_str()], + ) +} + /// `quoted form of` is `AppleScript`'s shell escaping, so a hostile URI cannot /// break out of the command. The CLI validates the URI itself. fn applescript(exe: &Path, log: &Path) -> String { @@ -184,6 +196,46 @@ fn run(program: &str, args: &[&std::ffi::OsStr]) -> crate::error::Result<()> { mod tests { use super::*; + #[test] + fn protocol_bundle_signature_covers_the_final_url_metadata() -> crate::error::Result<()> { + let mut nonce = [0; 16]; + getrandom::fill(&mut nonce)?; + let root = std::env::temp_dir().join(format!("spicetify-protocol-{}", hex::encode(nonce))); + std::fs::create_dir(&root)?; + let result = (|| { + let source = root.join("handler.applescript"); + let bundle = root.join("Spicetify.app"); + std::fs::write( + &source, + applescript(Path::new("/usr/bin/true"), &root.join("protocol.log")), + )?; + run("/usr/bin/osacompile", &["-o".as_ref(), bundle.as_os_str(), source.as_os_str()])?; + finalize_protocol_bundle(&bundle)?; + let metadata = std::process::Command::new("/usr/bin/plutil") + .args(["-convert", "json", "-o", "-"]) + .arg(bundle.join("Contents/Info.plist")) + .output()?; + assert!(metadata.status.success()); + let plist: serde_json::Value = serde_json::from_slice(&metadata.stdout)?; + assert_eq!( + plist.get("CFBundleIdentifier").and_then(serde_json::Value::as_str), + Some(BUNDLE_ID) + ); + assert_eq!( + plist + .pointer("/CFBundleURLTypes/0/CFBundleURLSchemes/0") + .and_then(serde_json::Value::as_str), + Some("spicetify") + ); + run( + "/usr/bin/codesign", + &["--verify".as_ref(), "--deep".as_ref(), "--strict".as_ref(), bundle.as_os_str()], + ) + })(); + std::fs::remove_dir_all(&root)?; + result + } + #[test] fn resolve_bundle_prefers_an_existing_candidate() { let dir = std::env::temp_dir().join(format!("spicetify-bundle-{}", std::process::id())); diff --git a/src/jsHelper/spicetifyWrapper/webpack/graphql-definitions.js b/src/jsHelper/spicetifyWrapper/webpack/graphql-definitions.js new file mode 100644 index 0000000000..570aeed100 --- /dev/null +++ b/src/jsHelper/spicetifyWrapper/webpack/graphql-definitions.js @@ -0,0 +1,56 @@ +const DEFINITION = /\bnew\s+[\w$]+(?:\.[\w$]+)*\(\s*"([_A-Za-z][_0-9A-Za-z]*)"\s*,\s*"(query|mutation)"\s*,\s*"([a-fA-F0-9]{64})"\s*,\s*null\s*\)/g; + +export function extractGraphQLDefinitions(source) { + return Array.from(source.matchAll(DEFINITION), ([, name, operation, sha256Hash]) => Object.freeze({ name, operation, sha256Hash, value: null })); +} + +// Read metadata without requiring modules or rewriting client code. A stable +// dictionary keeps destructured references useful when later chunks arrive. +export function createGraphQLDefinitions(getFactories) { + const definitions = Object.create(null); + const parsed = new WeakMap(); + let previous = new Set(); + + function refresh() { + const factories = new Set(Object.values(getFactories() ?? {}).filter((factory) => typeof factory === "function")); + if (factories.size === previous.size && [...factories].every((factory) => previous.has(factory))) return; + previous = factories; + for (const name of Object.keys(definitions)) delete definitions[name]; + const conflicts = new Set(); + for (const factory of factories) { + if (!parsed.has(factory)) parsed.set(factory, extractGraphQLDefinitions(Function.prototype.toString.call(factory))); + for (const definition of parsed.get(factory)) { + const existing = definitions[definition.name]; + if (existing && (existing.operation !== definition.operation || existing.sha256Hash !== definition.sha256Hash)) { + conflicts.add(definition.name); + } + definitions[definition.name] = definition; + } + } + for (const name of conflicts) delete definitions[name]; + } + + return new Proxy(definitions, { + get(target, name) { + if (typeof name === "string") refresh(); + return Reflect.get(target, name); + }, + has(target, name) { + refresh(); + return Reflect.has(target, name); + }, + ownKeys(target) { + refresh(); + return Reflect.ownKeys(target); + }, + getOwnPropertyDescriptor(target, name) { + refresh(); + return Reflect.getOwnPropertyDescriptor(target, name); + }, + set: () => false, + defineProperty: () => false, + deleteProperty: () => false, + preventExtensions: () => false, + setPrototypeOf: () => false, + }); +} diff --git a/src/jsHelper/spicetifyWrapper/webpack/graphql-definitions.test.mts b/src/jsHelper/spicetifyWrapper/webpack/graphql-definitions.test.mts new file mode 100644 index 0000000000..a2e3594bad --- /dev/null +++ b/src/jsHelper/spicetifyWrapper/webpack/graphql-definitions.test.mts @@ -0,0 +1,161 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { createGraphQLDefinitions, extractGraphQLDefinitions } from "./graphql-definitions.js"; +import { createSpicetifyBindings } from "./spicetify-bindings.js"; + +declare const api: { Document: new (name: string, operation: string, hash: string, value: null) => unknown }; + +function albumFactory() { + new api.Document("getAlbum", "query", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", null); + throw new Error("discovery must not execute factories"); +} + +function searchFactory() { + return new api.Document("searchDesktop", "query", "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", null); +} + +function mutationFactory() { + return new api.Document("saveRecentSearches", "mutation", "cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc", null); +} + +test("discovers current-client definitions without executing factories", () => { + const definitions = createGraphQLDefinitions(() => ({ album: albumFactory, mutation: mutationFactory })); + assert.deepEqual(definitions.getAlbum, { + name: "getAlbum", + operation: "query", + sha256Hash: "a".repeat(64), + value: null, + }); + assert.equal(definitions.saveRecentSearches.operation, "mutation"); + assert.deepEqual(Object.keys(definitions).sort(), ["getAlbum", "saveRecentSearches"]); + assert.equal(definitions.missingOperation, undefined); +}); + +test("late chunks are discovered and replaced or removed factories do not leave stale definitions", () => { + const factories: Record unknown> = { current: albumFactory }; + const definitions = createGraphQLDefinitions(() => factories); + const album = definitions.getAlbum; + assert.ok(album); + assert.equal(definitions.getAlbum, album, "unchanged definitions keep their identity"); + factories.current = searchFactory; + assert.equal(definitions.getAlbum, undefined); + assert.equal(definitions.searchDesktop.name, "searchDesktop"); + delete factories.current; + assert.deepEqual(Object.keys(definitions), []); +}); + +test("deduplicates identical definitions but refuses conflicting operation hashes", () => { + function conflict() { + return new api.Document("getAlbum", "query", "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", null); + } + const factories: Record unknown> = { first: albumFactory, duplicate: albumFactory }; + const definitions = createGraphQLDefinitions(() => factories); + assert.equal(definitions.getAlbum.sha256Hash, "a".repeat(64)); + factories.duplicate = conflict; + assert.equal(definitions.getAlbum, undefined); + assert.equal("getAlbum" in definitions, false); + delete factories.duplicate; + assert.equal(definitions.getAlbum.sha256Hash, "a".repeat(64)); +}); + +test("own-property inspection discovers late chunks and drops removed definitions", () => { + const factories: Record unknown> = {}; + const definitions = createGraphQLDefinitions(() => factories); + assert.equal(Object.hasOwn(definitions, "getAlbum"), false); + factories.album = albumFactory; + assert.equal(Object.hasOwn(definitions, "getAlbum"), true); + assert.equal(Object.getOwnPropertyDescriptor(definitions, "getAlbum")?.value.name, "getAlbum"); + delete factories.album; + assert.equal(Object.hasOwn(definitions, "getAlbum"), false); +}); + +test("conflicting operation types are omitted even when the hashes match", () => { + function conflict() { + return new api.Document("getAlbum", "mutation", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", null); + } + const definitions = createGraphQLDefinitions(() => ({ album: albumFactory, conflict })); + assert.equal(definitions.getAlbum, undefined); +}); + +test("invalid metadata is ignored and the registry tolerates unavailable capture", () => { + function invalid() { + const badHash = new api.Document("badHash", "query", "not-a-hash", null); + const subscription = new api.Document("stream", "subscription", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", null); + return [badHash, subscription]; + } + let factories: Record unknown> | undefined; + const definitions = createGraphQLDefinitions(() => factories); + assert.deepEqual(Object.keys(definitions), []); + factories = { invalid, album: albumFactory }; + assert.deepEqual(Object.keys(definitions), ["getAlbum"]); +}); + +test("definitions and their registry cannot be modified by consumers", () => { + const definitions = createGraphQLDefinitions(() => ({ album: albumFactory })); + assert.equal(Object.getPrototypeOf(definitions), null); + assert.equal(Reflect.setPrototypeOf(definitions, { fake: {} }), false); + assert.equal(Reflect.set(definitions, "getAlbum", {}), false); + assert.throws(() => { + definitions.getAlbum = {}; + }, TypeError); + assert.equal(Reflect.defineProperty(definitions, "fake", { value: {} }), false); + assert.equal(Reflect.deleteProperty(definitions, "getAlbum"), false); + assert.equal(Reflect.preventExtensions(definitions), false); + assert.equal(Reflect.set(definitions.getAlbum, "sha256Hash", "b".repeat(64)), false); + assert.equal(definitions.getAlbum.sha256Hash, "a".repeat(64)); + assert.equal(definitions.fake, undefined); +}); + +test("recognizes the compact persisted-operation shape used by Spotify bundles", () => { + const source = 'var a=new n.Ay("getAlbum","query","aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",null);'; + assert.deepEqual(extractGraphQLDefinitions(source), [ + { + name: "getAlbum", + operation: "query", + sha256Hash: "a".repeat(64), + value: null, + }, + ]); +}); + +for (const mode of [undefined, "direct", "snapshot"]) { + test(`wrapper GraphQL binding preserves the ${mode ?? "v2"} contract`, (t) => { + const legacyDefinitions = { legacy: { name: "legacy" } }; + for (const [name, value] of Object.entries({ + Spicetify: { GraphQL: { Definitions: legacyDefinitions } }, + __SPICETIFY_CLIENT_BUNDLE_MODE__: mode, + })) { + const original = Object.getOwnPropertyDescriptor(globalThis, name); + Object.defineProperty(globalThis, name, { configurable: true, writable: true, value }); + t.after(() => { + if (original) Object.defineProperty(globalThis, name, original); + else Reflect.deleteProperty(globalThis, name); + }); + } + const require = Object.assign( + () => { + throw new Error("must not execute factories"); + }, + { m: { album: albumFactory } }, + ); + const bindings = createSpicetifyBindings({ + cache: [], + chunks: [], + modules: [], + functionModules: [], + require, + exportedMemos: [], + exportedMemoFRefs: [], + reactComponentsUI: {}, + scrollableContainer: undefined, + }); + if (mode === undefined) { + assert.equal(bindings.GraphQL.Definitions, legacyDefinitions); + assert.equal(Reflect.set(bindings.GraphQL.Definitions, "extra", {}), true); + } else { + assert.equal(bindings.GraphQL.Definitions.getAlbum.sha256Hash, "a".repeat(64)); + assert.equal(Reflect.set(bindings.GraphQL.Definitions, "extra", {}), false); + } + }); +} diff --git a/src/jsHelper/spicetifyWrapper/webpack/spicetify-bindings.js b/src/jsHelper/spicetifyWrapper/webpack/spicetify-bindings.js index 56a3c8601d..5bfd02ed71 100644 --- a/src/jsHelper/spicetifyWrapper/webpack/spicetify-bindings.js +++ b/src/jsHelper/spicetifyWrapper/webpack/spicetify-bindings.js @@ -1,4 +1,5 @@ import { fnStr } from "../shared/string.js"; +import { createGraphQLDefinitions } from "./graphql-definitions.js"; import { createReactComponents } from "./react-components.js"; import { findReactQuery } from "./react-query.js"; @@ -31,6 +32,7 @@ export function createSpicetifyBindings({ }, GraphQL: { ...Spicetify.GraphQL, + Definitions: globalThis.__SPICETIFY_CLIENT_BUNDLE_MODE__ ? createGraphQLDefinitions(() => require.m) : Spicetify.GraphQL.Definitions, get Request() { return Spicetify.Platform?.GraphQLLoader || Spicetify.GraphQL.Handler?.(Spicetify.GraphQL.Context); }, From 4870bae67428bf8bf375460601dc18bc6499f455 Mon Sep 17 00:00:00 2001 From: Afonso Jorge Ramos Date: Thu, 17 Sep 2026 01:41:32 +0200 Subject: [PATCH 3/3] fix(daemon): preserve macOS launchd ownership during startup (#3952) --- rust/crates/spicetify/src/commands/apply.rs | 115 +++++- rust/crates/spicetify/src/daemon/manager.rs | 408 +++++++++++++++++++- 2 files changed, 499 insertions(+), 24 deletions(-) diff --git a/rust/crates/spicetify/src/commands/apply.rs b/rust/crates/spicetify/src/commands/apply.rs index 59c83bf9c2..e487fa146e 100644 --- a/rust/crates/spicetify/src/commands/apply.rs +++ b/rust/crates/spicetify/src/commands/apply.rs @@ -271,17 +271,41 @@ pub(crate) fn ensure_daemon(ctx: &AppContext) { // Registers auto-start with this CLI's own daemon binary. This runs after // any stop because stop unregisters; registering first left every // upgrade with the registration undone and the daemon back unsupervised. - if let Err(e) = super::daemon::install() { - tracing::warn!(error = %e, "could not enable the daemon at login"); + finish_daemon_startup( + super::daemon::install(), + || daemon_comes_up(std::time::Duration::from_secs(2)), + super::daemon::start, + ); +} + +fn finish_daemon_startup( + installation: Result<()>, + is_ready: impl FnOnce() -> bool, + start: impl FnOnce() -> Result<()>, +) { + let launchd_owns_startup = cfg!(target_os = "macos") && installation.is_ok(); + if let Err(error) = installation { + tracing::warn!(%error, "could not enable the daemon at login"); + if error + .downcast_ref::() + .is_some_and(|error| !error.allows_unmanaged_fallback()) + { + tracing::warn!("daemon ownership is unresolved; skipping unmanaged startup"); + return; + } } - // A supervisor that starts what it registers (systemd, launchd) has the - // daemon up by now or within a moment; only spawn when nothing answers, - // so there is never a second, unsupervised copy beside the managed one. - if !daemon_comes_up(std::time::Duration::from_secs(2)) - && let Err(e) = super::daemon::start() - { - tracing::warn!(error = %e, "could not start the daemon"); + if is_ready() { + return; + } + if launchd_owns_startup { + tracing::warn!( + "launchd registered the daemon but startup is still pending; skipping unmanaged startup" + ); + return; + } + if let Err(error) = start() { + tracing::warn!(%error, "could not start the daemon"); } } @@ -934,3 +958,76 @@ mod tests { std::fs::remove_dir_all(root).expect("cleanup runtime fixture"); } } + +#[cfg(test)] +mod daemon_startup_tests { + use super::finish_daemon_startup; + use crate::daemon::DaemonManagerError; + use std::cell::Cell; + + #[test] + fn unresolved_supervisor_or_shutdown_never_starts_an_unmanaged_daemon() { + for error in [ + DaemonManagerError::Launchctl("cannot query launchd".to_owned()), + DaemonManagerError::ShutdownIncomplete("instance lock is still held".to_owned()), + ] { + let checked_ready = Cell::new(false); + let spawned = Cell::new(false); + finish_daemon_startup( + Err(error.into()), + || { + checked_ready.set(true); + false + }, + || { + spawned.set(true); + Ok(()) + }, + ); + assert!(!checked_ready.get()); + assert!(!spawned.get()); + } + } + + #[test] + fn unsupported_supervisor_still_allows_unmanaged_startup() { + let spawned = Cell::new(false); + finish_daemon_startup( + Err(DaemonManagerError::Unsupported.into()), + || false, + || { + spawned.set(true); + Ok(()) + }, + ); + assert!(spawned.get()); + } + + #[test] + fn delayed_registered_supervisor_keeps_startup_ownership_on_macos() { + let spawned = Cell::new(false); + finish_daemon_startup( + Ok(()), + || false, + || { + spawned.set(true); + Ok(()) + }, + ); + assert_eq!(spawned.get(), !cfg!(target_os = "macos")); + } + + #[test] + fn ready_supervised_daemon_does_not_start_another_process() { + let spawned = Cell::new(false); + finish_daemon_startup( + Ok(()), + || true, + || { + spawned.set(true); + Ok(()) + }, + ); + assert!(!spawned.get()); + } +} diff --git a/rust/crates/spicetify/src/daemon/manager.rs b/rust/crates/spicetify/src/daemon/manager.rs index d54ca23063..258ebce1a4 100644 --- a/rust/crates/spicetify/src/daemon/manager.rs +++ b/rust/crates/spicetify/src/daemon/manager.rs @@ -1,5 +1,5 @@ use std::path::PathBuf; -#[cfg(target_os = "linux")] +#[cfg(any(target_os = "linux", target_os = "macos"))] use std::time::Duration; use thiserror::Error; @@ -15,10 +15,22 @@ pub enum DaemonManagerError { #[error("systemctl error: {0}")] Systemctl(String), + #[error("launchctl error: {0}")] + Launchctl(String), + + #[error("daemon shutdown is incomplete: {0}")] + ShutdownIncomplete(String), + #[error("failed to spawn daemon: {0}")] Spawn(#[from] super::process::DaemonSpawnError), } +impl DaemonManagerError { + pub(crate) fn allows_unmanaged_fallback(&self) -> bool { + !matches!(self, Self::Launchctl(_) | Self::ShutdownIncomplete(_)) + } +} + #[derive(Debug, Clone, Copy)] pub enum DaemonManager { #[cfg(windows)] @@ -168,14 +180,54 @@ fn registry_err(e: impl std::fmt::Display) -> DaemonManagerError { #[cfg(target_os = "macos")] #[derive(Debug, Clone, Copy)] pub struct MacosDaemonManager; +#[cfg(target_os = "macos")] +const LAUNCH_AGENT_LABEL: &str = "app.spicetify.daemon"; + +#[cfg(target_os = "macos")] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum LaunchAgentInstallAction { + Noop, + Load, + Reload, +} + +#[cfg(target_os = "macos")] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum LaunchAgentState { + Unloaded, + LoadedStopped, + Running, +} + +#[cfg(target_os = "macos")] +fn launch_agent_install_action( + existing_plist: Option<&str>, + desired_plist: &str, + state: LaunchAgentState, +) -> LaunchAgentInstallAction { + if state == LaunchAgentState::Running && existing_plist == Some(desired_plist) { + LaunchAgentInstallAction::Noop + } else if state == LaunchAgentState::Unloaded { + LaunchAgentInstallAction::Load + } else { + LaunchAgentInstallAction::Reload + } +} + +#[cfg(target_os = "macos")] +fn launch_agent_path(home: &std::path::Path) -> PathBuf { + home.join("Library/LaunchAgents").join(format!("{LAUNCH_AGENT_LABEL}.plist")) +} + #[cfg(target_os = "macos")] impl MacosDaemonManager { fn install() -> Result<(), DaemonManagerError> { - let plist_dir = home_dir()?.join("Library/LaunchAgents"); + let home = home_dir()?; + let plist_dir = home.join("Library/LaunchAgents"); std::fs::create_dir_all(&plist_dir)?; let exe = current_exe()?; let daemon_exe = super::daemon_binary_for(&exe); - let plist_path = plist_dir.join("app.spicetify.daemon.plist"); + let plist_path = launch_agent_path(&home); let plist = format!( r#" @@ -183,7 +235,7 @@ impl MacosDaemonManager { Label - app.spicetify.daemon + {} ProgramArguments {} @@ -194,18 +246,41 @@ impl MacosDaemonManager { "#, + LAUNCH_AGENT_LABEL, xml_escape(&daemon_exe.display().to_string()) ); - std::fs::write(&plist_path, plist)?; - run_launchctl(&["load", "-w"], &plist_path); - Ok(()) + let existing_plist = std::fs::read_to_string(&plist_path).ok(); + let plist_changed = existing_plist.as_deref() != Some(&plist); + let status = launch_agent_status()?; + let action = launch_agent_install_action(existing_plist.as_deref(), &plist, status); + + match action { + LaunchAgentInstallAction::Noop => Ok(()), + LaunchAgentInstallAction::Load => { + stop_unmanaged_daemon()?; + if plist_changed { + std::fs::write(&plist_path, &plist)?; + } + run_launchctl(&["load", "-w"], Some(&plist_path)).map(|_| ()) + } + LaunchAgentInstallAction::Reload => { + let _ = run_launchctl(&["remove", LAUNCH_AGENT_LABEL], None)?; + stop_unmanaged_daemon()?; + if plist_changed { + std::fs::write(&plist_path, &plist)?; + } + run_launchctl(&["load", "-w"], Some(&plist_path)).map(|_| ()) + } + } } fn uninstall() { if let Ok(home) = home_dir() { - let plist_path = home.join("Library/LaunchAgents/app.spicetify.daemon.plist"); + let plist_path = launch_agent_path(&home); if plist_path.exists() { - run_launchctl(&["unload", "-w"], &plist_path); + if let Err(e) = run_launchctl(&["unload", "-w"], Some(&plist_path)) { + tracing::warn!(error = %e, "failed to unload daemon auto-start"); + } if let Err(e) = std::fs::remove_file(&plist_path) && e.kind() != std::io::ErrorKind::NotFound { @@ -216,7 +291,7 @@ impl MacosDaemonManager { } fn is_installed() -> bool { - home_dir().is_ok_and(|h| h.join("Library/LaunchAgents/app.spicetify.daemon.plist").exists()) + home_dir().is_ok_and(|home| launch_agent_path(&home).exists()) } } @@ -308,12 +383,133 @@ fn home_dir() -> Result { } #[cfg(target_os = "macos")] -fn run_launchctl(args: &[&str], plist: &std::path::Path) { - match std::process::Command::new("launchctl").args(args).arg(plist).status() { - Ok(s) if !s.success() => tracing::warn!("launchctl exited with {s}"), - Err(e) => tracing::warn!(error = %e, "failed to run launchctl"), - _ => {} +fn launch_agent_status() -> Result { + let output = run_launchctl(&["list"], None)?; + Ok(parse_launch_agent_status(&String::from_utf8_lossy(&output))) +} + +#[cfg(target_os = "macos")] +fn parse_launch_agent_status(list: &str) -> LaunchAgentState { + for line in list.lines() { + let mut fields = line.split_whitespace(); + let pid = fields.next(); + let _ = fields.next(); + if fields.next() == Some(LAUNCH_AGENT_LABEL) { + return if pid.is_some_and(|pid| pid.parse::().is_ok_and(|pid| pid > 0)) { + LaunchAgentState::Running + } else { + LaunchAgentState::LoadedStopped + }; + } + } + LaunchAgentState::Unloaded +} + +#[cfg(target_os = "macos")] +fn stop_unmanaged_daemon() -> Result<(), DaemonManagerError> { + stop_unmanaged_daemon_and_wait() + .map_err(|error| DaemonManagerError::ShutdownIncomplete(error.to_string())) +} + +#[cfg(target_os = "macos")] +fn stop_unmanaged_daemon_and_wait() -> Result<(), DaemonManagerError> { + let path = crate::platform::default_spicetify_config_dir().join("spicetify-daemon.lock"); + // Keep the original inode open: the daemon unlinks this path before dropping its lock. + let lock_file = match std::fs::File::open(path) { + Ok(file) => Some(file), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => None, + Err(error) => return Err(error.into()), + }; + if super::is_daemon_running() || daemon_process_running()? { + super::shutdown_daemon(); + } + wait_for_daemon_exit(lock_file.as_ref(), Duration::from_secs(5), daemon_process_running) +} + +#[cfg(target_os = "macos")] +fn daemon_process_running() -> Result { + let status = std::process::Command::new("pgrep") + .args(["-x", super::daemon_binary_name()]) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status()?; + match status.code() { + Some(0) => Ok(true), + Some(1) => Ok(false), + _ => { + Err(std::io::Error::other(format!("failed to inspect daemon process: {status}")).into()) + } + } +} + +#[cfg(target_os = "macos")] +fn wait_for_daemon_exit( + lock_file: Option<&std::fs::File>, + timeout: Duration, + mut process_running: impl FnMut() -> Result, +) -> Result<(), DaemonManagerError> { + let deadline = std::time::Instant::now() + timeout; + loop { + let lock_released = if let Some(file) = lock_file { + match fs4::FileExt::try_lock(file) { + Ok(()) => { + fs4::FileExt::unlock(file)?; + true + } + Err(fs4::TryLockError::WouldBlock) => false, + Err(fs4::TryLockError::Error(error)) => return Err(error.into()), + } + } else { + true + }; + if !process_running()? && lock_released { + return Ok(()); + } + let remaining = deadline.saturating_duration_since(std::time::Instant::now()); + if remaining.is_zero() { + return Err(std::io::Error::new( + std::io::ErrorKind::TimedOut, + "daemon did not exit before launch-agent startup; auto-start was not loaded", + ) + .into()); + } + std::thread::sleep(remaining.min(Duration::from_millis(50))); + } +} + +#[cfg(target_os = "macos")] +fn run_launchctl( + args: &[&str], + plist: Option<&std::path::Path>, +) -> Result, DaemonManagerError> { + let mut command = std::process::Command::new("launchctl"); + let _ = command.args(args); + if let Some(plist) = plist { + let _ = command.arg(plist); + } + + let output = command + .output() + .map_err(|error| DaemonManagerError::Launchctl(format!("{}: {error}", args.join(" "))))?; + launchctl_output(args, output) +} + +#[cfg(target_os = "macos")] +fn launchctl_output( + args: &[&str], + output: std::process::Output, +) -> Result, DaemonManagerError> { + if output.status.success() { + return Ok(output.stdout); } + + let stderr = String::from_utf8_lossy(&output.stderr).trim().to_owned(); + let detail = if stderr.is_empty() { + format!("{} exited with {}", args.join(" "), output.status) + } else { + format!("{}: {stderr}", args.join(" ")) + }; + Err(DaemonManagerError::Launchctl(detail)) } #[cfg(target_os = "linux")] @@ -365,3 +561,185 @@ fn run_systemctl(args: &[&str]) -> Result<(), DaemonManagerError> { } } } + +#[cfg(all(test, target_os = "macos"))] +mod tests { + use super::{ + DaemonManagerError, LaunchAgentInstallAction, LaunchAgentState, + launch_agent_install_action, launchctl_output, parse_launch_agent_status, + wait_for_daemon_exit, + }; + use std::os::unix::process::ExitStatusExt; + + #[test] + fn an_unlinked_instance_lock_still_blocks_loading_after_the_listener_closes() + -> anyhow::Result<()> { + let path = std::env::temp_dir().join(format!("spicetify-exit-lock-{}", std::process::id())); + let owner = std::fs::File::create(&path)?; + fs4::FileExt::try_lock(&owner)?; + let observer = std::fs::File::open(&path)?; + std::fs::remove_file(&path)?; + let result = wait_for_daemon_exit(Some(&observer), std::time::Duration::ZERO, || Ok(false)); + assert!( + matches!(result, Err(DaemonManagerError::Io(error)) if error.kind() == std::io::ErrorKind::TimedOut) + ); + drop(owner); + wait_for_daemon_exit(Some(&observer), std::time::Duration::ZERO, || Ok(false))?; + fs4::FileExt::try_lock(&observer)?; + fs4::FileExt::unlock(&observer)?; + Ok(()) + } + + #[test] + fn a_process_that_has_not_exited_blocks_loading_without_a_lock_file() { + let result = wait_for_daemon_exit(None, std::time::Duration::ZERO, || Ok(true)); + assert!( + matches!(result, Err(DaemonManagerError::Io(error)) if error.kind() == std::io::ErrorKind::TimedOut) + ); + } + + #[test] + fn daemon_exit_waits_until_the_process_is_gone() -> anyhow::Result<()> { + let mut probes = 0; + wait_for_daemon_exit(None, std::time::Duration::from_secs(1), || { + probes += 1; + Ok(probes < 2) + })?; + assert_eq!(probes, 2); + Ok(()) + } + + #[test] + fn process_inspection_failure_does_not_allow_loading() { + let result = wait_for_daemon_exit(None, std::time::Duration::ZERO, || { + Err(std::io::Error::from(std::io::ErrorKind::PermissionDenied).into()) + }); + assert!( + matches!(result, Err(DaemonManagerError::Io(error)) if error.kind() == std::io::ErrorKind::PermissionDenied) + ); + } + + #[test] + fn launch_agent_list_distinguishes_running_stopped_and_missing_jobs() { + assert_eq!( + parse_launch_agent_status("PID\tStatus\tLabel\n123\t0\tapp.spicetify.daemon\n"), + LaunchAgentState::Running + ); + assert_eq!( + parse_launch_agent_status("PID\tStatus\tLabel\n-\t1\tapp.spicetify.daemon\n"), + LaunchAgentState::LoadedStopped + ); + assert_eq!( + parse_launch_agent_status("PID\tStatus\tLabel\n123\t0\tapp.spicetify.daemon.other\n"), + LaunchAgentState::Unloaded + ); + } + + #[test] + fn launchctl_failures_report_command_and_stderr() { + let error = launchctl_output( + &["load", "-w"], + std::process::Output { + status: std::process::ExitStatus::from_raw(5 << 8), + stdout: Vec::new(), + stderr: b"Input/output error\n".to_vec(), + }, + ) + .expect_err("failed launchctl must not report success"); + assert!(matches!(error, DaemonManagerError::Launchctl(_))); + assert!(error.to_string().contains("load -w: Input/output error")); + } + + #[test] + fn launchctl_list_failure_is_not_an_unloaded_job() { + let error = launchctl_output( + &["list"], + std::process::Output { + status: std::process::ExitStatus::from_raw(1 << 8), + stdout: Vec::new(), + stderr: Vec::new(), + }, + ) + .expect_err("an unavailable launchd must not trigger daemon replacement"); + assert!(error.to_string().contains("list exited with")); + } + + #[test] + fn successful_launchctl_returns_the_job_list() { + let list = b"PID\tStatus\tLabel\n123\t0\tapp.spicetify.daemon\n"; + let output = launchctl_output( + &["list"], + std::process::Output { + status: std::process::ExitStatus::from_raw(0), + stdout: list.to_vec(), + stderr: Vec::new(), + }, + ) + .expect("launchctl succeeded"); + assert_eq!(output, list); + } + + #[test] + fn missing_plist_reloads_an_existing_registration() { + assert_eq!( + launch_agent_install_action(None, "desired plist", LaunchAgentState::Running), + LaunchAgentInstallAction::Reload + ); + } + + #[test] + fn first_install_loads_the_launch_agent() { + assert_eq!( + launch_agent_install_action(None, "desired plist", LaunchAgentState::Unloaded), + LaunchAgentInstallAction::Load + ); + } + + #[test] + fn unchanged_loaded_launch_agent_is_not_loaded_twice() { + assert_eq!( + launch_agent_install_action( + Some("desired plist"), + "desired plist", + LaunchAgentState::Running, + ), + LaunchAgentInstallAction::Noop + ); + } + + #[test] + fn unchanged_loaded_launch_agent_replaces_an_unmanaged_daemon() { + assert_eq!( + launch_agent_install_action( + Some("desired plist"), + "desired plist", + LaunchAgentState::LoadedStopped, + ), + LaunchAgentInstallAction::Reload + ); + } + + #[test] + fn unloaded_launch_agent_is_loaded_even_when_plist_is_unchanged() { + assert_eq!( + launch_agent_install_action( + Some("desired plist"), + "desired plist", + LaunchAgentState::Unloaded, + ), + LaunchAgentInstallAction::Load + ); + } + + #[test] + fn changed_loaded_launch_agent_is_reloaded() { + assert_eq!( + launch_agent_install_action( + Some("old plist"), + "desired plist", + LaunchAgentState::Running, + ), + LaunchAgentInstallAction::Reload + ); + } +}