[https://nvbugs/6435121][fix] Eliminate the trtllm-serve port reservation race with --port 0 + --report_addr

[JunyiXu-nv]

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 a trtllm-serve that 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 message

launch_server and the disaggregated server bound their sockets without SO_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 reads 127.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:

original SO_REUSEADDR=False  new SO_REUSEADDR=False -> rebind FAIL (98)
original SO_REUSEADDR=False  new SO_REUSEADDR=True  -> rebind FAIL (98)
original SO_REUSEADDR=True   new SO_REUSEADDR=False -> rebind FAIL (98)
original SO_REUSEADDR=True   new SO_REUSEADDR=True  -> rebind OK

Set unconditionally on all three HTTP bind sites. The main beneficiary is the product path, where users pass an explicit --port and restart — --port 0 is immune to TIME_WAIT anyway (verified: 20000 bind(0) calls with 61 TIME_WAIT ports present, zero failures).

2. --report_addr — eliminating the reservation window

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 — that matters on the shared filesystems multi-node tests coordinate through).

This is the same shape the KV cache transceiver already uses: ucxCacheCommunicator.cpp:352 binds tcp://<ip>:*, reads the port back from ZMQ_LAST_ENDPOINT, and ships it to the peer in-band via CommState → 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_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.

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 resolved cluster_uri. The server's own copy of cluster_uri keeps a placeholder port because HttpClusterStorageServer.__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; those host:port files are now written by the servers rather than guessed by the harness. Two prerequisites this needed:

• 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 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.
• The directory scan filters to *.txt, or the transient .tmp rename siblings would inflate the expected-count check and get parsed as worker urls.

