fix(update): tree-aware orphan reap + drain Desktop update trees without pre-signalling (follow-up to #82179) - #82191
Conversation
…te trees without pre-signalling Follow-up to #82179 addressing helix4u's review comment (#82179 issuecomment-5229441571). Three parts: 1. Desktop teardown (salvaged from #77436, @4adwentures): the update hand-off's releaseBackendLock() sent SIGTERM to the primary backend BEFORE taskkill /T. If the launcher exits first, Windows can no longer enumerate its descendants and they survive holding the venv — the Electron path that creates the orphan #82179 then has to repair. New stopBackendTreesForUpdate() tree-kills the live root first, with the behavioral vitest from #77436. The scanner half of #77436 is deliberately NOT taken (superseded by #82158's full-cmdline scan). 2. Tree-aware orphan classification: _orphaned_desktop_backend_pids() previously refused the whole holder set when any holder had a live parent. But the scanner legitimately returns an orphaned serve root AND its descendants (the venv trampoline's uv-managed interpreter worker — which carries the same backend argv — plus .hermes-runtime children). Those have a live parent: the orphan root itself. Now holders inside an accepted orphan root's tree fold into that root (only roots are returned; taskkill /T reaps descendants), and live-parent backends defer to the ancestry check instead of refusing outright. Anything outside an orphan tree still refuses. 3. Tests for the mixed shapes: root+managed-runtime child, grandchild depth, non-descendant stray alongside an orphan root (still refuses), descendant exited mid-classify. E2E on a real Windows box: spawned a detached backend-shaped orphan that itself spawned children (3 python descendants); the scanner-shaped mixed holder set classified to [root], taskkill /T reaped root and all descendants. The live Desktop backend on the box still classified None (refusal preserved). The first E2E attempt caught exactly the trampoline/worker case the mocks missed — the live worker re-execs with the same backend argv and a live parent — which is what part 2 fixes. Co-Authored-By: 4adwentures <296413879+4adwentures@users.noreply.github.com>
૮ >ﻌ< ა ci reviewran on c982912
|
|
Thanks, this addresses the Electron teardown ordering and the mixed root/managed-runtime descendant case from my review. The source-side tree kill, downstream orphan recovery, tests, and #77436 attribution all look good. One correction to my previous comment: I was too broad in saying #82158 superseded the scanner portion of #77436. #82158 fixed truncated command lines for a gateway process whose own argv contains I do not see a blocker in the Electron or orphan-classifier changes here. For the scanner portion, either port |
alt-glitch
added
type/bug
4 participants
What does this PR do?
Follow-up to #82179, implementing all three asks from @helix4u's review comment (which landed as the PR was merging):
Desktop teardown fix (ported from fix(desktop): drain Windows update process trees #77436, credit @4adwentures):
releaseBackendLock()sent SIGTERM to the primary backend beforeforceKillProcessTree(). If that launcher exits beforetaskkill /Truns, Windows can no longer enumerate its descendants and they survive holding the venv — i.e. the owning Electron path could still create the orphan that fix(update): reap orphaned Desktop backends + stop managed runtime child trees (salvages #81327) #82179 repairs. NewstopBackendTreesForUpdate()inbackend-child.tstree-kills the live root first (no pre-signal), pool teardown unchanged. Includes fix(desktop): drain Windows update process trees #77436's behavioral vitest. Per the review, fix(desktop): drain Windows update process trees #77436's_scan_venv_blockers.pychanges are not taken — that area moved through fix(update): venv-blocker scan truncated cmdlines, breaking the gateway exemption and dead-ending Desktop updates #82158 (they no longer apply cleanly).Tree-aware orphan classification:
_orphaned_desktop_backend_pids()refused the whole set when any holder had a live parent. But the scanner legitimately returns an orphanedserveroot and its descendants — including the uv-managed interpreter worker, which re-execs with the same backend argv and whose live parent is the orphan root itself. Holders inside an accepted orphan root's tree now fold into that root (only roots are returned;taskkill /Treaps the descendants). Live-parent backends defer to the ancestry check instead of refusing outright; anything outside an orphan tree still refuses.Coverage for the mixed shapes: root + managed-runtime child, grandchild depth, non-descendant stray alongside an orphan root (still refuses), descendant exited mid-classify.
Type of Change
How to Test
pytest tests/hermes_cli/test_update_orphan_backend_reap.py tests/hermes_cli/test_update_venv_health.py -o "addopts=--timeout-method=thread"— 21 passed.npm run typecheck(3 projects),npx vitest run electron/windows-child-options.test.ts— 13 passed incl. the no-pre-signal ordering test,npx eslintclean on touched files.E2E verification (real Windows 11 box)
[root];_stop_process_treesreaped root and all descendants (verified via psutil).None, refusal preserved.serveargv + a live parent) is invisible to mocks. That live failure is exactly the case part 2 fixes.Checklist
_is_windows();stopBackendTreesForUpdateonly reached from the Windows shim-unlock pathSupersedes the desktop-teardown portion of #77436; the scanner portion is superseded by #82158.