test(file_uploads): buffer uploads the router rejects before reading the body - #10411
Conversation
…the body it_fails_with_file_count_limits was flaky. The router rejects its 100-file request as soon as it reads the map (the limit is 5), replies, and closes the connection without reading the remaining file parts, so the kernel resets it. The test streamed the form part by part, so the client could still be writing when the reset arrived. reqwest then failed the request before the response was read, and the test failed with no snapshot diff. execute_multipart_request can now collect the form into a single body with a Content-Length before sending, and FileUploadTestServer exposes this as `buffered`. The count-limit test and the other tests the router rejects before the body is read (invalid multipart order, missing boundary, incompatible query order) opt in. Tests that rely on a streamed body keep streaming. The router's behaviour is unchanged.
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 5bca778ac4b0f1b6e8db6bae ✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
|
@BrynCooke, please consider creating a changeset entry in |
| /// Make a raw multipart request to the router. | ||
| /// | ||
| /// By default the form is streamed part by part. With `buffered`, it is collected first and | ||
| /// sent as a single body with a `Content-Length`, so the whole upload is written before the |
There was a problem hiding this comment.
This doc says buffering works because the upload "is written before the router can respond". The router can still respond after reading only the first parts. What matters is what happens next. Once the whole body has arrived, the router reads the rest of it and keeps the connection open, so there's no reset. A streamed body is still arriving, so the router closes the connection with data still on the way.
I checked this with a raw-socket client against a local router on macOS. Sent in one write, the 100-file upload left the connection reusable for a second request in 45 out of 45 runs. Sent part by part, the client's write failed and the connection was reset in 15 out of 15. When I held back even the last 100 bytes, the router closed the connection.
Suggested wording:
/// By default the form is streamed part by part. With `buffered`, it is collected first and
/// sent as a single body with a `Content-Length`. Use this when the router rejects the request
/// after reading only the first parts: if the rest of a streamed body is still arriving, the
/// router closes the connection and the client can hit a reset before it reads the response.
| ); | ||
|
|
||
| // Run the test | ||
| // Run the test. The router rejects this as soon as it reads the map, so buffer the |
There was a problem hiding this comment.
This comment repeats the explanation on execute_multipart_request. The other three tests that opt in have no comment, so a reader might wonder what is special here. The doc added to FileUploadTestServer::new (lines 1782-1784) repeats it a third time. I'd go back to // Run the test here, delete line 755, and cut the builder doc to a pointer:
/// Set `buffered` for requests the router rejects before reading the files. See
/// [IntegrationTest::execute_multipart_request].
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
|
This merged before the follow-up for the two inline wording suggestions was pushed. Those corrections are now in #10413. It fixes the |
2 participants
What it does
integration::file_upload::it_fails_with_file_count_limitsfails intermittently in CI. When it does, the output shows no snapshot diff. This PR makes the test send its upload as a single buffered body with aContent-Length, instead of streaming the form part by part. The same applies to the other file-upload tests that the router rejects before it reads the body: invalid multipart order, missing boundary and incompatible query order. Router code is unchanged, and so are the expected responses.The test harness gains an opt-in
bufferedoption.IntegrationTest::execute_multipart_requestcollects the form into one body before sending it, andFileUploadTestServer::builder().buffered(true)exposes this to tests. Without it, requests stream as before. Tests that depend on a streamed body, such as the mid-stream size limits, the large-file uploads and the slow-body timeouts, keep streaming.Why
The test uploads 100 empty files against
max_files: 5. The router reads theoperationsandmapparts and rejects the request before calling any subgraph. It returns the expectedFILE_UPLOADS_LIMITS_MAX_FILES_EXCEEDEDerror and closes the connection without reading the remaining file parts. Because the closed socket still has unread data, the kernel sends a TCP reset.The streamed form goes out as hundreds of small writes, so the client is often still uploading when the reset arrives. If a write fails, or the reset lands before reqwest has read the whole response, the harness either panics with "unable to send successful request to router" or fails while reading the body. That produces a failure with no snapshot diff. With a buffered body, the client has written the whole request before the router can respond, and then simply reads the response.
sequenceDiagram participant T as Test client participant R as Router T->>R: operations + map (100 files) R->>R: 100 > max_files 5, reject (no subgraph call) R-->>T: FILE_UPLOADS_LIMITS_MAX_FILES_EXCEEDED R->>R: close with unread file parts alt streamed body (before) T->>R: more file parts R-->>T: TCP reset T->>T: request may fail before the response is read else buffered body (after) T->>T: whole body already written, reads the response endThe alternative was to accept a connection error in this test and assert on the
file_uploads.limits.max_files.exceededmetric instead. That would stop the test checking the error body clients actually receive, so this PR keeps the exact snapshot assertion.Real clients that stream large uploads can still see a reset instead of this error. That router behaviour is unchanged here and would need a separate change, for example draining a bounded amount of the body after an early rejection.
Verification
60 consecutive runs of
it_fails_with_file_count_limits(nextest--stress-count 60) all passed with the CPU oversubscribed: 36 busy-loop processes on 18 cores. The original flake was not reproduced through the test before the change, so this run shows the change holds under load rather than reproducing the failure and its fix.