fix: contain ALL post-commit body errors, not just disconnects
The post-commit body-transmission block only caught _CLIENT_DISCONNECT_ERRORS. A non-disconnect error during body write — a generic OSError/EIO from f.read(), or a PermissionError — escaped _serve_file_bytes entirely, and Handler.do_GET (server.py:388-395) caught it and emitted a second status line after the already-committed 200/206, corrupting the HTTP stream. Broaden the post-commit except to swallow every Exception (keeping the disconnect branch for its debug message). Regression tests drive both the snapshot and streaming paths with generic OSError/EIO and PermissionError mid-body, asserting the status sequence is exactly [200] with no trailing 500.
This commit is contained in:
1 parent
1ff002d170
commit
8228dbe4da
2 files changed
+108
No files matched your search
@@ -18631,6 +18631,20 @@ def _serve_file_bytes(handler, target: Path, mime: str, disposition: str, cache_
|
||||
type(exc).__name__,
|
||||
getattr(handler, "path", "?"),
|
||||
)
|
||||
except Exception as exc:
|
||||
# Post-commit fail-closed: the status line is already on the
|
||||
# wire, so ANY body-transmission error that is not a client
|
||||
# disconnect (EIO from a truncated read, a generic OSError,
|
||||
# PermissionError, ...) must still be contained here. Letting
|
||||
# it escape would reach Handler.do_GET's 500 path and emit a
|
||||
# SECOND status line after the committed 200/206, corrupting
|
||||
# the HTTP stream. Log at debug and stop — the client already
|
||||
# received its headers (and possibly a partial body).
|
||||
logging.getLogger("hermes.webui").debug(
|
||||
"Body transmission error after commit (%s): %s",
|
||||
type(exc).__name__,
|
||||
getattr(handler, "path", "?"),
|
||||
)
|
||||
return True
|
||||
finally:
|
||||
_close_fd_quietly(fd)
|
||||
|
||||
@@ -565,3 +565,97 @@ def test_disconnect_mid_body_stream_no_second_response(routes, tmp_path):
|
||||
)
|
||||
assert handler.headers_ended
|
||||
assert result is True
|
||||
|
||||
|
||||
# ── Post-commit body transmission: NON-disconnect errors must not escape ──
|
||||
|
||||
class _PostCommitErrorHandler(_FakeHandler):
|
||||
"""Fake handler whose writer raises a NON-disconnect error (a generic
|
||||
OSError/EIO from a truncated read, a PermissionError, ...) once the
|
||||
response is committed. These are NOT in _CLIENT_DISCONNECT_ERRORS, so
|
||||
they are only contained by the broad post-commit except — if it escapes,
|
||||
Handler.do_GET appends a trailing 500 after the committed 200."""
|
||||
|
||||
def __init__(self, error, headers=None):
|
||||
super().__init__(headers)
|
||||
self.send_response_calls: list[int] = []
|
||||
self.headers_ended = False
|
||||
self._error = error
|
||||
|
||||
def send_response(self, code):
|
||||
self.send_response_calls.append(code)
|
||||
super().send_response(code)
|
||||
|
||||
def end_headers(self):
|
||||
self.headers_ended = True
|
||||
|
||||
def write(self, data):
|
||||
raise self._error
|
||||
|
||||
|
||||
def _serve_with_caller(routes, handler, target, mime, cache_control):
|
||||
"""Mirror server.py Handler.do_GET: any exception escaping
|
||||
_serve_file_bytes after commit becomes a second send_response(500).
|
||||
With the round-7 fix in place nothing escapes, so the recorded status
|
||||
sequence stays exactly [200]."""
|
||||
try:
|
||||
return routes._serve_file_bytes(
|
||||
handler, target, mime, "inline", cache_control
|
||||
)
|
||||
except Exception:
|
||||
handler.send_response(500)
|
||||
return False
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"error",
|
||||
[
|
||||
pytest.param(OSError(5, "Input/output error"), id="generic-OSError-EIO"),
|
||||
pytest.param(PermissionError(13, "Permission denied"), id="PermissionError"),
|
||||
],
|
||||
)
|
||||
def test_non_disconnect_error_mid_body_snapshot_no_second_response(routes, tmp_path, error):
|
||||
"""A non-disconnect error while the snapshot body is written must not
|
||||
escape to the caller's 500 path: exactly [200], never [200, 500]."""
|
||||
target = tmp_path / "img.png"
|
||||
target.write_bytes(b"payload")
|
||||
handler = _PostCommitErrorHandler(error)
|
||||
result = _serve_with_caller(
|
||||
routes, handler, target, "image/png", "private, no-cache"
|
||||
)
|
||||
|
||||
assert handler.status == 200
|
||||
assert handler.send_response_calls == [200], (
|
||||
f"exactly one status expected, got {handler.send_response_calls}"
|
||||
)
|
||||
assert handler.headers_ended
|
||||
assert result is True
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"error",
|
||||
[
|
||||
pytest.param(OSError(5, "Input/output error"), id="generic-OSError-EIO"),
|
||||
pytest.param(PermissionError(13, "Permission denied"), id="PermissionError"),
|
||||
],
|
||||
)
|
||||
def test_non_disconnect_error_mid_body_stream_no_second_response(routes, tmp_path, error):
|
||||
"""Same contract for the over-cap streaming path: a non-disconnect error
|
||||
mid-stream must not produce a trailing 500."""
|
||||
target = tmp_path / "big.bin"
|
||||
target.write_bytes(b"X" * (10 * 1024 * 1024 + 1)) # _ETAG_SIZE_CAP + 1
|
||||
handler = _PostCommitErrorHandler(error)
|
||||
result = _serve_with_caller(
|
||||
routes,
|
||||
handler,
|
||||
target,
|
||||
"application/octet-stream",
|
||||
"private, no-cache",
|
||||
)
|
||||
|
||||
assert handler.status == 200
|
||||
assert handler.send_response_calls == [200], (
|
||||
f"exactly one status expected, got {handler.send_response_calls}"
|
||||
)
|
||||
assert handler.headers_ended
|
||||
assert result is True
|
||||
Reference in new issue
Block a user