fix(serve): write READY sentinel to real fd 1 so desktop handshake survives stdout redirect - #96289
Conversation
…rvives stdout redirect Root cause (NousResearch#96282): tui_gateway.server mutates sys.stdout at import time (sys.stdout = sys.stderr), and 6d4e851 added that import to the serve startup path before the port sentinel is printed. The desktop spawn's waitForDashboardPort watches only child.stdout for HERMES_BACKEND_READY/HERMES_DASHBOARD_READY, so a healthy backend was SIGTERMed after the 90s boot timeout and the renderer looped "refreshProfiles failed". Fix: after the print, also write the sentinel to sys.__stdout__ (the real fd 1) so the Electron handshake succeeds regardless of any serve-path stdout redirect. Guarded in try/except. Regression test: add _spawn_serve_separate_streams (existing _spawn_serve merges stderr into stdout with stderr=subprocess.STDOUT, which is why CI missed this) and test_ready_sentinel_on_stdout_for_desktop, asserting the sentinel appears on stdout. Verified: test fails without the fix (120s timeout), passes with it; full file 9/9 green. Closes NousResearch#96282
andrexibiza
left a comment
There was a problem hiding this comment.
Exact-head topology review for 771ed1b04b0d7c2a5a1fd42340994269d17c22ed.
This is a real fix for #96282's Python-level stream-redirection regression, and the separate-stream test closes the hole that let merged-stderr CI miss it. But this PR should not be treated as the general Desktop readiness fix, and it should not race a second carrier for the same two-file defect.
Two current facts matter:
- #96292 now changes the same two paths for the same #96282 root cause, but emits the sentinel once directly to the original stdout instead of first printing through redirected
sys.stdoutand then duplicating it throughsys.__stdout__. These carriers should be consolidated to one implementation before merge; landing both would be duplicate work in the same source/test seam. - #96280 is a distinct Windows realization failure. On the reported uv-managed venv, Electron owns the launcher process while the serving interpreter is a grandchild, and no child.stdout chunks reach Electron at all even though the backend is healthy. Writing fd 1 correctly inside the interpreter fixes #96282's
sys.stdoutrebind, but it does not prove that the launcher's inherited pipe reaches Electron on that Windows topology. The repository already has the stronger explicitHERMES_DESKTOP_READY_FILE/waitForDashboardReadyFile()contract for exactly that class.
So the clean boundary is: one real-stdout carrier closes #96282 and gets separate-stream regression evidence; #96280 remains open until the Desktop local-backend path actually opts into and proves the explicit readiness artifact across the uv launcher/interpreter process tree. Do not collapse those two bugs just because they share the same 90-second symptom.
I also interlocked this distinction into #91079's packaged-Windows acceptance contract: a future package/recovery witness may not use a stdout READY line as sufficient settlement; it must prove readiness survives the realized Windows process topology.
Duplicate of #92631: both restore the Desktop READY sentinel to real stdout/fd 1 after the tui gateway redirects Python stdout. The earlier open PR is the canonical focused repair. |
|
Fixed by #96311 (merged, commit 8d95ab1), which salvaged #96284 by @kitsonk with authorship preserved. Your PR identified the same fd-1 fix for the READY sentinel. #96284 was picked as the base because it shipped a split-stream regression test pinning the fix (the existing E2E tests merged stderr into stdout, which is how the regression passed CI). The merged PR also widened the fix to the BACKEND_PORT_IN_USE sibling sentinel. Thanks for the contribution! |
Bug Description
Fixes #96282
Since upgrading to the build containing
6d4e851d8(fix(serve): bounded flush-on-SIGTERM + periodic incremental session flush, #94724 item 2), Hermes Desktop fails to boot its local backend with:even though the backend is fully healthy —
HERMES_BACKEND_READY port=XXXXXis present indesktop.logbut arrives on the wrong stream.Root Cause
tui_gateway/server.py:401redirects stdout at import time:6d4e851d8added this import to the serve startup path (hermes_cli/web_server.py):which runs before the port sentinel is printed:
so the sentinel now lands on stderr, while the desktop spawn (
apps/desktop/electron/backend-ready.ts,waitForDashboardPort) watches child.stdout only. Result: healthy backend announces in ~2s, desktop times out at 90s, SIGTERMs the backend, and the renderer loopsrefreshProfiles failed after 3 attempt(s).Fix
After the sentinel
print, also write it to the real fd 1 (sys.__stdout__) so the Electron handshake succeeds regardless of any serve-path stdout redirect:This is minimal and robust to either future fix (not mutating
sys.stdoutglobally at import, or restoring it before the sentinel).Regression Test
tests/hermes_cli/test_serve_port_in_use.py:_spawn_serve_separate_streams— the existing_spawn_servemerges stderr into stdout (stderr=subprocess.STDOUT), which is exactly why CI missed this bug.test_ready_sentinel_on_stdout_for_desktop— asserts the sentinel appears on stdout specifically (the stream Electron reads).How to Verify
python -m pytest tests/hermes_cli/test_serve_port_in_use.py -v→ 9/9 passtest_ready_sentinel_on_stdout_for_desktopfails withno READY sentinel on stdout(verified locally, 120s timeout)desktop.logshowsHermes backend is ready. Finalizing desktop startup(verified locally on macOS arm64)Test Plan
Risk Assessment
Low — the added write is wrapped in try/except, only runs on the successful-bind path, and writes a line identical to what
printalready emits. The only behavioral change is that the sentinel line also appears on the real fd 1, which is what Electron already expects.