-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: use axios adapter for OpenAI SDK to support SOCKS5 proxies #7006
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Created axios-fetch-adapter utility that wraps axios to provide fetch-compatible interface - Added openAiUseAxiosForProxy option to ApiHandlerOptions - Modified OpenAI provider to use axios adapter when proxy is detected or configured - Updated tests to expect the fetch property in OpenAI constructor This fixes issues where VSCode patched fetch does not work correctly with SOCKS5 proxies, while axios handles them properly. The adapter automatically detects proxy environment variables and switches to axios when needed. Fixes #6991
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code because apparently I trust no one, not even myself.
*/ | ||
function nodeStreamToWebStream(nodeStream: Readable): ReadableStream<Uint8Array> { | ||
return new ReadableStream({ | ||
start(controller) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional? The Node.js stream event listeners are never removed if the ReadableStream is abandoned before completion. This could cause memory leaks. Consider adding cleanup logic:
start(controller) { | |
start(controller) { | |
const cleanup = () => { | |
nodeStream.removeAllListeners('data'); | |
nodeStream.removeAllListeners('end'); | |
nodeStream.removeAllListeners('error'); | |
}; | |
nodeStream.on('data', (chunk) => { | |
// ... existing code ... | |
}); | |
nodeStream.on('end', () => { | |
controller.close(); | |
cleanup(); | |
}); | |
nodeStream.on('error', (err) => { | |
controller.error(err); | |
cleanup(); | |
}); | |
}, |
|
||
// Return an axios-based fetch implementation | ||
return async (input: RequestInfo | URL, init?: RequestInit): Promise<Response> => { | ||
const url = typeof input === "string" ? input : input instanceof URL ? input.toString() : (input as Request).url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When input is a Request object, we're not handling the case where the Request might have a body that needs to be properly extracted. Could we add handling for this case?
const url = typeof input === "string" ? input : input instanceof URL ? input.toString() : (input as Request).url | |
const url = typeof input === 'string' ? input : input instanceof URL ? input.toString() : (input as Request).url | |
const requestInit = input instanceof Request ? { | |
method: input.method, | |
headers: Object.fromEntries(input.headers.entries()), | |
body: await input.text() // or input.blob() depending on content type | |
} : init |
* - VSCode proxy settings | ||
* - User configuration | ||
*/ | ||
export function shouldUseAxiosForProxy(): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proxy detection only checks environment variables but doesn't check VSCode's actual proxy settings. Could we enhance this to also check vscode.workspace.getConfiguration('http').get('proxy')? This would make the detection more comprehensive.
const url = typeof input === "string" ? input : input instanceof URL ? input.toString() : (input as Request).url | ||
|
||
// Convert RequestInit to AxiosRequestConfig | ||
const config: AxiosRequestConfig = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The axios adapter doesn't respect the timeout setting that's passed to the OpenAI client constructor. Consider adding timeout configuration to the AxiosRequestConfig:
const config: AxiosRequestConfig = { | |
const config: AxiosRequestConfig = { | |
url, | |
method: (init?.method || 'GET') as any, | |
headers: init?.headers as any, | |
data: init?.body, | |
timeout: /* get timeout from somewhere */, | |
responseType: 'stream', | |
decompress: false, | |
validateStatus: () => true, | |
} |
@@ -0,0 +1,130 @@ | |||
import axios, { AxiosRequestConfig, AxiosResponse } from "axios" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new file lacks dedicated unit tests. Would it be worth adding tests for stream conversion, error handling, and proxy detection to ensure the adapter works correctly in all scenarios?
"no_proxy", | ||
] | ||
|
||
const hasProxyEnvVars = proxyVars.some((varName) => process.env[varName]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shouldUseAxiosForProxy() function is called on every OpenAI handler instantiation. Could we consider caching this check for performance, especially since environment variables don't change during runtime?
Closing as this proxy configuration issue is outside of our control. See #6991 (comment) for details. |
Description
This PR fixes the proxy issue where OpenAI Compatible provider's chat completions fail with SOCKS5 proxies while model fetching works.
Problem
As correctly identified by @bstrdsmkr in #6991:
axios.get
→ WORKS ✅fetch
internally) → FAILS ❌fetch
to respect proxy settings, but the patched fetch doesn't work correctly with SOCKS5 proxiesaxios
works because it handles proxies differentlySolution
fetch
option, so we provide our axios adapter when proxies are detectedopenAiUseAxiosForProxy
option to allow manual overrideChanges
New utility:
src/api/providers/utils/axios-fetch-adapter.ts
Updated OpenAI provider:
src/api/providers/openai.ts
Updated types: Added
openAiUseAxiosForProxy
option toApiHandlerOptions
Updated tests: Fixed test expectations to include the
fetch
propertyTesting
Impact
Fixes #6991
Important
Adds axios-based fetch adapter to support SOCKS5 proxies in OpenAI SDK, updating
OpenAiHandler
to use this adapter when proxies are detected.createAxiosFetchAdapter
inaxios-fetch-adapter.ts
to wrap axios for fetch compatibility, supporting SOCKS5 proxies.OpenAiHandler
inopenai.ts
to use axios adapter when proxies are detected or configured.openAiUseAxiosForProxy
option inApiHandlerOptions
for manual override.openai.spec.ts
to includefetch
property expectations.This description was created by
for 99a9f2b. You can customize this summary. It will automatically update as commits are pushed.