Skip to content

Fix spurious client-side failures in agentic benchmark - #3296

Merged
wenxindongwork merged 2 commits into
mainfrom
worktree-agentic-bench-client-fixes
Jul 30, 2026
Merged

Fix spurious client-side failures in agentic benchmark#3296
wenxindongwork merged 2 commits into
mainfrom
worktree-agentic-bench-client-fixes

Conversation

@wenxindongwork

@wenxindongwork wenxindongwork commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Problem

Running scripts/vllm/benchmarking/agentic_benchmark/benchmark_agentic.py against a long-context model reports a small but persistent turn failure rate (0.05-0.5% in my runs) even though the server returns HTTP 200 for every single request. Both causes are aiohttp client defaults, not server behaviour:

  1. Default 5-minute timeout. aiohttp.ClientSession() applies ClientTimeout(total=5*60). Turns that legitimately take longer -- high concurrency with a large --output-len-max -- are aborted client-side. It surfaces as asyncio.TimeoutError, whose str() is the empty string, so "error": str(e) recorded the failure with no reason at all, making it undiagnosable from the report.
  2. Stale keep-alive connection reuse. The connection pool reuses a keep-alive connection that the server has already closed during an idle gap between turns, raising ServerDisconnectedError. This was the only residual failure mode once the timeout was fixed.

Fix

  • Add make_client_session() -- total=None (no cap on a single turn; sock_connect=30 retained) and TCPConnector(force_close=True) so a fresh connection is used per request. Used for both the health check and the benchmark session.
  • Record the exception type alongside its message, so an empty str(e) can never produce a blank reason.
  • Print a Failure Breakdown after the success-rate line, tallied by reason, so any future failure is self-identifying.

Result

On Qwen/Qwen3.5-397B-A17B-FP8 (TPU v7x-8, TP8 + attention-DP4, 65536 context), a concurrency sweep at 2/4/6/8/12/16 showed 99.79-99.97% success in every run, with every failure attributable to the two causes above. With this change the full run -- 50 groups at concurrency 16, 800 rollouts, 21,490 turns over 4.6 hours -- completed at 100.00% success, 0 failed.

Report excerpt showing the new breakdown format (synthetic failures):

Total Conversational Turns:4 (Success: 1, Failed: 3, Rate: 25.00%)
Failure Breakdown:
       2 x TimeoutError
       1 x ServerDisconnectedError: Server disconnected

Testing

  • yapf --diff clean on the changed file.
  • End-to-end run against a live vLLM server: health check OK, 4/4 turns, 100%.
  • print_report() exercised directly with synthetic failing stats to verify the breakdown renders and sorts by frequency.
  • Session config asserted at runtime (timeout.total is None, connector._force_close is True).
  • No change to any measured metric or to the request payload -- this only affects connection handling and error reporting.

Note on token counts

An earlier revision of this description quoted a total-tokens-processed figure for the validation run. That number came from the script's client-side token estimates, which are inaccurate in both directions -- the prompt count tokenizes the Python repr of the message list, and the completion count re-encodes detokenized text. #3298 fixes that by using the server-reported usage. The counts quoted above (rollouts, turns, success rate, duration) are unaffected by that issue, so they stand as-is.

#3298 touches the same function in this file, so whichever lands first will need a trivial rebase of the other.

Running benchmark_agentic.py against a long-context model reported a
0.05-0.5% turn failure rate even though the server returned HTTP 200 for
every single request. Both causes were aiohttp client defaults:

- ClientSession() applies a default ClientTimeout(total=5*60), which
  aborts turns that legitimately exceed 5 minutes at high concurrency
  with a large --output-len-max. It surfaces as asyncio.TimeoutError,
  whose str() is empty, so the recorded error message was blank and the
  failure had no attributable reason.
- Pooled keep-alive connections were reused after the server had already
  closed them during an idle gap between turns, raising
  ServerDisconnectedError.

Add make_client_session() with total=None and a force_close connector,
used for both the health check and the benchmark session. Also record
the exception type alongside its message, and print a failure breakdown
after the success rate so any future failure is self-identifying.

On Qwen3.5-397B-A17B-FP8 over 21,490 turns (50 groups, concurrency 16),
this takes the success rate from 99.79-99.97% to 100.00%.

Signed-off-by: wenxindongwork <wenxindong@google.com>
@github-actions

Copy link
Copy Markdown

Description

Start with a short description of what the PR does and how this is a change from
the past.

The rest of the description includes relevant details and context, examples:

  • why is this change being made,
  • the problem being solved and any relevant context,
  • why this is a good solution,
  • some information about the specific implementation,
  • shortcomings of the solution and possible future improvements.

If the change fixes a Github issue, please include a link, e.g.,:
FIXES: #123456

Tests

Please describe how you tested this change, and include any instructions and/or
commands to reproduce.

Checklist

Before submitting this PR, please make sure:

  • I have performed a self-review of my code.
  • I have necessary comments in my code, particularly in hard-to-understand areas.
  • I have made or will make corresponding changes to any relevant documentation.

Signed-off-by: wenxindongwork <wenxindong@google.com>
@wenxindongwork wenxindongwork added the ready ONLY add when PR is ready to merge/full CI is needed label Jul 30, 2026
@wenxindongwork
wenxindongwork force-pushed the worktree-agentic-bench-client-fixes branch from b96e1a1 to e006095 Compare July 30, 2026 17:47
@wenxindongwork
wenxindongwork marked this pull request as ready for review July 30, 2026 17:48

@sierraisland sierraisland left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@wenxindongwork
wenxindongwork enabled auto-merge (squash) July 30, 2026 18:11
@wenxindongwork
wenxindongwork requested a review from andytwigg July 30, 2026 18:15
@wenxindongwork
wenxindongwork merged commit 4629f24 into main Jul 30, 2026
51 checks passed
helloworld1 pushed a commit that referenced this pull request Aug 17, 2026
Signed-off-by: wenxindongwork <wenxindong@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants