diff --git a/apps/lightcraft-cli/tests/it/cli.rs b/apps/lightcraft-cli/tests/it/cli.rs index 0664677ee..0e3c472ab 100644 --- a/apps/lightcraft-cli/tests/it/cli.rs +++ b/apps/lightcraft-cli/tests/it/cli.rs @@ -508,6 +508,29 @@ fn snapshot_starts_without_a_gpu() { } } +/// A loopback port nothing listens on, for tests that need `--connect` to be refused (#3070). +/// A fixed port is not safe: agents give their apps' control servers ports in 18000–19999 (and +/// 7980 is the default), so a test could reach someone's running app and drive it. The OS hands +/// out a free port for `:0`; the listener is dropped so connecting to it is refused. Another +/// process could bind the port between the drop and the connect. That window is a few +/// milliseconds, and `:0` draws from the OS's ephemeral range (from 32768 on Linux, 49152 on +/// macOS and Windows), where nobody puts a control server (7980, 18000–19999): the standard +/// trade-off for a test that needs a refused connection. +/// +/// Within this binary the window is closed: the tests that take an ephemeral port and let it go +/// hold [`ephemeral_ports`], so `denoise-serve` can't start on a port a refused-connection test +/// picked while it runs. +fn unused_port() -> u16 { + std::net::TcpListener::bind("127.0.0.1:0").unwrap().local_addr().unwrap().port() +} + +/// Held by each test that picks an ephemeral port and releases it before use, for the whole test. +fn ephemeral_ports() -> std::sync::MutexGuard<'static, ()> { + static PORTS: std::sync::Mutex<()> = std::sync::Mutex::new(()); + // a test that failed while holding it doesn't fail the others + PORTS.lock().unwrap_or_else(std::sync::PoisonError::into_inner) +} + fn run_cli(args: &[&str], stdin: Option<&str>) -> (bool, Vec, String) { let mut child = Command::new(binary()).arg("run").args(args).stdin(Stdio::piped()).stdout(Stdio::piped()).stderr(Stdio::piped()).spawn().unwrap(); if let Some(text) = stdin { @@ -634,9 +657,11 @@ fn a_library_open_in_another_process_is_refused() { /// ADDR actually names — following the old fixed "7980" left the retry connecting elsewhere. #[test] fn connect_failure_hint_names_the_attempted_port() { - let (ok, _lines, err) = run_cli(&["--connect", "127.0.0.1:18437", "ui.inspect"], None); + let _ports = ephemeral_ports(); + let port = unused_port(); + let (ok, _lines, err) = run_cli(&["--connect", &format!("127.0.0.1:{port}"), "ui.inspect"], None); assert!(!ok); - assert!(err.contains("--control 18437"), "the hint must name the port it tried: {err}"); + assert!(err.contains(&format!("--control {port}")), "the hint must name the port it tried: {err}"); assert!(!err.contains("--control 7980"), "{err}"); } @@ -646,20 +671,23 @@ fn connect_failure_hint_names_the_attempted_port() { /// works; non-numeric, non-`ADDR`-shaped tokens (e.g. a real command id) keep falling through. #[test] fn connect_bare_port_is_treated_as_loopback_port() { + let _ports = ephemeral_ports(); + let port = unused_port(); + let hint = format!("--control {port}"); // Bare port: the failure hint names that port. - let (ok, _lines, err) = run_cli(&["--connect", "18437", "ui.inspect"], None); + let (ok, _lines, err) = run_cli(&["--connect", &port.to_string(), "ui.inspect"], None); assert!(!ok); - assert!(err.contains("--control 18437"), "bare port must reach the hint: {err}"); + assert!(err.contains(&hint), "bare port must reach the hint: {err}"); assert!(!err.contains("--control 7980"), "{err}"); // `:` still works (issue #167 case). - let (ok, _lines, err) = run_cli(&["--connect", "127.0.0.1:18437", "ui.inspect"], None); + let (ok, _lines, err) = run_cli(&["--connect", &format!("127.0.0.1:{port}"), "ui.inspect"], None); assert!(!ok); - assert!(err.contains("--control 18437"), "{err}"); + assert!(err.contains(&hint), "{err}"); // `--connect=` already worked (the `a.starts_with("--connect=")` arm); spot-check // the hint matches its port too. - let (ok, _lines, err) = run_cli(&["--connect=18437", "ui.inspect"], None); + let (ok, _lines, err) = run_cli(&[&format!("--connect={port}"), "ui.inspect"], None); assert!(!ok); - assert!(err.contains("--control 18437"), "{err}"); + assert!(err.contains(&hint), "{err}"); } /// The CLI logs warnings on stderr (#168): with `RUST_LOG=warn` an unknown GPU backend name is @@ -805,7 +833,8 @@ fn denoise_serve_runs_tiles_for_another_computer() { Command::new(binary()).args(["denoise-serve", "--models", models.to_str().unwrap(), "--token-env", "LC_TEST_NO_SUCH_VAR"]).output().unwrap(); assert!(!o.status.success() && String::from_utf8_lossy(&o.stderr).contains("LC_TEST_NO_SUCH_VAR")); - let port = std::net::TcpListener::bind("127.0.0.1:0").unwrap().local_addr().unwrap().port(); + let _ports = ephemeral_ports(); + let port = unused_port(); let addr = format!("127.0.0.1:{port}"); let token = "cli-test-token-0123456789"; let mut child = Command::new(binary())