fix(desktop): generic venv lock handling before update handoff on Windows - #75477
xy952666680 wants to merge 3 commits into
Conversation
…dows 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).
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the pythonw.exe lock-holder case from #75478. The current-main premise is real: releaseBackendLock still probes only venv\\Scripts\\hermes.exe at apps/desktop/electron/main.ts:2639-2646,2767-2774.
Problems
apps/desktop/electron/main.ts:2690uses-likewith a venv-derived path. That is not a literal prefix check; legal[/]path characters alter wildcard matching. The repository already requires ordinal case-insensitiveStartsWithfor this exact Windows path-scoping issue intests/test_install_unmerged_index.py:171-180. The generated pattern also contains two literal backslashes before*, rather than one separator.apps/desktop/electron/main.ts:2691force-kills every process running from the venv, including processes Desktop does not own. Current main intentionally kills only tracked Desktop backend/pool PIDs (main.ts:2738-2765) and reports external holders instead (main.ts:2910-2923).
Suggested changes
- Use a literal ordinal case-insensitive prefix comparison and preserve the external-holder abort/report behavior, or narrowly track and stop only a Desktop-owned daemon.
- Add focused Windows-path and process-ownership tests for the new selection logic.
This is an automated hermes-sweeper review.
| return | ||
| } | ||
| const scriptsDir = path.join(updateRoot, 'venv', 'Scripts').replace(/'/g, "''") | ||
| try { |
There was a problem hiding this comment.
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-insensitive StartsWith check against the normalized Scripts prefix, as required by tests/test_install_unmerged_index.py:171-180.
| } | ||
| const scriptsDir = path.join(updateRoot, 'venv', 'Scripts').replace(/'/g, "''") | ||
| try { | ||
| const ps = |
There was a problem hiding this comment.
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.
…e + tests Review feedback (hermes-sweeper on NousResearch#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.
…cess 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.
|
Added one more commit in this same Windows update-flow area: Problem it fixes (observed repeatedly on Windows): every GUI update attempt spawned multiple
Verification: |
|
Triage update against current main (sweeper verdict keep_open/medium still stands):
Not closing. This is queued behind the serve-classification consolidation (#98336/#81774) currently in flight, since both touch the same preflight teardown path; salvage should target only the lock-probe/daemon-kill half. |
|
Wave-3 salvage status: the surviving piece of this PR now rides in #100124 with your authorship preserved (commit authored as xy952666680). Scope per the earlier triage: what survived is the narrowly-scoped hindsight-daemon reap — your pure Dropped halves, with reasons: the generic every-exe This PR will be closed with credit once #100124 merges. Thanks for tracing the pythonw trampoline lock-holder from #75478 — that diagnosis is what made the scoped fix possible. |
…ff (Windows) The pre-handoff teardown tree-kills only the backends the Desktop owns (backendConnectionState + backendPool). The memory plugin's hindsight daemon is spawned DETACHED off venv\Scripts\pythonw.exe, so it survives the teardown, keeps venv files mapped, and either dead-ends the venv-blocker scan with no in-app remedy or (pre-#74805 shim-only gate) raced the updater into a half-updated venv. Add a narrowly-scoped reap: kill only processes whose exe lives under venv\Scripts (ordinal case-insensitive prefix — no PowerShell -like wildcard hazards) AND whose cmdline references hindsight_api.main. External holders (user terminals, unrelated scripts) are never killed — scanVenvBlockers still reports them and the hand-off aborts, per existing design. Selection logic is a pure DI'd module with unit tests. Salvaged from PR #75477 (scoped per review: the narrow daemon kill; the PR's generic every-exe kill was rejected as over-broad, its updateInFlight half was superseded by #75778/#73822, and its generic lock-probe half by the #74805 release gate + #99724 scanner classification).
|
Thanks @xy952666680 — closing as superseded: the generic venv-lock handling this proposed is now covered on main by the merged chain #99542 (foreign-owned venv refusal), #99711 (runtime repair deferred while the updater holds the venv), #99724 (ledger/identity-verified blocker classification), and #100124 (deferred-holder evidence). You were among the earliest to attack this class — appreciated. |
…ff (Windows) The pre-handoff teardown tree-kills only the backends the Desktop owns (backendConnectionState + backendPool). The memory plugin's hindsight daemon is spawned DETACHED off venv\Scripts\pythonw.exe, so it survives the teardown, keeps venv files mapped, and either dead-ends the venv-blocker scan with no in-app remedy or (pre-NousResearch#74805 shim-only gate) raced the updater into a half-updated venv. Add a narrowly-scoped reap: kill only processes whose exe lives under venv\Scripts (ordinal case-insensitive prefix — no PowerShell -like wildcard hazards) AND whose cmdline references hindsight_api.main. External holders (user terminals, unrelated scripts) are never killed — scanVenvBlockers still reports them and the hand-off aborts, per existing design. Selection logic is a pure DI'd module with unit tests. Salvaged from PR NousResearch#75477 (scoped per review: the narrow daemon kill; the PR's generic every-exe kill was rejected as over-broad, its updateInFlight half was superseded by NousResearch#75778/NousResearch#73822, and its generic lock-probe half by the NousResearch#74805 release gate + NousResearch#99724 scanner classification).
Summary
releaseBackendLock(apps/desktop/electron/main.ts) probed onlyvenv\Scripts\hermes.exefor the pre-update lock check. Any other process mapping venv files was invisible to the probe — most importantly the hindsight memory daemon, which runs offvenv\Scripts\pythonw.exe(in uv-created venvs this is a CONSOLE-subsystem trampoline). The updater then raced a still-lockedpythonw.exe, hit Access Denied mid venv sync, and stranded a half-updated install (broken imports, "An update is finishing…" boot loop from the stale marker).Change
Make the gate generic instead of a hardcoded shim list:
isVenvLocked(updateRoot)— probe-lock everyvenv\Scripts\*.exe(enumerate the directory, not 3 filenames); any mapped exe blocks the handoff.killVenvShimHolders(updateRoot)— before the wait loop, kill every process whoseExecutablePathis undervenv\Scripts(PowerShellGet-CimInstance Win32_Process→taskkill /T /F). Covers the hindsight daemon, stray CLIs, and any future holder — not just the one we knew about.releaseBackendLock; the existingscanVenvBlockerspsutil scan remains as the final abort-and-report backstop.Why this bug class
On Windows, any running process with a file under the venv mapped (shim exe,
.pyd) holds a mandatory lock. The previous code knew about exactly one file (hermes.exe) and one owner (the desktop's own backend). The hindsight daemon is spawned DETACHED by the memory plugin and outlives Hermes, so it was invisible to both the kill sweep and the lock probe.Verification
update-gate.test.ts+updater-process.test.ts: 10/10 passtsc -p tsconfig.electron.json --noEmit: cleangit stashbaseline comparison — same 17 fail without this change; ssh-config/ssh-connection/windows-hermes-path/desktop-installation/update-relaunch, environment-related)