[JunyiXu-nv]

Dev Engineer Review

GenerationResultBase now provides the read-only finished property. Serve post-processors and scaffolding workers use it instead of _done. Scaffolding also uses async iteration instead of _aresult_step(). The reported intent is no behavior change.

The change aligns cross-package code with the public result API. No current review findings or severity counts were supplied. GPU coverage remains for CI.

QA Engineer Review

Modified unit tests cover property behavior and immutability, stream finalization, usage-only chunks, Responses final output, and scaffolding iteration and end flags. The CPU test list adds unittest/serve/test_postprocess_finished.py to l0_cpu.yml. The available evidence does not establish manual-QA list coverage.

The author reports passing results for focused test runs and API-stability tests. The author also reports one broader CPU run with a failure attributed to the runner missing trtllm-render, and says the test passed when that script was on PATH. These results are author-reported, not independently verified here. GPU coverage remains for CI. Coverage verdict: needs follow-up until GPU CI coverage completes.

Per-File QA Perspective

• tensorrt_llm/executor/result.py — GenerationResultBase.finished exposes the completion state as a read-only property. Verify consistent behavior for GenerationResult and DetokenizedGenerationResultBase.
• tensorrt_llm/scaffolding/contrib/AsyncGeneration/stream_generation.py — Async generation uses iterator advancement and finished for completion. Verify completion and remaining-step behavior.
• tensorrt_llm/scaffolding/worker.py — Worker streaming uses async iteration and finished. Verify output collection and end-flag behavior.
• tensorrt_llm/serve/postprocess_handlers.py — Streaming post-processors use finished to detect completion and gate final usage output. Verify chat, completion, Harmony, and Responses stream finalization.
• tensorrt_llm/serve/responses_utils.py — Responses streaming passes finished to the event generator. Verify final response emission at completion.
• tests/integration/test_lists/test-db/l0_cpu.yml — Adds unittest/serve/test_postprocess_finished.py to the CPU test list.
• tests/unittest/llmapi/apps/test_harmony_parsing.py — Updates mocked completion state to use finished.
• tests/unittest/llmapi/apps/test_responses_stream_final_consistency.py — Updates the generation-chunk fixture to use finished.
• tests/unittest/llmapi/apps/test_responses_streaming_events.py — Updates mocked completion state and completion checks.
• tests/unittest/llmapi/apps/test_responses_usage.py — Updates the synthetic result to use finished.
• tests/unittest/llmapi/apps/test_tool_parsers.py — Updates response fixtures to use finished.
• tests/unittest/llmapi/test_executor.py — Tests that finished becomes true after a final response and rejects assignment.
• tests/unittest/scaffolding/test_worker_streaming_step.py — Adds CPU-only coverage for async iteration, step limits, and end-flag behavior.
• tests/unittest/serve/test_postprocess_finished.py — Adds CPU-only coverage for result-property behavior and stream finalization. l0_cpu.yml lists this test.

Description

The serve stream post-processors and the scaffolding workers read the private GenerationResultBase._done flag on result objects owned by tensorrt_llm/executor/, and the scaffolding workers also called the private _aresult_step(). This PR switches them to the public surface of the result objects, so the upcoming relocation of this code (epic TRTLLM-17056, task TRTLLM-17273) does not carry private cross-package accesses into the new files.

Part of the trtllm-serve refactor: cleanup and wrap-up (TRTLLM-17273). It must land before the Responses-package move, which relocates ResponsesStreamingProcessor, the holder of one of these reads.

Why the property moves first. finished was defined only on GenerationResult. With num_postprocess_workers > 0, the post-processing workers call chat_stream_post_processor / completion_stream_post_processor on DetokenizedGenerationResultBase records (executor/postproc_worker.py, default_record_creator), which had _done but no finished. Moving the property up to GenerationResultBase — where _done lives — gives every result object the public view; RequestOutput.finished is unchanged (it inherits the same property through its bases) and the api-stability references do not change.

finished stays a read-only view of _done (a property with no setter). The executor package writes _done directly on these objects (response handling, abort, engine-dead paths, a post-processor-hook terminate verdict, and several tests on real result objects), so a separately stored flag would go stale, and an accidental result.finished = ... write on a real result now raises AttributeError.

The 11 reads and 3 calls

