fix(desktop): let the venv blocker scan settle before aborting - #74831
fix(desktop): let the venv blocker scan settle before aborting#74831MaheshBhushan wants to merge 1 commit into
Conversation
applyUpdates tree-kills the desktop's own backend PIDs in releaseBackendLock, which then polls isShimLocked for up to 15s. On Windows the preflight immediately runs scanVenvBlockers exactly once, with no settling window. Windows does not retire tree-killed PIDs instantly — grandchildren cascade-settle over several scheduler ticks, so psutil.process_iter() still reports the backends the updater just killed. The single probe catches those remnants, returns 'blocked', and the update aborts with "another Hermes process is using this installation" on the first attempt from a healthy install. A manual retry succeeds because the process table has settled by then. Re-probe a blocked verdict (3 attempts, 500ms apart) before believing it, mirroring the poll-until-clear pattern releaseBackendLock already uses one call earlier. 'clear' is never re-probed, and neither is 'probe-failure' — a broken probe cannot become informative by repeating, and the caller aborts on that outcome regardless. The single-probe body moves to runVenvBlockerProbe so the retry policy and the subprocess call stay separately testable. main.ts is unchanged; it picks up the retry through the defaults. Only the process-table race is addressed here. The missing auto-relaunch after a retried update is a separate failure in the Tauri updater's Windows relaunch path and is left open. Refs NousResearch#74805
|
Thanks for the focused fix. Current main still calls The scope is appropriately limited to the process-table settlement path; #74805's separate updater relaunch report remains open. The current GitHub main is otherwise unchanged at the affected scanner and call-site, and GitHub reports the PR mergeable. Automated hermes-sweeper review. |
…ackend 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>
…ackend 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>
|
Your settle-and-retry design landed via #95313 (merge b04f857) — you were the earliest submitter of the re-scan idea for #74805, and you're credited as Co-authored-by on the merged commit. The landed version pairs your re-scan settle with a PID exit-wait in the unlock gate (from #78037), which closes the root race rather than only the scan-side symptom. Thanks for the contribution and the clean test file. |
…ackend processes (NousResearch#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 NousResearch#78037 fix 1 by @3x3xX3N0N onto the post-NousResearch#87599 code shape (stopBackendTreesForUpdate extraction, stopSafeBlockers re-scan path). The re-scan settle idea was first submitted by @MaheshBhushan (NousResearch#74831); the killed-PID tracking seam matches @webtecnica's NousResearch#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>
…ackend processes (NousResearch#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 NousResearch#78037 fix 1 by @3x3xX3N0N onto the post-NousResearch#87599 code shape (stopBackendTreesForUpdate extraction, stopSafeBlockers re-scan path). The re-scan settle idea was first submitted by @MaheshBhushan (NousResearch#74831); the killed-PID tracking seam matches @webtecnica's NousResearch#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>
What changed and why
applyUpdatescallsreleaseBackendLockForUpdate, which tree-kills the desktop's own backend PIDs and then pollsisShimLockedfor up to 15s. Immediately after that, on Windows, it runsscanVenvBlockers— once, with no settling window:Windows does not retire tree-killed PIDs instantly; grandchildren cascade-settle over several scheduler ticks, so
psutil.process_iter()still reports the dying backends the updater just killed. The single probe catches those remnants, returnsblocked, and the update aborts with "another Hermes process is using this installation" — on the first attempt from a healthy install. A manual retry succeeds because the table has settled by then.This makes
scanVenvBlockersre-probe ablockedverdict (3 attempts, 500ms apart) before believing it, mirroring the poll-until-clear patternreleaseBackendLockalready uses one call earlier.clearis never re-probed, andprobe-failureis never re-probed either — a broken probe cannot become informative by repeating, and the caller aborts on that outcome regardless.main.tsis unchanged: it callsscanVenvBlockers(updateRoot)and picks up the retry through the defaults.Refs #74805
Scope — this fixes root cause 1 only
The issue reports two root causes. I fixed the first and deliberately left the second alone:
Root cause 1 (fixed) — the scan/process-table race. This is a verifiable property of the code: one probe, no settle, directly adjacent to a function that polls for 15s.
Root cause 2 (not fixed) — no auto-relaunch after a retried update. The issue offers three competing hypotheses for this ("this can happen if…: renderer takes a different code path / stale marker not overwritten / updater state machine sees a pre-existing marker"). It lives in the Tauri updater's Windows relaunch path, which I cannot observe, instrument, or test from Linux. Picking one of three guesses and shipping it as a fix would be worse than leaving it clearly open. It needs someone who can run the Windows updater with logging.
Because of that, this is
Refs #74805rather thanFixes— the issue should stay open for the relaunch half.How it was tested
Six unit tests added to
apps/desktop/electron/venv-blocker-scan.test.ts, using the dependency-injection hooks the module already exposes:clear, 2 probes — the actual raceblocked, exhausts the 3-attempt budgetattempts: 2, waits[250]attempts: 0A
sleepinjection point keeps the tests instant rather than sleeping 1s.Result — whole file, 20 pre-existing tests plus the 6 new ones:
With
venv-blocker-scan.tsreverted, 3 of the 6 new tests fail (the three that assert re-probing); the other 3 assert a single probe and correctly hold either way, since the old code also probed once:I also checked the un-injected default path end to end: three probes with the real
setTimeoutsleep took 1009ms, confirming the 3×500ms defaults behave as intended.Both under the project's own runner,
vitest run --project electron:No regressions anywhere in the electron project.
What I could not test at all: I have no Windows machine, so I did not reproduce the underlying OS race, and I did not exercise the real
_scan_venv_blockers.pyagainst a settling process table. The retry logic is verified by injection; the premise that Windows lags in retiring tree-killed PIDs is taken from the issue report and from the fact thatreleaseBackendLockalready compensates for the same effect on the shim lock. If a maintainer on Windows can confirm the first-attempt failure disappears, that would close the loop.Timing impact
On the genuinely blocked path the preflight now takes longer before showing the error: worst case 3 probes plus 1s of sleeps.
SCAN_TIMEOUT_MSis a 15s ceiling rather than a typical duration — a normalpsutilsweep returns well under a second, so the realistic blocked path goes from roughly 0.5s to roughly 2.5s. Worst case is bounded at ~46s, comfortably insideUPDATE_WAIT_TIMEOUT_MS(20 min). Theclearpath is unchanged at one probe.If you'd rather trade less latency for less race tolerance,
SCAN_BLOCKED_ATTEMPTS/SCAN_SETTLE_DELAY_MSare the two knobs.Open questions
releaseBackendLock's 15s budget may fit better than a fixed attempt count.startHermes()? The issue's suggestion 2 proposes not restarting the backend when the blockers are the same PIDs we just killed, so a retry starts clean. That is a behaviour change to the error path and I left it out of this PR, but it would compose well with this change — happy to add it if you want it here rather than separately.spawnUpdaterProcessorphaning) both belong to root cause 2 and are untouched.Related
Adjacent but distinct: #74267 (persistent false positive, survives reboot — this is the transient variant), #63717, #44143, #62311. Open PR #74419 touches Windows update gateway coordination for #74386; no overlap with this file.