mirror of
https://github.com/l0ng-ai/tty7.git
synced 2026-09-22 00:02:23 +00:00
fix(update): close three gaps the update audit found
- macOS updater: wait for the parent by watching getppid() reparent to launchd instead of polling kill(pid, 0), which a recycled pid could satisfy forever. The kill loop remains only for a hand-run updater. - Windows: a new update guard (config-dir update.lock, held by the updater from daemon stop to relaunch) makes ensure_running refuse to spawn a daemon mid-install, so a tty7 CLI call or manual launch can no longer relock the images the installer is replacing. Stale guards — dead writer or past the TTL — are shed on sight. - Windows portable: the update backup now carries an incomplete marker from before the first file moves until the replacement lands. At launch the app reports a backup still carrying it as an interrupted update (the installation may mix two versions; the old files are preserved), and silently removes marker-less backups a finished update failed to delete past an antivirus hold.
This commit is contained in:
@@ -20,6 +20,12 @@ pub(crate) mod shell_integration;
|
||||
#[cfg(windows)]
|
||||
pub(crate) mod winproc;
|
||||
|
||||
/// Windows-only like the updater flow that holds it: the macOS updater swaps
|
||||
/// the bundle by rename and never stops the daemon, so it has no window in
|
||||
/// which a fresh daemon could relock anything.
|
||||
#[cfg(windows)]
|
||||
pub mod update_guard;
|
||||
|
||||
/// The Windows environment refresh new panes get (#333). Only the registry
|
||||
/// reader and the spawn wiring are Windows-only; the merge itself is a pure
|
||||
/// function, so `cfg(test)` keeps the module compiling everywhere and its
|
||||
|
||||
@@ -198,6 +198,18 @@ pub fn ensure_running() -> anyhow::Result<()> {
|
||||
}
|
||||
}
|
||||
|
||||
// While an updater is replacing the installation, spawning a daemon would
|
||||
// relock the very images the installer is clearing — the update would fail
|
||||
// with "files in use" caused by us. Connecting to a live daemon above is
|
||||
// fine; only creating a new one waits.
|
||||
#[cfg(windows)]
|
||||
if crate::daemon::update_guard::held() {
|
||||
anyhow::bail!(
|
||||
"a tty7 update is being installed right now; the daemon will return \
|
||||
when the updater relaunches the app"
|
||||
);
|
||||
}
|
||||
|
||||
spawn_detached()?;
|
||||
|
||||
let deadline = Instant::now() + STARTUP_TIMEOUT;
|
||||
|
||||
@@ -0,0 +1,129 @@
|
||||
//! A file that says "an updater is replacing this installation right now".
|
||||
//!
|
||||
//! Between `spawn::stop_for_update` clearing the installed images and the
|
||||
//! installer finishing, nothing used to stop a `tty7` CLI call — or a
|
||||
//! manually launched GUI — from spawning a fresh daemon that relocks the very
|
||||
//! files being replaced. The guard closes that window: the updater holds it
|
||||
//! for the whole installation and releases it just before relaunching the
|
||||
//! app, and `spawn::ensure_running` refuses to spawn a daemon while it is
|
||||
//! held. Only spawning is deferred; connecting to a daemon that is already
|
||||
//! running stays untouched.
|
||||
//!
|
||||
//! The guard names its holder by pid so it can never outlive a crashed
|
||||
//! updater: a guard whose writer is gone is removed on sight, and a TTL
|
||||
//! backstops the one coincidence pid-liveness cannot see — the dead writer's
|
||||
//! pid recycled by an unrelated long-lived process.
|
||||
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::time::Duration;
|
||||
|
||||
use crate::core::config;
|
||||
|
||||
/// Far above any real installation's duration (Setup runs in seconds), and
|
||||
/// the most a recycled writer pid can cost.
|
||||
const GUARD_TTL: Duration = Duration::from_secs(10 * 60);
|
||||
|
||||
fn path() -> Option<PathBuf> {
|
||||
config::config_path("update.lock")
|
||||
}
|
||||
|
||||
/// Claims the guard for the calling process.
|
||||
pub fn hold() {
|
||||
let Some(path) = path() else { return };
|
||||
if let Some(parent) = path.parent() {
|
||||
let _ = std::fs::create_dir_all(parent);
|
||||
}
|
||||
if let Err(error) = std::fs::write(&path, std::process::id().to_string()) {
|
||||
log::warn!(
|
||||
"could not write the update guard {}: {error}",
|
||||
path.display()
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// Releases the guard. Harmless when it is not held.
|
||||
pub fn clear() {
|
||||
if let Some(path) = path() {
|
||||
let _ = std::fs::remove_file(path);
|
||||
}
|
||||
}
|
||||
|
||||
/// Whether an updater is installing right now. A guard whose writer has
|
||||
/// exited or that outlived the TTL is stale — the updater clears it on every
|
||||
/// path that relaunches the app, so a leftover means it died — and is removed
|
||||
/// here so one crash never costs more than one look.
|
||||
pub(crate) fn held() -> bool {
|
||||
let Some(path) = path() else { return false };
|
||||
let Ok(contents) = std::fs::read_to_string(&path) else {
|
||||
return false;
|
||||
};
|
||||
let live = contents
|
||||
.trim()
|
||||
.parse::<u32>()
|
||||
.is_ok_and(|pid| process_alive(pid));
|
||||
if !live || expired(&path) {
|
||||
log::info!("removing a stale update guard at {}", path.display());
|
||||
let _ = std::fs::remove_file(&path);
|
||||
return false;
|
||||
}
|
||||
true
|
||||
}
|
||||
|
||||
fn expired(path: &Path) -> bool {
|
||||
std::fs::metadata(path)
|
||||
.and_then(|meta| meta.modified())
|
||||
.ok()
|
||||
.and_then(|modified| modified.elapsed().ok())
|
||||
.is_some_and(|age| age > GUARD_TTL)
|
||||
}
|
||||
|
||||
fn process_alive(pid: u32) -> bool {
|
||||
!crate::daemon::winproc::wait_for_exit(pid, Duration::ZERO)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
fn pin_config_dir() {
|
||||
let dir = std::env::temp_dir().join(format!("tty7-covtest-{}", std::process::id()));
|
||||
std::fs::create_dir_all(&dir).ok();
|
||||
config::set_config_dir(dir);
|
||||
}
|
||||
|
||||
fn exited_pid() -> u32 {
|
||||
let mut child = std::process::Command::new("cmd")
|
||||
.args(["/C", "exit"])
|
||||
.spawn()
|
||||
.unwrap();
|
||||
let pid = child.id();
|
||||
child.wait().unwrap();
|
||||
pid
|
||||
}
|
||||
|
||||
// One test, like the pidfile's: the guard file is process-global state,
|
||||
// and two tests sharing it would race each other.
|
||||
#[test]
|
||||
fn guard_lifecycle_holds_for_a_live_writer_and_sheds_stale_files() {
|
||||
pin_config_dir();
|
||||
clear();
|
||||
assert!(!held(), "no guard file, no guard");
|
||||
|
||||
hold();
|
||||
assert!(held(), "this process is alive, so its guard holds");
|
||||
clear();
|
||||
assert!(!held());
|
||||
|
||||
std::fs::write(path().unwrap(), exited_pid().to_string()).unwrap();
|
||||
assert!(!held(), "a dead writer cannot be installing anything");
|
||||
assert!(
|
||||
!path().unwrap().exists(),
|
||||
"the stale guard is gone after one look"
|
||||
);
|
||||
|
||||
// Garbage is stale by the same rule.
|
||||
std::fs::write(path().unwrap(), "not-a-pid").unwrap();
|
||||
assert!(!held());
|
||||
assert!(!path().unwrap().exists());
|
||||
}
|
||||
}
|
||||
+94
-2
@@ -283,13 +283,28 @@ mod macos {
|
||||
}
|
||||
|
||||
fn wait_for_exit(pid: u32) {
|
||||
// The updater is spawned directly by the app it waits for, so while
|
||||
// that app lives it *is* this process's parent, and the kernel
|
||||
// reparents us to launchd the moment it exits. Watching getppid() is
|
||||
// therefore immune to pid reuse, which `kill(pid, 0)` is not: a
|
||||
// recycled pid keeps answering 0 forever. (Windows solves the same
|
||||
// race by holding a process handle — see the windows module.)
|
||||
let pid = pid as libc::pid_t;
|
||||
if unsafe { libc::getppid() } == pid {
|
||||
while unsafe { libc::getppid() } == pid {
|
||||
thread::sleep(PARENT_POLL);
|
||||
}
|
||||
return;
|
||||
}
|
||||
// Not our parent — a hand-run updater. The polling fallback keeps
|
||||
// that invocation working, pid-reuse caveat and all.
|
||||
while process_alive(pid) {
|
||||
thread::sleep(PARENT_POLL);
|
||||
}
|
||||
}
|
||||
|
||||
fn process_alive(pid: u32) -> bool {
|
||||
unsafe { libc::kill(pid as libc::pid_t, 0) == 0 }
|
||||
fn process_alive(pid: libc::pid_t) -> bool {
|
||||
unsafe { libc::kill(pid, 0) == 0 }
|
||||
}
|
||||
|
||||
fn remove_path(path: &Path) -> Result<(), String> {
|
||||
@@ -486,6 +501,14 @@ mod windows {
|
||||
const PORTABLE_PAYLOAD_DIR: &str = "portable-payload";
|
||||
const PORTABLE_MARKER: &str = ".tty7-portable";
|
||||
const PORTABLE_MARKER_CONTENT: &[u8] = b"portable-v1";
|
||||
/// Lives inside a portable-update backup from before the first installed
|
||||
/// file moves until the replacement is fully in place. A backup found
|
||||
/// later still carrying it names a replacement that was cut short —
|
||||
/// power loss, a kill — and an installation that may mix two versions;
|
||||
/// one without it is a finished update whose backup deletion lost to an
|
||||
/// antivirus scan. The app reads it at launch: duplicated in
|
||||
/// src/core/update.rs, like the portable marker above.
|
||||
const PORTABLE_BACKUP_INCOMPLETE: &str = ".tty7-replace-incomplete";
|
||||
const MAX_PORTABLE_ENTRIES: usize = 4096;
|
||||
const MAX_PORTABLE_EXPANDED_BYTES: u64 = 1024 * 1024 * 1024;
|
||||
// Everything the release package owns: an entry outside this list is
|
||||
@@ -676,6 +699,10 @@ mod windows {
|
||||
&plan.log,
|
||||
"stopping the tty7 daemon and clearing installed-file locks",
|
||||
);
|
||||
// From here until the relaunch, a `tty7` CLI call or a manual launch
|
||||
// must not spawn a daemon that relocks the files Setup is replacing.
|
||||
// launch_app releases the guard on every path out of this function.
|
||||
tty7_core::daemon::update_guard::hold();
|
||||
if let Err(error) = tty7_core::daemon::spawn::stop_for_update(&plan.install_dir) {
|
||||
return recover_from_failed_update(&plan, error);
|
||||
}
|
||||
@@ -738,6 +765,10 @@ mod windows {
|
||||
&plan.log,
|
||||
"stopping the tty7 daemon before replacing portable files",
|
||||
);
|
||||
// Held for the whole replacement, exactly as in `install`; the pid it
|
||||
// records must be this process's — the payload child below exits
|
||||
// immediately, and a guard naming a dead writer holds nothing.
|
||||
tty7_core::daemon::update_guard::hold();
|
||||
if let Err(error) = stop_daemon_from_payload(&payload, &plan.install_dir) {
|
||||
return recover_without_replacement(&plan.log, &plan.install_dir, &plan.stage, error);
|
||||
}
|
||||
@@ -1158,6 +1189,15 @@ mod windows {
|
||||
return Err(with_relaunch_failure(cause, relaunch_previous(install_dir)));
|
||||
}
|
||||
};
|
||||
// Marked incomplete before any installed file moves. Nothing here
|
||||
// removes the marker on rollback: a rollback that succeeds removes
|
||||
// the whole backup, and one that fails leaves an installation whose
|
||||
// state really is suspect.
|
||||
if let Err(error) = fs::write(backup.join(PORTABLE_BACKUP_INCOMPLETE), b"") {
|
||||
let cause = format!("marking the update backup {}: {error}", backup.display());
|
||||
let _ = remove_path(&backup);
|
||||
return Err(with_relaunch_failure(cause, relaunch_previous(install_dir)));
|
||||
}
|
||||
|
||||
let mut moved = Vec::new();
|
||||
for root in PORTABLE_MANAGED_ROOTS {
|
||||
@@ -1197,6 +1237,9 @@ mod windows {
|
||||
// The replacement survived its launch grace period. Old managed files
|
||||
// are no longer needed; an antivirus-held backup is harmless and can be
|
||||
// removed manually rather than turning a successful update into rollback.
|
||||
// The marker leaves first: a backup that outlives this process without
|
||||
// it is finished business the next launch may discard on its own.
|
||||
let _ = fs::remove_file(backup.join(PORTABLE_BACKUP_INCOMPLETE));
|
||||
let _ = remove_path(&backup);
|
||||
Ok(())
|
||||
}
|
||||
@@ -1338,6 +1381,10 @@ mod windows {
|
||||
}
|
||||
|
||||
fn launch_app(install_dir: &Path) -> Result<(), String> {
|
||||
// Every relaunch — success, failure recovery, rollback — is a point
|
||||
// where the installation is no longer being replaced, so the daemon
|
||||
// spawn guard ends here, before the app comes up and asks for one.
|
||||
tty7_core::daemon::update_guard::clear();
|
||||
let executable = install_dir.join("tty7-app.exe");
|
||||
let mut child = Command::new(&executable)
|
||||
.stdin(Stdio::null())
|
||||
@@ -1861,6 +1908,51 @@ mod windows {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_backup_carries_the_incomplete_marker_exactly_while_files_move() {
|
||||
let install = tempfile::tempdir().unwrap();
|
||||
let payload = tempfile::tempdir().unwrap();
|
||||
fs::write(install.path().join("tty7-app.exe"), b"old app").unwrap();
|
||||
fs::write(payload.path().join("tty7-app.exe"), b"new app").unwrap();
|
||||
let install_dir = install.path().to_path_buf();
|
||||
|
||||
replace_portable_and_relaunch(
|
||||
install.path(),
|
||||
payload.path(),
|
||||
move |_| {
|
||||
let backups: Vec<_> = fs::read_dir(&install_dir)
|
||||
.unwrap()
|
||||
.flatten()
|
||||
.map(|entry| entry.path())
|
||||
.filter(|path| {
|
||||
path.file_name()
|
||||
.and_then(|name| name.to_str())
|
||||
.is_some_and(|name| name.starts_with(".tty7-update-backup-"))
|
||||
})
|
||||
.collect();
|
||||
assert_eq!(backups.len(), 1, "one backup during the replacement");
|
||||
assert!(
|
||||
backups[0].join(PORTABLE_BACKUP_INCOMPLETE).is_file(),
|
||||
"the marker is present while files are moving"
|
||||
);
|
||||
Ok(())
|
||||
},
|
||||
|_| panic!("the previous version must not relaunch after success"),
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let leftovers: Vec<_> = fs::read_dir(install.path())
|
||||
.unwrap()
|
||||
.flatten()
|
||||
.map(|entry| entry.file_name())
|
||||
.collect();
|
||||
assert_eq!(
|
||||
leftovers,
|
||||
vec![std::ffi::OsString::from("tty7-app.exe")],
|
||||
"no backup and no marker survive a completed replacement"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn portable_replacement_rolls_back_when_the_new_app_does_not_start() {
|
||||
let install = tempfile::tempdir().unwrap();
|
||||
|
||||
+112
-1
@@ -89,6 +89,14 @@ const WINDOWS_INNO_INSTALL_MARKER: &str = ".tty7-inno-install";
|
||||
const WINDOWS_PORTABLE_MARKER: &str = ".tty7-portable";
|
||||
#[cfg(target_os = "windows")]
|
||||
const WINDOWS_PORTABLE_MARKER_CONTENT: &[u8] = b"portable-v1";
|
||||
/// What the updater names the directory it moves the old portable files into.
|
||||
#[cfg(target_os = "windows")]
|
||||
const WINDOWS_PORTABLE_BACKUP_PREFIX: &str = ".tty7-update-backup-";
|
||||
/// Present inside that backup from before the first installed file moves
|
||||
/// until the replacement is complete — see `PORTABLE_BACKUP_INCOMPLETE` in
|
||||
/// src/bin/tty7-updater.rs, which this duplicates like the markers above.
|
||||
#[cfg(target_os = "windows")]
|
||||
const WINDOWS_PORTABLE_BACKUP_INCOMPLETE: &str = ".tty7-replace-incomplete";
|
||||
|
||||
#[derive(Clone, Debug, PartialEq, Eq)]
|
||||
pub struct AvailableUpdate {
|
||||
@@ -188,6 +196,15 @@ pub struct UpdateStatus {
|
||||
impl Global for UpdateStatus {}
|
||||
|
||||
pub fn spawn_check(cx: &mut App) {
|
||||
// Before hydration and before the config gate: an interrupted portable
|
||||
// replacement has to surface whether or not checking is on, and it is
|
||||
// recorded as a failure so `hydrate_from_disk` below carries it into the
|
||||
// UI. The scan itself is one `read_dir`; the deletions it hands back ride
|
||||
// the background sweep.
|
||||
#[cfg(target_os = "windows")]
|
||||
let finished_backups = reconcile_portable_backups();
|
||||
#[cfg(not(target_os = "windows"))]
|
||||
let finished_backups: Vec<PathBuf> = Vec::new();
|
||||
// Before the config gate: a package staged by an earlier run, or a failure
|
||||
// from one, has to reach Settings whether or not checking is still on.
|
||||
hydrate_from_disk(cx);
|
||||
@@ -195,7 +212,16 @@ pub fn spawn_check(cx: &mut App) {
|
||||
// waits on the result.
|
||||
let keep = UpdateState::load().pending.map(|pending| pending.stage);
|
||||
cx.background_executor()
|
||||
.spawn(async move { sweep_orphaned_stages(keep) })
|
||||
.spawn(async move {
|
||||
for backup in finished_backups {
|
||||
log::info!(
|
||||
"removing a completed update's leftover backup at {}",
|
||||
backup.display()
|
||||
);
|
||||
let _ = std::fs::remove_dir_all(&backup);
|
||||
}
|
||||
sweep_orphaned_stages(keep)
|
||||
})
|
||||
.detach();
|
||||
if !cx.global::<Config>().check_for_updates {
|
||||
return;
|
||||
@@ -971,6 +997,73 @@ fn is_stage_name(name: &str) -> bool {
|
||||
name.starts_with(".tty7-update-") || name.starts_with("tty7-update-")
|
||||
}
|
||||
|
||||
/// Deals with `.tty7-update-backup-*` directories a previous portable update
|
||||
/// left in the installation.
|
||||
///
|
||||
/// One still carrying the incomplete marker means the replacement was cut
|
||||
/// short — power loss, a kill — and the installed files may mix two versions.
|
||||
/// That is recorded as an update failure so Settings shows it (and shows it
|
||||
/// again at every launch until the user restores or deletes the backup: the
|
||||
/// warning describes a condition, not an event). The backup itself is kept —
|
||||
/// it holds the only copy of the previous files.
|
||||
///
|
||||
/// One without the marker is a completed update whose backup deletion lost to
|
||||
/// an antivirus scan; those are returned for the background sweep to remove.
|
||||
#[cfg(target_os = "windows")]
|
||||
fn reconcile_portable_backups() -> Vec<PathBuf> {
|
||||
let Some(WindowsUpdateLayout::Portable(dir)) = current_windows_update_layout() else {
|
||||
return Vec::new();
|
||||
};
|
||||
let (interrupted, finished) = scan_portable_backups(&dir);
|
||||
if let Some(backup) = interrupted.first() {
|
||||
let detail = format!(
|
||||
"a previous update was interrupted while replacing the installed files, so {} may \
|
||||
mix two versions; the files from before that update are preserved at {} — restore \
|
||||
them or reinstall, then delete the backup",
|
||||
dir.display(),
|
||||
backup.display()
|
||||
);
|
||||
log::warn!("{detail}");
|
||||
let mut state = UpdateState::load();
|
||||
if state.last_failure.as_ref().map(|failure| &failure.detail) != Some(&detail) {
|
||||
state.last_failure = Some(FailureRecord {
|
||||
version: current_version().to_string(),
|
||||
detail,
|
||||
});
|
||||
state.save();
|
||||
}
|
||||
}
|
||||
finished
|
||||
}
|
||||
|
||||
/// Splits the backups under `dir` into (interrupted, finished) by the
|
||||
/// incomplete marker. Pure directory inspection, so the policy above stays
|
||||
/// testable without a real installation.
|
||||
#[cfg(target_os = "windows")]
|
||||
fn scan_portable_backups(dir: &Path) -> (Vec<PathBuf>, Vec<PathBuf>) {
|
||||
let mut interrupted = Vec::new();
|
||||
let mut finished = Vec::new();
|
||||
let Ok(entries) = std::fs::read_dir(dir) else {
|
||||
return (interrupted, finished);
|
||||
};
|
||||
for entry in entries.flatten() {
|
||||
let path = entry.path();
|
||||
let named = path
|
||||
.file_name()
|
||||
.and_then(|name| name.to_str())
|
||||
.is_some_and(|name| name.starts_with(WINDOWS_PORTABLE_BACKUP_PREFIX));
|
||||
if !named || !path.is_dir() {
|
||||
continue;
|
||||
}
|
||||
if path.join(WINDOWS_PORTABLE_BACKUP_INCOMPLETE).is_file() {
|
||||
interrupted.push(path);
|
||||
} else {
|
||||
finished.push(path);
|
||||
}
|
||||
}
|
||||
(interrupted, finished)
|
||||
}
|
||||
|
||||
pub(crate) fn localized_update_phase(phase: &UpdatePhase) -> Option<String> {
|
||||
match phase {
|
||||
UpdatePhase::Idle => None,
|
||||
@@ -2039,6 +2132,24 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(target_os = "windows")]
|
||||
#[test]
|
||||
fn portable_backup_scan_separates_interrupted_from_finished() {
|
||||
let root = tempfile::tempdir().unwrap();
|
||||
let interrupted = root.path().join(".tty7-update-backup-cut");
|
||||
std::fs::create_dir(&interrupted).unwrap();
|
||||
std::fs::write(interrupted.join(WINDOWS_PORTABLE_BACKUP_INCOMPLETE), b"").unwrap();
|
||||
let finished = root.path().join(".tty7-update-backup-done");
|
||||
std::fs::create_dir(&finished).unwrap();
|
||||
// Neither a user's directory nor a stray file may be touched.
|
||||
std::fs::create_dir(root.path().join("completions")).unwrap();
|
||||
std::fs::write(root.path().join(".tty7-update-backup-not-a-dir"), b"file").unwrap();
|
||||
|
||||
let (got_interrupted, got_finished) = scan_portable_backups(root.path());
|
||||
assert_eq!(got_interrupted, vec![interrupted]);
|
||||
assert_eq!(got_finished, vec![finished]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parses_versions_with_and_without_prefix() {
|
||||
let release = |major, minor, patch| Some((major, minor, patch, true, vec![]));
|
||||
|
||||
Reference in New Issue
Block a user