Skip to content

refactor(gateway): separate streaming and plain request timeouts #1590

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
steebchen merged 1 commit into from
Feb 4, 2026
Merged

refactor(gateway): separate streaming and plain request timeouts #1590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 7 additions & 2 deletions .env.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,15 @@ TIMEOUT_MS=5000
# Defaults to 300000ms (5 minutes)
# GATEWAY_TIMEOUT_MS=300000

# AI API request timeout in milliseconds - maximum time for upstream provider calls
# AI API request timeout in milliseconds for streaming requests
# Should be shorter than GATEWAY_TIMEOUT_MS to allow for error handling
# Defaults to 240000ms (4 minutes) or 80% of GATEWAY_TIMEOUT_MS, whichever is smaller
# AI_REQUEST_TIMEOUT_MS=240000
# AI_STREAMING_TIMEOUT_MS=240000

# AI API request timeout in milliseconds for plain (non-streaming) requests
# Non-streaming requests use a shorter timeout since they don't benefit from incremental responses
# Defaults to 80000ms (80 seconds)
# AI_TIMEOUT_MS=80000

# =============================================================================
# WORKER CONFIGURATION
Expand Down
20 changes: 14 additions & 6 deletions apps/gateway/src/api.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,20 +793,28 @@ describe("test", () => {
// Timeout tests - use a short timeout via env var to test timeout handling
describe("Timeout handling", () => {
let originalTimeout: string | undefined;
let originalStreamingTimeout: string | undefined;

beforeAll(() => {
// Save original env value
originalTimeout = process.env.AI_REQUEST_TIMEOUT_MS;
// Save original env values
originalTimeout = process.env.AI_TIMEOUT_MS;
originalStreamingTimeout = process.env.AI_STREAMING_TIMEOUT_MS;
// Set a short timeout for testing (2 seconds)
process.env.AI_REQUEST_TIMEOUT_MS = "2000";
process.env.AI_TIMEOUT_MS = "2000";
process.env.AI_STREAMING_TIMEOUT_MS = "2000";
});

afterAll(() => {
// Restore original env value
// Restore original env values
if (originalTimeout !== undefined) {
process.env.AI_REQUEST_TIMEOUT_MS = originalTimeout;
process.env.AI_TIMEOUT_MS = originalTimeout;
} else {
delete process.env.AI_REQUEST_TIMEOUT_MS;
delete process.env.AI_TIMEOUT_MS;
}
if (originalStreamingTimeout !== undefined) {
process.env.AI_STREAMING_TIMEOUT_MS = originalStreamingTimeout;
} else {
delete process.env.AI_STREAMING_TIMEOUT_MS;
}
});

Expand Down
9 changes: 7 additions & 2 deletions apps/gateway/src/chat/chat.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ import { isCodingModel } from "@/lib/coding-models.js";
import { calculateCosts, shouldBillCancelledRequests } from "@/lib/costs.js";
import { throwIamException, validateModelAccess } from "@/lib/iam.js";
import { calculateDataStorageCost, insertLog } from "@/lib/logs.js";
import { createCombinedSignal, isTimeoutError } from "@/lib/timeout-config.js";
import {
createCombinedSignal,
createStreamingCombinedSignal,
isTimeoutError,
} from "@/lib/timeout-config.js";

import {
getCheapestFromAvailableProviders,
Expand Down Expand Up @@ -2141,7 +2145,7 @@ chat.openapi(completions, async (c) => {
}

// Create a combined signal for both timeout and cancellation
const fetchSignal = createCombinedSignal(
const fetchSignal = createStreamingCombinedSignal(
requestCanBeCanceled ? controller : undefined,
);

Expand Down Expand Up @@ -4113,6 +4117,7 @@ chat.openapi(completions, async (c) => {
}

// Create a combined signal for both timeout and cancellation
// Non-streaming requests use a shorter timeout (default 80s)
const fetchSignal = createCombinedSignal(
requestCanBeCanceled ? controller : undefined,
);
Expand Down
54 changes: 47 additions & 7 deletions apps/gateway/src/lib/timeout-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,37 +16,77 @@ export function getGatewayTimeoutMs(): number {
}

/**
* Gets the AI API request timeout - the maximum time for upstream provider calls.
* Gets the AI API request timeout for streaming requests - the maximum time for upstream provider calls.
* Should be shorter than gateway timeout to allow for error handling.
* Default: 4 minutes (240000ms) or 80% of gateway timeout, whichever is smaller
*/
export function getAIRequestTimeoutMs(): number {
const envValue = Number(process.env.AI_REQUEST_TIMEOUT_MS);
export function getStreamingTimeoutMs(): number {
const envValue = Number(process.env.AI_STREAMING_TIMEOUT_MS);
if (envValue > 0) {
return envValue;
}
// Default: 4 minutes or 80% of gateway timeout, whichever is smaller
return Math.min(240000, getGatewayTimeoutMs() * 0.8);
}

/**
* Gets the AI API request timeout for non-streaming (plain) requests.
* Non-streaming requests have a shorter default timeout since they don't benefit
* from incremental responses and long waits are usually indicative of issues.
* Default: 80 seconds (80000ms)
*/
export function getTimeoutMs(): number {

Copilot AI Feb 4, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
const envValue = Number(process.env.AI_TIMEOUT_MS);
if (envValue > 0) {
return envValue;
}
// Default: 80 seconds for non-streaming requests
return 80000;
Comment on lines +36 to +44

Copilot AI Feb 4, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Suggested change
* 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);

Copilot uses AI. Check for mistakes.
}

// Legacy exports for backwards compatibility (read at module load time)
// These should be avoided in new code - use the getter functions instead
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();

Copilot AI Feb 4, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Suggested change
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;

Copilot uses AI. Check for mistakes.

/**
* Creates an AbortSignal that will abort after the streaming request timeout.
* Can be combined with other signals (e.g., client cancellation) using AbortSignal.any().
*/
export function createStreamingTimeoutSignal(): AbortSignal {
return AbortSignal.timeout(getStreamingTimeoutMs());
}

/**
* Creates an AbortSignal that will abort after the AI request timeout.
* 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 {

Copilot AI Feb 4, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
return AbortSignal.timeout(getAIRequestTimeoutMs());
return AbortSignal.timeout(getTimeoutMs());
}

/**
* Combines a timeout signal with an optional cancellation signal.
* Combines a streaming timeout signal with an optional cancellation signal.
* If the cancellation signal is provided, the request will abort on either timeout or cancellation.
* If no cancellation signal is provided, only the timeout will cause an abort.
*/
export function createStreamingCombinedSignal(
cancellationController?: AbortController,
): AbortSignal {
const timeoutSignal = createStreamingTimeoutSignal();

if (cancellationController) {
return AbortSignal.any([timeoutSignal, cancellationController.signal]);
}

return timeoutSignal;
}

/**
* 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(

Copilot AI Feb 4, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
cancellationController?: AbortController,
): AbortSignal {
Expand Down
Loading