mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
fix(installer): refuse a second updater instead of clobbering the marker
UpdateMarkerGuard::acquire overwrote the in-progress marker unconditionally, so a Tauri update launched while a dashboard-spawned "hermes update" was mid-flight simply took the marker and ran a second updater over the same checkout. That is the race behind the reported Windows failure: install-mode bootstrap rewound the tree while the dashboard's updater was still running npm install against it. acquire now returns Result and refuses when a live foreign owner holds the marker, and Drop no longer deletes a marker this process does not own. Liveness matches the Python and Electron readers of the same file: dead pid or past the shared age ceiling means stale and reclaimable, so a crashed updater cannot wedge future updates. Adds a cfg(unix) libc dependency for the signal-0 liveness probe; the Windows path uses OpenProcess/GetExitCodeProcess.
This commit is contained in:
parent
fe8e4d93da
commit
38d5f44df1
2 changed files with 203 additions and 9 deletions
|
|
@ -66,6 +66,10 @@ windows-sys = { version = "0.59", features = [
|
|||
"Win32_UI_WindowsAndMessaging",
|
||||
] }
|
||||
|
||||
# Signal-0 liveness probe for the update-lock marker owner (update.rs).
|
||||
[target.'cfg(unix)'.dependencies]
|
||||
libc = "0.2"
|
||||
|
||||
[profile.release]
|
||||
# A 5-10MB signed installer is the goal. LTO + size-opt + single codegen unit.
|
||||
panic = "abort"
|
||||
|
|
|
|||
|
|
@ -107,16 +107,97 @@ pub async fn start_update(app: AppHandle) -> Result<(), String> {
|
|||
/// future desktop launches. The marker payload is `{pid}\n{started_at_unix}`
|
||||
/// so the desktop's launch gate can detect a stale marker (dead PID / past a
|
||||
/// hard ceiling) and self-heal rather than wait forever.
|
||||
///
|
||||
/// The marker is also the cross-process update lock: `hermes update` claims
|
||||
/// the same file (see `hermes_cli/update_lock.py`) so a dashboard-spawned
|
||||
/// update and this updater can't mutate one checkout at the same time.
|
||||
/// `acquire` therefore REFUSES when a live foreign owner holds it rather than
|
||||
/// overwriting — the pre-fix clobber is what let a dashboard `hermes update`
|
||||
/// keep running while install-mode bootstrap rewrote the tree underneath it.
|
||||
struct UpdateMarkerGuard {
|
||||
path: PathBuf,
|
||||
/// False when a live foreign updater already owns the marker: we hold no
|
||||
/// claim, so `Drop` must not delete their marker.
|
||||
owned: bool,
|
||||
}
|
||||
|
||||
/// Never treat a marker older than this as a live update. Mirrors
|
||||
/// UPDATE_MARKER_MAX_AGE_MS in apps/desktop/electron/update-marker.ts and
|
||||
/// UPDATE_MARKER_MAX_AGE_SECONDS in hermes_cli/update_lock.py — all three read
|
||||
/// this one file, so a shorter ceiling in any of them would steal a lock the
|
||||
/// others still consider live.
|
||||
const UPDATE_MARKER_MAX_AGE_SECS: u64 = 20 * 60;
|
||||
|
||||
/// The pid + age of a confirmed-live update holding the marker.
|
||||
struct MarkerOwner {
|
||||
pid: u32,
|
||||
age_secs: u64,
|
||||
}
|
||||
|
||||
/// Read the marker and report a live owner, if any. `None` for every "no live
|
||||
/// update" case — absent, unreadable, malformed, dead pid, past the ceiling —
|
||||
/// matching `readLiveUpdateMarker` in the Electron gate. Never panics.
|
||||
fn live_marker_owner(path: &Path) -> Option<MarkerOwner> {
|
||||
let raw = std::fs::read_to_string(path).ok()?;
|
||||
let mut lines = raw.lines();
|
||||
let pid: u32 = lines.next()?.trim().parse().ok()?;
|
||||
let started_at: u64 = lines.next().unwrap_or("").trim().parse().unwrap_or(0);
|
||||
let now = std::time::SystemTime::now()
|
||||
.duration_since(std::time::UNIX_EPOCH)
|
||||
.map(|d| d.as_secs())
|
||||
.unwrap_or(0);
|
||||
let age_secs = now.saturating_sub(started_at);
|
||||
if age_secs > UPDATE_MARKER_MAX_AGE_SECS || !pid_is_alive(pid) {
|
||||
return None;
|
||||
}
|
||||
Some(MarkerOwner { pid, age_secs })
|
||||
}
|
||||
|
||||
/// True when a process with `pid` currently exists.
|
||||
#[cfg(windows)]
|
||||
fn pid_is_alive(pid: u32) -> bool {
|
||||
use windows_sys::Win32::Foundation::{CloseHandle, STILL_ACTIVE};
|
||||
use windows_sys::Win32::System::Threading::{
|
||||
GetExitCodeProcess, OpenProcess, PROCESS_QUERY_LIMITED_INFORMATION,
|
||||
};
|
||||
|
||||
unsafe {
|
||||
let handle = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, 0, pid);
|
||||
if handle.is_null() {
|
||||
// Either the pid is gone or we lack rights to open it. A pid we
|
||||
// can't inspect is treated as dead so an unopenable straggler
|
||||
// can't wedge every future update.
|
||||
return false;
|
||||
}
|
||||
let mut code: u32 = 0;
|
||||
let ok = GetExitCodeProcess(handle, &mut code);
|
||||
CloseHandle(handle);
|
||||
ok != 0 && code == STILL_ACTIVE as u32
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(not(windows))]
|
||||
fn pid_is_alive(pid: u32) -> bool {
|
||||
// signal 0 delivers nothing; it only probes existence/permission.
|
||||
// ESRCH => dead. EPERM => alive but owned by another user.
|
||||
let rc = unsafe { libc::kill(pid as libc::pid_t, 0) };
|
||||
if rc == 0 {
|
||||
return true;
|
||||
}
|
||||
std::io::Error::last_os_error().raw_os_error() == Some(libc::EPERM)
|
||||
}
|
||||
|
||||
impl UpdateMarkerGuard {
|
||||
/// Write the marker. Best-effort: a write failure must NOT abort the
|
||||
/// update (the gate degrades to "no marker => proceed", i.e. exactly the
|
||||
/// pre-fix behavior), so we log and carry on with a guard that still
|
||||
/// attempts cleanup of whatever may exist at the path.
|
||||
fn acquire(path: PathBuf) -> Self {
|
||||
/// Claim the marker, or report the live updater that already owns it.
|
||||
///
|
||||
/// Writing is best-effort: a write failure must NOT abort the update (the
|
||||
/// gate degrades to "no marker => proceed", i.e. exactly the pre-marker
|
||||
/// behavior), so we log and carry on with a guard that still attempts
|
||||
/// cleanup of whatever may exist at the path.
|
||||
fn acquire(path: PathBuf) -> Result<Self, MarkerOwner> {
|
||||
if let Some(owner) = live_marker_owner(&path) {
|
||||
return Err(owner);
|
||||
}
|
||||
let pid = std::process::id();
|
||||
let started_at = std::time::SystemTime::now()
|
||||
.duration_since(std::time::UNIX_EPOCH)
|
||||
|
|
@ -128,12 +209,15 @@ impl UpdateMarkerGuard {
|
|||
if let Err(err) = std::fs::write(&path, format!("{pid}\n{started_at}")) {
|
||||
tracing::warn!(?path, %err, "could not write update-in-progress marker");
|
||||
}
|
||||
Self { path }
|
||||
Ok(Self { path, owned: true })
|
||||
}
|
||||
}
|
||||
|
||||
impl Drop for UpdateMarkerGuard {
|
||||
fn drop(&mut self) {
|
||||
if !self.owned {
|
||||
return;
|
||||
}
|
||||
if let Err(err) = std::fs::remove_file(&self.path) {
|
||||
if err.kind() != std::io::ErrorKind::NotFound {
|
||||
tracing::warn!(path = ?self.path, %err, "could not remove update-in-progress marker");
|
||||
|
|
@ -152,7 +236,39 @@ async fn run_update(app: AppHandle) -> Result<()> {
|
|||
// it, that backend re-locks the venv shim, our `force_kill_other_hermes`
|
||||
// straggler-cleanup kills it, and the relaunch/kill cycle loops. The guard
|
||||
// removes the marker on every exit path (incl. early returns / panics).
|
||||
let _update_marker = UpdateMarkerGuard::acquire(crate::paths::update_in_progress_marker());
|
||||
//
|
||||
// The same marker is the cross-process update lock (hermes_cli/
|
||||
// update_lock.py claims it too), so a live foreign owner means another
|
||||
// updater — most often a dashboard-spawned `hermes update` — is already
|
||||
// mutating this checkout. Refuse instead of running a second one over it.
|
||||
let _update_marker = match UpdateMarkerGuard::acquire(
|
||||
crate::paths::update_in_progress_marker(),
|
||||
) {
|
||||
Ok(guard) => guard,
|
||||
Err(owner) => {
|
||||
let mins = owner.age_secs / 60;
|
||||
let secs = owner.age_secs % 60;
|
||||
let elapsed = if mins > 0 {
|
||||
format!("{mins}m {secs}s")
|
||||
} else {
|
||||
format!("{secs}s")
|
||||
};
|
||||
let msg = format!(
|
||||
"Another Hermes update is already running (PID {}, started {} ago). \
|
||||
Wait for it to finish, or close the window or dashboard tab that \
|
||||
started it, then try again.",
|
||||
owner.pid, elapsed
|
||||
);
|
||||
emit(
|
||||
&app,
|
||||
BootstrapEvent::Failed {
|
||||
stage: None,
|
||||
error: msg.clone(),
|
||||
},
|
||||
);
|
||||
return Err(anyhow!(msg));
|
||||
}
|
||||
};
|
||||
|
||||
let update_branch = update_branch_from_args(std::env::args().skip(1))
|
||||
.or_else(|| option_env_string("BUILD_PIN_BRANCH"))
|
||||
|
|
@ -1102,7 +1218,8 @@ mod tests {
|
|||
let marker = dir.join(".hermes-update-in-progress");
|
||||
|
||||
{
|
||||
let _g = UpdateMarkerGuard::acquire(marker.clone());
|
||||
let _g = UpdateMarkerGuard::acquire(marker.clone())
|
||||
.unwrap_or_else(|_| panic!("no live owner => acquire must succeed"));
|
||||
assert!(marker.exists(), "marker must exist while the guard is held");
|
||||
let body = std::fs::read_to_string(&marker).unwrap();
|
||||
let pid_line = body.lines().next().unwrap();
|
||||
|
|
@ -1127,7 +1244,8 @@ mod tests {
|
|||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let marker = dir.join(".hermes-update-in-progress");
|
||||
|
||||
let guard = UpdateMarkerGuard::acquire(marker.clone());
|
||||
let guard = UpdateMarkerGuard::acquire(marker.clone())
|
||||
.unwrap_or_else(|_| panic!("no live owner => acquire must succeed"));
|
||||
// Simulate an external cleanup (e.g. the desktop pruned a marker it
|
||||
// judged stale) before our guard drops — Drop must not panic.
|
||||
std::fs::remove_file(&marker).unwrap();
|
||||
|
|
@ -1137,6 +1255,78 @@ mod tests {
|
|||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn acquire_refuses_while_a_live_updater_owns_the_marker() {
|
||||
let dir = unique_tmp_dir("marker-contended");
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let marker = dir.join(".hermes-update-in-progress");
|
||||
|
||||
// A live updater (us) holds it. A second updater must NOT clobber the
|
||||
// marker and run concurrently over the same checkout — that race is
|
||||
// what let a dashboard `hermes update` and install-mode bootstrap
|
||||
// mutate one tree at once.
|
||||
let held = UpdateMarkerGuard::acquire(marker.clone())
|
||||
.unwrap_or_else(|_| panic!("first acquire must succeed"));
|
||||
let owner = UpdateMarkerGuard::acquire(marker.clone())
|
||||
.err()
|
||||
.expect("second acquire must be refused while the first is live");
|
||||
assert_eq!(owner.pid, std::process::id());
|
||||
|
||||
// The refused guard must not delete the live owner's marker.
|
||||
assert!(marker.exists(), "refused acquire must leave the marker intact");
|
||||
drop(held);
|
||||
assert!(!marker.exists(), "the real owner still cleans up on drop");
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn acquire_reclaims_a_marker_owned_by_a_dead_pid() {
|
||||
let dir = unique_tmp_dir("marker-dead-pid");
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let marker = dir.join(".hermes-update-in-progress");
|
||||
|
||||
// pid 1 exists everywhere, so fabricate a dead one: a very large pid
|
||||
// that no live process owns. A crashed updater must never wedge every
|
||||
// future update.
|
||||
let started_at = std::time::SystemTime::now()
|
||||
.duration_since(std::time::UNIX_EPOCH)
|
||||
.map(|d| d.as_secs())
|
||||
.unwrap_or(0);
|
||||
std::fs::write(&marker, format!("4294967294\n{started_at}")).unwrap();
|
||||
|
||||
let guard = UpdateMarkerGuard::acquire(marker.clone())
|
||||
.unwrap_or_else(|_| panic!("a dead owner must not block acquisition"));
|
||||
let body = std::fs::read_to_string(&marker).unwrap();
|
||||
assert_eq!(
|
||||
body.lines().next().unwrap().trim().parse::<u32>().unwrap(),
|
||||
std::process::id(),
|
||||
"reclaiming rewrites the marker with our pid"
|
||||
);
|
||||
drop(guard);
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn acquire_reclaims_a_marker_past_the_age_ceiling() {
|
||||
let dir = unique_tmp_dir("marker-stale-age");
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let marker = dir.join(".hermes-update-in-progress");
|
||||
|
||||
// Our own (live) pid, but started well past the ceiling: a wedged
|
||||
// updater must not hold the lock forever.
|
||||
let long_ago = std::time::SystemTime::now()
|
||||
.duration_since(std::time::UNIX_EPOCH)
|
||||
.map(|d| d.as_secs())
|
||||
.unwrap_or(0)
|
||||
.saturating_sub(UPDATE_MARKER_MAX_AGE_SECS + 60);
|
||||
std::fs::write(&marker, format!("{}\n{long_ago}", std::process::id())).unwrap();
|
||||
|
||||
let guard = UpdateMarkerGuard::acquire(marker.clone())
|
||||
.unwrap_or_else(|_| panic!("a marker past the ceiling must be reclaimable"));
|
||||
drop(guard);
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parses_update_branch_from_space_or_equals_args() {
|
||||
assert_eq!(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue