fix(update): bound the Windows update hand-off's step pipe drain - #90564
fix(update): bound the Windows update hand-off's step pipe drain#90564jackulau wants to merge 2 commits into
Conversation
alt-glitch
added
type/bug
9c56f31 to
5ab801f
Compare
|
Note on the first CI run, since a red The only failure was: That is a Telegram slash-command test, and this branch cannot reach it — not "is unlikely to", cannot:
I could not re-run the job (fork PR, no admin rights on the repo), so I rebased onto Nothing else changed in the force-push: same two commits, clean rebase, |
5ab801f to
12c6184
Compare
Invoke-HermesStep collected each step's output with ReadToEndAsync().Result. That task does not complete when the step exits; it completes when the pipe reaches EOF. On Windows the write end of a redirected pipe goes to the child as an inheritable handle, so every descendant spawned without its own redirection holds a duplicate and EOF waits for the last of them to close it. hermes update deliberately runs its build steps with stdout inherited, so the tree under a step is arbitrarily deep and not something this script can enumerate. When one of those descendants is a resident gateway, the pipe stays open for the life of the gateway and the hand-off blocks forever. Everything the hand-off owes the Desktop is downstream of that call: .hermes-update-result.json is never written, .hermes-update-in-progress is never cleared, and the Desktop is never relaunched. The app sits on "Updating Hermes" until the user kills the gateway by hand, and the stale marker then refuses the next update too. Read both pipes in chunks into a StringBuilder and bound the drain once the step process itself has exited. The bound cannot truncate a slow step: the clock only starts after the process is gone, at which point everything it wrote is already in the pipe buffer waiting to be read, so the grace only has to cover the final drain. Chunked reads are what make abandoning safe at all, since .Result cannot hand back a partial read. Also switch to the bounded WaitForExit overload. The argument-less one waits on redirected streams as well, which is the same unbounded wait by another name. An abandoned drain logs one line to logs/desktop-update-handoff.log naming the cause, so a truncated step log is never mistaken for a step that printed nothing. Measured on Windows 11 / PowerShell 5.1 against a step whose grandchild inherits its stdout and outlives it by 45s: 47.4s before, 4.3s after, with the step's exit code and output preserved in both. Fixes NousResearch#90455
Four source-level guards on Invoke-HermesStep, scoped to that function so the legitimate WaitForExit and .Result uses elsewhere in the script cannot mask a regression: no ReadToEndAsync, a drain bound keyed on the step having exited, no argument-less WaitForExit, and a log line when a drain is abandoned. All four fail against the previous drain. They are source-level for the same reason the sibling python-handoff guard is: Linux CI cannot execute the PowerShell hand-off. Source-level is not enough for a deadlock, though, so the script also grows a -SelfTestPipeDrain fixture alongside the existing -SelfTestUi one. It needs no checkout, no install and no update: it starts a step that spawns a grandchild with UseShellExecute = $false and no redirection, which is exactly the shape that makes the grandchild inherit the step's stdout and stderr, then exits 7 while the grandchild sleeps on. The fixture asserts the grandchild was still alive when Invoke-HermesStep returned, so a pass cannot be a timing coincidence, and that the exit code and the step's output both survived the abandonment. A windows_only test drives it, so the OS lane runs the real drain rather than a text match. Measured on Windows 11 / PowerShell 5.1: 4.3s with the fix, 47.4s (the grandchild's full lifetime) with the previous drain restored. The python-handoff guard now reads the script with its -SelfTest* blocks removed. Those blocks exercise the machinery deliberately and exit before any marker, venv or desktop work, so the "every step drives python.exe, never the hermes.exe shim" rule does not apply to them. Scoping the source that way rather than allow-listing a target keeps that rule absolute for every real step. Refs NousResearch#90455
12c6184 to
7cb8b8b
Compare
|
Follow-up on the CI history here, so the red runs above are not left as an exercise for the reader. Run 1 — Run 2 — github.com was rate-limiting at the time — my own I can't re-run jobs on a fork PR, so I've rebased onto The |
|
Superseded by #90937, which keeps this diff as its first two commits — your diagnosis and layer choice were right, and the authorship is intact in history. Two changes on top. The drain was metering itself.
The four source-grep guards are gone. They are what let this through — all four pass on the submitted drain, because asserting the file's text contains Thanks for this one. The correction to the issue's root-cause section — going after suggestion 1, not finding the leaking spawn, and saying so instead of implying it — is the part that made the layer argument reviewable, and it's carried into the new PR. |
3 participants
What does this PR do?
Invoke-HermesStepinscripts/desktop-update/windows.ps1collected each update step's output withReadToEndAsync().Result. That task does not complete when the step exits; it completes when the pipe reaches EOF. On Windows the write end of a redirected pipe is handed to the child as an inheritable handle, so every descendant spawned without its own redirection holds a duplicate, and EOF waits for the last of them to close it.hermes updatedeliberately runs its build steps with stdout inherited (the tee-stderr runner inhermes_cli/main.py), so the process tree under a step is arbitrarily deep and not something this script can enumerate. When one of those descendants is a resident gateway, the pipe stays open for the life of the gateway.Everything the hand-off owes the Desktop sits downstream of that one call:
.hermes-update-result.jsonis never written,.hermes-update-in-progressis never cleared, and the Desktop is never relaunched. The app sits on "Updating Hermes" until the user kills the gateway by hand, and the stale marker then refuses the next update too.The fix: drain both pipes in chunks into a
StringBuilder, and bound the drain after the step process has exited.The bound cannot truncate a slow step. The clock only starts once the process is gone, at which point everything it wrote is already sitting in the pipe buffer ready to read, so the grace only has to cover the final drain — a 40-minute
uv pip installis untouched. Chunked reads are what make abandoning safe at all:ReadToEndAsync().Resultcannot hand back a partial read, so there is no way to give up on it without losing the whole step's log.Why this layer, and one honest correction to the issue
The issue's suggested fix 1 is to stop the gateway inheriting the pipes. I went looking for that spawn site first and could not find it — worth saying plainly rather than leaving it implied. Every Windows gateway spawn on the update path already redirects:
gateway_windows._spawn_detachedstdin=DEVNULL,stdout/stderr→logs/gateway-stdio.log,close_fds=Truegateway._spawn_gateway_restart_watcher(the watcher)stdout/stderr=DEVNULLstdout/stderr=DEVNULLSo I cannot name the descendant that holds the handle in the reporter's tree, and I have not claimed to. What I can show is that the hand-off's assumption — "the step exited, therefore its pipes are closed" — is unsound by construction on Windows, and that the shim is the layer where that matters, because it is the layer holding the marker and the result file. A fix at this layer is correct for whichever descendant it turns out to be, including ones that do not exist yet.
Issue suggestion 3 (a stale-marker watchdog) is a real second layer of defence but a different change; not bundled here.
The abandonment is visible
An abandoned drain writes one line to
logs/desktop-update-handoff.lognaming the cause. A silently truncated step log is indistinguishable from a step that printed nothing, and that log is whathermes debug sharecollects for update reports.Also switched to the bounded
WaitForExit(ms)overload: the argument-less one waits on redirected streams too, which is the same unbounded wait by another name.Related Issue
Fixes #90455
Type of Change
Changes Made
scripts/desktop-update/windows.ps1Invoke-HermesStep: chunkedReadAsyncinto aStringBuilderfor both pipes, with an abandon deadline armed at the step's exit ($script:StepDrainGraceSeconds, default 20s, overridable viaHERMES_UPDATE_PIPE_DRAIN_SECONDS— used by the self-test, not documented as a user knob).Step-PipeDrainhelper: advances one pipe by whatever has already arrived, never blocks, reports EOF. A faulted read is treated as EOF rather than retried forever.WaitForExit(5000)in place of the argument-less overload.-SelfTestPipeDrainswitch beside the existing-SelfTestUi, exiting before any marker/venv/desktop machinery.tests/test_desktop_update_windows_pipe_drain.py(new) — four source-level guards scoped to theInvoke-HermesStepbody, plus thewindows_onlytest that drives-SelfTestPipeDrain.tests/test_desktop_update_windows_python_handoff.py— its "every step drivespython.exe, never thehermes.exeshim" guard now reads the script with-SelfTest*blocks stripped. The pipe-drain fixture runs a synthetic PowerShell step throughInvoke-HermesStep, and it is not an update step. Scoping the source that way rather than allow-listing a target keeps the rule absolute for every real step.How to Test
The deadlock needs no update, no checkout and no Hermes install — only a step whose grandchild outlives it holding the inherited write end. That is what
-SelfTestPipeDrainbuilds:The fixture asserts the grandchild was still alive when
Invoke-HermesStepreturned — so a pass cannot be a timing coincidence — and that the exit code (7) and the step's output both survived.On Windows 11 / PowerShell 5.1:
47.4s is the grandchild's full 45s lifetime plus startup: the pre-fix drain waited out the leak exactly, which is the bug at 45-second scale. Note both runs preserve the exit code and the step's output — the fix costs nothing on the happy path.
Then:
All four source guards fail against the previous drain, and the
windows_onlytest fails with the FAIL line above, so none of them are decoration.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — partially: I ran the affected suite (tests/test_desktop_update_*.py, 14 passed / 2 skipped) rather than the whole tree, which has a pre-existing Windows-local failure baseline unrelated to this change. Linux CI is the authority for the full run; thewindows_onlylane is the authority for the new executable test.Documentation & Housekeeping
docs/, docstrings) — the reasoning lives in comments in the touched function and in the new test's module docstring; no user-facing docs change (the env var is a test seam, not a knob)cli-config.yaml.exampleCONTRIBUTING.md/AGENTS.mdscripts/desktop-update/posix.shis untouched. The new test's behavioural half iswindows_only; its source-level half runs everywhere and only reads the script.Screenshots / Logs
Included inline under How to Test above.