Shim: do not answer a client that already hung up

#5
by mjvk - opened

When a client gives up before its answer is ready (for example, its own deadline passed), the shim still finishes the decision and writes the answer to the closed connection. The write raises BrokenPipeError or ConnectionResetError. do_POST's catch-all treats that as a failed decision: it prints a traceback and then tries to send a 422 on the same closed connection. That second write fails too, and the standard library's server prints a second traceback.

Behind a caller with short deadlines this produces two or three tracebacks on stderr per abandoned request. Cloud logging platforms read those as errors, so they hide real failures.

The change: _send catches BrokenPipeError and ConnectionResetError, marks the connection to close, and returns. Nobody is left to answer, so no traceback is printed and no 422 follows. Every other error path stays as it is.

test_a_client_that_left_is_not_answered reproduces it. A client sends a request, waits until the decision starts, and closes the connection with a reset. The test checks that the shim prints no traceback. It fails on the current main with the two tracebacks above, and passes with the change. python3 test_shim.py passes in full.

Unrelated: ImageCalibrationTest.test_text_path_byte_identical fails now and then on the current main as well. It compares seen[0] and seen[1], which can arrive in either order.

mjvk changed pull request status to open

Thank you for the clear report and the fix. We've taken your change as written into v1.4.2: _send now treats BrokenPipeError / ConnectionResetError as a client that has already left, closes the connection, and prints nothing; every other error path is unchanged. Your test test_a_client_that_left_is_not_answered ships in test_shim.py: it fails on the v1.4.1 shim with the two tracebacks you describe and passes with the change, and clients that stay connected are answered byte for byte as before. The ordering flake in ImageCalibrationTest.test_text_path_byte_identical is fixed in v1.4.2 as well (it now compares the prompts sorted). The change lands with the v1.4.2 release commit, credited to you in the release notes; we'll post the link here and close this PR once v1.4.2 is tagged.

pseudotensor changed pull request status to closed

Thank you, mjvk: your fix shipped in v1.4.2 (https://huggingface.co/h2oai/h2o-lightning-4b/tree/v1.4.2). The shim no longer answers a client that has already hung up, and your test test_a_client_that_left_is_not_answered ships in test_shim.py, together with the fix for the ImageCalibrationTest ordering flake you reported. Closing this PR, since the change is now on main.

Sign up or log in to comment