[TRTLLM-13409][fix] give the benchmark-disagg fill gate's retry loop a deadline - #17202
Open
JunyiXu-nv wants to merge 3 commits into
Open
[TRTLLM-13409][fix] give the benchmark-disagg fill gate's retry loop a deadline#17202JunyiXu-nv wants to merge 3 commits into
JunyiXu-nv wants to merge 3 commits into
Conversation
JunyiXu-nv
force-pushed
the
dev-junyix-fix-disagg-fill-gate-deadlock
branch
from
August 3, 2026 13:40
10e576e to
d7c12ca
Compare
JunyiXu-nv
changed the title
Collaborator
Author
|
/bot run |
Collaborator
|
PR_Github #63472 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #63472 [ run ] completed with state
|
…a deadline `_check_benchmark_disagg_gate()` retries until the fill completes, with no bound. While it spins the job is invisible: the `continue` it drives sits before `iter_counter += 1`, so the iteration counter freezes while wall-clock advances, and the only outward sign is a stream of byte-identical iteration lines ~110 ms apart -- this gate's own `time.sleep(0.1)` seen from outside. Archived wedges show tens of thousands of them before Slurm kills the job. Bound the retry on LACK OF PROGRESS rather than on elapsed time: a fill that is merely slow keeps resetting the clock and is never killed; only a fill that makes no progress at all for the whole window raises. `TRTLLM_BENCHMARK_DISAGG_FILL_STALL_SEC=0` disables the bound; the default is 600 s. Raising surfaces the stall through the executor loop's existing error path rather than adding a second one, and the rank that raises names itself. Tests (no GPU): the bound fires after the window; the message names the rank and the knob; a slow-but-advancing fill survives 60 s against a 5 s window; progress resets the clock; 0 disables it; and a single no-progress pass only arms the clock rather than raising. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…st on CPU Two follow-ups before review. float() accepts "nan" and "inf", and each breaks the new bound in an opposite direction. Every nan comparison is False, so `nan <= 0` does not disable the bound and `stalled_for < nan` does not defer it -- control falls straight through to the raise, firing on the second consecutive stalled call (~0.1s) instead of after the 600s window, which destroys exactly the margin the default exists to give. inf is the mirror: `stalled_for < inf` is always True, so the bound never fires and is silently equivalent to 0. Reject both and fall back to the default, the same guard merged for TLLM_RANK_CRASH_HARD_KILL_GRACE in NVIDIA#16592. The tests are pure monkeypatch with no engine and no GPU, but lacked the cpu_only marker, so the l0_cpu `unittest/_torch/executor` entry collected nothing and they ran only on the h100/b300/gb300 stages. Add the marker so they also run in the CPU stage, where they belong. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
JunyiXu-nv
force-pushed
the
dev-junyix-fix-disagg-fill-gate-deadlock
branch
from
August 12, 2026 06:12
d7c12ca to
b992d6a
Compare
JunyiXu-nv
marked this pull request as ready for review
August 12, 2026 06:13
Collaborator
Author
|
/bot run --disable-fail-fast |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR adds configurable stall detection to the benchmark disaggregated-fill gate. It validates the timeout environment variable, tracks no-progress intervals, raises diagnostic errors after expiry, and adds CPU-only tests for configuration and retry behavior. ChangesDisaggregated-fill stall detection
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tensorrt_llm/_torch/pyexecutor/py_executor.py (1)
4089-4097: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftTrack asynchronous transfer progress in the fill-gate deadline.
The deadline resets only for synchronous transfer completion. A fill with periodic asynchronous completions can fail after the original timeout window even though it continues to advance.
tensorrt_llm/_torch/pyexecutor/py_executor.py#L4089-L4097: propagate async completion progress from the transfer-status poll into the model-parallel gate status before calling_fail_if_fill_gate_stalled().tests/unittest/_torch/executor/test_disagg_fill_gate_stall_bound.py#L71-L148: add a CPU-only test that completes one async transfer, leaves another incomplete, and verifies that the deadline starts a new window.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/_torch/pyexecutor/py_executor.py` around lines 4089 - 4097, Update the fill-gate loop around _fail_if_fill_gate_stalled() in tensorrt_llm/_torch/pyexecutor/py_executor.py:4089-4097 to include asynchronous completion progress from the transfer-status poll when determining model-parallel progress, so periodic async completions reset the deadline. Add a CPU-only test in tests/unittest/_torch/executor/test_disagg_fill_gate_stall_bound.py:71-148 that completes one async transfer while another remains incomplete and verifies a new deadline window begins.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unittest/_torch/executor/test_disagg_fill_gate_stall_bound.py`:
- Around line 43-148: Add complete type annotations to the new _Clock methods,
_executor, and _spin, including parameter and return types; use -> None for
procedural methods. Annotate all nine test functions with their fixture
parameters and -> None, preserving the existing test behavior and fixtures.
---
Outside diff comments:
In `@tensorrt_llm/_torch/pyexecutor/py_executor.py`:
- Around line 4089-4097: Update the fill-gate loop around
_fail_if_fill_gate_stalled() in
tensorrt_llm/_torch/pyexecutor/py_executor.py:4089-4097 to include asynchronous
completion progress from the transfer-status poll when determining
model-parallel progress, so periodic async completions reset the deadline. Add a
CPU-only test in
tests/unittest/_torch/executor/test_disagg_fill_gate_stall_bound.py:71-148 that
completes one async transfer while another remains incomplete and verifies a new
deadline window begins.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 85766003-4a91-472b-93f2-d67d3c4b9975
📒 Files selected for processing (2)
tensorrt_llm/_torch/pyexecutor/py_executor.pytests/unittest/_torch/executor/test_disagg_fill_gate_stall_bound.py
Comment on lines
+43
to
+148
| class _Clock: | ||
| def __init__(self): | ||
| self.t = 1000.0 | ||
|
|
||
| def __call__(self): | ||
| return self.t | ||
|
|
||
| def advance(self, dt): | ||
| self.t += dt | ||
|
|
||
|
|
||
| def _executor(timeout_s, clock, rank=0): | ||
| """A PyExecutor with only the fill-gate stall surface populated.""" | ||
| ex = object.__new__(PyExecutor) | ||
| ex._benchmark_fill_stall_since = None | ||
| ex._benchmark_fill_stall_timeout_sec = timeout_s | ||
| ex.dist = types.SimpleNamespace(rank=rank) | ||
| return ex | ||
|
|
||
|
|
||
| def _spin(ex, clock, monkeypatch, seconds, step=0.1, made_progress=False): | ||
| """Drive the no-progress retry path for `seconds` of wall clock.""" | ||
| monkeypatch.setattr("tensorrt_llm._torch.pyexecutor.py_executor.time.monotonic", clock) | ||
| for _ in range(int(seconds / step)): | ||
| ex._fail_if_fill_gate_stalled(made_progress) | ||
| clock.advance(step) | ||
|
|
||
|
|
||
| def test_raises_once_the_stall_window_elapses(monkeypatch): | ||
| clock = _Clock() | ||
| ex = _executor(5.0, clock, rank=3) | ||
| with pytest.raises(RuntimeError, match="made no progress"): | ||
| _spin(ex, clock, monkeypatch, seconds=8.0) | ||
|
|
||
|
|
||
| def test_message_names_the_rank_and_the_knob(monkeypatch): | ||
| clock = _Clock() | ||
| ex = _executor(5.0, clock, rank=7) | ||
| with pytest.raises(RuntimeError) as exc: | ||
| _spin(ex, clock, monkeypatch, seconds=8.0) | ||
| msg = str(exc.value) | ||
| assert "rank 7" in msg | ||
| assert "TRTLLM_BENCHMARK_DISAGG_FILL_STALL_SEC" in msg | ||
|
|
||
|
|
||
| def test_a_slow_but_advancing_fill_is_never_killed(monkeypatch): | ||
| """The bound is on no-progress, not on elapsed time. | ||
|
|
||
| A fill that keeps making progress may legitimately take far longer than | ||
| the window; killing it would be a regression, not a fix. | ||
| """ | ||
| clock = _Clock() | ||
| ex = _executor(5.0, clock) | ||
| _spin(ex, clock, monkeypatch, seconds=60.0, made_progress=True) | ||
| assert ex._benchmark_fill_stall_since is None | ||
|
|
||
|
|
||
| def test_progress_resets_the_clock(monkeypatch): | ||
| """Stall, recover, stall again -- the second window starts from zero.""" | ||
| clock = _Clock() | ||
| ex = _executor(5.0, clock) | ||
| _spin(ex, clock, monkeypatch, seconds=4.0) # just under | ||
| ex._fail_if_fill_gate_stalled(True) # progress | ||
| assert ex._benchmark_fill_stall_since is None | ||
| _spin(ex, clock, monkeypatch, seconds=4.0) # under again | ||
| assert ex._benchmark_fill_stall_since is not None # armed, not fired | ||
|
|
||
|
|
||
| def test_zero_disables_the_bound(monkeypatch): | ||
| clock = _Clock() | ||
| ex = _executor(0.0, clock) | ||
| _spin(ex, clock, monkeypatch, seconds=3600.0, step=10.0) | ||
| assert ex._benchmark_fill_stall_since is None | ||
|
|
||
|
|
||
| def test_first_stalled_call_only_arms_the_clock(monkeypatch): | ||
| """One no-progress pass is normal; it must not raise on its own.""" | ||
| monkeypatch.setattr("tensorrt_llm._torch.pyexecutor.py_executor.time.monotonic", _Clock()) | ||
| ex = _executor(5.0, _Clock()) | ||
| ex._fail_if_fill_gate_stalled(False) | ||
| assert ex._benchmark_fill_stall_since is not None | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("raw", ["nan", "NaN", "inf", "-inf", "Infinity"]) | ||
| def test_non_finite_env_falls_back_to_the_default(monkeypatch, raw): | ||
| """float() accepts nan/inf, and each breaks the bound a different way. | ||
|
|
||
| Every nan comparison is False, so ``nan <= 0`` does not disable the | ||
| bound and ``stalled_for < nan`` does not defer it: control falls through | ||
| to the raise, firing on the second consecutive stalled call rather than | ||
| after the window. ``inf`` is the mirror -- ``stalled_for < inf`` is | ||
| always True, so the bound never fires and is silently equivalent to 0. | ||
| """ | ||
| monkeypatch.setenv(BENCHMARK_DISAGG_FILL_STALL_ENV_VAR_NAME, raw) | ||
| assert _fill_stall_timeout_sec() == 600.0 | ||
|
|
||
|
|
||
| def test_a_valid_env_override_is_honoured(monkeypatch): | ||
| monkeypatch.setenv(BENCHMARK_DISAGG_FILL_STALL_ENV_VAR_NAME, "42.5") | ||
| assert _fill_stall_timeout_sec() == 42.5 | ||
|
|
||
|
|
||
| def test_zero_env_is_preserved_as_the_disable_switch(monkeypatch): | ||
| """0 must survive the finiteness check -- it is the documented opt-out.""" | ||
| monkeypatch.setenv(BENCHMARK_DISAGG_FILL_STALL_ENV_VAR_NAME, "0") | ||
| assert _fill_stall_timeout_sec() == 0.0 |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="tests/unittest/_torch/executor/test_disagg_fill_gate_stall_bound.py"
printf '%s\n' '--- tracked file and metadata ---'
git ls-files -- "$file" CODING_GUIDELINES.md tests/integration/test_lists | sed -n '1,120p'
printf '%s\n' '--- file outline ---'
ast-grep outline "$file" || true
printf '%s\n' '--- reviewed lines ---'
cat -n "$file" | sed -n '1,190p'
printf '%s\n' '--- applicable guidelines ---'
if [ -f CODING_GUIDELINES.md ]; then
cat -n CODING_GUIDELINES.md | sed -n '1,240p'
else
printf '%s\n' 'CODING_GUIDELINES.md not found at repository root'
fi
printf '%s\n' '--- references to test functions and test-list entries ---'
rg -n -F \
-e 'test_raises_once_the_stall_window_elapses' \
-e 'test_message_names_the_rank_and_the_knob' \
-e 'test_a_slow_but_advancing_fill_is_never_killed' \
-e 'test_progress_resets_the_clock' \
-e 'test_zero_disables_the_bound' \
-e 'test_first_stalled_call_only_arms_the_clock' \
-e 'test_non_finite_env_falls_back_to_the_default' \
-e 'test_a_valid_env_override_is_honoured' \
-e 'test_zero_env_is_preserved_as_the_disable_switch' \
tests/integration/test_lists .github 2>/dev/null | sed -n '1,240p'
printf '%s\n' '--- nearby test-list files ---'
fd -i 'test-db|qa' tests/integration/test_lists 2>/dev/null | sed -n '1,160p'Repository: NVIDIA/TensorRT-LLM
Length of output: 25360
🏁 Script executed:
#!/bin/bash
set -eu
file="tests/unittest/_torch/executor/test_disagg_fill_gate_stall_bound.py"
printf '%s\n' '--- Python annotation guidance ---'
rg -n -i 'annotat|type hint|function|pytest|Python' CODING_GUIDELINES.md | sed -n '1,180p'
printf '%s\n' '--- AST annotation check ---'
python3 - "$file" <<'PY'
import ast
import pathlib
import sys
path = pathlib.Path(sys.argv[1])
tree = ast.parse(path.read_text())
for node in ast.walk(tree):
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
missing = []
if node.args.posonlyargs:
missing.extend(arg.arg for arg in node.args.posonlyargs if arg.annotation is None)
missing.extend(arg.arg for arg in node.args.args if arg.annotation is None)
missing.extend(arg.arg for arg in node.args.kwonlyargs if arg.annotation is None)
if node.args.vararg and node.args.vararg.annotation is None:
missing.append("*" + node.args.vararg.arg)
if node.args.kwarg and node.args.kwarg.annotation is None:
missing.append("**" + node.args.kwarg.arg)
print(
f"{node.lineno}:{node.name}: "
f"missing_params={missing!r}, "
f"missing_return={node.returns is None}"
)
PY
printf '%s\n' '--- test file references in integration lists ---'
rg -n -i 'test_disagg_fill_gate_stall_bound|disagg.*fill|fill.*stall|_torch/executor' \
tests/integration/test_lists || true
printf '%s\n' '--- relevant list entries and list formats ---'
rg -n -i 'disagg|cpu_only|unittest|test_file|test_path' \
tests/integration/test_lists/test-db/l0_cpu.yml \
tests/integration/test_lists/qa/llm_perf_disagg.yml \
tests/integration/test_lists/test-db tests/integration/test_lists/qa \
2>/dev/null | sed -n '1,240p'Repository: NVIDIA/TensorRT-LLM
Length of output: 49137
Add type annotations to every new function.
Annotate _Clock methods, _executor, _spin, and the nine test functions. Add parameter and return annotations. Use -> None for procedures.
Test coverage is sufficient. The tests are included by tests/integration/test_lists/test-db/l0_cpu.yml through unittest/_torch/executor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unittest/_torch/executor/test_disagg_fill_gate_stall_bound.py` around
lines 43 - 148, Add complete type annotations to the new _Clock methods,
_executor, and _spin, including parameter and return types; use -> None for
procedural methods. Annotate all nine test functions with their fixture
parameters and -> None, preserving the existing test behavior and fixtures.
Source: Coding guidelines
Collaborator
|
PR_Github #65516 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #65516 [ run ] completed with state
|
…t now needs The gate now calls _fail_if_fill_gate_stalled(), and MockBenchmarkExecutor binds the gate methods off PyExecutor without having that one or the two attributes it reads. 12 CPU-Generic failures in pipeline 53254, all "AttributeError: 'MockBenchmarkExecutor' object has no attribute '_fail_if_fill_gate_stalled'". Only visible after the rebase: test_benchmark_disagg.py arrived on main after this branch was cut, so the pre-rebase runs never exercised it. Bind the method and seed the state with the bound disabled. That is right twice over: these tests assert retry semantics rather than the deadline, which test_disagg_fill_gate_stall_bound.py covers, and they patch the whole `time` module -- an enabled bound would do arithmetic on a Mock. The `timeout_s <= 0` guard returns before the clock is read, so the patched module is never touched. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
Collaborator
Author
|
/bot run --disable-fail-fast |
Collaborator
|
PR_Github #65569 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #65569 [ run ] completed with state
|
Collaborator
Author
|
/bot run --disable-fail-fast |
Collaborator
|
PR_Github #65615 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #65615 [ run ] completed with state
|
mikeiovine
approved these changes
Aug 12, 2026
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
_check_benchmark_disagg_gate()retries until the fill completes, with no bound.While it spins the job is effectively invisible. The
continuethe gate drives sits beforeiter_counter += 1, so the iteration counter freezes while wall-clock advances. The only outward sign is a stream of byte-identical iteration lines about 110 ms apart — which is this gate's owntime.sleep(0.1)observed from outside.Archived wedges show tens of thousands of those lines before Slurm kills the job. One example: 48,486 identical lines over ~90 minutes, with
num_scheduled_requests,kv_cache_utilandcurrank_total_requestsconstant to three decimals, andhost_step_time = 110.14ms.Correction (2026-08-05): those archived wedges are NOT what this PR fixes
A log analysis of 12 of those stages (GB300 disagg perf-sanity, builds 2859-2863)
root-caused them to a different bug, since fixed on main by
37a7c09818("[nvbugs/6510284][fix] Clamp benchmark fill target in PyExecutor", #16961).
There, gate condition (A)
num_fetch_requests >= benchmark_req_queues_sizewasarithmetically unsatisfiable: while the gate is closed nothing completes, so the
fetch count is bounded by
tp_size x max_batch_size, and the harness could set thetarget above that bound (4301 vs 4096, 180 vs 128, 666 vs 512, 8 vs 1). #16961 clamps
the target to the capacity, and the hang burn on those stages fell from ~338 to
~17 GPU-h/day.
So the 110 ms spin signature quoted above is real, but it is that bug's fingerprint,
not evidence of an unbounded wait that survives #16961. I am leaving the description
of the signature in place because it is still exactly what this bound would surface.
Why this PR is still worth having
The gate has three conditions. #16961 makes (A) always reachable. It does nothing
for:
kv_cache_transceiver.check_gen_transfer_complete().Both can stall indefinitely, and the same analysis found that the machinery meant to
bound them cannot fire in the shipped
gen_onlyconfiguration:kv_transfer_timeout_msis 600 s but is unreachable code on the GEN side under
TRTLLM_DISABLE_KV_CACHE_TRANSFER_OVERLAP=1(_recv_disagg_gen_cachereturns beforethe arming block), and in-flight cancellation is separately disabled by
transceiver_runtime: PYTHON.So a (B)/(C) stall today is still an unbounded, silent wait. That is the case this
bound covers. I have no archived instance of a (B)/(C) stall to point at -- the
justification is structural, not empirical, and reviewers should weigh it on that basis.
Related and worth doing alongside: the gate's own diagnostic at
_is_benchmark_disagg_fill_completenames the blocking ranks and per-state counts butis
logger.debug, so 90 minutes of spinning emitted zero lines about the cause. That isnot in this PR.
The fix
Bound the retry on lack of progress, not on elapsed time.
A fill that is merely slow keeps resetting the clock and is never killed. Only a fill that makes no progress at all for the whole window raises.
TRTLLM_BENCHMARK_DISAGG_FILL_STALL_SEC=0disables the bound; the default is 600 s.Raising surfaces the stall through the executor loop's existing error path rather than adding a second one, and the rank that raises names itself in the message.
Tests
tests/unittest/_torch/executor/test_disagg_fill_gate_stall_bound.py— no GPU:0disables the bound;What the first attempt got wrong
Worth recording, because the mistake is easy to repeat.
The original claim was that
_handle_kv_transfer_timeouts_synced()— the drain that would surface the timeout — sits after the gate'scontinue, so moving it above would let the existing error propagate. Two things were wrong:The state named was wrong.
_check_kv_transfer_timeout()setsreq.py_kv_transfer_timed_out, notDISAGG_TRANS_ERROR. That state is set on the transfer-status path (_update_sampler_state_for_disagg_gen_request,_check_disagg_gen_cache_transfer_status), which is unrelated to the timeout.The fix would have done nothing.
_pending_timed_out_requests— the buffer the drain reads — has exactly one populator,_handle_responses(). That is also downstream of thecontinue(L4156 in_executor_loop, reached via_process_previous_batch()at L4685 in_executor_loop_overlap). So during a spin the buffer is never filled, and the relocated drain would have run atp_allgatheron an empty list every iteration — pure cost, no effect.The verification error was checking that the drain was downstream of the
continueand stopping there, without checking where the buffer was filled. The original unit tests passed because the stub modelled the drain as clearing the error directly, which encoded the assumption rather than testing it.Status
Draft — not yet validated on multi-GPU hardware. The reasoning is from source and archived wedge logs; the unit tests cover the bound's arming, firing and reset, not a real stalled transfer.
Dev Engineer Review
_check_benchmark_disagg_gate().TRTLLM_BENCHMARK_DISAGG_FILL_STALL_SECfor overrides.0as disabled.QA Engineer Review
tests/unittest/_torch/executor/test_disagg_fill_gate_stall_bound.py.MockBenchmarkExecutorto support the new stall-checking logic.tests/integration/test_lists/.