mirror of
https://github.com/openai/codex.git
synced 2026-10-19 16:17:49 +00:00
Centralize executable fixture creation to avoid Linux ETXTBSY races (#48727)
## Why Concurrent tests can inherit writable descriptors for executable fixtures, causing `ETXTBSY` when those fixtures launch on Linux. ## What changed Expose shared `write_executable` and `copy_executable` helpers from `codex_utils_cargo_bin` and adopt them in CLI, exec-server, and MCP tests. On Linux, writes and copies complete in separate processes so sibling test spawns cannot inherit the writable descriptors. Script fixtures receive mode `0o755`, and copied executables retain their source permissions. Remove the executable-busy retry loop from the program resolver test now that its script uses the shared helper. GitOrigin-RevId: 61624158190bb1e75c27d9eb9c87a2defc147fb7
This commit is contained in:
@@ -1,8 +1,5 @@
|
||||
#![cfg(unix)]
|
||||
|
||||
#[path = "support/executable.rs"]
|
||||
mod executable;
|
||||
|
||||
use std::path::PathBuf;
|
||||
use std::process::Child;
|
||||
use std::process::Command;
|
||||
@@ -13,7 +10,7 @@ use std::time::Instant;
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
use anyhow::ensure;
|
||||
use executable::copy_executable;
|
||||
use codex_utils_cargo_bin::copy_executable;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::Value;
|
||||
use tempfile::TempDir;
|
||||
|
||||
@@ -1,8 +1,5 @@
|
||||
//! Black-box coverage for safe diagnostic execution and config error reporting.
|
||||
|
||||
#[path = "support/executable.rs"]
|
||||
mod executable;
|
||||
|
||||
use std::ffi::OsString;
|
||||
use std::path::PathBuf;
|
||||
use std::time::Duration;
|
||||
@@ -15,7 +12,7 @@ use codex_app_server_protocol::RequestId;
|
||||
use codex_config::loader::project_trust_key;
|
||||
use codex_state::SqliteConfig;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use executable::copy_executable;
|
||||
use codex_utils_cargo_bin::copy_executable;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::Value;
|
||||
use serde_json::json;
|
||||
|
||||
@@ -1,6 +1,3 @@
|
||||
#[path = "support/executable.rs"]
|
||||
mod executable;
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
#[path = "exec_server/pid_namespace_tests.rs"]
|
||||
mod pid_namespace_tests;
|
||||
@@ -36,7 +33,7 @@ use codex_exec_server::NoiseRendezvousConnectBundle;
|
||||
use codex_exec_server::ProcessId;
|
||||
use codex_http_client::HttpClientFactory;
|
||||
use codex_http_client::OutboundProxyPolicy;
|
||||
use executable::copy_executable;
|
||||
use codex_utils_cargo_bin::copy_executable;
|
||||
use futures::SinkExt;
|
||||
use futures::StreamExt;
|
||||
use predicates::prelude::PredicateBooleanExt;
|
||||
|
||||
@@ -1,39 +0,0 @@
|
||||
//! Copies executable fixtures without exposing writable descriptors to sibling test spawns.
|
||||
//! Linux copies finish in a separate process before the fixture can be launched.
|
||||
|
||||
use std::path::Path;
|
||||
#[cfg(target_os = "linux")]
|
||||
use std::process::Command;
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
#[cfg(target_os = "linux")]
|
||||
use anyhow::ensure;
|
||||
|
||||
pub(super) fn copy_executable(source: &Path, destination: &Path) -> Result<()> {
|
||||
#[cfg(target_os = "linux")]
|
||||
{
|
||||
// Sibling tests can spawn children while a copy holds the executable open
|
||||
// for writing. Keep that descriptor in a separate process and wait for it
|
||||
// to exit, so inherited descriptors cannot cause ETXTBSY during launch.
|
||||
let output = Command::new("/bin/cp")
|
||||
.arg("--")
|
||||
.arg(source)
|
||||
.arg(destination)
|
||||
.output()
|
||||
.context("failed to start fixture executable copy")?;
|
||||
ensure!(
|
||||
output.status.success(),
|
||||
"failed to copy fixture executable {} to {}: {}",
|
||||
source.display(),
|
||||
destination.display(),
|
||||
String::from_utf8_lossy(&output.stderr)
|
||||
);
|
||||
// Match fs::copy even when the test process has a restrictive umask.
|
||||
std::fs::set_permissions(destination, std::fs::metadata(source)?.permissions())?;
|
||||
}
|
||||
#[cfg(not(target_os = "linux"))]
|
||||
std::fs::copy(source, destination)?;
|
||||
Ok(())
|
||||
}
|
||||
@@ -1,11 +1,10 @@
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
|
||||
pub(crate) fn write_fake_bwrap(bin_dir: &Path) -> anyhow::Result<PathBuf> {
|
||||
std::fs::create_dir_all(bin_dir)?;
|
||||
let fake_bwrap = bin_dir.join("bwrap");
|
||||
std::fs::write(
|
||||
codex_utils_cargo_bin::write_executable(
|
||||
&fake_bwrap,
|
||||
r#"#!/bin/bash
|
||||
set -euo pipefail
|
||||
@@ -55,8 +54,5 @@ fi
|
||||
exec "${cmd[@]}"
|
||||
"#,
|
||||
)?;
|
||||
let mut permissions = std::fs::metadata(&fake_bwrap)?.permissions();
|
||||
permissions.set_mode(0o755);
|
||||
std::fs::set_permissions(&fake_bwrap, permissions)?;
|
||||
Ok(fake_bwrap)
|
||||
}
|
||||
|
||||
@@ -69,6 +69,7 @@ mod tests {
|
||||
use super::*;
|
||||
use crate::utils::create_env_for_mcp_server;
|
||||
use anyhow::Result;
|
||||
#[cfg(windows)]
|
||||
use std::fs;
|
||||
use std::path::Path;
|
||||
use tempfile::TempDir;
|
||||
@@ -79,24 +80,10 @@ mod tests {
|
||||
#[tokio::test]
|
||||
async fn test_unix_executes_script_without_extension() -> Result<()> {
|
||||
let env = TestExecutableEnv::new()?;
|
||||
// Linux can transiently report ETXTBSY while the freshly written test
|
||||
// script is becoming executable on the backing filesystem.
|
||||
let mut retries = 0;
|
||||
let output = loop {
|
||||
let mut cmd = Command::new(&env.program_name);
|
||||
cmd.envs(&env.mcp_env);
|
||||
|
||||
let output = cmd.output().await;
|
||||
if !output
|
||||
.as_ref()
|
||||
.is_err_and(|err| err.kind() == std::io::ErrorKind::ExecutableFileBusy)
|
||||
|| retries == 2
|
||||
{
|
||||
break output;
|
||||
}
|
||||
retries += 1;
|
||||
tokio::time::sleep(std::time::Duration::from_millis(10)).await;
|
||||
};
|
||||
let output = Command::new(&env.program_name)
|
||||
.envs(&env.mcp_env)
|
||||
.output()
|
||||
.await;
|
||||
|
||||
assert!(
|
||||
output.is_ok(),
|
||||
@@ -214,22 +201,12 @@ mod tests {
|
||||
#[cfg(unix)]
|
||||
{
|
||||
let file = dir.join(Self::TEST_PROGRAM);
|
||||
fs::write(&file, "#!/bin/sh\nexit 0")?;
|
||||
Self::set_executable(&file)?;
|
||||
codex_utils_cargo_bin::write_executable(&file, "#!/bin/sh\nexit 0")?;
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
fn set_executable(path: &Path) -> Result<()> {
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
let mut perms = fs::metadata(path)?.permissions();
|
||||
perms.set_mode(0o755);
|
||||
fs::set_permissions(path, perms)?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Prepends the given directory to the system's PATH variable.
|
||||
fn build_path_env_var(dir: &Path) -> OsString {
|
||||
let mut path = OsString::from(dir.as_os_str());
|
||||
|
||||
@@ -53,29 +53,9 @@ async fn exercise_stdio_server(
|
||||
let (server, cwd) = if use_executor {
|
||||
(server, cwd)
|
||||
} else {
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
let wrapper = root.path().join("server");
|
||||
let script = "#!/bin/sh\nprintf '%s' \"$0\" > argv0\nexec \"$MCP_SERVER\" \"$@\"\n";
|
||||
#[cfg(target_os = "linux")]
|
||||
{
|
||||
// Keep the writable descriptor out of this process so concurrent test
|
||||
// spawns cannot inherit it and cause ETXTBSY when launching the wrapper.
|
||||
let output = std::process::Command::new("/bin/sh")
|
||||
.arg("-c")
|
||||
.arg("printf '%s' \"$1\" > \"$2\"")
|
||||
.arg("write-mcp-fixture")
|
||||
.arg(script)
|
||||
.arg(&wrapper)
|
||||
.output()?;
|
||||
anyhow::ensure!(
|
||||
output.status.success(),
|
||||
"failed to write MCP wrapper: {}",
|
||||
String::from_utf8_lossy(&output.stderr)
|
||||
);
|
||||
}
|
||||
#[cfg(not(target_os = "linux"))]
|
||||
std::fs::write(&wrapper, script)?;
|
||||
std::fs::set_permissions(wrapper, std::fs::Permissions::from_mode(0o755))?;
|
||||
codex_utils_cargo_bin::write_executable(&wrapper, script)?;
|
||||
env.insert(OsString::from("MCP_SERVER"), server.into_os_string());
|
||||
(
|
||||
std::path::PathBuf::from("./server"),
|
||||
|
||||
@@ -3,7 +3,6 @@
|
||||
use std::collections::HashMap;
|
||||
use std::ffi::OsString;
|
||||
use std::fs;
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
use std::path::Path;
|
||||
use std::sync::Arc;
|
||||
use std::time::Duration;
|
||||
@@ -87,11 +86,10 @@ async fn drop_kills_wrapper_process_group() -> Result<()> {
|
||||
let child_pid_file = temp_dir.path().join("child.pid");
|
||||
let child_pid_file_str = child_pid_file.to_string_lossy().into_owned();
|
||||
let wrapper = temp_dir.path().join("wrapper");
|
||||
fs::write(
|
||||
codex_utils_cargo_bin::write_executable(
|
||||
&wrapper,
|
||||
"#!/bin/sh\nsleep 300 & child_pid=$!; echo \"$child_pid\" > \"$CHILD_PID_FILE\"; cat >/dev/null\n",
|
||||
)?;
|
||||
fs::set_permissions(wrapper, fs::Permissions::from_mode(0o755))?;
|
||||
|
||||
let client = RmcpClient::new_stdio_client(
|
||||
OsString::from("./wrapper"),
|
||||
|
||||
@@ -0,0 +1,62 @@
|
||||
//! Creates executable test fixtures without exposing writable descriptors to sibling spawns.
|
||||
//! On Linux, writes and copies finish in a separate process before the fixture can be launched.
|
||||
|
||||
use std::fs;
|
||||
use std::io;
|
||||
use std::path::Path;
|
||||
#[cfg(target_os = "linux")]
|
||||
use std::process::Command;
|
||||
|
||||
/// Writes a small Unix script fixture and makes it executable with mode 0o755.
|
||||
#[cfg(unix)]
|
||||
pub fn write_executable(path: &Path, script: &str) -> io::Result<()> {
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
{
|
||||
// A sibling test can fork while a parent-owned file is open for writing.
|
||||
// CLOEXEC does not release that inherited descriptor until the sibling execs.
|
||||
let output = Command::new("/bin/sh")
|
||||
.arg("-c")
|
||||
.arg("printf '%s' \"$1\" > \"$2\"")
|
||||
.arg("write-executable-fixture")
|
||||
.arg(script)
|
||||
.arg(path)
|
||||
.output()?;
|
||||
if !output.status.success() {
|
||||
return Err(io::Error::other(format!(
|
||||
"failed to write executable fixture {}: {}",
|
||||
path.display(),
|
||||
String::from_utf8_lossy(&output.stderr),
|
||||
)));
|
||||
}
|
||||
}
|
||||
#[cfg(not(target_os = "linux"))]
|
||||
fs::write(path, script)?;
|
||||
fs::set_permissions(path, fs::Permissions::from_mode(/*mode*/ 0o755))
|
||||
}
|
||||
|
||||
/// Copies an executable fixture, preserving the source permissions like [`fs::copy`].
|
||||
pub fn copy_executable(source: &Path, destination: &Path) -> io::Result<()> {
|
||||
#[cfg(target_os = "linux")]
|
||||
{
|
||||
let output = Command::new("/bin/cp")
|
||||
.arg("--")
|
||||
.arg(source)
|
||||
.arg(destination)
|
||||
.output()?;
|
||||
if !output.status.success() {
|
||||
return Err(io::Error::other(format!(
|
||||
"failed to copy executable fixture {} to {}: {}",
|
||||
source.display(),
|
||||
destination.display(),
|
||||
String::from_utf8_lossy(&output.stderr),
|
||||
)));
|
||||
}
|
||||
// Match fs::copy even when the test process has a restrictive umask.
|
||||
fs::set_permissions(destination, fs::metadata(source)?.permissions())?;
|
||||
}
|
||||
#[cfg(not(target_os = "linux"))]
|
||||
fs::copy(source, destination)?;
|
||||
Ok(())
|
||||
}
|
||||
@@ -5,6 +5,11 @@ use std::path::PathBuf;
|
||||
|
||||
pub use runfiles;
|
||||
|
||||
mod executable;
|
||||
pub use executable::copy_executable;
|
||||
#[cfg(unix)]
|
||||
pub use executable::write_executable;
|
||||
|
||||
/// Bazel sets this when runfiles directories are disabled, which we do on all platforms for consistency.
|
||||
const RUNFILES_MANIFEST_ONLY_ENV: &str = "RUNFILES_MANIFEST_ONLY";
|
||||
|
||||
|
||||
Reference in New Issue
Block a user