From 479a619b6cf1590b6e3c4681cc1897bab2c7364e Mon Sep 17 00:00:00 2001 From: t Date: Sat, 19 Sep 2026 14:45:24 -0700 Subject: [PATCH] fix(codex): kernel-assigned fixture ports close the EADDRINUSE port-theft flake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fixture-spawn helper pre-allocated its listening port via bind-drop, leaving a window where the kernel hands the just-freed port to a sibling test's fixture: ours dies EADDRINUSE (tokio reaps it, /proc gone) while the spawn probe happily handshakes with the thief — the same fixture protocol, indistinguishable — until /proc evidence capture panics with "live child has a starttime" (observed once, under a full workspace run on a 96-core host; fd-exhaustion ruled out, ulimit -n = 1M, and the kernel's bind(0) allocator re-issues just-freed ports measurably). The fixture now binds port 0 (kernel-atomic, no free-port window) and reports the real port through a port file; the helper waits for the report, then probes. Evidence-capture panics now name the pid and the likely cause. A 12-way concurrent-spawn regression test pins every child live + every port distinct. --- .../src/sidecar_reconcile_tests.rs | 52 ++++++++++++++ .../src/sidecar_test_support.rs | 70 ++++++++++++++++--- .../codex-app-server/fake-app-server.mjs | 22 +++++- 3 files changed, 134 insertions(+), 10 deletions(-) diff --git a/crates/freshell-codex/src/sidecar_reconcile_tests.rs b/crates/freshell-codex/src/sidecar_reconcile_tests.rs index 6917c06a6..eb3a32642 100644 --- a/crates/freshell-codex/src/sidecar_reconcile_tests.rs +++ b/crates/freshell-codex/src/sidecar_reconcile_tests.rs @@ -639,6 +639,58 @@ async fn reattach_shutdown_kills_only_after_reverification() { .expect("cleanup: kill this test's own fixture"); } +// --------------------------------------------------------------------------- +// Port-file protocol regression: fixture spawns once pre-allocated the +// listening port via bind-drop, leaving a window where the kernel hands +// the just-freed port to a concurrent test's fixture — ours dies +// EADDRINUSE while our probe handshakes with the thief, and evidence +// capture then panics ("live child has a starttime"). The port-file +// protocol (fixture binds port 0 and reports the real port) removes the +// window; this test recreates the dense concurrent-spawn shape and asserts +// every child stays live and every reported port is distinct. +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn concurrent_fixture_spawns_stay_live_and_report_distinct_ports() { + const N: usize = 12; + let mut jobs = Vec::new(); + for i in 0..N { + let ownership = format!("codex-sidecar-a6000210-ffff-4fff-8fff-fffffffff{i:03x}"); + jobs.push(tokio::spawn(async move { + spawn_own_fake_app_server(&ownership).await + })); + } + let mut children = Vec::new(); + let mut ports = std::collections::HashSet::new(); + for (i, job) in jobs.into_iter().enumerate() { + let (child, ws_url) = job.await.expect("join spawn task"); + // The exact capture the flake broke: /proc evidence for every + // child, immediately after the helper returned it as live. + let record = record_for_child( + &format!("codex-sidecar-a6000211-ffff-4fff-8fff-fffffffff{i:03x}"), + child.id().expect("live fixture pid"), + Some(SESSION), + ); + assert!(record.starttime > 0, "starttime evidence captured"); + let port = ws_url + .rsplit(':') + .next() + .expect("ws_url port suffix") + .to_string(); + assert!( + ports.insert(port), + "two concurrent fixtures reported the same port" + ); + children.push(child); + } + for mut child in children { + child + .kill() + .await + .expect("cleanup: kill this test's own fixture"); + } +} + // --------------------------------------------------------------------------- // Task 7: the plan-aware selection seam ([`crate::runtime_select`]). // diff --git a/crates/freshell-codex/src/sidecar_test_support.rs b/crates/freshell-codex/src/sidecar_test_support.rs index e9909791e..c1ebd803f 100644 --- a/crates/freshell-codex/src/sidecar_test_support.rs +++ b/crates/freshell-codex/src/sidecar_test_support.rs @@ -72,7 +72,11 @@ pub(crate) fn spawn_own_shell_child( /// A loopback `ws://` URL on an ephemeral port NOTHING listens on (bound, /// read, dropped) — probe dials fail fast with connection-refused. Never -/// port 3001. +/// port 3001. For dial-fail records ONLY: the freed port can in principle +/// be re-issued by the kernel to a concurrent listener within the test's +/// window (never observed). Fixture spawns must NOT use this — they use +/// the fixture's port-file protocol ([`FIXTURE_PORT_FILE_ENV`]) instead, +/// which has no free-port window at all. pub(crate) fn unused_loopback_ws_url() -> String { let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind ephemeral port"); let port = listener.local_addr().expect("local_addr").port(); @@ -80,6 +84,12 @@ pub(crate) fn unused_loopback_ws_url() -> String { format!("ws://127.0.0.1:{port}") } +/// Opt-in port-report mode of the fake app-server fixture: when set, the +/// fixture binds a kernel-assigned ephemeral port (`--listen +/// ws://127.0.0.1:0`) and writes the actual port (newline-terminated) to +/// the named file once listening. +pub(crate) const FIXTURE_PORT_FILE_ENV: &str = "FAKE_CODEX_APP_SERVER_PORT_FILE"; + /// A record carrying a spawned child's REAL `/proc` evidence. pub(crate) fn record_for_child( ownership_id: &str, @@ -90,8 +100,19 @@ pub(crate) fn record_for_child( record_version: SIDECAR_RECORD_VERSION, ownership_id: ownership_id.to_string(), pid, - starttime: proc_starttime(pid as i32).expect("live child has a starttime"), - cmdline: proc_cmdline(pid as i32).expect("live child has a cmdline"), + starttime: proc_starttime(pid as i32).unwrap_or_else(|| { + panic!( + "no /proc/{pid}/stat starttime for the fixture child (pid {pid}) — \ + it was expected to be live; it most likely exited first \ + (e.g. EADDRINUSE when another fixture won the port)" + ) + }), + cmdline: proc_cmdline(pid as i32).unwrap_or_else(|| { + panic!( + "no /proc/{pid}/cmdline for the fixture child (pid {pid}) — \ + it was expected to be live; it most likely exited first" + ) + }), ws_url: unused_loopback_ws_url(), session_id: session_id.map(str::to_string), terminal_id: None, @@ -114,9 +135,17 @@ pub(crate) fn fake_app_server_fixture() -> std::path::PathBuf { .join("../../test/fixtures/coding-cli/codex-app-server/fake-app-server.mjs") } -/// Spawn THIS TEST'S OWN fake app-server on a loopback ephemeral port and -/// wait for its WS listener to accept. `kill_on_drop(true)` guarantees -/// cleanup kills ONLY this recorded child, even on panic. +/// Spawn THIS TEST'S OWN fake app-server on a kernel-assigned loopback +/// ephemeral port and wait for its WS listener to accept. The fixture +/// reports its actual port through the [`FIXTURE_PORT_FILE_ENV`] +/// port-file protocol: there is NO pre-allocated free port for a sibling +/// fixture to steal. (The pre-fix helper allocated a port via bind-drop; +/// the randomized kernel allocator can hand the just-freed port to a +/// concurrent test's fixture, killing ours with EADDRINUSE while our +/// probe handshakes with the thief — the observed once-off +/// "live child has a starttime" flake.) +/// `kill_on_drop(true)` guarantees cleanup kills ONLY this recorded child, +/// even on panic. pub(crate) async fn spawn_own_fake_app_server( ownership_id: &str, ) -> (tokio::process::Child, String) { @@ -130,14 +159,18 @@ pub(crate) async fn spawn_own_fake_app_server_with_behavior( ownership_id: &str, behavior_json: Option<&str>, ) -> (tokio::process::Child, String) { - // Allocate a free loopback ephemeral port for the fixture to listen on. - let ws_url = unused_loopback_ws_url(); + // The fixture overwrites this file with its actual listening port. + // NamedTempFile gives a unique path and removes it on drop, even on + // panic. + let port_file = tempfile::NamedTempFile::new().expect("create fixture port file"); + let port_file_path = port_file.path().to_owned(); let mut command = tokio::process::Command::new("node"); command .arg(fake_app_server_fixture()) .arg("--listen") - .arg(&ws_url) + .arg("ws://127.0.0.1:0") .env(crate::durability::CODEX_SIDECAR_OWNERSHIP_ENV, ownership_id) + .env(FIXTURE_PORT_FILE_ENV, &port_file_path) .stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()) @@ -149,6 +182,25 @@ pub(crate) async fn spawn_own_fake_app_server_with_behavior( .spawn() .expect("spawn this test's own fake app-server"); let deadline = tokio::time::Instant::now() + Duration::from_secs(30); + // Phase 1: the fixture reports the port the kernel assigned IT — the + // assignment is atomic with the bind, so no other fixture can hold or + // steal it; the reported URL always belongs to this test's own child. + let ws_url = loop { + if let Ok(report) = std::fs::read_to_string(&port_file_path) { + if let Ok(port) = report.trim().parse::() { + break format!("ws://127.0.0.1:{port}"); + } + } + if let Ok(Some(status)) = child.try_wait() { + panic!("fake app-server exited before reporting its port: {status}"); + } + assert!( + tokio::time::Instant::now() < deadline, + "fake app-server never reported its listening port" + ); + tokio::time::sleep(Duration::from_millis(10)).await; + }; + // Phase 2: verify the reported port accepts a WS handshake. loop { if let Ok(Ok((probe, _response))) = tokio::time::timeout( Duration::from_secs(1), diff --git a/test/fixtures/coding-cli/codex-app-server/fake-app-server.mjs b/test/fixtures/coding-cli/codex-app-server/fake-app-server.mjs index 88def4400..02422f4e5 100644 --- a/test/fixtures/coding-cli/codex-app-server/fake-app-server.mjs +++ b/test/fixtures/coding-cli/codex-app-server/fake-app-server.mjs @@ -508,6 +508,18 @@ const url = new URL(listenUrl) const host = url.hostname const port = Number(url.port) +// Port-file mode (freshell-codex test support): bind a KERNEL-assigned +// ephemeral port and report it back, eliminating the bind→drop→respawn +// window where the kernel can hand the just-freed port to a sibling +// fixture — ours then dies EADDRINUSE while the spawning test's probe +// happily handshakes with the thief (the observed "live child has a +// starttime" flake). Strictly opt-in via env; requires --listen port 0 so +// a specific port can never be silently overridden. +const portFile = process.env.FAKE_CODEX_APP_SERVER_PORT_FILE +if (portFile && port !== 0) { + throw new Error('FAKE_CODEX_APP_SERVER_PORT_FILE requires --listen ws://127.0.0.1:0') +} + let nativeChild if (behavior.spawnNativeChild) { nativeChild = spawn(process.execPath, [new URL(import.meta.url).pathname, 'fake-native-child'], { @@ -551,7 +563,15 @@ if (behavior.spawnDurableWriter) { } } -const wss = new WebSocketServer({ host, port }) +const wss = portFile + ? new WebSocketServer({ host, port: 0 }, () => { + const address = wss.address() + if (!address || typeof address === 'string') { + throw new Error('fake app-server did not receive a loopback port') + } + fs.writeFileSync(portFile, `${address.port}\n`, 'utf8') + }) + : new WebSocketServer({ host, port }) const watches = new Map() const activeThreadIds = new Set() // kata 1wxv (LBC-1): thread/revert is paginated-only. Threads THIS process