fix(session): stamp COMPLETE with the arrival time of its chunks by kimseunghyun-kr · Pull Request #536 · mlcommons/endpoints

kimseunghyun-kr · 2026-10-14T06:46:22Z

What does this PR do?

Fixes #533: in streaming runs, a sample's sample.complete could be stamped before its own sample.recv_first, which gave a negative TPOT.

Cause. The ZMQ transport decodes every waiting message of one drain before the session handles any of them. QueryResult.completed_at is set at decode, but RECV_FIRST / RECV_NON_FIRST are stamped when the session handles each chunk. So when a response's chunks and its final result arrive in the same drain, COMPLETE is older than the chunks.

Fix. COMPLETE keeps QueryResult.completed_at (decode time), but it is never earlier than the response's own chunks. The session keeps the handling stamp of each streaming response's latest chunk (now_ns in _receive_responses) and pops it at the result; COMPLETE is max(completed_at, last chunk stamp). Only the same-drain case moves COMPLETE. A response without chunks (non-streaming) keeps completed_at, so COMPLETE - ISSUED latency and OSL/s do not change.

The design doc (docs/load_generator/DESIGN.md, "Timestamp fidelity" and the sequence diagram) is updated to match.

Type of change

Bug fix
New feature
Documentation update
Refactor/cleanup

Related issues

Closes #533

Testing

Tests added/updated
- New test_streaming_complete_not_before_chunks_of_one_drain. It runs a real session in which the QueryResult is created first (as a decode in the same drain would), then the chunks and the result are delivered. It checks that RECV_FIRST ≤ RECV_NON_FIRST ≤ COMPLETE. Without the fix it fails: COMPLETE is about 11 ms before the chunks.
- New test_complete_stamp_is_decode_time_floored_by_own_chunks: a response whose chunk was handled after its decode gets the chunk's stamp; one whose chunk came first keeps completed_at; the per-query state is empty afterwards.
- The _handle_response test checks that a response without chunks keeps completed_at.
- Both new tests fail when the floor is removed.
All tests pass locally
- tests/unit/load_generator/test_async_session.py: 63 passed.
- pytest -m unit on macOS: 2,194 passed, 63 failed. The same 63 tests fail on main in the same environment (62 in metrics_aggregator, plus the Linux-only os.sched_setaffinity test); the failure lists are identical.
Manual testing completed

Checklist

Code follows project style
Pre-commit hooks pass. ruff, ruff-format, prettier, license headers and the PR size check pass. On macOS, mypy reports only the Linux-only os.sched_*affinity attributes in cpu_affinity.py and token_metrics.py. Those files are not touched here, and main gives the same errors.
Documentation updated (if needed)

Note for reviewers. The issue lists a second option: stamp chunks at decode time too, so the client's own backlog is also removed from TTFT. That needs a new decode-time field on StreamChunk (more bytes on every chunk), so it is not in this focused fix. I can do it as a follow-up if you prefer it.

AI assistance: prepared with Claude Code. I reviewed every change.

🤖 Generated with Claude Code

In streaming runs a sample's COMPLETE could be earlier than its own RECV_FIRST, which gave a negative TPOT. The ZMQ transport decodes a whole drain before the session handles any message, so QueryResult.completed_at (set at decode) predates the handling-time stamp of chunks from the same drain. COMPLETE now uses the same per-response monotonic_ns() read as the RECV_FIRST/RECV_NON_FIRST events. Closes mlcommons#533 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-14T06:46:33Z

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

Copilot

🟡 Changes recommended

The design document still contains a contradictory description of the old completion timestamp behavior.

1 open finding

Update duplicate timestamp documentation to match current behavior · New

What changed in this PR

Fixes negative streaming TPOT by timestamping COMPLETE when responses are handled.

Changes:

Uses arrival timestamps for completion events.
Adds streaming ordering regression tests.
Updates timestamp documentation and diagrams.

File	Summary
`tests/unit/load_generator/test_async_session.py`	Adds regression coverage and updates timestamp expectations.
`src/inference_endpoint/load_generator/session.py`	Uses handling-time timestamps for `COMPLETE`.
`docs/load_generator/DESIGN.md`	Documents the revised timestamp behavior.

🧠 Review effort: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

