[https://nvbugs/6435121][fix] Eliminate the trtllm-serve port reservation race with --port 0 + --report_addr - #17460
Open
JunyiXu-nv wants to merge 4 commits into
Open
Conversation
JunyiXu-nv
requested review from
BowenFu,
QiJune,
Shixiaowei02,
Tabrizian,
ZhanruiSunCh,
fredricz-20070104,
mlefeb01 and
ruodil
August 10, 2026 03:55
Contributor
WalkthroughThe changes add atomic bound-address reporting, support kernel-assigned ports, and update integration workflows to discover server and worker addresses at runtime. Worker registration and respawn handling now use ChangesDynamic address publication and worker discovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TestWorkflow
participant Server
participant ReportFile
participant Worker
participant ClusterInfo
TestWorkflow->>Server: Start with port=0 and report_addr
Server->>ReportFile: Publish bound host and port
TestWorkflow->>ReportFile: Wait for reported address
TestWorkflow->>Worker: Start with port=0 and resolved configuration
Worker->>ClusterInfo: Register assigned port and worker index
TestWorkflow->>ClusterInfo: Request registered workers
ClusterInfo-->>TestWorkflow: Return worker endpoints
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 3
🧹 Nitpick comments (3)
tests/integration/defs/disaggregated/disagg_test_utils.py (1)
325-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type annotations to the new URL-discovery helper.
Annotate
get_registered_worker_urls()and nested_urls(). The public return type istuple[list[str], list[str]].As per coding guidelines, “Annotate every function.”
🤖 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/integration/defs/disaggregated/disagg_test_utils.py` around lines 325 - 348, Add type annotations to get_registered_worker_urls, including an int parameter and tuple[list[str], list[str]] return type. Annotate the nested _urls helper with its role_key parameter as str and its return type as list[str], preserving the existing URL discovery behavior.Source: Coding guidelines
tests/integration/defs/common.py (1)
681-762: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type annotations to the new helper functions.
Annotate
get_ephemeral_port_range(),get_static_port_range(),reserve_port_from_range(), andget_free_port_in_ci(). Use Python 3.10 union syntax for optional results.As per coding guidelines, “Annotate every function.”
🤖 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/integration/defs/common.py` around lines 681 - 762, Add type annotations to get_ephemeral_port_range, get_static_port_range, reserve_port_from_range, and get_free_port_in_ci, including parameter types and return types. Use Python 3.10 union syntax for optional return values, and annotate every function consistently with the existing data structures and behavior.Source: Coding guidelines
tests/integration/defs/stress_test/disagg_cancel/harness.py (1)
1908-1910: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreformat the new injector log messages.
tensorrt_llm.loggerjoins arguments. It does not apply%sor%dinterpolation. Use one f-string argument for each message.
tests/integration/defs/stress_test/disagg_cancel/harness.py#L1908-L1910: preformat the respawn failure message.tests/integration/defs/stress_test/disagg_cancel/harness.py#L1921-L1926: preformat the missing-registration message.tests/integration/defs/stress_test/disagg_cancel/harness.py#L1931-L1937: preformat the health-wait message.tests/integration/defs/stress_test/disagg_cancel/harness.py#L1989-L1996: preformat invalid-port and polling-failure messages.tests/integration/defs/stress_test/disagg_cancel/harness.py#L1999-L2003: preformat the registration-timeout message.Based on learnings,
tensorrt_llm.loggerjoins arguments rather than performing Python printf-style interpolation; preformat dynamic messages as a single f-string argument.🤖 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/integration/defs/stress_test/disagg_cancel/harness.py` around lines 1908 - 1910, Update the injector logging calls in tests/integration/defs/stress_test/disagg_cancel/harness.py at lines 1908-1910, 1921-1926, 1931-1937, 1989-1996, and 1999-2003 to pass each dynamic message as one preformatted f-string argument, covering respawn failures, missing registration, health-wait, invalid-port, polling-failure, and registration-timeout messages.Source: Learnings
🤖 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/integration/defs/common.py`:
- Around line 687-694: Update get_ephemeral_port_range() to validate the parsed
low and high bounds after reading them, accepting only ranges where 1 <= low <=
high <= 65535. Treat invalid bounds like parsing failures by reporting the
existing diagnostic and returning None, preserving get_static_port_range()’s
documented invalid-range behavior.
In `@tests/integration/defs/disaggregated/disagg_test_utils.py`:
- Around line 340-343: Update the helper containing the port and /cluster_info
checks to add port: int and -> tuple[list[str], list[str]] annotations, raise
ValueError when port is not positive, and raise an explicit exception when the
request status is not 200 instead of using assertions. Preserve the existing
worker extraction behavior for successful responses.
In `@tests/integration/defs/stress_test/disagg_cancel/harness.py`:
- Around line 1908-1913: Update the respawn logic around the tracked.wrapper
assignment so the corresponding entry in self._cluster’s ctx_workers or
gen_workers list is replaced with new_wrapper when respawning begins. Ensure
_teardown_cluster() can terminate the current wrapper for both successful and
failed respawns, preserving the existing tracked-worker bookkeeping.
---
Nitpick comments:
In `@tests/integration/defs/common.py`:
- Around line 681-762: Add type annotations to get_ephemeral_port_range,
get_static_port_range, reserve_port_from_range, and get_free_port_in_ci,
including parameter types and return types. Use Python 3.10 union syntax for
optional return values, and annotate every function consistently with the
existing data structures and behavior.
In `@tests/integration/defs/disaggregated/disagg_test_utils.py`:
- Around line 325-348: Add type annotations to get_registered_worker_urls,
including an int parameter and tuple[list[str], list[str]] return type. Annotate
the nested _urls helper with its role_key parameter as str and its return type
as list[str], preserving the existing URL discovery behavior.
In `@tests/integration/defs/stress_test/disagg_cancel/harness.py`:
- Around line 1908-1910: Update the injector logging calls in
tests/integration/defs/stress_test/disagg_cancel/harness.py at lines 1908-1910,
1921-1926, 1931-1937, 1989-1996, and 1999-2003 to pass each dynamic message as
one preformatted f-string argument, covering respawn failures, missing
registration, health-wait, invalid-port, polling-failure, and
registration-timeout messages.
🪄 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: c013471a-9171-47df-80ba-b058551950c7
📒 Files selected for processing (4)
tests/integration/defs/common.pytests/integration/defs/disaggregated/disagg_test_utils.pytests/integration/defs/disaggregated/test_workers.pytests/integration/defs/stress_test/disagg_cancel/harness.py
Comment on lines
+340
to
+343
| assert port > 0, "port must be positive" | ||
| info_resp = requests.get(f"http://localhost:{port}/cluster_info", timeout=5) | ||
| assert info_resp.status_code == 200, f"cluster_info returned {info_resp.status_code}" | ||
| workers = info_resp.json().get("current_workers", {}) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 --hidden 'PYTHONOPTIMIZE|python[[:space:]].*-O|pytest[[:space:]].*-O' \
.github tests pyproject.toml setup.cfg tox.ini 2>/dev/null || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- file outline ---'
ast-grep outline tests/integration/defs/disaggregated/disagg_test_utils.py 2>/dev/null || true
printf '%s\n' '--- target function and nearby code ---'
sed -n '300,365p' tests/integration/defs/disaggregated/disagg_test_utils.py
printf '%s\n' '--- relevant guideline sections ---'
if [ -f CODING_GUIDELINES.md ]; then
rg -n -C2 'assert|exception|annotation|copyright|test' CODING_GUIDELINES.md
fi
printf '%s\n' '--- repository change summary ---'
git diff --stat
git status --short
printf '%s\n' '--- helper references ---'
rg -n -C3 'get_registered_worker_urls|cluster_info' tests .github 2>/dev/null | head -240Repository: NVIDIA/TensorRT-LLM
Length of output: 32356
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
import subprocess
from pathlib import Path
path = Path("tests/integration/defs/disaggregated/disagg_test_utils.py")
tree = ast.parse(path.read_text())
function = next(
node for node in tree.body
if isinstance(node, ast.FunctionDef)
and node.name == "get_registered_worker_urls"
)
print("function_args:", [arg.arg for arg in function.args.args])
print("has_return_annotation:", function.returns is not None)
print(
"assertions_in_function:",
sum(isinstance(node, ast.Assert) for node in ast.walk(function)),
)
probe = "assert False, 'assertion was executed'; print('continued')"
normal = subprocess.run(
["python3", "-c", probe], capture_output=True, text=True
)
optimized = subprocess.run(
["python3", "-O", "-c", probe], capture_output=True, text=True
)
print("normal_returncode:", normal.returncode)
print("optimized_returncode:", optimized.returncode)
print("optimized_stdout:", optimized.stdout.strip())
PY
printf '%s\n' '--- relevant test-list files ---'
find tests/integration/test_lists -type f \( -name '*disagg*' -o -name '*worker*' \) -print 2>/dev/null | sort
rg -n -C2 'test_workers|workers' tests/integration/test_lists 2>/dev/null | head -160Repository: NVIDIA/TensorRT-LLM
Length of output: 9200
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("tests/integration/defs/disaggregated/test_workers.py")
tree = ast.parse(path.read_text())
for node in ast.walk(tree):
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
start = node.lineno
end = max(
[child.lineno for child in ast.walk(node) if hasattr(child, "lineno")]
or [start]
)
if start <= 645 <= end:
print("enclosing_test:", node.name)
print("function_range:", f"{start}-{end}")
break
PY
printf '%s\n' '--- all test_workers entries in test lists ---'
rg -n 'disaggregated/test_workers\.py::' tests/integration/test_lists/qa \
tests/integration/test_lists/test-db 2>/dev/nullRepository: NVIDIA/TensorRT-LLM
Length of output: 2636
Replace runtime assertions with explicit exceptions.
Raise ValueError for an invalid port, and raise an explicit exception when /cluster_info does not return 200. Add port: int and -> tuple[list[str], list[str]] annotations.
Coverage: background_workers exercises this helper, and its worker tests are listed in the QA and test-db lists.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 340-340: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.get(f"http://localhost:{port}/cluster_info", timeout=5)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
🤖 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/integration/defs/disaggregated/disagg_test_utils.py` around lines 340 -
343, Update the helper containing the port and /cluster_info checks to add port:
int and -> tuple[list[str], list[str]] annotations, raise ValueError when port
is not positive, and raise an explicit exception when the request status is not
200 instead of using assertions. Preserve the existing worker extraction
behavior for successful responses.
Source: Coding guidelines
JunyiXu-nv
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/integration/defs/common.py (1)
674-706: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd annotations to the new helper functions.
tests/integration/defs/common.py#L674-L706: annotateaddr_path,timeout,process, and thetuple[str, int]return value.tests/integration/defs/accuracy/test_disaggregated_serving.py#L221-L231: annotatecluster_uriasstrand the return value asNone.As per coding guidelines, “Annotate every function.”
🤖 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/integration/defs/common.py` around lines 674 - 706, Annotate wait_for_reported_addr in tests/integration/defs/common.py: use appropriate types for addr_path, timeout, and optional process, and retain tuple[str, int] as the return type. Also annotate the helper at tests/integration/defs/accuracy/test_disaggregated_serving.py:221-231 with cluster_uri: str and a None return type.Source: Coding guidelines
tensorrt_llm/commands/serve.py (1)
361-370: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the broad cleanup handler.
Line 367 catches
BaseException. This also catchesKeyboardInterruptandSystemExit.Use
finallyfor temporary-file cleanup. This preserves cleanup without a broad exception handler.Proposed fix
try: with os.fdopen(fd, "w") as f: f.write(f"{host}:{port}\n") f.flush() os.fsync(f.fileno()) os.replace(tmp_path, report_addr) - except BaseException: + finally: with contextlib.suppress(OSError): os.unlink(tmp_path) - raise🤖 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/commands/serve.py` around lines 361 - 370, Replace the BaseException handler surrounding the temporary report-file write in the serve flow with a finally block that suppresses OSError while unlinking tmp_path. Preserve the existing write, fsync, atomic os.replace, and exception propagation behavior while ensuring cleanup runs on every exit path.Source: Coding guidelines
🤖 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 `@tensorrt_llm/commands/serve.py`:
- Around line 362-366: Update the report-address formatting around the
os.fdopen/os.replace block to wrap IPv6 host literals in brackets before
appending the port, while leaving IPv4 and hostname formatting unchanged. Ensure
a host such as ::1 is persisted as [::1]:<port> so consumers can construct valid
URL authorities.
---
Nitpick comments:
In `@tensorrt_llm/commands/serve.py`:
- Around line 361-370: Replace the BaseException handler surrounding the
temporary report-file write in the serve flow with a finally block that
suppresses OSError while unlinking tmp_path. Preserve the existing write, fsync,
atomic os.replace, and exception propagation behavior while ensuring cleanup
runs on every exit path.
In `@tests/integration/defs/common.py`:
- Around line 674-706: Annotate wait_for_reported_addr in
tests/integration/defs/common.py: use appropriate types for addr_path, timeout,
and optional process, and retain tuple[str, int] as the return type. Also
annotate the helper at
tests/integration/defs/accuracy/test_disaggregated_serving.py:221-231 with
cluster_uri: str and a None return type.
🪄 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: 681d153c-539d-4908-9bf8-21b308d67943
📒 Files selected for processing (5)
tensorrt_llm/commands/serve.pytests/integration/defs/accuracy/test_disaggregated_serving.pytests/integration/defs/common.pytests/integration/defs/perf/test_perf_sanity.pytests/unittest/api_stability/references/trtllm_serve_cli.yaml
… the CI port allocator Several disagg CI failures share one mechanism: the test harness pre-picks a port with get_free_port(), which binds, reads getsockname() and then closes the socket, and hands the number to a trtllm-serve subprocess that binds it much later. Anything can take the port in between. 82c1ba8 addressed this for test_auto_scaling by passing --port 0 and letting service discovery report the address the worker actually bound. Apply the same method to the remaining call sites where service discovery is already configured, and close the allocator hole that made the race reachable at all. - test_workers.py::background_workers configured a full disagg_cluster and then still handed --port N to every ctx/gen worker. Launch with port=0 and read the real URLs back from the cluster registry once the server reports ready. The URL format is unchanged: a worker whose host and cluster_uri are both localhost registers as localhost, so the router/tester call sites are unaffected. Also pass worker_index, which the function omitted. - disagg_cancel/harness.py pre-picked a port when relaunching a SIGKILLed worker, while the initial launch already used port=0. Resolving the port needs a lookup, since new_wrapper.port feeds the /health poll, so match the registry entry on pid: WorkerInfo.worker_id embeds os.getpid() of the trtllm-serve process. Matching on "a port we have not seen before" would be ambiguous while the killed worker's stale registration is still being reaped. Registration and health now share one deadline instead of each getting the full timeout. Also pass worker_index, whose absence made a respawn of worker N truncate worker 0's log out from under the log scanner. - get_free_port_in_ci fell straight through to get_free_port() when CONTAINER_PORT_START is unset, i.e. the SLURM multi-node path, drawing reserved ports from the very ephemeral pool that trtllm-serve's own --port 0 workers bind from. Add an intermediate fallback that reserves from a window just below /proc/sys/net/ipv4/ip_local_port_range, which bind(('', 0)) never hands out, so a reserved port can no longer be taken by a sibling worker. The existing probe-bind loop is extracted into reserve_port_from_range() and shared by both ranges; the ephemeral fallback remains as a last resort. Partially addresses https://nvbugs/6567057, https://nvbugs/6435121 and https://nvbugs/6526529. The front ports those bugs fail on (the disagg server and perf-sanity worker ports) still pre-pick, now from a safer range; closing them needs trtllm-serve to publish its resolved bind address. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…se them in disagg tests Two independent causes hide behind the same EADDRINUSE: 1. TIME_WAIT tombstones. launch_server and the disaggregated server bound their sockets without SO_REUSEADDR, so after a server exits, the TIME_WAIT entries of the connections it accepted refuse a rebind of that port for ~60s. This is not a race and no amount of port juggling avoids it. Measured: the flag has to be set on the socket that owned the port first, because the TIME_WAIT entry inherits it -- setting it only on the later bind is not enough. Set it unconditionally on all three HTTP bind sites. The main beneficiary is the product path, where users pass an explicit --port and restart. 2. Reserving a port before the process that binds it exists. A harness picks a port, closes the probe socket, and hands the number to a trtllm-serve that binds it much later; anything can take it in between. For (2), add --report_addr: with --port 0 the kernel assigns the port, the socket stays bound from that moment until uvicorn takes it over, and the resolved host:port is published atomically (temp file + rename, so a reader never sees a partial line -- it matters on the shared filesystems multi-node tests coordinate through). This is the same shape the KV cache transceiver already uses, where the ZMQ rendezvous socket binds ":*" and its address rides out in-band; that path has never produced a port conflict. Reservation is inherently host-local, but publication is not, which is why this works multi-node: every site that picks a port does so on the node that will bind it, and only the resolved address has to travel. --report_addr is rejected for the gRPC and VisualGen servers, and for the disagg fleet topologies, rather than silently never being written: with num_workers>1 the port goes to N SO_REUSEPORT workers, which under port 0 would each get a different kernel-assigned port instead of sharing one. test_disaggregated_serving.py now starts the disaggregated server first with --port 0, reads back the address, and only then writes the worker configs carrying the resolved cluster_uri. The server's own copy of cluster_uri keeps a placeholder port because HttpClusterStorageServer serves the storage on the server's own port and never reads the URI; only workers dial it. Addresses https://nvbugs/6567057 and https://nvbugs/6435121. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…of reserving one The aggregated server, the disaggregated server and the CTX/GEN workers each picked a port with get_free_port() and handed the number to a trtllm-serve that bound it much later. On a 44-GPU/11-node stage that window is wide, and https://nvbugs/6526529 caught the GEN server losing its port in exactly that gap. Launch all three with --port 0 --report_addr instead: the kernel assigns the port, the socket stays bound from that moment, and the server publishes the resolved host:port itself. The cross-node coordination channel is unchanged in shape -- the CTX/GEN tasks still deposit host:port files that the DISAGG_SERVER task turns into its config -- except those files are now written by the servers rather than guessed by the harness. Reservation stays host-local, which is what makes this work multi-node: every task picks a port for a server on its own node, and only the resolved address crosses nodes. Two things this needs to be correct: - The coordination directory is now scoped by SLURM_JOB_ID. test_output_dir is derived from the test case name alone and created with exist_ok=True, so a rerun of the same case reused it; once the files are server-written rather than harness-written, a leftover file from a previous run would point the disagg server at a dead worker, which fails far less obviously than a port conflict. The step id is deliberately excluded, since each role is a separate srun step within one job and they must agree on the path. - The directory scan filters to *.txt. The address is published by renaming a "<name>.<rand>.tmp" sibling into place, and counting those transient entries would both inflate the expected-count check and get parsed as a worker url. The BENCHMARK task now waits on the disagg server's reported address rather than reading the port out of the generated config, which under port 0 would be 0. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…thority
Three findings from the PR review, all verified against the code first:
- disagg_cancel respawn leaked the new worker past teardown. _teardown_cluster
terminates the wrapper lists unpacked from self._cluster, not
self._tracked_workers, so a respawn that only updated tracked.wrapper stayed
alive after the test and kept holding its GPUs. Replace the slot in the
cluster list too, before the port wait, so a respawn that never registers is
cleaned up as well. spec.index is per-role, matching how the ctx/gen spec
lists are built. Pre-existing, but in the function this PR rewrote.
- get_ephemeral_port_range() accepted implausible /proc contents. With
"70000 80000" it yielded a static window of (65904, 69999), and bind() raises
OverflowError rather than OSError for ports above 65535, so
reserve_port_from_range would propagate it instead of trying another port.
Reject anything outside 1 <= low <= high <= 65535 and fall through, which is
what the docstring already claimed.
- The reported address was not a valid URL authority for IPv6. --host ::1 wrote
"::1:<port>", and consumers build "http://<reported>" verbatim. Bracket IPv6
literals so it reads "[::1]:<port>"; the reader's rpartition(":") keeps
working and now yields a host that is directly usable in a URL.
A fourth comment suggested replacing the asserts in get_registered_worker_urls
with explicit exceptions. Skipped: the "raise ValueError instead of assertions"
rule in CODING_GUIDELINES.md sits under the Pydantic validation section, the
neighbouring verify_cluster_info in the same file asserts the same way, and
these tests never run under -O.
Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
JunyiXu-nv
force-pushed
the
dev-junyix-fix-disagg-port-conflicts
branch
from
August 10, 2026 08:40
42bc8fd to
8d8e38b
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Collaborator
Author
|
/bot run |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/integration/defs/stress_test/disagg_cancel/harness.py (1)
1893-1910: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the exception handler around
_run_worker.Line 1908 catches
Exception._run_workeropens a config file, opens a log file, and callssubprocess.Popen, so the expected failures areOSErrorandyaml.YAMLError. A broad handler also swallows programming errors such asTypeErrorfrom a signature change and reports them as a respawn failure.♻️ Proposed change
- except Exception: + except (OSError, yaml.YAMLError): logger.exception("[injector] failed to respawn %s_%d", spec.role, spec.index) return FalseAs per coding guidelines: "Catch specific exceptions instead of using broad or bare
except:handlers."🤖 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/integration/defs/stress_test/disagg_cancel/harness.py` around lines 1893 - 1910, In the respawn block around `_run_worker`, replace the broad `except Exception` handler with handling only the expected `OSError` and `yaml.YAMLError` failures. Keep the existing logger and `False` return for those exceptions, while allowing programming errors such as `TypeError` to propagate.Source: Coding guidelines
tests/integration/defs/common.py (1)
674-706: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd type annotations to the new helpers.
The repository guidelines require an annotation on every function.
wait_for_reported_addr,get_ephemeral_port_range,get_static_port_range, andreserve_port_from_rangeare new and unannotated.revise_disaggregated_server_config_urls_with_free_portsin the same module is already annotated, so the annotated style is established here.♻️ Proposed annotations
-def wait_for_reported_addr(addr_path, timeout, process=None): +def wait_for_reported_addr( + addr_path: str, + timeout: float, + process: subprocess.Popen | None = None, +) -> tuple[str, int]:-def get_ephemeral_port_range(): +def get_ephemeral_port_range() -> tuple[int, int] | None:-def get_static_port_range(): +def get_static_port_range() -> tuple[int, int] | None:-def reserve_port_from_range(port_range, source): +def reserve_port_from_range(port_range: tuple[int, int], source: str) -> int | None:As per coding guidelines: "Annotate every function, use
Nonefor procedures".🤖 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/integration/defs/common.py` around lines 674 - 706, Add type annotations to wait_for_reported_addr, get_ephemeral_port_range, get_static_port_range, and reserve_port_from_range, covering every parameter and each return type; use None for procedures and preserve the established annotated style of revise_disaggregated_server_config_urls_with_free_ports.Source: Coding guidelines
🤖 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/integration/defs/perf/test_perf_sanity.py`:
- Around line 1513-1518: Before launching the DISAGG_SERVER command in the
surrounding setup flow, remove the stale address file returned by
_disagg_server_addr_file(server_idx), matching the aggregated path’s cleanup
behavior. Keep the existing --report_addr argument and ensure cleanup occurs for
every retry before the new server publishes its resolved address.
---
Nitpick comments:
In `@tests/integration/defs/common.py`:
- Around line 674-706: Add type annotations to wait_for_reported_addr,
get_ephemeral_port_range, get_static_port_range, and reserve_port_from_range,
covering every parameter and each return type; use None for procedures and
preserve the established annotated style of
revise_disaggregated_server_config_urls_with_free_ports.
In `@tests/integration/defs/stress_test/disagg_cancel/harness.py`:
- Around line 1893-1910: In the respawn block around `_run_worker`, replace the
broad `except Exception` handler with handling only the expected `OSError` and
`yaml.YAMLError` failures. Keep the existing logger and `False` return for those
exceptions, while allowing programming errors such as `TypeError` to propagate.
🪄 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: 8facfe86-7122-42f4-9a17-457eded4e5b3
📒 Files selected for processing (8)
tensorrt_llm/commands/serve.pytests/integration/defs/accuracy/test_disaggregated_serving.pytests/integration/defs/common.pytests/integration/defs/disaggregated/disagg_test_utils.pytests/integration/defs/disaggregated/test_workers.pytests/integration/defs/perf/test_perf_sanity.pytests/integration/defs/stress_test/disagg_cancel/harness.pytests/unittest/api_stability/references/trtllm_serve_cli.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/integration/defs/disaggregated/test_workers.py
- tests/unittest/api_stability/references/trtllm_serve_cli.yaml
- tensorrt_llm/commands/serve.py
Comment on lines
+1513
to
+1518
| # The config carries port 0; publish the resolved address so | ||
| # the BENCHMARK task can find the server. | ||
| disagg_cmd = disagg_cmd + [ | ||
| "--report_addr", | ||
| self._disagg_server_addr_file(server_idx), | ||
| ] |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove a stale disagg address file before launch.
_hostnames_dir is scoped by SLURM_JOB_ID, so a new job never sees the previous job's files. A retry inside the same job with the same server_idx still reads the previous attempt's DISAGG_SERVER.<idx>.addr. The BENCHMARK task would then connect to a dead port. The aggregated path already removes its stale address file at Line 1105.
The DISAGG_SERVER task owns this file exclusively, so removing it here is safe.
🛡️ Proposed fix
self._generate_disagg_server_config(server_idx)
# The config carries port 0; publish the resolved address so
# the BENCHMARK task can find the server.
+ disagg_addr_path = self._disagg_server_addr_file(server_idx)
+ if os.path.exists(disagg_addr_path):
+ os.remove(disagg_addr_path)
disagg_cmd = disagg_cmd + [
"--report_addr",
- self._disagg_server_addr_file(server_idx),
+ disagg_addr_path,
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # The config carries port 0; publish the resolved address so | |
| # the BENCHMARK task can find the server. | |
| disagg_cmd = disagg_cmd + [ | |
| "--report_addr", | |
| self._disagg_server_addr_file(server_idx), | |
| ] | |
| self._generate_disagg_server_config(server_idx) | |
| # The config carries port 0; publish the resolved address so | |
| # the BENCHMARK task can find the server. | |
| disagg_addr_path = self._disagg_server_addr_file(server_idx) | |
| if os.path.exists(disagg_addr_path): | |
| os.remove(disagg_addr_path) | |
| disagg_cmd = disagg_cmd + [ | |
| "--report_addr", | |
| disagg_addr_path, | |
| ] |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 1515-1518: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
🤖 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/integration/defs/perf/test_perf_sanity.py` around lines 1513 - 1518,
Before launching the DISAGG_SERVER command in the surrounding setup flow, remove
the stale address file returned by _disagg_server_addr_file(server_idx),
matching the aggregated path’s cleanup behavior. Keep the existing --report_addr
argument and ensure cleanup occurs for every retry before the new server
publishes its resolved address.
Collaborator
|
PR_Github #65002 [ run ] triggered by Bot. Commit: |
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
2 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.
Summary
Three CI bugs share one shape: a harness picks a port with
get_free_port(), which binds a probe socket, closes it, and hands the number to atrtllm-servethat binds it much later. Anything can take the port in between — including the test's own sibling workers launched with--port 0, drawing from the same ephemeral pool.This PR closes that race for the sites those bugs land on, and fixes a second, unrelated cause hiding behind the same
EADDRINUSE.1.
SO_REUSEADDR— a different bug with the same error messagelaunch_serverand the disaggregated server bound their sockets withoutSO_REUSEADDR, so after a server exits, the TIME_WAIT tombstones of the connections it accepted refuse a rebind of that port for ~60s. This is not a race, and no amount of port juggling avoids it. It is why nvbugs/6435121's diagnostic reads127.0.0.1:10879 status=TIME_WAIT.Measured matrix — the flag must be set on the socket that owned the port first, because the TIME_WAIT entry inherits it:
Set unconditionally on all three HTTP bind sites. The main beneficiary is the product path, where users pass an explicit
--portand restart —--port 0is immune to TIME_WAIT anyway (verified: 20000bind(0)calls with 61 TIME_WAIT ports present, zero failures).2.
--report_addr— eliminating the reservation windowWith
--port 0the kernel assigns the port, the socket stays bound from that moment until uvicorn takes it over, and the resolvedhost:portis published atomically (temp file +rename, so a reader never sees a partial line — that matters on the shared filesystems multi-node tests coordinate through).This is the same shape the KV cache transceiver already uses:
ucxCacheCommunicator.cpp:352bindstcp://<ip>:*, reads the port back fromZMQ_LAST_ENDPOINT, and ships it to the peer in-band viaCommState→opaque_state. That path has never produced a port conflict, because no port in it is chosen by a process other than the one that binds it.Why this works multi-node: reservation is inherently host-local, but publication is not. Every site that picks a port does so on the node that will bind it — including perf sanity, where each SLURM task launches its own local server — so only the resolved address has to cross nodes.
--report_addris rejected for the gRPC and VisualGen servers and for the disagg fleet topologies rather than silently never being written: withnum_workers>1the port goes to NSO_REUSEPORTworkers, which under port 0 would each get a different kernel-assigned port instead of sharing one.3. Migrated sites
test_disaggregated_serving.py(nvbugs/6567057, nvbugs/6435121) — starts the disagg server first with--port 0, reads back the address, and only then writes the worker configs carrying the resolvedcluster_uri. The server's own copy ofcluster_urikeeps a placeholder port becauseHttpClusterStorageServer.__init__serves the storage on the server's own port and never reads the URI; only workers dial it.test_perf_sanity.py(nvbugs/6526529) — all three sites (aggregated server, disagg server, CTX/GEN workers) now use--port 0 --report_addr. The cross-node channel is unchanged in shape; thosehost:portfiles are now written by the servers rather than guessed by the harness. Two prerequisites this needed:SLURM_JOB_ID.test_output_diris derived from the test case name alone and created withexist_ok=True, so a rerun reused it — once files are server-written, a leftover from a previous run points at a dead worker, which fails far less obviously than a port conflict. The step id is deliberately excluded, since each role is a separate srun step within one job.*.txt, or the transient.tmprename siblings would inflate the expected-count check and get parsed as worker urls.Earlier commit (test-only) —
test_workers.py::background_workersand thedisagg_cancelrespawn path now launch workers withport=0and read URLs back from/cluster_info;get_free_port_in_cigained a fallback below the ephemeral range for the SLURM path, whereCONTAINER_PORT_STARTis never set (jenkins/L0_Test.groovy:1200is the only setter, and it is the single-node container path).Scope — what is not fixed
Other sites still pre-pick, now only from a safer range:
test_disaggregated.py:683,694,test_workers.py:571,disagg_test_utils.py:426,test_ad_disagg_trtllm_serve.py:184,test_dwdp_disaggregated_serving.py, andRemoteOpenAIServer(tests/unittest/llmapi/apps/openai_server.py:35, 38 dependent files).Separately, the
MASTER_PORT/ c10d TCPStore sites cannot use--port 0as written, but are fixable by the same pattern — rank 0 createsTCPStore(host, 0, ...), which binds and holds, then broadcastsstore.portover thempi_broadcast/pipe channel these sites already have. VerifiedTCPStoreexposes.porton the pinned torch. Follow-up.Test Coverage
Product change is small and mechanical; the risk sits in the test migrations, which need GPUs and have not been run locally — CI is the real verification.
Verified directly here:
nvl72d066-T01), IPv4, fast-fail when the server process dies, and timeout.CTX_*,GEN_*,DISAGG_SERVER,BENCHMARK) agree on the coordination dir withSLURM_STEP_IDset differently per role; a differentSLURM_JOB_IDyields a different dir; the.txtfilter excludes the disagg.addrfile.SO_REUSEADDRandbind(0)-vs-TIME_WAIT matrices above.pre-commitpasses on all touched files;trtllm_serve_cli.yamlupdated for both subcommands.Affected CI:
disaggregated/test_workers.py→A10-PyTorch-{1,2,3},DGX_B200-PyTorch-*,DGX_H100-*-PyTorch-Others-*; plusaccuracy/test_disaggregated_serving.pyand the perf-sanity disagg stages.PR Checklist
[JIRA/NVBUG/None][type] Summary🤖 Generated with Claude Code
Dev Engineer Review
--report_addrsupport with validation for unsupported server topologies.SO_REUSEADDRfor reliable server restarts.port=0, service discovery, and reported addresses.serve.report_addranddisaggregated.report_addr.MASTER_PORT/TCPStore usage remain for follow-up.QA Engineer Review
tests/integration/defs/accuracy/test_disaggregated_serving.pytests/integration/defs/common.pytests/integration/defs/disaggregated/disagg_test_utils.pytests/integration/defs/disaggregated/test_workers.pytests/integration/defs/perf/test_perf_sanity.pytests/integration/defs/stress_test/disagg_cancel/harness.pytests/integration/test_lists/,test-db/, orqa/files changed.