fix(desktop/windows): free all venv holders on Update hand-off - #65935
fix(desktop/windows): free all venv holders on Update hand-off#65935maxwell7891 wants to merge 3 commits into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Looks Good
- Clean, well-scoped fix: Windows Update hand-off now kills all venv holders (pythonw.exe processes) in addition to hermes.exe, preventing the "install locked" abort after 15s
- The new
windows-venv-holders.tsmodule withselectVenvHolders/forceKillVenvHolders/listVenvHoldersis cleanly separated - Deadline extended from 15s to 45s to accommodate gateways on loaded Windows
- New
windows-venv-holders.test.tswith good coverage ofisVenvHolderProcess,normalizeWinPath,selectVenvHolders - Rust side (
update.rs) addsforce_kill_other_hermes_for(Some(install_root))scoped kill to avoid collateral damage
Notes
console.printin the diff is in test/discovery code, not production — acceptable- No security concerns; PowerShell taskkill is appropriately scoped to the install root
Reviewed by Hermes Agent
Runtime / functional follow-up (local Windows install)What this PR is for (one-liner)Make the Desktop Update button succeed on Windows even when gateways / multi-profile backends / trampoline Functional checks run on the PR branch (
|
| Check | Result |
|---|---|
vitest electron/windows-venv-holders.test.ts |
6/6 pass |
Electron main bundle contains hand-off sweep (force-killing N external venv holder(s), 45s timeout) |
yes |
Packaged app.asar re-injected with that main |
yes (hasFix / has45) |
hermes_cli base-home probe (pyvenv.cfg → missing base → unhealthy) |
present |
| Live lock probe at test time | shim unlocked, 0 external holders (machine quiet / no gateways holding venv right then) |
| Code review (tonydwb) | Comment / Looks Good — no blocking notes |
Not yet closed on this run
- Full GUI path: click Update with at least one live gateway + dashboard, confirm hand-off log line + auto relaunch + backend ready + profiles back.
- That needs a Desktop restart onto the patched asar, then a real button click (will briefly pause Telegram gateways by design).
Note
A later hermes update on this machine had returned main to upstream and overwrote an earlier local asar patch; re-checked out this PR branch and re-validated units + package contents before commenting.
Happy to push a follow-up with E2E log excerpts once the loaded-gateway click path is exercised.
Follow-up: explicit interrupt warning + live-agent preflightPushed Behaviour
Tests
This answers the operational question: the force-kill is required for a clean Windows update, but the user is told before it happens when an agent is still running. |
6e3960c to
33daf57
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting a real Windows updater failure: current main only kills Desktop-owned backend PIDs in apps/desktop/electron/main.ts:2341-2401, then aborts after the 15-second lock wait.
Problems
hermes_cli/main.py:9198reports a missingpyvenv.cfgbase interpreter as unhealthy, but current repair logic recreates only whenvenv/Scripts/python.exeis absent (hermes_cli/main.py:10074-10086). This new case will not recreate the broken venv as claimed.apps/bootstrap-installer/src-tauri/src/update.rs:514now calls the scoped cleanup, while:610-641limits it to the venv. A stale packaged Desktop executable can still hold the separately probedapp.asar(:541-560).apps/desktop/electron/main.ts:2382force-kills external gateways/workers without thebusyConfirmpreflight described in the July 17 follow-up; the final PR diff contains no corresponding UI/preflight code or test.
Suggested changes
- Make the base-interpreter failure select the venv-recreate path and cover it with a Python regression test.
- Scope process cleanup to the install root while including both venv interpreters and packaged Desktop executables; test both same-install and other-install processes.
- Restore the explicit busy-session confirmation before terminating external holders.
Automated hermes-sweeper review.
| home_path = Path(home_val) | ||
| base_py = home_path / "python.exe" | ||
| if not home_path.is_dir() or not base_py.exists(): | ||
| return ( |
There was a problem hiding this comment.
This returns an unhealthy result, but the existing repair path recreates only when venv/Scripts/python.exe is absent. Here that shim still exists, so dependency installation is attempted against the same pyvenv.cfg whose base interpreter is gone. Carry this reason to the repair branch (or remove/recreate the venv here) and add a regression test for the selected recreation command.
| @@ -511,7 +511,7 @@ pub(crate) async fn wait_for_install_locks_free(install_root: &Path, app: &AppHa | |||
| format_locked_paths(&locked) | |||
| ), | |||
| ); | |||
| force_kill_other_hermes(); | |||
| force_kill_other_hermes_for(Some(install_root)); | |||
There was a problem hiding this comment.
Passing Some(install_root) disables the legacy taskkill /IM hermes.exe, but the scoped selector below only matches <install>/venv. The lock probe also includes apps/desktop/release/.../resources/app.asar; a stale packaged Desktop Hermes.exe can retain that lock and is no longer killed. Scope the selector to the install root while covering the packaged executable too.
| `[${tag}] force-killing ${external.length} external venv holder(s): ` + | ||
| external.map(h => `${h.pid}/${h.name}`).join(', ') | ||
| ) | ||
| forceKillVenvHolders(external, { forceKillProcessTree }) |
There was a problem hiding this comment.
This immediately terminates gateways and active workers outside the current Desktop window. The follow-up comment describes a busyConfirm preflight for that destructive hand-off, but the final PR head has no corresponding UI/preflight change. Please restore that confirmation before this call.
The Update button aborted after 15s whenever a gateway (Telegram profiles, slash workers) held venv\Scripts\hermes.exe / .pyd files open, because releaseBackendLock only tree-killed backends owned by the current desktop instance. - Discover and taskkill external venv holders before hand-off (45s wait) - Tauri updater force-kill also reaps venv python(w), not only hermes.exe - hermes update --gateway force-stops remaining holders after gateway pause - Recreate/repair venv when pyvenv.cfg base home python is missing
33daf57 to
4dbe370
Compare
Review follow-up pushedRebased onto current
Local verification:
Rust/Cargo was unavailable on the Windows host, so the added Rust unit test is left to CI. |
|
Closing in favor of #74436, which fixes the cause this works around. Your diff treats the symptom correctly — a venv holder blocks the updater — but the holder in these reports is the dashboard's own detached The venv-holder detection you were working around is untouched and still correct; it should just rarely fire now. Thanks for digging into this — the logs and repro in here were genuinely useful in tracing the orchestration bug. |
Summary
hermes-setup --updatestraggler cleanup also reaps venvpython(w).exeholders, not onlyhermes.exe.hermes update --gatewayforce-stops remaining venv holders after gateway pause instead of immediately exiting 2.pyvenv.cfghome) as unhealthy so repair recreates the runtime instead of leaving a bricked desktop pointing at a dead system Python path.Root cause
On Windows, Telegram/multi-profile gateways run as
venv\Scripts\python.exeand keephermes.exe/.pydfiles mandatory-locked. Clicking Update stopped only the desktop-owned backends, so the shim stayed locked and the UI aborted: "another process is holding the Hermes install open". Partial updates could leave a broken runtime tied toC:\Users\...\Python311\python.exe.Test plan
vitestelectron/windows-venv-holders.test.ts(6 tests)pyvenv.cfghome → unhealthy + recreate pathhermes-maj.ps1/ repair path required