Site	Old	New
serve/postprocess_handlers.py chat_stream_post_processor	if include_usage and rsp._done:	if include_usage and rsp.finished:
serve/postprocess_handlers.py completion_stream_post_processor	if include_usage and rsp._done:	if include_usage and rsp.finished:
serve/postprocess_handlers.py chat_harmony_streaming_post_processor	done=rsp._done,	done=rsp.finished,
serve/postprocess_handlers.py responses_api_streaming_post_processor	if rsp._done:	if rsp.finished:
serve/responses_utils.py ResponsesStreamingProcessor.process_single_output	finished_generation=res._done,	finished_generation=res.finished,
scaffolding/worker.py streaming_generate_helper	while not generate_result._done:	while not generate_result.finished:
scaffolding/worker.py streaming_generate_helper	asyncio.create_task(generate_result._aresult_step())	asyncio.create_task(anext(generate_result))
scaffolding/worker.py streaming_generate_helper	... and not generate_result._done:	... and not generate_result.finished:
scaffolding/worker.py stream_generation_handler	if task.request_handle._done:	if task.request_handle.finished:
scaffolding/contrib/AsyncGeneration/stream_generation.py	await task.request_handle._aresult_step()	await anext(task.request_handle)
scaffolding/contrib/AsyncGeneration/stream_generation.py	if task.request_handle._done: (twice)	if task.request_handle.finished:
scaffolding/contrib/AsyncGeneration/stream_generation.py	while not task.request_handle._done:	while not task.request_handle.finished:
scaffolding/contrib/AsyncGeneration/stream_generation.py	asyncio.create_task(task.request_handle._aresult_step())	asyncio.create_task(anext(task.request_handle))

Out of scope, same name: ScaffoldingResult._done / ._aresult_step() (scaffolding/result.py) is scaffolding's own class, read through self, and keeps its field; IterationResult._done is another class; reads inside tensorrt_llm/executor/ are the package's own field.

The anext() equivalence (for the scaffolding owners). GenerationResult.__anext__ re-raises _terminal_error if set, raises StopAsyncIteration if _done, and otherwise awaits self._aresult_step() and returns self. At the three changed call sites the request is never finished when the call is made: two sit inside while not ...finished loops with no await between the check and the call, and the third sits in a for loop that breaks as soon as the request finishes. _terminal_error is only ever set together with _done = True (executor/result.py), so at these sites anext(handle) awaits _aresult_step() and discards the returned handle — exactly what the direct call did, including cancellation behavior under asyncio.create_task(...).cancel().

Mock-fixture updates (and why). Mock().finished is an auto-created, always-truthy Mock: a test double that only sets _done would silently make every chunk look final after this change. Every Mock/SimpleNamespace double fed to the five serve sites now sets finished instead of _done: tests/unittest/llmapi/apps/test_tool_parsers.py (two fixtures), test_harmony_parsing.py (two fixtures), test_responses_stream_final_consistency.py, test_responses_streaming_events.py (two fixtures plus the two test-internal reads of them), and test_responses_usage.py. A SimpleNamespace double that forgets finished now fails loudly with AttributeError instead of passing with stale semantics.

Behavior

No behavior change.

Moves

GenerationResult.finished → GenerationResultBase.finished (same body; the property additionally gained a one-line docstring). No file moves, no shims.

CI bookkeeping

• Multi-GPU table: none (no entries added or removed; the PR touches tensorrt_llm/executor/ and tensorrt_llm/serve/responses_utils.py, which are already listed, so the multi-GPU stages run on this PR)
• CODEOWNERS: none
• legacy-files.txt: none (no new file receives legacy-formatted code; the two new test files are new code under the default formatter)
• Test lists: tests/unittest/serve/test_postprocess_finished.py registered in tests/integration/test_lists/test-db/l0_cpu.yml (after unittest/scripts); tests/unittest/scaffolding/test_worker_streaming_step.py is collected by the existing unittest/scaffolding entry in l0_cpu.yml and carries pytest.mark.cpu_only

Test Coverage

New tests (both pytest.mark.cpu_only):

• tests/unittest/serve/test_postprocess_finished.py — on a real GenerationResultBase and a real DetokenizedGenerationResultBase (the postproc-worker record type; before this PR the latter had no finished): finished mirrors the completion flag and rejects writes; chat_stream_post_processor and completion_stream_post_processor with stream_options.include_usage=true emit the final usage chunk only once the request is finished; responses_api_streaming_post_processor requests the final Responses payload from its streaming processor only once finished.
• tests/unittest/scaffolding/test_worker_streaming_step.py — TRTLLMWorker.streaming_generate_helper, TRTLLMWorker.stream_generation_handler and the AsyncGeneration stream_generation_handler on a fake handle that implements only finished and __anext__ (no _done, no _aresult_step): steps until finished, queues one output per non-final step, stops early at the step budget, and sets task.end_flag exactly when the handle finishes.

Commands run locally (CPU-only dev container, no GPU; compiled artifacts from a source build of main at f6c4125c79, Python code under test from this branch — tensorrt_llm.__file__ printed the branch worktree):

