From c78d28cd1759c8a72ddb3f6f96dc31c8c3232739 Mon Sep 17 00:00:00 2001 From: Thales <> Date: Mon, 24 Aug 2026 21:45:33 +0100 Subject: [PATCH] A second StemDeck no longer adopts the first one's backend Launching a second StemDeck while one was running gave the new window the already-running instance's backend, and with it that instance's data directory and library. Nothing on screen suggested anything was wrong. Two independent faults had to line up, so both are fixed. The port reservation probed 127.0.0.1 while the backend binds 0.0.0.0. On Windows those do not collide, so an occupied port looked free, the fallback to another port never ran, and the backend we spawned died on bind with 10048. The reservation now claims the same address the backend will bind, so a taken port reads as taken. It claims it without listening. bind is what reserves an address; listen is what makes a program a server, and a server on 0.0.0.0 is what makes Windows Firewall interrupt the user. The shell should not be answering that prompt on the backend's behalf, so the reservation binds only. The health check accepted any 200 on the port. That is what turned a dead child into a silent adoption: the other instance answered instantly while ours was still starting. /api/health now reports the answering process, and only the child we spawned is accepted. The child is watched while polling too, so a backend that cannot bind fails in a second with a message naming the contended port, rather than after ninety with a stack trace. Verified by removing the identity check and confirming the scenario test fails without it. --- app/main.py | 9 + desktop/src-tauri/Cargo.lock | 1 + desktop/src-tauri/Cargo.toml | 4 + desktop/src-tauri/src/main.rs | 367 ++++++++++++++++++++++++++++++---- tests/test_health_api.py | 14 ++ 5 files changed, 353 insertions(+), 42 deletions(-) diff --git a/app/main.py b/app/main.py index 7c2043b9..784b0228 100644 --- a/app/main.py +++ b/app/main.py @@ -289,6 +289,15 @@ def health() -> dict[str, object]: "ffmpeg_configured": FFMPEG_BIN.is_file(), "demucs_model": DEMUCS_MODEL, "demucs_device": get_demucs_device(), + # Which process is answering. The desktop shell spawns this backend and + # then polls this endpoint to know it came up -- but a 200 alone only + # proves *something* is listening on that port, not that it is the child + # the shell just started. When a second StemDeck was launched, the new + # window adopted the already-running instance's backend, and with it + # that instance's data directory and library (#424). The shell compares + # this against the PID it spawned, so a stranger on the port is refused + # rather than silently trusted. + "pid": os.getpid(), } diff --git a/desktop/src-tauri/Cargo.lock b/desktop/src-tauri/Cargo.lock index ce197bd0..faa8524d 100644 --- a/desktop/src-tauri/Cargo.lock +++ b/desktop/src-tauri/Cargo.lock @@ -3212,6 +3212,7 @@ dependencies = [ "serde", "serde_json", "sha2", + "socket2", "tar", "tauri", "tauri-build", diff --git a/desktop/src-tauri/Cargo.toml b/desktop/src-tauri/Cargo.toml index c18b23e2..002d64bb 100644 --- a/desktop/src-tauri/Cargo.toml +++ b/desktop/src-tauri/Cargo.toml @@ -17,6 +17,10 @@ reqwest = { version = "0.12", default-features = false, features = ["rustls-tls" serde = { version = "1", features = ["derive"] } serde_json = "1" sha2 = "0.10" +# Binding a port without listening on it. std's TcpListener always listens, and +# a listener on 0.0.0.0 makes StemDeck.exe itself a server in the eyes of +# Windows Firewall, prompting the user. Reserving a port is not serving on it. +socket2 = "0.6" tar = "0.4" tauri = { version = "2", features = [] } tauri-plugin-dialog = "2" diff --git a/desktop/src-tauri/src/main.rs b/desktop/src-tauri/src/main.rs index 27606381..b09d79ee 100644 --- a/desktop/src-tauri/src/main.rs +++ b/desktop/src-tauri/src/main.rs @@ -1,11 +1,12 @@ use flate2::read::GzDecoder; use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; +use socket2::{Domain, Protocol, Socket, Type}; use std::{ collections::HashMap, env, fs, io::{Read, Write}, - net::{TcpListener, TcpStream}, + net::{SocketAddr, TcpStream}, path::{Path, PathBuf}, process::{Child, Command, Output, Stdio}, sync::Mutex, @@ -1485,7 +1486,7 @@ fn start_backend( "Python runtime not found. Expected python/ or .venv/ under StemDeck.".to_string() })?; patch_pyvenv_cfg(&python); - let (port, port_guard) = reserve_port(configured_port())?; + let (port, port_guard) = reserve_port(bind_host, configured_port())?; let url = format!("http://127.0.0.1:{port}"); let log_path = data_dir.join("logs").join("backend.log"); let (stdout, stderr) = prepare_backend_stdio(&log_path).unwrap_or_else(|_| { @@ -1574,7 +1575,7 @@ fn start_backend( // Release the reserved port immediately after spawn so uvicorn can bind it. drop(port_guard); - if let Err(err) = wait_for_health(port, Duration::from_secs(90), &log_path) { + if let Err(err) = wait_for_health(&mut child, port, Duration::from_secs(90), &log_path) { let _ = child.kill(); let _ = child.wait(); return Err(err); @@ -3324,16 +3325,42 @@ fn ffmpeg_dir_if_present(data_dir: &Path) -> Option { path.parent().map(Path::to_path_buf) } -/// Bind to port 0 and return both the chosen port and the live listener. -/// Caller must hold the listener until just after the child process is -/// spawned, then drop it so the child can bind the same port. Holding the -/// socket until spawn narrows the TOCTOU window to a single OS context -/// switch rather than the entire command-setup period. -fn free_port() -> Result<(u16, TcpListener), String> { - let listener = - TcpListener::bind("127.0.0.1:0").map_err(|e| format!("port bind failed: {e}"))?; - let port = listener.local_addr().map_err(|e| e.to_string())?.port(); - Ok((port, listener)) +/// Claim `host:port` without serving on it, and report the port that was +/// actually granted (`port` of 0 asks the OS to choose). +/// +/// Bound but never listening, on purpose. `bind` is what reserves the address, +/// which is all this needs to do; `listen` is what makes a program a server, +/// and a server on `0.0.0.0` is what makes Windows Firewall interrupt the user. +/// The backend is the thing that should be answering that prompt, not the shell +/// that starts it. +fn claim_port(host: &str, port: u16) -> Result<(u16, Socket), String> { + let addr: SocketAddr = format!("{host}:{port}") + .parse() + .map_err(|e| format!("bad bind address {host}:{port}: {e}"))?; + let socket = Socket::new(Domain::for_address(addr), Type::STREAM, Some(Protocol::TCP)) + .map_err(|e| format!("socket failed: {e}"))?; + socket + .bind(&addr.into()) + .map_err(|e| format!("port bind failed: {e}"))?; + let granted = socket + .local_addr() + .map_err(|e| e.to_string())? + .as_socket() + .ok_or_else(|| "bound socket has no address".to_string())? + .port(); + Ok((granted, socket)) +} + +/// Bind to port 0 and return both the chosen port and the held reservation. +/// Caller must hold it until just after the child process is spawned, then drop +/// it so the child can bind the same port. Holding the socket until spawn +/// narrows the TOCTOU window to a single OS context switch rather than the +/// entire command-setup period. +/// +/// `host` must be the address the backend itself will bind. Probing a +/// different one proves nothing: see [`reserve_port`]. +fn free_port(host: &str) -> Result<(u16, Socket), String> { + claim_port(host, 0) } /// The user's preferred port (Settings -> port), read from the backend's @@ -3357,40 +3384,70 @@ fn configured_port() -> u16 { /// Reserve the user's preferred port; fall back to any free port if it's taken, /// so a port conflict can never block startup. -fn reserve_port(desired: u16) -> Result<(u16, TcpListener), String> { - if let Ok(listener) = TcpListener::bind(("127.0.0.1", desired)) { - let port = listener.local_addr().map_err(|e| e.to_string())?.port(); - return Ok((port, listener)); +/// +/// `host` must be the address the backend will bind (`0.0.0.0`), not loopback. +/// The two are not interchangeable: on Windows, binding `127.0.0.1:8000` +/// succeeds even while another process holds `0.0.0.0:8000`, because neither +/// socket sets `SO_EXCLUSIVEADDRUSE`. Probing loopback therefore reported a +/// taken port as free, the fallback below never ran, and the backend we spawned +/// died with `10048` while the *other* instance kept answering on that port +/// (#424). +fn reserve_port(host: &str, desired: u16) -> Result<(u16, Socket), String> { + if let Ok(claimed) = claim_port(host, desired) { + return Ok(claimed); } - free_port() + free_port(host) } -fn wait_for_health(port: u16, timeout: Duration, log_path: &Path) -> Result<(), String> { +/// Wait until *our own* backend answers on `port`. +/// +/// Identity matters as much as liveness here. A 200 only proves something is +/// listening; before #424 that was enough, so a second StemDeck launched while +/// one was already running would adopt the first instance's backend, and with +/// it the first instance's data directory and library, with nothing on screen +/// to suggest anything was wrong. The health payload carries the answering +/// process's PID, and only the child we just spawned is accepted. +/// +/// Watching the child also turns the common failure into a fast, clear one: a +/// backend that cannot bind its port exits within a second or so, and there is +/// no reason to keep polling for ninety. +fn wait_for_health( + child: &mut Child, + port: u16, + timeout: Duration, + log_path: &Path, +) -> Result<(), String> { let deadline = Instant::now() + timeout; let mut interval = Duration::from_millis(250); + let expected_pid = child.id(); + let mut foreign_pid: Option = None; loop { + // Checked before the deadline so a child that died is always reported + // as a death rather than as a timeout. + if let Ok(Some(status)) = child.try_wait() { + return Err(format!( + "The backend stopped during startup ({}).{}\n\n{}", + status, + port_conflict_hint(port, foreign_pid), + log_hint(log_path) + )); + } if Instant::now() >= deadline { - let tail = file_tail(log_path, 30); - let hint = if tail.trim().is_empty() { - format!( - "No backend log output was captured at {}.", - log_path.display() - ) - } else { - format!( - "Last backend log lines from {}:\n{}", - log_path.display(), - tail - ) - }; return Err(format!( - "backend did not become healthy within {} seconds.\n\n{}", + "backend did not become healthy within {} seconds.{}\n\n{}", timeout.as_secs(), - hint + port_conflict_hint(port, foreign_pid), + log_hint(log_path) )); } - if health_once(port).is_ok() { - return Ok(()); + match health_once(port) { + Ok(pid) if pid == expected_pid => return Ok(()), + // Something is listening, but it is not the process we started. + // Keep waiting rather than failing outright: our child is still + // alive, and if it never gets the port it will exit and be caught + // above. What must never happen is returning Ok for this. + Ok(pid) => foreign_pid = Some(pid), + Err(_) => {} } thread::sleep(interval); // Exponential backoff capped at 2 s to reduce busy-polling while @@ -3399,6 +3456,35 @@ fn wait_for_health(port: u16, timeout: Duration, log_path: &Path) -> Result<(), } } +/// Names the real problem when another program holds the port, instead of +/// leaving the user to infer it from a stack trace in the log tail. +fn port_conflict_hint(port: u16, foreign_pid: Option) -> String { + match foreign_pid { + Some(pid) => format!( + "\n\nAnother program is already using port {port} (process {pid}). \ + If that is a second copy of StemDeck, close it and try again, or \ + change the port in Settings." + ), + None => String::new(), + } +} + +fn log_hint(log_path: &Path) -> String { + let tail = file_tail(log_path, 30); + if tail.trim().is_empty() { + format!( + "No backend log output was captured at {}.", + log_path.display() + ) + } else { + format!( + "Last backend log lines from {}:\n{}", + log_path.display(), + tail + ) + } +} + fn file_tail(path: &Path, max_lines: usize) -> String { fs::read_to_string(path) .map(|text| { @@ -3408,7 +3494,9 @@ fn file_tail(path: &Path, max_lines: usize) -> String { .unwrap_or_default() } -fn health_once(port: u16) -> Result<(), String> { +/// Returns the PID the backend reports for itself, so the caller can tell our +/// own child apart from any other process that happens to hold the port. +fn health_once(port: u16) -> Result { let mut stream = TcpStream::connect(("127.0.0.1", port)).map_err(|e| e.to_string())?; stream .set_read_timeout(Some(Duration::from_secs(2))) @@ -3420,11 +3508,25 @@ fn health_once(port: u16) -> Result<(), String> { stream .read_to_string(&mut response) .map_err(|e| e.to_string())?; - if response.starts_with("HTTP/1.1 200") || response.starts_with("HTTP/1.0 200") { - Ok(()) - } else { - Err("health endpoint did not return 200".to_string()) + if !(response.starts_with("HTTP/1.1 200") || response.starts_with("HTTP/1.0 200")) { + return Err("health endpoint did not return 200".to_string()); } + parse_health_pid(&response).ok_or_else(|| "health response carried no pid".to_string()) +} + +/// Pull `"pid"` out of a raw HTTP response. Deliberately parses only the JSON +/// body: the headers are not JSON, and a `pid` appearing there (or in a header +/// value) must not be mistaken for the backend's own. +fn parse_health_pid(response: &str) -> Option { + let body = response + .split_once("\r\n\r\n") + .map(|(_, body)| body) + .or_else(|| response.split_once("\n\n").map(|(_, body)| body))?; + let start = body.find('{')?; + let json: serde_json::Value = serde_json::from_str(body[start..].trim()).ok()?; + json.get("pid")? + .as_u64() + .and_then(|p| u32::try_from(p).ok()) } fn ensure_ffmpeg(data_dir: &Path) -> Result { @@ -4182,7 +4284,9 @@ mod tests { #[cfg(target_os = "macos")] use std::env; use std::fs; - use std::path::PathBuf; + use std::path::{Path, PathBuf}; + use std::process::{Child, Command, Stdio}; + use std::time::Duration; use tempfile::TempDir; fn make_tmp() -> TempDir { @@ -5013,4 +5117,183 @@ b6052160df96b31c9b1e33854a4dcda3d4b57641b880270f31736fb9f445d384 ffmpeg-n7.1-la assert!(super::validate_download_url("file:///etc/passwd").is_err()); assert!(super::validate_download_url("not a url").is_err()); } + + // #424: a second StemDeck adopted the first one's backend, and with it the + // first one's library. Both halves of that are pinned below. + + #[test] + fn a_taken_port_is_reported_as_taken() { + // The bug: the reservation probed 127.0.0.1 while the backend binds + // 0.0.0.0. On Windows those do not collide, so an occupied port looked + // free, the fallback never ran, and the spawned backend died on bind + // while the other instance kept answering. + let held = std::net::TcpListener::bind(("0.0.0.0", 0)).unwrap(); + let taken = held.local_addr().unwrap().port(); + + let (got, _guard) = super::reserve_port("0.0.0.0", taken).unwrap(); + + assert_ne!( + got, taken, + "handed back a port another socket already holds" + ); + } + + #[test] + fn a_free_port_is_granted_as_asked() { + // The fallback must not fire needlessly: the user's configured port is + // honoured whenever it genuinely is available. + let probe = std::net::TcpListener::bind(("0.0.0.0", 0)).unwrap(); + let wanted = probe.local_addr().unwrap().port(); + drop(probe); + + let (got, _guard) = super::reserve_port("0.0.0.0", wanted).unwrap(); + + assert_eq!(got, wanted); + } + + #[test] + fn a_held_reservation_keeps_everyone_else_out() { + // The reservation binds without listening, so that StemDeck.exe is not + // a server in the firewall's eyes. That only works if bind alone still + // holds the address against a real listener -- if it did not, the port + // could be stolen between reserving it and the backend binding it. + let (port, _guard) = super::free_port("0.0.0.0").unwrap(); + assert!( + std::net::TcpListener::bind(("0.0.0.0", port)).is_err(), + "a bound reservation did not hold port {port}" + ); + } + + #[test] + fn reserved_port_is_usable_by_the_backend_after_release() { + // The guard exists so nothing steals the port between reserving and + // spawning; dropping it must leave the port bindable, or every start + // would fail. + let (port, guard) = super::free_port("0.0.0.0").unwrap(); + drop(guard); + assert!(std::net::TcpListener::bind(("0.0.0.0", port)).is_ok()); + } + + #[test] + fn health_pid_comes_from_the_body_only() { + let ok = "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\n\r\n\ + {\"name\":\"StemDeck\",\"status\":\"ok\",\"pid\":4242}"; + assert_eq!(super::parse_health_pid(ok), Some(4242)); + + // A header must never be mistaken for the payload, or a stranger could + // claim to be our child just by setting one. + let header_only = "HTTP/1.1 200 OK\r\nX-Pid: 4242\r\n\r\n{\"status\":\"ok\"}"; + assert_eq!(super::parse_health_pid(header_only), None); + + // An older backend that does not report a pid cannot be verified, so it + // must not be accepted as ours. + let no_pid = "HTTP/1.1 200 OK\r\n\r\n{\"name\":\"StemDeck\",\"status\":\"ok\"}"; + assert_eq!(super::parse_health_pid(no_pid), None); + + assert_eq!( + super::parse_health_pid("HTTP/1.1 200 OK\r\n\r\nnot json"), + None + ); + assert_eq!(super::parse_health_pid(""), None); + } + + /// Stands in for the *other* StemDeck: something already listening on the + /// port, answering /api/health with a 200 that is not ours. + fn other_instance_on_a_port(pid: u32) -> u16 { + use std::io::{Read, Write}; + + let listener = std::net::TcpListener::bind(("127.0.0.1", 0)).unwrap(); + let port = listener.local_addr().unwrap().port(); + std::thread::spawn(move || { + for stream in listener.incoming().take(16) { + let Ok(mut stream) = stream else { continue }; + let mut buf = [0u8; 512]; + let _ = stream.read(&mut buf); + let body = format!("{{\"name\":\"StemDeck\",\"status\":\"ok\",\"pid\":{pid}}}"); + let _ = stream.write_all( + format!( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\n\ + Content-Length: {}\r\nConnection: close\r\n\r\n{}", + body.len(), + body + ) + .as_bytes(), + ); + } + }); + port + } + + /// A child that outlives the first poll and then exits, like a backend that + /// loses the race for its port and dies on bind. + fn briefly_alive_child() -> Child { + #[cfg(windows)] + let mut cmd = { + let mut c = Command::new("cmd"); + c.args(["/C", "ping", "-n", "2", "127.0.0.1"]); + c + }; + #[cfg(not(windows))] + let mut cmd = { + let mut c = Command::new("sh"); + c.args(["-c", "sleep 1"]); + c + }; + cmd.stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .unwrap() + } + + #[test] + fn a_stranger_on_the_port_is_never_accepted_as_our_backend() { + // The whole of #424 in one test. Another instance answers 200 on the + // port while the backend we spawned dies. Before the fix this returned + // Ok, the shell pointed the window at that backend, and the second + // install quietly drove the first install's library. + let port = other_instance_on_a_port(999_999); + let mut child = briefly_alive_child(); + + let result = super::wait_for_health( + &mut child, + port, + Duration::from_secs(20), + Path::new("does-not-exist.log"), + ); + let _ = child.kill(); + let _ = child.wait(); + + let err = result.expect_err("adopted a backend that was not ours"); + assert!( + err.contains(&port.to_string()), + "the error should name the contended port, got: {err}" + ); + } + + #[test] + fn our_own_backend_is_accepted() { + // The other half: verification must not be so strict that a healthy + // start is rejected. A responder reporting our child's pid is ours. + let mut child = briefly_alive_child(); + let port = other_instance_on_a_port(child.id()); + + let result = super::wait_for_health( + &mut child, + port, + Duration::from_secs(20), + Path::new("does-not-exist.log"), + ); + let _ = child.kill(); + let _ = child.wait(); + + assert!(result.is_ok(), "rejected our own backend: {result:?}"); + } + + #[test] + fn a_conflict_hint_names_the_port_and_stays_quiet_otherwise() { + let hint = super::port_conflict_hint(8000, Some(1234)); + assert!(hint.contains("8000") && hint.contains("1234")); + // No foreign responder seen: say nothing rather than guess at a cause. + assert!(super::port_conflict_hint(8000, None).is_empty()); + } } diff --git a/tests/test_health_api.py b/tests/test_health_api.py index 52eb34c6..10e74847 100644 --- a/tests/test_health_api.py +++ b/tests/test_health_api.py @@ -19,6 +19,20 @@ def test_health_endpoints_report_ok(): assert "data_dir" not in body +def test_health_identifies_the_answering_process(): + # The desktop shell spawns this backend and polls /api/health to know it + # started. A 200 alone only proves *something* holds the port: a second + # StemDeck used to adopt the first instance's backend, and with it the first + # instance's data directory and library (#424). The shell compares this pid + # against the child it spawned, so it must be the real one. + import os + + from app.main import app + + with TestClient(app) as client: + assert client.get("/api/health").json()["pid"] == os.getpid() + + # --- version source precedence (#421) --------------------------------------- # # The in-app updater replaces backend/ but never python/, where the installed