fix(test): stabilize openai compat oversized-body regression - #839
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
zmanian
left a comment
There was a problem hiding this comment.
The stabilization approach (switching from a spawned TCP server to in-process oneshot) is sound and will eliminate the flaky 503 caused by server lifecycle non-determinism. The use of TestGatewayBuilder and direct Router construction is clean.
However, there is a correctness problem with the body limit value:
The test uses a 10 MB limit, but production uses 1 MB. The production server at src/channels/web/server.rs:354 applies DefaultBodyLimit::max(1024 * 1024) (1 MB), and the spec in src/channels/web/CLAUDE.md:200 confirms this. The test constructs its own router with DefaultBodyLimit::max(10 * 1024 * 1024), which means it is not testing the actual production configuration. It appears the old test had the same issue (the comment said "10 MB" and sent 11 MB), so this is a pre-existing bug being carried forward.
To actually regress against the production body limit, the test should use DefaultBodyLimit::max(1024 * 1024) (1 MB) and send a payload just over 1 MB (e.g., "x".repeat(1025 * 1024)). Alternatively, if 10 MB is the intended limit for the OpenAI-compat endpoint specifically, that should be reflected in the production router with a route-level override, and documented.
Everything else looks correct: auth middleware is wired, the handler and state types match production, and oneshot avoids all the TCP flakiness.
CLAUDE.md:200 still documented the pre-nearai#725 body limit of 1 MB, but server.rs:354 was changed to 10 MB in nearai#725 (image upload support). Update the documentation to match the actual production value.
|
Thank you for the prompt and thorough review! I dug into the source code and found that the discrepancy is actually in
Looking at the git history, #725 ( So the test's 10 MB limit + 11 MB payload is consistent with the actual production configuration. I've pushed a follow-up commit (af95b46) that fixes the stale documentation in |
zmanian
left a comment
There was a problem hiding this comment.
The previous review feedback has been fully addressed. The core concern was a perceived mismatch between the test's 10 MB body limit and production. The second commit (af95b46) correctly identifies that production (server.rs:354) was already updated to 10 MB in #725 for image upload support -- it was the CLAUDE.md documentation that was stale, not the test.
Review of changes:
-
Test stabilization (first commit): Switching from a spawned TCP server to in-process
oneshotis the right fix. The flaky 503 was caused by server lifecycle non-determinism where the LLM provider appeared unavailable. The new test constructs the router directly withTestGatewayBuilder, wires auth middleware, appliesDefaultBodyLimit::max(10 * 1024 * 1024)matching production, and usesoneshot-- no TCP, no sleeps, no flake vectors. -
Documentation fix (second commit):
CLAUDE.mdline 200 updated from "1 MB" to "10 MB" with a reference to #725. Matchesserver.rs:354exactly.
No new issues found. Test correctly sends 11 MB to exceed the 10 MB limit and asserts 413.
zmanian
left a comment
There was a problem hiding this comment.
The previous review feedback has been fully addressed. The core concern was a perceived mismatch between the test's 10 MB body limit and production. The second commit (af95b46) correctly identifies that production (server.rs:354) was already updated to 10 MB in #725 for image upload support -- it was the CLAUDE.md documentation that was stale, not the test.
Review of changes:
-
Test stabilization (first commit): Switching from a spawned TCP server to in-process oneshot is the right fix. The flaky 503 was caused by server lifecycle non-determinism where the LLM provider appeared unavailable. The new test constructs the router directly with TestGatewayBuilder, wires auth middleware, applies DefaultBodyLimit::max(10 * 1024 * 1024) matching production, and uses oneshot -- no TCP, no sleeps, no flake vectors.
-
Documentation fix (second commit): CLAUDE.md line 200 updated from 1 MB to 10 MB with a reference to #725. Matches server.rs:354 exactly.
No new issues found. Test correctly sends 11 MB to exceed the 10 MB limit and asserts 413.
) * fix(test): stabilize openai compat oversized-body regression * docs(web): fix stale body limit in CLAUDE.md (1 MB → 10 MB) CLAUDE.md:200 still documented the pre-nearai#725 body limit of 1 MB, but server.rs:354 was changed to 10 MB in nearai#725 (image upload support). Update the documentation to match the actual production value.
) * fix(test): stabilize openai compat oversized-body regression * docs(web): fix stale body limit in CLAUDE.md (1 MB → 10 MB) CLAUDE.md:200 still documented the pre-nearai#725 body limit of 1 MB, but server.rs:354 was changed to 10 MB in nearai#725 (image upload support). Update the documentation to match the actual production value.
Summary
Stabilize the oversized-body regression coverage for the OpenAI-compatible
/v1/chat/completionsendpoint.The previous integration test intermittently observed
503 Service Unavailableinstead of the expected413 Payload Too Large, even though the route-level body limit remained correctly configured. This change keeps the fix scoped to regression stability and coverage.Root Cause
test_chat_completions_body_too_largerelied on a spawned TCP gateway server inside theopenai_compat_integrationtest binary. That made the assertion sensitive to non-deterministic gateway lifecycle behavior in the surrounding integration process.The flaky failure path returned
503, which matches the OpenAI-compatible handler path wherellm_provideris unavailable, instead of the deterministic body-limit rejection path the test intended to verify.Changes
test_chat_completions_body_too_largeto validate the real OpenAI-compatible route, auth middleware, andDefaultBodyLimitin-process413Test Plan
cargo fmt --checkcargo clippy --all --benches --tests --examples --all-featurescargo test --test openai_compat_integration -- --test-threads=1 --nocapturecargo test --test openai_compat_integration -- --test-threads=1multiple times to confirm the flake no longer reproducescargo testpasses except for a pre-existing SIGSEGV intests/e2e_advanced_traces, which is unrelated to this change and reproducible on the upstream staging branchFeature Parity
FEATURE_PARITY.mdnot updated; no tracked capability behavior changed.