• pytest -m cpu_only tests/unittest/serve/test_postprocess_finished.py tests/unittest/scaffolding -q → 25 passed
• pytest tests/unittest/llmapi/test_executor.py::test_GenerationResultBase -q → 1 passed
• LLM_MODELS_ROOT=... pytest -m cpu_only tests/unittest/llmapi/test_executor.py tests/unittest/llmapi/apps/test_tool_parsers.py -q → 989 passed
• pytest tests/unittest/executor/test_spec_decode_stats_payload.py -q → 22 passed (drives the chat, completion and Responses stream post-processors on real result objects)
• pytest tests/unittest/api_stability -q → 64 passed (RequestOutput still exposes the committed finished; no reference changed)
• LLM_MODELS_ROOT=... pytest -rs tests/unittest/llmapi/apps/test_harmony_parsing.py tests/unittest/llmapi/apps/test_harmony_channel_validation.py → 79 passed, zero "not importable" skip lines
• LLM_MODELS_ROOT=... pytest -m cpu_only tests/unittest/llmapi/apps tests/unittest/disaggregated -q → 3438 passed, 1 skipped, 2 xfailed; the one failure (test_render_standalone.py::TestCpuOnlyStartup) was the runner's own PATH missing the trtllm-render console script and passes (3 passed) with it on PATH — unrelated to this change
• Before/after pin: on unmodified main, GenerationResultBase and DetokenizedGenerationResultBase have no finished attribute, so the new tests cannot pass without the property move
• git grep -nE "\._done\b|_aresult_step\(" -- tensorrt_llm/serve tensorrt_llm/scaffolding → only scaffolding/result.py (its own field and method)
• pre-commit on all changed files, run twice → clean and stable

An independent review additionally ran a four-mutation gate against the new tests — adding a setter to finished, forcing each of the three changed completion guards always-on, and reverting the scaffolding worker to the private calls — and every mutant was caught red.

GPU coverage is left to CI: the streaming chat/completion examples with usage, the Responses and harmony streaming end-to-end tests, and both post-processing modes (test_openai_chat_example, test_openai_completions_example, test_openai_responses, test_openai_chat_harmony).

CI follow-up in 176d5870ca: added the missing AsyncGeneration/__init__.py so setuptools includes the streaming helper in the installed package. Package discovery omitted it before the fix. All 17 CPU scaffolding tests pass with the exact PR scaffolding loaded from build_py output against a compatible native runtime, including the five streaming-helper regressions. The run asserts that both the worker and AsyncGeneration handler were imported from the built package. Focused and commit hooks pass.

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.

🤖 Generated with Claude Code

[JunyiXu-nv]

/bot run

[tensorrt-cicd]

PR_Github #81840 [ run ] triggered by Bot. Commit: 254fa96 Link to invocation

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: NVIDIA/TensorRT-LLM
• Branch: main
• File: .coderabbit.yaml
• Commit: 5d86481
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Enterprise
• Run ID: c5d2be82-cd2d-48b3-8e31-d458aeb65271

Commits

Reviewing files that changed from the base of the PR and between 254fa96 and 176d587.

Files selected for processing: 1

• tensorrt_llm/scaffolding/contrib/AsyncGeneration/__init__.py

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

Walkthrough

GenerationResultBase now exposes a read-only finished property. Scaffolding code uses async iteration and the public property to track completion. Serving post-processors use the property to gate final output. Tests cover the property and updated streaming behavior.

Changes

Generation completion state

Layer / File(s)	Summary
Public completion property tensorrt_llm/executor/result.py, tests/unittest/llmapi/test_executor.py	GenerationResultBase exposes a read-only finished property backed by _done. GenerationResult inherits the property. Tests check its value and read-only behavior.
Scaffolding streaming tensorrt_llm/scaffolding/worker.py, tensorrt_llm/scaffolding/contrib/AsyncGeneration/stream_generation.py, tensorrt_llm/scaffolding/contrib/AsyncGeneration/__init__.py, tests/unittest/scaffolding/test_worker_streaming_step.py	Scaffolding helpers use async iteration and finished to track completion and set end flags. CPU-only tests check completion and step limits.
Serving completion checks tensorrt_llm/serve/postprocess_handlers.py, tensorrt_llm/serve/responses_utils.py, tests/unittest/serve/test_postprocess_finished.py, tests/unittest/llmapi/apps/*, tests/integration/test_lists/test-db/l0_cpu.yml	Serving post-processors use finished to gate final usage and Responses output. Tests and fixtures use the public completion field. The CPU test list includes the new post-processing test.

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

Change: Refactor

Suggested reviewers: juney-nvidia

Merge Risk: Minimal · up to 176d5

No actionable risk was identified in the reviewed change.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 20.83% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…	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 ticket, change type, and primary change: replacing private completion-state reads with the public finished property in serve and scaffolding.
Description check		The description follows the required template. It explains the issue and solution, documents scope and behavior, lists relevant tests and results, and includes the completed checklist.
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.

Full details: Docstring Coverage

Explanation

Docstring coverage is 20.83% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 24 functions across 14 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

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

[tensorrt-cicd]

PR_Github #81840 [ run ] completed with state SUCCESS. Commit: 254fa96
/LLM/main/L0_MergeRequest_PR pipeline #67492 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]

/bot run --disable-multi-gpu-test

[tensorrt-cicd]

PR_Github #81859 [ run ] triggered by Bot. Commit: 176d587 Link to invocation

[tensorrt-cicd]

PR_Github #81859 [ run ] completed with state SUCCESS. Commit: 176d587
/LLM/main/L0_MergeRequest_PR pipeline #67511 (Partly Tested) completed with status: 'SUCCESS'
Pipeline passed with automatic retried tests. Check the rerun report for details.

CI Report

Link to invocation