fix(streaming): suppress trailing empty STOP chunks with zero parts in SSE streaming#426
fix(streaming): suppress trailing empty STOP chunks with zero parts in SSE streaming#426kalenkevich wants to merge 1 commit into
Conversation
…n SSE streaming (#425)
There was a problem hiding this comment.
I performed a review of the changes:
-
Metadata Loss Bug in Non-Progressive Mode (Pre-existing but aggravated by PR changes):
InStreamingResponseAggregator.close(), iffinalPartsis undefined, the method returnsundefinedimmediately.
When the model only returns a function call (and no text parts) in non-progressive mode, the function call is yielded immediately inprocessResponse. The trailing emptySTOPchunk containing the finalusageMetadatais suppressed, and its metadata is saved tothis.usageMetadata. When the stream finishes andclose()is called,finalPartsis undefined (since no text was accumulated), soclose()returnsundefinedand the savedusageMetadatais completely lost.Proposed Fix:
Allowclose()to return anLlmResponsewith an empty parts array (parts: []) if any metadata (usageMetadata,groundingMetadata, orcitationMetadata) is present:close(): LlmResponse | undefined { const finalParts = this.strategy.close(); const hasMetadata = this.usageMetadata !== undefined || this.groundingMetadata !== undefined || this.citationMetadata !== undefined; if (!finalParts && !hasMetadata) { return undefined; } const candidate = this.response?.candidates?.[0]; const finishReason = this.finishReason ?? candidate?.finishReason; return { content: { role: 'model', parts: finalParts ?? [], }, groundingMetadata: this.groundingMetadata, citationMetadata: this.citationMetadata, errorCode: finishReason === FinishReason.STOP ? undefined : finishReason, errorMessage: finishReason === FinishReason.STOP ? undefined : candidate?.finishMessage, usageMetadata: this.usageMetadata, finishReason: finishReason, partial: false, }; }
Note: This bug existed before this PR, but since this PR changes the suppression logic, it is a great place to fix it.
-
Code Clean-up in
streaming_utils_test.ts:
In the new tests added,GenerateContentResponseis created manually:const response2 = new GenerateContentResponse(); response2.candidates = [{ finishReason: FinishReason.STOP }];
We should use the existing helper function
createResponse(...)defined at the top of the test file instead, to keep the test code clean and consistent:const response2 = createResponse({ finishReason: FinishReason.STOP });
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
In
StreamingMode.SSE(Server-Sent Events streaming mode), two bugs were observed when using Gemini models (e.g.,gemini-3.5-flash):Both symptoms share the same root cause:
finishReason: STOPand no content (e.g.,parts.length === 0).createLlmResponseconverts it to an event witherrorCode: FinishReason.STOPandcontent: undefined.isFinalResponse(event)returnstrue(since the event is empty, non-partial, and has no function calls), which prematurely terminates the runner's execution loop (runAsyncImplinLlmAgent), preventing the agent from calling the model with the tool results.Solution:
We updated
StreamingResponseAggregator.processResponsein streaming_utils.ts to comprehensively suppress all non-error empty chunks (chunks with no meaningful content and a finish reason ofundefinedorSTOP).partial: falsein the aggregator's final consolidated response fromclose().finishReason: SAFETY) are not suppressed, ensuring errors are still correctly reported to the user.Testing Plan
Unit Tests:
Summary of passed npm test results:
We added the following unit tests in streaming_utils_test.ts:
should capture metadata on trailing empty chunk with zero parts early return: Verifies progressive streaming mode empty STOP chunk suppression and metadata preservation.should suppress trailing empty STOP chunk with zero parts after a function call in non-progressive mode: Verifies tool call stream trailing empty chunk suppression in non-progressive mode.should suppress trailing empty STOP chunk for normal text streams in non-progressive mode: Verifies normal text stream trailing empty chunk suppression.should NOT suppress trailing empty chunk with non-STOP finish reason in non-progressive mode: Verifies that error finish reasons (e.g.SAFETY) are NOT suppressed and are correctly propagated.Manual End-to-End (E2E) Tests:
Manually tested using the
weather_time_agentsample in thedevpackage.npx adk web ./samplesin thedevdirectory."Hello, what can you do?"and"weather in New York".get_weathertool call finishes, the agent successfully receives the tool results and continues the execution loop to deliver the final response: "The weather in New York is currently sunny and 25°C (77°F)."Checklist
Additional context