test(frontend): stop the oversized-body test racing the server - #13704
Open
ayaangazali wants to merge 1 commit into
Open
test(frontend): stop the oversized-body test racing the server#13704ayaangazali wants to merge 1 commit into
ayaangazali wants to merge 1 commit into
Conversation
`test_oversized_body_returns_json_413` posts a 2 MiB body against a 1 MiB cap.
The server answers 413 and closes as soon as it has read one byte past the cap,
so the client is still writing into a socket whose peer has gone away. The
status arrives and then reading the body fails:
body must be JSON: reqwest::Error { kind: Decode,
source: hyper::Error(Body, "connection error") }
That is ordinary HTTP, not a server bug: a server may answer and close before
a request body is fully sent. The test was asserting a guarantee HTTP does not
give it for a large overshoot.
Overshoot the cap by 4 KiB instead of 1 MiB. The body is still oversized, the
assertions are unchanged, and the client finishes writing before the server
replies, so the response is readable every time.
Measured on macOS at a 1 MiB cap: the 2 MiB body failed 13 of 16 runs, the
4 KiB overshoot passed 24 of 24. The whole crate suite now passes cleanly
where it previously failed on most runs.
Linux CI does not surface this, so `rust-tests` was green throughout. The
comment says so, to stop a green CI run being read as evidence the race is
gone, and to stop the overshoot being enlarged again.
Signed-off-by: ayaangazali <ayaangazali.work@gmail.com>
Contributor
|
👋 Hi ayaangazali! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
Bot
added
external-contribution
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughChangesBody limit test
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to The change reduces the oversized-body test's request overshoot to avoid a client/server write-close race while preserving all response assertions. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
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
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.
Summary
test_oversized_body_returns_json_413posts a 2 MiB body against a 1 MiB cap. The server answers 413 and closes as soon as it has read one byte past the cap, so the client is still writing into a socket whose peer has gone away. The status arrives, and then reading the body fails:This is ordinary HTTP rather than a server bug. A server may answer and close before a request body is fully sent. The test was asserting a guarantee HTTP does not give it once the overshoot is large enough to outlive the socket buffers.
The fix is to stop racing: overshoot the cap by 4 KiB instead of 1 MiB. The body is still oversized, every assertion is unchanged (413,
application/json, the exact message namingDYN_HTTP_BODY_LIMIT_MB), and the client finishes writing before the server replies, so the response body is readable every time.I first reported this as a flake on #13624 and have been disclosing it in the validation notes of #13685, #13696, #13698 and #13699. It is worth fixing rather than continuing to annotate, because it makes
cargo test -p dynamo-llmred by default for a whole class of contributors, and a suite that is normally red stops being read.This is not a CI fix, and I do not want it read as one.
rust-testsis green on Linux and was green throughout; the race depends on socket buffering and does not surface there. The benefit is local development on macOS. The comment in the test says this explicitly, so a green CI run is not mistaken for evidence the race is gone, and so nobody enlarges the overshoot again.Validation
Measured rather than asserted, serialized so nothing else could interfere, at a 1 MiB cap on macOS arm64:
Whole-crate effect, which is the reason this matters:
cargo clippyandcargo fmt --all -- --checkclean.Not addressed here, deliberately: making the server deliver its 413 body reliably under a large overshoot would mean draining a request body that was just rejected for being too large. That is a cost decision for whoever owns that path, not something to smuggle into a test fix.
Summary by CodeRabbit