Repository navigation
fix(agentx): preserve streaming liveness across profile cancellation - #45
janbernloehr wants to merge 2 commits into
Conversation
Signed-off-by: Jan Bernlöhr <jan@bernloehrs.de>
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@4eda12b426c03b5dea9f7938fdb85b958b9f8916Recommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@4eda12b426c03b5dea9f7938fdb85b958b9f8916Last updated for commit: |
|
/ok to test 70af1f5 |
ajcasagrande
left a comment
There was a problem hiding this comment.
I reviewed this PR at 70af1f53 against master (merge-base 56a0cf70). Keeping streaming evidence separate from successful-request accounting fixes the cancelled-tail gap without inventing completed requests. Nothing blocks merging; the inline suggestions are optional naming and comment cleanup.
Verified
- A real loopback HTTP/SSE cancellation probe emitted two rate-limited activity reports and exactly one first-token notification, without a completed record or credit return. The exact final content timestamp remained 150,377,035 ns newer than the last periodic report; cancellation did not advance it.
- 1,863 focused worker, credit, records, timing, transport and exporter tests passed (11 deselected), including stale-content rejection, phase isolation and missing-return coverage.
- The full unit run reported 17,616 passed and one failure,
test_completed_excludes_analyzer_metrics; that same failure reproduced on the base commit. The original production workload was not rerun.
🤖 The agent keeps its streaming contacts alive.
|
|
||
| Detecting first token requires parsing each SSE chunk, so this overhead | ||
| is skipped when the orchestrator doesn't need TTFT events for slot management. | ||
| """Observe content for prefill release and scenario coverage when needed. |
There was a problem hiding this comment.
Nit: _make_first_token_callback and on_first_token now also observe content throughout the stream, as this docstring describes. Optional: _make_content_callback / on_content would make that broader role easier to spot; there is no behavioral issue here.
🤖 A streaming agent wears more than one badge.
| return | ||
|
|
||
| # 1. ATOMIC COUNTING (no await before this!) | ||
| handler.progress.observe_streaming_content( |
There was a problem hiding this comment.
Nit: This insertion separates the atomic-counting header from the counting step it describes. Keep the header immediately above that step; the observation is synchronous, so the existing no-await invariant already holds.
- # 1. ATOMIC COUNTING (no await before this!)
handler.progress.observe_streaming_content(
credit_return.last_streaming_content_ns
)
+ # 1. ATOMIC COUNTING (no await before this!)🤖 The agent keeps its counting instructions in order.
| if self._on_first_token_callback: | ||
| # Forward TTFT to orchestrator so it can release the prefill slot. | ||
| await self._on_first_token_callback(message) | ||
| case StreamingContent(): |
There was a problem hiding this comment.
Nit: The message enumeration at sticky_router.py:603 omits this new variant; the PULL-channel note at :586 also names only CreditReturn/FirstToken. Add StreamingContent to both, or use generic wording so another variant cannot stale the list.
🤖 One more contact joins the agent's routing directory.
|
I reviewed this at End-to-end reproductionI reproduced the original failure and the fix with
The cancelled request adds liveness evidence without adding a completed request, as described. The trace generator and run script are available if you want to include them. With that, the end-to-end evidence is covered. What remains is test coverage for two of the paths: Asks1. The final timestamp carried by 2. The fallback return path is untested too. Removing Optional
Question (not blocking)With this change, a single request that is still streaming satisfies the gate even if the server has stopped admitting new requests and TTFT coverage is low. In the run above, TTFT coverage was 67.7% and the run passed. The body already says this is not a continuous-availability guarantee. Is that the intended scope of the stalled-server diagnosis, or should the FAQ say explicitly that admission stalls are out of scope? For what it's worth, I checked the cost of parsing every chunk: The only CI check that ran is "Validate PR title and add label". It fails with |
Signed-off-by: Jan Bernlöhr <jan@bernloehrs.de>
Bugbot is paused — your team has an unpaid invoiceBugbot is paused because your team has an unpaid invoice. Pay it from the billing page in the Cursor dashboard and Bugbot resumes automatically. (requestId: cc65ef15-844f-4e1a-a66b-58bd42412b85) |
|
@FrankD412 Addressed in 4eda12b: the final-return test now requires the newer credit-return timestamp to pass, and fallback-return and aggregate-timestamp coverage are added. Each corresponding mutation fails its test. Also corrected the field description and documented tool-call activity. |
FrankD412
left a comment
There was a problem hiding this comment.
Re-reviewed at 4eda12b4. The source changes in this commit are behavior-neutral (callback rename, field description, comment and docstring moves), and the three test gaps from my earlier comment are closed. Each mutation now fails a test:
| Mutation | Fails at 4eda12b4 |
|---|---|
Drop observe_streaming_content(credit_return.last_streaming_content_ns) in callback_handler.py |
test_cancelled_credit_content_survives_wire_and_phase_stats[True] |
Drop last_streaming_content_ns= from the fallback CreditReturn in worker.py |
test_credit_task_fallback_preserves_last_content[12000000000] |
max → min in the aggregate last_streaming_content_ns (records_tracker.py) |
test_aggregate_stats_keep_latest_streaming_content[latest-second], [latest-first] |
The focused suites (tests/unit/credit, tests/unit/timing, test_worker.py, test_records_manager.py, test_records_manager_process_results.py) give 1649 passed and 1 failed. The failure is test_completed_excludes_analyzer_metrics, which also fails on the merge-base 56a0cf70. ruff check and ruff format --check at the pinned 0.14.14 are clean on the changed files.
I reran the same mock-server reproduction as before against --scenario inferencex-agentx-mvp: one trace of about 200 s per request, concurrency 1, --benchmark-duration 900, --benchmark-grace-period 10, --unsafe-override. Both arms ran in parallel:
master merge-base 56a0cf70 |
PR head 4eda12b4 |
|
|---|---|---|
| Profiling phase | completed=4, cancelled=1, grace_period_timeout=True, 910 s |
same |
| TTFT / ITL coverage | 67.8% / 90.4% | 67.8% / 90.4% |
streaming_content_ratio |
— | 100% |
| Coverage gate | fails, exit 1 | passes, exit 0 |
request_count |
4 | 4 |
The cancelled request contributes coverage evidence only; request counts are unchanged. The FAQ wording on admission stalls answers my scope question. Approving.
|
I ran a hardware A/B of this PR through the InferenceX AgentX pipeline to check that observing streamed content costs nothing in throughput or latency. Setup. Both arms built the harness from a git commit, with nothing else changed:
The workload was Qwen3.5-397B-A17B-FP8 on B300 with SGLang, TP4, concurrency 32, a 3600 s profile and the
Per-run results and run links
InferenceX branches: |
|
The tests finally went through and it looks like the fix isn't affecting performance. Looks like we're within noise. 😃 |
Summary
Prevent
ProfileMetricCoverageErrorwhen a healthy, long-running streaming request is cancelled at the profiling boundary before it produces a completed metric record.Retain actual content-arrival timestamps independently of request completion. Workers send rate-limited activity updates while a request is in flight and attach the exact last content timestamp to its credit return. The coverage gate accepts this evidence alongside its existing TTFT and inter-token-latency signals, retaining the scenario's 95% threshold.
This also covers forced phase completion when the cancellation drain times out before the worker returns its credit: activity already delivered to the Timing Manager remains available for validation.
Error and fix
Why the current gate can reject a healthy run
The gate introduced in this repository measures the latest TTFT and inter-token-latency observations in completed metric records against the configured profiling duration. PR #41 relaxed the required ratio from 98% to 95% to tolerate sparse tails, but it did not preserve activity from a request cancelled before completion.
The failure sequence is:
Increasing the quiet-tail allowance only moves this boundary. A sufficiently long final request can exceed any fixed allowance. Attaching activity only to the final credit return is also insufficient because the cancellation drain can time out and force completion before that return arrives.
Synthetic example
The following values illustrate the regression independently of any particular model, server, dataset, or deployment:
Previously, this fails because neither 0.80 nor 0.90 reaches 0.95. With this change,
streaming_content_ratiois approximately 0.9944, so the coverage check passes. The cancelled request still contributes no successful-request count, throughput, or latency samples.A stream whose last content arrived at 1,709 seconds still fails. Cancellation, an open connection, and a positive in-flight count do not establish recent content activity.
Implementation
Worker observation. For a scenario that enables coverage validation, profiling workers continue using the existing SSE callback after the first meaningful response. The endpoint parser determines whether a chunk contains content. Its arrival timestamp is converted from the performance clock to wall-clock nanoseconds using a per-request clock offset. The observer records arrival time, not cancellation or credit-return time.
In-flight reporting. A new typed
StreamingContentmessage carries the phase, concrete phase index, and content timestamp over the existing worker-to-router PUSH/PULL return channel. The first content observation is reported immediately; further reports are limited to one per second per request. These messages do not return credits, change request counts, or release concurrency slots.Final observation.
CreditContextretains the latest timestamp, and both normal and fallback credit-return paths include it inCreditReturn.last_streaming_content_ns. This preserves the exact final observation, including a chunk that arrived between periodic reports.Phase accounting. The router forwards activity to the matching phase callback. The phase progress tracker retains the maximum timestamp, so older observations cannot move coverage backwards. Unknown or completed phases ignore activity updates. The timestamp flows through credit phase statistics into records phase statistics; aggregate statistics retain the maximum, while validation uses each concrete profiling phase's own statistics.
Validation and export.
ProfileMetricDurationCoverageaddsstreaming_content_ratio, defaulting to zero. The Records Manager calculates it as:Coverage passes when any of
ttft_ratio,inter_token_latency_ratio, orstreaming_content_ratioreachesrequired_ratio. Success and failure diagnostics include all three signals. The new ratio usesFiniteFloatand is bounded to [0, 1].Behavior preserved
inferencex-agentx-mvpthreshold remains 0.95.[DONE]do not advance content coverage for the chat endpoint. Text and reasoning content do.Scope and limitations
The coverage check establishes recent activity; it is not a continuous-availability or successful-completion guarantee. Content observed during the grace period is clamped to full coverage, consistent with the existing handling of completed-record timestamps beyond the configured window.
If a credit never returns, the gate uses the last delivered periodic observation. Because reports are limited to once per second, that observation can precede the exact final content timestamp by less than one second. No timestamp is fabricated when the worker has never reported content. A worker or communication failure that prevents all activity reports remains distinguishable from a healthy reported stream only through other diagnostics.
The additional work is endpoint parsing throughout streaming for coverage-enabled profiling requests, plus at most one activity message per second per active request. Other scenarios do not incur continuous parsing for this feature. No inference-server changes or GPU-specific behavior are required. Worker and Timing Manager processes should run the same harness revision because the typed return protocol gains a new message variant.
Validation
Deterministic reproduction of the missing-evidence failure
A prior CPU-only reproduction exercised the shipped
ColumnStoreandMetricsAccumulator.profile_metric_duration_coverageat754356e9,using synthetic request records:
The decisive change was retaining evidence from the unfinished request. The threshold and
configured duration stayed fixed. This establishes that discarding the straggler's observations
can cause the false positive without any inference-server failure. It validates the diagnosis and
proposed use of in-flight activity; it does not constitute an end-to-end run of this PR's patch.
The added record was an experimental input to isolate the cause, not the implementation proposed
here: this PR preserves activity separately from successful-request metrics.
Verification of this implementation
The regression tests carry streaming evidence through typed message serialization, the real
router and credit callback, phase progress statistics, the records tracker, and the coverage gate.
They establish the behaviors needed to fix the reproduced failure:
to complete.
latency samples remain unchanged.
A real loopback HTTP/SSE stream also exercised
AioHttpClientwith the worker callback. Itproduced rate-limited content reports before external task cancellation, without requiring a
completed record or credit return. This verifies that the transport supplies the evidence used by
the regression tests.
Validation covers the deterministic failure mechanism and the new activity-delivery path. The
patched harness has not been rerun against the original workload. Repository checks were also
run; the full suite's two failures reproduce on unchanged upstream.
Documentation
Updated the AgentX FAQ to explain cancelled-stream coverage, content filtering, and missing-return behavior. Updated the architecture and developer patterns to describe the activity-message path, phase scoping, callback contract, and unchanged accounting semantics.
Contribution checklist
Created with the assistance of Codex (GPT-6)
Note
Medium Risk
Changes AgentX run-validity logic and the worker/credit return protocol, but liveness evidence is scoped to coverage-only and does not alter successful-request or latency accounting.
Overview
Fixes false
insufficient_profile_metric_coveragefailures when a long streaming request is cancelled at the profiling boundary but was still producing content.Coverage gate. Profiling phases can now pass the scenario’s 95% duration check via a third signal,
streaming_content_ratio, alongside TTFT and inter-token latency. The ratio uses the latest parsed content timestamp (including cancelled credits with no metric record), clamped over the configured phase duration.Worker → timing path. For scenarios with
minimum_profile_metric_coverage_ratio, profiling workers keep the SSE callback active after first token, retainlast_streaming_content_nson the credit context, and emit rate-limitedStreamingContentmessages (≤1/s) on the existing return channel.CreditReturncarries the exact final timestamp; phase progress tracks the max and flows through records stats intoRecordsManagervalidation. Activity messages do not return credits, release slots, or add latency/throughput samples; prefill release still happens once viaFirstToken.Docs and tests cover AgentX FAQ behavior, architecture/patterns, and edge cases (missing returns, phase isolation, non-content chunks).
Reviewed by Cursor Bugbot for commit 70af1f5. Bugbot is set up for automated code reviews on this repo. Configure here.