Hold the mock server's concurrency slot for the whole stream by theminjunchoi · Pull Request #1262 · vllm-project/guidellm

theminjunchoi · 2026-10-08T04:42:31Z

Summary

Fixes max_concurrent_requests for streaming requests. Right now the slot is released as soon as the handler returns its ResponseStream, before any tokens are generated, so streaming requests were never limited. This holds the slot until the stream actually finishes.

Details

In _run_generation, a streaming response's streaming_fn is wrapped so the slot is released when the stream ends, including when the client disconnects partway through. Non-streaming requests work the same as before.
Two new tests: overlapping streams now run one at a time, and a stream the client abandons still frees its slot so the next request doesn't hang.

Test Plan

lint-check, type-check, test-unit (3,658 passed), test-integration (32 passed) and test-e2e (14 passed) all pass.
The streaming test fails on main, and the disconnect test fails if the slot is only released after a stream completes normally.
Same repro as the issue, with this change: the second streaming request now waits for the slot (0.51s / 1.01s), same as non-streaming.

Related Issues

Resolves Mock server's max_concurrent_requests doesn't limit streaming requests #1261

"I certify that all code in this PR is my own, except as noted below."

Use of AI

Includes code generated or substantially modified by an AI agent
Includes tests generated or substantially modified by an AI agent

git log

commit 3cd601e
Author: MinJun Choi mj04300017@gmail.com
Date: Thu Oct 8 13:39:40 2026 +0900

Hold the mock server's concurrency slot for the whole stream

max_concurrent_requests is meant to cap in-flight generation requests, but for
streaming requests it didn't cap anything. The semaphore was only held while the
handler ran, and a streaming handler returns its ResponseStream right away.
Sanic runs the actual generation afterwards, so the slot was already free by
the time the first token went out.

Now the slot is released when the stream finishes instead, including when the
client disconnects partway through. Non-streaming requests behave as before.

Assisted-by: Claude Code claude-opus-5-5
Signed-off-by: MinJun Choi <mj04300017@gmail.com>

commit 416e962
Author: MinJun Choi mj04300017@gmail.com
Date: Thu Oct 8 13:39:40 2026 +0900

Test max_concurrent_requests with streaming requests

The existing test only covered non-streaming requests, which is how the
streaming gap went unnoticed. Add one test that overlapping streams run one at
a time, and one that a stream the client abandons still frees its slot, so the
next request doesn't hang.

Assisted-by: Claude Code claude-opus-5-5
Signed-off-by: MinJun Choi <mj04300017@gmail.com>

commit 9b3167e
Author: MinJun Choi mj04300017@gmail.com
Date: Fri Oct 9 22:31:23 2026 +0900

Compare stream finish times in the streaming concurrency test

The test passed when the slower of two overlapping streams took at least
0.3s. Without the fix the streams run together and the slower one took
about 0.26s, so a busy CI runner could push it past the bound and let the
test pass on a broken server.

Check the gap between the two finish times instead. Run together, they
finish within about a millisecond of each other; serialized, the second
finishes about a ttft later. Over ten runs each, the gap was 0.000-0.001s
without the fix and 0.205-0.220s with it, and a slower runner delays both
streams without closing the gap.

Assisted-by: Claude Code claude-opus-5-5
Signed-off-by: MinJun Choi <mj04300017@gmail.com>

commit 75e83c9
Author: MinJun Choi mj04300017@gmail.com
Date: Fri Oct 16 11:11:17 2026 +0900

Use httpx2 in the streaming concurrency tests

main moved the mock server tests from httpx to httpx2 in #1282, so the two new
tests no longer resolved httpx.AsyncClient.

Assisted-by: Claude Code claude-opus-5-5
Signed-off-by: MinJun Choi <mj04300017@gmail.com>

Assisted-by: Claude Code claude-opus-5-5
Signed-off-by: MinJun Choi mj04300017@gmail.com

HarshShroff

Ran this at 3061601 on macOS.

-k Concurrency in tests/unit/mock_server/test_server.py: 4 passed. With main's server.py put back, test_max_concurrent_requests_serializes_streaming fails (slowest stream 0.261s against the 0.3s bound) and the disconnect test passes, which is what you'd expect since main never holds the slot for streams.
Since the slot is now released by hand in two places, I wanted to check nothing leaks when clients bail at awkward moments. I started the mock server with max_concurrent_requests set, fired streaming requests from separate connections and cancelled each one after a random delay, then sent limit non-streaming requests at once with a 5s timeout. If any slot had leaked, at least one of those would hang. Results: limit 4 with 400 abandoned streams (cancel within 0-20ms), and limit 2 with 1,000 abandoned streams twice (cancel within 0-3ms and 0-8ms). Every follow-up finished each time, so I found no leak.

One small thing about the serialization test: main's unlimited run came in at 0.261s, only about 40ms below the 0.3s bound. On a busy CI runner the unlimited case could cross 0.3s, and then the test would pass even without the fix. I didn't time the branch side, but with the slot held the second stream can't start its 0.2s ttft until the first one ends, so it should take roughly 0.4s. A bound around 0.35s would leave more room on both sides.

theminjunchoi · 2026-10-09T13:29:43Z

@HarshShroff Thanks for testing this so thoroughly, especially the abandoned-stream runs. That's reassuring.

Good catch on the bound. Rather than raising it, I changed the test to compare when the two streams finish instead of how long the slowest one took. Without the fix they finish together, and with it the second one ends about a ttft later, so the gap doesn't depend on how fast the runner is.

theminjunchoi · 2026-10-14T07:14:54Z

Hi @jaredoconnell, sorry to ping you twice in one day! Since you've done a lot of the mock server work, I thought you'd be the right person for this one. It's a small fix so that max_concurrent_requests also holds for streaming requests (#1261), and @HarshShroff kindly tested it on macOS. Whenever you have a moment, I'd really appreciate a look. Thank you!

sjmonson

Sure, seems fine

mergify · 2026-10-14T20:46:46Z

Received a single approval. Will automatically queue for merge once two maintainers approve, all change requests are resolved, and DCO passes.

mergify · 2026-10-14T20:46:48Z

Queued — the merge queue status continues in this comment ↓.

dbutenhof

This looks good, at a layer of Sanic I hadn't looked at before. The only concern might be that streaming_fn is technically marked "deprecated" -- no idea whether they'll ever actually remove it, but in any case there's no point in expanding this PR to cover that much more extensive refactoring.

mergify · 2026-10-15T19:58:34Z

Merge Queue Status

✅ Entered queue — 2026-10-15 19:58 UTC · Rule: default · triggered by merge protections
❌ Checks failed · on draft merge queue: checking #1262 on main (899121d) #1318
🚫 Left the queue — 2026-10-15 20:06 UTC · at f51dd125c8e1f8a5a1a001b7fbcd94cfa09beed6

This pull request spent 7 minutes 54 seconds in the queue, including 7 minutes 11 seconds running CI.

Reason

The merge conditions cannot be satisfied due to failing checks

quality (3.10) / quality-checks

Failing checks:

❌ quality (3.10) / quality-checks (job log)

Hint

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Waiting for

All conditions

Requeued — the merge queue status continues in this comment ↓.

jaredoconnell

I tested it and it appears to behave as it should.

jaredoconnell · 2026-10-15T20:47:28Z

It appears that needs to be updated following this PR: https://github.com/vllm-project/guidellm/pull/1282/changes#diff-b9b431edd343d7d19efc5e36bde8365c1e357a5679a84d80c7c1733343013e9c

Main switched to a new http library.

max_concurrent_requests is meant to cap in-flight generation requests, but for streaming requests it didn't cap anything. The semaphore was only held while the handler ran, and a streaming handler returns its ResponseStream right away. Sanic runs the actual generation afterwards, so the slot was already free by the time the first token went out. Now the slot is released when the stream finishes instead, including when the client disconnects partway through. Non-streaming requests behave as before. Assisted-by: Claude Code claude-opus-5-5 Signed-off-by: MinJun Choi <mj04300017@gmail.com>

The existing test only covered non-streaming requests, which is how the streaming gap went unnoticed. Add one test that overlapping streams run one at a time, and one that a stream the client abandons still frees its slot, so the next request doesn't hang. Assisted-by: Claude Code claude-opus-5-5 Signed-off-by: MinJun Choi <mj04300017@gmail.com>

The test passed when the slower of two overlapping streams took at least 0.3s. Without the fix the streams run together and the slower one took about 0.26s, so a busy CI runner could push it past the bound and let the test pass on a broken server. Check the gap between the two finish times instead. Run together, they finish within about a millisecond of each other; serialized, the second finishes about a ttft later. Over ten runs each, the gap was 0.000-0.001s without the fix and 0.205-0.220s with it, and a slower runner delays both streams without closing the gap. Assisted-by: Claude Code claude-opus-5-5 Signed-off-by: MinJun Choi <mj04300017@gmail.com>

main moved the mock server tests from httpx to httpx2 in vllm-project#1282, so the two new tests no longer resolved httpx.AsyncClient. Assisted-by: Claude Code claude-opus-5-5 Signed-off-by: MinJun Choi <mj04300017@gmail.com>

theminjunchoi · 2026-10-16T02:14:43Z

Thanks for catching that, @jaredoconnell! I rebased onto main and switched the two new tests to httpx2, the same way #1282 updated the rest of the file. Lint, type-check and the unit tests pass locally now.

The push dismissed the earlier approvals, so sorry for the extra round. The only change since the reviews is the httpx → httpx2 swap in those two tests.

mergify · 2026-10-16T12:23:17Z

Received a single approval. Will automatically queue for merge once two maintainers approve, all change requests are resolved, and DCO passes.

mergify · 2026-10-16T16:02:33Z

Merge Queue Status

✅ Entered queue — 2026-10-16 16:02 UTC · Rule: default · triggered by merge protections
🚫 Left the queue — 2026-10-16 16:02 UTC · at 75e83c915b28dd09a94ed8bc9bccdfba576fadee

This pull request spent 32 seconds in the queue, with no time running CI.

Reason

The pull request conflicts with the base branch

Automatic conflict resolution can merge a conflict like this one instead of stopping on it, but only in a queue using the merge-batch or fast-forward merge method.

Hint

You should update or rebase your pull request.

If you want to requeue this pull request, you can post a @mergifyio queue comment.

Requeued — the merge queue status continues in this comment ↓.

Signed-off-by: MinJun Choi <mj04300017@gmail.com>

theminjunchoi · 2026-10-16T16:25:30Z

@sjmonson @dbutenhof Sorry for the extra round. The merge queue dropped this because of a conflict with main in tests/unit/mock_server/test_server.py. Both sides had added tests at the end of the file, so I merged main and kept both sets. The diff against main is the same as before. Would you mind re-approving when you get a chance? Thank you!

mergify · 2026-10-16T16:45:35Z

mergify · 2026-10-16T16:45:37Z

Received a single approval. Will automatically queue for merge once two maintainers approve, all change requests are resolved, and DCO passes.

theminjunchoi requested review from dbutenhof, dfeddema, jaredoconnell and sjmonson as code owners October 8, 2026 04:42

This was referenced Oct 8, 2026

Estimate the sweep's peak rate with Little's law #1248

Closed

Improve throughput upper limit detection #564

Open

HarshShroff reviewed Oct 9, 2026

View reviewed changes

theminjunchoi mentioned this pull request Oct 12, 2026

Add a sweep_next profile that finds the peak rate with a ramp #1295

