diff --git a/.github/scripts/windows-installer.iss b/.github/scripts/windows-installer.iss index e1d19dd9..26a4919d 100644 --- a/.github/scripts/windows-installer.iss +++ b/.github/scripts/windows-installer.iss @@ -148,14 +148,20 @@ Filename: "{app}\tty7-app.exe"; Parameters: "--unregister-explorer-menu"; Flags: understand --stop-daemon and would launch the GUI instead — so we extract the *new* tty7-app.exe to {tmp} and run that. It connects to the running daemon, hangs up every shell, waits for it to exit (releasing the file lock), then returns - without opening a window. Best effort: any failure falls through to the Restart - Manager backstop, and a fresh install simply has no daemon to stop. *) + without opening a window. Naming {app} widens the stop into "make this directory + replaceable": ConPTY hosts (OpenConsole.exe) orphaned by a daemon that never got + to shut down keep the installed images open — invisible to the daemon stop, fatal + to the DeleteFile below — so anything still running from {app} is terminated and + the call waits until the images there actually open for writing. Best effort: any + failure falls through to the Restart Manager backstop, and a fresh install simply + has nothing to stop. *) function PrepareToInstall(var NeedsRestart: Boolean): String; var ResultCode: Integer; begin ExtractTemporaryFile('tty7-app.exe'); - Exec(ExpandConstant('{tmp}\tty7-app.exe'), '--stop-daemon', '', + Exec(ExpandConstant('{tmp}\tty7-app.exe'), + '--stop-daemon --update-install-dir "' + ExpandConstant('{app}') + '"', '', SW_HIDE, ewWaitUntilTerminated, ResultCode); Result := ''; end; diff --git a/crates/tty7-core/src/daemon/server.rs b/crates/tty7-core/src/daemon/server.rs index 24310e74..6f930461 100644 --- a/crates/tty7-core/src/daemon/server.rs +++ b/crates/tty7-core/src/daemon/server.rs @@ -447,6 +447,18 @@ fn handle_conn(stream: Stream, registry: Arc) -> anyhow::Result<()> { ClientMsg::Shutdown => { log::info!("daemon shutting down on client request"); registry.drain_and_kill(); + // The ConPTY hosts (OpenConsole.exe) are this process's children, + // not the shells', so the per-pane kill never reaches them — and + // exiting right away would leave them holding the installed + // OpenConsole.exe image open while an updater tries to replace it. + // Reap everything still below us and wait for the images to be + // released before the endpoint disappears, because the endpoint + // going away is what tells `spawn::stop` the shutdown is complete. + #[cfg(windows)] + crate::daemon::winproc::reap_descendants_of( + std::process::id(), + std::time::Duration::from_secs(3), + ); on_shutdown(); std::process::exit(0); } diff --git a/crates/tty7-core/src/daemon/spawn.rs b/crates/tty7-core/src/daemon/spawn.rs index 01be294f..ef130fb1 100644 --- a/crates/tty7-core/src/daemon/spawn.rs +++ b/crates/tty7-core/src/daemon/spawn.rs @@ -15,6 +15,10 @@ const STARTUP_TIMEOUT: Duration = Duration::from_secs(3); const POLL_INTERVAL: Duration = Duration::from_millis(50); const HANDSHAKE_TIMEOUT: Duration = Duration::from_secs(2); const SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(6); +/// How long after the endpoint disappears the daemon process itself gets to +/// finish exiting. Under a graceful shutdown this is milliseconds; the margin +/// covers the Windows descendant reap that runs before `exit`. +const PROCESS_EXIT_TIMEOUT: Duration = Duration::from_secs(5); #[cfg(any(target_os = "macos", target_os = "linux"))] const REAP_TERM_TIMEOUT: Duration = Duration::from_secs(6); #[cfg(any(target_os = "macos", target_os = "linux"))] @@ -296,6 +300,12 @@ pub fn restart() -> anyhow::Result<()> { pub fn stop() { use std::io::Write as _; + // Read the pid before asking the daemon to die: a clean shutdown removes + // the pidfile, and the endpoint disappearing is not the same event as the + // process releasing its image — the gap between them is exactly where an + // installer starts replacing files that are still locked. + let recorded = pidfile::read().filter(|&pid| pid > 4 && pid != std::process::id()); + if let Ok(mut stream) = transport::connect() { if ClientMsg::Shutdown.encode(&mut stream).is_ok() { let _ = stream.flush(); @@ -306,6 +316,12 @@ pub fn stop() { } } + if let Some(pid) = recorded + && !wait_for_recorded_exit(pid, PROCESS_EXIT_TIMEOUT) + { + log::warn!("daemon pid {pid} released its endpoint but has not exited yet"); + } + reap_recorded_daemon(); if transport::endpoint_exists() { @@ -313,6 +329,31 @@ pub fn stop() { } } +/// Whether the recorded daemon process actually exited within `timeout`. The +/// endpoint file only says the daemon stopped listening; this is what says its +/// executable image is no longer mapped. +#[cfg(windows)] +fn wait_for_recorded_exit(pid: u32, timeout: Duration) -> bool { + crate::daemon::winproc::wait_for_exit(pid, timeout) +} + +#[cfg(any(target_os = "macos", target_os = "linux"))] +fn wait_for_recorded_exit(pid: u32, timeout: Duration) -> bool { + let deadline = Instant::now() + timeout; + while process_alive(pid as libc::pid_t) { + if Instant::now() >= deadline { + return false; + } + std::thread::sleep(POLL_INTERVAL); + } + true +} + +#[cfg(not(any(target_os = "macos", target_os = "linux", windows)))] +fn wait_for_recorded_exit(_pid: u32, _timeout: Duration) -> bool { + true +} + #[cfg(any(target_os = "macos", target_os = "linux"))] fn reap_recorded_daemon() { let Some(pid) = pidfile::read() else { return }; @@ -380,12 +421,103 @@ fn reap_recorded_daemon() { log::warn!("reaping unreachable daemon (pid {pid}); its sessions will be hung up"); for descendant in winproc::descendants(&procs, pid) { winproc::terminate(descendant); + winproc::wait_for_exit(descendant, REAP_WAIT_TIMEOUT); } winproc::terminate(pid); + winproc::wait_for_exit(pid, REAP_WAIT_TIMEOUT); } pidfile::remove(); } +#[cfg(windows)] +const REAP_WAIT_TIMEOUT: Duration = Duration::from_secs(2); + +/// Stops the daemon and then makes the installation directory actually +/// replaceable, which is more than `stop` alone can promise: a daemon that +/// died without cleaning up leaves its ConPTY hosts (OpenConsole.exe) running, +/// orphaned, each holding the installed image open — invisible to the pidfile +/// and fatal to any installer's `DeleteFile`. +/// +/// Terminates every process whose executable lives under `install_dir` +/// (except the caller), then waits until the replaceable images there can be +/// opened for writing. An error names what is still locked, so the update log +/// finally says *why* an upgrade could not replace its files. +#[cfg(windows)] +pub fn stop_for_update(install_dir: &Path) -> Result<(), String> { + use crate::daemon::winproc; + + stop(); + + let deadline = Instant::now() + UPDATE_CLEAR_TIMEOUT; + let holdouts = winproc::processes_running_from(install_dir); + for &pid in &holdouts { + log::warn!( + "terminating pid {pid} still running from {}", + install_dir.display() + ); + winproc::terminate(pid); + } + for pid in holdouts { + let left = deadline.saturating_duration_since(Instant::now()); + winproc::wait_for_exit(pid, left); + } + + wait_until_images_unlocked(install_dir, deadline) +} + +#[cfg(windows)] +const UPDATE_CLEAR_TIMEOUT: Duration = Duration::from_secs(10); + +/// Waits until every .exe and .dll directly in `dir` can be opened for +/// writing — the same access an installer needs to replace it. Only the top +/// level: that is where the locked images (tty7-app.exe, OpenConsole.exe, +/// conpty.dll) live, and a recursive sweep would stall on unrelated content. +#[cfg(windows)] +fn wait_until_images_unlocked(dir: &Path, deadline: Instant) -> Result<(), String> { + // Never probe our own image: the legitimate callers run from a staged + // copy outside `dir`, but if someone invokes the *installed* binary with + // this flag, its own image can never open for writing and the wait would + // only ever time out. + let own = std::env::current_exe().and_then(std::fs::canonicalize).ok(); + let images: Vec = std::fs::read_dir(dir) + .map_err(|error| format!("reading {}: {error}", dir.display()))? + .flatten() + .map(|entry| entry.path()) + .filter(|path| { + path.extension() + .and_then(|ext| ext.to_str()) + .is_some_and(|ext| { + ext.eq_ignore_ascii_case("exe") || ext.eq_ignore_ascii_case("dll") + }) + }) + .filter(|path| { + own.as_deref() + .is_none_or(|own| std::fs::canonicalize(path).is_ok_and(|path| path != own)) + }) + .collect(); + + let mut locked: Vec<&PathBuf> = images.iter().collect(); + loop { + locked.retain(|path| std::fs::OpenOptions::new().write(true).open(path).is_err()); + if locked.is_empty() { + return Ok(()); + } + if Instant::now() >= deadline { + let names: Vec = locked + .iter() + .filter_map(|path| path.file_name()) + .map(|name| name.to_string_lossy().into_owned()) + .collect(); + return Err(format!( + "these files in {} are still in use by another process: {}", + dir.display(), + names.join(", ") + )); + } + std::thread::sleep(POLL_INTERVAL); + } +} + #[cfg(not(any(target_os = "macos", target_os = "linux", windows)))] fn reap_recorded_daemon() {} diff --git a/crates/tty7-core/src/daemon/winproc.rs b/crates/tty7-core/src/daemon/winproc.rs index f9442ba5..4e9eb3a0 100644 --- a/crates/tty7-core/src/daemon/winproc.rs +++ b/crates/tty7-core/src/daemon/winproc.rs @@ -86,6 +86,105 @@ fn exe_name(raw: &[u16]) -> String { String::from_utf16_lossy(&raw[..len]) } +/// The full image path of a running process, or `None` for one this user +/// cannot query (system processes, another session's). +pub(crate) fn image_path(pid: u32) -> Option { + use windows_sys::Win32::Foundation::CloseHandle; + use windows_sys::Win32::System::Threading::{ + OpenProcess, PROCESS_NAME_WIN32, PROCESS_QUERY_LIMITED_INFORMATION, + QueryFullProcessImageNameW, + }; + + unsafe { + let handle = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, 0, pid); + if handle.is_null() { + return None; + } + let mut buf = [0u16; 1024]; + let mut len = buf.len() as u32; + let ok = QueryFullProcessImageNameW(handle, PROCESS_NAME_WIN32, buf.as_mut_ptr(), &mut len); + CloseHandle(handle); + if ok == 0 { + return None; + } + Some(std::path::PathBuf::from(String::from_utf16_lossy( + &buf[..len as usize], + ))) + } +} + +/// Waits until the process is gone, up to `timeout`. Returns whether it exited +/// in time. A pid that cannot be opened is reported as exited: the handle is +/// what names the process, and no handle means there is nothing left to wait +/// on that this user could ever observe. +pub(crate) fn wait_for_exit(pid: u32, timeout: std::time::Duration) -> bool { + use windows_sys::Win32::Foundation::{CloseHandle, WAIT_OBJECT_0}; + use windows_sys::Win32::System::Threading::{ + OpenProcess, PROCESS_SYNCHRONIZE, WaitForSingleObject, + }; + + unsafe { + let handle = OpenProcess(PROCESS_SYNCHRONIZE, 0, pid); + if handle.is_null() { + return true; + } + let millis = timeout.as_millis().min(u128::from(u32::MAX - 1)) as u32; + let result = WaitForSingleObject(handle, millis); + CloseHandle(handle); + result == WAIT_OBJECT_0 + } +} + +/// Terminates every descendant of `root` and waits for them to release their +/// images, bounded by `timeout` overall. Deepest-first, like the per-pane +/// kill, so a parent never respawns a child we already visited. +pub(crate) fn reap_descendants_of(root: u32, timeout: std::time::Duration) { + let deadline = std::time::Instant::now() + timeout; + let targets = descendants(&snapshot(), root); + for &pid in &targets { + terminate(pid); + } + for pid in targets { + let left = deadline.saturating_duration_since(std::time::Instant::now()); + if !wait_for_exit(pid, left) { + log::warn!("descendant process {pid} did not exit in time"); + } + } +} + +/// Pids (other than the caller's own) whose executable image lives under +/// `dir`. These are the processes that keep Windows from replacing the files +/// there: a stale daemon, or ConPTY hosts orphaned by a daemon that never got +/// to shut down. +pub(crate) fn processes_running_from(dir: &std::path::Path) -> Vec { + let own = std::process::id(); + snapshot() + .iter() + .filter(|p| p.pid != own && p.pid > 4) + .filter(|p| image_path(p.pid).is_some_and(|image| path_is_under(&image, dir))) + .map(|p| p.pid) + .collect() +} + +/// Whether `path` names a file inside `dir`, the way the filesystem sees it: +/// case-insensitive, and only at a component boundary so `...\tty7-two` is not +/// inside `...\tty7`. +pub(crate) fn path_is_under(path: &std::path::Path, dir: &std::path::Path) -> bool { + let (Some(path), Some(dir)) = (path.to_str(), dir.to_str()) else { + return false; + }; + let path = path.to_lowercase().replace('/', "\\"); + let mut dir = dir + .trim_end_matches(['\\', '/']) + .to_lowercase() + .replace('/', "\\"); + if dir.is_empty() { + return false; + } + dir.push('\\'); + path.starts_with(&dir) +} + #[cfg(test)] mod tests { use super::*; @@ -159,6 +258,30 @@ mod tests { assert_eq!(foreground_name(&procs, 100).as_deref(), Some("b")); } + #[test] + fn path_is_under_respects_component_boundaries_and_case() { + use std::path::Path; + let dir = Path::new(r"C:\Users\me\Apps\tty7"); + assert!(path_is_under( + Path::new(r"C:\Users\me\Apps\tty7\OpenConsole.exe"), + dir + )); + assert!(path_is_under( + Path::new(r"c:\users\me\apps\TTY7\server\tty7-server"), + dir + )); + assert!(!path_is_under( + Path::new(r"C:\Users\me\Apps\tty7-two\a.exe"), + dir + )); + assert!(!path_is_under(Path::new(r"C:\Users\me\Apps\tty7"), dir)); + assert!(path_is_under( + Path::new(r"C:\Users\me\Apps\tty7\x.exe"), + Path::new(r"C:\Users\me\Apps\tty7\"), + )); + assert!(!path_is_under(Path::new(r"C:\anything"), Path::new(""))); + } + #[test] fn exe_name_reads_up_to_the_nul() { let mut raw = [0u16; 260]; diff --git a/src/bin/tty7-updater.rs b/src/bin/tty7-updater.rs index e15aec45..ee1a7779 100644 --- a/src/bin/tty7-updater.rs +++ b/src/bin/tty7-updater.rs @@ -668,6 +668,18 @@ mod windows { return recover_from_failed_update(&plan, error); } + // Setup's own PrepareToInstall repeats this, but doing it here first + // means a directory that cannot be cleared fails with a named cause in + // this log instead of Inno's bare "DeleteFile failed; code 5" — and the + // previous app is relaunched instead of being left half-replaced. + log_line( + &plan.log, + "stopping the tty7 daemon and clearing installed-file locks", + ); + if let Err(error) = tty7_core::daemon::spawn::stop_for_update(&plan.install_dir) { + return recover_from_failed_update(&plan, error); + } + log_line(&plan.log, "running the tty7 Windows installer"); let status = match run_installer(&plan.installer, &plan.log) { Ok(status) => status, @@ -726,7 +738,7 @@ mod windows { &plan.log, "stopping the tty7 daemon before replacing portable files", ); - if let Err(error) = stop_daemon_from_payload(&payload) { + if let Err(error) = stop_daemon_from_payload(&payload, &plan.install_dir) { return recover_without_replacement(&plan.log, &plan.install_dir, &plan.stage, error); } @@ -1097,10 +1109,12 @@ mod windows { .map_err(|error| format!("starting {}: {error}", installer.display())) } - fn stop_daemon_from_payload(payload: &Path) -> Result<(), String> { + fn stop_daemon_from_payload(payload: &Path, install_dir: &Path) -> Result<(), String> { let executable = payload.join("tty7-app.exe"); let status = Command::new(&executable) .arg("--stop-daemon") + .arg("--update-install-dir") + .arg(install_dir) .stdin(Stdio::null()) .stdout(Stdio::null()) .stderr(Stdio::null()) diff --git a/src/core/update.rs b/src/core/update.rs index 4b321578..f7fee426 100644 --- a/src/core/update.rs +++ b/src/core/update.rs @@ -406,8 +406,16 @@ fn prompt_update(update: &AvailableUpdate, window: &mut Window, cx: &mut App) { .as_ref() .map(localized_update_install_hint); let detail = if update.installable { + // Windows cannot replace a running daemon's image, so its install path + // stops the background service — the promise that panes survive is + // only true where the daemon really does keep running (macOS). + let detail_key = if cfg!(target_os = "windows") { + L10nKey::UpdateDialogDetailWindows + } else { + L10nKey::UpdateDialogDetail + }; let base = t_fmt( - L10nKey::UpdateDialogDetail, + detail_key, &[ ("version", update.version.as_str()), ("current", current_version()), diff --git a/src/main.rs b/src/main.rs index 894d4343..e65aa1b6 100644 --- a/src/main.rs +++ b/src/main.rs @@ -143,6 +143,21 @@ fn open_path_from( None } +/// The directory an installer is about to replace, from +/// `--stop-daemon --update-install-dir `. Meaningful only next to +/// `--stop-daemon`; the caller checks that flag first. +#[cfg(windows)] +fn update_install_dir_from( + mut args: impl Iterator, +) -> Option { + while let Some(arg) = args.next() { + if arg == std::ffi::OsStr::new("--update-install-dir") { + return args.next().map(Into::into); + } + } + None +} + /// `Some(true)` to register the Explorer verbs, `Some(false)` to remove them. fn explorer_menu_action_from(args: &[std::ffi::OsString]) -> Option { args.iter().find_map(|arg| match arg.as_os_str() { @@ -343,6 +358,22 @@ fn main() { .iter() .any(|arg| arg == std::ffi::OsStr::new("--stop-daemon")) { + // An installer about to replace `dir` says so, and gets more than a + // stop: orphaned ConPTY hosts and anything else still running from + // that directory are terminated, and the call does not return until + // the images there are actually replaceable (or says why they are + // not). Invoked by the Inno PrepareToInstall step and the updater. + #[cfg(windows)] + if let Some(dir) = update_install_dir_from(args.iter().cloned()) { + if let Err(error) = crate::daemon::spawn::stop_for_update(&dir) { + log::error!( + "preparing {} for replacement failed: {error}", + dir.display() + ); + std::process::exit(1); + } + return; + } crate::daemon::spawn::stop(); return; } diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index f8767753..14dfdb0f 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -496,6 +496,9 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::UpdateDialogDetail => { "tty7 {version} is available — you're on {current}. Installing restarts the app; the background service keeps running, so whatever is open in your panes survives." } + L10nKey::UpdateDialogDetailWindows => { + "tty7 {version} is available — you're on {current}. Installing restarts the app and the background service: processes running in your panes are ended, and your tabs and layout come back with fresh shells." + } L10nKey::UpdateDialogDetailManual => { "tty7 {version} is available — you're on {current}. {hint}" } diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index 2641042d..dda2817f 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -496,6 +496,9 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::UpdateDialogDetail => { "tty7 {version} が利用できます（現在 {current}）。インストールするとアプリが再起動します。バックグラウンドサービスは動いたままなので、ペインで開いているものはそのまま残ります。" } + L10nKey::UpdateDialogDetailWindows => { + "tty7 {version} が利用できます（現在 {current}）。インストールするとアプリとバックグラウンドサービスが再起動します。ペインで実行中のプロセスは終了し、タブとレイアウトは新しいシェルで復元されます。" + } L10nKey::UpdateDialogDetailManual => { "tty7 {version} が利用できます（現在 {current}）。{hint}" } diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index add96591..88f175a6 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -420,6 +420,7 @@ pub enum L10nKey { SettingsDaemonStaleRestart, UpdateDialogTitle, UpdateDialogDetail, + UpdateDialogDetailWindows, UpdateDialogDetailManual, UpdateDialogCannotSelfUpdate, UpdateDialogLater, @@ -1434,6 +1435,7 @@ mod tests { L10nKey::SettingsDaemonStaleRestart, L10nKey::UpdateDialogTitle, L10nKey::UpdateDialogDetail, + L10nKey::UpdateDialogDetailWindows, L10nKey::UpdateDialogDetailManual, L10nKey::UpdateDialogCannotSelfUpdate, L10nKey::UpdateDialogLater, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index cd02070f..0ecf6886 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -439,6 +439,9 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::UpdateDialogDetail => { "tty7 {version} 已发布，你现在是 {current}。安装会重启应用；后台服务不动，pane 里开着的东西都还在。" } + L10nKey::UpdateDialogDetailWindows => { + "tty7 {version} 已发布，你现在是 {current}。安装会重启应用和后台服务：pane 里正在运行的进程会被结束，标签页和布局会以全新的 shell 恢复。" + } L10nKey::UpdateDialogDetailManual => "tty7 {version} 已发布，你现在是 {current}。{hint}", L10nKey::UpdateDialogCannotSelfUpdate => "这份安装无法自行更新。", L10nKey::UpdateDialogLater => "以后再说",