Implement Retry with Backoff for Video Generation#27
Conversation
…te logic - Extracted `handleApiError` to `lib/api-utils.ts` to centralize SDK error to HTTP status mapping. - Created `withNodeRetry` helper in `lib/executors.ts` to encapsulate retry logic and UI updates. - Refactored `executeImageNode` and `executeVideoNode` to use the shared retry helper. - Added unit tests for executor retry logic. Co-authored-by: mblanc <662145+mblanc@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and maintainability of the application's AI generation features. By introducing centralized utilities for API error handling and retry mechanisms with exponential backoff, it ensures a more consistent and resilient user experience, especially during temporary service disruptions or rate limiting. This refactoring also streamlines the codebase, making future development and debugging more efficient. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements retry with backoff for video generation and refactors shared logic into handleApiError and withNodeRetry helpers, improving maintainability and consistency, and includes tests for the new retry logic. However, the current implementation of handleApiError and the retry logging mechanism are susceptible to information leakage, as raw error messages and full error objects could expose sensitive internal details, API keys, or PII. I recommend sanitizing error messages before sending them to the client and ensuring that logs only contain non-sensitive metadata or sanitized error descriptions. Additionally, I've included a few suggestions to further improve code clarity, reduce duplication, and enhance test performance.
| logger.warn( | ||
| `[Executor] Attempt ${attempt} failed for node ${nodeId}. Retrying in ${Math.round(delay)}ms...`, | ||
| error, | ||
| ); |
There was a problem hiding this comment.
The onRetry callback in withNodeRetry logs the full error object. This can lead to sensitive data exposure in logs if the error object contains secrets or PII. Consider sanitizing error messages before logging to prevent information leakage.
logger.warn(
`[Executor] Attempt ${attempt} failed for node ${nodeId}. Retrying in ${Math.round(delay)}ms...: ${error instanceof Error ? error.message : String(error)}`
);| return NextResponse.json( | ||
| { | ||
| error: errorMessage, | ||
| details: error instanceof Error ? error.message : String(error), | ||
| }, | ||
| { status }, | ||
| ); |
There was a problem hiding this comment.
The handleApiError function returns the raw error message to the client in both the error and details fields of the JSON response. If the error object contains sensitive information such as internal system paths, database connection strings, or API keys, this information will be exposed to the client. This is a classic information leakage vulnerability (CWE-209).
| return NextResponse.json( | |
| { | |
| error: errorMessage, | |
| details: error instanceof Error ? error.message : String(error), | |
| }, | |
| { status }, | |
| ); | |
| return NextResponse.json( | |
| { | |
| error: errorMessage, | |
| // details: error instanceof Error ? error.message : String(error), | |
| }, | |
| { status }, | |
| ); |
| } | ||
|
|
||
| export function handleApiError(error: unknown, fallbackMessage: string) { | ||
| logger.error(`[SERVER] ${fallbackMessage}:`, error); |
There was a problem hiding this comment.
The handleApiError function logs the full error object. If the error object contains sensitive information such as API keys, PII, or internal system details, this information will be stored in the server logs. This can lead to sensitive data exposure in logs (CWE-532).
| logger.error(`[SERVER] ${fallbackMessage}:`, error); | |
| logger.error(`[SERVER] ${fallbackMessage}: ${error instanceof Error ? error.message : String(error)}`); |
| it("executeVideoNode should fail after maximum retries", async () => { | ||
| const mockNode = { | ||
| id: "video-1", | ||
| data: { | ||
| type: "video", | ||
| prompt: "a jumping cat", | ||
| } as VideoData, | ||
| } as Node<VideoData>; | ||
|
|
||
| const onNodeUpdate = vi.fn(); | ||
| const mockFetch = vi.fn().mockResolvedValue({ | ||
| ok: false, | ||
| status: 429, | ||
| text: async () => "Rate limit exceeded", | ||
| }); | ||
|
|
||
| // withRetry uses exponential backoff. In tests we might want to mock timers | ||
| // but for now let's just see if it fails as expected. | ||
| // Note: Default maxRetries is 3 in withNodeRetry. | ||
|
|
||
| await expect( | ||
| executeVideoNode( | ||
| mockNode, | ||
| { prompt: "a jumping cat" }, | ||
| { | ||
| fetch: mockFetch as unknown as typeof fetch, | ||
| onNodeUpdate, | ||
| }, | ||
| ) | ||
| ).rejects.toThrow(/High traffic detected/); | ||
|
|
||
| expect(mockFetch).toHaveBeenCalledTimes(4); // initial + 3 retries | ||
| }, 20000); // Increase timeout for backoff |
There was a problem hiding this comment.
This test uses a 20-second timeout to wait for the exponential backoff, which can slow down the test suite. You can use vi.useFakeTimers() to mock the timers and make the test run instantly without relying on real time. This will make your tests faster and more deterministic.
it("executeVideoNode should fail after maximum retries", async () => {
vi.useFakeTimers();
const mockNode = {
id: "video-1",
data: {
type: "video",
prompt: "a jumping cat",
} as VideoData,
} as Node<VideoData>;
const onNodeUpdate = vi.fn();
const mockFetch = vi.fn().mockResolvedValue({
ok: false,
status: 429,
text: async () => "Rate limit exceeded",
});
const promise = executeVideoNode(
mockNode,
{ prompt: "a jumping cat" },
{
fetch: mockFetch as unknown as typeof fetch,
onNodeUpdate,
},
);
// Advance timers to allow all retries to execute
await vi.runAllTimersAsync();
await expect(promise).rejects.toThrow(/High traffic detected/);
expect(mockFetch).toHaveBeenCalledTimes(4); // initial + 3 retries
vi.useRealTimers();
});| if (error instanceof Error) { | ||
| errorMessage = error.message; | ||
| if ( | ||
| errorMessage.includes("429") || | ||
| errorMessage.includes("Quota") || | ||
| ("status" in error && (error as { status: number }).status === 429) | ||
| ) { | ||
| status = 429; | ||
| } | ||
| } else if ( | ||
| typeof error === "object" && | ||
| error !== null && | ||
| "status" in error | ||
| ) { | ||
| if ((error as { status: number }).status === 429) { | ||
| status = 429; | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for detecting a 429 status code is a bit complex and contains some repetition across the if and else if blocks. You can simplify this by consolidating the checks, which will make the function more readable and easier to maintain.
if (error instanceof Error) {
errorMessage = error.message;
}
const errorIsObject = typeof error === "object" && error !== null;
const hasStatus429 =
errorIsObject &&
"status" in error &&
(error as { status: unknown }).status === 429;
const messageHas429 =
error instanceof Error &&
(error.message.includes("429") || error.message.includes("Quota"));
if (hasStatus429 || messageHas429) {
status = 429;
}
mblanc
deleted the
feature/video-generation-retry-backoff-795834932259936455
branch
1 participant
This PR implements retry with exponential backoff for video generation, matching the existing behavior for image generation. It also deduplicates the retry and error handling logic by introducing common helpers:
handleApiErrorinlib/api-utils.ts: Centralizes the logic for mapping Google AI SDK errors (including 429 rate limits) to appropriate HTTP status codes.withNodeRetryinlib/executors.ts: A higher-order function that wraps node execution withwithRetry, handles UI status updates viacontext.onNodeUpdate, and manages 429-specific messaging.Both Image and Video executors now use these shared utilities, ensuring consistent behavior and easier maintenance. A new test suite
__tests__/retry-executors.test.tsverifies the retry behavior for both node types.PR created automatically by Jules for task 795834932259936455 started by @mblanc