kimseunghyun-kr · 2026-10-14T07:02:30Z

recheck

The "Timestamp Fidelity" section and its _handle_response example still described COMPLETE as stamped with QueryResult.completed_at. Both now match the implementation: one arrival stamp per response, shared by the chunk events and COMPLETE. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot

🟡 Changes recommended

Update the design documentation’s timestamp explanation and remaining _handle_response example.

1 open finding

Update _receive_responses example for now_ns argument · New

1 resolved since last review

Update duplicate timestamp documentation to match current behavior

🧠 Review effort: Lite

The _receive_responses example called _handle_response(resp) and read the clock again for the liveness stamp. It now reads monotonic_ns() once per response and passes it to _handle_response, as the implementation does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

kimseunghyun-kr · 2026-10-14T08:05:27Z

recheck

Copilot

🟢 Approval recommended

The focused fix, tests, and documentation updates address the reported timing issue with no unresolved review concerns.

0 open findings

1 resolved since last review

Update _receive_responses example for now_ns argument

🧠 Review effort: Lite

arekay-nv · 2026-10-14T15:43:01Z

@kimseunghyun-kr Please complete the CLA process:

Thank you very much for your submission; we really appreciate it. Before we can accept your contribution,
we ask that you sign the MLCommons CLA (Apache 2). Please submit your GitHub ID to our onboarding form to initiate
authorization. If you are from a MLCommons member organization, we will request that you be added to the CLA.
If you are not from a member organization, we will email you a CLA to sign. For any questions, please contact
support@mlcommons.org.

codecov-commenter · 2026-10-22T21:50:05Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@4b385eb). Learn more about missing BASE report.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #536   +/-   ##
=======================================
  Coverage        ?   81.38%           
=======================================
  Files           ?      157           
  Lines           ?    22482           
  Branches        ?        0           
=======================================
  Hits            ?    18296           
  Misses          ?     4186           
  Partials        ?        0

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

arekay-nv · 2026-10-26T17:21:33Z

@kimseunghyun-kr please complete the CLA so we can review the PR.

MLCommons CLA bot: Thank you very much for your submission; we really appreciate it. Before we can accept your contribution, we ask that you sign the MLCommons CLA (Apache 2). Please submit your GitHub ID to our onboarding form to initiate authorization. If you are from a MLCommons member organization, we will request that you be added to the CLA. If you are not from a member organization, we will email you a CLA to sign. For any questions, please contact support@mlcommons.org.1 out of 2 committers have signed the MLCommons CLA.✅ @arekay-nv❌ @kimseunghyun-krYou can retrigger this bot by commenting recheck in this Pull Request

Copilot

🔵 Needs a closer look

The ordering fix is correct, but it also changes the reported COMPLETE-based latency and OSL/s for all runs, and maintainers need to decide whether to accept or document that shift.

2 open issues

_{Review effort: Lite}

Reilly-Fairbanks · 2026-10-28T16:00:43Z

@kimseunghyun-kr Please sign MLC CLA in order to get this PR merged. If you have questions on how to complete this please reach out to me at reillyfairbanks@mlcommons.org

…ater Review feedback: stamping COMPLETE at handling time also moved COMPLETE for non-streaming responses, so client backlog entered COMPLETE - ISSUED latency and OSL/s. COMPLETE now keeps QueryResult.completed_at, and only a response whose own chunks were handled later (same transport drain) takes its last chunk's stamp. The session keeps that stamp per query and pops it at the result. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: kimseunghyun <incongnito12@gmail.com>

kimseunghyun-kr · 2026-11-02T02:45:16Z

recheck

Copilot

🟡 Changes recommended

Active documentation and session-state issues remain, including an unqualified timestamp claim and retention of stale or timed-out stream entries.

3 open issues

1 resolved since last review

Document or limit handling-time stamps to streaming responses

_{Review effort: Lite}

…LETE bias Chunks of a previous phase or of a terminated query may never get a QueryResult, so their stamps would stay in _last_chunk_ns for the whole session. Keep a stamp only while the query is active in the current phase (any query when no phase is set). The design doc now says that COMPLETE includes the session's handling backlog when it is floored to the last chunk. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: kimseunghyun <incongnito12@gmail.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: kimseunghyun <incongnito12@gmail.com>

