fix(serve): drain request body on reject so the client isn't RST mid-read - #36
Merged
Conversation
…read test_serve.py flaked intermittently under full-suite load — a DIFFERENT serve test failed each run with ConnectionResetError [Errno 54] on resp.read(), passing in isolation. Not a port-bind race (the port is already ephemeral and bind+listen happen in the constructor). The real cause is in serve.py's handler: do_POST replies 503 (mesh disabled) and 413 (oversized) WITHOUT reading the posted body; only the 202 and bad-JSON-400 paths read it first. A rejection that leaves bytes in the kernel receive buffer makes the OS send RST instead of FIN on close, so the client — which has a valid response status — gets its connection reset mid-body-read. That exactly matches which tests flaked: is_disabled (503) and rejects_oversized (413) did; rejects_non_json (400) never did, because it reads the body to parse it. This is a real server defect, not a test artifact: a live telemetry producer POSTing to a disabled endpoint hits the same reset, not a clean 503. Fix: - serve.py: a bounded, idempotent `_drain()` discards any unread body before every reject reply (wired through `_reply`, plus the wrong-path 404). Bounded to MESH_MAX_BODY + 64 KiB so a normal/slightly-oversized body drains fully (no RST) while the 413 DoS guard still holds for a pathological body. The path that reads the body sets `_body_consumed` so the drain is a no-op there (never blocks). - test_serve.py: also close the listening socket (`server_close()`) and join the serve_forever thread in finally — each call previously leaked both. Verified: 20/20 consecutive full-suite `pytest tools/tests/` runs clean (was ~1-in-6 failing). At the old rate, 20 clean by luck is ~4%.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the intermittent `test_serve.py` flake — and the real server defect underneath it.
Diagnosis (reproduced, not guessed)
Under full-suite `make test` load, a different serve test failed each run with `ConnectionResetError: [Errno 54] Connection reset by peer` on `resp.read()`; each passed in isolation. Caught the real traceback across 12 runs.
It is not a port-bind race — the port is already ephemeral (`0`) and bind+listen happen in the `ThreadingHTTPServer` constructor. The cause is in `serve.py`:
```
do_POST: 503 (mesh disabled) → _reply WITHOUT reading the body
413 (oversized) → _reply WITHOUT reading the body
400 (bad JSON) → reads the body first, THEN _reply
202 (accepted) → reads the body first
```
A rejection that leaves the posted body unread in the kernel receive buffer makes the OS send RST instead of FIN on close. The client — which already has a valid response status — then gets its connection reset mid body-read. That exactly matches which tests flaked: 503 and 413 did; 400-bad-JSON never did, because it reads the body to parse it.
This is a real server bug, not a test artifact. A live telemetry producer POSTing to a disabled endpoint hits the same intermittent reset instead of a clean 503.
Fix
serve.py: a bounded, idempotent_drain()discards any unread body before every reject reply (wired through_reply, plus the wrong-path 404). Bounded toMESH_MAX_BODY + 64 KiBso a normal or slightly-oversized body drains fully (no reset) while the 413 DoS guard still holds for a pathological body; the path that reads the body sets_body_consumedso the drain is a no-op there and never blocks.tools/tests/test_serve.py: alsoserver_close()the listening socket andjoin()theserve_foreverthread infinally— each call previously leaked both.Verification
20/20 consecutive full-suite
pytest tools/tests/runs clean (was ~1-in-6 failing). At the old rate, 20 clean by luck is ~4%.