Conversation
Minh3132
left a comment
There was a problem hiding this comment.
On exact head b070b9174d5ed34b6a012b8b5f6eac05f7dfc90d, this changes a measured outcome into a deterministic expectation even though the probe itself does not control the condition that decides it.
The docstring immediately below these lines correctly says --h11-max-incomplete-event-size bounds only buffered incomplete data. But send() uses one socket.sendall(raw) and never controls how that write is split into data_received() calls on the server. If the complete header block reaches h11 before it needs more data, the incomplete-event ceiling is not the thing rejecting it and the request can reach HeaderLimits, which then returns 431; if the same bytes arrive fragmented such that h11 is left holding >16 KiB of an incomplete event, h11 rejects first with 400. That is the same transport-segmentation distinction the file already calls out for large request targets.
So a single run observing 64 KiB => 400 does not justify documenting single 64/256KB header value -> 400 as the expected shape. Either describe the 64 KiB band as segmentation-dependent (with 431 when it reaches the app and 400 when h11 trips first), or make the probe deliberately send controlled fragments and label the expectation for that fragmentation pattern. Otherwise this 'correction' can still report a healthy server as violating its own documented expected shape depending only on socket packetization.
You're right, and it goes further than I expected. I ran the unfragmented case 10 times against an unchanged server and got 431, 400, 400, 431, 400, 400, 400, 431, 400, 400 at 64KB. Same request, same sendall(), three different outcomes on identical bytes, so this isn't a segmentation edge case, it's the normal case at that size. 256KB, by contrast, came back 400 consistently across 10 trials, so that line stays deterministic. Pushed a follow-up commit splitting 64KB into its own segmentation-dependent line, phrased the same way the file already handles the 12/24KiB request-target case. Thanks for catching this. |
|
On current head |
Applied the segmentation-dependent framing to every case that's actually over the --h11-max-incomplete-event-size cap, not just the ones flagged: 16KB and 64KB headers, and 2000 headers, which also crosses the cap in total and reproduces the same 400/431 split under controlled fragmentation. The existing "12/24KB target -> may reach the app" line turned out to be hiding the identical issue, 12KB is genuinely under the cap and deterministic, 24KB is over it and segmentation-dependent the same way, so I split those into two accurate lines instead of one vague one. 256KB stayed consistently 400 under both plain and deliberately fragmented delivery across repeated trials, so it keeps a single line but stated as strong empirical evidence rather than a guarantee, since "far over the cap" doesn't by itself imply determinism (64KB is proof of that). I also discovered the request-line case (8/64KB) turned out to be a different, unrelated mechanism: it's the app's own room-name validator, not a parser length limit, confirmed by checking the actual response body (400 bad name '...', with the name echoed back) at both sizes, not just the status code. Every line in the table now traces to either a size argument (under the cap, so the failure mode is structurally impossible) or repeated trials under both plain and forced-fragmented delivery. Appreciate the catch. |
What
Rewrites the probe docstring's expected-shape table so every line matches measured behaviour at the documented flags. No caller-visible change; this is a comment in a diagnostic script.
Why
The table stated fixed codes for cases the probe can't make deterministic. Any request over --h11-max-incomplete-event-size (2000 headers, 16/64KB header values, a 24KB target) can return either code depending on read segmentation, which one sendall() doesn't control. Under-cap cases (50/200 headers, 8KB header, 12KB target) stay deterministic. Each line was checked with repeated plain and deliberately fragmented sends. The request-line row turned out to be the app's room-name validator, confirmed from the response body. Review from @Minh3132 and @yukkie3276 shaped this. Making the probe assert and exit non-zero is a separate follow-up.
Checks
uv run coverage run -m pytest tests -q && uv run coverage report— this file is excluded from pytest collection by design; unaffecteduv run ruff check . && uv run ruff format --check .anduv run ty checkREADME.md,src/patterns.md,mcp/README.md— no other doc states the specific 8/16/64KB breakdown