mirror of
https://github.com/openai/codex.git
synced 2026-05-23 20:44:50 +00:00
Follow-up to https://github.com/openai/codex/pull/18178, where we called out enabling the await-holding lint as a follow-up. The long-term goal is to enable Clippy coverage for async guards held across awaits. This PR is intentionally only the first, low-risk cleanup pass: it narrows obvious lock guard lifetimes and leaves `codex-rs/Cargo.toml` unchanged so the lint is not enabled until the remaining cases are fixed or explicitly justified. It intentionally leaves the active-turn/turn-state locking pattern alone because those checks and mutations need to stay atomic. ## Common fixes used here These are the main patterns reviewers should expect in this PR, and they are also the patterns to reach for when fixing future `await_holding_*` findings: - **Scope the guard to the synchronous work.** If the code only needs data from a locked value, move the lock into a small block, clone or compute the needed values, and do the later `.await` after the block. - **Use direct one-line mutations when there is no later await.** Cases like `map.lock().await.remove(&id)` are acceptable when the guard is only needed for that single mutation and the statement ends before any async work. - **Drain or clone work out of the lock before notifying or awaiting.** For example, the JS REPL drains pending exec senders into a local vector and the websocket writer clones buffered envelopes before it serializes or sends them. - **Use a `Semaphore` only when serialization is intentional across async work.** The test serialization guards intentionally span awaited setup or execution, so using a semaphore communicates "one at a time" without holding a mutex guard. - **Remove the mutex when there is only one owner.** The PTY stdin writer task owns `stdin` directly; the old `Arc<Mutex<_>>` did not protect shared access because nothing else had access to the writer. - **Do not split locks that protect an atomic invariant.** This PR deliberately leaves active-turn/turn-state paths alone because those checks and mutations need to stay atomic. Those cases should be fixed separately with a design change or documented with `#[expect]`. ## What changed - Narrow scoped async mutex guards in app-server, JS REPL, network approval, remote-control websocket, and the RMCP test server. - Replace test-only async mutex serialization guards with semaphores where the guard intentionally lives across async work. - Let the PTY pipe writer task own stdin directly instead of wrapping it in an async mutex. ## Verification - `just fix -p codex-core -p codex-app-server -p codex-rmcp-client -p codex-shell-escalation -p codex-utils-pty -p codex-utils-readiness` - `just clippy -p codex-core` - `cargo test -p codex-core -p codex-app-server -p codex-rmcp-client -p codex-shell-escalation -p codex-utils-pty -p codex-utils-readiness` was run; the app-server suite passed, and `codex-core` failed in the local sandbox on six otel approval tests plus `suite::user_shell_cmd::user_shell_command_does_not_set_network_sandbox_env_var`, which appear to depend on local command approval/default rules and `CODEX_SANDBOX_NETWORK_DISABLED=1` in this environment.
290 lines
7.8 KiB
Rust
290 lines
7.8 KiB
Rust
use std::collections::HashMap;
|
|
use std::io;
|
|
use std::io::ErrorKind;
|
|
use std::path::Path;
|
|
use std::process::Stdio;
|
|
use std::sync::Arc;
|
|
use std::sync::Mutex as StdMutex;
|
|
use std::sync::atomic::AtomicBool;
|
|
|
|
use anyhow::Result;
|
|
use tokio::io::AsyncRead;
|
|
use tokio::io::AsyncReadExt;
|
|
use tokio::io::AsyncWriteExt;
|
|
use tokio::io::BufReader;
|
|
use tokio::process::Command;
|
|
use tokio::sync::mpsc;
|
|
use tokio::sync::oneshot;
|
|
use tokio::task::JoinHandle;
|
|
|
|
use crate::process::ChildTerminator;
|
|
use crate::process::ProcessHandle;
|
|
use crate::process::SpawnedProcess;
|
|
|
|
#[cfg(target_os = "linux")]
|
|
use libc;
|
|
|
|
struct PipeChildTerminator {
|
|
#[cfg(windows)]
|
|
pid: u32,
|
|
#[cfg(unix)]
|
|
process_group_id: u32,
|
|
}
|
|
|
|
impl ChildTerminator for PipeChildTerminator {
|
|
fn kill(&mut self) -> io::Result<()> {
|
|
#[cfg(unix)]
|
|
{
|
|
crate::process_group::kill_process_group(self.process_group_id)
|
|
}
|
|
|
|
#[cfg(windows)]
|
|
{
|
|
kill_process(self.pid)
|
|
}
|
|
|
|
#[cfg(not(any(unix, windows)))]
|
|
{
|
|
Ok(())
|
|
}
|
|
}
|
|
}
|
|
|
|
#[cfg(windows)]
|
|
fn kill_process(pid: u32) -> io::Result<()> {
|
|
unsafe {
|
|
let handle = winapi::um::processthreadsapi::OpenProcess(
|
|
winapi::um::winnt::PROCESS_TERMINATE,
|
|
0,
|
|
pid,
|
|
);
|
|
if handle.is_null() {
|
|
return Err(io::Error::last_os_error());
|
|
}
|
|
let success = winapi::um::processthreadsapi::TerminateProcess(handle, 1);
|
|
let err = io::Error::last_os_error();
|
|
winapi::um::handleapi::CloseHandle(handle);
|
|
if success == 0 { Err(err) } else { Ok(()) }
|
|
}
|
|
}
|
|
|
|
async fn read_output_stream<R>(mut reader: R, output_tx: mpsc::Sender<Vec<u8>>)
|
|
where
|
|
R: AsyncRead + Unpin,
|
|
{
|
|
let mut buf = vec![0u8; 8_192];
|
|
loop {
|
|
match reader.read(&mut buf).await {
|
|
Ok(0) => break,
|
|
Ok(n) => {
|
|
let _ = output_tx.send(buf[..n].to_vec()).await;
|
|
}
|
|
Err(ref e) if e.kind() == ErrorKind::Interrupted => continue,
|
|
Err(_) => break,
|
|
}
|
|
}
|
|
}
|
|
|
|
#[derive(Clone, Copy)]
|
|
enum PipeStdinMode {
|
|
Piped,
|
|
Null,
|
|
}
|
|
|
|
async fn spawn_process_with_stdin_mode(
|
|
program: &str,
|
|
args: &[String],
|
|
cwd: &Path,
|
|
env: &HashMap<String, String>,
|
|
arg0: &Option<String>,
|
|
stdin_mode: PipeStdinMode,
|
|
inherited_fds: &[i32],
|
|
) -> Result<SpawnedProcess> {
|
|
if program.is_empty() {
|
|
anyhow::bail!("missing program for pipe spawn");
|
|
}
|
|
|
|
#[cfg(not(unix))]
|
|
let _ = inherited_fds;
|
|
|
|
let mut command = Command::new(program);
|
|
#[cfg(unix)]
|
|
if let Some(arg0) = arg0 {
|
|
command.arg0(arg0);
|
|
}
|
|
#[cfg(target_os = "linux")]
|
|
let parent_pid = unsafe { libc::getpid() };
|
|
#[cfg(unix)]
|
|
let inherited_fds = inherited_fds.to_vec();
|
|
#[cfg(unix)]
|
|
unsafe {
|
|
command.pre_exec(move || {
|
|
crate::process_group::detach_from_tty()?;
|
|
#[cfg(target_os = "linux")]
|
|
crate::process_group::set_parent_death_signal(parent_pid)?;
|
|
crate::pty::close_inherited_fds_except(&inherited_fds);
|
|
Ok(())
|
|
});
|
|
}
|
|
#[cfg(not(unix))]
|
|
let _ = arg0;
|
|
command.current_dir(cwd);
|
|
command.env_clear();
|
|
for (key, value) in env {
|
|
command.env(key, value);
|
|
}
|
|
for arg in args {
|
|
command.arg(arg);
|
|
}
|
|
match stdin_mode {
|
|
PipeStdinMode::Piped => {
|
|
command.stdin(Stdio::piped());
|
|
}
|
|
PipeStdinMode::Null => {
|
|
command.stdin(Stdio::null());
|
|
}
|
|
}
|
|
command.stdout(Stdio::piped());
|
|
command.stderr(Stdio::piped());
|
|
|
|
let mut child = command.spawn()?;
|
|
let pid = child
|
|
.id()
|
|
.ok_or_else(|| io::Error::other("missing child pid"))?;
|
|
#[cfg(unix)]
|
|
let process_group_id = pid;
|
|
|
|
let stdin = child.stdin.take();
|
|
let stdout = child.stdout.take();
|
|
let stderr = child.stderr.take();
|
|
|
|
let (writer_tx, mut writer_rx) = mpsc::channel::<Vec<u8>>(128);
|
|
let (stdout_tx, stdout_rx) = mpsc::channel::<Vec<u8>>(128);
|
|
let (stderr_tx, stderr_rx) = mpsc::channel::<Vec<u8>>(128);
|
|
let writer_handle = if let Some(stdin) = stdin {
|
|
tokio::spawn(async move {
|
|
let mut writer = stdin;
|
|
while let Some(bytes) = writer_rx.recv().await {
|
|
let _ = writer.write_all(&bytes).await;
|
|
let _ = writer.flush().await;
|
|
}
|
|
})
|
|
} else {
|
|
drop(writer_rx);
|
|
tokio::spawn(async {})
|
|
};
|
|
|
|
let stdout_handle = stdout.map(|stdout| {
|
|
let stdout_tx = stdout_tx.clone();
|
|
tokio::spawn(async move {
|
|
read_output_stream(BufReader::new(stdout), stdout_tx).await;
|
|
})
|
|
});
|
|
let stderr_handle = stderr.map(|stderr| {
|
|
let stderr_tx = stderr_tx.clone();
|
|
tokio::spawn(async move {
|
|
read_output_stream(BufReader::new(stderr), stderr_tx).await;
|
|
})
|
|
});
|
|
let mut reader_abort_handles = Vec::new();
|
|
if let Some(handle) = stdout_handle.as_ref() {
|
|
reader_abort_handles.push(handle.abort_handle());
|
|
}
|
|
if let Some(handle) = stderr_handle.as_ref() {
|
|
reader_abort_handles.push(handle.abort_handle());
|
|
}
|
|
let reader_handle = tokio::spawn(async move {
|
|
if let Some(handle) = stdout_handle {
|
|
let _ = handle.await;
|
|
}
|
|
if let Some(handle) = stderr_handle {
|
|
let _ = handle.await;
|
|
}
|
|
});
|
|
|
|
let (exit_tx, exit_rx) = oneshot::channel::<i32>();
|
|
let exit_status = Arc::new(AtomicBool::new(false));
|
|
let wait_exit_status = Arc::clone(&exit_status);
|
|
let exit_code = Arc::new(StdMutex::new(None));
|
|
let wait_exit_code = Arc::clone(&exit_code);
|
|
let wait_handle: JoinHandle<()> = tokio::spawn(async move {
|
|
let code = match child.wait().await {
|
|
Ok(status) => status.code().unwrap_or(-1),
|
|
Err(_) => -1,
|
|
};
|
|
wait_exit_status.store(true, std::sync::atomic::Ordering::SeqCst);
|
|
if let Ok(mut guard) = wait_exit_code.lock() {
|
|
*guard = Some(code);
|
|
}
|
|
let _ = exit_tx.send(code);
|
|
});
|
|
|
|
let handle = ProcessHandle::new(
|
|
writer_tx,
|
|
Box::new(PipeChildTerminator {
|
|
#[cfg(windows)]
|
|
pid,
|
|
#[cfg(unix)]
|
|
process_group_id,
|
|
}),
|
|
reader_handle,
|
|
reader_abort_handles,
|
|
writer_handle,
|
|
wait_handle,
|
|
exit_status,
|
|
exit_code,
|
|
/*pty_handles*/ None,
|
|
);
|
|
|
|
Ok(SpawnedProcess {
|
|
session: handle,
|
|
stdout_rx,
|
|
stderr_rx,
|
|
exit_rx,
|
|
})
|
|
}
|
|
|
|
/// Spawn a process using regular pipes (no PTY), returning handles for stdin, split output, and exit.
|
|
pub async fn spawn_process(
|
|
program: &str,
|
|
args: &[String],
|
|
cwd: &Path,
|
|
env: &HashMap<String, String>,
|
|
arg0: &Option<String>,
|
|
) -> Result<SpawnedProcess> {
|
|
spawn_process_with_stdin_mode(program, args, cwd, env, arg0, PipeStdinMode::Piped, &[]).await
|
|
}
|
|
|
|
/// Spawn a process using regular pipes, but close stdin immediately.
|
|
pub async fn spawn_process_no_stdin(
|
|
program: &str,
|
|
args: &[String],
|
|
cwd: &Path,
|
|
env: &HashMap<String, String>,
|
|
arg0: &Option<String>,
|
|
) -> Result<SpawnedProcess> {
|
|
spawn_process_no_stdin_with_inherited_fds(program, args, cwd, env, arg0, &[]).await
|
|
}
|
|
|
|
/// Spawn a process using regular pipes, close stdin immediately, and preserve
|
|
/// selected inherited file descriptors across exec on Unix.
|
|
pub async fn spawn_process_no_stdin_with_inherited_fds(
|
|
program: &str,
|
|
args: &[String],
|
|
cwd: &Path,
|
|
env: &HashMap<String, String>,
|
|
arg0: &Option<String>,
|
|
inherited_fds: &[i32],
|
|
) -> Result<SpawnedProcess> {
|
|
spawn_process_with_stdin_mode(
|
|
program,
|
|
args,
|
|
cwd,
|
|
env,
|
|
arg0,
|
|
PipeStdinMode::Null,
|
|
inherited_fds,
|
|
)
|
|
.await
|
|
}
|