Merge pull request #252 from ScriptedAlchemy/fix/flaky-tests

ScriptedAlchemy · web-flow · commit e5da19600eb7 · 2026-07-03T21:03:42.000Z
test: de-flake intermittent CI tests (parallel-load resilience)
diff --git a/src/mcp/hook_events.rs b/src/mcp/hook_events.rs
@@ -276,12 +276,66 @@ mod tests {
         }
     }
 
+    /// Resolves the `git` executable to an absolute path exactly once per
+    /// process. Under heavy parallel test load (nextest spawns one process per
+    /// test, each spawning several `git` subprocesses), a bare
+    /// `Command::new("git")` PATH lookup can transiently fail the spawn with
+    /// `ENOENT` ("No such file or directory") even though git is installed.
+    /// Resolving to an absolute path up front, plus a `GIT` env override,
+    /// removes the per-spawn PATH walk and makes the lookup deterministic.
+    fn git_program() -> std::ffi::OsString {
+        use std::sync::OnceLock;
+        static GIT: OnceLock<std::ffi::OsString> = OnceLock::new();
+        GIT.get_or_init(|| {
+            if let Some(explicit) = std::env::var_os("GIT") {
+                return explicit;
+            }
+            let exe_name = if cfg!(windows) { "git.exe" } else { "git" };
+            if let Some(paths) = std::env::var_os("PATH") {
+                for dir in std::env::split_paths(&paths) {
+                    let candidate = dir.join(exe_name);
+                    if candidate.is_file() {
+                        return candidate.into_os_string();
+                    }
+                }
+            }
+            // Fall back to a bare name and let the OS resolve it.
+            std::ffi::OsString::from("git")
+        })
+        .clone()
+    }
+
     fn run_git(cwd: &Path, args: &[&str]) {
-        let output = Command::new("git")
-            .args(args)
-            .current_dir(cwd)
-            .output()
-            .unwrap_or_else(|e| panic!("git {args:?} should run: {e}"));
+        // A cwd that does not yet exist makes the spawn itself fail with
+        // ENOENT, which is indistinguishable from git-not-found; guard it so
+        // any real failure is attributable.
+        assert!(
+            cwd.is_dir(),
+            "git cwd {cwd:?} should exist before running git {args:?}"
+        );
+        let git = git_program();
+        // Retry a transient spawn ENOENT a few times: under load the initial
+        // fork/exec can spuriously fail even with a valid absolute program.
+        let mut last_err: Option<std::io::Error> = None;
+        let mut output = None;
+        for attempt in 0..5 {
+            match Command::new(&git).args(args).current_dir(cwd).output() {
+                Ok(out) => {
+                    output = Some(out);
+                    break;
+                }
+                Err(e) if e.kind() == std::io::ErrorKind::NotFound && attempt < 4 => {
+                    last_err = Some(e);
+                    std::thread::sleep(std::time::Duration::from_millis(20 * (attempt + 1)));
+                }
+                Err(e) => {
+                    panic!("git {args:?} should run (program {git:?}): {e}");
+                }
+            }
+        }
+        let output = output.unwrap_or_else(|| {
+            panic!("git {args:?} should run (program {git:?}) after retries: {last_err:?}")
+        });
         assert!(
             output.status.success(),
             "git {:?} failed\nstdout:\n{}\nstderr:\n{}",
diff --git a/tests/automation_runner_test/backend.rs b/tests/automation_runner_test/backend.rs
@@ -24,12 +24,15 @@ use crate::common::{
 
 static ENV_LOCK: Mutex<()> = Mutex::new(());
 
+/// Success-path budget for the fake codex app-server child to spawn (a real
+/// python interpreter) and complete its scripted turn. This is the upper bound
+/// the backend waits before declaring a timeout; it must be generous enough
+/// that a slow python spawn/schedule under nextest's process-per-test
+/// parallelism can never false-fire it, while still failing fast on a genuine
+/// hang. Tests that deliberately exercise the timeout path pass their own tight
+/// `Duration` (e.g. 300ms) and are unaffected by this value.
 fn fake_codex_response_timeout() -> Duration {
-    if cfg!(windows) {
-        Duration::from_secs(30)
-    } else {
-        Duration::from_secs(5)
-    }
+    Duration::from_secs(30)
 }
 
 fn fake_codex_response_timeout_secs() -> u64 {
diff --git a/tests/common/mod.rs b/tests/common/mod.rs
@@ -389,6 +389,35 @@ pub fn tracedecay_command_with_home(home: &Path) -> Command {
     command
 }
 
+/// Resolves the `git` executable to an absolute path exactly once per process.
+///
+/// Under heavy parallel test load (nextest spawns one process per test, each
+/// spawning several `git` subprocesses), a bare `Command::new("git")` PATH
+/// lookup can transiently fail the spawn with `ENOENT` ("No such file or
+/// directory") even though git is installed. Resolving to an absolute path up
+/// front — with an optional `GIT` env override — removes the per-spawn PATH
+/// walk and makes the lookup deterministic.
+pub fn git_program() -> std::ffi::OsString {
+    use std::sync::OnceLock;
+    static GIT: OnceLock<std::ffi::OsString> = OnceLock::new();
+    GIT.get_or_init(|| {
+        if let Some(explicit) = std::env::var_os("GIT") {
+            return explicit;
+        }
+        let exe_name = if cfg!(windows) { "git.exe" } else { "git" };
+        if let Some(paths) = std::env::var_os("PATH") {
+            for dir in std::env::split_paths(&paths) {
+                let candidate = dir.join(exe_name);
+                if candidate.is_file() {
+                    return candidate.into_os_string();
+                }
+            }
+        }
+        std::ffi::OsString::from("git")
+    })
+    .clone()
+}
+
 #[cfg(unix)]
 pub fn daemon_socket_path(home: &Path) -> PathBuf {
     canonical_existing_path(home).join(".tracedecay/daemon.sock")
@@ -438,19 +467,68 @@ pub fn response_to_json(mut response: ureq::http::Response<ureq::Body>) -> (u16,
     (status, parsed)
 }
 
+/// True when a `ureq` error is a connection-level failure that a freshly
+/// started (or briefly overloaded) server can transiently raise before it is
+/// steadily accepting requests: peer disconnected, connection refused/reset,
+/// or a bare I/O error. These are safe to retry for idempotent test requests;
+/// an HTTP status error is NOT one of these (the agent is built with
+/// `http_status_as_error(false)`, so 4xx/5xx come back as `Ok`).
+pub fn is_transient_connection_error(err: &ureq::Error) -> bool {
+    match err {
+        ureq::Error::ConnectionFailed => true,
+        ureq::Error::Io(_) => true,
+        other => {
+            // Fall back to a message match so newer/renamed variants (e.g.
+            // "Peer disconnected", "connection reset") still count as transient
+            // without pinning to a specific ureq version's enum shape.
+            let text = other.to_string().to_ascii_lowercase();
+            text.contains("peer disconnected")
+                || text.contains("connection refused")
+                || text.contains("connection reset")
+                || text.contains("broken pipe")
+                || text.contains("timed out")
+        }
+    }
+}
+
+/// Issues an idempotent HTTP request, retrying transient connection-level
+/// errors (the server racing its own readiness under parallel load) with a
+/// short bounded backoff. `send` performs one attempt; a `ureq::Error` that
+/// passes [`is_transient_connection_error`] is retried, any other error (or
+/// exhausted retries) panics with `label`.
+pub fn http_call_with_retry(
+    label: &str,
+    send: impl Fn() -> Result<ureq::http::Response<ureq::Body>, ureq::Error>,
+) -> ureq::http::Response<ureq::Body> {
+    let mut last_err: Option<ureq::Error> = None;
+    for attempt in 0..12 {
+        match send() {
+            Ok(response) => return response,
+            Err(err) if is_transient_connection_error(&err) => {
+                last_err = Some(err);
+                std::thread::sleep(Duration::from_millis(25 * (attempt + 1)));
+            }
+            Err(err) => panic!("{label} failed: {err}"),
+        }
+    }
+    panic!("{label} failed after retries: {last_err:?}");
+}
+
 pub fn get_json(agent: &ureq::Agent, url: &str) -> (u16, Value) {
-    let response = match agent.get(url).call() {
-        Ok(response) => response,
-        Err(err) => panic!("GET {url} failed: {err}"),
-    };
+    let response = http_call_with_retry(&format!("GET {url}"), || agent.get(url).call());
     response_to_json(response)
 }
 
 pub async fn wait_for_dashboard(agent: &ureq::Agent, base_url: &str) {
     let probe = format!("{base_url}/api/capabilities");
-    for _ in 0..80 {
-        if agent.get(&probe).call().is_ok() {
-            return;
+    // Poll until the server both accepts the connection AND returns a real
+    // HTTP response (2xx). A bare connect success is not enough — the server
+    // can accept then drop the socket during startup ("Peer disconnected").
+    for _ in 0..160 {
+        if let Ok(response) = agent.get(&probe).call() {
+            if response.status().is_success() {
+                return;
+            }
         }
         tokio::time::sleep(Duration::from_millis(50)).await;
     }
diff --git a/tests/core_cli_suite/tool_daemon_test.rs b/tests/core_cli_suite/tool_daemon_test.rs
@@ -16,6 +16,20 @@ use tracedecay::storage::{
     default_profile_project_id, write_enrollment_marker, EnrollmentMarker, StorageMode,
 };
 
+/// Bound for waits that depend on spawning and running the real `tracedecay`
+/// CLI as a child process: connecting to the fake daemon socket and forwarding
+/// the observed request back to the test thread. Under nextest's
+/// process-per-test parallelism the fork/exec + init of that child can be
+/// scheduled slowly on a loaded runner, so a 2s bound false-fires. This is a
+/// generous ceiling that still fails fast on a genuine hang (the CLI normally
+/// connects in well under a second).
+const CLI_ROUNDTRIP_TIMEOUT: Duration = Duration::from_secs(20);
+
+/// Bound for local, in-process readiness signals (a spawned thread binding a
+/// socket and sending on an mpsc channel). These do not spawn external
+/// processes, but the thread can still be scheduled slowly under load.
+const LOCAL_READY_TIMEOUT: Duration = Duration::from_secs(10);
+
 fn init_project_with_cli(home: &Path, project: &Path) {
     std::fs::create_dir_all(project.join("src")).unwrap();
     std::fs::write(
@@ -41,11 +55,29 @@ fn init_project_with_cli(home: &Path, project: &Path) {
 }
 
 fn git(project: &Path, args: &[&str]) {
-    let output = std::process::Command::new("git")
-        .args(args)
-        .current_dir(project)
-        .output()
-        .expect("git should run");
+    let git = crate::common::git_program();
+    // Retry a transient spawn ENOENT under heavy parallel load.
+    let mut last_err: Option<std::io::Error> = None;
+    let mut output = None;
+    for attempt in 0..5 {
+        match std::process::Command::new(&git)
+            .args(args)
+            .current_dir(project)
+            .output()
+        {
+            Ok(out) => {
+                output = Some(out);
+                break;
+            }
+            Err(e) if e.kind() == std::io::ErrorKind::NotFound && attempt < 4 => {
+                last_err = Some(e);
+                std::thread::sleep(Duration::from_millis(20 * (attempt + 1)));
+            }
+            Err(e) => panic!("git {args:?} should run (program {git:?}): {e}"),
+        }
+    }
+    let output =
+        output.unwrap_or_else(|| panic!("git {args:?} should run after retries: {last_err:?}"));
     assert!(
         output.status.success(),
         "git {:?} failed\nstdout:\n{}\nstderr:\n{}",
@@ -154,7 +186,7 @@ fn spawn_sentinel_daemon_with_notification(
             .expect("set listener nonblocking");
         ready_tx.send(()).expect("notify fake daemon readiness");
 
-        let deadline = Instant::now() + Duration::from_secs(2);
+        let deadline = Instant::now() + CLI_ROUNDTRIP_TIMEOUT;
         let (stream, _) = loop {
             match listener.accept() {
                 Ok(accepted) => break accepted,
@@ -171,7 +203,7 @@ fn spawn_sentinel_daemon_with_notification(
             .set_nonblocking(false)
             .expect("set accepted stream blocking");
         stream
-            .set_write_timeout(Some(Duration::from_secs(2)))
+            .set_write_timeout(Some(CLI_ROUNDTRIP_TIMEOUT))
             .expect("write timeout");
 
         let mut reader = BufReader::new(stream.try_clone().expect("clone fake daemon stream"));
@@ -228,7 +260,7 @@ fn spawn_sentinel_daemon_with_notification(
     });
 
     ready_rx
-        .recv_timeout(Duration::from_secs(2))
+        .recv_timeout(LOCAL_READY_TIMEOUT)
         .expect("fake daemon should become ready");
     request_rx
 }
@@ -245,7 +277,7 @@ fn spawn_hook_event_daemon(socket_path: PathBuf) -> mpsc::Receiver<Value> {
             .expect("set listener nonblocking");
         ready_tx.send(()).expect("notify fake daemon readiness");
 
-        let deadline = Instant::now() + Duration::from_secs(2);
+        let deadline = Instant::now() + CLI_ROUNDTRIP_TIMEOUT;
         let (stream, _) = loop {
             match listener.accept() {
                 Ok(accepted) => break accepted,
@@ -292,7 +324,7 @@ fn spawn_hook_event_daemon(socket_path: PathBuf) -> mpsc::Receiver<Value> {
     });
 
     ready_rx
-        .recv_timeout(Duration::from_secs(2))
+        .recv_timeout(LOCAL_READY_TIMEOUT)
         .expect("fake daemon should become ready");
     request_rx
 }
@@ -342,7 +374,7 @@ fn assert_hook_notification(
     );
 
     let request = observed_request
-        .recv_timeout(Duration::from_secs(2))
+        .recv_timeout(CLI_ROUNDTRIP_TIMEOUT)
         .expect("fake daemon should receive hook event");
     assert_eq!(request["params"]["agent"], expected_agent);
     assert_eq!(request["params"]["event"], expected_event);
@@ -592,7 +624,7 @@ fn tool_cli_invokes_mcp_tool_through_daemon_socket() {
         "tool CLI should print daemon response, got:\n{stdout}"
     );
     observed_request
-        .recv_timeout(Duration::from_secs(2))
+        .recv_timeout(CLI_ROUNDTRIP_TIMEOUT)
         .expect("fake daemon should receive tools/call request");
 }
 
@@ -635,7 +667,7 @@ fn tool_cli_skips_daemon_notifications_until_matching_response() {
         "tool CLI should print daemon response after notification, got:\n{stdout}"
     );
     observed_request
-        .recv_timeout(Duration::from_secs(2))
+        .recv_timeout(CLI_ROUNDTRIP_TIMEOUT)
         .expect("fake daemon should receive tools/call request");
 }
 
@@ -690,7 +722,7 @@ fn profile_scoped_tool_cli_invokes_daemon_without_project_handshake() {
         "tool CLI should print daemon response, got:\n{stdout}"
     );
     let request = observed_request
-        .recv_timeout(Duration::from_secs(2))
+        .recv_timeout(CLI_ROUNDTRIP_TIMEOUT)
         .expect("fake daemon should receive profile-scoped tools/call request");
     assert_eq!(
         request["params"]["arguments"]["storage_scope"],
@@ -786,7 +818,7 @@ fn first_touch_store_tool_cli_invokes_daemon_with_init_permission() {
         "tool CLI should print daemon response, got:\n{stdout}"
     );
     let request = observed_request
-        .recv_timeout(Duration::from_secs(2))
+        .recv_timeout(CLI_ROUNDTRIP_TIMEOUT)
         .expect("fake daemon should receive first-touch tools/call request");
     assert_eq!(request["params"]["arguments"]["action"], "add");
 }
diff --git a/tests/dashboard_api_test/dashboard_api_support.rs b/tests/dashboard_api_test/dashboard_api_support.rs
diff --git a/tests/hooks_lsp_suite/lsp_code_diagnostics_test.rs b/tests/hooks_lsp_suite/lsp_code_diagnostics_test.rs
