diff --git a/codex-rs/windows-sandbox-rs/src/provisioning_client.rs b/codex-rs/windows-sandbox-rs/src/provisioning_client.rs index c602092850..de1823aef9 100644 --- a/codex-rs/windows-sandbox-rs/src/provisioning_client.rs +++ b/codex-rs/windows-sandbox-rs/src/provisioning_client.rs @@ -35,6 +35,7 @@ use windows_sys::Win32::System::Services; const PROVISIONING_TIMEOUT: Duration = Duration::from_secs(120); mod group_change; +mod refresh_retry; impl WindowsSandboxProvisioningSettings { /// Derives the full firewall settings using the same environment handling as elevated setup. @@ -203,6 +204,12 @@ fn send_service_request( let Some(mut pipe) = connect(deadline)? else { return Ok(crate::SandboxProvisioningResponse::Unavailable); }; + if let crate::ProvisioningMessage::ProvisionSandboxRequest { payload } = &request.message + && payload.registered_core + && payload.refresh_only + { + return refresh_retry::send(pipe, request, deadline); + } let response = match exchange_request(&mut pipe, request, deadline) { Ok(response) => response, Err(error) @@ -241,6 +248,13 @@ fn exchange_request( .context("authenticate provisioning pipe server")?; crate::write_provisioning_frame(&mut *pipe, request) .context("send sandbox provisioning request")?; + read_response(pipe, deadline) +} + +fn read_response( + pipe: &mut File, + deadline: Instant, +) -> anyhow::Result { crate::framed_io::wait_for_complete_frame(pipe, deadline) .context("wait for sandbox provisioning response")?; let response = crate::read_provisioning_frame(pipe) @@ -304,12 +318,27 @@ fn connect(deadline: Instant) -> anyhow::Result> { } } -fn verify_server(pipe: HANDLE) -> anyhow::Result<()> { +fn pipe_server_process_id(pipe: HANDLE) -> anyhow::Result { let mut pipe_process_id = 0; if unsafe { GetNamedPipeServerProcessId(pipe, &mut pipe_process_id) } == 0 { return Err(io::Error::last_os_error()).context("identify provisioning pipe server"); } + Ok(pipe_process_id) +} +fn verify_server(pipe: HANDLE) -> anyhow::Result { + let pipe_process_id = pipe_server_process_id(pipe)?; + let status = query_service_status()?; + if status.dwCurrentState != Services::SERVICE_RUNNING + || status.dwProcessId == 0 + || status.dwProcessId != pipe_process_id + { + bail!("the provisioning pipe server does not match the running service"); + } + Ok(pipe_process_id) +} + +fn query_service_status() -> anyhow::Result { let manager = unsafe { Services::OpenSCManagerW(ptr::null(), ptr::null(), Services::SC_MANAGER_CONNECT) }; if manager == 0 { @@ -344,13 +373,7 @@ fn verify_server(pipe: HANDLE) -> anyhow::Result<()> { { return Err(io::Error::last_os_error()).context("query sandbox provisioning service"); } - if status.dwCurrentState != Services::SERVICE_RUNNING - || status.dwProcessId == 0 - || status.dwProcessId != pipe_process_id - { - bail!("the provisioning pipe server does not match the running service"); - } - Ok(()) + Ok(status) } struct ServiceHandle(SC_HANDLE); diff --git a/codex-rs/windows-sandbox-rs/src/provisioning_client/refresh_retry.rs b/codex-rs/windows-sandbox-rs/src/provisioning_client/refresh_retry.rs new file mode 100644 index 0000000000..c4ea23369c --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/provisioning_client/refresh_retry.rs @@ -0,0 +1,105 @@ +//! Resume gated registration refresh only after an authenticated service restart. +//! Only response-side pipe disconnections qualify, never failed authentication, +//! failed request writes, explicit replies, or protocol errors. + +use std::fs::File; +use std::io; +use std::os::windows::io::AsRawHandle; +use std::time::Duration; +use std::time::Instant; + +use anyhow::Context; +use windows_sys::Win32::Foundation::ERROR_BROKEN_PIPE; +use windows_sys::Win32::Foundation::ERROR_NO_DATA; +use windows_sys::Win32::Foundation::ERROR_PIPE_NOT_CONNECTED; +use windows_sys::Win32::Foundation::HANDLE; +use windows_sys::Win32::System::Services::SERVICE_START_PENDING; + +use crate::FramedProvisioningMessage; +use crate::SandboxProvisioningResponse; + +// Registering the service-bearing package can restart the service once for each +// of the two managed accounts. Keep both retries inside the original deadline. +const MAX_SERVICE_RESTARTS: usize = 2; +const RECONNECT_POLL_INTERVAL: Duration = Duration::from_millis(25); + +pub(super) fn send( + mut pipe: File, + request: &FramedProvisioningMessage, + deadline: Instant, +) -> anyhow::Result { + let mut restarts_remaining = MAX_SERVICE_RESTARTS; + loop { + remaining_time(deadline)?; + let server_pid = super::verify_server(pipe.as_raw_handle() as HANDLE) + .context("authenticate provisioning pipe server")?; + crate::write_provisioning_frame(&mut pipe, request) + .context("send sandbox provisioning request")?; + let response = super::read_response(&mut pipe, deadline); + if !take_restart(&response, &mut restarts_remaining, deadline) { + return response; + } + drop(pipe); + pipe = connect_after_restart(server_pid, deadline)?; + } +} + +fn connect_after_restart(previous_pid: u32, deadline: Instant) -> anyhow::Result { + loop { + remaining_time(deadline)?; + if let Some(pipe) = super::connect(deadline)? + && super::pipe_server_process_id(pipe.as_raw_handle() as HANDLE)? != previous_pid + // The service creates its listener before reporting RUNNING. + && super::query_service_status()?.dwCurrentState != SERVICE_START_PENDING + { + // The caller authenticates this new PID against the running SCM + // service before writing. A same-PID reuse is conservatively refused. + return Ok(pipe); + } + std::thread::sleep(remaining_time(deadline)?.min(RECONNECT_POLL_INTERVAL)); + } +} + +fn remaining_time(deadline: Instant) -> io::Result { + let remaining = deadline.saturating_duration_since(Instant::now()); + if remaining.is_zero() { + return Err(io::Error::new( + io::ErrorKind::TimedOut, + "timed out waiting for restarted sandbox provisioning service", + )); + } + Ok(remaining) +} + +fn take_restart( + response: &anyhow::Result, + restarts_remaining: &mut usize, + deadline: Instant, +) -> bool { + let Err(error) = response else { + return false; + }; + if *restarts_remaining == 0 + || Instant::now() >= deadline + || !error.downcast_ref::().is_some_and(|error| { + matches!( + error.kind(), + io::ErrorKind::UnexpectedEof | io::ErrorKind::BrokenPipe + ) || matches!( + error.raw_os_error(), + Some(code) + if code == ERROR_BROKEN_PIPE as i32 + || code == ERROR_NO_DATA as i32 + || code == ERROR_PIPE_NOT_CONNECTED as i32 + ) + }) + { + return false; + } + *restarts_remaining -= 1; + true +} + +#[cfg(test)] +#[path = "refresh_retry_tests.rs"] +mod tests; diff --git a/codex-rs/windows-sandbox-rs/src/provisioning_client/refresh_retry_tests.rs b/codex-rs/windows-sandbox-rs/src/provisioning_client/refresh_retry_tests.rs new file mode 100644 index 0000000000..ab553e75df --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/provisioning_client/refresh_retry_tests.rs @@ -0,0 +1,121 @@ +//! Regression coverage for reply classification and the shared restart deadline/budget. + +use super::*; +use pretty_assertions::assert_eq; + +#[test] +fn authentication_failure_cannot_write_or_replay_the_request() -> anyhow::Result<()> { + let pipe = tempfile::tempfile()?; + let observation = pipe.try_clone()?; + let request = FramedProvisioningMessage { + version: crate::PROVISIONING_PROTOCOL_VERSION, + message: crate::ProvisioningMessage::ProvisionSandboxRequest { + payload: crate::SandboxProvisioningRequest { + codex_home: r"C:\test-home".to_owned(), + registered_core: true, + refresh_only: true, + settings: crate::WindowsSandboxProvisioningSettings { + proxy_ports: Vec::new(), + allow_local_binding: false, + }, + listeners: crate::WindowsSandboxProxyListeners::default(), + }, + }, + }; + let error = send(pipe, &request, Instant::now() + Duration::from_secs(60)).unwrap_err(); + assert_eq!(error.to_string(), "authenticate provisioning pipe server"); + assert_eq!(observation.metadata()?.len(), 0); + Ok(()) +} + +#[test] +fn permits_two_response_disconnects_but_not_a_third() { + let deadline = Instant::now() + Duration::from_secs(60); + let response = Err(io::Error::from(io::ErrorKind::UnexpectedEof).into()); + let mut remaining = MAX_SERVICE_RESTARTS; + let decisions = (0..3) + .map(|_| take_restart(&response, &mut remaining, deadline)) + .collect::>(); + assert_eq!((decisions, remaining), (vec![true, true, false], 0)); +} + +#[test] +fn accepts_only_response_pipe_disconnect_errors() { + let deadline = Instant::now() + Duration::from_secs(60); + for error in [ + io::Error::from(io::ErrorKind::UnexpectedEof), + io::Error::from(io::ErrorKind::BrokenPipe), + io::Error::from_raw_os_error(ERROR_BROKEN_PIPE as i32), + io::Error::from_raw_os_error(ERROR_NO_DATA as i32), + io::Error::from_raw_os_error(ERROR_PIPE_NOT_CONNECTED as i32), + ] { + let response = Err(anyhow::Error::from(error).context("read response")); + let mut remaining = MAX_SERVICE_RESTARTS; + assert_eq!( + (take_restart(&response, &mut remaining, deadline), remaining), + (true, 1) + ); + } +} + +#[test] +fn explicit_replies_never_consume_a_restart() { + let deadline = Instant::now() + Duration::from_secs(60); + for response in [ + SandboxProvisioningResponse::Ok, + SandboxProvisioningResponse::Unavailable, + SandboxProvisioningResponse::Error { + message: "refused".to_owned(), + }, + SandboxProvisioningResponse::Error { + message: crate::SANDBOX_GROUP_CHANGED.to_owned(), + }, + ] { + let mut remaining = MAX_SERVICE_RESTARTS; + assert_eq!( + ( + take_restart(&Ok(response), &mut remaining, deadline), + remaining + ), + (false, MAX_SERVICE_RESTARTS) + ); + } +} + +#[test] +fn timeout_permission_and_protocol_errors_do_not_consume_a_restart() { + let deadline = Instant::now() + Duration::from_secs(60); + for error in [ + io::Error::from(io::ErrorKind::TimedOut).into(), + io::Error::from(io::ErrorKind::PermissionDenied).into(), + io::Error::from(io::ErrorKind::InvalidData).into(), + anyhow::anyhow!("unexpected sandbox provisioning response message"), + serde_json::from_str::("not JSON") + .unwrap_err() + .into(), + ] { + let mut remaining = MAX_SERVICE_RESTARTS; + assert_eq!( + ( + take_restart(&Err(error), &mut remaining, deadline), + remaining + ), + (false, MAX_SERVICE_RESTARTS) + ); + } +} + +#[test] +fn disconnect_after_original_deadline_is_not_retried() { + let deadline = Instant::now(); + let response = Err(io::Error::from(io::ErrorKind::UnexpectedEof).into()); + let mut remaining = MAX_SERVICE_RESTARTS; + assert_eq!( + (take_restart(&response, &mut remaining, deadline), remaining), + (false, MAX_SERVICE_RESTARTS) + ); + assert_eq!( + remaining_time(deadline).unwrap_err().kind(), + io::ErrorKind::TimedOut + ); +} diff --git a/codex-rs/windows-sandbox-rs/src/runtime_ownership.rs b/codex-rs/windows-sandbox-rs/src/runtime_ownership.rs index 7cba7bfd4d..eea65af888 100644 --- a/codex-rs/windows-sandbox-rs/src/runtime_ownership.rs +++ b/codex-rs/windows-sandbox-rs/src/runtime_ownership.rs @@ -80,8 +80,13 @@ pub struct RuntimeRegistration { impl RuntimeRegistration { pub fn ready_for_package(&self, full_name: &str) -> bool { + self.can_resume_registration() && self.ready_package.as_deref() == Some(full_name) + } + + /// Complete account ownership allows registration to resume, not runtime execution. + /// Callers must still authenticate the owner and verify the live account SIDs/settings. + pub fn can_resume_registration(&self) -> bool { self.retiring.is_none() - && self.ready_package.as_deref() == Some(full_name) && self.accounts.len() == 2 && self.accounts[0].account != self.accounts[1].account && self diff --git a/codex-rs/windows-sandbox-rs/src/runtime_ownership_tests.rs b/codex-rs/windows-sandbox-rs/src/runtime_ownership_tests.rs index f9bfed8151..9ef77269af 100644 --- a/codex-rs/windows-sandbox-rs/src/runtime_ownership_tests.rs +++ b/codex-rs/windows-sandbox-rs/src/runtime_ownership_tests.rs @@ -52,10 +52,12 @@ fn ready_runtime() -> RuntimeRegistration { fn readiness_requires_a_receipt_for_the_current_package_version() { let mut runtime = ready_runtime(); assert!(runtime.ready_for_package(READY_PACKAGE)); + assert!(runtime.can_resume_registration()); assert!(!runtime.ready_for_package("OpenAI.Codex_2.0.0.0_arm64__publisher")); runtime.ready_package = None; assert!(!runtime.ready_for_package(READY_PACKAGE)); + assert!(runtime.can_resume_registration()); } #[test] @@ -78,6 +80,7 @@ fn readiness_requires_both_distinct_account_receipts() { extra_account, ] { assert!(!runtime.ready_for_package(READY_PACKAGE)); + assert!(!runtime.can_resume_registration()); } } @@ -86,6 +89,7 @@ fn readiness_is_revoked_by_the_retirement_fence() { let mut runtime = ready_runtime(); runtime.retiring = Some("one-cleanup-generation".into()); assert!(!runtime.ready_for_package(READY_PACKAGE)); + assert!(!runtime.can_resume_registration()); } #[test] diff --git a/codex-rs/windows-sandbox-service/src/provisioning/registered.rs b/codex-rs/windows-sandbox-service/src/provisioning/registered.rs index 5715042e91..13c53721d1 100644 --- a/codex-rs/windows-sandbox-service/src/provisioning/registered.rs +++ b/codex-rs/windows-sandbox-service/src/provisioning/registered.rs @@ -43,17 +43,15 @@ pub(super) fn run( let previous = crate::installation_record::load_runtime()? .context("registration refresh requires completed setup")?; let runtime = previous.runtime()?; - let ready = runtime - .ready_package - .as_deref() - .context("registration refresh requires a ready runtime")?; + // Windows may restart this service during registration, after readiness was + // revoked. Resume only the same provisioned accounts; never repair setup. ensure!( previous.user_sid == identity.user_sid && previous.codex_home == identity.codex_home && crate::installation_record::is_current_package_family(&previous)? - && runtime.ready_for_package(ready) + && runtime.can_resume_registration() && setup_complete, - "registration refresh requires unchanged, ready sandbox ownership and settings" + "registration refresh requires unchanged sandbox ownership and settings" ); for entry in &runtime.accounts { let account = entry.account.username();