[https://nvbugs/6792615][fix] Keep session-prefetch output off sys.stdout by sunnyqgg · Pull Request #20152 · NVIDIA/TensorRT-LLM

sunnyqgg · 2026-10-12T09:25:49Z

Dev Engineer Review

session_prefetcher.py routes prefetcher diagnostics through _log(), which writes to file descriptor 1 with os.write() and ignores OSError. This avoids accessing Python’s stdout stream during capture teardown. Logging failures no longer interrupt cleanup. The prefetch and cleanup logic is otherwise unchanged.

The change removes the A30 waiver for test_speculative_d2h_predictor_always_hit. The test name was not found in the available CI or manual-QA test lists.

QA Engineer Review

The PR modifies tests/test_common/session_prefetcher.py and tests/integration/test_lists/waives.txt. It adds no regression test functions or IDs. The waiver removal makes the A30 test eligible to run rather than skipping it.

The author reports 46 passing prefetcher tests on a CPU-only host. One test failed and one was skipped because CUDA-enabled tensorrt_llm could not be imported; the author reports the same result on unmodified main. No deterministic test covers the capture-teardown race. Coverage verdict: needs follow-up.

Per-File QA Perspective

tests/test_common/session_prefetcher.py: Verify that fd capture records diagnostic output and background logging does not access Python stdout streams. The author reports that the existing prefetcher tests passed.
tests/integration/test_lists/waives.txt: The A30 waiver for test_speculative_d2h_predictor_always_hit was removed. The test name was not found in the available CI or manual-QA lists.

Description

Fixes NVBug 6792615 and its recurrence NVBug 6902356. The unittest stage (A30-PyTorch-1) dies with Fatal Python error: Segmentation fault (exit -11). faulthandler shows the crashing thread is the session prefetcher's session-prefetch-build thread, inside a print() in SessionPrefetcher._build (tests/test_common/session_prefetcher.py). The main thread is at a test-phase boundary at that moment. The test named in the bug title is only the test that was in flight.

Root cause:

The prefetcher prints from background threads: the shadow build, the page-cache warm, and the dead-pool discard.
CI runs CPython 3.12.3. Its print() holds only a borrowed reference to sys.stdout; CPython 3.12.10 and 3.13.3 take a strong reference instead.
tests/unittest/conftest.py::cuda_error_early_quit is an autouse fixture that requests capfd. Every unit test therefore installs its own capture stream as sys.stdout and frees it at fixture teardown.
A background print() that races that teardown writes through a freed object and crashes the whole pytest process.

Fix:

Route every prefetcher diagnostic through a new _log() helper, which writes the line to fd 1 with os.write(). pytest's fd capture redirects fd 1, so the output lands where print() put it: the per-test capture during a test, and the console at session finish. No Python stream object is involved.
Remove the A30 waiver of test_speculative_d2h_predictor_always_hit filed under 6792615. That test only failed because it was in flight when the harness crashed.

Target branch: release/1.3, because the bug is committed to v1.3.0. tests/test_common/session_prefetcher.py is identical on release/1.3 and main, and main carries the same waiver, so main needs the same change.

Test Coverage

Existing tests/unittest/llmapi/test_session_prefetcher.py (listed in l0_cpu.yml): 46 passed on a CPU-only host, validated on main. The changed file is byte-identical on release/1.3. test_session_summary_counters_and_emission reads the prefetcher's summary line through capfd, so it also covers the new fd-level output path. test_patch_targets_cover_all_library_construction_sites failed and one case was skipped, both because the host cannot import a CUDA-enabled tensorrt_llm (libc10_cuda.so). This also happens on unmodified main.
The crash is a timing race, so no deterministic regression test is added. The A30 unittest stage runs the previously waived test again.
The mechanism was reproduced without GPUs on CPython 3.12.3 (Ubuntu 24.04) with pytest 9.0.3. An autouse capfd fixture plus a daemon thread calling print() segfaults with the same thread layout as the CI crash. The same setup using os.write() never failed.

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-compatible or api-breaking. For api-breaking, include BREAKING in 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.

coderabbitai · 2026-10-12T09:31:07Z

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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: NVIDIA/TensorRT-LLM/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a8b7d0d1-6b33-4895-98b3-0a8dbf2dda3d

📥 Commits

Reviewing files that changed from the base of the PR and between dcbe2d0 and 13e45ff.

📒 Files selected for processing (1)

tests/test_common/session_prefetcher.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

Walkthrough

The prefetcher now writes diagnostic messages directly to file descriptor 1 and ignores write errors. The A30 waiver for the speculative D2H beam-search test was removed.

Changes

Prefetcher diagnostic output

Layer / File(s)	Summary
Direct diagnostic output `tests/test_common/session_prefetcher.py`	A new helper writes newline-terminated UTF-8 messages to file descriptor 1 and ignores `OSError`. Existing prefetcher diagnostics now use the helper.

Integration-test waiver update

Layer / File(s)	Summary
Remove the A30 waiver `tests/integration/test_lists/waives.txt`	The waiver entry for `test_speculative_d2h_predictor_always_hit` was removed.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: bowenfu

Merge Risk: ⚪ Minimal · up to 13e45

This change routes prefetcher diagnostics through fd writes and removes an A30 test waiver. No actionable remaining failure is established, so no additional merge-blocking risk is identified beyond normal checks.

Pre-merge checks · 4 · 1

Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks (4 passed)

Check name	Status	Explanation
Title check		The title clearly identifies the fix: keeping session-prefetch diagnostics off sys.stdout. It uses the required NVBugs and fix format.
Description check		The description includes the required Description, Test Coverage, and PR Checklist sections. It explains the root cause, implementation, affected waiver, test results, limitations, and checklist statu…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

github-actions · 2026-10-12T10:21:29Z

Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements.

xxi-nv · 2026-10-12T10:58:40Z

Please rebase to release/1.3 instead of main.

brnguyen2

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

The root-cause write-up matches the crashing thread layout. All print() calls in the prefetcher now go through _log, so no background thread in this module touches sys.stdout anymore. Because CI uses fd-level capture, the output still lands in the same place.

One side effect to keep in mind: under --capture=sys or capsys, these lines now bypass capture and go straight to the console. That's fine for diagnostics, and the summary test already uses capfd.

…dout The session prefetcher printed from its build, warm and discard threads. On CPython 3.12.3, print() holds only a borrowed reference to sys.stdout, and the autouse capfd fixture in tests/unittest/conftest.py installs and frees a per-test stdout object at every teardown. A background print that races that teardown writes to freed memory and segfaults the pytest process; the faulthandler stack shows the "session-prefetch-build" thread inside print(). Route every prefetcher diagnostic through _log(), which writes to fd 1 with os.write(). pytest's fd capture redirects fd 1, so output still lands where print() put it, but no Python stream object is touched. Remove the A30 waiver of test_speculative_d2h_predictor_always_hit, which only failed because it was the test in flight when the harness crashed. Signed-off-by: qgai <qgai@nvidia.com>

Drop OSError from the fd write in _log(). Several log calls come right before cleanup: the SIGKILL sweep in _reap_dead_pool, session.shutdown() in _publish, starting the reap thread in take(), and the except handler in _build. If fd 1 is a closed pipe (EPIPE) or a full non-blocking pipe (EAGAIN), a raising log call would skip that cleanup and leak a pool. Signed-off-by: qgai <qgai@nvidia.com>

sunnyqgg · 2026-10-13T07:21:59Z

/bot run --disable-fail-fast

tensorrt-cicd · 2026-10-13T07:29:15Z

PR_Github #77455 [ run ] triggered by Bot. Commit: 13e45ff Link to invocation

tensorrt-cicd · 2026-10-13T11:13:10Z

PR_Github #77455 [ run ] completed with state SUCCESS. Commit: 13e45ff
/LLM/release-1.3/L0_MergeRequest_PR pipeline #184 completed with status: 'SUCCESS'

CI Report

Link to invocation

….3: Keep session-prefetch output off sys.stdout Forward-port of release/1.3 PR NVIDIA#20152 (release/1.3 commit ca9c5b6) to main. Signed-off-by: qgai <qgai@nvidia.com> Signed-off-by: Simeng Liu <simengl@nvidia.com>

sunnyqgg requested review from a team as code owners October 12, 2026 09:25

sunnyqgg requested review from allisonlim-nv, brnguyen2, mikeiovine and schetlur-nv October 12, 2026 09:25

github-actions Bot assigned sunnyqgg Oct 12, 2026

sunnyqgg force-pushed the fix/nvbug-6792615-prefetch-bg-print branch from aef0ab1 to 4e4cdd4 Compare October 12, 2026 09:43

BowenFu approved these changes Oct 12, 2026

View reviewed changes

trtllm-agent added the ci: full pre-merge approved label Oct 12, 2026

brnguyen2 approved these changes Oct 12, 2026

View reviewed changes

Comment thread tests/test_common/session_prefetcher.py Resolved

mikeiovine approved these changes Oct 12, 2026

View reviewed changes

sunnyqgg marked this pull request as draft October 13, 2026 06:53

sunnyqgg force-pushed the fix/nvbug-6792615-prefetch-bg-print branch from 4e4cdd4 to adf5831 Compare October 13, 2026 06:53

sunnyqgg changed the base branch from main to release/1.3 October 13, 2026 06:53

sunnyqgg marked this pull request as ready for review October 13, 2026 06:54

sunnyqgg force-pushed the fix/nvbug-6792615-prefetch-bg-print branch from adf5831 to dcbe2d0 Compare October 13, 2026 07:03

BowenFu approved these changes Oct 13, 2026

View reviewed changes

sunnyqgg merged commit ca9c5b6 into NVIDIA:release/1.3 Oct 14, 2026

SimengLiu-nv mentioned this pull request Oct 22, 2026

[None][chore] Mass merge release/1.3 branch back to main (batch 2) #20843

Merged

1 task done

Repository navigation

[https://nvbugs/6792615][fix] Keep session-prefetch output off sys.stdout - #20152

[https://nvbugs/6792615][fix] Keep session-prefetch output off sys.stdout#20152
sunnyqgg merged 2 commits into
NVIDIA:release/1.3from
sunnyqgg:fix/nvbug-6792615-prefetch-bg-print

sunnyqgg commented Oct 12, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 12, 2026 •

edited

Loading

Reviews paused

Uh oh!

github-actions Bot commented Oct 12, 2026

Uh oh!

xxi-nv commented Oct 12, 2026

Uh oh!

brnguyen2 left a comment

Uh oh!

Uh oh!

sunnyqgg commented Oct 13, 2026

Uh oh!

tensorrt-cicd commented Oct 13, 2026

Uh oh!

tensorrt-cicd commented Oct 13, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

sunnyqgg commented Oct 12, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dev Engineer Review

QA Engineer Review

Per-File QA Perspective

Description

Test Coverage

PR Checklist

Uh oh!

coderabbitai Bot commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Uh oh!

github-actions Bot commented Oct 12, 2026

Uh oh!

xxi-nv commented Oct 12, 2026

Uh oh!

brnguyen2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sunnyqgg commented Oct 13, 2026

Uh oh!

tensorrt-cicd commented Oct 13, 2026

Uh oh!

tensorrt-cicd commented Oct 13, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sunnyqgg commented Oct 12, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 12, 2026 •

edited

Loading