From 162a31ae92ddb1649158e869124f63f5d9c424eb Mon Sep 17 00:00:00 2001 From: iuyo5678 Date: Mon, 7 Sep 2026 20:04:15 +0800 Subject: [PATCH 1/2] fix(update): make Windows self-updates reliable --- .github/workflows/ci.yml | 8 +- crates/bsk-cli/src/cli/update.rs | 314 +++++++++++++++++------ crates/bsk-cli/src/cli/update/windows.rs | 203 +++++++++++++++ crates/bsk-cli/src/daemon/start.rs | 23 +- crates/bsk-cli/tests/windows_update.rs | 309 ++++++++++++++++++++++ 5 files changed, 771 insertions(+), 86 deletions(-) create mode 100644 crates/bsk-cli/src/cli/update/windows.rs create mode 100644 crates/bsk-cli/tests/windows_update.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a74512f..be5c2eb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -38,7 +38,7 @@ jobs: run: cargo test --workspace --locked windows-daemon: - name: Windows daemon regression tests + name: Windows daemon and update regression tests runs-on: windows-latest steps: @@ -52,6 +52,12 @@ jobs: - name: Run Windows process liveness tests run: cargo test -p bsk --lib --locked daemon::lockfile::tests + - name: Run Windows update tests + run: cargo test -p bsk --lib --locked cli::update::tests + + - name: Run Windows update process tests + run: cargo test -p bsk --test windows_update --locked + frontend: name: Frontend lint, typecheck, tests, build runs-on: ubuntu-latest diff --git a/crates/bsk-cli/src/cli/update.rs b/crates/bsk-cli/src/cli/update.rs index 5672739..630c99b 100644 --- a/crates/bsk-cli/src/cli/update.rs +++ b/crates/bsk-cli/src/cli/update.rs @@ -6,9 +6,7 @@ use std::path::{Path, PathBuf}; use std::time::{Duration, SystemTime}; #[cfg(windows)] -use std::os::windows::process::CommandExt; -#[cfg(windows)] -use std::process::{Command, Stdio}; +mod windows; use anyhow::{Context, Result, bail}; use clap::Args; @@ -363,7 +361,8 @@ fn install_candidate_with_client( crate::daemon::start::run_stop().context("stop bsk daemon before update")?; } - let action = replace_binary_for_update(&target, &binary, daemon_was_running)?; + let restart_args = daemon_was_running.then(StartArgs::default); + let action = replace_binary_for_update(&target, &binary, restart_args.as_ref())?; if daemon_was_running && matches!(action, InstallAction::Replaced) { crate::daemon::start::run_start(StartArgs::default()) @@ -394,8 +393,8 @@ pub(crate) fn download_candidate_binary( /// Daemon-side install: download, verify, and replace the executable at /// `target` (on Windows: stage the replacement next to it). Unlike the -/// CLI path this never stops or starts the daemon — the daemon drives -/// its own restart once the binary is replaced. +/// CLI path this never stops the daemon. On Windows the helper starts +/// its replacement after exit; elsewhere the daemon drives its own restart. /// /// `target` must be captured *before* any replacement happens: on Linux /// `std::env::current_exe` starts returning a ` (deleted)`-suffixed @@ -403,10 +402,11 @@ pub(crate) fn download_candidate_binary( pub(crate) fn self_install_candidate( candidate: &UpdateCandidate, target: &Path, + restart_args: &StartArgs, ) -> Result { let client = update_http_client(ARCHIVE_FETCH_TIMEOUT)?; let binary = download_candidate_binary(candidate, &client)?; - replace_binary_at_path(target, &binary) + replace_binary_for_update(target, &binary, Some(restart_args)) } /// Outcome of one daemon auto-update step (see [`auto_update_step`]). @@ -423,8 +423,8 @@ pub(crate) enum AutoUpdateOutcome { /// The new binary replaced the old one; the daemon should now /// restart into it. Replaced { latest: String }, - /// Windows staged the replacement next to the running binary; a - /// daemon/terminal restart finishes it. + /// Windows started a helper that will replace the executable and + /// restart the daemon once the current process exits. Staged { latest: String }, } @@ -668,6 +668,7 @@ fn render_report(format: Format, report: &UpdateReport) -> Result<()> { } if matches!(report.install_action, Some("staged")) { println!("the detached update helper will apply the replacement after exit"); + println!("if it fails, see the *.update-*.log file next to the executable"); } } Format::Json => { @@ -688,22 +689,22 @@ pub fn extract_bsk_binary(archive_bytes: &[u8], kind: ArchiveKind) -> Result Result { - replace_binary_for_update(target, binary, false) + replace_binary_for_update(target, binary, None) } fn replace_binary_for_update( target: &Path, binary: &[u8], - restart_daemon: bool, + restart_args: Option<&StartArgs>, ) -> Result { #[cfg(windows)] { - stage_windows_replacement(target, binary, restart_daemon) + stage_windows_replacement(target, binary, restart_args) } #[cfg(not(windows))] { - let _ = restart_daemon; + let _ = restart_args; replace_binary_atomically(target, binary) } } @@ -749,34 +750,69 @@ fn replace_binary_atomically(target: &Path, binary: &[u8]) -> Result, ) -> Result { - let paths = staged_replacement_paths(target, std::process::id())?; - std::fs::write(&paths.binary_path, binary) - .with_context(|| format!("write {}", paths.binary_path.display()))?; - std::fs::write( - &paths.script_path, - windows_replacement_script(target, &paths.binary_path, restart_daemon), - ) - .with_context(|| format!("write {}", paths.script_path.display()))?; + // The helper owns replacement/restart after this process exits. Do not + // report Staged until it has actually begun executing the script. + let _child = spawn_windows_replacement(target, binary, restart_args, 120)?; + Ok(InstallAction::Staged) +} - const CREATE_NO_WINDOW: u32 = 0x0800_0000; - const CREATE_NEW_PROCESS_GROUP: u32 = 0x0000_0200; - let command = format!("\"{}\"", paths.script_path.display()); - if let Err(err) = Command::new("cmd.exe") - .args(["/D", "/S", "/C"]) - .arg(command) - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .creation_flags(CREATE_NO_WINDOW | CREATE_NEW_PROCESS_GROUP) - .spawn() - { +#[cfg(windows)] +fn spawn_windows_replacement( + target: &Path, + binary: &[u8], + restart_args: Option<&StartArgs>, + attempts: u32, +) -> Result { + let paths = staged_replacement_paths(target, std::process::id())?; + let ready_path = paths.script_path.with_extension("ready"); + let log_path = paths.script_path.with_extension("log"); + let result = (|| { + // A previous failed attempt by this process must not acknowledge a + // new helper. All paths are private to this target and process id. + match std::fs::remove_file(&ready_path) { + Ok(()) => {} + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) => return Err(err.into()), + } + std::fs::write(&paths.binary_path, binary) + .with_context(|| format!("write {}", paths.binary_path.display()))?; + std::fs::write( + &paths.script_path, + windows_replacement_script(restart_args, attempts), + ) + .with_context(|| format!("write {}", paths.script_path.display()))?; + let mut child = windows::spawn( + &paths.script_path, + &paths.binary_path, + target, + &ready_path, + &log_path, + ) + .context("launch detached Windows update helper")?; + let deadline = std::time::Instant::now() + Duration::from_secs(5); + loop { + if std::fs::remove_file(&ready_path).is_ok() { + return Ok(child); + } + if let Some(status) = child.try_wait()? { + bail!("Windows update helper exited before becoming ready: {status}"); + } + if std::time::Instant::now() >= deadline { + let _ = child.kill(); + let _ = child.wait(); + bail!("Windows update helper did not become ready within 5 seconds"); + } + std::thread::sleep(Duration::from_millis(20)); + } + })(); + if result.is_err() { let _ = std::fs::remove_file(&paths.binary_path); let _ = std::fs::remove_file(&paths.script_path); - return Err(err).context("launch detached Windows update helper"); + let _ = std::fs::remove_file(&ready_path); } - Ok(InstallAction::Staged) + result.with_context(|| format!("Windows update helper failed; see {}", log_path.display())) } pub fn staged_replacement_paths(target: &Path, pid: u32) -> Result { @@ -793,25 +829,43 @@ pub fn staged_replacement_paths(target: &Path, pid: u32) -> Result String { - let target = target.display().to_string().replace('%', "%%"); - let staged_binary = staged_binary.display().to_string().replace('%', "%%"); - let restart = if restart_daemon { - format!("start \"\" /B \"{target}\" daemon start >nul 2>nul\r\n") - } else { - String::new() - }; +fn windows_replacement_script(restart_args: Option<&StartArgs>, attempts: u32) -> String { + // Only fixed switches and numeric values go into the ASCII script. Paths + // stay in Unicode environment variables, including %, ! and shell symbols. + let restart = restart_args.map_or_else(String::new, |args| { + format!( + "\"%BSK_UPDATE_TARGET%\" daemon start --port {} --session-idle {}ms --daemon-idle {}ms\r\nif errorlevel 1 goto failed_restart\r\n", + args.resolved_port(), + args.resolved_session_idle().as_millis(), + args.resolved_daemon_idle().as_millis(), + ) + }); format!( "@echo off\r\n\ - setlocal\r\n\ + setlocal DisableDelayedExpansion\r\n\ + > \"%BSK_UPDATE_READY%\" echo ready\r\n\ + if errorlevel 1 exit /b 1\r\n\ + set /a attempts=0\r\n\ :retry\r\n\ - move /Y \"{staged_binary}\" \"{target}\" >nul 2>nul\r\n\ - if errorlevel 1 (\r\n\ - ping -n 2 127.0.0.1 >nul\r\n\ - goto retry\r\n\ - )\r\n\ + move /Y \"%BSK_UPDATE_SOURCE%\" \"%BSK_UPDATE_TARGET%\" >nul\r\n\ + if not errorlevel 1 goto replaced\r\n\ + set /a attempts+=1\r\n\ + if %attempts% geq {attempts} goto failed_replace\r\n\ + if not exist \"%BSK_UPDATE_SOURCE%\" goto failed_replace\r\n\ + ping -n 2 127.0.0.1 >nul\r\n\ + goto retry\r\n\ + :failed_replace\r\n\ + echo Failed to replace the executable after %attempts% attempts.\r\n\ + goto failed\r\n\ + :replaced\r\n\ {restart}\ - del /F /Q \"%~f0\" >nul 2>nul\r\n" + del /F /Q \"%BSK_UPDATE_LOG%\" >nul 2>nul\r\n\ + (del /F /Q \"%BSK_UPDATE_SCRIPT%\" >nul 2>nul & exit 0)\r\n\ + :failed_restart\r\n\ + echo Replaced the executable but failed to restart the daemon.\r\n\ + :failed\r\n\ + del /F /Q \"%BSK_UPDATE_SOURCE%\" >nul 2>nul\r\n\ + (del /F /Q \"%BSK_UPDATE_SCRIPT%\" >nul 2>nul & exit 1)\r\n" ) } @@ -965,6 +1019,7 @@ mod tests { assert_eq!(extracted, b"windows binary"); } + #[cfg(not(windows))] #[test] fn replaces_binary_at_path() { let tmp = tempfile::TempDir::new().unwrap(); @@ -999,31 +1054,139 @@ mod tests { } #[test] - fn windows_replacement_script_retries_restarts_and_cleans_itself() { - let script = windows_replacement_script( - Path::new(r"C:\Program Files\bsk.exe"), - Path::new(r"C:\Program Files\bsk.exe.update-42"), - true, + fn windows_replacement_script_preserves_restart_config() { + let args = StartArgs { + port: Some(54321), + session_idle: Some(Duration::from_millis(1234)), + daemon_idle: Some(Duration::from_secs(75)), + ..Default::default() + }; + let script = windows_replacement_script(Some(&args), 120); + assert!(script.is_ascii()); + assert!( + script + .contains("daemon start --port 54321 --session-idle 1234ms --daemon-idle 75000ms") ); - - assert!(script.contains(":retry")); - assert!(script.contains("move /Y")); - assert!(script.contains("ping -n 2 127.0.0.1")); - assert!(!script.contains("timeout")); - assert!(script.contains(r#"start "" /B "C:\Program Files\bsk.exe" daemon start"#)); - assert!(script.contains(r#"del /F /Q "%~f0""#)); + assert!(!windows_replacement_script(None, 120).contains("daemon start")); } - #[test] - fn windows_replacement_script_omits_restart_when_daemon_was_not_running() { - let script = windows_replacement_script( - Path::new(r"C:\bsk.exe"), - Path::new(r"C:\bsk.exe.update-42"), - false, - ); + #[cfg(windows)] + fn wait_for_helper(mut child: windows::Helper) -> std::process::ExitStatus { + let deadline = std::time::Instant::now() + Duration::from_secs(10); + loop { + if let Some(status) = child.try_wait().unwrap() { + return status; + } + if std::time::Instant::now() >= deadline { + let _ = child.kill(); + let _ = child.wait(); + panic!("Windows update helper did not exit"); + } + std::thread::sleep(Duration::from_millis(20)); + } + } - assert!(!script.contains("daemon start")); - assert!(script.contains(r#"del /F /Q "%~f0""#)); + #[cfg(windows)] + #[test] + fn windows_helper_replaces_in_unicode_spaces_and_shell_symbol_paths() { + let tmp = tempfile::TempDir::new().unwrap(); + for name in [ + "ascii", + "with space", + "中文目录", + "literal %PATH% ! & (folder)", + ] { + let dir = tmp.path().join(name); + std::fs::create_dir(&dir).unwrap(); + let target = dir.join("bsk.exe"); + std::fs::write(&target, b"old binary").unwrap(); + let child = spawn_windows_replacement(&target, b"new binary", None, 5).unwrap(); + let paths = staged_replacement_paths(&target, std::process::id()).unwrap(); + let status = wait_for_helper(child); + assert!( + status.success(), + "{name}: {:?}", + std::fs::read_to_string(paths.script_path.with_extension("log")) + ); + assert_eq!(std::fs::read(&target).unwrap(), b"new binary", "{name}"); + // Successful updates leave no helper, staged binary, ready file or log. + assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 1, "{name}"); + } + } + + #[cfg(windows)] + #[test] + fn windows_helper_waits_for_unlock_then_replaces() { + use std::os::windows::fs::OpenOptionsExt; + let tmp = tempfile::TempDir::new().unwrap(); + let target = tmp.path().join("bsk.exe"); + std::fs::write(&target, b"old binary").unwrap(); + // Deny delete sharing, like a running Windows executable. + let lock = std::fs::OpenOptions::new() + .read(true) + .share_mode(1) + .open(&target) + .unwrap(); + let mut child = spawn_windows_replacement(&target, b"new binary", None, 5).unwrap(); + std::thread::sleep(Duration::from_millis(150)); + assert!(child.try_wait().unwrap().is_none()); + assert_eq!(std::fs::read(&target).unwrap(), b"old binary"); + drop(lock); + assert!(wait_for_helper(child).success()); + assert_eq!(std::fs::read(&target).unwrap(), b"new binary"); + } + + #[cfg(windows)] + #[test] + fn windows_helper_bounds_retries_and_preserves_old_binary_on_failure() { + use std::os::windows::fs::OpenOptionsExt; + let tmp = tempfile::TempDir::new().unwrap(); + let target = tmp.path().join("bsk.exe"); + std::fs::write(&target, b"old binary").unwrap(); + let _lock = std::fs::OpenOptions::new() + .read(true) + .share_mode(1) + .open(&target) + .unwrap(); + let child = spawn_windows_replacement(&target, b"new binary", None, 2).unwrap(); + assert!(!wait_for_helper(child).success()); + let paths = staged_replacement_paths(&target, std::process::id()).unwrap(); + assert_eq!(std::fs::read(&target).unwrap(), b"old binary"); + assert!(!paths.binary_path.exists()); + assert!(!paths.script_path.exists()); + let log_bytes = std::fs::read(paths.script_path.with_extension("log")).unwrap(); + let log = String::from_utf8_lossy(&log_bytes); + assert!( + log.contains("Failed to replace the executable after 2 attempts."), + "{log}" + ); + } + + #[cfg(windows)] + #[test] + fn windows_helper_reports_restart_failure_without_losing_replacement() { + let tmp = tempfile::TempDir::new().unwrap(); + let target = tmp.path().join("bsk.exe"); + std::fs::write(&target, b"old binary").unwrap(); + // Not a valid PE executable: replacement succeeds but restart fails. + let child = spawn_windows_replacement( + &target, + b"invalid executable", + Some(&StartArgs::default()), + 2, + ) + .unwrap(); + assert!(!wait_for_helper(child).success()); + let paths = staged_replacement_paths(&target, std::process::id()).unwrap(); + let bytes = std::fs::read(paths.script_path.with_extension("log")).unwrap(); + let log = String::from_utf8_lossy(&bytes); + assert!( + log.contains("Replaced the executable but failed to restart the daemon."), + "{log}" + ); + assert_eq!(std::fs::read(&target).unwrap(), b"invalid executable"); + assert!(!paths.script_path.exists()); + assert!(!paths.binary_path.exists()); } #[test] @@ -1282,10 +1445,9 @@ mod tests { } #[test] - fn auto_update_step_staged_outcome_requests_no_restart() { - // The Windows shape: the replacement is staged next to the - // running binary, so the outcome must not ask for the immediate - // self-restart that `Replaced` triggers. + fn auto_update_step_reports_staged_handoff() { + // The helper, rather than this process, starts the new executable. + // The daemon must exit to let that staged handoff finish. let candidate = test_candidate(); let outcome = auto_update_step(Some(&candidate), true, 0, |_| Ok(InstallAction::Staged)).unwrap(); diff --git a/crates/bsk-cli/src/cli/update/windows.rs b/crates/bsk-cli/src/cli/update/windows.rs new file mode 100644 index 0000000..6b4ca9e --- /dev/null +++ b/crates/bsk-cli/src/cli/update/windows.rs @@ -0,0 +1,203 @@ +//! Launch the update helper with only its stdio handles inherited. +//! +//! std::process::Command on Windows also inherits other inheritable handles. +//! A daemon's listening socket must not survive in the helper: it would keep +//! the port occupied after the daemon exits and prevent its replacement starting. + +use std::ffi::OsStr; +use std::fs::File; +use std::io; +use std::os::windows::ffi::OsStrExt; +use std::os::windows::io::{AsRawHandle, FromRawHandle, OwnedHandle}; +use std::os::windows::process::ExitStatusExt; +use std::path::Path; +use std::process::ExitStatus; + +use windows_sys::Win32::Foundation::{ + HANDLE, HANDLE_FLAG_INHERIT, SetHandleInformation, WAIT_OBJECT_0, WAIT_TIMEOUT, +}; +use windows_sys::Win32::System::Threading::{ + CREATE_NEW_PROCESS_GROUP, CREATE_NO_WINDOW, CREATE_UNICODE_ENVIRONMENT, CreateProcessW, + DeleteProcThreadAttributeList, EXTENDED_STARTUPINFO_PRESENT, GetExitCodeProcess, + InitializeProcThreadAttributeList, PROC_THREAD_ATTRIBUTE_HANDLE_LIST, PROCESS_INFORMATION, + STARTF_USESTDHANDLES, STARTUPINFOEXW, TerminateProcess, UpdateProcThreadAttribute, + WaitForSingleObject, +}; + +pub(super) struct Helper(OwnedHandle); + +impl Helper { + pub(super) fn try_wait(&mut self) -> io::Result> { + // SAFETY: the owned process handle remains valid for both calls. + match unsafe { WaitForSingleObject(self.0.as_raw_handle(), 0) } { + WAIT_OBJECT_0 => { + let mut code = 0; + if unsafe { GetExitCodeProcess(self.0.as_raw_handle(), &mut code) } == 0 { + return Err(io::Error::last_os_error()); + } + Ok(Some(ExitStatus::from_raw(code))) + } + WAIT_TIMEOUT => Ok(None), + _ => Err(io::Error::last_os_error()), + } + } + + pub(super) fn kill(&mut self) -> io::Result<()> { + // SAFETY: this handle belongs to the helper we created. + if unsafe { TerminateProcess(self.0.as_raw_handle(), 1) } == 0 { + return Err(io::Error::last_os_error()); + } + Ok(()) + } + + pub(super) fn wait(&mut self) -> io::Result<()> { + // Only used after kill(); do not leave a live helper on startup failure. + if unsafe { WaitForSingleObject(self.0.as_raw_handle(), 5000) } != WAIT_OBJECT_0 { + return Err(io::Error::new( + io::ErrorKind::TimedOut, + "update helper did not exit", + )); + } + Ok(()) + } +} + +// The opaque attribute list needs pointer-aligned storage and explicit teardown. +struct AttributeList(Vec); + +impl Drop for AttributeList { + fn drop(&mut self) { + // SAFETY: this wrapper is constructed only after successful initialization. + unsafe { DeleteProcThreadAttributeList(self.0.as_mut_ptr().cast()) }; + } +} + +pub(super) fn spawn( + script: &Path, + source: &Path, + target: &Path, + ready: &Path, + log_path: &Path, +) -> io::Result { + let root = std::env::var_os("SystemRoot") + .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, "SystemRoot is not set"))?; + let application = wide(Path::new(&root).join("System32/cmd.exe").as_os_str()); + // /S /C strips one outer pair of quotes. Environment expansion keeps paths + // Unicode and avoids CRT escaping, batch-file encoding, and CALL expansion. + let mut command = wide(OsStr::new( + "cmd.exe /D /V:OFF /S /C \"\"%BSK_UPDATE_SCRIPT%\"\"", + )); + let overrides = [ + ("BSK_UPDATE_SCRIPT", script.as_os_str()), + ("BSK_UPDATE_SOURCE", source.as_os_str()), + ("BSK_UPDATE_TARGET", target.as_os_str()), + ("BSK_UPDATE_READY", ready.as_os_str()), + ("BSK_UPDATE_LOG", log_path.as_os_str()), + ]; + let mut env: Vec<_> = std::env::vars_os() + .filter(|(key, _)| { + let key = key.to_string_lossy(); + !overrides + .iter() + .any(|(name, _)| key.eq_ignore_ascii_case(name)) + && !key.eq_ignore_ascii_case(crate::daemon::start::DAEMONIZED_ENV) + && !key.eq_ignore_ascii_case(crate::daemon::start::DAEMON_REPLACEMENT_WAIT_ENV) + }) + .collect(); + env.extend( + overrides + .into_iter() + .map(|(key, value)| (key.into(), value.to_owned())), + ); + env.sort_by_key(|(key, _)| key.to_string_lossy().to_uppercase()); + let mut environment = Vec::new(); + for (key, value) in env { + environment.extend(key.encode_wide()); + environment.push(b'=' as u16); + environment.extend(value.encode_wide()); + environment.push(0); + } + environment.push(0); + + let input = File::open("NUL")?; + let log = File::create(log_path)?; + let handles: [HANDLE; 2] = [input.as_raw_handle(), log.as_raw_handle()]; + for &handle in &handles { + // SAFETY: these are dedicated helper stdio handles, held until spawn + // finishes. The explicit handle list excludes every other parent handle. + if unsafe { SetHandleInformation(handle, HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT) } == 0 { + return Err(io::Error::last_os_error()); + } + } + + let mut size = 0; + // SAFETY: the first call queries the allocation size, as required by Win32. + unsafe { InitializeProcThreadAttributeList(std::ptr::null_mut(), 1, 0, &mut size) }; + if size == 0 { + return Err(io::Error::last_os_error()); + } + let mut storage = vec![0usize; size.div_ceil(std::mem::size_of::())]; + // SAFETY: storage has the requested size and pointer alignment. + if unsafe { InitializeProcThreadAttributeList(storage.as_mut_ptr().cast(), 1, 0, &mut size) } + == 0 + { + return Err(io::Error::last_os_error()); + } + let mut attributes = AttributeList(storage); + // SAFETY: the handle array and attribute storage outlive CreateProcessW. + if unsafe { + UpdateProcThreadAttribute( + attributes.0.as_mut_ptr().cast(), + 0, + PROC_THREAD_ATTRIBUTE_HANDLE_LIST as usize, + handles.as_ptr().cast(), + std::mem::size_of_val(&handles), + std::ptr::null_mut(), + std::ptr::null(), + ) + } == 0 + { + return Err(io::Error::last_os_error()); + } + // SAFETY: these Win32 structs permit zero initialization; required fields + // are populated below before the process is created. + let mut startup: STARTUPINFOEXW = unsafe { std::mem::zeroed() }; + startup.StartupInfo.cb = std::mem::size_of::() as u32; + startup.StartupInfo.dwFlags = STARTF_USESTDHANDLES; + startup.StartupInfo.hStdInput = handles[0]; + startup.StartupInfo.hStdOutput = handles[1]; + startup.StartupInfo.hStdError = handles[1]; + startup.lpAttributeList = attributes.0.as_mut_ptr().cast(); + let mut process: PROCESS_INFORMATION = unsafe { std::mem::zeroed() }; + // SAFETY: strings are NUL-terminated UTF-16, the environment is double-NUL + // terminated, and all buffers/handles remain alive for the duration of spawn. + if unsafe { + CreateProcessW( + application.as_ptr(), + command.as_mut_ptr(), + std::ptr::null(), + std::ptr::null(), + 1, + CREATE_NO_WINDOW + | CREATE_NEW_PROCESS_GROUP + | CREATE_UNICODE_ENVIRONMENT + | EXTENDED_STARTUPINFO_PRESENT, + environment.as_ptr().cast(), + std::ptr::null(), + &startup.StartupInfo, + &mut process, + ) + } == 0 + { + return Err(io::Error::last_os_error()); + } + // SAFETY: successful CreateProcessW transfers these two handles to us. + let _thread = unsafe { OwnedHandle::from_raw_handle(process.hThread) }; + Ok(Helper(unsafe { + OwnedHandle::from_raw_handle(process.hProcess) + })) +} + +fn wide(value: &OsStr) -> Vec { + value.encode_wide().chain(Some(0)).collect() +} diff --git a/crates/bsk-cli/src/daemon/start.rs b/crates/bsk-cli/src/daemon/start.rs index cb6460c..017c55d 100644 --- a/crates/bsk-cli/src/daemon/start.rs +++ b/crates/bsk-cli/src/daemon/start.rs @@ -232,8 +232,8 @@ pub fn run_foreground(cfg: DaemonConfig) -> Result<()> { let session_idle_task = spawn_session_idle_reaper(Arc::clone(&state)); let browser_liveness_task = spawn_browser_liveness_reaper(Arc::clone(&state)); // Fired by the update check task after a successful auto-update: - // the replacement daemon has already been spawned, so this - // process should shut down and let it take over. + // the replacement daemon (or Windows update helper) is ready, so + // this process should shut down and let it take over. let restart_notify = Arc::new(tokio::sync::Notify::new()); let update_check_task = spawn_update_check_task(Arc::clone(&state), Arc::clone(&restart_notify)); @@ -539,8 +539,8 @@ pub(crate) fn spawn_session_idle_reaper(state: Arc) -> tokio::task: /// task spawns the replacement daemon (see /// [`DAEMON_REPLACEMENT_WAIT_ENV`]) and fires `restart` so this process /// shuts down and the new version takes over; on Windows the -/// replacement can only be staged, so it just logs that a restart is -/// needed. +/// helper waits for this process to exit, replaces the binary, and starts +/// the new daemon with the same configuration. pub(crate) fn spawn_update_check_task( state: Arc, restart: Arc, @@ -605,7 +605,11 @@ pub(crate) fn spawn_update_check_task( |candidate| { let target = exe_path.as_deref().context("current executable unknown")?; - update::self_install_candidate(candidate, target) + update::self_install_candidate( + candidate, + target, + &restart_start_args(&state.config), + ) }, ) }) @@ -635,10 +639,11 @@ pub(crate) fn spawn_update_check_task( sessions, "auto-update postponed: agent session(s) active; will retry on the next tick" ), - update::AutoUpdateOutcome::Staged { latest } => warn!( - %latest, - "auto-update staged the new binary but cannot replace the running daemon in place; restart the daemon (or terminal) to finish the upgrade" - ), + update::AutoUpdateOutcome::Staged { latest } => { + info!(%latest, "update helper ready; exiting so it can replace and restart the daemon"); + restart.notify_one(); + return; + } update::AutoUpdateOutcome::Replaced { latest } => { info!( current = env!("CARGO_PKG_VERSION"), diff --git a/crates/bsk-cli/tests/windows_update.rs b/crates/bsk-cli/tests/windows_update.rs new file mode 100644 index 0000000..76fab91 --- /dev/null +++ b/crates/bsk-cli/tests/windows_update.rs @@ -0,0 +1,309 @@ +//! Exercise self-update with a real executable and a local release server. +#![cfg(windows)] + +use std::fs; +use std::io::{Cursor, Read, Write}; +use std::net::TcpListener; +use std::os::windows::process::CommandExt; +use std::path::PathBuf; +use std::process::{Child, Command, Stdio}; +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::thread; +use std::time::{Duration, Instant}; + +use bsk::daemon::info::DaemonInfo; +use sha2::{Digest, Sha256}; + +const MARKER: &[u8] = b"windows-update-regression-fixture"; + +struct ReleaseServer { + url: String, + requests: Arc, + stop: Arc, + worker: Option>, +} + +impl ReleaseServer { + fn new(binary: &[u8]) -> Self { + let mut archive = zip::ZipWriter::new(Cursor::new(Vec::new())); + archive + .start_file( + "bsk.exe", + zip::write::SimpleFileOptions::default() + .compression_method(zip::CompressionMethod::Stored), + ) + .unwrap(); + archive.write_all(binary).unwrap(); + let archive = archive.finish().unwrap().into_inner(); + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + listener.set_nonblocking(true).unwrap(); + let url = format!("http://{}", listener.local_addr().unwrap()); + let manifest = serde_json::to_vec(&serde_json::json!({ + "version": "999.0.0", + "assets": {"windows-x64": { + "url": format!("{url}/bsk.zip"), + "sha256": Sha256::digest(&archive).iter().map(|byte| format!("{byte:02x}")).collect::(), + }}, + })) + .unwrap(); + let stop = Arc::new(AtomicBool::new(false)); + let worker_stop = Arc::clone(&stop); + let requests = Arc::new(AtomicUsize::new(0)); + let worker_requests = Arc::clone(&requests); + let worker = thread::spawn(move || { + while !worker_stop.load(Ordering::SeqCst) { + let (mut stream, _) = match listener.accept() { + Ok(connection) => connection, + Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => { + thread::sleep(Duration::from_millis(10)); + continue; + } + Err(err) => panic!("accept release request: {err}"), + }; + stream + .set_read_timeout(Some(Duration::from_secs(5))) + .unwrap(); + stream + .set_write_timeout(Some(Duration::from_secs(5))) + .unwrap(); + let mut request = Vec::new(); + let mut chunk = [0; 1024]; + while !request.windows(4).any(|window| window == b"\r\n\r\n") { + let len = stream.read(&mut chunk).unwrap(); + if len == 0 { + break; + } + request.extend_from_slice(&chunk[..len]); + } + let body = if request.starts_with(b"GET /bsk.zip ") { + worker_requests.fetch_add(1, Ordering::SeqCst); + &archive + } else { + &manifest + }; + write!( + stream, + "HTTP/1.1 200 OK\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", + body.len() + ) + .unwrap(); + stream.write_all(body).unwrap(); + } + }); + Self { + url: format!("{url}/version.json"), + requests, + stop, + worker: Some(worker), + } + } +} + +impl Drop for ReleaseServer { + fn drop(&mut self) { + self.stop.store(true, Ordering::SeqCst); + let _ = self.worker.take().unwrap().join(); + } +} + +struct Fixture { + _tmp: tempfile::TempDir, + exe: PathBuf, + home: PathBuf, + binary: Vec, + server: ReleaseServer, + daemon: Option, +} + +impl Fixture { + fn new() -> Self { + let tmp = tempfile::TempDir::new().unwrap(); + let dir = tmp.path().join("中文 space %PATH% ! & (update)"); + fs::create_dir(&dir).unwrap(); + let exe = dir.join("bsk.exe"); + let home = tmp.path().join("home"); + fs::create_dir(&home).unwrap(); + fs::copy(env!("CARGO_BIN_EXE_bsk"), &exe).unwrap(); + // A PE overlay distinguishes the replacement without requiring a + // second build or changing the executable's behavior/version. + let mut binary = fs::read(&exe).unwrap(); + binary.extend_from_slice(MARKER); + let server = ReleaseServer::new(&binary); + Self { + _tmp: tmp, + exe, + home, + binary, + server, + daemon: None, + } + } + + fn command(&self) -> Command { + let mut cmd = Command::new(&self.exe); + cmd.env("BSK_HOME", &self.home) + .env("BSK_UPDATE_MANIFEST_URL", &self.server.url) + .env("BSK_AUTO_UPDATE", "off") + .env("RUST_LOG", "info") + .env("NO_PROXY", "127.0.0.1,localhost") + .env_remove("BSK_DAEMONIZED") + .env_remove("BSK_DAEMON_REPLACES_PID") + .stdin(Stdio::null()) + .creation_flags(0x0800_0000); + cmd + } + + fn wait_for(&self, description: &str, mut check: impl FnMut() -> bool) { + let deadline = Instant::now() + Duration::from_secs(30); + while Instant::now() < deadline { + if check() { + return; + } + thread::sleep(Duration::from_millis(50)); + } + let diagnostics: Vec<_> = fs::read_dir(self.exe.parent().unwrap()) + .unwrap() + .flatten() + .filter(|entry| entry.path().extension().is_some_and(|ext| ext == "log")) + .map(|entry| { + ( + entry.path(), + String::from_utf8_lossy(&fs::read(entry.path()).unwrap_or_default()) + .into_owned(), + ) + }) + .collect(); + let home_logs: Vec<_> = fs::read_dir(&self.home) + .unwrap() + .flatten() + .filter(|entry| entry.file_name().to_string_lossy().contains("log")) + .map(|entry| { + ( + entry.path(), + String::from_utf8_lossy(&fs::read(entry.path()).unwrap_or_default()) + .into_owned(), + ) + }) + .collect(); + panic!( + "timed out waiting for {description}; helper logs: {diagnostics:?}; daemon logs: {home_logs:?}" + ); + } + + fn info(&self) -> Option { + serde_json::from_slice(&fs::read(self.home.join("daemon.json")).ok()?).ok() + } + + fn updated(&self) -> bool { + // A locked executable may briefly reject reads during replacement. + fs::read(&self.exe).is_ok_and(|binary| binary == self.binary) + } +} + +impl Drop for Fixture { + fn drop(&mut self) { + // Stop only the daemon belonging to this isolated BSK_HOME. Also reap + // the foreground process on assertion failures to avoid leaking locks. + let _ = self.command().args(["daemon", "stop"]).output(); + if let Some(child) = self.daemon.as_mut() { + let _ = child.kill(); + let _ = child.wait(); + } + } +} + +#[test] +fn manual_update_replaces_the_running_executable_after_cli_exit() { + let fixture = Fixture::new(); + let out = fixture + .command() + .args(["--json", "update", "--yes"]) + .output() + .unwrap(); + assert!( + out.status.success(), + "{}", + String::from_utf8_lossy(&out.stderr) + ); + let report: serde_json::Value = serde_json::from_slice(&out.stdout).unwrap(); + assert_eq!(report["status"], "staged"); + fixture.wait_for("CLI replacement and helper cleanup", || { + fixture.updated() && fs::read_dir(fixture.exe.parent().unwrap()).unwrap().count() == 1 + }); + assert!( + fixture.info().is_none(), + "an update must not start a previously absent daemon" + ); + assert_eq!(fixture.server.requests.load(Ordering::SeqCst), 1); +} + +#[test] +fn automatic_update_exits_old_daemon_and_restarts_on_the_same_port() { + let mut fixture = Fixture::new(); + let port = unused_port(); + let log = fs::File::create(fixture.home.join("foreground.log")).unwrap(); + let child = fixture + .command() + .env("BSK_AUTO_UPDATE", "on") + // Exercise the environment inherited by a normally detached daemon. + .env("BSK_DAEMONIZED", "1") + .args([ + "daemon", + "start", + "--port", + &port.to_string(), + "--session-idle", + "1234ms", + "--daemon-idle", + "30s", + ]) + .stdout(Stdio::from(log.try_clone().unwrap())) + .stderr(Stdio::from(log)) + .spawn() + .unwrap(); + let old_pid = child.id(); + fixture.daemon = Some(child); + fixture.wait_for("replacement daemon", || { + fixture.updated() + && fixture + .info() + .is_some_and(|info| info.pid != old_pid && info.ws_port == port) + }); + assert!( + fixture + .daemon + .as_mut() + .unwrap() + .try_wait() + .unwrap() + .is_some(), + "old daemon must exit" + ); + fixture.wait_for("helper cleanup", || { + fs::read_dir(fixture.exe.parent().unwrap()).unwrap().count() == 1 + }); + let status = fixture + .command() + .args(["--json", "status"]) + .output() + .unwrap(); + assert!( + status.status.success(), + "{}", + String::from_utf8_lossy(&status.stderr) + ); + assert_eq!( + fixture.server.requests.load(Ordering::SeqCst), + 1, + "the replacement must not stage another update immediately" + ); +} + +fn unused_port() -> u16 { + TcpListener::bind("127.0.0.1:0") + .unwrap() + .local_addr() + .unwrap() + .port() +} From 9337bd557bb1692c219acab7bdaa0874bf98643b Mon Sep 17 00:00:00 2001 From: iuyo5678 Date: Mon, 7 Sep 2026 20:27:04 +0800 Subject: [PATCH 2/2] test(update): use blocking connections in Windows release server --- crates/bsk-cli/tests/windows_update.rs | 36 +++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/crates/bsk-cli/tests/windows_update.rs b/crates/bsk-cli/tests/windows_update.rs index 76fab91..1c0d15e 100644 --- a/crates/bsk-cli/tests/windows_update.rs +++ b/crates/bsk-cli/tests/windows_update.rs @@ -3,7 +3,7 @@ use std::fs; use std::io::{Cursor, Read, Write}; -use std::net::TcpListener; +use std::net::{TcpListener, TcpStream}; use std::os::windows::process::CommandExt; use std::path::PathBuf; use std::process::{Child, Command, Stdio}; @@ -61,6 +61,10 @@ impl ReleaseServer { } Err(err) => panic!("accept release request: {err}"), }; + // Windows accept() inherits the listener's nonblocking mode. + // Keep accept polling for shutdown, but read/write each request + // in blocking mode with the bounded timeouts below. + stream.set_nonblocking(false).unwrap(); stream .set_read_timeout(Some(Duration::from_secs(5))) .unwrap(); @@ -107,6 +111,36 @@ impl Drop for ReleaseServer { } } +#[test] +fn release_server_waits_for_delayed_and_fragmented_request_headers() { + let server = ReleaseServer::new(b"test binary"); + let address = server + .url + .strip_prefix("http://") + .unwrap() + .strip_suffix("/version.json") + .unwrap(); + let mut stream = TcpStream::connect(address).unwrap(); + stream + .set_read_timeout(Some(Duration::from_secs(5))) + .unwrap(); + stream + .set_write_timeout(Some(Duration::from_secs(5))) + .unwrap(); + // Give accept() time to run before any bytes arrive, then split the + // headers across writes. Both reads must wait rather than return WouldBlock. + thread::sleep(Duration::from_millis(100)); + stream.write_all(b"GET /version.json HTTP/1.1\r\n").unwrap(); + thread::sleep(Duration::from_millis(100)); + stream.write_all(b"Host: localhost\r\n\r\n").unwrap(); + let mut response = String::new(); + stream.read_to_string(&mut response).unwrap(); + assert!(response.starts_with("HTTP/1.1 200 OK\r\n"), "{response}"); + let (_, body) = response.split_once("\r\n\r\n").unwrap(); + let manifest: serde_json::Value = serde_json::from_str(body).unwrap(); + assert_eq!(manifest["version"], "999.0.0"); +} + struct Fixture { _tmp: tempfile::TempDir, exe: PathBuf,