From d60a2eb3b40be9b41ae0a2e39a4f9b5036121fe2 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Tue, 28 Jul 2026 23:21:19 -0700 Subject: [PATCH] fix(desktop): gate backend respawns on updateInFlight, not just the update marker (#73822) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Windows, applyUpdates kills its own backend (releaseBackendLock) BEFORE the venv-blocker preflight but only writes the on-disk update marker AFTER the scan. Killing the backend drops the renderer's WebSocket; the renderer reconnects within ~1s and the marker-only waitForUpdateToFinish gate happily spawns a fresh 'hermes serve' inside the update's own critical section. scanVenvBlockers then finds that brand-new process and aborts with 'another Hermes process is using this installation' — a different PID on every attempt, so Desktop self-update can never succeed. Fix: extract the gate into update-gate.ts (pure, DI-testable) and make it consult BOTH signals — the on-disk marker AND the in-process updateInFlight flag. The success path writes the marker before the flag clears in applyUpdates' finally, so there is no instant where both are false and a waiter can slip through. Also gate spawnPoolBackend, which previously had no waitForLocalStart at all — a background profile window could respawn a pool backend during the same window with the identical abort. Tests: update-gate.test.ts covers the open gate, the flag-only window (the #73822 shape), the flag→marker handoff with no gap, and timeout. --- apps/desktop/electron/main.ts | 74 ++++++++--- apps/desktop/electron/update-gate.test.ts | 144 ++++++++++++++++++++++ apps/desktop/electron/update-gate.ts | 95 ++++++++++++++ 3 files changed, 297 insertions(+), 16 deletions(-) create mode 100644 apps/desktop/electron/update-gate.test.ts create mode 100644 apps/desktop/electron/update-gate.ts diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index 433320f370e5..50a6f040d25b 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -179,6 +179,7 @@ import { } from './ssh-connection' import { nativeOverlayWidth as computeNativeOverlayWidth, macTitleBarOverlayHeight } from './titlebar-overlay-width' import { resolveBehindCount, shouldCountCommits } from './update-count' +import { waitForUpdateClearance } from './update-gate' import { readLiveUpdateMarker, writeUpdateMarker } from './update-marker' import { runRebuildWithRetry } from './update-rebuild' import { @@ -1730,30 +1731,48 @@ const UPDATE_WAIT_POLL_MS = 1000 // updater's own progress window appears. (#50419) const UPDATE_HANDOFF_DWELL_MS = 2500 +// Gate deps shared by the primary-window boot path and the pool-backend +// spawn path. Consulting BOTH the on-disk marker and the in-process +// updateInFlight flag is load-bearing (#73822): applyUpdates kills its own +// backend BEFORE the Windows venv-blocker scan but only writes the marker +// AFTER it, so a marker-only gate lets the renderer's ~1s reconnect respawn +// a backend inside the update's own critical section — which the scan then +// reports as a blocker, aborting every update attempt. +function updateGateDeps() { + return { + hasLiveMarker: () => Boolean(readLiveUpdateMarker(HERMES_HOME)), + isUpdateInFlight: () => updateInFlight + } +} + // Block until no live update is in progress (or we hit the wait timeout). // Emits a boot-progress phase so the renderer shows "Update in progress…" // rather than a frozen splash. Returns true if it parked at all. async function waitForUpdateToFinish() { - let marker = readLiveUpdateMarker(HERMES_HOME) + let announced = false - if (!marker) { + const outcome = await waitForUpdateClearance(updateGateDeps(), { + onWaitTick: async reason => { + if (!announced) { + announced = true + rememberLog(`[updates] update in progress (${reason}); deferring backend start until it finishes`) + } + + await advanceBootProgress( + 'backend.update-wait', + 'An update is finishing — Hermes will start automatically when it completes…', + 12 + ) + }, + pollMs: UPDATE_WAIT_POLL_MS, + timeoutMs: UPDATE_WAIT_TIMEOUT_MS + }) + + if (outcome === 'clear') { return false } - rememberLog(`[updates] update in progress (pid=${marker.pid}); deferring backend start until it finishes`) - const deadline = Date.now() + UPDATE_WAIT_TIMEOUT_MS - - while (marker && Date.now() < deadline) { - await advanceBootProgress( - 'backend.update-wait', - 'An update is finishing — Hermes will start automatically when it completes…', - 12 - ) - await new Promise(r => setTimeout(r, UPDATE_WAIT_POLL_MS)) - marker = readLiveUpdateMarker(HERMES_HOME) - } - - if (marker) { + if (outcome === 'timeout') { rememberLog('[updates] update still in progress after wait timeout; starting backend anyway') } else { rememberLog('[updates] update finished; proceeding with backend start') @@ -8007,6 +8026,29 @@ async function spawnPoolBackend(profile, entry) { } const token = crypto.randomBytes(32).toString('base64url') + + // Same update mutual exclusion as the primary window's waitForLocalStart + // (#73822): pool backends spawn from the same venv, so an ungated respawn + // during applyUpdates' critical section re-locks the venv and trips the + // venv-blocker preflight. No boot-progress UI here — pool backends boot + // silently for background profiles — so we only log while parked. + { + let poolAnnounced = false + + await waitForUpdateClearance(updateGateDeps(), { + onWaitTick: reason => { + if (!poolAnnounced) { + poolAnnounced = true + rememberLog( + `[updates] update in progress (${reason}); deferring pool backend start for profile "${profile}"` + ) + } + }, + pollMs: UPDATE_WAIT_POLL_MS, + timeoutMs: UPDATE_WAIT_TIMEOUT_MS + }) + } + // --profile wins over the inherited HERMES_HOME env (see _apply_profile_override // step 3 in hermes_cli/main.py), so the child re-homes to this profile. // --port 0: the OS assigns an ephemeral port; the child announces it on stdout. diff --git a/apps/desktop/electron/update-gate.test.ts b/apps/desktop/electron/update-gate.test.ts new file mode 100644 index 000000000000..4a06bf6bdfa2 --- /dev/null +++ b/apps/desktop/electron/update-gate.test.ts @@ -0,0 +1,144 @@ +/** + * Tests for electron/update-gate.ts — the update mutual-exclusion gate that + * parks local backend spawns while an in-app update is running. + * + * The regression this guards (#73822): applyUpdates kills its own backend + * BEFORE the Windows venv-blocker scan but writes the on-disk marker AFTER + * it. A marker-only gate therefore let the renderer's reconnect spawn a + * fresh backend inside the update's own critical section, which the scan + * reported as a blocker — aborting every Desktop update attempt on Windows. + * The gate must consult the in-process updateInFlight flag as well. + */ + +import assert from 'node:assert/strict' + +import { test } from 'vitest' + +import { updateGateReason, waitForUpdateClearance } from './update-gate' + +function deps(marker: boolean, inFlight: boolean) { + return { + hasLiveMarker: () => marker, + isUpdateInFlight: () => inFlight + } +} + +// --------------------------------------------------------------------------- +// updateGateReason +// --------------------------------------------------------------------------- + +test('gate open when neither marker nor flag is set', () => { + assert.equal(updateGateReason(deps(false, false)), null) +}) + +test('marker alone closes the gate', () => { + assert.equal(updateGateReason(deps(true, false)), 'marker') +}) + +test('updateInFlight alone closes the gate (#73822 — the pre-marker window)', () => { + assert.equal(updateGateReason(deps(false, true)), 'update-in-flight') +}) + +test('marker wins as the reported reason when both are set', () => { + assert.equal(updateGateReason(deps(true, true)), 'marker') +}) + +// --------------------------------------------------------------------------- +// waitForUpdateClearance +// --------------------------------------------------------------------------- + +test('returns clear immediately without sleeping when the gate is open', async () => { + let slept = 0 + + const outcome = await waitForUpdateClearance(deps(false, false), { + pollMs: 10, + sleep: async () => { + slept += 1 + }, + timeoutMs: 1000 + }) + + assert.equal(outcome, 'clear') + assert.equal(slept, 0) +}) + +test('parks on the in-flight flag and finishes when it clears', async () => { + // Simulates the #73822 sequence: the reconnect arrives while updateInFlight + // is true and no marker exists yet; the flag clears (abort path finally) + // and the waiter proceeds. + let inFlight = true + let ticks = 0 + + const outcome = await waitForUpdateClearance( + { hasLiveMarker: () => false, isUpdateInFlight: () => inFlight }, + { + onWaitTick: reason => { + ticks += 1 + assert.equal(reason, 'update-in-flight') + + if (ticks >= 3) { + inFlight = false + } + }, + pollMs: 1, + sleep: async () => {}, + timeoutMs: 10_000 + } + ) + + assert.equal(outcome, 'finished') + assert.equal(ticks, 3) +}) + +test('parks across the flag→marker handoff without a gap', async () => { + // Success path: the marker is written (main.ts:2936) BEFORE applyUpdates' + // finally clears the flag, so a waiter that arrived during the scan stays + // parked through the transition instead of slipping through. + let inFlight = true + let marker = false + let ticks = 0 + const reasons: string[] = [] + + const outcome = await waitForUpdateClearance( + { hasLiveMarker: () => marker, isUpdateInFlight: () => inFlight }, + { + onWaitTick: reason => { + ticks += 1 + reasons.push(reason) + + if (ticks === 2) { + marker = true // updater hand-off: marker written first… + } + + if (ticks === 3) { + inFlight = false // …then the flag clears; marker still holds the gate + } + + if (ticks === 5) { + marker = false // updater finished + } + }, + pollMs: 1, + sleep: async () => {}, + timeoutMs: 10_000 + } + ) + + assert.equal(outcome, 'finished') + assert.deepEqual(reasons, ['update-in-flight', 'update-in-flight', 'marker', 'marker', 'marker']) +}) + +test('returns timeout when the gate never opens', async () => { + let clock = 0 + + const outcome = await waitForUpdateClearance(deps(true, false), { + now: () => clock, + pollMs: 10, + sleep: async ms => { + clock += ms + }, + timeoutMs: 50 + }) + + assert.equal(outcome, 'timeout') +}) diff --git a/apps/desktop/electron/update-gate.ts b/apps/desktop/electron/update-gate.ts new file mode 100644 index 000000000000..e7326af37825 --- /dev/null +++ b/apps/desktop/electron/update-gate.ts @@ -0,0 +1,95 @@ +'use strict' + +/** + * update-gate.ts + * + * Pure, dependency-injected gate that parks local backend spawns while an + * in-app update is running (#73822, #50238). + * + * Two independent signals mean "an update owns the venv right now": + * + * - the on-disk marker (`HERMES_HOME/.hermes-update-in-progress`), written + * by the updater — and by the desktop itself just before hand-off — and + * - the in-process `updateInFlight` flag, true for the whole + * `applyUpdates()` critical section. + * + * The marker alone is NOT enough (#73822): `applyUpdates` kills its own + * backend early (`releaseBackendLock`) but only writes the marker AFTER the + * Windows venv-blocker scan. Killing the backend drops the renderer's + * WebSocket, the renderer reconnects within ~1s, and a marker-only gate + * happily spawns a fresh backend inside the update's own critical section — + * which `scanVenvBlockers` then reports as a blocker, aborting every update + * attempt forever. Consulting the flag closes that window. On the success + * path the marker is written BEFORE the flag clears in `applyUpdates`' + * `finally`, so there is no instant where both signals are false and a + * waiter could slip through mid-update. + */ + +export type UpdateGateReason = 'marker' | 'update-in-flight' | null + +export interface UpdateGateDeps { + /** True when a live on-disk update marker exists (see update-marker.ts). */ + hasLiveMarker: () => boolean + /** True while this process is inside applyUpdates()' critical section. */ + isUpdateInFlight: () => boolean +} + +/** Why the gate is closed right now, or null when it is open. */ +export function updateGateReason(deps: UpdateGateDeps): UpdateGateReason { + if (deps.hasLiveMarker()) { + return 'marker' + } + + if (deps.isUpdateInFlight()) { + return 'update-in-flight' + } + + return null +} + +export type UpdateClearanceOutcome = 'clear' | 'finished' | 'timeout' + +export interface WaitForUpdateClearanceOptions { + timeoutMs: number + pollMs: number + /** Invoked once per poll while parked (boot progress / logging). */ + onWaitTick?: (reason: Exclude) => void | Promise + now?: () => number + sleep?: (ms: number) => Promise +} + +/** + * Park until no update signal remains, or the deadline passes. + * + * Returns 'clear' when the gate was already open (no wait happened), + * 'finished' when it opened during the wait, and 'timeout' when the deadline + * expired with the gate still closed (callers proceed anyway — matching the + * long-standing marker-gate behavior, since a wedged updater must not brick + * the app forever). + */ +export async function waitForUpdateClearance( + deps: UpdateGateDeps, + options: WaitForUpdateClearanceOptions +): Promise { + const now = options.now || Date.now + const sleep = options.sleep || (ms => new Promise(r => setTimeout(r, ms))) + + let reason = updateGateReason(deps) + + if (!reason) { + return 'clear' + } + + const deadline = now() + options.timeoutMs + + while (reason && now() < deadline) { + if (options.onWaitTick) { + await options.onWaitTick(reason) + } + + await sleep(options.pollMs) + reason = updateGateReason(deps) + } + + return reason ? 'timeout' : 'finished' +}