fix(core): set 1.201s default HTTP agent timeout, disable keep-alive for AI Core requests - #2266
fix(core): set 1.201s default HTTP agent timeout, disable keep-alive for AI Core requests#2266InjunPark-sap wants to merge 5 commits into
Conversation
…or AI Core requests (#623)
12c0cbe to
438b5fc
Compare
SummaryThe following content is AI-generated and provides a summary of the pull request: ContextCloses SAP/ai-sdk-js-backlog#623. What this PR does and why it is neededSets a 12-minute (720,000 ms) default HTTP socket timeout for AI Core requests, overriding the inherited SAP Cloud SDK default of 5 seconds, which is too short for long-running chat and streaming completions. Keep-alive is also disabled on SDK-managed destinations. With the previous 5s timeout, stale pooled sockets were evicted quickly; the longer timeout would otherwise keep those sockets alive past the load balancer's idle timeout, causing Changes
Category: 🐛 Bug Fix
PR Bot InformationVersion:
|
| // Disabled so the long timeout above does not keep stale pooled sockets alive past | ||
| // a load balancer's idle timeout, which would cause ECONNRESET on reuse. | ||
| const DEFAULT_AGENT_KEEP_ALIVE = false; |
There was a problem hiding this comment.
I would prefer enabling this by default. Please investigate if increasing the timeout actually leads to issues and if the keep alive timeout can be configured in isolation.
As far as I know, this is also the default.
There was a problem hiding this comment.
-
Node core can't isolate idle vs active timeout — one
timeoutgoverns both (keepAliveMsecsis only the TCP probe delay). Isolation needsagentkeepalive(new dep). -
12min + keepAlive:true has a documented risk (ECONNRESET while doing http 1.1 keep alive requests and server closes the connections nodejs/node#47130): if the LB drops idle conns first, the reused half-open socket → ECONNRESET, no core fix. The old 5s timeout avoided this implicitly.
So it hinges on the AI Core LB idle timeout. If it is more than 12min, keepAlive:true + 1200s is safe and I'll switch; if shorter, we add agentkeepalive or keep keepAlive:false. WDYT?
There was a problem hiding this comment.
Sounds good, sadly axios does not appear to handle this, please add a comment here to re-visit this if we do switch to a different HTTP library like unidici.
The fetch adapter would be an option for non-mTLS connections, but that could also lead to issues with any custom HTTP middleware users might have.
There was a problem hiding this comment.
Done. Added a comment to revisit keepAlive if we switch HTTP library (undici / fetch adapter).
There was a problem hiding this comment.
Please add the TODO: string to make it easier to find.
There was a problem hiding this comment.
Done — prefixed the revisit comment with TODO:.
| } | ||
| )) as HttpDestination; | ||
| return aiCoreDestination; | ||
| return { |
There was a problem hiding this comment.
[req] Ensure the other axios options mentioned in the documentation are added; I think we might already set timeout: 0 somewhere.
There was a problem hiding this comment.
timeout: 0 is the axios request-level timeout in Cloud SDK default — no request deadline, deliberate for streaming.
Separate layer from the agent socket timeouthere, no conflict. CustomRequestConfig already exposes httpsAgent/httpAgent/proxy/signal/middleware; axios timeout isn't picked.
Do you want it to be added, or other options?
There was a problem hiding this comment.
There was a problem hiding this comment.
Added maxContentLength/maxBodyLength: Infinity to the default request config.
Note it's a no-op on the current http adapter (axios defaults both to -1 = unlimited), but makes the intent explicit and future-proofs a switch to undici/fetch.
timeout: 0 is already set by the Cloud SDK (https://github.com/SAP/cloud-sdk-js/blob/052efd32b668687051e9f0f91883a241ecd6a5f4/packages/http-client/src/http-client.ts#L494).
Co-authored-by: David Knaack <david.knaack@sap.com>
Context
Closes SAP/ai-sdk-js-backlog#623.
What this PR does and why it is needed
The SDK inherited the SAP Cloud SDK's 5s HTTP socket timeout, which is too short for long-running chat and streaming completions. This PR sets a 12-minute (720000 ms) default socket timeout on the service-binding-derived AI Core destination (covering both client-secret and mTLS auth).
Keep-alive is disabled on these requests: with the old 5s timeout, idle pooled sockets were evicted before any load balancer closed them; the longer timeout would otherwise keep stale sockets around long enough to be reused and trigger ECONNRESET. Disabling pooling avoids stale-socket reuse while the long timeout still covers slow responses. Rationale and the researched load-balancer idle timeouts are in the decision record.
Both defaults apply only to the SDK-managed destination and can be overridden via a custom destination's
agentOptionsor per-requestCustomRequestConfig.httpsAgent.Open for reviewers: confirm disabling keep-alive is acceptable vs. adding
agentkeepaliveto preserve pooling (would need SAP BTP's actual LB idle timeout to tune). Docs handled in a separate SAP/ai-sdk PR — link to follow.Definition of Done