fix: use shared HTTP client for sync and model fetch requests - #4467
fix: use shared HTTP client for sync and model fetch requests#4467seefs001 wants to merge 3 commits into
Conversation
- route official model sync through the shared HTTP client - route upstream ratio sync through the shared HTTP client - route channel model fetch through the shared HTTP client - route Ollama model operations through the shared HTTP client - route Ali image task polling through the shared HTTP client
WalkthroughRefactors HTTP client acquisition across controllers, relays, and service layer to use centralized Changes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
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: 1
🧹 Nitpick comments (4)
relay/channel/ollama/relay-ollama.go (2)
496-496: Optional: align with JSON wrapper convention while touching this function.
FetchOllamaVersionis being modified for the shared client; line 525 still usesjson.Unmarshaldirectly. Consider switching tocommon.Unmarshalfor consistency (other handlers in this file already usecommon.Unmarshal/common.Marshal).♻️ Proposed change
- if err := json.Unmarshal(body, &versionResp); err != nil { + if err := common.Unmarshal(body, &versionResp); err != nil { return "", fmt.Errorf("解析响应失败: %v", err) }As per coding guidelines: "All JSON marshal/unmarshal operations MUST use wrapper functions from
common/json.go... Do NOT directly import or callencoding/jsonin business code."Also applies to: 525-525
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/ollama/relay-ollama.go` at line 496, The FetchOllamaVersion flow uses the shared HTTP client created by getOllamaHTTPClient but still calls json.Unmarshal directly; replace that direct encoding/json call with the project's wrapper common.Unmarshal and ensure any corresponding Marshal usage follows common.Marshal conventions so FetchOllamaVersion (and its response handling) aligns with other handlers in this file.
22-36: Helper LGTM, with one optional polish.The shallow copy works correctly here (
http.Clienthas no lockable fields, and Transport is shared so connection pooling is preserved). A more idiomatic alternative would be applying the per-call timeout viacontext.WithTimeouton the request and always returning the shared client unchanged — that avoids allocating a newhttp.Clientper call. Not blocking.// Alternative sketch (callers would pass ctx-bound requests): // ctx, cancel := context.WithTimeout(parentCtx, timeout) // req, _ := http.NewRequestWithContext(ctx, ...) // service.GetHttpClient().Do(req)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/ollama/relay-ollama.go` around lines 22 - 36, The getOllamaHTTPClient helper currently clones service.GetHttpClient() and sets Timeout per-call; instead, avoid allocating per-call http.Clients by always returning the shared client from service.GetHttpClient() (or http.DefaultClient when nil) and apply per-request timeouts via context.WithTimeout in callers; update callers that use getOllamaHTTPClient to create a context-bound request (using context.WithTimeout(parentCtx, timeout)) and call the shared client's Do/DoContext, leaving getOllamaHTTPClient to simply return the shared client unchanged and keep connection pooling intact.controller/channel.go (1)
1078-1078: Optional: align JSON decoding withcommon.DecodeJson.While here, consider switching this
json.NewDecoder(...).Decode(...)tocommon.DecodeJson(response.Body, &result)for consistency with the JSON wrapper convention used elsewhere in this PR (e.g.,controller/model_sync.go).♻️ Proposed change
- if err := json.NewDecoder(response.Body).Decode(&result); err != nil { + if err := common.DecodeJson(response.Body, &result); err != nil {As per coding guidelines: "All JSON marshal/unmarshal operations MUST use wrapper functions in
common/json.go:common.Marshal(),common.Unmarshal(),common.UnmarshalJsonStr(),common.DecodeJson(),common.GetJsonType(). Do NOT directly import or callencoding/jsonin business code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/channel.go` at line 1078, Replace the direct use of json.NewDecoder(response.Body).Decode(&result) with the shared wrapper common.DecodeJson(response.Body, &result) to follow the project's JSON helper convention; update imports to remove direct encoding/json usage if no longer needed and ensure error handling remains the same (check the returned error from common.DecodeJson and propagate/log it as before), referencing the existing symbols json.NewDecoder, response.Body, result and the common.DecodeJson helper.controller/model_sync.go (1)
149-157: Nit: dual-decode failure surfaces only the envelope error.When both the envelope decode and the array fallback fail,
lastErr = errloseserr2, which is usually the more informative failure for the array path. Consider wrapping both, e.g.:♻️ Proposed change
- if err := common.Unmarshal(buf, out); err != nil { + if errEnv := common.Unmarshal(buf, out); errEnv != nil { // Try decode as pure array var arr []T - if err2 := common.Unmarshal(buf, &arr); err2 != nil { - lastErr = err + if errArr := common.Unmarshal(buf, &arr); errArr != nil { + lastErr = fmt.Errorf("envelope decode: %v; array decode: %w", errEnv, errArr) return }Same pattern applies to the 304 branch. Purely diagnostic; behavior unchanged.
Also applies to: 174-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/model_sync.go` around lines 149 - 157, When common.Unmarshal fails first (err) and the fallback array decode (err2) also fails, update the error handling to preserve both failures instead of assigning only lastErr = err; e.g., set lastErr to a wrapped/combined error that includes both err and err2 (using fmt.Errorf or errors.Join) so diagnostics show which decode failed. Apply the same change to the similar 304-branch error path so both the envelope and array decode errors are preserved for debugging (referencing common.Unmarshal, variables err and err2, and lastErr).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/ratio_sync.go`:
- Around line 70-75: getRatioSyncHTTPClient currently returns
service.GetHttpClient() or http.DefaultClient but dropped a previous custom
dialer that tried IPv4 first for github.io hosts, causing regressions for users
who rely on IPv4 reachability; restore a targeted fallback by returning an
*http.Client whose Transport uses Proxy: http.ProxyFromEnvironment and a
DialContext that, for hosts matching "*.github.io" or "basellm.github.io", first
attempts a "tcp4" dial and on failure tries "tcp6" (using net.Dialer.DialContext
for each); leave other hosts to the normal DialContext behavior; implement this
inside getRatioSyncHTTPClient only when service.GetHttpClient() is nil so
existing shared clients continue to be used.
---
Nitpick comments:
In `@controller/channel.go`:
- Line 1078: Replace the direct use of
json.NewDecoder(response.Body).Decode(&result) with the shared wrapper
common.DecodeJson(response.Body, &result) to follow the project's JSON helper
convention; update imports to remove direct encoding/json usage if no longer
needed and ensure error handling remains the same (check the returned error from
common.DecodeJson and propagate/log it as before), referencing the existing
symbols json.NewDecoder, response.Body, result and the common.DecodeJson helper.
In `@controller/model_sync.go`:
- Around line 149-157: When common.Unmarshal fails first (err) and the fallback
array decode (err2) also fails, update the error handling to preserve both
failures instead of assigning only lastErr = err; e.g., set lastErr to a
wrapped/combined error that includes both err and err2 (using fmt.Errorf or
errors.Join) so diagnostics show which decode failed. Apply the same change to
the similar 304-branch error path so both the envelope and array decode errors
are preserved for debugging (referencing common.Unmarshal, variables err and
err2, and lastErr).
In `@relay/channel/ollama/relay-ollama.go`:
- Line 496: The FetchOllamaVersion flow uses the shared HTTP client created by
getOllamaHTTPClient but still calls json.Unmarshal directly; replace that direct
encoding/json call with the project's wrapper common.Unmarshal and ensure any
corresponding Marshal usage follows common.Marshal conventions so
FetchOllamaVersion (and its response handling) aligns with other handlers in
this file.
- Around line 22-36: The getOllamaHTTPClient helper currently clones
service.GetHttpClient() and sets Timeout per-call; instead, avoid allocating
per-call http.Clients by always returning the shared client from
service.GetHttpClient() (or http.DefaultClient when nil) and apply per-request
timeouts via context.WithTimeout in callers; update callers that use
getOllamaHTTPClient to create a context-bound request (using
context.WithTimeout(parentCtx, timeout)) and call the shared client's
Do/DoContext, leaving getOllamaHTTPClient to simply return the shared client
unchanged and keep connection pooling intact.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc14d686-38c2-450f-b809-df6bcfe283b4
📒 Files selected for processing (5)
controller/channel.gocontroller/model_sync.gocontroller/ratio_sync.gorelay/channel/ali/image.gorelay/channel/ollama/relay-ollama.go
| func getRatioSyncHTTPClient() *http.Client { | ||
| if client := service.GetHttpClient(); client != nil { | ||
| return client | ||
| } | ||
| return http.DefaultClient | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n controller/ratio_sync.go | head -100Repository: QuantumNous/new-api
Length of output: 3139
🏁 Script executed:
git log --oneline -20 controller/ratio_sync.goRepository: QuantumNous/new-api
Length of output: 134
🏁 Script executed:
rg "getRatioSyncHTTPClient|GetHttpClient" -B 3 -A 5Repository: QuantumNous/new-api
Length of output: 20192
🏁 Script executed:
rg "github\.io" --type goRepository: QuantumNous/new-api
Length of output: 496
🏁 Script executed:
rg "IPv4|IPv6|Dial" controller/ --type go -B 2 -A 2Repository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
git show 9c4d5126 --statRepository: QuantumNous/new-api
Length of output: 50375
🏁 Script executed:
git show 9c4d5126 -- controller/ratio_sync.go | head -150Repository: QuantumNous/new-api
Length of output: 4188
🏁 Script executed:
cat -n service/http_client.go | head -80Repository: QuantumNous/new-api
Length of output: 2874
🏁 Script executed:
cat -n controller/ratio_sync.go | sed -n '125,215p'Repository: QuantumNous/new-api
Length of output: 3493
🏁 Script executed:
cat -n controller/model_sync.go | sed -n '1,100p'Repository: QuantumNous/new-api
Length of output: 3318
🏁 Script executed:
rg "IPv4|IPv6|Dial|github\.io" --type go -B 5 -A 5Repository: QuantumNous/new-api
Length of output: 10583
🏁 Script executed:
git log --all --oneline -- controller/ratio_sync.goRepository: QuantumNous/new-api
Length of output: 1189
🏁 Script executed:
git log --all --oneline -50 | grep -i ratioRepository: QuantumNous/new-api
Length of output: 388
🏁 Script executed:
git show 55c82713:controller/ratio_sync.go 2>/dev/null | head -100Repository: QuantumNous/new-api
Length of output: 2306
🏁 Script executed:
rg "retryIPv4|fallback|ipv4.*ipv6|github\.io.*retry" --type goRepository: QuantumNous/new-api
Length of output: 2906
🏁 Script executed:
cat -n service/http_client.go | sed -n '80,169p'Repository: QuantumNous/new-api
Length of output: 3107
🏁 Script executed:
git log --all --oneline -- controller/ratio_sync.goRepository: QuantumNous/new-api
Length of output: 1189
🏁 Script executed:
git show 55c82713 -- controller/ratio_sync.go | grep -A 20 -B 5 "getRatioSyncHTTPClient\|IPv4\|IPv6\|fallback\|Dial" | head -60Repository: QuantumNous/new-api
Length of output: 2806
🏁 Script executed:
git log -p --all -S "github.io" -- "*.go" | head -200Repository: QuantumNous/new-api
Length of output: 45
Shared client adoption LGTM; note: removed github.io IPv4→IPv6 dial fallback.
The helper and swap to client.Do(httpReq) are correct and enable HTTP_PROXY/HTTPS_PROXY env vars (fixing #4466).
The prior implementation at commit 55c8271 included a custom dialer that tried IPv4 first for *.github.io, falling back to IPv6 on failure. This fallback is now gone. For users without a proxy configured but relying on IPv4 reachability for basellm.github.io, this surfaces a connectivity regression. Consider either:
- Adding a dedicated dialer fallback for
*.github.iopreset hosts, or - Documenting that the recommended fix for github.io reachability issues is now
HTTP_PROXY/HTTPS_PROXY.
The retry loop at lines 200-208 safely reuses the same httpReq since the body is nil.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/ratio_sync.go` around lines 70 - 75, getRatioSyncHTTPClient
currently returns service.GetHttpClient() or http.DefaultClient but dropped a
previous custom dialer that tried IPv4 first for github.io hosts, causing
regressions for users who rely on IPv4 reachability; restore a targeted fallback
by returning an *http.Client whose Transport uses Proxy:
http.ProxyFromEnvironment and a DialContext that, for hosts matching
"*.github.io" or "basellm.github.io", first attempts a "tcp4" dial and on
failure tries "tcp6" (using net.Dialer.DialContext for each); leave other hosts
to the normal DialContext behavior; implement this inside getRatioSyncHTTPClient
only when service.GetHttpClient() is nil so existing shared clients continue to
be used.
# Conflicts: # controller/ratio_sync.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
service/http_client.go (1)
145-214: Optional: hoist the duplicated finalize block.The HTTP/HTTPS and SOCKS5 branches both repeat the
if opts.trustedRedirects { client.CheckRedirect = nil }+client.Timeout = ...+ cache-store sequence. A small helperfinalizeProxyClient(client, cacheKey, opts)would reduce duplication and keep future SSRF/timeout changes in one place. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/http_client.go` around lines 145 - 214, There is duplicated finalization logic after creating the http.Client in both the "http/https" and "socks5/socks5h" branches (the if opts.trustedRedirects { client.CheckRedirect = nil } + client.Timeout = ... + proxyClients cache-store block); extract that into a helper function (e.g., finalizeProxyClient(client *http.Client, cacheKey string, opts proxyOptions)) that sets CheckRedirect when opts.trustedRedirects is true, sets client.Timeout = time.Duration(common.RelayTimeout)*time.Second, acquires proxyClientLock, stores proxyClients[cacheKey] = client, releases the lock, and returns the client; then replace the duplicated sequences in both branches with a call to finalizeProxyClient so all timeout/SSRF-related finalization is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/user_notify.go`:
- Line 172: The change replacing GetHttpClient() with
GetHttpClient(WithTrustedRedirects()) for Bark/Gotify removes the per-redirect
SSRF protection (CheckRedirect becomes nil) so only the initial URL is validated
by ValidateURLWithFetchSetting and redirects to internal IPs can be followed
(dangerous for Gotify POSTs). Revert these callers to use the protected client
(remove WithTrustedRedirects) or explicitly restore a CheckRedirect that
enforces ValidateURLWithFetchSetting on each redirect; update the Bark and
Gotify call sites that call GetHttpClient/WithTrustedRedirects (references:
GetHttpClient, WithTrustedRedirects, ValidateURLWithFetchSetting, Bark/Gotify
send handlers) so redirects are not allowed to reach private/internal addresses
unless explicitly trusted.
---
Nitpick comments:
In `@service/http_client.go`:
- Around line 145-214: There is duplicated finalization logic after creating the
http.Client in both the "http/https" and "socks5/socks5h" branches (the if
opts.trustedRedirects { client.CheckRedirect = nil } + client.Timeout = ... +
proxyClients cache-store block); extract that into a helper function (e.g.,
finalizeProxyClient(client *http.Client, cacheKey string, opts proxyOptions))
that sets CheckRedirect when opts.trustedRedirects is true, sets client.Timeout
= time.Duration(common.RelayTimeout)*time.Second, acquires proxyClientLock,
stores proxyClients[cacheKey] = client, releases the lock, and returns the
client; then replace the duplicated sequences in both branches with a call to
finalizeProxyClient so all timeout/SSRF-related finalization is centralized.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63b211ff-9144-4eb5-8da4-0c75b2c1a3ed
📒 Files selected for processing (8)
controller/channel-billing.gocontroller/channel.gocontroller/model_sync.gocontroller/ratio_sync.gorelay/channel/ollama/relay-ollama.gorelay/channel/task/ali/adaptor.goservice/http_client.goservice/user_notify.go
🚧 Files skipped from review as they are similar to previous changes (1)
- controller/ratio_sync.go
|
|
||
| // 发送请求 | ||
| client := GetHttpClient() | ||
| client := GetHttpClient(WithTrustedRedirects()) |
There was a problem hiding this comment.
Question: should Bark/Gotify really opt into WithTrustedRedirects?
Switching these from GetHttpClient() to GetHttpClient(WithTrustedRedirects()) disables the per-redirect SSRF check (CheckRedirect becomes nil). The pre-flight ValidateURLWithFetchSetting (lines 158/252) only validates the initial URL — redirects to private/internal IPs are now followed. For Gotify in particular this is a POST with a JSON payload, so a malicious or compromised endpoint that returns a 30x toward http://127.0.0.1:<internal-port> can trigger an internal request originating from the server.
Bark/Gotify URLs are user-controlled (per-user setting), so the blast radius is limited to self-targeting, but this is still a regression vs. the previous behavior and is outside the stated PR scope (model sync / proxy fix). If there’s no concrete reason these flows must follow untrusted redirects, consider keeping the protected client here.
Suggested revert
- client := GetHttpClient(WithTrustedRedirects())
+ client := GetHttpClient()
resp, err = client.Do(req)(applies to both Bark at line 172 and Gotify at line 267)
Also applies to: 267-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/user_notify.go` at line 172, The change replacing GetHttpClient()
with GetHttpClient(WithTrustedRedirects()) for Bark/Gotify removes the
per-redirect SSRF protection (CheckRedirect becomes nil) so only the initial URL
is validated by ValidateURLWithFetchSetting and redirects to internal IPs can be
followed (dangerous for Gotify POSTs). Revert these callers to use the protected
client (remove WithTrustedRedirects) or explicitly restore a CheckRedirect that
enforces ValidateURLWithFetchSetting on each redirect; update the Bark and
Gotify call sites that call GetHttpClient/WithTrustedRedirects (references:
GetHttpClient, WithTrustedRedirects, ValidateURLWithFetchSetting, Bark/Gotify
send handlers) so redirects are not allowed to reach private/internal addresses
unless explicitly trusted.
1 participant
Important
📝 变更描述 / Description
(简述：做了什么？为什么这样改能生效？请基于你对代码逻辑的理解来写，避免粘贴未经整理的内容)
部分逻辑没有使用项目通用的http.Client，导致HTTP_PROXY等环境变量配置无效。
涉及：
他们都是可以信任的请求地址，不能使用默认的redirect fetch protection策略，对默认的http.Client增加option跳过拦截，另对通知场景（Bark / Gotify ）的http请求也跳过redirect fetch protection。
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
(请在此粘贴截图、关键日志或测试报告，以证明变更生效)
Summary by CodeRabbit