Earlier commit (test-only) — test_workers.py::background_workers and the disagg_cancel respawn path now launch workers with port=0 and read URLs back from /cluster_info; get_free_port_in_ci gained a fallback below the ephemeral range for the SLURM path, where CONTAINER_PORT_START is never set (jenkins/L0_Test.groovy:1200 is 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, and RemoteOpenAIServer (tests/unittest/llmapi/apps/openai_server.py:35, 38 dependent files).

Separately, the MASTER_PORT / c10d TCPStore sites cannot use --port 0 as written, but are fixable by the same pattern — rank 0 creates TCPStore(host, 0, ...), which binds and holds, then broadcasts store.port over the mpi_broadcast/pipe channel these sites already have. Verified TCPStore exposes .port on 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:

• Publisher: atomic across 2000 concurrent overwrites (no partial reads, no leftover temp files), creates parent dirs, no-ops when unset.
• Publisher/reader pair end to end, including delayed writes, hyphenated hostnames (nvl72d066-T01), IPv4, fast-fail when the server process dies, and timeout.
• perf sanity path algebra: all four roles (CTX_*, GEN_*, DISAGG_SERVER, BENCHMARK) agree on the coordination dir with SLURM_STEP_ID set differently per role; a different SLURM_JOB_ID yields a different dir; the .txt filter excludes the disagg .addr file.
• SO_REUSEADDR and bind(0)-vs-TIME_WAIT matrices above.
• pre-commit passes on all touched files; trtllm_serve_cli.yaml updated for both subcommands.

Affected CI: disaggregated/test_workers.py → A10-PyTorch-{1,2,3}, DGX_B200-PyTorch-*, DGX_H100-*-PyTorch-Others-*; plus accuracy/test_disaggregated_serving.py and the perf-sanity disagg stages.

PR Checklist

• PR title follows [JIRA/NVBUG/None][type] Summary
• Commits are DCO signed off
• API stability reference updated for the new CLI option

🤖 Generated with Claude Code

Dev Engineer Review

• Added atomic --report_addr publication and SO_REUSEADDR.
• Added port reservation helpers and a static-range CI fallback.
• Updated disaggregated serving, worker respawn, and performance tests to use reported addresses and service discovery.
• Added validation for unsupported --port 0 and --report_addr topologies.
• Added CLI API stability references.
• Pre-commit and direct verification passed. GPU integration validation remains pending.
• Existing pre-selected ports and MASTER_PORT/TCPStore usage remain out of scope.

QA Engineer Review

• Added 11 unit tests in tests/unittest/llmapi/test_serve_report_addr.py.
• Coverage includes address publication, IPv4/IPv6 and wildcard handling, atomic reads, cleanup, delayed writes, timeout and process-failure behavior, and launch_server validation.
• Updated disaggregated serving, worker, performance, stress, and shared integration test utilities.
• Added unittest/llmapi/test_serve_report_addr.py to tests/integration/test_lists/test-db/l0_cpu.yml.
• The new unit tests have CI test-list coverage.
• Verdict: sufficient.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

The 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 /cluster_info.

Changes

Dynamic address publication and worker discovery

Layer / File(s)	Summary
Server address publication tensorrt_llm/commands/serve.py, tests/unittest/api_stability/references/trtllm_serve_cli.yaml	HTTP and single-server disaggregated modes support --report_addr and port zero. The server publishes the finalized address atomically and rejects unsupported modes.
Address waiting and port allocation tests/integration/defs/common.py	Helpers wait for reported addresses, detect early process exit, reserve ports from configured ranges, and use a static-range fallback.
Disaggregated startup coordination tests/integration/defs/accuracy/test_disaggregated_serving.py, tests/integration/defs/perf/test_perf_sanity.py	Test workflows start servers before workers, wait for reported addresses, generate configuration from resolved endpoints, and publish worker addresses.
Registered worker endpoint discovery tests/integration/defs/disaggregated/disagg_test_utils.py, tests/integration/defs/disaggregated/test_workers.py	Workers bind to OS-assigned ports. Tests retrieve sorted worker URLs from /cluster_info and validate worker counts.
Respawn registration and health verification tests/integration/defs/stress_test/disagg_cancel/harness.py	Respawned workers preserve their indices. The harness matches registration by PID and performs health checks within the remaining timeout.
Address reporting validation tests/unittest/llmapi/test_serve_report_addr.py, tests/integration/test_lists/test-db/l0_cpu.yml	CPU tests cover address publication, atomic reads, timeout handling, launcher validation, and test-suite registration.

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

Loading

Suggested reviewers: bowenfu, qijune, shixiaowei02

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 68.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the bug fix and the main solution using --port 0 and --report_addr.
Description check	✅ Passed	The description explains the problem, solution, scope, testing, affected CI, and checklist items in sufficient detail.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (3)

tests/integration/defs/disaggregated/disagg_test_utils.py (1)

325-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add type annotations to the new URL-discovery helper.

Annotate get_registered_worker_urls() and nested _urls(). The public return type is tuple[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 win

Add type annotations to the new helper functions.

Annotate get_ephemeral_port_range(), get_static_port_range(), reserve_port_from_range(), and get_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 win

Preformat the new injector log messages.

tensorrt_llm.logger joins arguments. It does not apply %s or %d interpolation. 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.logger joins 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

📥 Commits

Reviewing files that changed from the base of the PR and between 0fa708b and d642921.

📒 Files selected for processing (4)

• tests/integration/defs/common.py
• tests/integration/defs/disaggregated/disagg_test_utils.py
• tests/integration/defs/disaggregated/test_workers.py
• tests/integration/defs/stress_test/disagg_cancel/harness.py

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

tests/integration/defs/common.py (1)

674-706: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add annotations to the new helper functions.

• tests/integration/defs/common.py#L674-L706: annotate addr_path, timeout, process, and the tuple[str, int] return value.
• tests/integration/defs/accuracy/test_disaggregated_serving.py#L221-L231: annotate cluster_uri as str and the return value as None.

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 win

Replace the broad cleanup handler.

Line 367 catches BaseException. This also catches KeyboardInterrupt and SystemExit.

Use finally for 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

📥 Commits

Reviewing files that changed from the base of the PR and between d642921 and 42bc8fd.

📒 Files selected for processing (5)

• tensorrt_llm/commands/serve.py
• tests/integration/defs/accuracy/test_disaggregated_serving.py
• tests/integration/defs/common.py
• tests/integration/defs/perf/test_perf_sanity.py
• tests/unittest/api_stability/references/trtllm_serve_cli.yaml

[coderabbitai]

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.

[JunyiXu-nv]

/bot run

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

tests/integration/defs/stress_test/disagg_cancel/harness.py (1)

1893-1910: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Narrow the exception handler around _run_worker.

Line 1908 catches Exception. _run_worker opens a config file, opens a log file, and calls subprocess.Popen, so the expected failures are OSError and yaml.YAMLError. A broad handler also swallows programming errors such as TypeError from 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 False

As 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 value

Add 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, and reserve_port_from_range are new and unannotated. revise_disaggregated_server_config_urls_with_free_ports in 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 None for 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

📥 Commits

Reviewing files that changed from the base of the PR and between f13a0be and 8d8e38b.

📒 Files selected for processing (8)

• tensorrt_llm/commands/serve.py
• tests/integration/defs/accuracy/test_disaggregated_serving.py
• tests/integration/defs/common.py
• tests/integration/defs/disaggregated/disagg_test_utils.py
• tests/integration/defs/disaggregated/test_workers.py
• tests/integration/defs/perf/test_perf_sanity.py
• tests/integration/defs/stress_test/disagg_cancel/harness.py
• tests/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

[tensorrt-cicd]

PR_Github #65002 [ run ] triggered by Bot. Commit: 8d8e38b Link to invocation

[tensorrt-cicd]

PR_Github #65002 [ run ] completed with state SUCCESS. Commit: 8d8e38b
/LLM/main/L0_MergeRequest_PR pipeline #52817 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

• Please check the failed tests and fix your PR
• If you cannot view the failures, ask the CI triggerer to share details
• Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

[brnguyen2]

Approving — the comments below are optional touch-ups, not blockers.

The two-cause separation (TOCTOU race vs TIME_WAIT) with the measured SO_REUSEADDR matrix is exactly the level of analysis these flaky-port bugs needed, and I verified the key claims in-tree (HttpClusterStorageServer.__init__ ignoring the URI, pid-embedded worker_id matching, no index-sensitive consumers of the now-sorted worker URLs).

One gap worth closing before merge (inline): the num_serve_frontends > 1 path has the same port-0 hazard the disagg fleet rejection guards against, but is not rejected.

Smaller items:

• No unit tests for the new protocol. _publish_bound_address + wait_for_reported_addr round-trip (including the IPv6-bracket and crashed-process paths) and the two click.BadParameter rejections are all cheaply testable with tmp_path and CliRunner; right now the only coverage is indirect via GPU integration tests.
• The PR description is cut off mid-sentence in section 3 ("The step id is de") — worth fixing since the rest of it is a useful reference for the next person who hits an Errno 98 in CI.

[fredricz-20070104]

Review summary - CONCERNS

Verdict: The two-cause analysis (reservation TOCTOU vs TIME_WAIT) is correct and the --port 0 + --report_addr + SO_REUSEADDR design fixes the targeted bug sites by construction, but there is one unhandled combination and no unit-level coverage of the new product code, so I'd close those before merge.

Concerns

1. [MAJOR] tensorrt_llm/commands/serve.py:~1602 - --report_addr not rejected for multi-frontend

   • What is wrong: serve() rejects --report_addr only for the gRPC and VisualGen servers, and disaggregated() rejects the SO_REUSEPORT fleet. But launch_server's multi-frontend path (each frontend binds its own socket on the same port) has the identical port-0 hazard the fleet guard exists for.
   • How it fails: run trtllm-serve ... --port 0 --report_addr f with num_serve_frontends > 1; each frontend child gets a different kernel-assigned port, _publish_bound_address publishes only the launcher socket's port, and a reader connecting to it hits a port that serves at best 1/N of requests → intermittent connection failures.
   • Suggested fix: mirror the disagg-fleet guard — reject --port 0/--report_addr when more than one frontend is configured, or only publish once a single shared socket is guaranteed inherited by all frontends.

2. [MAJOR] tensorrt_llm/commands/serve.py:641 - new publisher/rejection logic has no unit test

   • What is wrong: _publish_bound_address, the SO_REUSEADDR sites, wait_for_reported_addr, and the two click.BadParameter rejections are only exercised indirectly through GPU integration tests, which the PR says were not run locally.
   • How it fails: a regression in the publish/parse round-trip (IPv6 bracketing, dead-process fast-fail, timeout) or a mis-scoped rejection ships untested.
   • Suggested fix: add a tmp_path + CliRunner unit test covering the publish/read round-trip (IPv4, IPv6, crashed process, timeout) and the grpc/visual_gen/fleet rejections.

Minor notes (non-blocking)

• tensorrt_llm/commands/serve.py:367 - except BaseException for temp-file cleanup; a finally: expresses the intent without catching KeyboardInterrupt/SystemExit.
• tests/integration/defs/stress_test/disagg_cancel/harness.py:1908 - broad except Exception around _run_worker also masks a TypeError from the new worker_index= arg; narrow to (OSError, yaml.YAMLError).
• tests/integration/defs/disaggregated/disagg_test_utils.py:326 and the new common.py helpers lack type annotations required by the repo guidelines.

QA view

• Test coverage: partial - integration migrations touch the changed paths but need GPUs and were not run locally; no unit test covers the publisher, reader, or the two rejection branches.
• SM coverage: architecture-independent - pure socket/port/file coordination, no arch guards or fp8/nvfp4 paths.
• Test code: missing annotations; broad except Exception in the respawn path; migrations unverified locally.
• Test time: unknown - polling loops (sleep(0.5)/sleep(10)) replace fixed-port logic; no new parametrisation or model, so no clear signal from the diff.
• Needs /qa-verify: yes - intermittent-race fix with no reproducing test, integration migrations not run locally, and changes to perf_sanity SLURM coordination + disagg_cancel respawn infra that only CI can exercise.

Does this actually fix nvbugs/6435121, 6567057, 6526529?

Partial. The mechanism is correct: --port 0 keeps the socket bound from kernel assignment through uvicorn takeover (killing the reservation TOCTOU), and SO_REUSEADDR on all three bind sites addresses the TIME_WAIT rebind failure the 6435121 diagnostic showed. This is applied to exactly the sites the three bugs land on. But there is no test that reproduces the original intermittent failure, the migrations were not run locally, and other pre-picking sites are explicitly left racing (now from a safer range). Fixed by construction, unverified in practice.

Possible new issues

• If launch_server does not reassign port = s.getsockname()[1] after binding with port 0 (the diff shows this only in the disaggregated command, not in launch_server), _publish_bound_address would publish 0. Please confirm the resolution happens before the publish call.
• perf_sanity now reads address files written by trtllm-serve with a trailing newline (host:port\n), where the old _generate_hostname_file wrote none; confirm the file-read in _generate_disagg_server_config strips whitespace before parsing the port.

What I could not verify

• The elided launch_server binding region between the two hunks — specifically whether port is resolved from getsockname() before _publish_bound_address.
• Whether the default num_serve_frontends is 1 (making the multi-frontend gap opt-in) or greater (making it a common-path defect).
• The whitespace-stripping in perf_sanity's hostname-file read (outside the shown hunk).

Automated review by NVCortex Lite, run by @fredricz-20070104.

[fredricz-20070104]

Review summary - Approve (non-blocking)

Approving so this is not blocked on me. The points raised in my review comment above are non-blocking — please read them and address what you agree with before merging.

Worth doing before this is relied on: It is a fix for an intermittent port race with no test that reproduces the original failure, the integration migrations were not run locally, and it touches test infrastructure (perf_sanity SLURM coordination, disagg_cancel respawn) plus multi-node coordination paths that only CI can exercise.

Automated review by NVCortex Lite, run by @fredricz-20070104.

[JunyiXu-nv]

Rebased onto main to pick up the infrastructure fix. Re-running with fail-fast disabled.

[JunyiXu-nv]

/bot run --disable-fail-fast

[tensorrt-cicd]

PR_Github #65869 [ run ] triggered by Bot. Commit: a67e6f6 Link to invocation

[tensorrt-cicd]

PR_Github #65866 [ run ] completed with state ABORTED. Commit: fabf48d

Link to invocation

[JunyiXu-nv]

Rebased onto main again, now including the mypy fix from #17637. Re-running with fail-fast disabled.

[JunyiXu-nv]

/bot run --disable-fail-fast

[tensorrt-cicd]

PR_Github #65936 [ run ] triggered by Bot. Commit: a28b00b Link to invocation

[tensorrt-cicd]

PR_Github #65869 [ run ] completed with state ABORTED. Commit: a67e6f6

Link to invocation

[tensorrt-cicd]

PR_Github #65936 [ run ] completed with state FAILURE. Commit: a28b00b
/LLM/main/L0_MergeRequest_PR pipeline #53625 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

• Please check the failed tests and fix your PR
• If you cannot view the failures, ask the CI triggerer to share details
• Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

[JunyiXu-nv]

Status after the last run on a28b00bb3a, which finished four days ago.

The coverage this pull request was waiting for has now passed. [Test-SBSA-Multi-GPU] succeeded with all three multi-node disaggregated perf sanity stages green:

• GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-FUNCTIONAL-ONLY-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-1
• GB300-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-FUNCTIONAL-ONLY-CTX1-NODE1-GPU2-GEN1-NODE2-GPU8-1
• GB300-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-FUNCTIONAL-ONLY-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-1

Those exercise the address publication path across two and three nodes: port 0 with --report_addr on the context and generation workers and on the disaggregated server, the job-scoped address directory, and the benchmark task reading the reported address.

Why the pipeline is still red: x86 single-GPU had 7 failures out of 53933, none in code this pull request touches.

• Three cpp.test_unit_tests cases failed at setup on a cmake invocation
• kv_cache.test_kv_cache_v2_scheduler and test_e2e::test_ptp_quickstart_bert both terminated unexpectedly
• Two AutoDeploy cases failed on a CUDA out-of-memory during setup

x86 multi-GPU was then blocked by the rule that multi-GPU requires single-GPU to pass, which fail-fast being disabled does not bypass, so DGX_B200-16_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-FUNCTIONAL-ONLY-CTX1-NODE1-GPU4-GEN1-NODE1-GPU8-1 still has not run. The SBSA stages above cover the same code paths at two and three nodes.

Re-running.

[JunyiXu-nv]

/bot run --disable-fail-fast

[tensorrt-cicd]

PR_Github #66600 [ run ] triggered by Bot. Commit: a28b00b Link to invocation

[tensorrt-cicd]

PR_Github #66600 [ run ] completed with state SUCCESS. Commit: a28b00b
/LLM/main/L0_MergeRequest_PR pipeline #54223 completed with status: 'SUCCESS'
Pipeline passed with automatic retried tests. Check the rerun report for details.

CI Report

Link to invocation