Copilot

🔵 Needs a closer look

A human assessment is warranted because the streamed-query timeout and cleanup lifecycle has a subtle state-retention risk that is difficult to validate automatically.

No open issues 🎉

3 resolved since last review

_{Review effort: Lite}

kimseunghyun-kr · 2026-11-02T08:53:57Z

The audit failure is msgpack 1.2.1 (CVE-2026-114613), the same on main; #551 fixes it. I will merge main into this PR after #551 lands.

(Drafted with AI assistance; I reviewed it.)

Copilot

🔵 Needs a closer look

Human assessment is needed because timed-out streamed queries may leave per-query timestamps retained for the session lifetime, creating a moderate resource-lifecycle concern in a complex code path.

No open issues 🎉

_{Review effort: Lite}

kimseunghyun-kr requested review from a team and a lite review from Copilot October 14, 2026 06:46

github-actions Bot added the size/normal PR Review Policy: <=500 non-test lines & <=20 files label Oct 14, 2026

Copilot started reviewing on behalf of kimseunghyun-kr October 14, 2026 06:46

View session

Copilot AI reviewed Oct 14, 2026

View reviewed changes

Comment thread docs/load_generator/DESIGN.md Resolved

Copilot AI lite review requested due to automatic review settings October 14, 2026 07:05

Copilot started reviewing on behalf of kimseunghyun-kr October 14, 2026 07:05

View session

Copilot AI reviewed Oct 14, 2026

View reviewed changes

Comment thread docs/load_generator/DESIGN.md Resolved

Copilot AI lite review requested due to automatic review settings October 14, 2026 08:05

Copilot started reviewing on behalf of kimseunghyun-kr October 14, 2026 08:05

View session

Copilot AI reviewed Oct 14, 2026

View reviewed changes

arekay-nv requested a review from nv-alicheng October 14, 2026 15:43

viraatc approved these changes Oct 22, 2026 •

edited

Loading

View reviewed changes

Merge branch 'main' into fix/complete-arrival-timestamp

ecfd49e

Copilot AI lite review requested due to automatic review settings October 26, 2026 17:20

Copilot started reviewing on behalf of kimseunghyun-kr October 26, 2026 17:20

View session

Copilot AI reviewed Oct 26, 2026

View reviewed changes

Comment thread src/inference_endpoint/load_generator/session.py Resolved

Comment thread docs/load_generator/DESIGN.md Resolved

Copilot AI lite review requested due to automatic review settings November 2, 2026 02:44

Copilot started reviewing on behalf of kimseunghyun-kr November 2, 2026 02:45

View session

Copilot AI reviewed Nov 2, 2026

View reviewed changes

Comment thread src/inference_endpoint/load_generator/session.py Resolved

Comment thread docs/load_generator/DESIGN.md Resolved

Copilot AI lite review requested due to automatic review settings November 2, 2026 08:49

Copilot started reviewing on behalf of kimseunghyun-kr November 2, 2026 08:49

View session

docs(design): prettier wrap

6287873

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: kimseunghyun <incongnito12@gmail.com>

Copilot AI reviewed Nov 2, 2026

View reviewed changes

Copilot AI lite review requested due to automatic review settings November 2, 2026 08:52

Copilot started reviewing on behalf of kimseunghyun-kr November 2, 2026 08:53

View session

Copilot AI reviewed Nov 2, 2026

View reviewed changes

Conversation

kimseunghyun-kr commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Type of change

Related issues

Testing

Checklist

Uh oh!

github-actions Bot commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

kimseunghyun-kr commented Oct 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

kimseunghyun-kr commented Oct 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

arekay-nv commented Oct 14, 2026

Uh oh!

Uh oh!

codecov-commenter commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arekay-nv commented Oct 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Uh oh!

Uh oh!

Reilly-Fairbanks commented Oct 28, 2026

Uh oh!

kimseunghyun-kr commented Nov 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

kimseunghyun-kr commented Nov 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kimseunghyun-kr commented Oct 14, 2026 •

edited

Loading

github-actions Bot commented Oct 14, 2026 •

edited

Loading

codecov-commenter commented Oct 22, 2026 •

edited

Loading