fix: apply apiRequestTimeout consistently across providers#567
Conversation
…c SDK providers Wire getApiRequestTimeout() into every provider that instantiates an OpenAI or Anthropic SDK client so the user-configured apiRequestTimeout setting applies uniformly. Previously only openai/lm-studio and providers extending BaseOpenAiCompatibleProvider honored it. OpenAI SDK: openai-native, openai-codex, openrouter, router-provider (lite-llm, zoo-gateway, opencode-go, vercel-ai-gateway), requesty, unbound, xai, qwen-code. Anthropic SDK: anthropic, minimax, anthropic-vertex.
Append the list of unsupported providers (Amazon Bedrock, Google Gemini, GCP Vertex AI, Mistral, Ollama, VS Code LM API, Poe) to the apiRequestTimeout setting description across all package.nls locales so users can see at a glance which providers ignore the value.
daewoongoh
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors the timeout utility to enforce 1–3600 second bounds with a 600-second default and always return milliseconds, wires ChangesAPI Provider Timeout Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/package.nls.ca.json`:
- Line 41: The description for "settings.apiRequestTimeout.description" contains
a contradiction: it recommends higher timeouts for "Ollama" but also lists
"Ollama" among providers where the setting has no effect; edit that JSON string
to remove the contradiction by deleting "Ollama" from the unsupported providers
clause so it remains recommended as a local provider (ensure punctuation and
spacing remain valid in the JSON string).
In `@src/package.nls.id.json`:
- Line 41: The description for settings.apiRequestTimeout.description contains a
contradiction by naming "Ollama" both as a recommended local provider and as an
unsupported provider; choose one resolution and make it consistent across all
localization files by either removing "Ollama" from the recommendation clause
(e.g., "like LM Studio and Ollama") or removing it from the unsupported
providers list, then update the string for
settings.apiRequestTimeout.description in every localization file to reflect
that choice so the wording is no longer contradictory.
In `@src/package.nls.nl.json`:
- Line 41: The localization string for "settings.apiRequestTimeout.description"
contains contradictory guidance about Ollama; update the string so it's
consistent by removing "Ollama" from the recommendation clause (leave it in the
unsupported list), i.e., change the phrase "zoals LM Studio en Ollama" to
something like "zoals LM Studio" and ensure punctuation and spacing remain
correct around the comma so the key "settings.apiRequestTimeout.description"
reflects the resolved text.
In `@src/package.nls.tr.json`:
- Line 41: The description for "settings.apiRequestTimeout.description" wrongly
mentions Ollama as a local provider that may need higher timeout while also
listing it in the unsupported providers list; update the English source
(src/package.nls.json key settings.apiRequestTimeout.description) to remove
"Ollama" from the recommendation sentence (leave LM Studio and other genuine
local providers only) but keep "Ollama" in the unsupported-providers list, then
apply the same corrected text to all translated files including this file
(src/package.nls.tr.json) so the recommendation and unsupported lists are
consistent across locales.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 287b4f98-b9cb-46ce-aa25-f763d4bc6491
📒 Files selected for processing (29)
src/api/providers/anthropic-vertex.tssrc/api/providers/anthropic.tssrc/api/providers/minimax.tssrc/api/providers/openai-codex.tssrc/api/providers/openai-native.tssrc/api/providers/openrouter.tssrc/api/providers/qwen-code.tssrc/api/providers/requesty.tssrc/api/providers/router-provider.tssrc/api/providers/unbound.tssrc/api/providers/xai.tssrc/package.nls.ca.jsonsrc/package.nls.de.jsonsrc/package.nls.es.jsonsrc/package.nls.fr.jsonsrc/package.nls.hi.jsonsrc/package.nls.id.jsonsrc/package.nls.it.jsonsrc/package.nls.ja.jsonsrc/package.nls.jsonsrc/package.nls.ko.jsonsrc/package.nls.nl.jsonsrc/package.nls.pl.jsonsrc/package.nls.pt-BR.jsonsrc/package.nls.ru.jsonsrc/package.nls.tr.jsonsrc/package.nls.vi.jsonsrc/package.nls.zh-CN.jsonsrc/package.nls.zh-TW.json
Assert SDK clients receive a timeout option in providers updated by 4c82de4 (anthropic-vertex, openrouter, requesty, vercel-ai-gateway, zoo-gateway), and stub vscode.workspace.getConfiguration in tests that mock vscode as an empty object so getApiRequestTimeout() can run during provider construction.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The previous description recommended raising the timeout for "local providers like LM Studio and Ollama" while also listing Ollama among the providers where the setting has no effect. Drop the provider examples from the recommendation so the two parts no longer contradict; the unsupported list still spells out where the setting has no effect.
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Nice! Thanks for this contribution.
Do you think it would be worth filing an issue to ultimately update the base class to enforce timeout going forward?
| "settings.maximumIndexedFilesForFileSearch.description": "Maximum number of files to index for the @ file search feature. Higher values provide better search results in large projects but may use more memory. Default: 10,000.", | ||
| "settings.useAgentRules.description": "Enable loading of AGENTS.md files for agent-specific rules (see https://agent-rules.org/)", | ||
| "settings.apiRequestTimeout.description": "Maximum time in seconds to wait for API responses (0 = no timeout, 1-3600s, default: 600s). Higher values are recommended for local providers like LM Studio and Ollama that may need more processing time.", | ||
| "settings.apiRequestTimeout.description": "Maximum time in seconds to wait for API responses (0 = no timeout, 1-3600s, default: 600s). Higher values are recommended for local providers that may need more processing time. Unsupported providers (setting has no effect): Amazon Bedrock, Google Gemini, GCP Vertex AI, Mistral, Ollama, VS Code LM API, Poe.", |
There was a problem hiding this comment.
Two issues with this unsupported-providers list:
-
"GCP Vertex AI" is now partially incorrect —
apiRequestTimeoutworks for Claude models on Vertex (routed toAnthropicVertexHandler, which this PR wires). It only has no effect for Gemini models on Vertex (routed toVertexHandler). Could this be scoped to"GCP Vertex AI (Gemini models)"to avoid confusing users on the Anthropic/Claude Vertex path? -
Moonshot is missing — the PR description notes that
MoonshotHandlerextendsOpenAICompatibleHandlerwhich uses the Vercel AI SDK and has no client-level timeout support. Should Moonshot appear in this list too?
There was a problem hiding this comment.
I updated the unsupported-provider wording across the NLS files so Vertex is scoped to Gemini models only, and added Moonshot to the unsupported list. The text now uses GCP Vertex AI (Gemini) rather than broadly listing all of GCP Vertex AI, so the Claude-on-Vertex path remains correctly represented as supported via AnthropicVertexHandler.
| expect(AnthropicVertex).toHaveBeenCalledWith({ | ||
| projectId: "test-project", | ||
| region: "us-central1", | ||
| timeout: expect.any(Number), |
There was a problem hiding this comment.
This only tests the bare construction path ({ projectId, region, timeout }). anthropic-vertex.ts has two other branches — parsedVertexCredentials and vertexKeyFile — that also pass timeout. Are those covered somewhere, or worth adding here?
There was a problem hiding this comment.
I added explicit coverage for both additional Anthropic Vertex constructor branches: vertexJsonCredentials and vertexKeyFile.
The tests now assert that GoogleAuth receives the expected credentials/keyFile options and that the resulting AnthropicVertex constructor config includes timeout. I also adjusted the GoogleAuth mock to use a function expression so it can be invoked with new, matching the handler path.
| this.client = new Anthropic({ | ||
| baseURL: this.options.anthropicBaseUrl || undefined, | ||
| [apiKeyFieldName]: this.options.apiKey, | ||
| timeout: getApiRequestTimeout(), |
There was a problem hiding this comment.
The apiRequestTimeout setting has "type": "number" in package.json, so users can enter decimal values like 16.1. 16.1 * 1000 produces a non-integer float, and the Anthropic SDK calls validatePositiveInteger("timeout", ...) in its constructor — which would throw for any decimal input. Would Math.round in getApiRequestTimeout() (or tightening the schema to "type": "integer") be the right fix?
There was a problem hiding this comment.
I fixed this in two places: the configuration schema now uses type: "integer", and getApiRequestTimeout() rounds the computed milliseconds before passing the value to SDK clients. I also centralized the timeout bounds/default handling around the documented 1–3600s range, so invalid or out-of-range values fall back to the default instead of propagating an SDK-invalid timeout value.
Good point. I agree that enforcing timeouts in a common base class would be preferable in the long run. I looked into that direction, but the current implementations don't all share the same base class and some rely on different SDKs/providers with provider-specific timeout handling. Because of that, I scoped this PR to the immediate fix rather than a broader architectural refactor. I'd be supportive of opening a follow-up issue to explore a more unified approach. |
- Math.round() prevents float ms from Anthropic SDK validatePositiveInteger throw - minimum changed from 0 to 1; removed unreachable <= 0 branch in getApiRequestTimeout - return type narrowed from number | undefined to number
…t to unsupported list - removed '0 = no timeout' from all locales; minimum is now 1 - scoped GCP Vertex AI to '(Gemini models)' since Claude models on Vertex are supported - added Moonshot to unsupported providers (uses Vercel AI SDK, no client-level timeout)
Restructured into 3 concise sentences: timeout info, local provider recommendation, and alphabetized unsupported providers list. Fixed Korean phrasing and unified Japanese fullwidth punctuation.
Pull the timeout bounds (1-3600s) and default (600s) into named constants and a small isValidTimeout type-guard, so out-of-range values fall back to the default just like NaN/non-number inputs. Tests updated to cover the new boundary and out-of-range fallback cases.
Add cases verifying timeout is forwarded to AnthropicVertex when constructing with vertexJsonCredentials or vertexKeyFile, and mock GoogleAuth so the auth-bound paths are exercised.
Arrow functions cannot be invoked with `new`, so the mock threw TypeError when the handler did `new GoogleAuth(...)`. Switch to a regular function expression to match the AnthropicVertex mock.
|
Thanks for the review. I pushed follow-up commits addressing the timeout-related comments:
I also removed the previous |
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Hey @daewoongoh thanks again for addressing the feedback, after some testing I have a few more comments.
| .get<number>("apiRequestTimeout", DEFAULT_TIMEOUT_SECONDS) | ||
|
|
||
| return configTimeout * 1000 // Convert to milliseconds | ||
| const seconds = isValidTimeout(configTimeout) ? configTimeout : DEFAULT_TIMEOUT_SECONDS |
There was a problem hiding this comment.
Previously, setting apiRequestTimeout: 0 returned undefined and the SDK used its own default (no explicit cap) — this was explicitly documented as "0 = no timeout" in the old i18n string. Now it silently falls back to 600s. Worth a CHANGELOG note so users who relied on that escape hatch know what changed.
There was a problem hiding this comment.
I added a changeset note documenting the behavior change: Updated apiRequestTimeout validation. Values must be integers between 1 and 3600 seconds; invalid or out-of-range values, including 0, now fall back to 600 seconds. This aligns with the SDK's default timeout value.
| apiKey: accessToken, | ||
| baseURL: CODEX_API_BASE_URL, | ||
| defaultHeaders: codexHeaders, | ||
| timeout: getApiRequestTimeout(), |
There was a problem hiding this comment.
Every other provider calls getApiRequestTimeout() once in the constructor and stores it in the client. Here it's re-read from VS Code config on every request (because the client is created lazily). Is the per-request re-evaluation intentional? If not, a private readonly timeoutMs = getApiRequestTimeout() field set at construction would align this with the other providers.
There was a problem hiding this comment.
Not intentional. I updated OpenAI Codex to cache the timeout at handler construction time and pass that cached value when the lazy client is created, so it now matches the behavior of the other providers.
| "X-Title": "Zoo Code", | ||
| "User-Agent": `ZooCode/${Package.version}`, | ||
| }, | ||
| timeout: expect.any(Number), |
There was a problem hiding this comment.
Could this use a concrete value instead of expect.any(Number)? As written, a mutation replacing getApiRequestTimeout() with 0 (which the OpenAI SDK treats as "abort immediately") would still pass. The pattern in openai-timeout.spec.ts — mocking getApiRequestTimeout to return a sentinel and asserting the exact value — catches that class of bug.
There was a problem hiding this comment.
Updated. I mocked getApiRequestTimeout() with a concrete sentinel value and now assert that OpenRouter passes that exact value to the OpenAI client instead of only checking expect.any(Number).
| "X-Title": "Zoo Code", | ||
| "User-Agent": `ZooCode/${Package.version}`, | ||
| }, | ||
| timeout: expect.any(Number), |
There was a problem hiding this comment.
Same question as the openrouter test — expect.any(Number) accepts 0 or any hardcoded literal. Worth pinning to a specific sentinel so mutations are detectable.
There was a problem hiding this comment.
Updated Requesty as well. Both client-construction assertions now verify the exact mocked timeout value rather than accepting any number.
| expect(AnthropicVertex).toHaveBeenCalledWith({ | ||
| projectId: "test-project", | ||
| region: "us-central1", | ||
| timeout: expect.any(Number), |
There was a problem hiding this comment.
Same expect.any(Number) concern here (and lines 108 and 130). If timeout: getApiRequestTimeout() were replaced with timeout: 0, all three assertions would still pass.
There was a problem hiding this comment.
Updated the Anthropic Vertex tests to use the same concrete timeout sentinel across the bare, JSON credentials, and key file credentials paths.
| "settings.maximumIndexedFilesForFileSearch.description": "Maximum number of files to index for the @ file search feature. Higher values provide better search results in large projects but may use more memory. Default: 10,000.", | ||
| "settings.useAgentRules.description": "Enable loading of AGENTS.md files for agent-specific rules (see https://agent-rules.org/)", | ||
| "settings.apiRequestTimeout.description": "Maximum time in seconds to wait for API responses (0 = no timeout, 1-3600s, default: 600s). Higher values are recommended for local providers like LM Studio and Ollama that may need more processing time.", | ||
| "settings.apiRequestTimeout.description": "API response timeout (seconds, default: 600, range: 1–3600). Higher values are recommended for local providers. Unsupported providers: Amazon Bedrock, GCP Vertex AI (Gemini), Google Gemini, Mistral, Moonshot, Ollama, Poe, VS Code LM API.", |
There was a problem hiding this comment.
"GCP Vertex AI (Gemini)" in the unsupported list might confuse users running Anthropic Claude on Vertex — AnthropicVertexHandler does wire getApiRequestTimeout() now (see anthropic-vertex.ts:47), so the timeout is effective for that path. Something like "Google Gemini (including Vertex AI)" might make the distinction clearer.
There was a problem hiding this comment.
Agreed. I updated the unsupported-provider wording to Google Gemini(directly or through the Vertex AI platform) across the NLS files so Claude-on-Vertex users do not read this as applying to Anthropic Vertex.
github-actions
Bot
added
the
awaiting-author
- Refactor timeout handling logic in multiple API providers - Improve consistency in timeout configuration and error handling - Reduce code duplication across provider implementations
- Replace constructor-based initialization with class field initialization - Removes unnecessary constructor when only initializing properties - Follows modern TypeScript/JavaScript conventions for cleaner code
Add workspace.getConfiguration mock to vertex.spec.ts and vscode-lm.spec.ts to fix test failures
…larification - Update translation files to clarify that Google Gemini is not supported either directly or through the Vertex AI platform
- Mock timeout-config module in anthropic-vertex, openai, openrouter, requesty, and zoo-gateway tests - Add MOCK_TIMEOUT_MS constant for consistent timeout assertions - Replace expect.any(Number) with explicit MOCK_TIMEOUT_MS value in timeout assertions
github-actions
Bot
added
awaiting-review
3 participants
Related GitHub Issue
Closes: #565
Description
This PR makes the
zoo-code.apiRequestTimeoutsetting behave consistently across API providers.Previously, only a subset of providers explicitly propagated
getApiRequestTimeout()into their underlying SDK/client configuration, while others relied on default client behavior. As a result, changing the global timeout setting could produce different runtime behavior depending on the selected provider.Test Procedure
Pre-Submission Checklist
Screenshots / Videos
N/A
Documentation Updates
Additional Notes
This change is focused on consistency rather than introducing new timeout behavior. The goal is to ensure that the global
apiRequestTimeoutsetting is applied uniformly across provider implementations so users receive predictable behavior regardless of the selected provider.Providers without client-level timeout support
src/api/providers/bedrock.ts@aws-sdk/client-bedrock-runtimerequestTimeout: 0. Streaming semantics differ from OpenAI/Anthropic and are handled separately.src/api/providers/gemini.ts@google/genaitimeout; only per-requesthttpOptions.timeoutis supported. Would require threading the value into every call site.src/api/providers/vertex.ts@google/genai(extendsGeminiHandler)src/api/providers/mistral.ts@mistralai/mistralaitimeoutoption on the client.src/api/providers/native-ollama.tsollamanpm packagesrc/api/providers/vscode-lm.tssrc/api/providers/poe.tsai-sdk-provider-poe(Vercel AI SDK)createPoehas no client-leveltimeout; only per-callabortSignalis available.Get in Touch
hehegwk_23849
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Documentation
apiRequestTimeoutdescriptions (including translations) to show default/range (600, 1–3600), remove “0 disables timeout” wording, and explicitly list providers where the setting has no effect.Tests
Chores
typeto integer, minimum to 1).