feat(streaming): replace fixed timeout with resettable idle timeout#13497
feat(streaming): replace fixed timeout with resettable idle timeout#13497
Conversation
The previous AbortSignal.timeout() created a fixed total timeout from the start of the request. For SSE streaming, this is not ideal because long-running streams could be terminated even when data is actively flowing. Replace it with an IdleTimeoutController that resets the countdown every time a stream chunk is received, only aborting when the stream is truly idle. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: icarus <[email protected]>
EurFelux
left a comment
EurFelux
left a comment
There was a problem hiding this comment.
Great PR! The IdleTimeoutController is clean, well-tested, and solves a real problem — long-running streams being killed even while actively receiving data.
What I liked:
- Arrow function methods (
reset =,cleanup =) ensure correctthisbinding when passed as callbacks — nice attention to detail - Test coverage is thorough: normal expiry, reset, multiple resets, post-abort no-op, and cleanup
- Backward-compatible — existing callers (e.g.,
messageThunk.ts) are unaffected
Minor suggestions (non-blocking):
- The
AiSdkToChunkAdapterconstructor now has 9 positional params — consider an options object pattern as a follow-up resetIdleTimeout/cleanupIdleTimeoutinAiSdkMiddlewareConfigis a layering concern — they're stream lifecycle, not middleware config
LGTM 🎉
| @@ -41,14 +43,18 @@ export class AiSdkToChunkAdapter { | |||
| enableWebSearch?: boolean, | |||
| onSessionUpdate?: (sessionId: string) => void, | |||
| getSessionWasCleared?: () => boolean, | |||
| providerId?: string | |||
| providerId?: string, | |||
| resetIdleTimeout?: () => void, | |||
| cleanupIdleTimeout?: () => void | |||
There was a problem hiding this comment.
Suggestion: Consider using an options object to reduce positional parameter sprawl
The constructor now has 9 positional parameters, many of which are optional. This is becoming hard to read and maintain — callers need to pass undefined placeholders to reach later parameters (as seen in index_new.ts lines 336-337).
Consider refactoring to an options object pattern:
interface AiSdkToChunkAdapterOptions {
mcpTools?: MCPTool[]
accumulate?: boolean
enableWebSearch?: boolean
onSessionUpdate?: (sessionId: string) => void
getSessionWasCleared?: () => boolean
providerId?: string
resetIdleTimeout?: () => void
cleanupIdleTimeout?: () => void
}
constructor(
onChunk: (chunk: Chunk) => void,
options?: AiSdkToChunkAdapterOptions
)This is not a blocker for this PR since it's a pre-existing issue you're extending, but worth considering as a follow-up to improve the DX.
| resetIdleTimeout?: () => void | ||
| cleanupIdleTimeout?: () => void |
There was a problem hiding this comment.
Nit: These callbacks feel out of place in AiSdkMiddlewareConfig
resetIdleTimeout and cleanupIdleTimeout are not middleware concerns — they're stream lifecycle callbacks. Threading them through the middleware config just to pass them to AiSdkToChunkAdapter creates coupling between unrelated layers.
An alternative would be to pass them directly when constructing the adapter (e.g., via a separate streamLifecycle option), keeping the middleware config focused on actual middleware behavior.
Low priority — the current approach works, just noting the layering concern.
…tHandle Move resetIdleTimeout/cleanupIdleTimeout from separate parameters into a single IdleTimeoutHandle interface. Place the handle on ModernAiProviderConfig instead of AiSdkMiddlewareConfig to keep middleware config focused on middleware concerns. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: icarus <[email protected]>
EurFelux
left a comment
EurFelux
left a comment
There was a problem hiding this comment.
Re-reviewed after the refactor commit (e8642d8b). Both suggestions from the previous round have been addressed nicely:
IdleTimeoutHandleinterface — cleanly aggregatesreset/cleanupinto a single handle object, reducing the constructor param count and improving readability- Layering fix —
AiSdkMiddlewareConfigis now free of idle timeout concerns;idleTimeoutlives only inModernAiProviderConfigwhere it belongs
The code is clean and well-structured. No further comments — LGTM 🎉
The IdleTimeoutController already implements IdleTimeoutHandle, so pass it directly instead of destructuring into a new object. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: icarus <[email protected]>
![cherry-ai-bot[bot]](https://avatars.githubusercontent.com/in/3013849?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the latest revision. The idle-timeout design is clean and the wiring is consistent end-to-end: the controller is created at request-build time, its signal is used for the request, the stream adapter resets it on chunk arrival, and cleanup happens in the stream finally path. This fixes the original failure mode where long-lived but still-active streams could be aborted just because total elapsed time exceeded the fixed timeout. The added tests also give good confidence for the reset / expiry / cleanup behavior. I don't see any remaining blocking issues in the current version.
2 participants
What this PR does
Before this PR:
The streaming timeout used a fixed
AbortSignal.timeout()that starts counting from the initial request. For long-running SSE streams (e.g., complex reasoning or tool-calling chains), the request could be aborted even while data was actively flowing.After this PR:
Introduces an
IdleTimeoutControllerthat resets its countdown every time a stream chunk is received. The timeout now only fires when the stream is truly idle (no data received for the configured duration), not based on total elapsed time.Fixes #13489
Why we need it and why it was done in this way
The following tradeoffs were made:
reset()andcleanup()methods, exposed via anIdleTimeoutHandleinterface and threaded throughModernAiProviderConfigtoAiSdkToChunkAdapter.reset()callback is invoked on everyreader.read()return inreadFullStream, which is the natural place where SSE chunks are consumed.cleanup()is called in thefinallyblock to prevent timer leaks when the stream ends normally or errors out.The following alternatives were considered:
reader.read()withPromise.race— more invasive and less clean than resetting a timer on each chunk.Breaking changes
None. The default timeout duration remains unchanged (30 minutes). The only behavioral change is that the timeout now resets on each chunk instead of being fixed from request start.
Special notes for your reviewer
IdleTimeoutControlleris covered by 6 unit tests (fake timers).idleTimeoutconstructor parameter onAiSdkToChunkAdapteris backward-compatible — existing callers (e.g.,messageThunk.ts) are unaffected.Checklist
/gh-pr-review,gh pr diff, or GitHub UI) before requesting review from othersRelease note