fix: apply apiRequestTimeout consistently across providers#567
fix: apply apiRequestTimeout consistently across providers#567daewoongoh wants to merge 5 commits into
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
📝 WalkthroughWalkthroughThis PR imports and applies getApiRequestTimeout() across multiple provider client initializations, updates localization strings to list unsupported providers, and adjusts tests/mocks to expect the timeout option. ChangesProvider Timeout Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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❌ Patch coverage is
📢 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?
| 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?
| 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?
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. |
2 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
Bug Fixes
Documentation
Tests