diff --git a/api/helpers.py b/api/helpers.py index 05fbd2f6..e9abdbc1 100644 --- a/api/helpers.py +++ b/api/helpers.py @@ -79,12 +79,22 @@ def _accepts_gzip(handler) -> bool: def _safe_write(handler, body: bytes) -> None: - """Write response body, silently ignoring client disconnect errors.""" + """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: - pass + 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: diff --git a/server.py b/server.py index 731fbfa8..74482266 100644 --- a/server.py +++ b/server.py @@ -311,19 +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: - if isinstance(e, _CLIENT_DISCONNECT_ERRORS): - return + except Exception: print(f'[webui] ERROR {self.command} {self.path}\n' + traceback.format_exc(), flush=True) try: j(self, {'error': 'Internal server error'}, status=500) - except Exception: + 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() @@ -347,19 +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: - if isinstance(e, _CLIENT_DISCONNECT_ERRORS): - return + except Exception: print(f'[webui] ERROR {self.command} {self.path}\n' + traceback.format_exc(), flush=True) try: j(self, {'error': 'Internal server error'}, status=500) - except Exception: + 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 index b03e3c57..a1e2c1b0 100644 --- a/tests/test_broken_pipe_cascade.py +++ b/tests/test_broken_pipe_cascade.py @@ -55,7 +55,7 @@ class MockHandler: class TestSafeWrite(unittest.TestCase): - """Test _safe_write swallows client disconnect errors silently.""" + """Test _safe_write swallows client disconnect errors without raising.""" def _make_handler(self, end_headers_raises=None, write_raises=None): handler = MockHandler( @@ -274,6 +274,119 @@ class TestServerDisconnectHandling(unittest.TestCase): # 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 + _server_mod.handle_get = _fake_handle_get + try: + Handler.do_GET(handler) + finally: + _server_mod.handle_get = orig_handle_get + + 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 + _server_mod.handle_get = _fake_handle_get + try: + Handler.do_GET(handler) + finally: + _server_mod.handle_get = orig_handle_get + + 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()