Repository navigation
[API server][Conductor][Critical fix] Hold a finished request until every emitted chunk has arrived #551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
merceod
merged 3 commits into
mstar-project:main
from
arun-elr:fix/delivery-emission-counts
Oct 20, 2026
Merged
[API server][Conductor][Critical fix] Hold a finished request until every emitted chunk has arrived #551
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#419 (38113a4) shares output_loop_idxs with the data worker thread under _admit_lock, its thread teardown drops the entry, and its received_final_chunks treats a missing entry as done unless an error is held. Whoever rebases second needs to carry the rename into that teardown, keep the increment under the lock and keep the None branch, otherwise a request #419 tore down reads as 0 received here and sits to the 15 s TTL with a spurious not-delivered error. 8 textual hunks in this file plus 2 in entrypoint.py, the rest merges clean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the runs. Merge order: #551 first, then #419 rebases onto it; both bodies now say so. No change to #551's code since the approval.
The resolution is ready and tested on main + #551 + #419: #419's side of
data_worker.pyandentrypoint.py, with the counts carried into every use, including the worker thread's constructor argument and_drop_producer_state, both increments kept under_admit_lock, and theNonebranch kept. Both PRs' test files pass on that tree (247 passed, 1 skipped). Breaking each of the three things on that tree fails a #419 test:output_loop_idxstest_cancel_queued_then_new_request_then_drain_cleans_mapsNonebranch droppedtest_teardown_between_the_pending_check_and_the_read_completes_normally_admit_locktest_new_result_token_waits_for_the_admit_lock_and_respects_teardownThe rebase also adds
test_a_request_torn_down_short_of_its_chunk_count_completes_without_the_ttl, the case here: completion reports 3 chunks, 1 arrived, then the thread tears the request down. It completes at once with what arrived and no error, and fails if theNonebranch is lost. If #419 lands first instead, #551's rebase takes the same tree.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good on the merge order. One small thing for the description while you are in it, the phrase about any AR model with separate prefill and decode worker graphs is wider than the code. Qwen3.5 does not reach this because its prefill and decode use different output names (qwen3_5_model.py:274-279 vs 291-296), Qwen3-Omni does because both emit new_token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in the body: it now names models whose prefill and decode emit under the same output name, such as Qwen3-Omni (
new_tokenfrom both), and says Qwen3.5 does not reach it (new_tokenfrom prefill,text_inputsfrom decode). No code change.