From 67e4d4c7a838d5d02617c1957b7dd681055050a5 Mon Sep 17 00:00:00 2001 From: Tee8z Date: Tue, 20 Oct 2026 13:10:20 -0400 Subject: [PATCH] fix(agents): retry transient executable conflicts A concurrent fork can briefly inherit an executable's writable descriptor before exec closes it. Provider launches and adapter version probes then fail with ExecutableFileBusy even after the staging writer closes. Share the bounded launch retry between provider execution and version probes. Retry only ExecutableFileBusy, at most ten times at 10 ms intervals. Keep probe retries within the existing five-second deadline. Exercise transient and persistent writers, other launch errors, and the real version-probe path with a script held open for writing. Signed-off-by: Tee8z --- .../src-tauri/src/managed_agents/backend.rs | 8 +-- .../src/managed_agents/backend_tests.rs | 53 +++++++++++++++++++ .../src-tauri/src/managed_agents/discovery.rs | 9 ++-- .../discovery/tests/codex_version.rs | 53 +++++++++++++++++++ desktop/src-tauri/src/managed_agents/mod.rs | 1 + .../src/managed_agents/process_spawn.rs | 16 ++++++ 6 files changed, 131 insertions(+), 9 deletions(-) create mode 100644 desktop/src-tauri/src/managed_agents/process_spawn.rs diff --git a/desktop/src-tauri/src/managed_agents/backend.rs b/desktop/src-tauri/src/managed_agents/backend.rs index c6c96c73647..9bd2c3c8a68 100644 --- a/desktop/src-tauri/src/managed_agents/backend.rs +++ b/desktop/src-tauri/src/managed_agents/backend.rs @@ -1,4 +1,5 @@ use super::discovery::{command_search::command_discovery_dirs, resolve_command}; +use super::process_spawn::spawn_with_busy_retry as spawn_provider; use sha2::{Digest, Sha256}; use std::io::{BufReader, Read, Write}; use std::path::{Path, PathBuf}; @@ -88,11 +89,10 @@ pub fn invoke_provider( cmd.current_dir(home); } crate::util::configure_no_window(&mut cmd); - let mut child = cmd - .stdin(std::process::Stdio::piped()) + cmd.stdin(std::process::Stdio::piped()) .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::piped()) - .spawn() + .stderr(std::process::Stdio::piped()); + let mut child = spawn_provider(&mut cmd) .map_err(|e| format!("failed to spawn {}: {e}", binary.display()))?; // Write request and close stdin immediately so the provider sees EOF. diff --git a/desktop/src-tauri/src/managed_agents/backend_tests.rs b/desktop/src-tauri/src/managed_agents/backend_tests.rs index d467f75dda1..b8af06e1c20 100644 --- a/desktop/src-tauri/src/managed_agents/backend_tests.rs +++ b/desktop/src-tauri/src/managed_agents/backend_tests.rs @@ -142,6 +142,59 @@ fn write_test_provider(path: &Path, body: &str) { std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o700)).unwrap(); } +#[cfg(target_os = "linux")] +#[test] +fn provider_spawn_waits_for_a_transient_writer() { + let directory = tempfile::tempdir().unwrap(); + let provider = directory.path().join("provider"); + write_test_provider(&provider, "exit 0"); + let writer = std::fs::OpenOptions::new() + .write(true) + .open(&provider) + .unwrap(); + assert_eq!( + std::process::Command::new(&provider) + .spawn() + .unwrap_err() + .kind(), + std::io::ErrorKind::ExecutableFileBusy, + ); + let closer = std::thread::spawn(move || { + std::thread::sleep(Duration::from_millis(20)); + drop(writer); + }); + + let mut child = spawn_provider(&mut std::process::Command::new(&provider)) + .expect("launch after the inherited writer closes"); + assert!(child.wait().unwrap().success()); + closer.join().unwrap(); +} + +#[cfg(target_os = "linux")] +#[test] +fn provider_spawn_returns_persistent_write_conflicts() { + let directory = tempfile::tempdir().unwrap(); + let provider = directory.path().join("provider"); + write_test_provider(&provider, "exit 0"); + let _writer = std::fs::OpenOptions::new() + .write(true) + .open(&provider) + .unwrap(); + + let error = spawn_provider(&mut std::process::Command::new(&provider)).unwrap_err(); + assert_eq!(error.kind(), std::io::ErrorKind::ExecutableFileBusy); +} + +#[test] +fn provider_spawn_preserves_other_launch_errors() { + let directory = tempfile::tempdir().unwrap(); + let error = spawn_provider(&mut std::process::Command::new( + directory.path().join("missing"), + )) + .unwrap_err(); + assert_eq!(error.kind(), std::io::ErrorKind::NotFound); +} + #[cfg(unix)] #[test] fn provider_deploy_negotiates_and_deploys_the_same_staged_bytes() { diff --git a/desktop/src-tauri/src/managed_agents/discovery.rs b/desktop/src-tauri/src/managed_agents/discovery.rs index 8a5f422a25a..b9ca77d58ef 100644 --- a/desktop/src-tauri/src/managed_agents/discovery.rs +++ b/desktop/src-tauri/src/managed_agents/discovery.rs @@ -840,14 +840,13 @@ pub(crate) fn probe_codex_acp_version_with_path( command.env("PATH", path); } crate::util::configure_no_window(&mut command); - let mut child = command + let deadline = Instant::now() + VERSION_PROBE_TIMEOUT; + command .stdout(tmp.try_clone().ok()?) - .stderr(std::process::Stdio::null()) - .spawn() - .ok()?; + .stderr(std::process::Stdio::null()); + let mut child = super::process_spawn::spawn_with_busy_retry(&mut command).ok()?; // Poll until the deadline rather than blocking on stdout EOF. - let deadline = Instant::now() + VERSION_PROBE_TIMEOUT; let exit_status = loop { match child.try_wait() { Ok(Some(status)) => break status, diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests/codex_version.rs b/desktop/src-tauri/src/managed_agents/discovery/tests/codex_version.rs index 19053865cbe..817d104041f 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests/codex_version.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests/codex_version.rs @@ -73,3 +73,56 @@ fn codex_adapter_availability_outdated_for_older_1x_binary() { assert!(codex_adapter_is_outdated(&bin)); } } + +#[cfg(target_os = "linux")] +#[test] +fn probe_codex_acp_version_waits_for_a_transient_executable_writer() { + use std::os::unix::fs::PermissionsExt; + let temp = tempfile::tempdir().expect("temp dir"); + let binary = temp.path().join("codex-acp"); + std::fs::write( + &binary, + "#!/bin/sh\necho '@agentclientprotocol/codex-acp 1.1.2'\n", + ) + .expect("write adapter"); + std::fs::set_permissions(&binary, std::fs::Permissions::from_mode(0o755)) + .expect("chmod adapter"); + let writer = std::fs::OpenOptions::new() + .write(true) + .open(&binary) + .unwrap(); + assert_eq!( + std::process::Command::new(&binary) + .spawn() + .unwrap_err() + .kind(), + std::io::ErrorKind::ExecutableFileBusy + ); + let closer = std::thread::spawn(move || { + std::thread::sleep(std::time::Duration::from_millis(20)); + drop(writer); + }); + let version = probe_codex_acp_version_with_path(&binary, None); + closer.join().unwrap(); + assert_eq!(version, Some((1, 1, 2))); +} + +#[cfg(target_os = "linux")] +#[test] +fn probe_codex_acp_version_rejects_a_persistent_executable_writer() { + use std::os::unix::fs::PermissionsExt; + let temp = tempfile::tempdir().expect("temp dir"); + let binary = temp.path().join("codex-acp"); + std::fs::write( + &binary, + "#!/bin/sh\necho '@agentclientprotocol/codex-acp 1.1.2'\n", + ) + .expect("write adapter"); + std::fs::set_permissions(&binary, std::fs::Permissions::from_mode(0o755)) + .expect("chmod adapter"); + let _writer = std::fs::OpenOptions::new() + .write(true) + .open(&binary) + .unwrap(); + assert_eq!(probe_codex_acp_version_with_path(&binary, None), None); +} diff --git a/desktop/src-tauri/src/managed_agents/mod.rs b/desktop/src-tauri/src/managed_agents/mod.rs index ad030769de0..7accae3b0f6 100644 --- a/desktop/src-tauri/src/managed_agents/mod.rs +++ b/desktop/src-tauri/src/managed_agents/mod.rs @@ -32,6 +32,7 @@ pub(crate) mod persona_events; mod personas; #[cfg(windows)] mod process_lifecycle; +mod process_spawn; pub(crate) mod readiness; pub(crate) mod reconcile; mod relay_mesh; diff --git a/desktop/src-tauri/src/managed_agents/process_spawn.rs b/desktop/src-tauri/src/managed_agents/process_spawn.rs new file mode 100644 index 00000000000..8209b2966c6 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/process_spawn.rs @@ -0,0 +1,16 @@ +pub(super) fn spawn_with_busy_retry( + cmd: &mut std::process::Command, +) -> std::io::Result { + // A concurrent fork can inherit the staging writer until that child execs, + // even after this thread closes it. Retry only that failed exec: the command + // never started. Persistent writers still fail after 100 ms. + for _ in 0..10 { + match cmd.spawn() { + Err(error) if error.kind() == std::io::ErrorKind::ExecutableFileBusy => { + std::thread::sleep(std::time::Duration::from_millis(10)); + } + result => return result, + } + } + cmd.spawn() +}