Skip to content

fix: handle empty responses from Gemini API #7047

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/api/providers/__tests__/gemini.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,24 @@ describe("GeminiHandler", () => {
)
})

it("should handle empty response from API", async () => {
// Setup the mock to return an async generator with no text content
;(handler["client"].models.generateContentStream as any).mockResolvedValue({
[Symbol.asyncIterator]: async function* () {
// Yield only usage metadata, no text content
yield { usageMetadata: { promptTokenCount: 10, candidatesTokenCount: 0 } }
},
})

const stream = handler.createMessage(systemPrompt, mockMessages)

await expect(async () => {
for await (const _chunk of stream) {
// Should throw before yielding any chunks
}
}).rejects.toThrow(t("common:errors.gemini.generate_stream"))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test expects t("common:errors.gemini.generate_stream") but the actual error thrown is t("common:errors.gemini.empty_response"). While this works due to the error wrapping in the catch block, would it be clearer to test for the specific empty response error message to make the test's intent more obvious?

})

it("should handle API errors", async () => {
const mockError = new Error("Gemini API error")
;(handler["client"].models.generateContentStream as any).mockRejectedValue(mockError)
Expand Down
16 changes: 16 additions & 0 deletions src/api/providers/gemini.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ export class GeminiHandler extends BaseProvider implements SingleCompletionHandl

let lastUsageMetadata: GenerateContentResponseUsageMetadata | undefined
let pendingGroundingMetadata: GroundingMetadata | undefined
let hasYieldedContent = false // Track if we've yielded any text content

for await (const chunk of result) {
// Process candidates and their parts to separate thoughts from content
Expand All @@ -115,6 +116,7 @@ export class GeminiHandler extends BaseProvider implements SingleCompletionHandl
// This is regular content
if (part.text) {
yield { type: "text", text: part.text }
hasYieldedContent = true
}
}
}
Expand All @@ -124,13 +126,27 @@ export class GeminiHandler extends BaseProvider implements SingleCompletionHandl
// Fallback to the original text property if no candidates structure
else if (chunk.text) {
yield { type: "text", text: chunk.text }
hasYieldedContent = true
}

if (chunk.usageMetadata) {
lastUsageMetadata = chunk.usageMetadata
}
}

// Check if we got an empty response
if (!hasYieldedContent) {
// Log the issue for debugging
console.warn("Gemini API returned empty response, no text content was generated")
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is console.warn() intentional for production? Should we consider using a debug logger that can be toggled based on environment settings instead?


// Throw a specific error that can be caught and retried
throw new Error(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice we're throwing an error with t("common:errors.gemini.empty_response") here, but then it gets wrapped again with t("common:errors.gemini.generate_stream") in the catch block at line 174. This could lead to nested/confusing error messages. Should we consider throwing this error in a way that bypasses the outer catch, or perhaps use a custom error type that the catch block can handle differently?

t("common:errors.gemini.empty_response", {
error: "The Gemini API did not return any text content. This may be a temporary issue.",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message includes both a translated message and a hardcoded English fallback. Could we simplify this by ensuring all necessary context is in the translation files themselves, avoiding the redundancy?

}),
)
}

if (pendingGroundingMetadata) {
const citations = this.extractCitationsOnly(pendingGroundingMetadata)
if (citations) {
Expand Down
3 changes: 2 additions & 1 deletion src/i18n/locales/en/common.json
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@
"gemini": {
"generate_stream": "Gemini generate context stream error: {{error}}",
"generate_complete_prompt": "Gemini completion error: {{error}}",
"sources": "Sources:"
"sources": "Sources:",
"empty_response": "Gemini API returned empty response: {{error}}"
},
"cerebras": {
"authenticationFailed": "Cerebras API authentication failed. Please check your API key is valid and not expired.",
Expand Down
3 changes: 2 additions & 1 deletion src/i18n/locales/zh-CN/common.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading