2b1bff624e
* fix(update): bound the Windows update hand-off's step pipe drain 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 #90455 * test(update): prove the hand-off survives a step that leaks its pipe 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 #90455 * fix(update): don't meter the step drain that #90455's bound introduced Chunked reads make the bounded drain possible, but the loop idled 150ms after every chunk it consumed, so a step's output moved at one 16 KiB buffer per tick (~107 KB/s). The pipe then backs up, which is backpressure on the *running* step rather than a slow read: a chatty step blocks on write() waiting for the reader. `hermes update` is exactly that shape -- the Electron/vite build alone is megabytes -- so the layer that fixed "the hand-off waits forever" would have shipped "the hand-off is slow" in its place. Idle only when both pipes came up empty, and idle on the reads themselves (WaitAny with the same 150ms cap) rather than on the clock: a freshly issued ReadAsync is rarely complete by the very next pass, so a bare `if (-not $moved)` still sleeps between chunks. WaitAny expires on its own, so a silent step keeps the marquee animating and keeps the abandon deadline advancing. Measured against the drain as submitted, same harness, one variable: 4 MiB of step stdout 38.99s -> 0.07s 1 MiB stdout + 1 MiB err 18.22s -> 0.27s leaked grandchild (20s) 3.24s -> 3.20s, exit code + output kept quiet step, exits at 4s 4.29s -> 4.04s, 29 passes (not spinning) * test(update): make the pipe-drain fixture cover metering, not just deadlock The fixture proved the drain returns while a descendant holds the pipe. It could not have caught the opposite failure -- a drain slow enough to backpressure the step it is reading -- and that is the regression the first version of this fix shipped. Add a flood arm: a step that writes megabytes and holds nothing, with a wall-clock budget far under what a sleep-per-chunk drain needs. The two arms bracket the contract from both sides: bounded when a descendant holds the pipe open, never slower than the step can write. Few large lines rather than many small ones, deliberately -- Write-HandoffLog is one Add-Content per line and runs inside the measured window, so line-heavy output would time the logger. Also drops the four source-grep guards. Reading windows.ps1's text to assert it contains `$abandonAt` tests the shape of the source, not its behavior: it passes on a drain that is wired wrong but spelled right, fails on a correct refactor, and blocks the extraction it should survive. AGENTS.md bans the pattern outright, and all four pass on the metered drain. The executable arms cover the same contract and actually run the code -- the Windows lane is where this is verified either way. --------- Co-authored-by: Jack Lau <72348727+jackulau@users.noreply.github.com>
124 lines
5.3 KiB
Python
124 lines
5.3 KiB
Python
"""Regression: the Windows Desktop update hand-off must run through python.exe.
|
|
|
|
`scripts/desktop-update/windows.ps1` drives `hermes update` for the in-app
|
|
Desktop updater. It used to invoke the update through the venv's
|
|
`venv\\Scripts\\hermes.exe` console-script launcher. On Windows that launcher is
|
|
a real process that keeps `hermes.exe` mapped as its running image and spawns
|
|
`python.exe` as a child. The update ends in `uv pip install -e .`, which rewrites
|
|
the console-script shims -- including the `hermes.exe` the launcher still has
|
|
mapped -- and Windows refuses to replace a file mapped as a running image
|
|
("os error 32"). The rename fallback then defers to next reboot via
|
|
`MOVEFILE_DELAY_UNTIL_REBOOT`, which needs elevation a Desktop-driven update
|
|
does not have, so `uv pip install -e .` exits non-zero, the ZIP fallback repeats
|
|
the same sequence, the desktop build stage is never reached, and the pre-build
|
|
clean has already removed `apps/desktop/release` -- leaving an install whose
|
|
Start Menu shortcut points at a `Hermes.exe` that no longer exists.
|
|
|
|
Driving the update as `python.exe -m hermes_cli.main update` puts the inherited
|
|
image handle on `python.exe`, which uv never has to replace, so the shim is an
|
|
ordinary unlocked file when uv rewrites it.
|
|
|
|
This test is source-level because Linux CI cannot execute the PowerShell
|
|
hand-off. The invariant it guards is that every `Invoke-HermesStep` call site
|
|
(the update, its retry, and the desktop rebuild) drives `$pythonExe`, never the
|
|
`$hermesExe` shim. `hermes.exe` may still be *named* in the file for the
|
|
step-2 unlock preflight -- that is a lock probe, not an invocation -- so we
|
|
assert against the invocation sites specifically.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import re
|
|
from pathlib import Path
|
|
|
|
|
|
REPO_ROOT = Path(__file__).resolve().parent.parent
|
|
WINDOWS_PS1 = REPO_ROOT / "scripts" / "desktop-update" / "windows.ps1"
|
|
|
|
|
|
def _read() -> str:
|
|
return WINDOWS_PS1.read_text(encoding="utf-8")
|
|
|
|
|
|
def _handoff_source() -> str:
|
|
"""The script with its ``-SelfTest*`` fixture blocks removed.
|
|
|
|
Those blocks exercise the hand-off machinery deliberately -- the pipe-drain
|
|
fixture runs a synthetic PowerShell step through ``Invoke-HermesStep`` to
|
|
prove the drain cannot deadlock (#90455) -- so they are not update steps
|
|
and the "must drive python.exe" rule does not apply to them. Each exits
|
|
before any marker/venv/desktop machinery runs.
|
|
|
|
Scoped here rather than allow-listing a target, so the rule stays absolute
|
|
for every real step. The non-greedy match ends at the first closing brace
|
|
in column 0; the blocks' own braces are all indented.
|
|
"""
|
|
return re.sub(r"\nif \(\$SelfTest\w+\) \{.*?\n\}\n", "\n", _read(), flags=re.S)
|
|
|
|
|
|
def test_invoke_hermes_step_calls_drive_python_not_the_shim() -> None:
|
|
source = _handoff_source()
|
|
|
|
invocations = re.findall(r"Invoke-HermesStep\s+(\$\w+)", source)
|
|
assert invocations, (
|
|
"Expected at least one Invoke-HermesStep call in "
|
|
"scripts/desktop-update/windows.ps1; the update hand-off structure "
|
|
"changed -- update this guard."
|
|
)
|
|
|
|
offenders = [exe for exe in invocations if exe != "$pythonExe"]
|
|
assert not offenders, (
|
|
"Every Invoke-HermesStep call in scripts/desktop-update/windows.ps1 "
|
|
"must drive $pythonExe, not the hermes.exe shim. Driving the update "
|
|
"through the shim keeps hermes.exe mapped as a running image, so uv's "
|
|
"final shim rewrite fails with os error 32 and the Desktop update can "
|
|
"never complete. Offending target(s): "
|
|
f"{sorted(set(offenders))}."
|
|
)
|
|
|
|
|
|
def test_update_invocation_uses_module_entrypoint() -> None:
|
|
source = _read()
|
|
|
|
assert '@("-m", "hermes_cli.main", "update"' in source, (
|
|
"The update step must invoke `python.exe -m hermes_cli.main update ...` "
|
|
"so the inherited image handle lands on python.exe, which uv never has "
|
|
"to replace."
|
|
)
|
|
assert (
|
|
'@("-m", "hermes_cli.main", "desktop", "--force-build", "--build-only")'
|
|
in source
|
|
), (
|
|
"The desktop rebuild step must also go through "
|
|
"`python.exe -m hermes_cli.main desktop ...` for the same reason."
|
|
)
|
|
|
|
|
|
def test_update_no_longer_invokes_the_hermes_exe_shim() -> None:
|
|
source = _read()
|
|
|
|
assert "Invoke-HermesStep $hermesExe" not in source, (
|
|
"scripts/desktop-update/windows.ps1 still invokes the update through "
|
|
"the hermes.exe shim (`Invoke-HermesStep $hermesExe`). That is the "
|
|
"exact self-lock this fix removes -- route it through $pythonExe "
|
|
"instead."
|
|
)
|
|
|
|
|
|
def test_desktop_relaunch_waits_for_an_in_place_rebuild() -> None:
|
|
source = _read()
|
|
relaunch = re.search(
|
|
r"function Start-DesktopRelaunch \{(?P<body>.*?)\n\}\n\nfunction Invoke-HermesStep",
|
|
source,
|
|
re.DOTALL,
|
|
)
|
|
assert relaunch, "Expected Start-DesktopRelaunch in the Windows hand-off script."
|
|
|
|
body = relaunch.group("body")
|
|
assert "if (-not $RelaunchExe) { return $false }" in body
|
|
assert "$relaunchDeadline = (Get-Date).AddSeconds(120)" in body
|
|
assert "while (-not (Test-Path -LiteralPath $RelaunchExe))" in body
|
|
assert "if ((Get-Date) -ge $relaunchDeadline)" in body
|
|
assert "Start-Sleep -Milliseconds 500" in body
|
|
assert "[System.Windows.Forms.Application]::DoEvents()" in body
|