-
Couldn't load subscription status.
- Fork 2.4k
fix: Tackling Race/State condition issue by Changing the Code Design for Gemini Grounding Sources #7434
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
fix: Tackling Race/State condition issue by Changing the Code Design for Gemini Grounding Sources #7434
Changes from 4 commits
a250a7e
edd6f3d
7e9a8c4
4636584
d428028
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,11 @@ | ||
| export type ApiStream = AsyncGenerator<ApiStreamChunk> | ||
|
|
||
| export type ApiStreamChunk = ApiStreamTextChunk | ApiStreamUsageChunk | ApiStreamReasoningChunk | ApiStreamError | ||
| export type ApiStreamChunk = | ||
| | ApiStreamTextChunk | ||
| | ApiStreamUsageChunk | ||
| | ApiStreamReasoningChunk | ||
| | ApiStreamGroundingChunk | ||
| | ApiStreamError | ||
|
|
||
| export interface ApiStreamError { | ||
| type: "error" | ||
|
|
@@ -27,3 +32,14 @@ export interface ApiStreamUsageChunk { | |
| reasoningTokens?: number | ||
| totalCost?: number | ||
| } | ||
|
|
||
| export interface ApiStreamGroundingChunk { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding JSDoc documentation for the new grounding types to clarify their purpose. For example: /**
* Represents grounding metadata from search results or citations.
* Used to decouple source information from the main content stream.
*/
export interface ApiStreamGroundingChunk {
type: "grounding"
sources: GroundingSource[]
} |
||
| type: "grounding" | ||
| sources: GroundingSource[] | ||
| } | ||
|
|
||
| export interface GroundingSource { | ||
| title: string | ||
| url: string | ||
| snippet?: string | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,7 +41,7 @@ import { CloudService, BridgeOrchestrator } from "@roo-code/cloud" | |
|
|
||
| // api | ||
| import { ApiHandler, ApiHandlerCreateMessageMetadata, buildApiHandler } from "../../api" | ||
| import { ApiStream } from "../../api/transform/stream" | ||
| import { ApiStream, GroundingSource } from "../../api/transform/stream" | ||
|
|
||
| // shared | ||
| import { findLastIndex } from "../../shared/array" | ||
|
|
@@ -1783,7 +1783,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| this.didFinishAbortingStream = true | ||
| } | ||
|
|
||
| // Reset streaming state. | ||
| // Reset streaming state for each new API request | ||
| this.currentStreamingContentIndex = 0 | ||
| this.currentStreamingDidCheckpoint = false | ||
| this.assistantMessageContent = [] | ||
|
|
@@ -1804,6 +1804,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| const stream = this.attemptApiRequest() | ||
| let assistantMessage = "" | ||
| let reasoningMessage = "" | ||
| let pendingGroundingSources: GroundingSource[] = [] | ||
| this.isStreaming = true | ||
|
|
||
| try { | ||
|
|
@@ -1830,6 +1831,13 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| cacheReadTokens += chunk.cacheReadTokens ?? 0 | ||
| totalCost = chunk.totalCost | ||
| break | ||
| case "grounding": | ||
| // Handle grounding sources separately from regular content | ||
| // to prevent state persistence issues - store them separately | ||
| if (chunk.sources && chunk.sources.length > 0) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good separation of concerns here! The grounding sources are now properly handled independently from the assistant message content, which should prevent the race condition issues mentioned in #6372. |
||
| pendingGroundingSources.push(...chunk.sources) | ||
| } | ||
| break | ||
| case "text": { | ||
| assistantMessage += chunk.text | ||
|
|
||
|
|
@@ -2123,6 +2131,16 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| let didEndLoop = false | ||
|
|
||
| if (assistantMessage.length > 0) { | ||
| // Display grounding sources to the user if they exist | ||
| if (pendingGroundingSources.length > 0) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice implementation detail - storing grounding sources separately and only displaying them to the user while keeping the API history clean. This prevents the state persistence issues that were causing the race condition. |
||
| const citationLinks = pendingGroundingSources.map((source, i) => `[${i + 1}](${source.url})`) | ||
| const sourcesText = `${t("common:gemini.sources")} ${citationLinks.join(", ")}` | ||
|
|
||
| await this.say("text", sourcesText, undefined, false, undefined, undefined, { | ||
| isNonInteractive: true, | ||
| }) | ||
| } | ||
|
|
||
| await this.addToApiConversationHistory({ | ||
| role: "assistant", | ||
| content: [{ type: "text", text: assistantMessage }], | ||
|
|
||
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.
Consider adding debug logging when sources are filtered out to help with troubleshooting malformed grounding metadata during development. This would make it easier to identify issues with the grounding chunks.