Repository navigation
refactor(gateway): separate streaming and plain request timeouts - #1590
Conversation
Add distinct timeout configurations for streaming and non-streaming requests with clearer naming. Non-streaming requests now default to 80 seconds (AI_TIMEOUT_MS) while streaming requests use 4 minutes (AI_STREAMING_TIMEOUT_MS). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
WalkthroughThe PR introduces separate timeout configurations for streaming and non-streaming AI requests. It replaces the unified Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Chat as Chat Handler
participant TimeoutConfig as Timeout Config
participant Upstream
rect rgba(100, 150, 200, 0.5)
Note over Chat,Upstream: Streaming Request Path
Client->>Chat: Stream request
Chat->>TimeoutConfig: createStreamingCombinedSignal()
TimeoutConfig->>TimeoutConfig: getStreamingTimeoutMs() [AI_STREAMING_TIMEOUT_MS]
TimeoutConfig->>Chat: AbortSignal (streaming timeout)
Chat->>Upstream: fetch with streaming timeout signal
Upstream-->>Chat: stream response
end
rect rgba(150, 100, 200, 0.5)
Note over Chat,Upstream: Non-Streaming Request Path
Client->>Chat: Regular request
Chat->>TimeoutConfig: createCombinedSignal()
TimeoutConfig->>TimeoutConfig: getTimeoutMs() [AI_TIMEOUT_MS, default 80s]
TimeoutConfig->>Chat: AbortSignal (non-streaming timeout)
Chat->>Upstream: fetch with non-streaming timeout signal
Upstream-->>Chat: response
end
Possibly Related PRs
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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
This pull request refactors the timeout configuration to differentiate between streaming and non-streaming AI requests with more explicit naming conventions. Streaming requests continue to use a 4-minute default timeout, while non-streaming requests now use a shorter 80-second default timeout for faster failure detection.
Changes:
- Renamed environment variables from
AI_REQUEST_TIMEOUT_MSto separateAI_STREAMING_TIMEOUT_MSandAI_TIMEOUT_MS - Added separate timeout functions and signal creation functions for streaming vs non-streaming requests
- Updated the gateway chat handler to use the appropriate timeout based on request type
- Updated tests to handle both timeout environment variables
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| apps/gateway/src/lib/timeout-config.ts | Added separate timeout functions for streaming (getStreamingTimeoutMs()) and non-streaming (getTimeoutMs()) requests, along with corresponding signal creation functions |
| apps/gateway/src/chat/chat.ts | Updated to import and use createStreamingCombinedSignal() for streaming requests and createCombinedSignal() for non-streaming requests |
| apps/gateway/src/api.spec.ts | Updated test setup to save and restore both AI_TIMEOUT_MS and AI_STREAMING_TIMEOUT_MS environment variables |
| .env.example | Updated documentation to describe both AI_STREAMING_TIMEOUT_MS and AI_TIMEOUT_MS with clear explanations of their purposes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Creates an AbortSignal that will abort after the plain (non-streaming) request timeout. | ||
| * Can be combined with other signals (e.g., client cancellation) using AbortSignal.any(). | ||
| */ | ||
| export function createTimeoutSignal(): AbortSignal { |
There was a problem hiding this comment.
The function name createTimeoutSignal() is too generic and doesn't clearly indicate it's for non-streaming requests. This creates ambiguity with createStreamingTimeoutSignal(). Consider renaming to createNonStreamingTimeoutSignal() or createPlainTimeoutSignal() to maintain naming consistency and clarity throughout the codebase.
| * Combines a plain (non-streaming) timeout signal with an optional cancellation signal. | ||
| * Uses the shorter timeout (default 80s) for non-streaming requests. | ||
| */ | ||
| export function createCombinedSignal( |
There was a problem hiding this comment.
The function name createCombinedSignal() is ambiguous since there's also createStreamingCombinedSignal(). The naming pattern is inconsistent - this function handles non-streaming requests but doesn't indicate that in its name. Consider renaming to createNonStreamingCombinedSignal() or createPlainCombinedSignal() to maintain clear distinction and naming consistency with the streaming variant.
| export const GATEWAY_TIMEOUT_MS = getGatewayTimeoutMs(); | ||
| export const AI_REQUEST_TIMEOUT_MS = getAIRequestTimeoutMs(); | ||
| export const AI_STREAMING_TIMEOUT_MS = getStreamingTimeoutMs(); | ||
| export const AI_TIMEOUT_MS = getTimeoutMs(); |
There was a problem hiding this comment.
The export name AI_TIMEOUT_MS is ambiguous compared to AI_STREAMING_TIMEOUT_MS. It's not immediately clear that this constant is for non-streaming requests. While marked as legacy, maintaining clear naming even in backwards compatibility exports helps prevent confusion. Consider renaming to AI_PLAIN_TIMEOUT_MS or AI_NONSTREAMING_TIMEOUT_MS for consistency.
| export const AI_TIMEOUT_MS = getTimeoutMs(); | |
| export const AI_PLAIN_TIMEOUT_MS = getTimeoutMs(); | |
| // Legacy alias for non-streaming timeout; prefer AI_PLAIN_TIMEOUT_MS in new code. | |
| export const AI_TIMEOUT_MS = AI_PLAIN_TIMEOUT_MS; |
| * Default: 80 seconds (80000ms) | ||
| */ | ||
| export function getTimeoutMs(): number { | ||
| const envValue = Number(process.env.AI_TIMEOUT_MS); | ||
| if (envValue > 0) { | ||
| return envValue; | ||
| } | ||
| // Default: 80 seconds for non-streaming requests | ||
| return 80000; |
There was a problem hiding this comment.
The non-streaming timeout validation is missing logic to ensure it stays shorter than the gateway timeout. Unlike getStreamingTimeoutMs() which uses Math.min(240000, getGatewayTimeoutMs() * 0.8) to cap the timeout relative to the gateway timeout, this function doesn't validate against GATEWAY_TIMEOUT_MS. If a user sets AI_TIMEOUT_MS to a value exceeding the gateway timeout, the request will always be terminated by the gateway timeout rather than this AI timeout, potentially leading to unexpected behavior. Consider adding validation similar to the streaming timeout to ensure this value is always less than the gateway timeout.
| * Default: 80 seconds (80000ms) | |
| */ | |
| export function getTimeoutMs(): number { | |
| const envValue = Number(process.env.AI_TIMEOUT_MS); | |
| if (envValue > 0) { | |
| return envValue; | |
| } | |
| // Default: 80 seconds for non-streaming requests | |
| return 80000; | |
| * Default: 80 seconds (80000ms) or 80% of gateway timeout, whichever is smaller | |
| */ | |
| export function getTimeoutMs(): number { | |
| const envValue = Number(process.env.AI_TIMEOUT_MS); | |
| if (envValue > 0) { | |
| // Ensure configured timeout does not exceed a safe fraction of the gateway timeout | |
| return Math.min(envValue, getGatewayTimeoutMs() * 0.8); | |
| } | |
| // Default: 80 seconds or 80% of gateway timeout, whichever is smaller | |
| return Math.min(80000, getGatewayTimeoutMs() * 0.8); |
| * from incremental responses and long waits are usually indicative of issues. | ||
| * Default: 80 seconds (80000ms) | ||
| */ | ||
| export function getTimeoutMs(): number { |
There was a problem hiding this comment.
The function name getTimeoutMs() is too generic and ambiguous. It doesn't clearly indicate that it's specifically for non-streaming/plain requests. Consider renaming to getNonStreamingTimeoutMs() or getPlainRequestTimeoutMs() to match the clarity of getStreamingTimeoutMs() and make the distinction between the two timeout types immediately obvious.
Summary
Add distinct timeout configurations for streaming and non-streaming requests with clearer naming. Non-streaming requests now default to 80 seconds (AI_TIMEOUT_MS) while streaming requests use 4 minutes (AI_STREAMING_TIMEOUT_MS).
Changes
🤖 Generated with Claude Code
Summary by CodeRabbit