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

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<JoinHandle<()>> {
     #[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<std::path::PathBuf>
 
 pub struct AppServerStartupLock {
     _file: std::fs::File,
+    #[cfg(unix)]
+    removable_path: Option<AbsolutePathBuf>,
+    #[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<AppServerStartupLock> {
+    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<AppServerStartupLock> {
+    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<bool> {
+    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,

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.