From 4e6450bbfd60bdfa845182f30aaa9d6f068e8bbd Mon Sep 17 00:00:00 2001 From: zm-oai Date: Tue, 15 Sep 2026 00:26:43 +0000 Subject: [PATCH] Resume Windows sandbox registration refresh after service restarts (#45559) ## Why Registering the service-bearing package can restart the provisioning service once for each managed sandbox account. A restart can interrupt the response after runtime readiness has been revoked, preventing the refresh from completing. ## What changed - Retry registered sandbox refresh requests after response pipe disconnections, allowing up to two service restarts within the original deadline. Require a different service process ID and authenticate it against the running service before resending. - Allow refresh to resume without `ready_package` when account ownership is complete, teardown is not underway, and the existing owner, package family, live account SIDs, and settings checks pass. Runtime execution still requires readiness for the current package. - Keep authentication failures, failed writes, explicit replies, and protocol errors outside the retry path. ## Testing Add regression tests for disconnect classification, the two-restart budget, deadline expiry, and authentication failure before writing. Extend ownership tests to distinguish resumable registration from runtime readiness and reject incomplete accounts or retirement state. GitOrigin-RevId: d932f179b31752fe918297b7b12e5077c17b19e5 --- .../src/provisioning_client.rs | 39 ++++-- .../src/provisioning_client/refresh_retry.rs | 105 +++++++++++++++ .../refresh_retry_tests.rs | 121 ++++++++++++++++++ .../src/runtime_ownership.rs | 7 +- .../src/runtime_ownership_tests.rs | 4 + .../src/provisioning/registered.rs | 10 +- 6 files changed, 271 insertions(+), 15 deletions(-) create mode 100644 codex-rs/windows-sandbox-rs/src/provisioning_client/refresh_retry.rs create mode 100644 codex-rs/windows-sandbox-rs/src/provisioning_client/refresh_retry_tests.rs 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();