Repository navigation
⏱️ fix: Bound Remaining Outbound Calls With Timeout Budgets - #17468
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb8ea603c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| stream: true, | ||
| }, | ||
| allowedAddresses, | ||
| requestOptions, |
There was a problem hiding this comment.
For input of at least 4096 characters, every chunk reuses this single requestOptions.signal, so speechMs limits the entire sequence rather than each provider request as documented. Later chunks are cancelled even when every individual request finishes within its budget. An isolated execution with a 60 ms budget and two 35 ms requests completed only the first chunk. Keep the disconnect signal shared, but create a new outbound budget for each ttsRequest, as streamAudio already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2d4aa6b. createSpeechBudget returns a factory, so each TTS chunk takes a fresh speechMs deadline while the disconnect signal stays shared. Unit spec in timeouts.spec.ts.
| accept: 'application/json', | ||
| }, | ||
| data: [file.file_id], | ||
| ...outboundRequestBudget(DEFAULT_OUTBOUND_TIMEOUTS_MS.ragMs, 'ragMs'), |
There was a problem hiding this comment.
When an operator changes timeouts.ragMs, deletions through the local, S3, Azure, and Firebase storage paths still use this fixed 30-second deadline; only createVectorFileDeleter honors the configuration. Consequently, raising the documented budget cannot prevent these embedding-cleanup requests from timing out, and lowering it does not bound them as requested. Pass the configured budget from the storage callers into deleteRagFile, using the constant only as a fallback. This also satisfies the repository requirement that new timeouts remain configurable.
AGENTS.md reference: AGENTS.md:L91-L93
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2d4aa6b. Local, S3 and owned-blob (Azure/Firebase) deleters pass req.config.timeouts into deleteRagFile; covered by rag.spec.ts and the stalled-rag-delete scenario.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: fb8ea603c8
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
fb8ea60 to
2d4aa6b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de15dc804b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| timeout: SERVICE_REQUEST_TIMEOUT_MS, | ||
| signal: AbortSignal.timeout(SERVICE_REQUEST_TIMEOUT_MS), |
There was a problem hiding this comment.
The Mailgun path still constructs its new timeout and abort lifecycle directly inside legacy CJS. This violates the explicit module-boundary rule in AGENTS.md, which applies to additions in existing /api files, not just new services. Move the deadline construction into packages/api and have this adapter consume the returned request options, as the speech and tool callers now do.
AGENTS.md reference: AGENTS.md:L78-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a461964. sendEmail.js calls serviceRequestBudget() from packages/api, which the remote config loader shares.
| stream: true, | ||
| }, | ||
| allowedAddresses, | ||
| nextBudget(), |
There was a problem hiding this comment.
For manual TTS input of at least 4096 characters, if one chunk has already streamed and a subsequent provider request exceeds speechMs before returning headers, the new deadline rejects ttsRequest before pipeSegment can attach its stream cleanup. The catch then returns without ending or destroying the response because res.headersSent is already true, and the /manual route has no final response cleanup. An isolated execution of this method with a successful first chunk and a timed-out second chunk confirmed that the handler returns with the response neither ended nor destroyed. Destroy the partially written response on this timeout path so the client does not keep waiting indefinitely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a461964. The manual TTS loop now destroys the response when a chunk fails after headers were sent; covered by a spec with a successful first chunk and a timed-out second.
9f14f9d to
c06e33f
Compare
Add timeouts.toolsMs, speechMs and imageGenerationMs to the timeouts block and apply the existing budgets to the RAG delete paths, built-in tool APIs, TTS/STT providers (TTS also aborts when the client closes), Stable Diffusion, Firebase reads and Mailgun. Calls made before app config exists (remote config, RAG health, service key URL, Google refresh) get a fixed 10 second budget.
…he service budget helper
c06e33f to
51e131f
Compare
Summary
Bounds the outbound HTTP calls from the 2026-10-19 Devin performance scan that #17263 did not cover. A stalled provider could hold a request, a tool call or startup indefinitely: the OpenWeather, Wolfram, Google Search, Traversaal, Tavily and Stable Diffusion tools, TTS and STT providers, RAG file deletes, Firebase download streams, the service key and remote config fetches, the RAG health check, Google admin refresh and Mailgun had no timeout.
They now reuse the #17263
timeoutsbudget helpers. Three keys join thetimeoutsblock inlibrechat.yaml:toolsMs(60 s),speechMs(300 s) andimageGenerationMs(300 s). TTS requests also abort when the client disconnects (abortOnClose). Startup and auth fetches that run before app config exists use a fixed 10 s budget.Type of change
Testing
Tested environments/configuration: Node 20 unit specs.
Automated tests:
abortOnCloseandtoolRequestBudget; updated mocks in the key, rag, STT, TTS, speech SSRF and Tavily specs.packages/apijest over utils, app, auth/googleRefresh, rag, streams, key and cloudfront: 73 suites, 1976 tests pass.apijest over Files services, utils, tools and middleware: pass.tsc --noEmitinpackages/data-providerandpackages/apiclean.Screenshots / recordings
No user-facing change.
Risk / compatibility
New finite defaults mean a tool, speech or image generation call that previously ran longer than its budget now fails with a timeout; the budgets are configurable. The Flux tool (tracked in berry-13#400), RAG
/embedand the CloudFront body stream are not changed here.Checklist