From 36df9544a3a77508da9d3f89528854883b826187 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 5 Oct 2026 20:36:59 +0000 Subject: [PATCH] Clean up Unix app-server control-socket startup lock files (#51191) ## Why Unix control-socket shutdown removes the socket and rendezvous link but leaves the startup lock file behind. ## What changed Keep a removable startup lock with the control-socket guard and release the lock after publishing the rendezvous link. On cleanup, reacquire the lock and verify that the path still identifies the same file before removing it. Use nonblocking acquisition with asynchronous retries, checking file identity after acquisition to handle lock-file replacement. ## Testing Add tests for nonblocking lock contention and dangling symlink rejection, and extend the control-socket lifecycle test to verify that the lock file is removed on shutdown. GitOrigin-RevId: 469ed5269f32ae1c0655d40d28519ef5e94ba6be --- .../src/transport/unix_socket.rs | 122 +++++++++++++++++- .../src/transport/unix_socket_tests.rs | 44 +++++++ 2 files changed, 161 insertions(+), 5 deletions(-) diff --git a/codex-rs/app-server-transport/src/transport/unix_socket.rs b/codex-rs/app-server-transport/src/transport/unix_socket.rs index 3f70bcced4..afc69753b2 100644 --- a/codex-rs/app-server-transport/src/transport/unix_socket.rs +++ b/codex-rs/app-server-transport/src/transport/unix_socket.rs @@ -47,7 +47,7 @@ pub async fn start_control_socket_acceptor( daemon_shutdown_access: DaemonShutdownAccess, ) -> IoResult> { #[cfg(unix)] - let (socket_path, rendezvous_path, _startup_lock) = { + let (socket_path, rendezvous_path, mut startup_lock) = { use std::os::unix::fs::MetadataExt; if let Some(parent) = socket_path.as_path().parent() { @@ -74,9 +74,9 @@ pub async fn start_control_socket_acceptor( } codex_uds::prepare_shared_daemon_socket_directory()?; let physical_path = protected_socket_path(socket_path.as_path())?; - let lock = acquire_app_server_startup_lock(AbsolutePathBuf::from_absolute_path_checked( - physical_path.with_extension("lock"), - )?) + let lock = acquire_removable_app_server_startup_lock( + AbsolutePathBuf::from_absolute_path_checked(physical_path.with_extension("lock"))?, + ) .await?; prepare_control_socket_path(socket_path.as_path()).await?; ( @@ -95,10 +95,16 @@ pub async fn start_control_socket_acceptor( }; prepare_control_socket_path(socket_path.as_path()).await?; let listener = UnixListener::bind(socket_path.as_path()).await?; + #[cfg(unix)] + { + startup_lock.remove_on_drop = true; + } let socket_guard = ControlSocketFileGuard { socket_path, #[cfg(unix)] rendezvous_path, + #[cfg(unix)] + _startup_lock: startup_lock, #[cfg(windows)] _directory_guard: directory_guard, }; @@ -108,6 +114,8 @@ pub async fn start_control_socket_acceptor( socket_guard.socket_path.as_path(), socket_guard.rendezvous_path.as_path(), )?; + #[cfg(unix)] + socket_guard._startup_lock._file.unlock()?; info!( socket_path = %socket_guard.socket_path.display(), "app-server control socket listening" @@ -297,6 +305,10 @@ fn protected_socket_path(rendezvous_path: &Path) -> IoResult pub struct AppServerStartupLock { _file: std::fs::File, + #[cfg(unix)] + removable_path: Option, + #[cfg(unix)] + remove_on_drop: bool, } pub async fn acquire_app_server_startup_lock( @@ -313,12 +325,110 @@ pub async fn acquire_app_server_startup_lock( .write(true) .open(startup_lock_path.as_path())?; file.lock()?; - Ok(AppServerStartupLock { _file: file }) + Ok(AppServerStartupLock { + _file: file, + #[cfg(unix)] + removable_path: None, + #[cfg(unix)] + remove_on_drop: false, + }) }) .await .map_err(|err| std::io::Error::other(format!("startup lock task failed: {err}")))? } +#[cfg(unix)] +pub(super) async fn acquire_removable_app_server_startup_lock( + startup_lock_path: AbsolutePathBuf, +) -> IoResult { + let parent = startup_lock_path.as_path().parent().ok_or_else(|| { + std::io::Error::new(ErrorKind::InvalidInput, "startup lock must have a parent") + })?; + codex_uds::prepare_private_socket_directory(parent).await?; + loop { + let startup_lock_path = startup_lock_path.clone(); + let attempt = tokio::task::spawn_blocking(move || { + try_acquire_removable_app_server_startup_lock(startup_lock_path) + }) + .await + .map_err(|err| std::io::Error::other(format!("startup lock task failed: {err}")))?; + match attempt { + Ok(lock) => return Ok(lock), + Err(err) if err.kind() == ErrorKind::WouldBlock => { + tokio::time::sleep(Duration::from_millis(10)).await; + } + Err(err) => return Err(err), + } + } +} + +#[cfg(unix)] +impl AppServerStartupLock { + fn remove_file(&self) -> IoResult<()> { + let Some(path) = self.removable_path.as_ref() else { + return Ok(()); + }; + match self._file.try_lock() { + Ok(()) => {} + Err(std::fs::TryLockError::WouldBlock) => return Ok(()), + Err(std::fs::TryLockError::Error(err)) => return Err(err), + } + startup_lock_file_matches_path(&self._file, path.as_path())? + .then(|| std::fs::remove_file(path.as_path())) + .transpose() + .map(|_| ()) + } +} + +#[cfg(unix)] +pub(super) fn try_acquire_removable_app_server_startup_lock( + startup_lock_path: AbsolutePathBuf, +) -> IoResult { + let file = OpenOptions::new() + .create(true) + .truncate(false) + .read(true) + .write(true) + .open(startup_lock_path.as_path())?; + file.try_lock()?; + if !startup_lock_file_matches_path(&file, startup_lock_path.as_path())? { + return Err(ErrorKind::WouldBlock.into()); + } + Ok(AppServerStartupLock { + _file: file, + removable_path: Some(startup_lock_path), + remove_on_drop: false, + }) +} + +#[cfg(unix)] +impl Drop for AppServerStartupLock { + fn drop(&mut self) { + if self.remove_on_drop + && let Err(err) = self.remove_file() + { + warn!( + lock_path = ?self.removable_path, + %err, + "failed to remove app-server startup lock file" + ); + } + } +} + +#[cfg(unix)] +fn startup_lock_file_matches_path(file: &std::fs::File, path: &Path) -> IoResult { + use std::os::unix::fs::MetadataExt; + + let locked_metadata = file.metadata()?; + match std::fs::metadata(path) { + Ok(path_metadata) => Ok(locked_metadata.dev() == path_metadata.dev() + && locked_metadata.ino() == path_metadata.ino()), + Err(err) if err.kind() == ErrorKind::NotFound => Ok(false), + Err(err) => Err(err), + } +} + #[cfg(unix)] async fn set_control_socket_permissions(socket_path: &Path) -> IoResult<()> { use std::os::unix::fs::PermissionsExt; @@ -339,6 +449,8 @@ struct ControlSocketFileGuard { socket_path: AbsolutePathBuf, #[cfg(unix)] rendezvous_path: AbsolutePathBuf, + #[cfg(unix)] + _startup_lock: AppServerStartupLock, // Keep the directory pinned until after the socket file is removed in Drop. #[cfg(windows)] _directory_guard: std::os::windows::io::OwnedHandle, diff --git a/codex-rs/app-server-transport/src/transport/unix_socket_tests.rs b/codex-rs/app-server-transport/src/transport/unix_socket_tests.rs index e19834f8fa..3172bb2e38 100644 --- a/codex-rs/app-server-transport/src/transport/unix_socket_tests.rs +++ b/codex-rs/app-server-transport/src/transport/unix_socket_tests.rs @@ -5,6 +5,10 @@ use super::TransportEvent; use super::acquire_app_server_startup_lock; use super::app_server_control_socket_path; use super::start_control_socket_acceptor; +#[cfg(unix)] +use super::unix_socket::acquire_removable_app_server_startup_lock; +#[cfg(unix)] +use super::unix_socket::try_acquire_removable_app_server_startup_lock; use codex_app_server_protocol::JSONRPCMessage; use codex_app_server_protocol::JSONRPCNotification; use codex_core::config::find_codex_home; @@ -309,6 +313,44 @@ async fn app_server_startup_lock_serializes_waiters() { .expect("second startup lock should succeed"); } +#[cfg(unix)] +#[tokio::test] +async fn removable_startup_lock_attempt_is_nonblocking() { + let temp_dir = tempfile::TempDir::new().expect("temp dir"); + let lock_path = AbsolutePathBuf::from_absolute_path(temp_dir.path().join("socket.lock")) + .expect("lock path should resolve"); + let _lock = try_acquire_removable_app_server_startup_lock(lock_path.clone()) + .expect("first lock should succeed"); + let error = timeout( + Duration::from_secs(1), + tokio::task::spawn_blocking(move || { + try_acquire_removable_app_server_startup_lock(lock_path) + }), + ) + .await + .unwrap() + .unwrap() + .err() + .unwrap(); + assert_eq!(error.kind(), std::io::ErrorKind::WouldBlock); +} + +#[cfg(unix)] +#[tokio::test] +async fn removable_startup_lock_rejects_a_dangling_symlink_collision() { + let temp_dir = tempfile::TempDir::new().expect("temp dir"); + let lock_path = AbsolutePathBuf::from_absolute_path(temp_dir.path().join("socket.lock")) + .expect("lock path should resolve"); + std::os::unix::fs::symlink(temp_dir.path().join("missing/target"), lock_path.as_path()) + .expect("dangling symlink should be created"); + + let error = acquire_removable_app_server_startup_lock(lock_path) + .await + .err() + .unwrap(); + assert_eq!(error.kind(), std::io::ErrorKind::NotFound); +} + #[cfg(unix)] #[tokio::test] async fn control_socket_rejects_writable_parent_without_changing_permissions() { @@ -377,10 +419,12 @@ async fn control_socket_file_is_private_after_bind() { .as_path() ) ); + assert!(physical_path.with_extension("lock").exists()); shutdown_token.cancel(); accept_handle.await.expect("acceptor should join"); assert!(!physical_path.exists()); + assert!(!physical_path.with_extension("lock").exists()); assert!(std::fs::symlink_metadata(socket_path.as_path()).is_err()); // Simulate a dangling rendezvous left by an interrupted cleanup.