Repository navigation
Conversation
…e request local media policy trtllm-serve had two unrelated rules for a request that names a file on the server. A chat media part followed TRTLLM_MEDIA_ALLOWED_LOCAL_PATH and was refused unless the file lies under that directory, while a VisualGen reference with format='path' was read from anywhere on the server unless TRTLLM_DISALLOW_LOCAL_MEDIA_PATH=1, the switch that also turns off response_format='path'. Give each direction one rule. A VisualGen path reference is now read at the HTTP boundary through read_allowed_local_media, the same policy as a chat media part: refused unless TRTLLM_MEDIA_ALLOWED_LOCAL_PATH names a directory holding it, and then only a regular file within the media size limit, handed to the engine as bytes like a multipart upload. TRTLLM_DISALLOW_LOCAL_MEDIA_PATH keeps governing response_format='path' only. The offline Python API still reads its caller's own files. Since parsing a VisualGen request can now read a file, the four routes run parse_visual_gen_params on a worker thread, as they already do for generate_async. The VisualGen docs, the serve README and the benchmark docs describe the policy, and the tests cover the boundary under each setting. Signed-off-by: berryz <berryz@nvidia.com>
…at they govern TRTLLM_MEDIA_ALLOWED_LOCAL_PATH and TRTLLM_DISALLOW_LOCAL_MEDIA_PATH read as two settings of one thing, while each governs one direction of local file handling. Rename them after their job: TRTLLM_MEDIA_ALLOWED_INPUT_DIR is the directory from which a request may have the server read media, and TRTLLM_MEDIA_DISABLE_PATH_RESPONSE=1 turns off response_format='path'. TRTLLM_MEDIA_ALLOWED_INPUT_DIR replaces its former name outright: that name was introduced on main by NVIDIA#21183 and has not shipped in a release. TRTLLM_DISALLOW_LOCAL_MEDIA_PATH has shipped, so a server started with it set refuses to start and names the variable to set instead. Keeping it as an alias would carry the outdated name on, and ignoring it would silently turn path responses back on for a deployment that relied on it. The server reads the switch once at startup and logs when path responses are disabled, so an unrecognized value shows when the server starts rather than at the first request that asks for a path response. The VisualGen benchmark recognizes the server's refusal of a path response by its wording rather than by the variable name, so it works against servers on either side of the rename. Code, docs, examples and tests use the new names. Signed-off-by: berryz <berryz@nvidia.com>
… was opened open_allowed_local_media resolved the request's path, checked that it lies under TRTLLM_MEDIA_ALLOWED_INPUT_DIR, then opened it with O_NOFOLLOW. That flag guards only the last path component, so a directory on the path that became a symlink between the resolution and the open was still followed, and the open file could lie outside the directory. After the open, read /proc/self/fd/<fd> to learn where the open file is and refuse the request unless that location is under the allowed directory as well. Platforms without /proc/self/fd keep the previous behavior. A test swaps a directory for a symlink between the two steps and expects the refusal; it is skipped where /proc/self/fd is absent. Signed-off-by: berryz <berryz@nvidia.com>
|
PR_Github #82785 [ run ] triggered by Bot. Commit: |
sunnyqgg
left a comment
There was a problem hiding this comment.
Moving VisualGen format='path' reads to the HTTP boundary under TRTLLM_MEDIA_ALLOWED_INPUT_DIR is the right fix, and the post-open /proc/self/fd check closes the directory-swap race. This review has 1 Blocker, 2 Possibly blocking, 8 Required and 11 Nice-to-have items: inline comments, plus the bullets below for points with no diff line to anchor on.
The blocker is the retired-variable startup refusal. It applies to every server role, refuses any value including 0, and fires only after the model has loaded.
- [Required] Missing copyright headers:
tensorrt_llm/serve/visual_gen_utils.py(+93/-49) andtests/unittest/llmapi/apps/test_chat_utils.py(+30) are substantially modified but have no NVIDIA SPDX header. CODING_GUIDELINES.md:910 requires one "that includes the year of its latest meaningful modification", and AGENTS.md says to update the year on modified files. Please add the 2026 header. - [Required] Breaking-change process: the description calls this a breaking change, but the PR has no
api-breakinglabel, noBREAKINGin the title, and no release note. The PR template says "an appropriate PR label is added - eitherapi-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title". The only release note an upgrading operator finds (docs/source/release-notes.md:85) still says to setTRTLLM_DISALLOW_LOCAL_MEDIA_PATH=1, which now stops the server from starting. Please addBREAKING:and the label, and a[BREAKING CHANGE]entry under Release 1.4 -> API Changes covering:format='path'now needsTRTLLM_MEDIA_ALLOWED_INPUT_DIR(regular files only, 200 MiB per file, 512 MiB per request);- both renames;
- the startup refusal.
#17490, which introduced the variable, did both.
- [Required] Description vs. code:
/v1/images/editsis listed as takingformat='path'references, butImageEditRequesthas no*_referencefields and itsimagefield already refused paths.- "The offline Python API is unchanged", but the
MediaRef/VisualGenParamsrepr changed. - "Adds no copy", but the bytes now live in the route-side params until the job ends (see the inline comment).
- Test Coverage omits the tests from the later commits (
TestReferenceBounds, the repr test, the cancellation and shutdown tests) and the 512 MiB budget.
- [Nice to have] +1 to coderabbitai's resolved thread on
tensorrt_llm/serve/visual_gen_utils.py:182about bounding reads before they happen.PATH_REFERENCE_MAX_TOTAL_BYTESis checked only after each file is fully read (_build_reference_lists, L241-247), so a refused request may already hold ~512 + 200 MiB. Theformat == "path"test is also repeated after_reference_transportalready branched on it. Passing the remaining budget intoread_allowed_local_mediaand checking it againstfstatbefore reading fixes both. - [Nice to have] Question: is a release/1.3 backport planned for this NVBug? release/1.3 still reads VisualGen
format='path'references from anywhere on the server (params.py:444). If so, backporting the input-dir policy while keeping the shippedTRTLLM_DISALLOW_LOCAL_MEDIA_PATHhonoured there seems safer than cherry-picking the rename and the startup refusal into a patch release.
|
PR_Github #82785 [ run ] completed with state
|
…itch value TRTLLM_MEDIA_DISABLE_PATH_RESPONSE accepted any value and read everything other than "1" as allowed, with a warning. A server now refuses to start when the switch holds anything other than "1", "0" or empty, and a request that still sees such a value treats path responses as off. Signed-off-by: berryz <berryz@nvidia.com>
…n VisualGen only, before the model loads The startup refusal of TRTLLM_DISALLOW_LOCAL_MEDIA_PATH ran in OpenAIServer.__init__ for every server role, refused any value including "0", and ran only after the model had loaded. The check now runs at the top of launch_visual_gen_server, before the model is built, and again in OpenAIServer.__init__ for VisualGen servers before the thread pools are created. A value of "0" or empty asked for the default, so it only logs a warning; any other value still refuses to start and names TRTLLM_MEDIA_DISABLE_PATH_RESPONSE=1. Signed-off-by: berryz <berryz@nvidia.com>
sunnyqgg
left a comment
There was a problem hiding this comment.
The Blockers from my review of 2026-11-05 are resolved or answered at a3ced03:
serve.py:967now checks the retired variable at the top oflaunch_visual_gen_server, before the model loads;openai_server.py:682runs it for VisualGen only and before the pools;0/empty only warn (a3ced03)
The last CI run (ad5cad9) ended UNSTABLE, and the data shows no run on a3ced03 yet; L0 needs to pass on the new head before merge.
The Possibly-blocking and Required items are not covered: path-reference bytes kept on the route side, the fixed 8-worker parse pool, missing SPDX headers, no BREAKING label or release note, and the 200/512 MiB limits not documented. All are unchanged at H.
crazydemo
left a comment
There was a problem hiding this comment.
Review summary - Approve
Reviewed the full diff; no blocking or major issues found.
Automated review by NVCortex Lite, run by @crazydemo.
…aseMediaIO.clear_executor Server teardown read BaseMediaIO._executor directly and set_executor took None only for that. BaseMediaIO.clear_executor(executor) now stops publishing a pool only when it is still the published one, and set_executor takes an Executor again. Signed-off-by: berryz <berryz@nvidia.com>
…e the engine has its snapshot The video routes read format='path' references into params before generate_async, which snapshots params for the engine. The route kept its own params, and with it the reference bytes, until the job finished: the async route hands params to the background task and the sync route holds it across the wait for the result. Both routes now clear the reference fields right after generate_async returns, which also frees multipart uploads early. The test generator snapshots params as the engine does, and route-level tests send a format='path' reference to both video routes, with and without TRTLLM_MEDIA_ALLOWED_INPUT_DIR. Signed-off-by: berryz <berryz@nvidia.com>
…the request reads files Every VisualGen request was parsed on the media-load pool, including one with nothing to read, so it could wait behind slow format='path' reads, and a VisualGen server always sized that pool at 8. The parse now moves to the pool only for a request with a multipart upload or a format='path' reference (request_reads_files); any other request is parsed inline, as before this PR. trtllm-serve forwards --num_media_load_workers to the VisualGen server. _parse_visual_gen_params moves from the video-routes mixin to OpenAIServer, next to _reject_disabled_path, as both image routes use it too. Signed-off-by: berryz <berryz@nvidia.com>
…fuses the former variable before loading the model Signed-off-by: berryz <berryz@nvidia.com>
…o modified files Signed-off-by: berryz <berryz@nvidia.com>
…h' references An HTTP format='path' reference is read only up to 200 MiB per file and 512 MiB across the path references of one request. The VisualGen model page, the serve example README and the MediaReferenceItem.format description now say so and point a larger file to a multipart upload. Signed-off-by: berryz <berryz@nvidia.com>
…LLM_MEDIA_ALLOWED_INPUT_DIR Four bundled /v1/videos workloads send format: path references from the run's reference_media/, which the server now reads only under TRTLLM_MEDIA_ALLOWED_INPUT_DIR. Serve step C now exports it as the parent of the vg-bench-* runs before any of its launch commands, so a reused server also reads the media of later runs. Signed-off-by: berryz <berryz@nvidia.com>
…trings in line with the code _build_reference_lists takes a VideoGenerationRequest and returns lists of MediaRef, and MediaRef.__repr_args__ declares its return type. check_path_response_policy and the comment on the former variable now state the contract only. parse_visual_gen_params says it reads uploads and format='path' references and blocks for those. prepare_reference_slots, the generate_async comment and the test helper no longer describe serve and the Python API as sharing one resolution path, and the __repr_args__ comment no longer claims that a printed VisualGenParams never carries media. Signed-off-by: berryz <berryz@nvidia.com>
… its type and size The post-open location check ran after the regular-file and size checks, so a file reached through a directory swapped mid-request was refused as "not a regular file" or as too large rather than with the location refusal. It now runs right after the fstat. It also has its own constant, _FD_LINK_DIR, instead of testing _DESCRIPTOR_DIR, so how decoders reach the descriptor no longer decides whether the check runs. Signed-off-by: berryz <berryz@nvidia.com>
…for the local media policy Covers that VisualGen format='path' references now need TRTLLM_MEDIA_ALLOWED_INPUT_DIR and are capped at 200 MiB per file and 512 MiB per request, both variable renames, and that a VisualGen server refuses to start while the former path-response variable is set. Signed-off-by: berryz <berryz@nvidia.com>
…edia-policy-followup Signed-off-by: berryz <berryz@nvidia.com> # Conflicts: # docs/source/release-notes.md
|
/bot run |
|
PR_Github #83448 [ run ] triggered by Bot. Commit: |
|
PR_Github #83448 [ run ] completed with state
|
zhaoyangwang-nvidia
left a comment
There was a problem hiding this comment.
Approve with nits.
| os.close(fd) | ||
|
|
||
|
|
||
| def log_allowed_local_media_path() -> None: |
There was a problem hiding this comment.
A deployment that still sets TRTLLM_MEDIA_ALLOWED_LOCAL_PATH now gets a 400 on every local media request with nothing in the server log. Could log_allowed_local_media_path warn when the former name is set and name TRTLLM_MEDIA_ALLOWED_INPUT_DIR, as check_path_response_policy does for the output-side rename?
…irectory variable is set A deployment that still sets TRTLLM_MEDIA_ALLOWED_LOCAL_PATH has every local media request refused. log_allowed_local_media_path now names the new variable in a warning when the former one is set. Signed-off-by: berryz <berryz@nvidia.com>
|
/bot run |
|
PR_Github #84049 [ run ] triggered by Bot. Commit: |
|
PR_Github #84049 [ run ] completed with state
|
|
@NVIDIA/trt-llm-models-devs @NVIDIA/trt-llm-torch-visual-gen-devs Just kind reminder. |
Description
Follow-up to #21183 (NVBug 6308608), from the review discussion there.
trtllm-serve had two unrelated rules for a request that names a file on the server:
/v1/chat/completionsmedia parts (image_url,video_url,audio_url) were refused unlessTRTLLM_MEDIA_ALLOWED_LOCAL_PATH=<directory>holds the file ([https://nvbugs/6308608][fix] Read request media from local paths only under an allowed directory #21183).format='path'(image_reference,video_reference,audio_referenceon/v1/videosand/v1/videos/sync) were read from anywhere on the server unlessTRTLLM_DISALLOW_LOCAL_MEDIA_PATH=1, the switch that also turns offresponse_format='path'.After this change each direction has one rule and one variable named after it:
TRTLLM_MEDIA_ALLOWED_INPUT_DIR(wasTRTLLM_MEDIA_ALLOWED_LOCAL_PATH): the directory from which a request may have the server read media. Unset refuses every local path. A VisualGenformat='path'reference is now read at the HTTP boundary through the same policy as a chat media part (read_allowed_local_media): refused with HTTP 400 unless the variable names a directory holding it, and then only a regular file of at most 200 MiB, with at most 512 MiB across thepathreferences of one request. It reaches the engine as bytes, like a multipart upload.TRTLLM_MEDIA_DISABLE_PATH_RESPONSE=1(wasTRTLLM_DISALLOW_LOCAL_MEDIA_PATH): turns offresponse_format='path', as before, and nothing else. It accepts only1,0or empty; a VisualGen server refuses to start on any other value.Renames: both former names shipped in release candidates (
TRTLLM_DISALLOW_LOCAL_MEDIA_PATHsince 1.3,TRTLLM_MEDIA_ALLOWED_LOCAL_PATHin v1.4.0rc3). The former input name is no longer read, so local paths stay refused until the new one is set. A VisualGen server started withTRTLLM_DISALLOW_LOCAL_MEDIA_PATHset to anything other than0or empty refuses to start and namesTRTLLM_MEDIA_DISABLE_PATH_RESPONSE=1;0or empty only logs a warning. The check runs at the top oflaunch_visual_gen_server, before the model loads, and again inOpenAIServer.__init__for VisualGen servers only, so other server roles never read the variable. The VisualGen benchmark recognizes the server's refusal of a path response by its wording, so it works against servers on either side of the rename.Reading files at the boundary: a request with a multipart upload or a
format='path'reference is parsed on the media-load pool, which bounds how many requests read at once; any other request is parsed inline, as before. trtllm-serve forwards--num_media_load_workersto the VisualGen server, and the server unpublishes and shuts the pool down when it stops. The video routes clear the reference fields of their ownparamsoncegenerate_asynchas taken its snapshot, so a queued or running job does not keep a second copy of its media.open_allowed_local_mediaalso checks the location of the file it opened, not only the path it resolved: on Linux it reads/proc/self/fd/<fd>right after the open and refuses the request unless that location is under the allowed directory as well, before the type and size checks.O_NOFOLLOWcovers only the last path component, so a directory on the path that became a symlink between resolution and open was followed before. Platforms without/proc/self/fdkeep the previous behavior.The offline Python API reads files as before:
VisualGenParamswithMediaRef(format='path')and the media loaders'async_loadstill read the caller's own files.MediaRef's repr now shows abytespayload by its size. The trtllm-serve docs, the VisualGen docs, the VisualGen serve README, the benchmark docs, the benchmark skill (its server now starts withTRTLLM_MEDIA_ALLOWED_INPUT_DIR) and the Release 1.4 notes describe the policy.Breaking change for VisualGen deployments that send
format='path'referencesBefore this change such a reference was read from any path on the server, with no size limit, unless
TRTLLM_DISALLOW_LOCAL_MEDIA_PATH=1. Now it is refused with HTTP 400 unless the server is started withTRTLLM_MEDIA_ALLOWED_INPUT_DIR=<directory>, and then only regular files inside that directory and within the size limits are read; a larger file has to be sent as a multipart upload. Deployments whose clients send local paths, includingbenchmark_visual_gen.pyworkloads withformat: pathreferences, must set the variable to the directory that holds their media.url,base64and multipart uploads are unaffected, and so isresponse_format='path'.Renamed variables: a deployment that set
TRTLLM_MEDIA_ALLOWED_LOCAL_PATHmust setTRTLLM_MEDIA_ALLOWED_INPUT_DIRinstead. A VisualGen deployment that setTRTLLM_DISALLOW_LOCAL_MEDIA_PATH=1must unset it and setTRTLLM_MEDIA_DISABLE_PATH_RESPONSE=1instead.Test Coverage
All in l0_cpu except
tests/unittest/llmapi/test_config_database.py, which no L0 list includes; its new case passed in a local run of the CI image.tests/unittest/llmapi/apps/test_chat_utils.py::TestRequestLocalMediaPaths: the existing policy cases under the new variable name; a directory on the path swapped for a symlink between resolution and open is refused, and a non-regular file reached that way gets the same location refusal with either descriptor directory (Linux; skipped without/proc/self/fd). The shared open/read function is covered here, so the VisualGen tests below check the wiring.tests/unittest/_torch/visual_gen/test_visual_gen_utils.py:TestPathReferencePolicy: aformat='path'reference is refused without the directory, read to bytes at the boundary inside it (bare path andfile://), and unaffected by the output switch.TestReferenceBounds: the per-request 512 MiB budget across path references.TestPathResponseSwitch: an unrecognized value refused at startup and treated as off;0or empty allowing path responses; the former name set to disable refusing to start; set to0or empty only warning; the startup check silent by default and logging a disabled switch.tests/unittest/_torch/visual_gen/test_trtllm_serve_endpoints.py:response_format='path'tests under the new variable name;TestPathReferenceRoutes:/v1/videosand/v1/videos/syncrefuse aformat='path'reference without the directory before reaching the engine, and with it hand the engine the file's bytes and drop the route's own copy;TestPathResponseStartupCheck: a VisualGen server refuses the former name before creating its pools, and an LLM server ignores it.tests/unittest/_torch/visual_gen/test_visual_gen_params.py: abytesreference payload shown by size in the repr.tests/unittest/llmapi/test_visual_gen_startup_shutdown.py:launch_visual_gen_serverrefuses the former name before loading the model, and passes the media-load pool size to the server.tests/unittest/llmapi/test_config_database.py(not in an L0 list):--num_media_load_workersreacheslaunch_visual_gen_server.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.
Dev Engineer Review
The server uses
TRTLLM_MEDIA_ALLOWED_INPUT_DIRto restrict chat media and VisualGenformat="path"inputs. VisualGen reads path references at the HTTP boundary and passes their bytes to the engine. The policy checks regular files, directory containment, and size limits.TRTLLM_MEDIA_DISABLE_PATH_RESPONSEcontrolsresponse_format="path"separately. Startup rejects the retiredTRTLLM_DISALLOW_LOCAL_MEDIA_PATHvariable. On Linux, the server checks the opened file location through/proc/self/fd. VisualGen request parsing runs in the media-load pool, which shuts down with the server. URL, base64, multipart inputs, and offline Python API reads remain outside the HTTP input policy.QA Engineer Review
Tests cover local-media restrictions, VisualGen path reads, path-response controls, startup validation, byte limits, symlink replacement, cancellation while parsing, worker-pool shutdown, and payload-safe parameter representation. No test-list files changed. The modified integration test file has entries in CI and manual-QA lists, but the changed environment-setting line is setup code rather than a listed test. Test execution results were not supplied. Coverage verdict: sufficient for the stated policy and lifecycle changes.
Per-File QA Perspective
.claude/skills/trtllm-visualgen-bench/SKILL.md— Verify benchmark guidance names the input-directory setting for server-side path references..claude/skills/trtllm-visualgen-bench/workloads/README.md— Verify path-reference guidance matches the server’s directory restriction.docs/source/commands/trtllm-serve/trtllm-serve.rst— Verify the documented setting and file restrictions match chat and VisualGen behavior.docs/source/models/visual-generation.md— Verify input-path policy and output-path response controls are documented separately.examples/serve/openai_chat_client_for_multimodal.py— Verify the example names the current input-directory setting.examples/visual_gen/serve/README.md— Verify the input and output path settings are distinct and accurate.tensorrt_llm/inputs/media_io.py— Verify directory containment, regular-file checks, size limits, descriptor closing, and the Linux opened-file location check.tensorrt_llm/serve/chat_utils.py— Verify local-media policy documentation matches enforced behavior.tensorrt_llm/serve/openai_protocol.py— Verify theformat="path"API description matches accepted server inputs.tensorrt_llm/serve/openai_server.py— Verify startup policy checks, path-response rejection, worker-thread parsing for image routes, and media-pool shutdown.tensorrt_llm/serve/openai_video_routes.py— Verify both video routes parse VisualGen parameters off the event loop.tensorrt_llm/serve/scripts/BENCHMARKING_VISUAL_GEN.md— Verify benchmark instructions use the current path-input setting.tensorrt_llm/serve/scripts/benchmark_visual_gen.py— Verify the path-response failure hint recognizes the new error and setting.tensorrt_llm/serve/visual_gen_utils.py— Verify path references use the input policy, other formats remain unchanged, and the response switch behaves as documented.tensorrt_llm/visual_gen/params.py— Verify offline Python API reads remain separate from serve-boundary reads and byte payloads are not exposed in representations.tests/integration/defs/accuracy/test_glm53_flash.py— Verify the integration fixture uses the current input-directory setting. The file has CI and manual-QA entries; the changed fixture setup is not a separate listed test.tests/unittest/_torch/visual_gen/test_trtllm_serve_endpoints.py— Covers path-response rejection, cancellation while parsing, and media-pool shutdown. Integration-list status is not applicable to this unit test.tests/unittest/_torch/visual_gen/test_visual_gen_params.py— Covers byte-payload size representation while preserving visible path references. Integration-list status is not applicable.tests/unittest/_torch/visual_gen/test_visual_gen_utils.py— Covers path-input policy, byte reads, request byte limits, response-switch values, logging, and retired-setting startup validation. Integration-list status is not applicable.tests/unittest/llmapi/apps/_test_openai_chat_multimodal.py— Verify the server fixture uses the current input-directory setting. Integration-list status is not applicable.tests/unittest/llmapi/apps/_test_openai_chat_vlm_guided_decoding.py— Verify the server fixture uses the current input-directory setting. Integration-list status is not applicable.tests/unittest/llmapi/apps/_test_openai_mmencoder.py— Verify the server fixture uses the current input-directory setting. Integration-list status is not applicable.tests/unittest/llmapi/apps/_test_trtllm_serve_multimodal_example.py— Verify the server setup uses the current input-directory setting. Integration-list status is not applicable.tests/unittest/llmapi/apps/test_chat_utils.py— Covers local-media restrictions and rejects a Linux directory-symlink swap. Integration-list status is not applicable.tests/unittest/pytest.ini— Verify the thread-leak exclusion matches media-load worker threads without hiding unrelated threads.