Skip to content

[Bugfix][Router] Only buffer the proxied response body when it is consumed - #1104

Open
lfsun02 wants to merge 4 commits into
vllm-project:mainfrom
lfsun02:fix-router-response-buffering
Open

lfsun02 wants to merge 4 commits into
vllm-project:mainfrom
lfsun02:fix-router-response-buffering

Conversation

@lfsun02

@lfsun02 lfsun02 commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Summary

process_request accumulated every proxied response body into a bytearray, including streaming responses where nothing on the default path reads it. The router therefore held a full copy of every in-flight stream until the stream ended, so router memory grew with (concurrent streams × response size).

The loop already guarded the append with if full_response is not None: but the buffer was initialized to bytearray() unconditionally and was never None.

Change

full_response is now allocated only when a consumer exists:

  • Token accounting (non-streaming only)
  • Semantic cache (the streaming path passes the last chunk, not the accumulated body, so when it is streaming we don't need to buffer full response)
  • post_request custom callback

So the body is buffered for every non-streaming response, and for streaming responses only when a post_request callback is configured and background_tasks is available to run it.

Tests

  • pre-commit checks pass
  • unit tests pass
  • manual e2e tests for sanity check

  • Make sure the code changes pass the pre-commit checks.
  • Sign-off your commit by using -s when doing git commit
  • Try to classify PRs for easy understanding of the type of changes, such as [Bugfix], [Feat], and [CI].
Detailed Checklist (Click to Expand)

Thank you for your contribution to production-stack! Before submitting the pull request, please ensure the PR meets the following criteria. This helps us maintain the code quality and improve the efficiency of the review process.

PR Title and Classification

Please try to classify PRs for easy understanding of the type of changes. The PR title is prefixed appropriately to indicate the type of change. Please use one of the following:

  • [Bugfix] for bug fixes.
  • [CI/Build] for build or continuous integration improvements.
  • [Doc] for documentation fixes and improvements.
  • [Feat] for new features in the cluster (e.g., autoscaling, disaggregated prefill, etc.).
  • [Router] for changes to the vllm_router (e.g., routing algorithm, router observability, etc.).
  • [Misc] for PRs that do not fit the above categories. Please use this sparingly.

Note: If the PR spans more than one category, please include all relevant prefixes.

Code Quality

The PR need to meet the following code quality standards:

  • Pass all linter checks. Please use pre-commit to format your code. See README.md for installation.
  • The code need to be well-documented to ensure future contributors can easily understand the code.
  • Please include sufficient tests to ensure the change is stay correct and robust. This includes both unit tests and integration tests.

DCO and Signed-off-by

When contributing changes to this project, you must agree to the DCO. Commits must include a Signed-off-by: header which certifies agreement with the terms of the DCO.

Using -s with git commit will automatically add this header.

What to Expect for the Reviews

We aim to address all PRs in a timely manner. If no one reviews your PR within 5 days, please @-mention one of YuhanLiu11
, Shaoting-Feng or ApostaC.

…streaming

Signed-off-by: Lifan Sun <lifansun1412@gmail.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request optimizes memory usage in the router by only accumulating the response body when it is actually needed, such as for non-streaming requests or when a post-request callback is configured. It also introduces a new test suite to verify response buffering behavior. The feedback recommends replacing direct calls to the __anext__() dunder method with the built-in anext() function in the test file for cleaner, more idiomatic Python code.

Comment thread src/tests/test_response_buffering.py Outdated
gen = process_request(
request, body, URL, "req-1", "/v1/chat/completions", background_tasks
)
await gen.__anext__() # headers and status

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Use the built-in anext() function instead of calling the dunder method __anext__() directly. This is more idiomatic in Python 3.10+ and maintains consistency with the rest of the codebase.

Suggested change
await gen.__anext__() # headers and status
await anext(gen) # headers and status

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

taken. will fix in next revision

Comment thread src/tests/test_response_buffering.py Outdated
tracemalloc.start()
try:
gen = process_request(request, body, URL, "req-1", "/v1/chat/completions", None)
await gen.__anext__()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Use the built-in anext() function instead of calling the dunder method __anext__() directly. This is more idiomatic in Python 3.10+ and maintains consistency with the rest of the codebase.

Suggested change
await gen.__anext__()
await anext(gen)

Signed-off-by: Lifan Sun <lifansun1412@gmail.com>

@chrikrah chrikrah left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lfsun02 I would merge this once the anext() revision you mentioned is in, since that touches only the tests. At 2374d04 I reverted request.py to 691dffa with your tests kept, and mutated each half of the new allocation condition in turn. All three break a test:

$ PYTHONPATH=src python -m pytest src/tests/test_response_buffering.py -q
3 passed                  # at 2374d04
1 failed, 2 passed        # request.py reverted: retained 4621438 bytes, limit 1048576
1 failed, 2 passed        # condition cut to `not is_streaming`
1 failed, 2 passed        # condition cut to `body_consumed_by_callback`
# not run: the rest of src/tests, and no live vLLM backend

The three red checks are the self-hosted runner's minikube (Kubernetes cluster unreachable, No such container: minikube), not this change, so your next push should re-run them.

When that revision is up, I will re-run the three mutations on the new head.

@lfsun02

lfsun02 commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

@lfsun02 I would merge this once the anext() revision you mentioned is in, since that touches only the tests. At 2374d04 I reverted request.py to 691dffa with your tests kept, and mutated each half of the new allocation condition in turn. All three break a test:

$ PYTHONPATH=src python -m pytest src/tests/test_response_buffering.py -q
3 passed                  # at 2374d04
1 failed, 2 passed        # request.py reverted: retained 4621438 bytes, limit 1048576
1 failed, 2 passed        # condition cut to `not is_streaming`
1 failed, 2 passed        # condition cut to `body_consumed_by_callback`
# not run: the rest of src/tests, and no live vLLM backend

The three red checks are the self-hosted runner's minikube (Kubernetes cluster unreachable, No such container: minikube), not this change, so your next push should re-run them.

When that revision is up, I will re-run the three mutations on the new head.

thanks for your review. the revision is already in 2374d04

@lfsun02

lfsun02 commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

@ruizhang0101 gently bumping up. the failing check is due to timeout, not relevant to the change

@lfsun02

lfsun02 commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor Author

looks like there are some infra issues. in the logs it shows the vLLM backend pods were repeatedly OOMKilled on the self-hosted runner, so every routing test failed at model listing before any inference request reached the router.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants