Repository navigation
fix(desktop): generic venv lock handling before update handoff on Windows #75477
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
156a104
f128e8d
8874baa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2652,6 +2652,63 @@ 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 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 | ||
| } | ||
| const scriptsDir = path.join(updateRoot, 'venv', 'Scripts').replace(/'/g, "''") | ||
| try { | ||
| const ps = | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This force-kills every venv-resident process and its tree, including user-managed terminals, gateways, or services. Current handoff only kills Desktop-owned backend/pool PIDs and reports external holders; please retain that ownership boundary or track a specific Desktop-owned daemon. |
||
| `$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', | ||
| ['-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 +2828,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 } | ||
| } | ||
|
|
@@ -2830,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() | ||
|
|
||
|
|
@@ -2999,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 | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| * `<venv>\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) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not scope this with
-like: it treats legal[/]path characters as wildcard syntax, and this literal emits two backslashes before*. Use an ordinal, case-insensitiveStartsWithcheck against the normalized Scripts prefix, as required bytests/test_install_unmerged_index.py:171-180.