From 156a10485e71bf44dd021d27730f9122f7e1966a Mon Sep 17 00:00:00 2001 From: xy952666680 Date: Fri, 31 Jul 2026 22:55:06 +0800 Subject: [PATCH 1/3] fix(desktop): generic venv lock handling before update handoff on Windows releaseBackendLock probed only venv/Scripts/hermes.exe for the pre-update lock check, so any other process mapping venv files - e.g. the hindsight memory daemon running off venv/Scripts/pythonw.exe (a console-subsystem trampoline in uv-created venvs) - was invisible to the probe. The updater then raced a still-locked pythonw.exe, hit Access Denied mid venv sync, and stranded a half-updated install. Make the gate generic instead of a hardcoded shim list: - isVenvLocked(): probe-lock EVERY venv/Scripts/*.exe (enumerate the dir) - killVenvShimHolders(): before waiting, kill every process whose ExecutablePath is under venv/Scripts (PowerShell Get-CimInstance) - covers the hindsight daemon, stray CLIs, and any future holder. Verified: update-gate/updater-process vitest 10/10, tsc --noEmit clean; the 17 failing suite tests are pre-existing (proven via git stash baseline). --- apps/desktop/electron/main.ts | 58 +++++++++++++++++++++++++++++++++-- 1 file changed, 55 insertions(+), 3 deletions(-) diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index 41686883f71d..68b64015a07e 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -2652,6 +2652,55 @@ function venvHermesShimPath(updateRoot) { : path.join(updateRoot, 'venv', 'bin', 'hermes') } +// Every entry-point exe under venv\Scripts (generic — not a hardcoded list). +function venvScriptsExePaths(updateRoot) { + const scriptsDir = path.join(updateRoot, 'venv', 'Scripts') + try { + return fs + .readdirSync(scriptsDir) + .filter((f) => f.toLowerCase().endsWith('.exe')) + .map((f) => path.join(scriptsDir, f)) + } catch { + return [] + } +} + +// Generic lock probe across EVERY entry-point exe under venv\Scripts, not a +// hardcoded shim list: any mapped exe (hindsight daemon's pythonw trampoline, +// a stray CLI, a future helper) blocks the update the same way. +function isVenvLocked(updateRoot) { + if (!IS_WINDOWS) { + return false + } + for (const exe of venvScriptsExePaths(updateRoot)) { + if (isShimLocked(exe)) { + return true + } + } + return false +} + +// Kill every process whose exe lives under venv\Scripts (hindsight daemon, +// leftover CLIs, anything else) so the updater never races a mapped shim. +function killVenvShimHolders(updateRoot) { + if (!IS_WINDOWS) { + return + } + const scriptsDir = path.join(updateRoot, 'venv', 'Scripts').replace(/'/g, "''") + try { + const ps = + `$p = Get-CimInstance Win32_Process | Where-Object { $_.ExecutablePath -like '${scriptsDir}\\*' }; ` + + `foreach ($x in $p) { taskkill /PID $($x.ProcessId) /T /F 2>$null | Out-Null }` + execFileSync( + 'powershell', + ['-NoProfile', '-Command', ps], + hiddenWindowsChildOptions({ stdio: 'ignore' }) + ) + } catch { + // best-effort: the lock probe below is the real gate + } +} + // Best-effort lock probe mirroring the Rust updater's is_locked(): a running // .exe on Windows refuses an O_RDWR open with a sharing violation. On POSIX // this practically always succeeds (no mandatory locking), so it returns false @@ -2771,12 +2820,15 @@ async function releaseBackendLock(updateRoot, tag) { forceKillProcessTree(pid) } - const shim = venvHermesShimPath(updateRoot) + // Kill every venv\Scripts holder (hindsight daemon, stray CLIs, anything + // else) so the updater never races a mapped pythonw/python/hermes shim. + killVenvShimHolders(updateRoot) + const deadlineMs = Date.now() + 15000 while (Date.now() < deadlineMs) { - if (!isShimLocked(shim)) { - rememberLog(`[${tag}] venv shim unlocked; safe to proceed`) + if (!isVenvLocked(updateRoot)) { + rememberLog(`[${tag}] venv shims unlocked; safe to proceed`) return { unlocked: true } } From f128e8d71166f734ca6c86282d0d249b175fe8ea Mon Sep 17 00:00:00 2001 From: xy952666680 Date: Fri, 31 Jul 2026 23:13:44 +0800 Subject: [PATCH 2/3] =?UTF-8?q?fix(desktop):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20literal=20path=20prefix=20+=20narrow=20kill=20scope?= =?UTF-8?q?=20+=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback (hermes-sweeper on #75477): 1. PowerShell -like treats [ ] * as wildcards and \* as an escape, so the generated pattern was not a literal prefix check. Replace with an ordinal case-insensitive StartsWith (same requirement as tests/test_install_unmerged_index.py:171-180). 2. Force-killing EVERY venv process overstepped the existing contract, which kills only Desktop-owned backends and reports/aborts on external holders. Narrow killVenvShimHolders to Hermes-OWNED daemons only (exe under venv\Scripts AND cmdline matching hindsight_api.main) — external holders still go through scanVenvBlockers report+abort. 3. Add focused tests for the selection logic: pure, Electron-free module (venv-holder-select.ts) with 6 tests covering path-prefix semantics, case-insensitivity, hindsight-only matching, and external-holder exclusion. Verified: venv-holder-select 6/6, update-gate + updater-process 10/10, tsc --noEmit clean, dist node --check OK. --- apps/desktop/electron/main.ts | 14 ++++- .../electron/venv-holder-select.test.ts | 63 +++++++++++++++++++ apps/desktop/electron/venv-holder-select.ts | 40 ++++++++++++ 3 files changed, 114 insertions(+), 3 deletions(-) create mode 100644 apps/desktop/electron/venv-holder-select.test.ts create mode 100644 apps/desktop/electron/venv-holder-select.ts diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index 68b64015a07e..4819e85cff4c 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -2680,8 +2680,13 @@ function isVenvLocked(updateRoot) { return false } -// Kill every process whose exe lives under venv\Scripts (hindsight daemon, -// leftover CLIs, anything else) so the updater never races a mapped shim. +// Kill only Hermes-OWNED venv daemons (the memory plugin's hindsight daemon: +// exe under venv\Scripts AND cmdline containing hindsight_api.main). External +// holders (a user terminal running `hermes`, unrelated scripts) are NOT killed +// — scanVenvBlockers reports them and the handoff aborts, per existing design. +// Path match is an ordinal case-insensitive prefix (PowerShell -like would +// treat `[`/`]`/`*` as wildcards and `\*` as an escape — a literal check is +// required, cf. tests/test_install_unmerged_index.py:171-180). function killVenvShimHolders(updateRoot) { if (!IS_WINDOWS) { return @@ -2689,7 +2694,10 @@ function killVenvShimHolders(updateRoot) { const scriptsDir = path.join(updateRoot, 'venv', 'Scripts').replace(/'/g, "''") try { const ps = - `$p = Get-CimInstance Win32_Process | Where-Object { $_.ExecutablePath -like '${scriptsDir}\\*' }; ` + + `$p = Get-CimInstance Win32_Process | Where-Object { ` + + `$_.ExecutablePath -and $_.CommandLine -and ` + + `$_.ExecutablePath.StartsWith('${scriptsDir}\\', [System.StringComparison]::OrdinalIgnoreCase) ` + + `-and $_.CommandLine -match 'hindsight_api\\.main' }; ` + `foreach ($x in $p) { taskkill /PID $($x.ProcessId) /T /F 2>$null | Out-Null }` execFileSync( 'powershell', diff --git a/apps/desktop/electron/venv-holder-select.test.ts b/apps/desktop/electron/venv-holder-select.test.ts new file mode 100644 index 000000000000..9067159f6e1f --- /dev/null +++ b/apps/desktop/electron/venv-holder-select.test.ts @@ -0,0 +1,63 @@ +import assert from 'node:assert/strict' + +import { test } from 'vitest' + +import { hasWindowsPathPrefix, isHermesOwnedVenvDaemon } from './venv-holder-select' + +const SCRIPTS = 'C:\\Hermes\\venv\\Scripts' + +test('matches the hindsight daemon shim (exe under venv Scripts + hindsight cmdline)', () => { + assert.equal( + isHermesOwnedVenvDaemon( + 'C:\\Hermes\\venv\\Scripts\\pythonw.exe', + 'C:\\Hermes\\venv\\Scripts\\pythonw.exe -m hindsight_api.main --daemon --idle-timeout 300 --port 9177', + SCRIPTS + ), + true + ) +}) + +test('Windows path prefix match is ordinal case-insensitive', () => { + assert.equal( + isHermesOwnedVenvDaemon( + 'c:\\hermes\\venv\\scripts\\python.exe', + 'python.exe -m hindsight_api.main --daemon', + 'C:\\Hermes\\venv\\Scripts' + ), + true + ) +}) + +test('excludes external venv holders that are not the hindsight daemon', () => { + // a user terminal running the hermes CLI from the venv — must NOT be killed + assert.equal( + isHermesOwnedVenvDaemon('C:\\Hermes\\venv\\Scripts\\hermes.exe', 'hermes chat -q "hi"', SCRIPTS), + false + ) + // an unrelated python script using the venv interpreter + assert.equal( + isHermesOwnedVenvDaemon('C:\\Hermes\\venv\\Scripts\\python.exe', 'python C:\\tools\\import.py', SCRIPTS), + false + ) +}) + +test('excludes exes outside the venv even when the cmdline mentions hindsight', () => { + assert.equal( + isHermesOwnedVenvDaemon('C:\\Other\\pythonw.exe', 'pythonw -m hindsight_api.main --daemon', SCRIPTS), + false + ) +}) + +test('prefix boundary: sibling dirs (ScriptsX) do not match', () => { + assert.equal( + hasWindowsPathPrefix('C:\\Hermes\\venv\\ScriptsX\\python.exe', SCRIPTS), + false + ) + assert.equal(hasWindowsPathPrefix('C:\\Hermes\\venv\\Scripts\\python.exe', SCRIPTS), true) +}) + +test('null/undefined fields never match', () => { + assert.equal(isHermesOwnedVenvDaemon(null, 'x', SCRIPTS), false) + assert.equal(isHermesOwnedVenvDaemon('C:\\Hermes\\venv\\Scripts\\pythonw.exe', null, SCRIPTS), false) + assert.equal(isHermesOwnedVenvDaemon(undefined, undefined, SCRIPTS), false) +}) diff --git a/apps/desktop/electron/venv-holder-select.ts b/apps/desktop/electron/venv-holder-select.ts new file mode 100644 index 000000000000..141a9e1ed88c --- /dev/null +++ b/apps/desktop/electron/venv-holder-select.ts @@ -0,0 +1,40 @@ +/** + * venv-holder-select.ts + * + * Pure Windows venv-holder selection logic (testable without Electron). + * + * The pre-update handoff kills Hermes-OWNED venv daemons (the memory plugin's + * hindsight daemon) so the updater never races a mapped shim. External + * holders (a user terminal running `hermes`, unrelated scripts) must NOT be + * killed — current design reports them via scanVenvBlockers and ABORTS the + * handoff instead (main.ts releaseBackendLock / applyUpdates). + */ + +/** Ordinal case-insensitive prefix check for Windows paths. */ +export function hasWindowsPathPrefix( + exePath: string, + venvScriptsDir: string +): boolean { + const prefix = `${venvScriptsDir}\\` + return ( + exePath.length >= prefix.length && + exePath.slice(0, prefix.length).toLowerCase() === prefix.toLowerCase() + ) +} + +/** + * True when a process is a Hermes-owned venv daemon: its exe lives under + * `\Scripts\` (ordinal case-insensitive prefix) AND its cmdline + * references `hindsight_api.main` (the memory daemon the memory plugin + * spawns DETACHED — it outlives Hermes and holds venv shims mapped). + */ +export function isHermesOwnedVenvDaemon( + exePath: string | null | undefined, + cmdline: string | null | undefined, + venvScriptsDir: string +): boolean { + if (!exePath || !cmdline) { + return false + } + return hasWindowsPathPrefix(exePath, venvScriptsDir) && /hindsight_api\.main/i.test(cmdline) +} From 8874baa925eec344cba12bbe54e5806a5b6c1bfa Mon Sep 17 00:00:00 2001 From: xy952666680 Date: Sat, 1 Aug 2026 00:36:05 +0800 Subject: [PATCH 3/3] fix(desktop): hold updateInFlight through handoff + gate on cross-process marker Two defects made every GUI update spawn multiple hermes-setup.exe processes that then rejected each other via the update-in-progress marker ("Another Hermes update is already running"): 1. applyUpdates cleared updateInFlight in a finally block while app.quit() was still ~2.5s away (UPDATE_HANDOFF_DWELL_MS), reopening a window where a second click/renderer retry spawned a second updater. Only reset the flag on failure; on success it stays set until the process exits. 2. applyUpdates only checked the in-process boolean, never the cross-process .hermes-update-in-progress marker that the Rust updater and hermes_cli/update_lock.py share. Gate on readLiveUpdateMarker(HERMES_HOME) so a foreign live update (manual hermes-setup.exe run, second window, dashboard) is refused instead of racing it. --- apps/desktop/electron/main.ts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index 4819e85cff4c..2ac6b093ef39 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -2890,12 +2890,14 @@ async function releaseBackendLock(updateRoot, tag) { // Detection (checkUpdates / commit changelog / "N behind") stays in the UI; // only this apply action changed. async function applyUpdates(opts = {}) { - if (updateInFlight) { + if (updateInFlight || readLiveUpdateMarker(HERMES_HOME)) { throw new Error('An update is already in progress.') } updateInFlight = true + let handedOff = false + try { const updater = resolveUpdaterBinary() @@ -3059,9 +3061,19 @@ async function applyUpdates(opts = {}) { app.quit() }, UPDATE_HANDOFF_DWELL_MS) + handedOff = true + return { ok: true, handedOff: true, updater } } finally { - updateInFlight = false + // Only reset the in-flight flag on failure. On success the process is + // about to quit (UPDATE_HANDOFF_DWELL_MS); clearing the flag here would + // reopen a ~2.5s window where a second click / renderer retry spawns a + // second updater that then trips the cross-process marker and aborts + // ("Another Hermes update is already running") — every GUI update then + // fails. The flag stays set until the process exits. + if (!handedOff) { + updateInFlight = false + } } }