fix: eliminate silent failures in client disconnect handling
- api/helpers.py: _safe_write() now logs disconnects at debug level instead of silently passing. No more invisible errors. - server.py: Restructure exception handlers to catch _CLIENT_DISCONNECT_ERRORS first, then Exception. Remove the isinstance() filter inside except Exception (LBYL anti-pattern). The 500-response fallback now catches _CLIENT_DISCONNECT_ERRORS separately (expected) and logs unexpected failures via traceback.print_exc() instead of bare except Exception: pass. - tests/test_broken_pipe_cascade.py: Add coverage for SSL/Timeout disconnect routing and 500-response safety (both disconnect survival and unexpected error logging).
This commit is contained in:
Ed
@@ -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:
|
||||
|
||||
26
server.py
26
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()
|
||||
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user