diff --git a/CHANGELOG.md b/CHANGELOG.md index a1748568..9feb92f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,8 +3,13 @@ ## [Unreleased] +## [v0.51.182] — 2026-05-31 — Release FB (stage-batchB2 — headless browser smoke gate + consolidated client-disconnect handling) + ### Added -- CI now runs a headless browser smoke test (`tests/browser_smoke.py`, `.github/workflows/browser-smoke.yml`) on every PR and push: it boots the real server agent-free and loads the key pages in Chromium, failing on any console error or uncaught JS exception. This catches the runtime-JS brick class (e.g. a `const` reassigned at runtime as in #3162, or a `function`/`window` name collision as in #2715/#2771) that `node --check`, ESLint, and the mocked test suite cannot see because they only manifest when a real browser executes the page. The smoke is credential-free — it strips `*_API_KEY` from the environment and drives no real model. +- CI now runs a headless browser smoke test (`tests/browser_smoke.py`, `.github/workflows/browser-smoke.yml`) on every PR and push: it boots the real server agent-free and loads the key pages in Chromium, failing on any console error or uncaught JS exception. This catches the runtime-JS brick class (e.g. a `const` reassigned at runtime as in #3162, or a `function`/`window` name collision as in #2715/#2771) that `node --check`, ESLint, and the mocked test suite cannot see because they only manifest when a real browser executes the page. The smoke is credential-free — it strips `*_API_KEY` from the environment and drives no real model (#3231). + +### Fixed +- Client disconnects during response writes (browser tab close, SSE reconnect races, mobile network switches, half-closed sockets) are now handled through a single `_CLIENT_DISCONNECT_ERRORS` set and a `_safe_write` helper instead of ad-hoc per-call-site `try/except`, so an expected disconnect no longer surfaces as a misleading server 500 in the logs. The error-response path is itself wrapped so a disconnect while sending a 500 is swallowed quietly rather than cascading. A bare `TimeoutError` from the Joplin notes integration's `urlopen` is now converted to a clean "not reachable" error at the route rather than escaping to the dispatch-level disconnect handler (#3210). ## [v0.51.181] — 2026-05-30 — Release FA (stage-batchA — agent-cache eviction teardown + streaming finalize race) diff --git a/api/helpers.py b/api/helpers.py index 0fe91a2e..e9abdbc1 100644 --- a/api/helpers.py +++ b/api/helpers.py @@ -4,10 +4,23 @@ Hermes Web UI -- HTTP helper functions. import json as _json import os import re as _re +import ssl from pathlib import Path from api.config import IMAGE_EXTS, MD_EXTS +# Treat stalled/closed HTTP clients as normal disconnects. Long-lived SSE +# connections often end this way when a browser tab sleeps, a phone switches +# networks, or Tailscale leaves the socket half-closed. +_CLIENT_DISCONNECT_ERRORS = ( + BrokenPipeError, + ConnectionResetError, + ConnectionAbortedError, + TimeoutError, + ssl.SSLError, +) + + def require(body: dict, *fields) -> None: """Phase D: Validate required fields. Raises ValueError with clean message.""" missing = [f for f in fields if not body.get(f) and body.get(f) != 0] @@ -65,6 +78,25 @@ def _accepts_gzip(handler) -> bool: return 'gzip' in ae +def _safe_write(handler, body: bytes) -> None: + """Write response body, ignoring expected client disconnect errors. + + Logs disconnects at debug level so they are observable without + polluting stdout/stderr during normal operation (SSE reconnects, + tab closes, mobile network switches, etc.). + """ + try: + handler.end_headers() + handler.wfile.write(body) + except _CLIENT_DISCONNECT_ERRORS as exc: + import logging + logging.getLogger("hermes.webui").debug( + "Client disconnected mid-response (%s): %s", + type(exc).__name__, + getattr(handler, "path", "?"), + ) + + def j(handler, payload, status: int=200, extra_headers: dict=None) -> None: """Send a JSON response. @@ -89,8 +121,7 @@ def j(handler, payload, status: int=200, extra_headers: dict=None) -> None: if extra_headers: for k, v in extra_headers.items(): handler.send_header(k, v) - handler.end_headers() - handler.wfile.write(body) + _safe_write(handler, body) def t(handler, payload, status: int=200, content_type: str='text/plain; charset=utf-8') -> None: @@ -101,8 +132,7 @@ def t(handler, payload, status: int=200, content_type: str='text/plain; charset= handler.send_header('Content-Length', str(len(body))) handler.send_header('Cache-Control', 'no-store') _security_headers(handler) - handler.end_headers() - handler.wfile.write(body) + _safe_write(handler, body) MAX_BODY_BYTES = 20 * 1024 * 1024 # 20MB limit for non-upload POST bodies diff --git a/api/routes.py b/api/routes.py index 66d237f7..74d15740 100644 --- a/api/routes.py +++ b/api/routes.py @@ -40,19 +40,6 @@ from api.session_events import ( logger = logging.getLogger(__name__) -# Treat stalled/closed HTTP clients as normal disconnects. Long-lived SSE -# connections often end this way when a browser tab sleeps, a phone switches -# networks, or Tailscale leaves the socket half-closed. If these bubble to the -# request handler, the server logs 500s and can leave CLOSE-WAIT sockets around -# until the OS-level timeout fires. -_CLIENT_DISCONNECT_ERRORS = ( - BrokenPipeError, - ConnectionResetError, - ConnectionAbortedError, - TimeoutError, - OSError, -) - # ── Cron run tracking ──────────────────────────────────────────────────────── # Track job IDs currently being executed so the frontend can poll status. _RUNNING_CRON_JOBS: dict[str, float] = {} # job_id → start_timestamp @@ -1054,6 +1041,7 @@ from api.helpers import ( _sanitize_error, redact_session_data, _redact_text, + _CLIENT_DISCONNECT_ERRORS, ) from api.agent_health import build_agent_health_payload from api.gateway_chat import gateway_chat_config_status @@ -13288,7 +13276,13 @@ def _joplin_api_get(path: str, params: dict | None = None) -> dict: raw = response.read(2_000_000).decode("utf-8", errors="replace") except HTTPError as exc: raise ValueError(f"Joplin API returned HTTP {exc.code}") from None - except URLError as exc: + except (URLError, TimeoutError) as exc: + # A bare socket-connect TimeoutError from urlopen(timeout=8) is NOT + # always URLError-wrapped, so catch it explicitly here. Otherwise it + # propagates past _handle_notes_search's `except ValueError` to the + # request dispatch, where the consolidated client-disconnect handler + # (#3210) would swallow it as a fake disconnect — silent empty response, + # no log. Convert it to a normal "not reachable" ValueError instead. raise ValueError("Joplin API is not reachable") from None try: data = json.loads(raw) diff --git a/server.py b/server.py index 9f481799..74482266 100644 --- a/server.py +++ b/server.py @@ -164,7 +164,7 @@ def _build_csp_report_only_policy() -> str: from api.auth import check_auth from api.config import HOST, PORT, STATE_DIR, SESSION_DIR, DEFAULT_WORKSPACE -from api.helpers import j, get_profile_cookie +from api.helpers import j, get_profile_cookie, _CLIENT_DISCONNECT_ERRORS from api.profiles import set_request_profile, clear_request_profile from api.routes import handle_delete, handle_get, handle_patch, handle_post, handle_put from api.startup import auto_install_agent_deps, fix_credential_permissions @@ -311,14 +311,22 @@ class Handler(BaseHTTPRequestHandler): result = handle_get(self, parsed) if result is False: return j(self, {'error': 'not found'}, status=404) - except (BrokenPipeError, ConnectionResetError, ConnectionAbortedError): + except _CLIENT_DISCONNECT_ERRORS: # The browser/client closed the socket while we were writing the # response. This is expected for probes, tab closes, and SSE # reconnect races; do not convert it into a misleading server 500. return - except Exception as e: + except Exception: print(f'[webui] ERROR {self.command} {self.path}\n' + traceback.format_exc(), flush=True) - return j(self, {'error': 'Internal server error'}, status=500) + try: + j(self, {'error': 'Internal server error'}, status=500) + except _CLIENT_DISCONNECT_ERRORS: + # Client disconnected while we were sending the 500 — nothing to do. + pass + except Exception: + # Unexpected failure while sending the error response itself. + # Log it so we know something is wrong with our error handler. + traceback.print_exc() finally: clear_request_profile() @@ -342,14 +350,22 @@ class Handler(BaseHTTPRequestHandler): result = route_func(self, parsed) if result is False: return j(self, {'error': 'not found'}, status=404) - except (BrokenPipeError, ConnectionResetError, ConnectionAbortedError): + except _CLIENT_DISCONNECT_ERRORS: # The browser/client closed the socket while we were writing the # response. This is expected for probes, tab closes, and SSE # reconnect races; do not convert it into a misleading server 500. return - except Exception as e: + except Exception: print(f'[webui] ERROR {self.command} {self.path}\n' + traceback.format_exc(), flush=True) - return j(self, {'error': 'Internal server error'}, status=500) + try: + j(self, {'error': 'Internal server error'}, status=500) + except _CLIENT_DISCONNECT_ERRORS: + # Client disconnected while we were sending the 500 — nothing to do. + pass + except Exception: + # Unexpected failure while sending the error response itself. + # Log it so we know something is wrong with our error handler. + traceback.print_exc() finally: clear_request_profile() diff --git a/tests/test_broken_pipe_cascade.py b/tests/test_broken_pipe_cascade.py new file mode 100644 index 00000000..88a09ac7 --- /dev/null +++ b/tests/test_broken_pipe_cascade.py @@ -0,0 +1,401 @@ +""" +Test for BrokenPipeError + SSL BAD_LENGTH cascading failure fix. + +When a client disconnects mid-response: +1. First write raises BrokenPipeError (or ssl.SSLError on TLS) +2. Exception handler tries to send 500 JSON through same broken socket +3. Without the fix, this produces a second exception and noisy traceback + +The fix: +- helpers._safe_write() catches _CLIENT_DISCONNECT_ERRORS (including ssl.SSLError) +- server.py exception handlers wrap their 500-response j() calls in try/except +""" +import ssl +import unittest +from unittest.mock import MagicMock + +import sys +import os + +# Ensure project root is on path +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +from api.helpers import j, t, _safe_write, _CLIENT_DISCONNECT_ERRORS + + +class MockHandler: + """Minimal mock of BaseHTTPRequestHandler for testing response helpers.""" + + def __init__(self, write_raises=None, end_headers_raises=None): + self._write_raises = write_raises + self._end_headers_raises = end_headers_raises + self.headers = {} + self._wfile = MagicMock() + self._sent_headers = [] + self._response_status = None + + @property + def wfile(self): + return self._wfile + + def send_response(self, status): + self._response_status = status + + def send_header(self, key, value): + self._sent_headers.append((key, value)) + + def end_headers(self): + if self._end_headers_raises: + raise self._end_headers_raises + + def _wfile_write(self, data): + if self._write_raises: + raise self._write_raises + return len(data) + + +class TestSafeWrite(unittest.TestCase): + """Test _safe_write swallows client disconnect errors without raising.""" + + def _make_handler(self, end_headers_raises=None, write_raises=None): + handler = MockHandler( + end_headers_raises=end_headers_raises, + write_raises=write_raises, + ) + handler.wfile.write = handler._wfile_write + return handler + + def test_safe_write_success(self): + handler = self._make_handler() + _safe_write(handler, b"hello") + self.assertEqual(handler._response_status, None) # send_response not called by _safe_write + + def test_safe_write_broken_pipe(self): + handler = self._make_handler(write_raises=BrokenPipeError()) + # Should NOT raise + _safe_write(handler, b"hello") + + def test_safe_write_connection_reset(self): + handler = self._make_handler(write_raises=ConnectionResetError()) + _safe_write(handler, b"hello") + + def test_safe_write_connection_aborted(self): + handler = self._make_handler(write_raises=ConnectionAbortedError()) + _safe_write(handler, b"hello") + + def test_safe_write_timeout(self): + handler = self._make_handler(write_raises=TimeoutError()) + _safe_write(handler, b"hello") + + def test_safe_write_ssl_bad_length(self): + handler = self._make_handler( + write_raises=ssl.SSLError("[BAD_LENGTH] write failed") + ) + _safe_write(handler, b"hello") + + def test_safe_write_end_headers_broken_pipe(self): + handler = self._make_handler(end_headers_raises=BrokenPipeError()) + _safe_write(handler, b"hello") + + def test_safe_write_end_headers_ssl_error(self): + handler = self._make_handler( + end_headers_raises=ssl.SSLError("[BAD_LENGTH]") + ) + _safe_write(handler, b"hello") + + def test_safe_write_other_error_propagates(self): + handler = self._make_handler(write_raises=ValueError("unexpected")) + with self.assertRaises(ValueError): + _safe_write(handler, b"hello") + + +class TestJsonHelper(unittest.TestCase): + """Test j() helper with disconnect errors.""" + + def _make_handler(self, end_headers_raises=None, write_raises=None): + handler = MockHandler( + end_headers_raises=end_headers_raises, + write_raises=write_raises, + ) + handler.wfile.write = handler._wfile_write + return handler + + def test_j_success(self): + handler = self._make_handler() + j(handler, {"ok": True}, status=200) + self.assertEqual(handler._response_status, 200) + self.assertTrue(any(h[0] == "Content-Type" for h in handler._sent_headers)) + + def test_j_broken_pipe_on_write(self): + handler = self._make_handler(write_raises=BrokenPipeError()) + # Should NOT raise — headers sent, write fails silently + j(handler, {"ok": True}, status=200) + self.assertEqual(handler._response_status, 200) + + def test_j_ssl_error_on_write(self): + handler = self._make_handler( + write_raises=ssl.SSLError("[BAD_LENGTH] write failed") + ) + j(handler, {"ok": True}, status=200) + self.assertEqual(handler._response_status, 200) + + def test_j_broken_pipe_on_end_headers(self): + handler = self._make_handler(end_headers_raises=BrokenPipeError()) + j(handler, {"ok": True}, status=200) + self.assertEqual(handler._response_status, 200) + + +class TestTextHelper(unittest.TestCase): + """Test t() helper with disconnect errors.""" + + def _make_handler(self, end_headers_raises=None, write_raises=None): + handler = MockHandler( + end_headers_raises=end_headers_raises, + write_raises=write_raises, + ) + handler.wfile.write = handler._wfile_write + return handler + + def test_t_success(self): + handler = self._make_handler() + t(handler, "hello", status=200) + self.assertEqual(handler._response_status, 200) + + def test_t_broken_pipe(self): + handler = self._make_handler(write_raises=BrokenPipeError()) + t(handler, "hello", status=200) + self.assertEqual(handler._response_status, 200) + + def test_t_ssl_error(self): + handler = self._make_handler( + write_raises=ssl.SSLError("[BAD_LENGTH]") + ) + t(handler, "hello", status=200) + self.assertEqual(handler._response_status, 200) + + +class TestClientDisconnectErrorsTuple(unittest.TestCase): + """Verify _CLIENT_DISCONNECT_ERRORS includes all expected types.""" + + def test_includes_broken_pipe(self): + self.assertIn(BrokenPipeError, _CLIENT_DISCONNECT_ERRORS) + + def test_includes_connection_reset(self): + self.assertIn(ConnectionResetError, _CLIENT_DISCONNECT_ERRORS) + + def test_includes_connection_aborted(self): + self.assertIn(ConnectionAbortedError, _CLIENT_DISCONNECT_ERRORS) + + def test_includes_timeout(self): + self.assertIn(TimeoutError, _CLIENT_DISCONNECT_ERRORS) + + def test_includes_ssl_error(self): + self.assertIn(ssl.SSLError, _CLIENT_DISCONNECT_ERRORS) + + def test_excludes_broad_oserror(self): + """OSError is too broad — it masks real errors like file-not-found.""" + self.assertNotIn(OSError, _CLIENT_DISCONNECT_ERRORS) + + +class TestServerDisconnectHandling(unittest.TestCase): + """Test server.py skips 500 response when client disconnects.""" + + def _make_handler(self, route_raises=None): + """Build a Handler with mocked socket and route.""" + from server import Handler + handler = Handler.__new__(Handler) + handler.command = "GET" + handler.path = "/api/test" + handler._req_t0 = 0.0 + handler.headers = {} + handler.wfile = MagicMock() + handler.wfile.write = MagicMock() + handler.send_response = MagicMock() + handler.send_header = MagicMock() + handler.end_headers = MagicMock() + handler._route_raises = route_raises + return handler + + def test_do_get_skips_500_on_broken_pipe(self): + from server import Handler + handler = self._make_handler(route_raises=BrokenPipeError()) + + def _fake_handle_get(self, parsed): + raise self._route_raises + + # Patch handle_get to raise BrokenPipeError + import server as _server_mod + orig_handle_get = _server_mod.handle_get + orig_check_auth = _server_mod.check_auth + _server_mod.handle_get = _fake_handle_get + _server_mod.check_auth = lambda h, p: True + try: + Handler.do_GET(handler) + finally: + _server_mod.handle_get = orig_handle_get + _server_mod.check_auth = orig_check_auth + + # send_response should NEVER be called for the 500 — client is gone + handler.send_response.assert_not_called() + + def test_handle_write_skips_500_on_connection_reset(self): + from server import Handler + handler = self._make_handler(route_raises=ConnectionResetError()) + handler.command = "POST" + + def _fake_route(self, parsed): + raise self._route_raises + + import server as _server_mod + orig_check_auth = _server_mod.check_auth + _server_mod.check_auth = lambda h, p: True + try: + Handler._handle_write(handler, _fake_route) + finally: + _server_mod.check_auth = orig_check_auth + + handler.send_response.assert_not_called() + + def test_do_get_sends_500_on_real_error(self): + from server import Handler + handler = self._make_handler(route_raises=ValueError("real bug")) + + def _fake_handle_get(self, parsed): + raise self._route_raises + + import server as _server_mod + orig_handle_get = _server_mod.handle_get + orig_check_auth = _server_mod.check_auth + _server_mod.handle_get = _fake_handle_get + _server_mod.check_auth = lambda h, p: True + try: + Handler.do_GET(handler) + finally: + _server_mod.handle_get = orig_handle_get + _server_mod.check_auth = orig_check_auth + + # Should send 500 for real errors + handler.send_response.assert_called_once_with(500) + + def test_do_get_skips_500_on_ssl_disconnect(self): + """SSL disconnect during route handling should not trigger 500.""" + from server import Handler + handler = self._make_handler(route_raises=ssl.SSLError("[BAD_LENGTH]")) + + def _fake_handle_get(self, parsed): + raise self._route_raises + + import server as _server_mod + orig_handle_get = _server_mod.handle_get + orig_check_auth = _server_mod.check_auth + _server_mod.handle_get = _fake_handle_get + _server_mod.check_auth = lambda h, p: True + try: + Handler.do_GET(handler) + finally: + _server_mod.handle_get = orig_handle_get + _server_mod.check_auth = orig_check_auth + + handler.send_response.assert_not_called() + + def test_do_get_skips_500_on_timeout_disconnect(self): + """Timeout disconnect during route handling should not trigger 500.""" + from server import Handler + handler = self._make_handler(route_raises=TimeoutError()) + + def _fake_handle_get(self, parsed): + raise self._route_raises + + import server as _server_mod + orig_handle_get = _server_mod.handle_get + orig_check_auth = _server_mod.check_auth + _server_mod.handle_get = _fake_handle_get + _server_mod.check_auth = lambda h, p: True + try: + Handler.do_GET(handler) + finally: + _server_mod.handle_get = orig_handle_get + _server_mod.check_auth = orig_check_auth + + handler.send_response.assert_not_called() + + +class TestServer500ResponseSafety(unittest.TestCase): + """Test that 500-response failures are handled gracefully.""" + + def _make_handler(self, route_raises=None, json_write_raises=None): + """Build a Handler where the 500-response j() call may also fail.""" + from server import Handler + handler = Handler.__new__(Handler) + handler.command = "GET" + handler.path = "/api/test" + handler._req_t0 = 0.0 + handler.headers = {} + handler.wfile = MagicMock() + handler._json_write_raises = json_write_raises + + def _raising_write(data): + if handler._json_write_raises: + raise handler._json_write_raises + return len(data) + + handler.wfile.write = _raising_write + handler.send_response = MagicMock() + handler.send_header = MagicMock() + handler.end_headers = MagicMock() + handler._route_raises = route_raises + return handler + + def test_500_response_survives_client_disconnect(self): + """If the 500-response itself hits a disconnect, don't crash.""" + from server import Handler + handler = self._make_handler( + route_raises=ValueError("real bug"), + json_write_raises=BrokenPipeError(), + ) + + def _fake_handle_get(self, parsed): + raise self._route_raises + + import server as _server_mod + orig_handle_get = _server_mod.handle_get + orig_check_auth = _server_mod.check_auth + _server_mod.handle_get = _fake_handle_get + _server_mod.check_auth = lambda h, p: True + try: + Handler.do_GET(handler) + finally: + _server_mod.handle_get = orig_handle_get + _server_mod.check_auth = orig_check_auth + + # send_response WAS called (we tried to send 500), but write failed + handler.send_response.assert_called_once_with(500) + + def test_500_response_logs_unexpected_failure(self): + """If the 500-response fails for a NON-disconnect reason, log it.""" + from server import Handler + handler = self._make_handler( + route_raises=ValueError("real bug"), + json_write_raises=RuntimeError("json serializer exploded"), + ) + + def _fake_handle_get(self, parsed): + raise self._route_raises + + import server as _server_mod + orig_handle_get = _server_mod.handle_get + orig_check_auth = _server_mod.check_auth + _server_mod.handle_get = _fake_handle_get + _server_mod.check_auth = lambda h, p: True + try: + Handler.do_GET(handler) + finally: + _server_mod.handle_get = orig_handle_get + _server_mod.check_auth = orig_check_auth + + # send_response WAS called (we tried to send 500), but write failed + handler.send_response.assert_called_once_with(500) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_session_lost_response_regression.py b/tests/test_session_lost_response_regression.py index 9f8b63c7..f0d56291 100644 --- a/tests/test_session_lost_response_regression.py +++ b/tests/test_session_lost_response_regression.py @@ -326,7 +326,8 @@ def test_messages_js_names_browser_sse_disconnect_separately(): def test_server_treats_broken_pipe_as_client_disconnect_not_500(): server_py = (models.Path(__file__).parent.parent / "server.py").read_text(encoding="utf-8") - assert "except (BrokenPipeError, ConnectionResetError, ConnectionAbortedError):" in server_py + # server.py now uses the centralized _CLIENT_DISCONNECT_ERRORS tuple from api.helpers + assert "_CLIENT_DISCONNECT_ERRORS" in server_py assert "do not convert it into a misleading server 500" in server_py diff --git a/tests/test_webui_notes_sources.py b/tests/test_webui_notes_sources.py index bac05eb5..f70d4d13 100644 --- a/tests/test_webui_notes_sources.py +++ b/tests/test_webui_notes_sources.py @@ -138,6 +138,30 @@ def test_joplin_get_note_validates_id_and_truncates_body(monkeypatch): assert "Preview truncated" in note["body"] +def test_joplin_api_get_converts_bare_timeout_to_valueerror(monkeypatch): + """Regression (#3210 review): a bare socket TimeoutError from urlopen must + surface as a ValueError, not propagate. + + `urlopen(timeout=8)` can raise a bare ``TimeoutError`` (not URLError-wrapped) + on a slow/unreachable Joplin server. After the consolidated client-disconnect + handling landed, ``TimeoutError`` is in the request dispatch's + ``_CLIENT_DISCONNECT_ERRORS`` set — so if it escaped ``_joplin_api_get`` it + would be swallowed as a fake client disconnect (silent empty response, no + log). It must be converted to a "not reachable" ValueError at the route. + """ + import pytest + from api import routes + + def fake_urlopen(request, timeout): + raise TimeoutError("timed out") + + monkeypatch.setattr(routes, "_joplin_connection_from_config", lambda: ("http://127.0.0.1:41184", "secret-token")) + monkeypatch.setattr("urllib.request.urlopen", fake_urlopen) + + with pytest.raises(ValueError, match="not reachable"): + routes._joplin_api_get("/search", {"query": "hello world"}) + + def test_joplin_api_get_sends_header_and_query_token_for_clip_search_compat(monkeypatch): from api import routes