[https://nvbugs/6208457][fix] Pass IPC HMAC key through file descriptor#15654
Conversation
|
/bot help |
GitHub Bot Help
Provide a user friendly way for developers to interact with a Jenkins server. Run See details below for each supported subcommand. Details
Launch build/test pipelines. All previously running jobs will be killed.
kill
Kill all running builds associated with pull request. skip
Skip testing for latest commit on pull request. reuse-pipeline
Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break. |
yibinl-nvidia
force-pushed
the
dev-yibinl-remove-hmac-key-env-variable
branch
from
05e7f7e to
a49a94e
Compare
|
/bot run --post-merge |
|
PR_Github #56000 [ run ] triggered by Bot. Commit: |
|
PR_Github #56000 [ run ] completed with state
|
|
/bot run --post-merge |
|
PR_Github #56092 [ run ] triggered by Bot. Commit: |
|
PR_Github #56092 [ run ] completed with state
|
|
/bot run --post-merge |
|
PR_Github #56097 [ run ] triggered by Bot. Commit: |
|
PR_Github #56097 [ run ] completed with state
|
|
/bot run --post-merge |
|
PR_Github #56099 [ run ] triggered by Bot. Commit: |
|
PR_Github #56099 [ run ] completed with state
|
|
/bot run --post-merge |
|
PR_Github #56111 [ run ] triggered by Bot. Commit: |
|
PR_Github #56111 [ run ] completed with state
|
|
/bot run --post-merge |
|
PR_Github #56126 [ run ] triggered by Bot. Commit: |
|
PR_Github #56126 [ run ] completed with state
|
|
/bot run --post-merge |
|
PR_Github #56150 [ run ] triggered by Bot. Commit: |
|
PR_Github #56150 [ run ] completed with state
|
yibinl-nvidia
force-pushed
the
dev-yibinl-remove-hmac-key-env-variable
branch
from
b6d0fcc to
1176c37
Compare
|
/bot run --post-merge |
|
PR_Github #56177 [ run ] triggered by Bot. Commit: |
|
PR_Github #56177 [ run ] completed with state
|
|
/bot run --post-merge |
Signed-off-by: Yibin Li <109242046+yibinl-nvidia@users.noreply.github.com>
yibinl-nvidia
force-pushed
the
dev-yibinl-remove-hmac-key-env-variable
branch
from
1176c37 to
a2909be
Compare
📝 WalkthroughWalkthroughThis PR changes how the spawn-proxy-process IPC HMAC key is delivered to child processes, replacing environment-variable hex-string export with a file-descriptor based transfer. Affected areas include serve.py's disaggregated leader spawn logic, executor utils' key caching/normalization helpers, the trtllm-llmapi-launch script's new run_with_ipc_hmac_key wrapper, and stricter validation (ValueError instead of assert) in ZeroMqQueue and GenerationExecutorWorker. New unit tests cover FD-based key retrieval and validation failures. ChangesIPC HMAC Key FD-Based Delivery
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/unittest/executor/test_ipc.py (1)
260-271: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCoverage is insufficient for the worker-side HMAC guard.
This adds the right regression test for
ZeroMqQueue(..., use_hmac_encryption=False), but the PR also hardensGenerationExecutorWorker.__init__intensorrt_llm/executor/worker.py. Please add a companion negative test intests/unittest/executor/test_rpc_proxy.pyor a focusedtests/unittest/executor/test_worker.pythat passesrpc_addrwith an emptyhmac_keyand asserts the newValueError("hmac_key is required when rpc_addr is set"). Based on path instructions, test feedback fortests/**should state whether coverage is sufficient and name follow-up files.🤖 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/executor/test_ipc.py` around lines 260 - 271, The new coverage only exercises ZeroMqQueue and does not cover the worker-side HMAC guard in GenerationExecutorWorker.__init__. Add a negative test in tests/unittest/executor/test_rpc_proxy.py or tests/unittest/executor/test_worker.py that constructs GenerationExecutorWorker with rpc_addr set and an empty hmac_key, then asserts the ValueError message "hmac_key is required when rpc_addr is set". Keep the existing ZeroMqQueue regression test, but update the tests/** coverage note to indicate the worker-side case is now covered and name the follow-up test file explicitly.Source: Path instructions
tests/unittest/executor/test_launcher_envs.py (2)
24-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCoverage is insufficient for malformed key validation.
The happy-path FD/cache cases are covered, but
tests/unittest/executor/test_launcher_envs.pyshould also include negative cases for malformed FD payloads, such as short hex, non-hex ASCII, and the directset_spawn_proxy_process_ipc_hmac_key()cache path used byserve.py.As per path instructions, coverage for this test change is currently insufficient and should be extended in
tests/unittest/executor/test_launcher_envs.py.🤖 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/executor/test_launcher_envs.py` around lines 24 - 79, Coverage in executor_utils.get_spawn_proxy_process_ipc_hmac_key_env is missing malformed-input and cache-path validation. Extend the tests in test_launcher_envs to add negative cases for an FD containing short hex and non-hex ASCII, and also cover the direct set_spawn_proxy_process_ipc_hmac_key cache path used by serve.py. Reuse the existing helpers like _reset_ipc_hmac_key_env and the LlmLauncherEnvs.TLLM_SPAWN_PROXY_PROCESS_IPC_HMAC_KEY_FD setup to locate the affected flow.Source: Path instructions
10-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd return annotations to the new test functions.
Use
-> Nonefor helpers/tests that do not return, including the nestedwrite_key().Proposed refactor
-def _reset_ipc_hmac_key_env(monkeypatch): +def _reset_ipc_hmac_key_env(monkeypatch) -> None: ... -def test_get_spawn_proxy_process_ipc_hmac_key_from_fd(monkeypatch): +def test_get_spawn_proxy_process_ipc_hmac_key_from_fd(monkeypatch) -> None: ... -def test_get_spawn_proxy_process_ipc_hmac_key_caches_fd_key(monkeypatch): +def test_get_spawn_proxy_process_ipc_hmac_key_caches_fd_key(monkeypatch) -> None: ... -def test_get_spawn_proxy_process_ipc_hmac_key_from_nonblocking_fd(monkeypatch): +def test_get_spawn_proxy_process_ipc_hmac_key_from_nonblocking_fd(monkeypatch) -> None: ... - def write_key(): + def write_key() -> None: ... -def test_get_spawn_proxy_process_ipc_hmac_key_missing(monkeypatch): +def test_get_spawn_proxy_process_ipc_hmac_key_missing(monkeypatch) -> None:As per coding guidelines, “Always annotate functions. Make the return type
Noneif the function does not return anything.”🤖 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/executor/test_launcher_envs.py` around lines 10 - 79, The new test helpers and test cases in test_launcher_envs.py are missing return type annotations. Add explicit return annotations to _reset_ipc_hmac_key_env, _write_key_fd, test_get_spawn_proxy_process_ipc_hmac_key_from_fd, test_get_spawn_proxy_process_ipc_hmac_key_caches_fd_key, test_get_spawn_proxy_process_ipc_hmac_key_from_nonblocking_fd, and test_get_spawn_proxy_process_ipc_hmac_key_missing, and use -> None for helpers/tests that do not return values. Also annotate the nested write_key() helper inside test_get_spawn_proxy_process_ipc_hmac_key_from_nonblocking_fd with -> None.Source: Coding guidelines
tensorrt_llm/executor/utils.py (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse snake_case for the mutable cache.
_SPAWN_PROXY_PROCESS_IPC_HMAC_KEYis reassigned by the setter/getter, so it is mutable module state rather than a constant. Rename it to snake_case to match the Python naming guideline.Proposed refactor
-_SPAWN_PROXY_PROCESS_IPC_HMAC_KEY: bytes | None = None +_spawn_proxy_process_ipc_hmac_key: bytes | None = NoneAs per coding guidelines, “Local Variables or Mutable Global Variables” should use
snake_case, while constants useUPPER_SNAKE_CASE.🤖 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/executor/utils.py` at line 33, The module-level cache currently uses UPPER_SNAKE_CASE even though it is reassigned by the getter/setter, so rename `_SPAWN_PROXY_PROCESS_IPC_HMAC_KEY` to snake_case and update every reference in `utils.py` that reads or writes it. Make sure the change stays consistent in the related getter/setter logic so the mutable global state follows the Python naming guideline.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 1550-1551: The environment cleanup in serve.py only removes
TLLM_SPAWN_PROXY_PROCESS_IPC_HMAC_KEY_FD before split_mpi_env(), but the legacy
raw HMAC-key variable can still leak into the child via non_mpi_env. Update the
cleanup around the spawn setup to also pop TLLM_SPAWN_PROXY_PROCESS_IPC_HMAC_KEY
alongside the FD env var, using the same env-handling block in the serve() flow
so both secrets are stripped before forwarding.
In `@tensorrt_llm/executor/utils.py`:
- Around line 54-67: The FD payload reader in
_read_spawn_proxy_process_ipc_hmac_key_fd is treating the collected bytes as raw
key material instead of ASCII hex, which can let malformed FD input slip
through. Update this helper to decode the assembled FD payload as ASCII/hex
first, then pass the decoded key into
_normalize_spawn_proxy_process_ipc_hmac_key so invalid non-hex or truncated FD
data is rejected. Keep the change localized to the FD read/normalize flow in
_read_spawn_proxy_process_ipc_hmac_key_fd and the normalization call it feeds.
In `@tensorrt_llm/llmapi/trtllm-llmapi-launch`:
- Line 43: The launch wrapper currently clears only
TLLM_SPAWN_PROXY_PROCESS_IPC_HMAC_KEY_FD, so the legacy raw HMAC key can still
leak into child processes. Update the environment cleanup in
trtllm-llmapi-launch to also unset TLLM_SPAWN_PROXY_PROCESS_IPC_HMAC_KEY
alongside the existing unset, keeping the same wrapper behavior for wrapped
commands.
---
Nitpick comments:
In `@tensorrt_llm/executor/utils.py`:
- Line 33: The module-level cache currently uses UPPER_SNAKE_CASE even though it
is reassigned by the getter/setter, so rename
`_SPAWN_PROXY_PROCESS_IPC_HMAC_KEY` to snake_case and update every reference in
`utils.py` that reads or writes it. Make sure the change stays consistent in the
related getter/setter logic so the mutable global state follows the Python
naming guideline.
In `@tests/unittest/executor/test_ipc.py`:
- Around line 260-271: The new coverage only exercises ZeroMqQueue and does not
cover the worker-side HMAC guard in GenerationExecutorWorker.__init__. Add a
negative test in tests/unittest/executor/test_rpc_proxy.py or
tests/unittest/executor/test_worker.py that constructs GenerationExecutorWorker
with rpc_addr set and an empty hmac_key, then asserts the ValueError message
"hmac_key is required when rpc_addr is set". Keep the existing ZeroMqQueue
regression test, but update the tests/** coverage note to indicate the
worker-side case is now covered and name the follow-up test file explicitly.
In `@tests/unittest/executor/test_launcher_envs.py`:
- Around line 24-79: Coverage in
executor_utils.get_spawn_proxy_process_ipc_hmac_key_env is missing
malformed-input and cache-path validation. Extend the tests in
test_launcher_envs to add negative cases for an FD containing short hex and
non-hex ASCII, and also cover the direct set_spawn_proxy_process_ipc_hmac_key
cache path used by serve.py. Reuse the existing helpers like
_reset_ipc_hmac_key_env and the
LlmLauncherEnvs.TLLM_SPAWN_PROXY_PROCESS_IPC_HMAC_KEY_FD setup to locate the
affected flow.
- Around line 10-79: The new test helpers and test cases in
test_launcher_envs.py are missing return type annotations. Add explicit return
annotations to _reset_ipc_hmac_key_env, _write_key_fd,
test_get_spawn_proxy_process_ipc_hmac_key_from_fd,
test_get_spawn_proxy_process_ipc_hmac_key_caches_fd_key,
test_get_spawn_proxy_process_ipc_hmac_key_from_nonblocking_fd, and
test_get_spawn_proxy_process_ipc_hmac_key_missing, and use -> None for
helpers/tests that do not return values. Also annotate the nested write_key()
helper inside test_get_spawn_proxy_process_ipc_hmac_key_from_nonblocking_fd with
-> None.
🪄 Autofix (Beta)
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: 610becb8-566e-4569-bbf1-2eff69b88f43
📒 Files selected for processing (7)
tensorrt_llm/commands/serve.pytensorrt_llm/executor/ipc.pytensorrt_llm/executor/utils.pytensorrt_llm/executor/worker.pytensorrt_llm/llmapi/trtllm-llmapi-launchtests/unittest/executor/test_ipc.pytests/unittest/executor/test_launcher_envs.py
Signed-off-by: Yibin Li <109242046+yibinl-nvidia@users.noreply.github.com>
|
/bot run --post-merge --disable-fail-fast |
|
PR_Github #56786 [ run ] triggered by Bot. Commit: |
|
PR_Github #56786 [ run ] completed with state
|
|
/bot run --post-merge --disable-fail-fast |
|
PR_Github #56895 [ run ] triggered by Bot. Commit: |
|
PR_Github #56895 [ run ] completed with state |
|
/bot run --post-merge --disable-fail-fast |
|
PR_Github #56936 [ run ] triggered by Bot. Commit: |
|
PR_Github #56936 [ run ] completed with state
|
|
/bot run --post-merge --disable-fail-fast |
|
PR_Github #56975 [ run ] triggered by Bot. Commit: |
|
/bot run |
|
PR_Github #57113 [ run ] triggered by Bot. Commit: |
|
PR_Github #56975 [ run ] completed with state |
|
PR_Github #57113 [ run ] completed with state |
…escriptor (NVIDIA#15654) Reverts NVIDIA#15654 (48fc753). PR NVIDIA#15654 passes the IPC HMAC key from the parent process to the LLM-API proxy child via a file descriptor. In the QA-pytest Slurm/MPI launch path used by trtllm-bench and trtllm-serve, the fd handshake between Rank0's forked subshell (spawning internal mpirun) and the mgmn_leader_node RemoteMpiCommSessionServer deadlocks silently: - trtllm-bench (nvbugs/6388787): after "start MpiSession with N workers" the bench process emits no further stdout for ~30 min, workers stay alive at 0% GPU / 4 MiB VRAM, no exception, no MPI abort. Perf harness SIGKILLs at _STALL_TIMEOUT=1800s. - trtllm-serve (nvbugs/6405747): serve mode's /health endpoint never binds, and pytest fails with "Server http://localhost:<port>/health did not become ready within 3600s" on both baseline and candidate wheels. This is the third time the "Pass IPC HMAC key via fd" idea has broken bench/serve (previously PR NVIDIA#14378 was reverted by PR NVIDIA#14782 for the same class of failure: BlockingIOError in _read_spawn_proxy_process_ ipc_hmac_key_fd). Reverting until the fd inheritance across fork+exec into the mpirun child is verified end-to-end for both bench and serve launch paths. Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
…escriptor (NVIDIA#15654) Reverts NVIDIA#15654 (48fc753). PR NVIDIA#15654 passes the IPC HMAC key from the parent process to the LLM-API proxy child via a file descriptor. In the QA-pytest Slurm/MPI launch path used by trtllm-bench and trtllm-serve, the fd handshake between Rank0's forked subshell (spawning internal mpirun) and the mgmn_leader_node RemoteMpiCommSessionServer deadlocks silently: - trtllm-bench (nvbugs/6388787): after "start MpiSession with N workers" the bench process emits no further stdout for ~30 min, workers stay alive at 0% GPU / 4 MiB VRAM, no exception, no MPI abort. Perf harness SIGKILLs at _STALL_TIMEOUT=1800s. - trtllm-serve (nvbugs/6405747): serve mode's /health endpoint never binds, and pytest fails with "Server http://localhost:<port>/health did not become ready within 3600s" on both baseline and candidate wheels. This is the third time the "Pass IPC HMAC key via fd" idea has broken bench/serve (previously PR NVIDIA#14378 was reverted by PR NVIDIA#14782 for the same class of failure: BlockingIOError in _read_spawn_proxy_process_ ipc_hmac_key_fd). Reverting until the fd inheritance across fork+exec into the mpirun child is verified end-to-end for both bench and serve launch paths. Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
…escriptor (NVIDIA#15654) Reverts NVIDIA#15654 (48fc753). PR NVIDIA#15654 passes the IPC HMAC key from the parent process to the LLM-API proxy child via a file descriptor. In the QA-pytest Slurm/MPI launch path used by trtllm-bench and trtllm-serve, the fd handshake between Rank0's forked subshell (spawning internal mpirun) and the mgmn_leader_node RemoteMpiCommSessionServer deadlocks silently: - trtllm-bench (nvbugs/6388787): after "start MpiSession with N workers" the bench process emits no further stdout for ~30 min, workers stay alive at 0% GPU / 4 MiB VRAM, no exception, no MPI abort. Perf harness SIGKILLs at _STALL_TIMEOUT=1800s. - trtllm-serve (nvbugs/6405747): serve mode's /health endpoint never binds, and pytest fails with "Server http://localhost:<port>/health did not become ready within 3600s" on both baseline and candidate wheels. This is the third time the "Pass IPC HMAC key via fd" idea has broken bench/serve (previously PR NVIDIA#14378 was reverted by PR NVIDIA#14782 for the same class of failure: BlockingIOError in _read_spawn_proxy_process_ ipc_hmac_key_fd). Reverting until the fd inheritance across fork+exec into the mpirun child is verified end-to-end for both bench and serve launch paths. Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
…or (NVIDIA#15654) Signed-off-by: Yibin Li <109242046+yibinl-nvidia@users.noreply.github.com>
…escriptor (NVIDIA#15654) (NVIDIA#15961) Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Description
This is a successor of #14378 after fixing the issue mentioned in https://nvbugspro.nvidia.com/bug/6244695 for post-merge pipeline. The original code changes have been reviewed.
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.