Handle disabled Windows sandbox accounts during cleanup (#46333)

## Why

Cleanup needs fresh logon tokens for sandbox accounts that may already be disabled. Temporarily enabling those accounts must leave a durable obligation to disable them again if the service exits unexpectedly.

## What changed

- Persist `cleanup_logon_pending` before enabling an account, then disable it again after the logon attempt before clearing the marker.
- Recover pending account disables before owner restoration or IPC admission, validating account SIDs before restoration. Block runtime readiness and provisioning while recovery is pending, and defer retirement until cleanup logons are prepared.
- Include the blocking logon details in cleanup timeout errors and suppress repeated identical cleanup errors in the Windows event log.

## Testing

Add receipt tests covering backward-compatible defaults and pending cleanup state surviving serialization, blocking readiness and owner admission until cleared.

GitOrigin-RevId: 801bec408a27ac85ccdc3eb5ca2bdb2ccb3d5827

codex-rs/windows-sandbox-rs/src/runtime_ownership.rs

@@ -93,7 +93,7 @@ impl RuntimeRegistration {
             && self
                 .accounts
                 .iter()
-                .all(|account| account.alias_path.is_some())
+                .all(|account| account.alias_path.is_some() && !account.cleanup_logon_pending)
     }
 }
 
@@ -119,7 +119,12 @@ impl InstallationRecord {
             "registered sandbox resources belong to a different owner or package"
         );
         ensure!(
-            self.runtime()?.retiring.is_none(),
+            self.runtime()?.retiring.is_none()
+                && self
+                    .runtime()?
+                    .accounts
+                    .iter()
+                    .all(|account| !account.cleanup_logon_pending),
             "registered sandbox cleanup must finish before provisioning"
         );
         Ok(())
