Skip to content

fix: fail fast on endpoint response stalls - #462

Open
roborluo wants to merge 1 commit into
mlcommons:mainfrom
roborluo:fix-bofengl-no-progress-watchdog
Open

fix: fail fast on endpoint response stalls#462
roborluo wants to merge 1 commit into
mlcommons:mainfrom
roborluo:fix-bofengl-no-progress-watchdog

Conversation

@roborluo

@roborluo roborluo commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • add an opt-in no-progress deadline for in-flight endpoint requests
  • expose it as settings.timeouts.no_progress_timeout_s plus a --no-progress-timeout CLI alias
  • arm the guard only while work is in flight; retire it as soon as the phase drains

When this is useful

Use this for automated runs where a request can be accepted but the endpoint then becomes silent — for example, a TensorRT-LLM disaggregated executor or KV-transfer stall that never reaches the normal terminal-error path. It is engine-agnostic: it catches the same client-visible silent failure through vLLM, a frontend, or transport. Without it the benchmark blocks until its outer wall-time limit, because the phase drain waits on responses that never arrive and drain_timeout defaults to unlimited.

Disabled by default. It starts after work is issued, runs only while requests are in flight, and resets on an observed stream chunk or final result. It does not diagnose or restart the backend — it makes the benchmark fail with a clear error. For non-streaming endpoints, configure it above the full expected request latency. For TensorRT-LLM disaggregated serving the documented starting value is 300 s, matching the executor hang_detection_timeout.

Changes since the last review

Rebased onto latest main and moved the setting into the Timeouts model added by #409 (@viraatc).

Review point Change
[P1] completed_uuids retained every completed UUID Only register_skipped adds tombstones now, so it no longer grows O(total requests) alongside uuid_to_index
[P2] disabled path did work per response Gated on the feature being enabled, and the receiver reuses the arrival timestamp it already takes instead of reading the clock twice
[P2] task + timer per 0→1 transition Single self-rearming loop.call_later TimerHandle — no task, no Event, no per-iteration wait_for
[P2] full-timeout sleep could detect silence ~2x late Remaining deadline is computed from the last progress stamp
cohort wording Removed from the docs and from the local variable in issue()
help/description drift Schema description is the short form; docs/config/DESIGN.md holds the tuning detail

Measured cost when disabled (A/B against the base commit): +3.5 ns per response, +9.4 ns per request. With stream_all_chunks: false (the default) the main process sees 2 messages per request, not one per token.

Measured cost of the arming change under the production eager task factory: arm+cancel per request went from 2205 ns to 535 ns (4.1x).

Also fixed while here

A receiver transport error could overwrite an earlier NoProgressError. A stalled endpoint often drops its connection too, so the real diagnosis was being masked by a generic "receiver failed" message. First error now wins, with a regression test.

Not adopted

The suggestion to use one phase/session-lifetime watchdog driven by an activity event. The guard is still armed on the 0→1 in-flight transition and retired on drain — now with a TimerHandle rather than a task. This keeps the timer heap clear once a phase drains, and measured 4.1x cheaper than the reviewed version. Happy to switch to a resident watchdog if that is preferred.

Validation

  • pytest tests/unit/config/test_schema.py tests/unit/commands/test_benchmark.py tests/unit/load_generator/test_async_session.py425 passed
  • pre-commit run --all-files — all hooks pass except mypy, which reports 3 pre-existing os.sched_*affinity errors on macOS in cpu_affinity.py / token_metrics.py; neither file is touched by this PR and the symbols exist on Linux
  • python scripts/regenerate_templates.py --check
  • AGA disaggregated held-response canary, job 578946: failed as intended with Endpoint made no response progress for 10.0s with 1 request(s) in flight, 10.002 s after the phase started against a 10 s deadline. The 10 s value is fault-injection coverage, not the deployment recommendation.

Timer lifecycle is covered in both directions: the perf issue cap (stop_current_phase) deliberately leaves the guard armed so a stall during drain is still caught, while drain completion, stop(), a phase change, and session teardown each retire it.

@roborluo
roborluo requested a review from a team August 22, 2026 17:29
@github-actions

Copy link
Copy Markdown

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@github-actions github-actions Bot added the size/normal PR Review Policy: <=500 non-test lines & <=20 files label Aug 22, 2026
@codecov-commenter

codecov-commenter commented Aug 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.76543% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@ccf6929). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/inference_endpoint/load_generator/session.py 98.75% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #462   +/-   ##
=======================================
  Coverage        ?   81.31%           
=======================================
  Files           ?      151           
  Lines           ?    20528           
  Branches        ?        0           
=======================================
  Hits            ?    16693           
  Misses          ?     3835           
  Partials        ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@roborluo
roborluo force-pushed the fix-bofengl-no-progress-watchdog branch from 02eeec7 to a5b80ac Compare August 24, 2026 17:18
@roborluo
roborluo requested a review from viraatc August 24, 2026 17:39
Comment thread docs/load_generator/DESIGN.md Outdated

1. Publish `SessionEventType.STARTED`
2. Start receiver coroutine (`_receive_responses`)
2. Start receiver coroutine (`_receive_responses`). When the no-progress timeout is set, each active in-flight cohort arms a liveness watchdog; it exits as soon as the cohort drains. The watchdog fails the session only when an in-flight request has no streamed chunk or final result for the configured interval.

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.

Lets not use cohort here.

Comment thread src/inference_endpoint/config/schema.py Outdated
Comment on lines +655 to +669
help=(
"Fail a run when in-flight requests make no "
"response progress for this many seconds"
),
),
] = Field(
None,
gt=0,
description=(
"Fail a run when requests are in flight but no "
"response chunk or completion arrives for this many seconds. Disabled "
"by default; set above the longest expected interval between response "
"progress (full request latency for non-streaming endpoints) and, for "
"TensorRT-LLM disaggregated serving, match hang_detection_timeout."
),

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.

Lets keep a single copy in the help/description so there isn't drift over time.

@arekay-nv arekay-nv 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.

The opt-in liveness guard is useful, but this implementation adds avoidable work and state to the load generator's hot paths. The most serious issue is retaining every completed UUID in a second phase-lifetime container, which is an O(total requests) memory regression at the repository's 50k+ QPS target. The disabled path also performs work for every response, and low-concurrency workloads can create/cancel a watchdog task per request. Please keep the disabled path inert and use bounded active-request state plus a single event/deadline-driven watchdog.

Comment thread src/inference_endpoint/load_generator/session.py Outdated
Comment thread src/inference_endpoint/load_generator/session.py Outdated
Comment thread src/inference_endpoint/load_generator/session.py Outdated
Comment thread src/inference_endpoint/load_generator/session.py Outdated
Comment thread src/inference_endpoint/config/schema.py Outdated
Add a no-progress deadline for in-flight endpoint requests. An accepted
request that never produces a response chunk or final result would
otherwise block the benchmark until its outer wall-time limit, because
the phase drain waits on responses that never arrive. Disabled by
default (`settings.timeouts.no_progress_timeout_s`, `null`).

Rebased onto main and moved the setting into the Timeouts model added by
 mlcommons#409, as requested in review.

Review feedback addressed:

- [P1] Do not retain every completed UUID. `completed_uuids` now only
  holds synthetic completions from `register_skipped`, so it no longer
  grows O(total requests) alongside `uuid_to_index`.
- [P2] Keep the disabled path inert. Activity tracking is gated on the
  feature being enabled, and the receiver reuses the arrival timestamp
  it already takes rather than reading the clock a second time.
  Measured cost when disabled: +3.5 ns per response, +9.4 ns per request.
- [P2] Replace the per-transition watchdog task with a single
  self-rearming `loop.call_later` TimerHandle. No task, no Event, no
  per-iteration `wait_for`. Under the production eager task factory this
  cuts arm+cancel from 2205 ns to 535 ns per request (4.1x).
- [P2] Compute the remaining deadline from the last progress stamp
  instead of sleeping a full interval each iteration, which could delay
  detection by up to 2x the configured timeout.
- Drop `cohort` from the docs and from the local variable in `issue()`.
- Keep a single copy of the tuning guidance: the schema description is
  the short form, docs/config/DESIGN.md holds the detail.

Also fixed while here: a receiver transport error could overwrite an
earlier NoProgressError, masking the real diagnosis when a stalled
endpoint also drops its connection. First error now wins.

Validation:
- `pytest tests/unit/config/test_schema.py tests/unit/commands/test_benchmark.py
  tests/unit/load_generator/test_async_session.py` (425 passed)
- `pre-commit run --all-files` (mypy reports 3 pre-existing Darwin-only
  `os.sched_*affinity` errors in files this change does not touch)
- `python scripts/regenerate_templates.py --check`
- AGA disaggregated held-response canary, job 578946: failed as intended
  with `Endpoint made no response progress for 10.0s with 1 request(s) in
  flight`, 10.002 s after the phase started against a 10 s deadline. The
  10 s value is fault-injection coverage, not the deployment
  recommendation.

Not adopted: the suggestion to use one phase/session-lifetime watchdog
driven by an activity event. The guard is still armed on the 0-to-1
in-flight transition and retired on drain, now with a TimerHandle rather
than a task. This keeps the timer heap clear once a phase drains and
measured 4.1x cheaper than the reviewed version; happy to revisit if a
resident watchdog is preferred.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@roborluo
roborluo force-pushed the fix-bofengl-no-progress-watchdog branch from a5b80ac to 6d6f42a Compare August 27, 2026 00:46
@roborluo

Copy link
Copy Markdown
Collaborator Author

Thanks for the comments @viraatc and @arekay-nv . All issues resovled other than one.

For the per transition watchdog, I choosed a different approche than your suggested ,so flagging it here.

the guard is still armed on the 0→1 in-flight transition and retired on drain — but it's now a single loop.call_later TimerHandle instead of an asyncio task. No task, no Event, no per-iteration `wait_for.

Also measured arm+cancel per request, under the eager task factory this repo uses in production:

Task (reviewed version): 2205 ns
Timer (now): 535 ns → 4.1x

It also leaves the timer heap clear once a phase drains, which covers the "stale timer work" part of your comment.

let me know if that make sense to you, Thanks a lot!

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

Labels

size/normal PR Review Policy: <=500 non-test lines & <=20 files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants