diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 6a6f026c749..4c7687f5281 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -14119,13 +14119,24 @@ def start_server( # OSError inside create_server() and exits with a clear error — no # separate preflight probe needed. # Loopback binds are the Desktop case: a single local client, no reverse - # proxy in front. A GIL-heavy agent turn can stall the event loop past 20s, - # and uvicorn's ws keepalive ping runs on that same starved loop — so a - # 20s ping timeout kills an otherwise-healthy local connection over a - # recoverable stall (QW-1). Give loopback a longer 60s timeout / 30s - # interval to ride out those stalls. Non-loopback binds sit behind a - # Cloudflare Tunnel (idle timeout ~100s), so keep them at 20/20 to detect - # half-open connections promptly and stay under the tunnel's idle window. + # proxy in front. uvicorn's ws keepalive ping runs ON the same event loop + # as agent turns, and a single synchronous GIL-holding call on a worker + # thread (e.g. a regex/scrub over a large model output, or a long + # delegate_task subagent turn) can starve that loop for *minutes* — the + # loop cannot process the incoming pong, so uvicorn declares the socket + # dead and closes it, dropping an otherwise-healthy local connection + # (#53773: "event loop stalled 226.3s"; #48445/#50005). A longer timeout + # only raises the threshold — a multi-minute stall sails past any finite + # window. The keepalive ping exists to detect *half-open* connections + # (reverse-proxy 524, dropped tunnels), which cannot happen on loopback: + # there is no network or proxy in the path, and a dead local client tears + # the socket down with a real FIN/RST that starlette surfaces as + # WebSocketDisconnect regardless of the ping. So on loopback the ping + # provides ~no liveness value while actively killing recoverable stalls — + # disable it entirely. Non-loopback binds sit behind a Cloudflare Tunnel + # (idle timeout ~100s) where half-open IS a real failure mode, so keep the + # ping at 20/20 to detect it promptly and stay under the tunnel's idle + # window. _is_loopback = host in ("127.0.0.1", "localhost", "::1") config = uvicorn.Config( app, host=host, port=port, log_level="warning", @@ -14137,12 +14148,12 @@ def start_server( # decide cookie Secure flags, so we flip proxy_headers on for that # mode. proxy_headers=bool(app.state.auth_required), - # Detect half-open WS connections (reverse-proxy 524, dropped - # tunnels) within ~20-40s so WebSocketDisconnect fires the - # disconnect→reap path. 20s stays under Cloudflare Tunnel's idle - # timeout, keeping it warm. Loopback gets a longer window (see above). - ws_ping_interval=30.0 if _is_loopback else 20.0, - ws_ping_timeout=60.0 if _is_loopback else 20.0, + # Half-open detection for public binds only (see above). Loopback + # disables the protocol ping (None) so an event-loop stall can never + # trigger a false disconnect; a genuinely dead local client is still + # reaped via the WebSocketDisconnect → disconnect/reap path. + ws_ping_interval=None if _is_loopback else 20.0, + ws_ping_timeout=None if _is_loopback else 20.0, ) server = uvicorn.Server(config) diff --git a/tests/test_web_server.py b/tests/test_web_server.py index a525b6f828a..ee795542d85 100644 --- a/tests/test_web_server.py +++ b/tests/test_web_server.py @@ -69,20 +69,48 @@ def _stub_uvicorn(monkeypatch): return captured -def test_start_server_enables_ws_ping_for_half_open_detection(monkeypatch): - """WS ping must be configured so half-open connections (reverse-proxy 524, - dropped tunnels) raise WebSocketDisconnect into the reaping path (#32377). +def test_start_server_disables_ws_ping_on_loopback(monkeypatch): + """Loopback binds (the Desktop case) MUST disable uvicorn's protocol-level + keepalive ping so an event-loop stall can never trigger a false disconnect. - Loopback binds (the Desktop case) get a longer window to ride out - GIL-pressure event-loop stalls (#48445/#50005). The invariant asserted - here is that ping stays enabled (non-None, positive) and the timeout is - never shorter than the interval — not a frozen literal, which churns every - time the window is retuned.""" + uvicorn's ws ping runs on the same event loop as agent turns. A single + synchronous GIL-holding call on a worker thread can starve that loop for + minutes, so the loop can't process the pong and uvicorn kills an + otherwise-healthy local connection (#53773 "event loop stalled 226.3s", + #48445/#50005). On loopback there is no network/proxy path where a + half-open connection can occur — a dead local client tears the socket down + with a real FIN/RST that surfaces as WebSocketDisconnect regardless — so + the ping provides no liveness value and only harms. Assert it is disabled. + """ captured = _stub_uvicorn(monkeypatch) # Loopback bind => no auth gate, so this reaches the Config constructor. web_server.start_server(host="127.0.0.1", port=0, open_browser=False) + assert captured["ws_ping_interval"] is None + assert captured["ws_ping_timeout"] is None + + +def test_start_server_enables_ws_ping_for_half_open_detection(monkeypatch): + """Non-loopback (public) binds MUST keep the ws ping enabled so half-open + connections (reverse-proxy 524, dropped Cloudflare Tunnel) raise + WebSocketDisconnect into the reaping path (#32377). + + The invariant asserted here is that ping stays enabled (non-None, positive) + and the timeout is never shorter than the interval — not a frozen literal, + which churns every time the window is retuned. Loopback disables the ping + (see test_start_server_disables_ws_ping_on_loopback); this covers the + public-bind half-open case, so the auth gate is active here. + """ + captured = _stub_uvicorn(monkeypatch) + + # Non-loopback bind so the _is_loopback branch selects the enabled-ping + # window. Neutralize the auth gate so start_server reaches uvicorn.Config + # without requiring a registered provider (a real public bind would raise + # SystemExit here). The ping window keys off the host, not the auth flag. + monkeypatch.setattr(web_server, "should_require_auth", lambda *a, **k: False) + web_server.start_server(host="0.0.0.0", port=0, open_browser=False) + assert captured["ws_ping_interval"] and captured["ws_ping_interval"] > 0 assert captured["ws_ping_timeout"] and captured["ws_ping_timeout"] > 0 assert captured["ws_ping_timeout"] >= captured["ws_ping_interval"]