@@ -128,6 +133,9 @@ impl InstallationRecord {
 
 #[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
 pub struct RuntimeAccountRegistration {
+    /// Persisted before temporarily enabling an account; cleared only after re-disabling it.
+    #[serde(default)]
+    pub cleanup_logon_pending: bool,
     pub account: SandboxRuntimeAccount,
     pub user_sid: String,
     /// OS-resolved alias written by the service, never inferred from a user name.

codex-rs/windows-sandbox-rs/src/runtime_ownership_tests.rs

@@ -19,6 +19,7 @@ fn record() -> InstallationRecord {
             metadata_roots: Vec::new(),
             ready_package: None,
             accounts: vec![RuntimeAccountRegistration {
+                cleanup_logon_pending: false,
                 account: SandboxRuntimeAccount::Offline,
                 user_sid: "S-1-5-21-1-2-3-1001".into(),
                 alias_path: None,
@@ -41,6 +42,7 @@ fn ready_runtime() -> RuntimeRegistration {
     runtime.ready_package = Some(READY_PACKAGE.into());
     runtime.accounts[0].alias_path = Some(PathBuf::from(r"C:\aliases\offline.exe"));
     runtime.accounts.push(RuntimeAccountRegistration {
+        cleanup_logon_pending: false,
         account: SandboxRuntimeAccount::Online,
         user_sid: "S-1-5-21-1-2-3-1002".into(),
         alias_path: Some(PathBuf::from(r"C:\aliases\online.exe")),
@@ -200,9 +202,13 @@ fn metadata_root_history_survives_record_round_trip() {
 }
 
 #[test]
-fn older_runtime_receipts_default_to_no_recorded_metadata_roots() {
+fn older_runtime_receipts_default_to_no_metadata_roots_or_cleanup_logon() {
     let expected = record();
-    let json = serde_json::to_value(&expected).unwrap();
+    let mut json = serde_json::to_value(&expected).unwrap();
+    json["runtime"]["accounts"][0]
+        .as_object_mut()
+        .unwrap()
+        .remove("cleanup_logon_pending");
     assert!(json["runtime"].get("metadata_roots").is_none());
     assert_eq!(
         serde_json::from_value::<InstallationRecord>(json).unwrap(),
@@ -243,3 +249,27 @@ fn legacy_non_null_cleanup_journal_cannot_silently_drop_its_fence() {
         assert!(serde_json::from_value::<InstallationRecord>(json).is_err());
     }
 }
+
+#[test]
+fn interrupted_cleanup_logon_blocks_admission_after_record_reload() {
+    let mut original = record();
+    original.runtime = Some(ready_runtime());
+    original.runtime_mut().unwrap().accounts[0].cleanup_logon_pending = true;
+    let mut restored: InstallationRecord =
+        serde_json::from_str(&serde_json::to_string(&original).unwrap()).unwrap();
+    assert_eq!(restored, original);
+    assert!(!restored.runtime().unwrap().ready_for_package(READY_PACKAGE));
+    assert!(
+        restored
+            .admit_owner(&original, "OpenAI.Codex_publisher")
+            .is_err()
+    );
+
+    restored.runtime_mut().unwrap().accounts[0].cleanup_logon_pending = false;
+    assert!(restored.runtime().unwrap().ready_for_package(READY_PACKAGE));
+    assert!(
+        restored
+            .admit_owner(&original, "OpenAI.Codex_publisher")
+            .is_ok()
+    );
+}

codex-rs/windows-sandbox-rs/src/uninstall_windows/processes.rs

@@ -83,11 +83,11 @@ pub(super) fn stop(users: &DisabledSandboxUsers, retained: &RetainedLogons) -> R
         // A token can outlive its process or exist before a runner starts. Account disable does
         // not revoke it, so check for remaining logins before removing protections. Only
         // explicitly pinned, private SYSTEM-finalizer tokens may remain; no process is exempt.
-        if !has_sandbox_logon_session(users, retained)? {
+        let Some(blocker) = blocking_logon_session(users, retained)? else {
             return Ok(());
-        }
+        };
         if Instant::now() >= deadline {
-            bail!("sandbox processes or logon tokens remain after the uninstall deadline");
+            bail!("sandbox cleanup blocked after the uninstall deadline: {blocker}");
         }
         // Repeat to catch descendants created during the previous snapshot.
         std::thread::sleep(
@@ -159,10 +159,10 @@ fn sandbox_processes(users: &DisabledSandboxUsers) -> Result<Vec<OwnedHandle>> {
     Ok(handles)
 }
 
-fn has_sandbox_logon_session(
+fn blocking_logon_session(
     users: &DisabledSandboxUsers,
     retained: &RetainedLogons,
-) -> Result<bool> {
+) -> Result<Option<String>> {
     let mut count = 0;
     let mut logons = null_mut();
     let status = unsafe { LsaEnumerateLogonSessions(&mut count, &mut logons) };
@@ -171,7 +171,7 @@ fn has_sandbox_logon_session(
     }
     let logons = LsaBuffer(logons.cast());
     if count == 0 {
-        return Ok(false);
+        return Ok(None);
     }
     for logon in unsafe { std::slice::from_raw_parts(logons.0.cast::<LUID>(), count as usize) } {
         // LocalSystem uses the reserved logon ID 0:0x3e7 and has no normal logon data.
@@ -188,17 +188,23 @@ fn has_sandbox_logon_session(
         }
         // Keep protections when LSA returns no session data.
         if data.is_null() {
-            return Ok(true);
+            return Ok(Some(format!(
+                "LSA returned no data for logon {:08x}:{:08x}",
+                logon.HighPart, logon.LowPart
+            )));
         }
         let data = LsaBuffer(data.cast());
         let data = unsafe { &*data.0.cast::<SECURITY_LOGON_SESSION_DATA>() };
         // LSA also lists sessions without a user SID, even before sandbox accounts exist.
         // Only sessions identified as sandbox users are evidence of remaining sandbox tokens.
         if is_sandbox_user(users, data.Sid) && !retained.contains(*logon) {
-            return Ok(true);
+            return Ok(Some(format!(
+                "sandbox logon {:08x}:{:08x} remains (logon type {}, session {})",
+                logon.HighPart, logon.LowPart, data.LogonType, data.Session
+            )));
         }
     }
-    Ok(false)
+    Ok(None)
 }
 
 fn is_sandbox_user(users: &DisabledSandboxUsers, sid: *mut c_void) -> bool {

codex-rs/windows-sandbox-service/src/package_lifecycle/registered.rs

@@ -75,13 +75,13 @@ pub(super) fn clean_up(lifecycle: &PackageLifecycle, record: InstallationRecord)
             }),
         "waiting for the authenticated runtime owner and home before sandbox file cleanup"
     );
-    let record = crate::registered_runtime::prepare_cleanup(record)?;
+    let mut record = crate::registered_runtime::prepare_cleanup(record)?;
     let removal = {
         let installation = lifecycle.installation.borrow();
         let installation = installation
             .as_ref()
             .context("authenticated installation is missing")?;
-        crate::registered_runtime::prepare_removal(installation.user_token.0, &record)?
+        crate::registered_runtime::prepare_removal(installation.user_token.0, &mut record)?
     };
     let prepared =
         prepare_packaged_windows_sandbox_cleanup_with_retained_tokens(&removal.tokens())?;

codex-rs/windows-sandbox-service/src/registered_runtime.rs

@@ -43,6 +43,7 @@ mod removal;
 
 pub(crate) use metadata::remove as remove_metadata;
 pub(crate) use removal::prepare as prepare_removal;
+pub(crate) use removal::restore_disabled_accounts;
 
 /// Must escape request handling: pending resources are retained until this service exits.
 #[derive(Debug)]
@@ -132,6 +133,7 @@ pub(crate) fn provision(
         .join(&family)
         .join(APP_CORE_RUNNER_ALIAS);
         accounts.push(RuntimeAccountRegistration {
+            cleanup_logon_pending: false,
             account,
             user_sid: profile.user_sid.clone(),
             alias_path: Some(alias_path),
@@ -202,28 +204,38 @@ pub(crate) fn provision(
     Ok(())
 }
 
-/// Prepare one retirement fence under the caller's setup lock. Removal preparation
-/// then retains exact account logons before the native guard disables those users.
-pub(crate) fn prepare_cleanup(mut record: InstallationRecord) -> Result<InstallationRecord> {
+/// Validate cleanup ownership under the caller's setup lock before obtaining logons.
+pub(crate) fn prepare_cleanup(record: InstallationRecord) -> Result<InstallationRecord> {
     validate_record(&record)?;
     for account in &record.runtime()?.accounts {
         if codex_windows_sandbox::local_user_flags(account.account.username())?.is_some() {
-            let sid = lookup_sid(account.account.username())?;
-            ensure!(
-                string_from_sid_bytes(&sid).map_err(anyhow::Error::msg)? == account.user_sid,
-                "managed runtime account was replaced before cleanup"
-            );
+            validate_account_sid(account)?;
         }
     }
     ensure!(
         record.runtime()?.retiring.is_none(),
         "interrupted runtime cleanup requires repair"
     );
-    record.runtime_mut()?.ready_package = None;
-    record.runtime_mut()?.retiring = Some(format!("{:?}", windows::core::GUID::new()?));
+    ensure!(
+        record
+            .runtime()?
+            .accounts
+            .iter()
+            .all(|account| !account.cleanup_logon_pending),
+        "cleanup logon recovery is pending"
+    );
     Ok(record)
 }
 
+fn validate_account_sid(account: &RuntimeAccountRegistration) -> Result<()> {
+    let sid = lookup_sid(account.account.username())?;
+    ensure!(
+        string_from_sid_bytes(&sid).map_err(anyhow::Error::msg)? == account.user_sid,
+        "managed runtime account was replaced before cleanup"
+    );
+    Ok(())
+}
+
 fn validate_record(record: &InstallationRecord) -> Result<()> {
     let runtime = record.runtime()?;
     ensure!(

codex-rs/windows-sandbox-service/src/registered_runtime/removal.rs

@@ -22,6 +22,7 @@ use anyhow::ensure;
 use windows_sys::Win32::Foundation::DUPLICATE_SAME_ACCESS;
 use windows_sys::Win32::Foundation::DuplicateHandle;
 use windows_sys::Win32::Foundation::HANDLE;
+use windows_sys::Win32::NetworkManagement::NetManagement::UF_ACCOUNTDISABLE;
 use windows_sys::Win32::System::Pipes::PeekNamedPipe;
 use windows_sys::Win32::System::SystemInformation::GetSystemDirectoryW;
 use windows_sys::Win32::System::Threading as threading;
@@ -54,23 +55,67 @@ impl PreparedRemoval {
     }
 }
 
+/// Restore durable temporary-enable intent before owner restoration or IPC admission.
+pub(crate) fn restore_disabled_accounts() -> Result<()> {
+    let _lock = codex_windows_sandbox::acquire_sandbox_setup_lock(/*timeout_ms*/ 5_000)?;
+    let Some(mut record) = crate::installation_record::load_runtime()? else {
+        return Ok(());
+    };
+    if !crate::installation_record::is_current_package_family(&record)? {
+        return Ok(());
+    }
+    super::validate_record(&record)?;
+    for index in 0..record.runtime()?.accounts.len() {
+        let account = &record.runtime()?.accounts[index];
+        if !account.cleanup_logon_pending {
+            continue;
+        }
+        super::validate_account_sid(account)?;
+        let flags = codex_windows_sandbox::local_user_flags(account.account.username())?
+            .context("cleanup account disappeared before flag restoration")?;
+        codex_windows_sandbox::set_local_user_flags(
+            account.account.username(),
+            flags | UF_ACCOUNTDISABLE,
+        )?;
+        record.runtime_mut()?.accounts[index].cleanup_logon_pending = false;
+        crate::installation_record::save_runtime(&record)?;
+    }
+    Ok(())
+}
+
 // The cleanup entry checked the service family; prepare_cleanup validated this record
 // under the same setup lock. Fresh logons below still require exact account-SID checks.
-pub(crate) fn prepare(owner_token: HANDLE, record: &InstallationRecord) -> Result<PreparedRemoval> {
+pub(crate) fn prepare(
+    owner_token: HANDLE,
+    record: &mut InstallationRecord,
+) -> Result<PreparedRemoval> {
     ensure!(
-        record.runtime()?.retiring.is_some(),
-        "runtime is not retiring"
+        record.runtime()?.retiring.is_none(),
+        "runtime is already retiring"
     );
     let mut tokens = Vec::new();
     let mut targets = Vec::new();
-    for account in &record.runtime()?.accounts {
-        if codex_windows_sandbox::local_user_flags(account.account.username())?.is_none() {
+    for index in 0..record.runtime()?.accounts.len() {
+        let account = record.runtime()?.accounts[index].clone();
+        let Some(flags) = codex_windows_sandbox::local_user_flags(account.account.username())?
+        else {
             ensure!(
                 super::registered_packages(&account.user_sid, &record.runtime()?.package_family)?
                     .is_empty(),
                 "runtime account is missing while its package remains registered"
             );
             continue;
+        };
+        // Persist the obligation before changing SAM, so process death cannot lose it.
+        if flags & UF_ACCOUNTDISABLE != 0 {
+            record.runtime_mut()?.accounts[index].cleanup_logon_pending = true;
+            // Older bundled clients also fail closed while account recovery is pending.
+            record.runtime_mut()?.ready_package = None;
+            crate::installation_record::save_runtime(record)?;
+            codex_windows_sandbox::set_local_user_flags(
+                account.account.username(),
+                flags & !UF_ACCOUNTDISABLE,
+            )?;
         }
         let token = with_owner_impersonation(owner_token, || {
             let mut pins = Vec::new();
@@ -82,7 +127,21 @@ pub(crate) fn prepare(owner_token: HANDLE, record: &InstallationRecord) -> Resul
                 &record.codex_home,
                 account.account,
             )
-        })?;
+        });
+        if flags & UF_ACCOUNTDISABLE != 0 {
+            super::validate_account_sid(&account)?;
+            let current_flags =
+                codex_windows_sandbox::local_user_flags(account.account.username())?
+                    .context("cleanup account disappeared before flag restoration")?;
+            codex_windows_sandbox::set_local_user_flags(
+                account.account.username(),
+                current_flags | UF_ACCOUNTDISABLE,
+            )
+            .context("restore disabled sandbox account after cleanup logon")?;
+            record.runtime_mut()?.accounts[index].cleanup_logon_pending = false;
+            crate::installation_record::save_runtime(record)?;
+        }
+        let token = token?;
         super::validate_target(
             token.as_raw_handle() as _,
             account.account,
@@ -94,6 +153,9 @@ pub(crate) fn prepare(owner_token: HANDLE, record: &InstallationRecord) -> Resul
         }));
         tokens.push(token);
     }
+    // Flag recovery is durable before the in-memory retirement generation exists.
+    record.runtime_mut()?.ready_package = None;
+    record.runtime_mut()?.retiring = Some(format!("{:?}", windows::core::GUID::new()?));
     let group_sid = if tokens.is_empty() {
         None
     } else {

codex-rs/windows-sandbox-service/src/service/runtime_lifecycle.rs

@@ -53,7 +53,9 @@ pub(super) fn foreground_owner(
 
 pub(super) fn run(state: &ServiceState, package_lifecycle: &PackageLifecycle) -> Result<()> {
     let cleaned = Cell::new(false);
-    let restore_owner = || {
+    let last_cleanup_error = Cell::new(None);
+    let restore_owner = || -> Result<()> {
+        crate::registered_runtime::restore_disabled_accounts()?;
         let restored = crate::installation_record::load().and_then(|record| {
             record.map_or(Ok(()), |record| {
                 package_lifecycle.restore_logged_in_owner(record.session_id)
@@ -65,11 +67,12 @@ pub(super) fn run(state: &ServiceState, package_lifecycle: &PackageLifecycle) ->
                 &format!("unable to restore package uninstall listener: {error:#}"),
             );
         }
+        Ok(())
     };
     crate::ipc::run(
         Arc::clone(&state.shutdown),
         || {
-            restore_owner();
+            restore_owner()?;
             state.report_status(SERVICE_RUNNING, NO_ERROR)?;
             log_information(
                 EVENT_SERVICE_STARTED,
@@ -81,8 +84,9 @@ pub(super) fn run(state: &ServiceState, package_lifecycle: &PackageLifecycle) ->
             package_lifecycle.register_authenticated_user(installation, user_token)
         },
         || loop {
-            restore_owner();
+            restore_owner()?;
             if !crate::package_lifecycle::runtime_owner_removed()? {
+                last_cleanup_error.set(None);
                 return Ok(());
             }
             let Err(error) = package_lifecycle.clean_up() else {
@@ -97,10 +101,7 @@ pub(super) fn run(state: &ServiceState, package_lifecycle: &PackageLifecycle) ->
             }) {
                 return Err(error);
             }
-            log_error(
-                EVENT_SERVICE_FAILED,
-                &format!("sandbox cleanup deferred: {error:#}"),
-            );
+            log_cleanup_error(&error, &last_cleanup_error);
             // Keep the pipe available during pre-fence backoff. A reinstall
             // resumes admission on the next owner check; SCM stops are never reset.
             if !wait_for_cleanup_retry(state) {
@@ -122,6 +123,7 @@ pub(super) fn run(state: &ServiceState, package_lifecycle: &PackageLifecycle) ->
 /// Retries the current teardown step, never a phase recovered from stored intent.
 pub(crate) fn retry_cleanup(mut operation: impl FnMut() -> Result<()>) -> Result<()> {
     let mut stop_failures = 0;
+    let last_cleanup_error = Cell::new(None);
     loop {
         let Err(error) = operation() else {
             return Ok(());
@@ -129,10 +131,7 @@ pub(crate) fn retry_cleanup(mut operation: impl FnMut() -> Result<()>) -> Result
         let Some(state) = SERVICE_STATE.get() else {
             return Err(error);
         };
-        log_error(
-            EVENT_SERVICE_FAILED,
-            &format!("sandbox cleanup deferred: {error:#}"),
-        );
+        log_cleanup_error(&error, &last_cleanup_error);
         if state.stop_requested.load(Ordering::Acquire) {
             // SCM stop initiates uninstall cleanup rather than cancelling it.
             // Four one-second retry waits fit within the 10-second SCM wait hint.
@@ -147,6 +146,15 @@ pub(crate) fn retry_cleanup(mut operation: impl FnMut() -> Result<()>) -> Result
     }
 }
 
+fn log_cleanup_error(error: &anyhow::Error, previous: &Cell<Option<String>>) {
+    let message = format!("sandbox cleanup deferred: {error:#}");
+    // Retrying an unchanged failure must not flood the Windows event log.
+    if previous.take().as_deref() != Some(message.as_str()) {
+        log_error(EVENT_SERVICE_FAILED, &message);
+    }
+    previous.set(Some(message));
+}
+
 fn wait_for_cleanup_retry(state: &ServiceState) -> bool {
     for _ in 0..30 {
         if state.shutdown.load(Ordering::Acquire) {