-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
fix(cc-compatible): restore upstream SSE and correct stream/combo timeout behavior #1257
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
Changes from all commits
0c8c770
9a282a4
be03076
fa55c77
05453b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -122,19 +122,23 @@ export function applyConfiguredUserAgent( | |||||||
| export function mergeAbortSignals(primary: AbortSignal, secondary: AbortSignal): AbortSignal { | ||||||||
| const controller = new AbortController(); | ||||||||
|
|
||||||||
| const abortBoth = () => { | ||||||||
| const abortFrom = (source: AbortSignal) => { | ||||||||
| if (!controller.signal.aborted) { | ||||||||
| controller.abort(); | ||||||||
| controller.abort(source.reason); | ||||||||
| } | ||||||||
| }; | ||||||||
|
|
||||||||
| if (primary.aborted || secondary.aborted) { | ||||||||
| abortBoth(); | ||||||||
| if (primary.aborted) { | ||||||||
| abortFrom(primary); | ||||||||
| return controller.signal; | ||||||||
| } | ||||||||
| if (secondary.aborted) { | ||||||||
| abortFrom(secondary); | ||||||||
| return controller.signal; | ||||||||
| } | ||||||||
|
|
||||||||
| primary.addEventListener("abort", abortBoth, { once: true }); | ||||||||
| secondary.addEventListener("abort", abortBoth, { once: true }); | ||||||||
| primary.addEventListener("abort", () => abortFrom(primary), { once: true }); | ||||||||
| secondary.addEventListener("abort", () => abortFrom(secondary), { once: true }); | ||||||||
| return controller.signal; | ||||||||
| } | ||||||||
|
|
||||||||
|
|
@@ -252,6 +256,9 @@ export class BaseExecutor { | |||||||
|
|
||||||||
| // Intra-URL retry config: retry same URL before falling back to next node | ||||||||
| static readonly RETRY_CONFIG = { maxAttempts: 2, delayMs: 2000 }; | ||||||||
| // Timeout for receiving the initial upstream response headers. Once the response | ||||||||
| // starts streaming, STREAM_IDLE_TIMEOUT_MS / Undici bodyTimeout handle stalls. | ||||||||
| static FETCH_START_TIMEOUT_MS = FETCH_TIMEOUT_MS; | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The use of a
To fix this, |
||||||||
|
|
||||||||
| // Override in subclass for provider-specific refresh | ||||||||
| async refreshCredentials(credentials: ProviderCredentials, log: ExecutorLog | null) { | ||||||||
|
|
@@ -404,13 +411,26 @@ export class BaseExecutor { | |||||||
| const transformedBody = await this.transformRequest(model, body, stream, activeCredentials); | ||||||||
|
|
||||||||
| try { | ||||||||
| // Apply timeout to all requests. Non-streaming requests need this to prevent | ||||||||
| // stalled connections. Streaming requests also need it for the initial fetch() call | ||||||||
| // to prevent hanging on unresponsive providers (e.g. 300s TCP default timeout — #769). | ||||||||
| // Stream idle detection (STREAM_IDLE_TIMEOUT_MS) handles stalls after data starts flowing. | ||||||||
| const timeoutMs = this.getTimeoutMs(); | ||||||||
| const timeoutSignal = AbortSignal.timeout(timeoutMs); | ||||||||
| const combinedSignal = signal ? mergeAbortSignals(signal, timeoutSignal) : timeoutSignal; | ||||||||
| // Only enforce the timeout while waiting for the initial fetch() response. | ||||||||
| // Once headers arrive, active streams must not be cut off by total elapsed time; | ||||||||
| // post-start stalls are handled separately by STREAM_IDLE_TIMEOUT_MS / bodyTimeout. | ||||||||
| const fetchStartTimeoutMs = this.getTimeoutMs(); | ||||||||
| const timeoutController = fetchStartTimeoutMs > 0 ? new AbortController() : null; | ||||||||
| let timeoutId: ReturnType<typeof setTimeout> | null = null; | ||||||||
| if (timeoutController) { | ||||||||
| timeoutId = setTimeout(() => { | ||||||||
| const timeoutError = new Error( | ||||||||
| `Fetch timeout after ${fetchStartTimeoutMs}ms on ${url}` | ||||||||
| ); | ||||||||
| timeoutError.name = "TimeoutError"; | ||||||||
| timeoutController.abort(timeoutError); | ||||||||
| }, fetchStartTimeoutMs); | ||||||||
|
||||||||
| }, fetchStartTimeoutMs); | |
| }, fetchStartTimeoutMs); | |
| timeoutId.unref?.(); |
Copilot
AI
Apr 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timeout logging uses BaseExecutor.FETCH_START_TIMEOUT_MS in the warning message, but the actual timeout duration used for this request is read into fetchStartTimeoutMs earlier. If FETCH_START_TIMEOUT_MS is changed at runtime (e.g., in tests or future dynamic config), the log can report the wrong value. Consider capturing the effective timeout outside the try/catch and using that captured value in the log message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation of
mergeAbortSignalsadds listeners to theprimarysignal (usually the client request signal) but never removes them if the operation completes without an abort. In scenarios with multiple fallbacks or retries, each attempt will add a new listener to the sameprimarysignal. This can lead to aMaxListenersExceededWarningin Node.js (default limit is 10) and a small memory leak for the duration of the request. Since Node 18 is supported,AbortSignal.anyis not available, but you should consider a mechanism to remove these listeners once thefetchcall or the resulting stream is finished.