Explicitly unlock daemon PID reservation probes on Unix (#57512)

## Why

A concurrent fork can inherit a PID reservation probe's file descriptor and keep its lock alive after the probe closes it. This can make the app-server daemon appear to be starting even when no live startup reservation exists.

## What changed

Explicitly release the probe's lock on Unix before returning that no reservation is active, and propagate unlock failures.

## Testing

Add a regression test using a duplicated file handle to simulate fork inheritance and verify that the probe leaves the daemon inactive. Extend coverage to confirm that a held reservation still reports `Starting` when the PID file is missing, and include pruning results in release-retirement assertion failures.

GitOrigin-RevId: ecc58df1fe5cadecb6ed658352f1300c39237358
This commit is contained in:
jif
2026-10-27 16:50:34 +00:00
committed by copyberry
parent 73bdd06fed
commit 5a361b5596
3 changed files with 66 additions and 3 deletions
+21 -1
View File
@@ -1,4 +1,5 @@
//! PID reservations serialize detached launches; process identities protect stale-record cleanup.
//! Missing-PID probes explicitly unlock so concurrent forks cannot retain their transient reservations.
#[path = "pid_identity.rs"]
mod identity;
@@ -703,7 +704,26 @@ async fn reservation_lock_is_active(path: &Path) -> Result<bool> {
.with_context(|| format!("failed to inspect pid lock file {}", path.display()));
}
};
Ok(!try_lock_file(&file)?)
probe_reservation_lock(file)
}
#[cfg(any(unix, windows))]
fn probe_reservation_lock(file: fs::File) -> Result<bool> {
if !try_lock_file(&file)? {
return Ok(true);
}
#[cfg(unix)]
{
use std::os::fd::AsRawFd;
// A concurrent fork can inherit this descriptor before it closes. Release the
// probe's lock explicitly so the child cannot look like a live reservation.
if unsafe { libc::flock(file.as_raw_fd(), libc::LOCK_UN) } != 0 {
return Err(std::io::Error::last_os_error())
.context("failed to unlock pid reservation probe");
}
}
Ok(false)
}
#[cfg(not(any(unix, windows)))]
@@ -67,6 +67,17 @@ async fn locked_empty_pid_file_is_treated_as_active_reservation() {
PidFileState::Starting
);
assert!(pid_file.exists());
tokio::fs::remove_file(&pid_file)
.await
.expect("remove pid file");
assert_eq!(
backend
.read_pid_file_state()
.await
.expect("read missing pid"),
PidFileState::Starting
);
}
#[tokio::test]
@@ -89,6 +100,38 @@ async fn unlocked_empty_pid_file_is_treated_as_stale_reservation() {
assert!(!pid_file.exists());
}
#[cfg(unix)]
#[tokio::test]
async fn reservation_probe_releases_lock_with_duplicated_handle() {
let temp_dir = TempDir::new().expect("temp dir");
let backend = PidBackend::new(
temp_dir.path().join("codex"),
temp_dir.path().join("app-server.pid"),
/*remote_control_enabled*/ false,
);
let file = std::fs::OpenOptions::new()
.create(true)
.truncate(false)
.write(true)
.open(&backend.lock_file)
.expect("open pid lock file");
// Share the open file description as fork would, without inheriting
// unrelated descriptors from other tests.
let inherited = file.try_clone().expect("clone pid lock file");
let probe = super::probe_reservation_lock(tokio::fs::File::from_std(file));
let running = backend.is_starting_or_running().await;
drop(inherited);
assert_eq!(
(
probe.expect("probe reservation"),
running.expect("read daemon status"),
),
(false, false),
);
}
#[tokio::test]
async fn stop_waits_for_live_reservation_to_resolve() {
let temp_dir = TempDir::new().expect("temp dir");
@@ -120,8 +120,8 @@ async fn seeds_full_package() {
use std::os::unix::fs::PermissionsExt;
let read_only = old.join("codex-resources/nested");
std::fs::set_permissions(&read_only, std::fs::Permissions::from_mode(0o555)).unwrap();
let _ = release_pruning::prune(&daemon, &root, &settings).await;
assert!(!old.exists());
let result = release_pruning::prune(&daemon, &root, &settings).await;
assert!(!old.exists(), "release was not retired: {result:#?}");
for retained in [&selected, &previous, &outside, &linked] {
assert!(retained.join("codex-resources/nested/runtime").is_file());
}