Repository navigation
Conversation
WalkthroughThis pull request introduces video creation timeout handling throughout the gateway and playground services. It adds timeout configuration functions, implements upstream timeout detection with fallback to alternate providers when video creation requests exceed configured time limits, and includes mock server support for testing provider-specific timeout scenarios. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Gateway
participant Provider1 as Provider 1<br/>(google-vertex)
participant Provider2 as Provider 2<br/>(avalanche)
Client->>Gateway: POST /v1/videos<br/>(video create request)
Gateway->>Provider1: Create video request<br/>(with timeout signal)
Provider1-->>Gateway: Timeout (exceeds<br/>AI_VIDEO_CREATE_TIMEOUT_MS)
Gateway->>Gateway: Detect timeout error<br/>Log warning + 504 candidate
Gateway->>Provider2: Retry: Create video request<br/>(next provider in eligibility)
Provider2->>Provider2: Process video creation
Provider2-->>Gateway: 200 OK + video ID
Gateway->>Gateway: Record routing metadata:<br/>Provider1 (504/upstream_error)<br/>Provider2 (200/none)
Gateway-->>Client: 200 OK<br/>(video job persisted)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds explicit, configurable timeouts around upstream video creation so requests don’t hang indefinitely, and ensures both the gateway and playground proxy return a clear 504 on timeout while preserving provider fallback behavior.
Changes:
- Add video-create-specific timeout configuration helpers and apply them to upstream fetches in the gateway.
- Add a timeout + 504 JSON response to the playground
/api/videoproxy route. - Extend mock server triggers and add gateway tests covering timeout errors and timeout-driven fallback.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/playground/src/app/api/video/route.ts | Adds an abort timeout to the playground proxy so the UI gets a JSON 504 instead of hanging. |
| apps/gateway/src/videos/videos.ts | Wraps upstream video fetches with a video-create timeout signal and returns HTTP 504 on timeout. |
| apps/gateway/src/videos/videos.spec.ts | Adds test coverage for upstream create timeouts and fallback after a timeout. |
| apps/gateway/src/test-utils/mock-openai-server.ts | Adds provider-specific timeout delay triggers to simulate stuck upstreams in tests. |
| apps/gateway/src/lib/timeout-config.ts | Introduces AI_VIDEO_CREATE_TIMEOUT_MS-backed timeout getter + AbortSignal factory for video creation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| : await c.req.json(); | ||
| const prompt = typeof body.prompt === "string" ? body.prompt : ""; | ||
| const timeoutDelay = extractProviderSpecificTimeoutDelay(prompt, "obsidian"); | ||
| if (timeoutDelay) { |
There was a problem hiding this comment.
extractProviderSpecificTimeoutDelay can legitimately return 0 (e.g., ..._0), but the current if (timeoutDelay) check treats 0 as falsy and skips the delay. Use an explicit null check (e.g., timeoutDelay !== null) so the behavior matches the trigger value exactly.
| if (timeoutDelay) { | |
| if (timeoutDelay !== null) { |
| const body = await c.req.json(); | ||
| const prompt = typeof body.prompt === "string" ? body.prompt : ""; | ||
| const timeoutDelay = extractProviderSpecificTimeoutDelay(prompt, "avalanche"); | ||
| if (timeoutDelay) { |
There was a problem hiding this comment.
extractProviderSpecificTimeoutDelay can return 0, but if (timeoutDelay) treats it as falsy and won’t apply the delay. Prefer checking timeoutDelay !== null (or typeof timeoutDelay === 'number') before calling delay(...).
| if (timeoutDelay) { | |
| if (typeof timeoutDelay === "number") { |
| prompt, | ||
| "vertex", | ||
| ); | ||
| if (timeoutDelay) { |
There was a problem hiding this comment.
extractProviderSpecificTimeoutDelay returns number | null; using if (timeoutDelay) skips the delay for 0. Switch to an explicit null check so ..._0 behaves as intended and to avoid relying on truthiness.
| if (timeoutDelay) { | |
| if (timeoutDelay !== null) { |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: caef6dafce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| logger.warn("Upstream video create request timed out", { | ||
| url, | ||
| timeoutMs: getVideoCreateTimeoutMs(), | ||
| }); | ||
| throw new HTTPException(504, { |
There was a problem hiding this comment.
The new 504 here is treated as retryable by the outer loop in apps/gateway/src/videos/videos.ts:2811-2844, so a timeout on provider A now immediately sends the same create request to provider B. That is unsafe for these side-effecting POSTs: createObsidianVideoJob and createAvalancheVideoJob build plain create payloads with no idempotency key or cancellation handle, so a slow provider can still enqueue the first job after our client gives up. In that case one user request can create two upstream videos (and two billable jobs), while we only persist the fallback attempt.
Useful? React with 👍 / 👎.
| body: JSON.stringify(requestBody), | ||
| signal: AbortSignal.timeout(getVideoCreateProxyTimeoutMs()), |
There was a problem hiding this comment.
This new timeout makes the playground return a 504 after ~55s, but the gateway video-create path never threads the incoming request abort signal into its upstream work (I checked apps/gateway/src/videos/videos.ts and the create helpers only use their own timeout signal). For requests that spend a long time preprocessing/uploading images or timing out on one provider before falling back, the UI can now report failure while /v1/videos keeps running and eventually inserts a videoJob; if the user retries after that JSON error, they can duplicate the generation and billing.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/playground/src/app/api/video/route.ts (1)
10-17: Add parentheses to clarify operator precedence.ESLint flags the mixed
*and-operators. While the current evaluation order is correct, adding explicit parentheses improves readability.Suggested fix
function getVideoCreateProxyTimeoutMs(): number { const envValue = Number(process.env.PLAYGROUND_VIDEO_CREATE_TIMEOUT_MS); if (envValue > 0) { return envValue; } - return Math.max(1000, maxDuration * 1000 - 5000); + return Math.max(1000, (maxDuration * 1000) - 5000); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/playground/src/app/api/video/route.ts` around lines 10 - 17, The expression in getVideoCreateProxyTimeoutMs uses mixed * and - operators which ESLint flags; update the fallback return to use explicit parentheses so precedence is clear — change Math.max(1000, maxDuration * 1000 - 5000) to Math.max(1000, (maxDuration * 1000) - 5000) (referring to getVideoCreateProxyTimeoutMs, PLAYGROUND_VIDEO_CREATE_TIMEOUT_MS and maxDuration).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/playground/src/app/api/video/route.ts`:
- Around line 10-17: The expression in getVideoCreateProxyTimeoutMs uses mixed *
and - operators which ESLint flags; update the fallback return to use explicit
parentheses so precedence is clear — change Math.max(1000, maxDuration * 1000 -
5000) to Math.max(1000, (maxDuration * 1000) - 5000) (referring to
getVideoCreateProxyTimeoutMs, PLAYGROUND_VIDEO_CREATE_TIMEOUT_MS and
maxDuration).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 39703491-3273-433c-b39a-41e8aeffb328
📒 Files selected for processing (5)
apps/gateway/src/lib/timeout-config.tsapps/gateway/src/test-utils/mock-openai-server.tsapps/gateway/src/videos/videos.spec.tsapps/gateway/src/videos/videos.tsapps/playground/src/app/api/video/route.ts
|
Closing this duplicate in favor of #1882, which contains the same video timeout work rebased onto the latest main. |
Summary
Add explicit timeouts to video creation so stuck upstream provider requests fail with a clear 504 instead of hanging indefinitely.
Apply the same fail-fast behavior in the playground /api/video proxy so the UI gets JSON before the route times out, and preserve provider fallback when a create call times out.
Add mock-server coverage and gateway tests for timed-out video creation and timeout-driven fallback.
Verification
pnpm build
Summary by CodeRabbit
Improvements
Tests