From caabf6e0c4f534da7ec9f18fe45f7d74bd98113f Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Sun, 14 Jun 2026 00:26:23 +0000 Subject: [PATCH] codex: address PR review feedback (#28120) --- bazel/rules/testing/wine/src/lib.rs | 58 +++++------------------ bazel/rules/testing/wine/src/lib_tests.rs | 37 +++++++-------- 2 files changed, 31 insertions(+), 64 deletions(-) diff --git a/bazel/rules/testing/wine/src/lib.rs b/bazel/rules/testing/wine/src/lib.rs index b349be7fb5..0cc4c6a0b7 100644 --- a/bazel/rules/testing/wine/src/lib.rs +++ b/bazel/rules/testing/wine/src/lib.rs @@ -17,10 +17,6 @@ use tempfile::TempDir; use tokio::process::Child; use tokio::process::ChildStdout; use tokio::process::Command as TokioCommand; -use tokio::time::timeout; - -const ASYNC_SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(10); -const BLOCKING_SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(5); /// Builds a command that runs a Windows executable in an isolated Wine prefix. pub struct WineTestCommand { @@ -76,7 +72,6 @@ impl WineTestCommand { self } - /// Starts the Windows executable with a fresh `WINEPREFIX`. pub fn spawn(self) -> Result { let runtime = WineRuntimePaths::from_runfiles()?; @@ -198,8 +193,10 @@ impl WineProcesses { Err(error) => (Err(error).context("check Windows process status"), false), }; let wait_result = self - .wait_for_child() + .child + .wait() .await + .context("wait for Windows process running under Wine") .and_then(|status| { anyhow::ensure!( !check_exit_status || status.success(), @@ -207,30 +204,20 @@ impl WineProcesses { ); Ok(()) }); - let wineserver_result = timeout(ASYNC_SHUTDOWN_TIMEOUT, async { + let wineserver_result = async { let mut command = TokioCommand::from(self.stop_wineserver_command()); let status = command.status().await.context("stop isolated wineserver")?; anyhow::ensure!(status.success(), "wineserver exited with {status}"); Ok(()) - }) - .await - .context("stop isolated wineserver timed out") - .and_then(std::convert::identity); - - let result = kill_result.and(wait_result).and(wineserver_result); - if result.is_ok() { - self.cleanup_complete = true; - } else { - self.shutdown_blocking(); } - result - } + .await; - async fn wait_for_child(&mut self) -> Result { - timeout(ASYNC_SHUTDOWN_TIMEOUT, self.child.wait()) - .await - .context("wait for Windows process running under Wine timed out")? - .context("wait for Windows process running under Wine") + // Every cleanup action has been attempted, so an individual error + // should not cause the blocking fallback to repeat them. + self.cleanup_complete = true; + kill_result?; + wait_result?; + wineserver_result } fn stop_wineserver_command(&self) -> StdCommand { @@ -255,7 +242,6 @@ impl WineProcesses { } log_panic_cleanup(format_args!("Wine panic cleanup waiting for its child")); - let deadline = std::time::Instant::now() + BLOCKING_SHUTDOWN_TIMEOUT; loop { match self.child.try_wait() { Ok(Some(status)) => { @@ -264,15 +250,7 @@ impl WineProcesses { )); break; } - Ok(None) if std::time::Instant::now() < deadline => { - std::thread::sleep(Duration::from_millis(10)); - } - Ok(None) => { - log_panic_cleanup(format_args!( - "Wine panic cleanup timed out waiting for its child" - )); - break; - } + Ok(None) => std::thread::sleep(Duration::from_millis(10)), Err(error) => { log_panic_cleanup(format_args!( "Wine panic cleanup could not wait for its child: {error}" @@ -283,7 +261,7 @@ impl WineProcesses { } log_panic_cleanup(format_args!("Wine panic cleanup stopping its wineserver")); - match self.kill_wineserver_command().status() { + match self.stop_wineserver_command().status() { Ok(status) => log_panic_cleanup(format_args!( "Wine panic cleanup wineserver exited with {status}" )), @@ -294,16 +272,6 @@ impl WineProcesses { self.cleanup_complete = true; log_panic_cleanup(format_args!("Wine panic cleanup complete")); } - - fn kill_wineserver_command(&self) -> StdCommand { - let mut command = StdCommand::new(&self.runtime.wineserver); - configure_wine_environment(&mut command, &self.runtime, self.prefix.path()); - command - .arg("-k") - .stdout(Stdio::null()) - .stderr(Stdio::null()); - command - } } impl Drop for WineProcesses { diff --git a/bazel/rules/testing/wine/src/lib_tests.rs b/bazel/rules/testing/wine/src/lib_tests.rs index cf38dbc499..fd8e667fd1 100644 --- a/bazel/rules/testing/wine/src/lib_tests.rs +++ b/bazel/rules/testing/wine/src/lib_tests.rs @@ -25,25 +25,6 @@ use super::WineTestProcess; use super::WineRuntimePaths; use super::install_powershell_runtime; -// The marker makes the assertion resilient to Wine or PTY startup chatter. -const POWERSHELL_SMOKE_MARKER: &str = "WINE_PWSH_SMOKE"; -// Besides proving that the pinned runtime starts, report the properties that -// shell detection and command construction rely on: PowerShell 7 Core running -// with Windows semantics and a backslash path separator. -const POWERSHELL_SMOKE_SCRIPT: &str = concat!( - "$ErrorActionPreference = 'Stop'; ", - "[Console]::OutputEncoding = [System.Text.UTF8Encoding]::new($false); ", - "$separatorCode = [int]([System.IO.Path]::DirectorySeparatorChar); ", - "if ($PSVersionTable.PSVersion.Major -ne 7) { throw 'expected PowerShell 7' }; ", - "if ($PSVersionTable.PSEdition -ne 'Core') { throw 'expected PowerShell Core' }; ", - "if (-not $IsWindows) { throw 'expected Windows semantics' }; ", - "if ($separatorCode -ne 92) { throw 'expected backslash path separator' }; ", - "Write-Output ('WINE_PWSH_SMOKE|' + ", - "$PSVersionTable.PSVersion.ToString() + '|' + ", - "$PSVersionTable.PSEdition + '|' + ", - "$IsWindows.ToString().ToLowerInvariant() + '|' + $separatorCode)", -); - async fn waiting_smoke_process() -> Result { let executable = codex_utils_cargo_bin::cargo_bin("wine-smoke")?; let mut process = WineTestCommand::new(executable).arg("--wait").spawn()?; @@ -287,6 +268,24 @@ fn powershell_runtime_is_materialized_at_the_windows_fallback_path() -> Result<( async fn pinned_powershell_runs_under_wine_with_a_pty() -> Result<()> { // Keep this integration smoke test local to the Wine support crate. The // production-shaped PowerShell launch path belongs to exec-server tests. + // The marker makes the assertion resilient to Wine or PTY startup chatter. + const POWERSHELL_SMOKE_MARKER: &str = "WINE_PWSH_SMOKE"; + // Besides proving that the pinned runtime starts, report the properties + // that shell detection and command construction rely on: PowerShell 7 Core + // running with Windows semantics and a backslash path separator. + const POWERSHELL_SMOKE_SCRIPT: &str = concat!( + "$ErrorActionPreference = 'Stop'; ", + "[Console]::OutputEncoding = [System.Text.UTF8Encoding]::new($false); ", + "$separatorCode = [int]([System.IO.Path]::DirectorySeparatorChar); ", + "if ($PSVersionTable.PSVersion.Major -ne 7) { throw 'expected PowerShell 7' }; ", + "if ($PSVersionTable.PSEdition -ne 'Core') { throw 'expected PowerShell Core' }; ", + "if (-not $IsWindows) { throw 'expected Windows semantics' }; ", + "if ($separatorCode -ne 92) { throw 'expected backslash path separator' }; ", + "Write-Output ('WINE_PWSH_SMOKE|' + ", + "$PSVersionTable.PSVersion.ToString() + '|' + ", + "$PSVersionTable.PSEdition + '|' + ", + "$IsWindows.ToString().ToLowerInvariant() + '|' + $separatorCode)", + ); let runtime = WineRuntimePaths::from_runfiles()?; let prefix = TempDir::new()?; install_powershell_runtime(prefix.path(), &runtime.powershell_runtime)?;