Open

7 of 8 tasks

sjmonson approved these changes Oct 14, 2026

View reviewed changes

dbutenhof previously approved these changes Oct 15, 2026

View reviewed changes

mergify Bot added the queued label Oct 15, 2026

mergify Bot mentioned this pull request Oct 15, 2026

merge queue: checking #1262 on main (899121d) #1318

Closed

9 of 43 tasks

mergify Bot added dequeued and removed queued labels Oct 15, 2026

jaredoconnell previously approved these changes Oct 15, 2026

View reviewed changes

sjmonson previously approved these changes Oct 15, 2026

View reviewed changes

theminjunchoi added 4 commits October 16, 2026 11:06

Use httpx2 in the streaming concurrency tests

75e83c9

main moved the mock server tests from httpx to httpx2 in vllm-project#1282, so the two new tests no longer resolved httpx.AsyncClient. Assisted-by: Claude Code claude-opus-5-5 Signed-off-by: MinJun Choi <mj04300017@gmail.com>

theminjunchoi dismissed stale reviews from sjmonson, jaredoconnell, and dbutenhof via 75e83c9 October 16, 2026 02:11

theminjunchoi force-pushed the fix/mock-server-stream-concurrency branch from f51dd12 to 75e83c9 Compare October 16, 2026 02:11

mergify Bot removed the dequeued label Oct 16, 2026

theminjunchoi requested review from dbutenhof, jaredoconnell and sjmonson October 16, 2026 02:15

dbutenhof previously approved these changes Oct 16, 2026

View reviewed changes

sjmonson previously approved these changes Oct 16, 2026

View reviewed changes

mergify Bot added queued dequeued and removed queued labels Oct 16, 2026

Merge main into fix/mock-server-stream-concurrency

c0c95a1

Signed-off-by: MinJun Choi <mj04300017@gmail.com>

theminjunchoi dismissed stale reviews from sjmonson and dbutenhof via c0c95a1 October 16, 2026 16:24

mergify Bot removed the dequeued label Oct 16, 2026

theminjunchoi requested review from dbutenhof and sjmonson October 16, 2026 16:25

sjmonson approved these changes Oct 16, 2026

View reviewed changes

mergify Bot added the queued label Oct 16, 2026

mergify Bot merged commit 6168d28 into vllm-project:main Oct 16, 2026
9 checks passed

mergify Bot removed the queued label Oct 16, 2026

Conversation

theminjunchoi commented Oct 8, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

Test Plan

Related Issues

Use of AI

git log

Uh oh!

HarshShroff left a comment

Choose a reason for hiding this comment

Uh oh!

theminjunchoi commented Oct 9, 2026

Uh oh!

theminjunchoi commented Oct 14, 2026

Uh oh!

sjmonson left a comment

Choose a reason for hiding this comment

Uh oh!

mergify Bot commented Oct 14, 2026

Uh oh!

mergify Bot commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbutenhof left a comment

Choose a reason for hiding this comment

Uh oh!

mergify Bot commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Reason

Hint

Uh oh!

jaredoconnell left a comment

Choose a reason for hiding this comment

Uh oh!

jaredoconnell commented Oct 15, 2026

Uh oh!

theminjunchoi commented Oct 16, 2026

Uh oh!

mergify Bot commented Oct 16, 2026

Uh oh!

mergify Bot commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Reason

Hint

Uh oh!

theminjunchoi commented Oct 16, 2026

Uh oh!

mergify Bot commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Uh oh!

mergify Bot commented Oct 16, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

theminjunchoi commented Oct 8, 2026 •

edited by github-actions Bot

Loading

mergify Bot commented Oct 14, 2026 •

edited

Loading

mergify Bot commented Oct 15, 2026 •

edited

Loading

mergify Bot commented Oct 16, 2026 •

edited

Loading

mergify Bot commented Oct 16, 2026 •

edited

Loading