Merge PR #3098: Issue #3070: the CLI's refused-connection tests use a free port, not 18437

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<Value>, 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}");
     // `<host>:<port>` 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=<bare-port>` 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())