Skip to content

test(llm): check the transcription 413 in memory, not over TCP - #18578

Closed
jthomson04 wants to merge 1 commit into
mainfrom
jthomson04/fix-transcriptions-413-flake
Closed

jthomson04 wants to merge 1 commit into
mainfrom
jthomson04/fix-transcriptions-413-flake

Conversation

@jthomson04

@jthomson04 jthomson04 commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Overview

upload_too_large_for_the_request_plane_returns_payload_too_large flakes in Pre Merge rust-tests (.). It failed on three main pushes on 2026-11-10 and on #18553. The test posts a 24 MiB upload over TCP. The route stops reading at its 23 MiB limit and answers 413, then the server resets the connection while the client is still uploading. If the client's next body write hits the reset, hyper's client aborts the connection and drops the 413 body it had already received. response.json() then fails with hyper::Error(Body, "connection error"). This PR checks the same thing in memory against the real route, with no TCP connection.

Test Runs Failures
Old test, over TCP 200 47, all at the response.json() unwrap
New test, run with the tests that lower DYN_HTTP_BODY_LIMIT_MB 200 0
cargo test -p dynamo-llm --test http-transcriptions 100 0

Key Changes

  • Moved the test into the transcriptions.rs unit tests. It drives transcriptions_router with oneshot and makes the same assertions: a 24 MiB upload gets a 413 whose message names the transcription limit.
  • The test runs with DYN_HTTP_BODY_LIMIT_MB unset under temp_env::async_with_vars. temp_env's lock serializes it with the two service_v2 tests in the same binary that set that limit to 1 MiB.
  • Removed the TCP version from tests/http-transcriptions.rs.
  • Split the request half of parse_status into post_form, so both tests share it.

Validation

Root cause, from running the old test in a network namespace:

  • All 200 runs ended in a reset (TcpEstabResets 200). In 154 the server closed with unread request bytes (TCPAbortOnClose). In 46, request bytes arrived after the close (TCPAbortOnData).
  • In a failing run under strace, the client's last request-body writev returned EPIPE. Hyper's client dispatcher then fails the pending response body with "connection error" (proto/h1/dispatch.rs, poll_catch).
  • Hyper's server drains only what is already buffered before closing the read side (poll_drain_or_close_read), so every oversized upload ends this way.
cargo test -p dynamo-llm --lib http::service::transcriptions
cargo test -p dynamo-llm --test http-transcriptions        # looped 100x
cargo clippy -p dynamo-llm --no-deps --all-targets --keep-going -- -D warnings

Clippy is clean. Its only error is the existing model_eos compile error in streaming_tool_dispatch_http.

Issues and Ongoing Work

Pull Request Checklist
  • I used Dynamo's /pr-checklist skill.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Added coverage confirming that streamed transcription uploads exceeding the request limit receive an HTTP 413 response with an error message identifying the limit. This verifies that oversized requests are rejected even when their body does not include a Content-Length header.

upload_too_large_for_the_request_plane_returns_payload_too_large posted a
24 MiB upload over TCP. The route stops reading at its 23 MiB limit and
answers 413. Hyper drains only what is already buffered, so the server
resets the connection while the client is still uploading. When the
client's next body write hits the reset, hyper's client aborts the
connection and the 413 body it already received is lost. response.json()
then fails with hyper::Error(Body, "connection error"). The old test
failed 47 of 200 runs locally.

Move the check into the transcriptions unit tests. It drives the real
transcriptions_router with oneshot and asserts the same 413 naming the
transcription limit. It runs with DYN_HTTP_BODY_LIMIT_MB unset under
temp_env, whose lock serializes it with the service_v2 tests that lower
that limit in the same binary.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jthomson04
jthomson04 requested review from a team as code owners November 11, 2026 06:36
@github-actions github-actions Bot added test frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` labels Nov 11, 2026
@coderabbitai

coderabbitai Bot commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The transcription tests now share streamed multipart request construction. An in-memory router test checks the response to an upload that exceeds the transcription request-plane limit. The external test for the same limit was removed.

Changes

Transcription upload limit tests

Layer / File(s) Summary
Streamed upload limit test
lib/llm/src/http/service/transcriptions.rs, lib/llm/tests/http-transcriptions.rs
A shared helper submits streamed multipart requests without a Content-Length. The in-memory transcription-router test checks that a 24 MiB upload returns 413 with a message containing “transcription limit.” The external test for this assertion was removed.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: Minimal · up to bc22e

The upload-limit check now runs in memory. No merge-blocking risk was identified.

Pre-merge checks: Passed 5

Check name Status Explanation
Title check Passed The title clearly identifies the main change: moving the transcription 413 test from TCP to in-memory execution.
Description check Passed The description includes all required sections, explains the flaky TCP test, documents the in-memory replacement, lists validation results, and notes ongoing work. The unchecked checklist item and adv…
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description includes all required sections, explains the flaky TCP test, documents the in-memory replacement, lists validation results, and notes ongoing work. The unchecked checklist item and advisory issue placeholder do not prevent approval.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 57d7a1d and bc22e9c.

Files selected for processing: 2

  • lib/llm/src/http/service/transcriptions.rs
  • lib/llm/tests/http-transcriptions.rs
Files with no reviewable changes: 1

  • lib/llm/tests/http-transcriptions.rs

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

@devin-ai-integration

Copy link
Copy Markdown
Contributor

✅ Dynamo PR CI passed — run 42973670067 (attempt 1) on bc22e9c850

Gate checks: ✅ backend-status-check · ✅ deploy-status-check · ✅ dynamo-status-check

Posted automatically by Devin for run 42973670067. Updated on every full-CI run of this PR.

@bhargavchintam

Copy link
Copy Markdown

Your trace matches what I see on macOS, and it isn't only this test. service_v2::tests::test_oversized_body_returns_json_413 and test_batch_upload_limit_preserves_online_json_limit post an oversized body to /v1/responses over TCP. They fail the same way, with hyper::Error(Body, "connection error") after the 413. In 9 full local --lib runs, they failed 6 and 3 times.

#18175 explains why the body is lost. track_inflight_inference re-streams every response, so the 413 goes out chunked, and hyper hasn't handed over the whole body when the reset lands. #18177 keeps the size hint there. With only that change, the old transcription test failed 0 of 400 runs on my Mac, against 57 of 600 on main.

@jthomson04

Copy link
Copy Markdown
Contributor Author

Closing in favor of #18177. That PR keeps the response's Content-Length in track_inflight_inference, which is why the 413 body was being lost. With it applied to main, the TCP version of this test failed 0 of 200 runs locally, against 7 of 200 on main. It also covers the two service_v2 oversized-body tests and keeps the end-to-end check this PR would have deleted.

@jthomson04 jthomson04 closed this Nov 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` size/M test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants