diff --git a/codex-rs/app-server-daemon/src/backend/pid_start.rs b/codex-rs/app-server-daemon/src/backend/pid_start.rs index 11fdd3495b..212f90e9aa 100644 --- a/codex-rs/app-server-daemon/src/backend/pid_start.rs +++ b/codex-rs/app-server-daemon/src/backend/pid_start.rs @@ -258,10 +258,6 @@ impl PidBackend { .id() .context("spawned app-server process has no pid")?; let record = match async { - #[cfg(windows)] - super::super::windows::Process::open(pid)? - .context("daemon exited during launch")? - .ensure_detached()?; let process_start_time = read_process_start_time(pid).await?; #[cfg(any(target_os = "linux", target_os = "macos"))] let process_identity = super::identity::read_process_details(pid) diff --git a/codex-rs/app-server-daemon/src/backend/windows.rs b/codex-rs/app-server-daemon/src/backend/windows.rs index 157bf3343a..57514b73cd 100644 --- a/codex-rs/app-server-daemon/src/backend/windows.rs +++ b/codex-rs/app-server-daemon/src/backend/windows.rs @@ -28,7 +28,6 @@ use windows_sys::Win32::Storage::FileSystem::LOCKFILE_FAIL_IMMEDIATELY; use windows_sys::Win32::Storage::FileSystem::LockFileEx; use windows_sys::Win32::System::JobObjects::AssignProcessToJobObject; use windows_sys::Win32::System::JobObjects::CreateJobObjectW; -use windows_sys::Win32::System::JobObjects::IsProcessInJob; use windows_sys::Win32::System::JobObjects::JOB_OBJECT_LIMIT_BREAKAWAY_OK; use windows_sys::Win32::System::JobObjects::JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; use windows_sys::Win32::System::JobObjects::JOBOBJECT_EXTENDED_LIMIT_INFORMATION; @@ -77,8 +76,9 @@ pub(crate) fn ensure_not_elevated() -> Result<()> { Ok(()) } -// Probe the actual child association: escaping an inner job can leave an outer -// job attached. Suspend the image so no application code runs before cleanup. +// Check that breakaway launch is permitted before stopping an existing daemon. +// An outer system job may remain attached; membership alone does not establish +// whether it will terminate the daemon. Suspend the probe before cleanup. pub(crate) fn ensure_detached_launch(executable: &Path) -> Result<()> { let mut child = Command::new(executable) .creation_flags(CREATE_SUSPENDED | DETACHED_PROCESS | CREATE_BREAKAWAY_FROM_JOB) @@ -87,30 +87,13 @@ pub(crate) fn ensure_detached_launch(executable: &Path) -> Result<()> { .stderr(Stdio::null()) .spawn() .context("cannot launch detached daemon; existing daemon was not stopped")?; - let mut in_job = 0; - let result = if unsafe { - IsProcessInJob( - child.as_raw_handle() as _, - /*jobhandle*/ 0, - &mut in_job, - ) - } == 0 - { - Err(io::Error::last_os_error()).context("failed to verify daemon launch capability") - } else if in_job != 0 { - Err(anyhow::anyhow!( - "host Job Object prevents daemon detachment; start from a host that allows breakaway" - )) - } else { - Ok(()) - }; child .kill() .context("failed to terminate suspended launch probe")?; child .wait() .context("failed to reap suspended launch probe")?; - result + Ok(()) } pub(super) struct Process(OwnedHandle); @@ -170,29 +153,6 @@ impl Process { } } - pub(super) fn ensure_detached(&self) -> Result<()> { - let mut in_job = 0; - if unsafe { - IsProcessInJob( - self.0.as_raw_handle() as _, - /*jobhandle*/ 0, - &mut in_job, - ) - } == 0 - { - let error = io::Error::last_os_error(); - self.terminate()?; - return Err(error).context("failed to verify daemon detachment"); - } - if in_job != 0 { - self.terminate()?; - anyhow::bail!( - "host Job Object prevents daemon detachment; start from a host that allows breakaway" - ); - } - Ok(()) - } - pub(super) fn terminate(&self) -> Result<()> { if !self.is_running()? { return Ok(()); diff --git a/codex-rs/app-server-daemon/src/backend/windows_tests.rs b/codex-rs/app-server-daemon/src/backend/windows_tests.rs index 7ce59b332b..87104adddb 100644 --- a/codex-rs/app-server-daemon/src/backend/windows_tests.rs +++ b/codex-rs/app-server-daemon/src/backend/windows_tests.rs @@ -1,7 +1,11 @@ use super::Process; use pretty_assertions::assert_eq; use std::os::windows::io::AsRawHandle; +use std::os::windows::io::FromRawHandle; +use std::os::windows::io::OwnedHandle; +use std::os::windows::process::CommandExt; use windows_sys::Win32::Foundation::ERROR_ACCESS_DENIED; +use windows_sys::Win32::System::JobObjects::IsProcessInJob; use windows_sys::Win32::System::Threading::CREATE_NO_WINDOW; use windows_sys::Win32::System::Threading::TerminateProcess; @@ -45,6 +49,154 @@ fn detached_launch_preflight_rejects_restrictive_job() { assert!(String::from_utf8_lossy(&output.stdout).contains("1 passed")); } +#[test] +fn detached_launch_preflight_allows_residual_job() { + const CHILD: &str = "CODEX_TEST_RESIDUAL_LAUNCH_JOB"; + let executable = std::env::current_exe().expect("test executable"); + if std::env::var_os(CHILD).is_some() { + let mut jobs = Vec::new(); + for inner in [false, true] { + let job = unsafe { super::CreateJobObjectW(std::ptr::null(), std::ptr::null()) }; + assert_ne!(job, 0); + let job = unsafe { OwnedHandle::from_raw_handle(job as _) }; + if inner { + let mut limits: super::JOBOBJECT_EXTENDED_LIMIT_INFORMATION = + unsafe { std::mem::zeroed() }; + limits.BasicLimitInformation.LimitFlags = super::JOB_OBJECT_LIMIT_BREAKAWAY_OK; + assert_ne!( + unsafe { + super::SetInformationJobObject( + job.as_raw_handle() as _, + super::JobObjectExtendedLimitInformation, + (&limits as *const super::JOBOBJECT_EXTENDED_LIMIT_INFORMATION).cast(), + std::mem::size_of_val(&limits) as u32, + ) + }, + 0 + ); + } + assert_ne!( + unsafe { + super::AssignProcessToJobObject( + job.as_raw_handle() as _, + super::GetCurrentProcess(), + ) + }, + 0 + ); + jobs.push(job); + } + + let mut probe = std::process::Command::new(&executable) + .creation_flags( + super::CREATE_SUSPENDED + | super::DETACHED_PROCESS + | super::CREATE_BREAKAWAY_FROM_JOB, + ) + .spawn() + .expect("breakaway probe"); + let mut in_outer = 0; + let mut in_inner = 0; + let outer_checked = unsafe { + IsProcessInJob( + probe.as_raw_handle() as _, + jobs[0].as_raw_handle() as _, + &mut in_outer, + ) + }; + let inner_checked = unsafe { + IsProcessInJob( + probe.as_raw_handle() as _, + jobs[1].as_raw_handle() as _, + &mut in_inner, + ) + }; + probe.kill().expect("terminate probe"); + probe.wait().expect("reap probe"); + assert_ne!(outer_checked, 0); + assert_ne!(inner_checked, 0); + assert_ne!(in_outer, 0, "probe must retain its outer job"); + assert_eq!(in_inner, 0, "probe must leave its inner job"); + super::ensure_detached_launch(&executable).expect("residual job must be accepted"); + + if let Err(err) = super::ensure_not_elevated() { + assert!(err.to_string().contains("non-elevated terminal")); + return; + } + let temp = tempfile::tempdir().expect("temp dir"); + let script = temp.path().join("codex.cmd"); + std::fs::write( + &script, + "@echo off\r\nif \"%3\"==\"--help\" exit /b 1\r\necho ready > \"%~dp0ready\"\r\nfor /l %%i in (1,1,10000000) do @rem\r\n", + ) + .expect("write daemon fixture"); + let backend = crate::backend::pid::PidBackend::new( + script, + temp.path().join("state").join("daemon.pid"), + /*remote_control_enabled*/ false, + ); + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("runtime"); + let pid = runtime + .block_on(backend.start()) + .expect("daemon launch") + .expect("new daemon pid"); + let process = Process::open(pid) + .expect("open daemon") + .expect("live daemon"); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while !temp.path().join("ready").exists() && std::time::Instant::now() < deadline { + std::thread::sleep(std::time::Duration::from_millis(50)); + } + let ready = temp.path().join("ready").exists(); + let outer_checked = unsafe { + IsProcessInJob( + process.0.as_raw_handle() as _, + jobs[0].as_raw_handle() as _, + &mut in_outer, + ) + }; + let inner_checked = unsafe { + IsProcessInJob( + process.0.as_raw_handle() as _, + jobs[1].as_raw_handle() as _, + &mut in_inner, + ) + }; + process.terminate().expect("terminate daemon fixture"); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while process.is_running().expect("daemon liveness") && std::time::Instant::now() < deadline + { + std::thread::sleep(std::time::Duration::from_millis(50)); + } + assert!(!process.is_running().expect("daemon liveness")); + assert!(ready, "daemon fixture did not start"); + assert_ne!(outer_checked, 0); + assert_ne!(inner_checked, 0); + assert_ne!(in_outer, 0, "daemon must retain its outer job"); + assert_eq!(in_inner, 0, "daemon must leave its inner job"); + return; + } + let output = std::process::Command::new(executable) + .args([ + "--exact", + "backend::windows::tests::detached_launch_preflight_allows_residual_job", + "--nocapture", + ]) + .env(CHILD, "1") + .output() + .expect("isolated job test"); + assert!( + output.status.success(), + "{}\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!(String::from_utf8_lossy(&output.stdout).contains("1 passed")); +} + #[tokio::test] async fn identity_queries_do_not_require_termination_access() { let mut child = tokio::process::Command::new("powershell.exe")