fix: normalize multi-round gateway streaming by harivilasp · Pull Request #136 · vllm-project/agentic-api

harivilasp · 2026-07-21T04:19:23Z

Summary

Normalize multi-round gateway streaming into one client-visible event sequence.
Preserve lossless upstream event frames while emitting gateway-executed built-in tool lifecycle events.
Supersedes fix: normalize multi-round gateway streaming #132, which GitHub closed automatically after its source branch was renamed.

Test Plan

cargo fmt -- --check
cargo clippy --all-targets -- -D warnings
cargo test
pre-commit run --all-files

Signed-off-by: harivilasp <harivilasp@gmail.com>

ashwing

Reworked cleanly — the accumulator is owned now (no Arc<Mutex> guard held across .await), error frames route through error_chunk so they stay numbered, the synthetic/deferred events share one EventFrame/process_event path, and the multi-round test covers single-lifecycle + monotonic sequence + cross-round output_index. One question on normalize_sse_line below; non-blocking.

ashwing · 2026-07-21T05:22:48Z

-        .map_or(SSEEventType::Other, classify_event_type);
-
-    let sequence_number = json.get("sequence_number").and_then(Value::as_u64);
+    let event_type = SSEEventType::from(json.get("type")?.as_str()?);


The switch to SSEEventType::from(json.get("type")?.as_str()?) drops any data: frame that parses as JSON but has no string type — previously map_or(SSEEventType::Other, ...) kept it and forwarded it as Other. Given the "preserve lossless upstream frames" goal, is dropping intended here? Responses SSE always carries type in practice, so likely benign, but a typeless keepalive/data frame from a provider would now vanish silently instead of passing through. Either keeping the Other fallback for the type-missing case (distinct from malformed JSON, which should still drop) or a one-line comment noting the deliberate drop would make the intent clear — plus an assert!(normalize_sse_line("data: {\"foo\":1}").is_none()) (or .is_some()) to lock it in; the normalizer tests currently cover [DONE], malformed JSON, and unknown type, but not valid-JSON-without-type.

Fixed in ae0f5b1. A missing or non-string type maps to Other; WireEvent preserves the absent wire field during serialization, so the frame relays losslessly. Added test_typeless_json_event_is_preserved.

maralbahari

@harivilasp thanks for the changes. I added a few concerns inline comments.

maralbahari · 2026-07-21T04:29:32Z

                            let ch = exec_ctx.conv_handler.clone();
                            let rh = exec_ctx.resp_handler.clone();
-                            if let Err(e) = persist_if_needed(payload, ctx, ch, rh).await {
+                            if let Err(e) = persist_if_needed(payload.clone(), ctx, ch, rh).await {


why there is a need for payload.clone() ? the older version didnt require it.

Removed in ae0f5b1. The terminal SSE chunk is now prepared before persistence, then the payload moves into persist_if_needed. This keeps persistence before client-visible emission without cloning the payload.

maralbahari · 2026-07-21T04:58:01Z

                .entry(item_id.clone())
                .or_default()
-                .push(line.to_owned());
+                .push(frame.clone());


the entire frame.clone() seems too expensive. see how can be it be avoided.

Removed in ae0f5b1. ResponseAccumulator now processes the frame before stream handling, and deferred EventFrames are moved into the pending queue and moved out again on flush. No deep frame clone remains.

maralbahari · 2026-07-21T05:19:04Z

+    pub(crate) fn terminal_response_chunk(&mut self, payload: &ResponsePayload) -> ExecutorResult<Option<String>> {
+        let mut frame = terminal_response_frame(payload)?;
+        if !self.process_event(&mut frame, 0) {
+            return Ok(None);


A payload with final status in_progress maps to SSEEventType::ResponseInProgress (line 95), which the lifecycle dedup has usually already consumed from the relayed stream, so this returns Ok(None) and the client gets [DONE] with no terminal response snapshot. The terminal event is not the same thing as the early lifecycle announcement; the previous revision always emitted it, and no test covers this case.

Fixed in ae0f5b1. Terminal frames now bypass lifecycle dedup but still use the shared sequence stamper, so a final in_progress response is always emitted with the next sequence number. Added emits_in_progress_terminal_event_after_lifecycle_event.

maralbahari · 2026-07-21T05:26:17Z

-                            yield terminal_event;
+                            match stream_accumulator.terminal_response_chunk(&payload) {
+                                Ok(Some(chunk)) => yield chunk,
+                                Ok(None) => {}


Ok(None) seems to be regression in behavior compared to the older version. Silently dropping the terminal chunk means the stream ends with bare [DONE] and no final response state. If Ok(None) is really possible here, it needs a log and a test; otherwise make the terminal chunk unsuppressible and delete this arm.

Fixed in ae0f5b1 alongside the terminal lifecycle fix. terminal_response_chunk now always returns a serialized terminal frame, so the bare DONE path and the Ok(None) arm are gone.

Signed-off-by: harivilasp <harivilasp@gmail.com>

harivilasp · 2026-07-21T06:37:15Z

Thank you for the detailed review. All five points are addressed in ae0f5b1, with focused regressions plus the full local test and lint suite.

harivilasp added 5 commits July 20, 2026 21:03

fix: normalize multi-round gateway streaming

dd14587

Signed-off-by: harivilasp <harivilasp@gmail.com>

fix: address gateway stream review feedback

e073097

Signed-off-by: harivilasp <harivilasp@gmail.com>

test: tighten gateway stream event ordering

5954c8a

Signed-off-by: harivilasp <harivilasp@gmail.com>

fix: unify gateway stream event frames

07b913f

Signed-off-by: harivilasp <harivilasp@gmail.com>

fix: complete gateway stream review updates

10edb4c

Signed-off-by: harivilasp <harivilasp@gmail.com>

harivilasp requested review from bbrowning, franciscojavierarceo, jiahuei, leseb, maralbahari, noobHappylife, qandrew and tjtanaa as code owners July 21, 2026 04:19

harivilasp mentioned this pull request Jul 21, 2026

fix: normalize multi-round gateway streaming #132

Closed

ashwing approved these changes Jul 21, 2026

View reviewed changes

maralbahari reviewed Jul 21, 2026

View reviewed changes

fix: preserve gateway stream terminal frames

ae0f5b1

Signed-off-by: harivilasp <harivilasp@gmail.com>

Uh oh!

Conversation

harivilasp commented Jul 21, 2026

Summary

Test Plan

Uh oh!

ashwing left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maralbahari left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harivilasp commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants