fix(desktop): first Windows update attempt no longer fails on dying backend processes (#74805)

taskkill /T /F returns when termination is INITIATED, not completed, and the pre-handoff unlock gate only probed the venv hermes.exe shim — which the 'python.exe -m hermes_cli.main serve' backend need not hold at all. The gate could therefore pass on its first iteration with zero dwell while the killed pythons were still unmapping .pyd files; the venv-blocker scan (no liveness filter) then reported those dying processes as holders and aborted the hand-off. Every first update attempt from the footbar failed; the manual retry succeeded because the process table had settled by then. The unlock gate now lives in backend-release-gate.ts (dependency-free, backend-child.ts pattern) and requires BOTH the shim unlocked AND every signalled PID to have actually left the process table; stragglers collected per-pass are killed and join the watch set. On deadline the old shim-only criterion survives as the escape hatch — lingering PIDs past 15s are the venv-blocker re-scan's job. applyUpdates additionally re-scans up to 2x with a 1.5s settle before aborting on 'blocked', so untracked grandchildren an AV driver holds in teardown stop failing the update while a REAL holder still aborts on the third scan. Surgical reapply of PR #78037 fix 1 by @3x3xX3N0N onto the post-#87599 code shape (stopBackendTreesForUpdate extraction, stopSafeBlockers re-scan path). The re-scan settle idea was first submitted by @MaheshBhushan (#74831); the killed-PID tracking seam matches @webtecnica's #74956. Co-authored-by: MaheshBhushan <128616744+MaheshBhushan@users.noreply.github.com> Co-authored-by: webtecnica <75556242+webtecnica@users.noreply.github.com> Co-authored-by: Hermes <hermes@nousresearch.com>
2026-08-25 23:21:06 -07:00
parent 9dbb8868e8
commit b04f8578eb
4 changed files with 514 additions and 25 deletions
@@ -0,0 +1,174 @@
+/**
+ * backend-release-gate.test.ts
+ *
+ * The #74805 first-attempt race, pinned as a contract on the extracted gate:
+ * the desktop must not hand off to the updater while PIDs it signalled are
+ * still in the process table, even when the venv shim probe reads unlocked
+ * (the backend `python.exe -m hermes_cli.main serve` need not hold the shim
+ * at all). On merge-base main.ts the gate was shim-only and passed on its
+ * first iteration with zero dwell — the sabotage A/B run proves these tests
+ * bite on that behavior.
+ */
+
+import { describe, expect, it } from 'vitest'
+
+import {
+  RELEASE_GATE_POLL_MS,
+  type ReleaseGateDeps,
+  waitForBackendRelease
+} from './backend-release-gate'
+
+/** A fake clock where sleep() advances time instantly. */
+function fakeClock() {
+  let t = 0
+
+  return {
+    now: () => t,
+    sleep: async (ms: number) => {
+      t += ms
+    },
+    advance: (ms: number) => {
+      t += ms
+    }
+  }
+}
+
+function makeDeps(overrides: Partial<ReleaseGateDeps> = {}): ReleaseGateDeps & {
+  logs: string[]
+  kills: number[]
+} {
+  const clock = fakeClock()
+  const logs: string[] = []
+  const kills: number[] = []
+
+  return {
+    isShimLocked: () => false,
+    isPidAlive: () => false,
+    collectStragglerPids: () => [],
+    killProcessTree: pid => kills.push(pid),
+    sleep: clock.sleep,
+    now: clock.now,
+    log: line => logs.push(line),
+    logs,
+    kills,
+    ...overrides
+  }
+}
+
+describe('waitForBackendRelease (#74805 first-attempt race)', () => {
+  it('does NOT pass while a signalled PID is still in the process table, even with the shim unlocked', async () => {
+    // The exact #74805 shape: shim unlocked from tick 0 (serve backend never
+    // held it), but the killed python is still tearing down for ~1.2s.
+    let aliveUntil = 4 * RELEASE_GATE_POLL_MS
+    const clock = fakeClock()
+
+    const deps = makeDeps({
+      now: clock.now,
+      sleep: clock.sleep,
+      isShimLocked: () => false,
+      isPidAlive: () => clock.now() < aliveUntil
+    })
+
+    const result = await waitForBackendRelease([4021], deps, 'test')
+
+    expect(result.unlocked).toBe(true)
+    expect(result.lingeringPids).toEqual([])
+    // The gate must have dwelled at least until the PID actually exited —
+    // on merge-base (shim-only gate) it would have returned at t=0.
+    expect(clock.now()).toBeGreaterThanOrEqual(aliveUntil)
+  })
+
+  it('passes immediately when the shim is unlocked and no signalled PID lingers', async () => {
+    const deps = makeDeps()
+
+    const result = await waitForBackendRelease([4021, 4022], deps, 'test')
+
+    expect(result.unlocked).toBe(true)
+    expect(deps.now()).toBe(0) // no dwell needed — everything already gone
+  })
+
+  it('keeps waiting while the shim is locked and fails closed at the deadline', async () => {
+    const deps = makeDeps({ isShimLocked: () => true })
+
+    const result = await waitForBackendRelease([], deps, 'test', 3 * RELEASE_GATE_POLL_MS)
+
+    expect(result.unlocked).toBe(false)
+  })
+
+  it('proceeds at the deadline when the shim is unlocked but PIDs still linger (pre-#74805 escape hatch)', async () => {
+    // Lingering PIDs past the deadline are the venv-blocker re-scan's job —
+    // the gate must not invent a new failure mode for them.
+    const deps = makeDeps({ isPidAlive: () => true })
+
+    const result = await waitForBackendRelease([4021], deps, 'test', 3 * RELEASE_GATE_POLL_MS)
+
+    expect(result.unlocked).toBe(true)
+    expect(result.lingeringPids).toEqual([4021])
+  })
+
+  it('kills and then waits out stragglers that respawn mid-teardown', async () => {
+    // A pool entry registered mid-teardown appears on pass 2; the gate must
+    // signal it AND add it to the exit-wait set.
+    const clock = fakeClock()
+    let stragglerServed = false
+    let stragglerKilledAt: number | null = null
+    const kills: number[] = []
+
+    const deps = makeDeps({
+      now: clock.now,
+      sleep: clock.sleep,
+      collectStragglerPids: () => {
+        if (!stragglerServed) {
+          stragglerServed = true
+
+          return [7777]
+        }
+
+        return []
+      },
+      killProcessTree: pid => {
+        stragglerKilledAt = clock.now()
+        kills.push(pid)
+      },
+      // Primary PID 4021 lingers for one poll (forcing a straggler-collect
+      // pass); the straggler stays alive for two polls after being killed.
+      isPidAlive: pid => {
+        if (pid === 4021) {
+          return clock.now() < RELEASE_GATE_POLL_MS
+        }
+
+        return (
+          pid === 7777 &&
+          stragglerKilledAt !== null &&
+          clock.now() < stragglerKilledAt + 2 * RELEASE_GATE_POLL_MS
+        )
+      }
+    })
+
+    const result = await waitForBackendRelease([4021], deps, 'test')
+
+    expect(kills).toContain(7777)
+    expect(result.unlocked).toBe(true)
+    expect(result.lingeringPids).toEqual([])
+    // The gate must have dwelled until the straggler actually exited.
+    expect(clock.now()).toBeGreaterThanOrEqual(
+      (stragglerKilledAt ?? 0) + 2 * RELEASE_GATE_POLL_MS
+    )
+  })
+
+  it('ignores invalid PIDs in the seed and straggler sets', async () => {
+    const deps = makeDeps({
+      collectStragglerPids: () => [0, -4, NaN as unknown as number]
+    })
+
+    const result = await waitForBackendRelease(
+      [0, -1, 2.5, NaN as unknown as number],
+      deps,
+      'test',
+      2 * RELEASE_GATE_POLL_MS
+    )
+
+    expect(result.unlocked).toBe(true)
+    expect(deps.kills).toEqual([])
+  })
+})
@@ -0,0 +1,131 @@
+/**
+ * backend-release-gate.ts
+ *
+ * The Windows pre-update unlock gate: after the desktop tree-kills its own
+ * backends, decide when it is actually safe to hand off to the updater.
+ *
+ * Why this exists (#74805): `taskkill /T /F` returns once termination is
+ * INITIATED, not completed. A dying `python.exe -m hermes_cli.main serve`
+ * stays in the process table while it unmaps .pyd files (AV / NTFS filter
+ * drivers stretch this out), and it need not hold the venv `hermes.exe` shim
+ * at all — so a gate that only probes the shim can pass on its very first
+ * iteration, with zero dwell, while the killed pythons are still
+ * terminating. The venv-blocker scan downstream has no liveness filter; it
+ * enumerates those dying processes as holders and aborts the hand-off.
+ * Result: the FIRST update attempt from the footbar always failed, and the
+ * manual retry (by which time the table had settled) succeeded.
+ *
+ * The gate therefore requires BOTH: the shim unlocked AND every PID we have
+ * ever signalled to have actually left the process table. On deadline, the
+ * old shim-only criterion is kept as the escape hatch — lingering PIDs past
+ * 15s are the venv-blocker re-scan's job, not a new failure mode.
+ *
+ * Extracted into its own dependency-free module (no electron import) so the
+ * gate's decision logic can be asserted directly with fake clocks and fake
+ * process tables, following the backend-child.ts pattern.
+ */
+
+export interface ReleaseGateDeps {
+  /** Probe the venv hermes.exe shim (real: O_RDWR open attempt). */
+  isShimLocked: () => boolean
+  /** True while `pid` is still enumerable in the process table. */
+  isPidAlive: (pid: number) => boolean
+  /**
+   * Re-collect PIDs that may have (re)spawned since the initial sweep —
+   * the supervised primary backend and pool entries. Called every pass.
+   */
+  collectStragglerPids: () => number[]
+  /** Tree-kill (real: taskkill /PID n /T /F). */
+  killProcessTree: (pid: number) => void
+  /** Async sleep; injectable so tests run on a fake clock. */
+  sleep: (ms: number) => Promise<void>
+  /** Monotonic-enough clock; injectable for tests. */
+  now: () => number
+  /** Log sink (real: rememberLog). */
+  log: (line: string) => void
+}
+
+export interface ReleaseGateResult {
+  unlocked: boolean
+  /** PIDs we signalled that were still enumerable when the gate resolved. */
+  lingeringPids: number[]
+}
+
+export const RELEASE_GATE_DEADLINE_MS = 15000
+export const RELEASE_GATE_POLL_MS = 300
+
+/**
+ * Wait until the install is genuinely releasable: shim unlocked AND every
+ * signalled PID gone — or the deadline passes.
+ *
+ * `initialPids` are the PIDs the caller already signalled (primary backend +
+ * pool) before invoking the gate; stragglers collected on each pass are
+ * killed and added to the same watch set.
+ */
+export async function waitForBackendRelease(
+  initialPids: number[],
+  deps: ReleaseGateDeps,
+  tag: string,
+  deadlineMs: number = RELEASE_GATE_DEADLINE_MS
+): Promise<ReleaseGateResult> {
+  const killedPids = new Set<number>(
+    initialPids.filter(pid => Number.isInteger(pid) && pid > 0)
+  )
+
+  const deadline = deps.now() + deadlineMs
+
+  while (deps.now() < deadline) {
+    const lingering = [...killedPids].filter(pid => deps.isPidAlive(pid))
+
+    if (!deps.isShimLocked() && lingering.length === 0) {
+      deps.log(
+        `[${tag}] venv shim unlocked and ${killedPids.size} signalled backend PID(s) exited; safe to proceed`
+      )
+
+      return { unlocked: true, lingeringPids: [] }
+    }
+
+    // A supervised backend can respawn between kill and check (grandchildren,
+    // pool entries registered mid-teardown). Re-collect and re-kill each pass
+    // instead of trusting the initial sweep.
+    for (const pid of deps.collectStragglerPids()) {
+      if (Number.isInteger(pid) && pid > 0) {
+        killedPids.add(pid)
+        deps.killProcessTree(pid)
+      }
+    }
+
+    await deps.sleep(RELEASE_GATE_POLL_MS)
+  }
+
+  // Deadline reached. Keep the pre-#74805 success criterion — an unlocked
+  // shim — rather than inventing a new failure mode for PIDs that linger
+  // past the deadline; the venv-blocker re-scan downstream covers that
+  // residue (and a REAL foreign holder still fails the shim probe).
+  const lingering = [...killedPids].filter(pid => deps.isPidAlive(pid))
+
+  if (!deps.isShimLocked()) {
+    deps.log(
+      `[${tag}] proceeding after deadline: venv shim unlocked, but ${lingering.length} signalled PID(s) still enumerable`
+    )
+
+    return { unlocked: true, lingeringPids: lingering }
+  }
+
+  return { unlocked: false, lingeringPids: lingering }
+}
+
+/**
+ * Liveness probe for a PID on Windows. `process.kill(pid, 0)` delivers
+ * nothing; it only probes existence: EPERM ⇒ exists but inaccessible (still
+ * alive), ESRCH ⇒ gone.
+ */
+export function isPidAliveWindows(pid: number): boolean {
+  try {
+    process.kill(pid, 0)
+
+    return true
+  } catch (err: any) {
+    return Boolean(err) && err.code === 'EPERM'
+  }
+}
@@ -0,0 +1,150 @@
+/**
+ * backend-release-gate.windows-live.test.ts
+ *
+ * LIVE Windows E2E for the #74805 unlock gate: real spawned processes, the
+ * REAL isPidAliveWindows probe against the live process table, real
+ * taskkill — no fake clocks, no fake tables. Runs only on win32 (the
+ * ephemeral wine2e lane); skipped everywhere else.
+ *
+ * This is the platform half of the proof: the unit suite pins the gate's
+ * decision logic on a fake table; this file proves the two real-world
+ * premises the fix rests on:
+ *   1. taskkill /T /F returns while the killed process is still enumerable
+ *      (the race window exists), and
+ *   2. the gate, wired to the real probes, dwells through that window and
+ *      only passes once the PID has genuinely left the table.
+ */
+
+import { execFileSync, spawn } from 'node:child_process'
+
+import { describe, expect, it } from 'vitest'
+
+import { isPidAliveWindows, waitForBackendRelease } from './backend-release-gate'
+
+const isWindows = process.platform === 'win32'
+
+function spawnSleeper(): { pid: number; kill: () => void } {
+  // A real python if available (mirrors the backend shape), else powershell.
+  const child = spawn(
+    'powershell',
+    ['-NoProfile', '-Command', 'Start-Sleep -Seconds 300'],
+    { stdio: 'ignore' }
+  )
+
+  if (!child.pid) {
+    throw new Error('sleeper failed to spawn')
+  }
+
+  return {
+    pid: child.pid,
+    kill: () => {
+      try {
+        child.kill()
+      } catch {
+        /* already gone */
+      }
+    }
+  }
+}
+
+function taskkillTree(pid: number): void {
+  try {
+    execFileSync('taskkill', ['/PID', String(pid), '/T', '/F'], { stdio: 'ignore' })
+  } catch {
+    /* already gone */
+  }
+}
+
+describe.skipIf(!isWindows)('waitForBackendRelease — live Windows (#74805)', () => {
+  it('isPidAliveWindows tracks a real process through spawn and exit', async () => {
+    const sleeper = spawnSleeper()
+
+    expect(isPidAliveWindows(sleeper.pid)).toBe(true)
+
+    taskkillTree(sleeper.pid)
+
+    // Poll until the table retires the PID (bounded).
+    const deadline = Date.now() + 10000
+
+    while (isPidAliveWindows(sleeper.pid) && Date.now() < deadline) {
+      await new Promise(r => setTimeout(r, 100))
+    }
+
+    expect(isPidAliveWindows(sleeper.pid)).toBe(false)
+  })
+
+  it('the gate dwells until a real killed PID leaves the live process table', async () => {
+    const sleeper = spawnSleeper()
+    const logs: string[] = []
+    let firstAliveCheck: boolean | null = null
+
+    // Fire the real taskkill and IMMEDIATELY enter the gate — the #74805
+    // shape. The shim probe reads unlocked throughout (the serve backend
+    // never held it); only the PID exit-wait can hold the gate closed.
+    taskkillTree(sleeper.pid)
+
+    const result = await waitForBackendRelease(
+      [sleeper.pid],
+      {
+        isShimLocked: () => false,
+        isPidAlive: pid => {
+          const alive = isPidAliveWindows(pid)
+
+          if (firstAliveCheck === null) {
+            firstAliveCheck = alive
+          }
+
+          return alive
+        },
+        collectStragglerPids: () => [],
+        killProcessTree: taskkillTree,
+        sleep: ms => new Promise(r => setTimeout(r, ms)),
+        now: () => Date.now(),
+        log: line => logs.push(line)
+      },
+      'live-e2e'
+    )
+
+    expect(result.unlocked).toBe(true)
+    // The gate resolved only after the real PID left the real table:
+    expect(isPidAliveWindows(sleeper.pid)).toBe(false)
+    expect(result.lingeringPids).toEqual([])
+    // Record whether the race window was observable on this runner (taskkill
+    // returned while the PID was still enumerable). Informational: fast
+    // runners can retire tiny process trees before our first check, but the
+    // gate's correctness (above) does not depend on winning that race.
+    logs.push(`race-window-observed=${firstAliveCheck}`)
+
+    expect(logs.some(l => l.includes('safe to proceed'))).toBe(true)
+  })
+
+  it('a live foreign holder keeps the gate closed until the deadline', async () => {
+    const holder = spawnSleeper()
+
+    try {
+      const result = await waitForBackendRelease(
+        [holder.pid],
+        {
+          // Simulates the shim held by a process we did NOT kill — the gate
+          // must fail closed rather than hand off over a live holder.
+          isShimLocked: () => true,
+          isPidAlive: isPidAliveWindows,
+          collectStragglerPids: () => [],
+          killProcessTree: () => {
+            /* nothing else to kill */
+          },
+          sleep: ms => new Promise(r => setTimeout(r, ms)),
+          now: () => Date.now(),
+          log: () => {}
+        },
+        'live-e2e',
+        2000
+      )
+
+      expect(result.unlocked).toBe(false)
+      expect(result.lingeringPids).toEqual([holder.pid])
+    } finally {
+      taskkillTree(holder.pid)
+    }
+  })
+})
@@ -61,6 +61,7 @@ import {
  verifyHermesCli
 } from './backend-probes'
 import { waitForDashboardPortAnnouncement } from './backend-ready'
+import { isPidAliveWindows, waitForBackendRelease } from './backend-release-gate'
 import {
  isHostKeyChangedBootFailure,
  isRetryableRemoteBootFailure,
@@ -3469,43 +3470,59 @@ async function releaseBackendLock(updateRoot, tag) {

  const hermesProcess = backendConnectionState.getProcess()

+  // Seed the release gate with every PID we are about to signal: the
+  // supervised primary backend and all pool backends. The gate waits for
+  // these to actually LEAVE the process table, not just for the shim to
+  // unlock — the shim probe only covers venv\Scripts\hermes.exe, but the
+  // backend is `python.exe -m hermes_cli.main serve`, which need not hold
+  // the shim at all (#74805 first-attempt race).
+  const initialPids = []
+
+  if (hermesProcess && Number.isInteger(hermesProcess.pid)) {
+    initialPids.push(hermesProcess.pid)
+  }
+
+  for (const entry of backendPool.values()) {
+    if (entry.process && Number.isInteger(entry.process.pid)) {
+      initialPids.push(entry.process.pid)
+    }
+  }
+
  stopBackendTreesForUpdate(hermesProcess, {
    forceKillProcessTree,
    stopAllPoolBackends
  })

  const shim = venvHermesShimPath(updateRoot)
-  const deadlineMs = Date.now() + 15000

-  while (Date.now() < deadlineMs) {
-    if (!isShimLocked(shim)) {
-      rememberLog(`[${tag}] venv shim unlocked; safe to proceed`)
+  const gate = await waitForBackendRelease(initialPids, {
+    isShimLocked: () => Boolean(isShimLocked(shim)),
+    isPidAlive: isPidAliveWindows,
+    collectStragglerPids: () => {
+      const stragglers = []

-      return { unlocked: true }
-    }
+      const currentHermesProcess = backendConnectionState.getProcess()

-    // A supervised backend can respawn between kill and check (grandchildren,
-    // pool entries registered mid-teardown). Re-collect and re-kill each pass
-    // instead of trusting the initial sweep.
-    const stragglers = []
-
-    const currentHermesProcess = backendConnectionState.getProcess()
-
-    if (currentHermesProcess && Number.isInteger(currentHermesProcess.pid)) {
-      stragglers.push(currentHermesProcess.pid)
-    }
-
-    for (const entry of backendPool.values()) {
-      if (entry.process && Number.isInteger(entry.process.pid)) {
-        stragglers.push(entry.process.pid)
+      if (currentHermesProcess && Number.isInteger(currentHermesProcess.pid)) {
+        stragglers.push(currentHermesProcess.pid)
      }
-    }

-    for (const pid of stragglers) {
-      forceKillProcessTree(pid)
-    }
+      for (const entry of backendPool.values()) {
+        if (entry.process && Number.isInteger(entry.process.pid)) {
+          stragglers.push(entry.process.pid)
+        }
+      }

-    await new Promise(r => setTimeout(r, 300))
+      return stragglers
+    },
+    killProcessTree: forceKillProcessTree,
+    sleep: (ms: number) => new Promise(r => setTimeout(r, ms)),
+    now: () => Date.now(),
+    log: rememberLog
+  }, tag)
+
+  if (gate.unlocked) {
+    return { unlocked: true }
  }

  // Do NOT proceed past a held lock: handing off to the updater while another
@@ -3682,6 +3699,23 @@ async function applyUpdates(opts: { stopSafeBlockers?: boolean } = {}) {
        scanOutcome = await scanVenvBlockers(updateRoot)
      }

+      // Re-scan before aborting on 'blocked' (#74805). Process-table teardown
+      // is asynchronous on Windows: even after releaseBackendLock's PID-exit
+      // wait, a grandchild the desktop never tracked (or a process an AV /
+      // NTFS filter driver is holding in teardown) can stay enumerable for a
+      // few more seconds and read as a holder. Each scan already costs
+      // seconds (spawns a venv python + psutil sweep), so two retries with a
+      // short dwell give the table time to settle without meaningfully
+      // delaying the abort path when a REAL holder (a user terminal, second
+      // window) is present — that holder is still there on the third scan.
+      for (let attempt = 0; scanOutcome.kind === 'blocked' && attempt < 2; attempt++) {
+        rememberLog(
+          `[updates] venv-blocker scan reported ${scanOutcome.result.processes.length} holder(s); re-scanning after settle (attempt ${attempt + 2}/3)`
+        )
+        await new Promise(resolve => setTimeout(resolve, 1500))
+        scanOutcome = await scanVenvBlockers(updateRoot)
+      }
+
      if (scanOutcome.kind === 'blocked') {
        const message = formatBlockerMessage(scanOutcome.result)