diff --git a/apps/bootstrap-installer/src-tauri/src/update.rs b/apps/bootstrap-installer/src-tauri/src/update.rs index 7df1903820..034f82f497 100644 --- a/apps/bootstrap-installer/src-tauri/src/update.rs +++ b/apps/bootstrap-installer/src-tauri/src/update.rs @@ -133,17 +133,13 @@ struct MarkerOwner { age_secs: u64, } -/// Read the marker and report a live *foreign* owner, if any. `None` for every -/// "no live update" case — absent, unreadable, malformed, dead pid, past the -/// ceiling, or a marker whose pid is **this** process — matching -/// `readLiveUpdateMarker` in the Electron gate. Never panics. +/// Read the marker and report a live owner, if any. `None` for every "no live +/// update" case — absent, unreadable, malformed, dead pid, or past the ceiling +/// — matching `readLiveUpdateMarker` in the Electron gate. Never panics. /// -/// Self-PID is treated as non-ownership on purpose (#74761): since #50238 the -/// desktop pre-writes this marker with the spawned updater's pid before the -/// updater reaches `acquire`. Without the exclusion, `acquire` sees a live -/// owner that is itself and aborts ("Another Hermes update is already -/// running"), then the desktop relaunches and retries forever. A foreign live -/// pid (e.g. a dashboard-spawned `hermes update`) still blocks. +/// Self-PID is returned so `acquire` can adopt the desktop's pre-written claim +/// without refreshing its acquisition time (#74761). A foreign live pid (e.g. +/// a dashboard-spawned `hermes update`) still blocks. fn live_marker_owner(path: &Path) -> Option { let raw = std::fs::read_to_string(path).ok()?; let mut lines = raw.lines(); @@ -157,11 +153,6 @@ fn live_marker_owner(path: &Path) -> Option { if age_secs > UPDATE_MARKER_MAX_AGE_SECS || !pid_is_alive(pid) { return None; } - // Desktop `writeUpdateMarker(hermesHome, child.pid)` races ahead of us; - // adopt that pre-claim rather than refusing our own marker. - if pid == std::process::id() { - return None; - } Some(MarkerOwner { pid, age_secs }) } @@ -207,10 +198,16 @@ impl UpdateMarkerGuard { /// 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 { + let pid = std::process::id(); if let Some(owner) = live_marker_owner(&path) { + if owner.pid == pid { + // The desktop races ahead and pre-writes our pid. Adopt that + // claim verbatim: rewriting started_at here lets retries reset + // a wedged updater's age before the stale ceiling can clear it. + return Ok(Self { path, owned: true }); + } return Err(owner); } - let pid = std::process::id(); let started_at = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .map(|d| d.as_secs()) @@ -1321,8 +1318,8 @@ mod tests { /// Spawn a short-lived sibling process whose pid stands in for a foreign /// updater. Same-process double-acquire no longer models contention: since - /// #74761 `live_marker_owner` treats our own pid as adoptable (desktop - /// pre-writes it), so a second acquire in *this* process would succeed. + /// #74761 `acquire` treats our own pid as adoptable (desktop pre-writes it), + /// so a second acquire in *this* process would succeed. fn spawn_foreign_holder() -> std::process::Child { #[cfg(windows)] { @@ -1379,7 +1376,8 @@ mod tests { fn acquire_adopts_a_marker_prewritten_with_our_own_pid() { // #74761: desktop writeUpdateMarker(hermesHome, child.pid) races ahead // of UpdateMarkerGuard::acquire. The marker names US; refusing it made - // every in-app desktop update loop forever. Adopt and rewrite. + // every in-app desktop update loop forever. Adopt it without resetting + // the holder age, so a wedged updater still reaches the stale ceiling. let dir = unique_tmp_dir("marker-own-pid"); std::fs::create_dir_all(&dir).unwrap(); let marker = dir.join(".hermes-update-in-progress"); @@ -1402,7 +1400,12 @@ mod tests { assert_eq!( body.lines().next().unwrap().trim().parse::().unwrap(), std::process::id(), - "acquire rewrites the marker with our pid + fresh started_at" + "acquire keeps the adopted marker owner" + ); + assert_eq!( + body.lines().nth(1).unwrap().trim().parse::().unwrap(), + started_at, + "adopting an own-pid marker must preserve its original holder age" ); drop(guard); assert!( diff --git a/apps/desktop/electron/update-marker.test.ts b/apps/desktop/electron/update-marker.test.ts index 0fb1142b4c..216497d554 100644 --- a/apps/desktop/electron/update-marker.test.ts +++ b/apps/desktop/electron/update-marker.test.ts @@ -115,6 +115,19 @@ test('writeUpdateMarker writes a marker that readLiveUpdateMarker accepts', () = assert.ok(fs.existsSync(markerPath(home)), 'marker file should exist after write') }) +test('writeUpdateMarker preserves a live holder age across pid hand-off', () => { + const home = tmpHome('write-handoff-age') + const now = 1_000_000_000_000 + const startedAt = Math.floor(now / 1000) - 300 + + writeMarker(home, 1010, startedAt) + writeUpdateMarker(home, 2020, { kill: ALIVE, now: () => now }) + + const [pidLine, startedLine] = fs.readFileSync(markerPath(home), 'utf8').split('\n') + assert.equal(Number.parseInt(pidLine, 10), 2020, 'the hand-off records the new owner') + assert.equal(Number.parseInt(startedLine, 10), startedAt, 'the holder age must not restart during hand-off') +}) + test('writeUpdateMarker is best-effort (no throw on bad path)', () => { // A non-existent directory should not throw. const badHome = path.join(os.tmpdir(), 'hermes-marker-nonexistent-' + Date.now()) diff --git a/apps/desktop/electron/update-marker.ts b/apps/desktop/electron/update-marker.ts index ee50b52e21..79fabbbe65 100644 --- a/apps/desktop/electron/update-marker.ts +++ b/apps/desktop/electron/update-marker.ts @@ -119,15 +119,30 @@ export function readLiveUpdateMarker( * * Fix: the desktop writes the marker itself, using the spawned updater's * PID, immediately after `spawn()`. The updater's `UpdateMarkerGuard` will - * later overwrite it with its own PID — that's fine, the marker body is - * the same format and `readLiveUpdateMarker` only cares that *some* live - * pid owns it. When the updater finishes it deletes the marker as before. + * later adopt it or another hand-off stage may replace the PID. A live + * holder's original timestamp is preserved across those transfers so retries + * cannot keep resetting the 20-minute stale ceiling. When the updater finishes + * it deletes the marker as before. * If the updater never starts (spawn failure) the marker still contains a * real PID, so `readLiveUpdateMarker` will self-heal once that PID exits. */ -export function writeUpdateMarker(hermesHome, pid, { now = Date.now } = {}) { +export function writeUpdateMarker( + hermesHome, + pid, + { + kill, + now = Date.now, + maxAgeMs = UPDATE_MARKER_MAX_AGE_MS + }: { + now?: () => number + maxAgeMs?: number + kill?: typeof process.kill + } = {} +) { const file = markerPath(hermesHome) - const startedAt = Math.floor(now() / 1000) + const nowMs = now() + const owner = readLiveUpdateMarker(hermesHome, { kill, maxAgeMs, now: () => nowMs }) + const startedAt = owner ? Math.floor((nowMs - owner.ageMs) / 1000) : Math.floor(nowMs / 1000) try { fs.writeFileSync(file, `${pid}\n${startedAt}\n`, 'utf8')