feat(relay): exclude-driven channel failover, adaptive retry budget, and rate-limit cooldown circuit breaker - #6580
Open
Luckylos wants to merge 7 commits into
Open
Conversation
added 7 commits
August 1, 2026 10:59
Fixes "no available channel" errors where healthy channels were never tried before the request failed. Three root causes addressed: 1. Retry no longer re-selects failed channels (QuantumNous#1) GetRandomSatisfiedChannel / GetChannel now take an excludeChannels set (channel IDs already tried this request). Selection is rewritten to be exclude-driven: group non-excluded candidates by priority, pick the highest tier that still has channels, weighted-random within it. Returns (nil,nil) when the pool is exhausted so the caller stops cleanly. The relay loop populates the set after each attempt. 2. Same-priority channels are all exhausted before descending (QuantumNous#6) Previously retry was a priority index, so same-priority siblings were skipped. Now every channel in a tier is tried first. 3. 504/524 timeouts fail over instead of aborting (QuantumNous#2) Removed the hardcoded always-skip guard from the sync relay path; AutomaticRetryStatusCodeRanges is now authoritative and defaults to the full 5xx range. The async task relay keeps its own timeout protection to avoid duplicate submits. 4. Adaptive retry budget (QuantumNous#5) Retry cap = max(RetryTimes, availableChannels-1) via new CountAvailableChannels, so every channel can be tried even when RetryTimes is smaller than the pool. Pinned-channel requests stay at 1. Tests: channel_failover_test.go + updated status_code_ranges_test.go. go build + vet + all package tests pass.
…e, pure exclude-driven auto group, adaptive cap fixes - Remove dead `retry` param from GetRandomSatisfiedChannel/GetChannel (debt QuantumNous#1) - Rewrite auto-group selection to be purely exclude-driven; drop priorityRetry index, retry-counter manipulation, resetNextTry, and dead AutoGroupRetryIndex writes. Distinguish discovery miss (always skip group) from failover exhaustion (advance only when cross-group retry enabled) (debt QuantumNous#2) - Multi-key aware exclude: a channel is only excluded after all its enabled keys have been tried, preserving per-request key rotation (debt QuantumNous#3) - Size retry budget by total enabled keys (CountEnabledKeys) so every key of a multi-key channel fits within the adaptive cap (debt QuantumNous#3) - Fix shouldRetry to use retryCap instead of common.RetryTimes so the adaptive cap is not silently clamped (debt QuantumNous#4) - Task relay loop now carries exclude set with the same multi-key logic - Tests: multi-key CountEnabledKeys + key-weighted budget
…ry-After, skip cooling channels/keys in selection with fallback - Add cross-request key-level cooldown store (TTL map, clamped to 300s) - Parse upstream Retry-After / X-RateLimit-Reset headers in RelayErrorHandler - Register cooldown on 429 (or any Retry-After) in processChannelError, key-aware - GetNextEnabledKey and channel selection prefer non-cooling keys/channels, always fall back so cooldown never denies service - Clear cooldown on success (sync + task relay loops) - TDD: cooldown store, selection skip, key skip, Retry-After parsing
… health-check path, parse OpenAI duration reset headers
- Move cooldown mark out of processChannelError (shared with channel
health-check path) into the relay loops via markCooldownFromError, so
admin channel tests never pollute live rate-limit state.
- Extract contextKeyIndex / clearCooldownForContext helpers, removing
three copies of the multi-key-index lookup.
- Parse OpenAI's Go-duration reset headers ("6m0s", "1s", "88ms") in
ParseRetryAfterSeconds; previously ParseFloat failed on them and the
precise upstream hint was silently dropped to the default cooldown.
Sub-second resets round up to 1s. Adds regression tests.
shouldRetry now bails out when relayInfo.HasSendResponse() is true, so a mid-stream upstream error (e.g. Claude emitting an error event after message_start has already flushed SSE bytes) can no longer trigger a retry that appends a second response onto the partial output on the same http.ResponseWriter, which corrupted/duplicated what the client saw. Individual stream handlers mostly avoid propagating a retryable error after their first flush, but that protection was scattered and had gaps (Claude's WithClaudeError path returns a retryable 500 that is not in alwaysSkipRetryCodes). This adds the single authoritative guard.
…ile key rotation Channel-level upstream failures (5xx, auth_unavailable — anything that is not a per-key 429/Retry-After) hit every key of a channel the same way, so burning the remaining keys only adds latency before failover. Introduce NewAPIError.IsRateLimited as the single source of truth (shared with cooldown) and a recordChannelFailure helper that excludes the channel immediately on a channel-level error, while a rate-limit still rotates keys until all are throttled. Applied to both the sync relay loop and the async task loop.
upstream extracted relaykit (QuantumNous#6369), moving NewAPIError/ErrorCode to relaykit/types and AdvancedCustomConfig to relaykit/dto. Update the test imports to match; behavior unchanged.
Contributor
WalkthroughRelay retries now exclude failed channels, rotate keys for rate-limited channels, apply parsed cooldowns, and adapt retry limits to available keys. Channel selection and group routing accept exclusions. Synchronous retries include the full 5xx range. The version is updated. ChangesAdaptive channel failover
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Relay
participant ChannelSelection
participant Upstream
participant RetryState
Relay->>ChannelSelection: request channel with exclusions
ChannelSelection-->>Relay: return channel and key
Relay->>Upstream: send request
Upstream-->>Relay: return response or failure
Relay->>RetryState: clear cooldown or record failure
RetryState-->>Relay: provide retry decision
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
model/ability.go (1)
109-120: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider applying key cooldown in the cache-disabled path too.
model/channel_cache.goLines 181-189 skip channels whose enabled keys are all in cooldown. This candidate list has no equivalent step, so deployments withMemoryCacheEnabled=falsekeep selecting a just-throttled channel. Load the candidate channels once and applyEnabledKeysAllCoolingDownwith the same full-tier fallback to keep behavior identical in both modes.🤖 Prompt for 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. In `@model/ability.go` around lines 109 - 120, The candidate selection loop in the ability selection flow must also exclude channels whose enabled keys are all cooling down when the cache is disabled. Load candidate channels once, apply EnabledKeysAllCoolingDown before appending candidates, and preserve the same full-tier fallback behavior used by model/channel_cache.go so both cache modes select consistently.controller/relay.go (1)
384-396: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDo not use
HasSendResponse()to mean client writes.Several assignment sites mark the response as sent after upstream reads/events and before rendering:
relay/helper/stream_scanner.go:SetFirstResponseTime()runs beforedataChan.relay/channel/openai/relay_realtime.go:SetFirstResponseTime()runs before JSON unmarshalling.relay/channel/aws/relay-aws.go:SetFirstResponseTime()runs beforeHandleStreamResponseData.relay/channel/openai/relay_image.go:SetFirstResponseTime()runs before marshalling usage and SSE payloads.Only some direct render sites are close to writes, such as
relay/channel/cloudflare/relay_cloudflare.goandrelay/channel/cohere/relay-cohere.go. Update theshouldRetrycomment to say that this guard checks whether the relay session has begun reporting response timing, or move the write guard to actualWriter/render state for each channel.🤖 Prompt for 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. In `@controller/relay.go` around lines 384 - 396, Update the comment in shouldRetry to accurately describe info.HasSendResponse() as indicating that the relay session has begun reporting response timing, not that bytes were written to the client; leave the retry guard behavior unchanged unless an existing writer/render-state signal is already available.
🤖 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 `@model/ability.go`:
- Around line 69-83: The GetChannel function currently filters only abilities
loaded for the exact model, causing discovery to miss channels that match the
normalized model name. Before filterAbilitiesByRequestPathAndModel, apply the
same ratio_setting.FormatMatchingModelName(model) fallback used by
model/channel_cache.go when exact-model abilities produce no matches, and
preserve the existing ordering and empty-result behavior.
In `@model/channel_cache.go`:
- Around line 268-296: Update CountAvailableChannels in the cache-disabled
branch to replace per-ability GetChannelById calls with one batched channel
query using the distinct ChannelId values, selecting only fields needed to count
enabled keys rather than full key material. Preserve the existing fallback count
for missing channel records, deduplication, request-path filtering, and
normalized-model retry behavior.
In `@model/channel_cooldown_test.go`:
- Around line 11-174: Replace raw testing assertions in
model/channel_cooldown_test.go lines 11-174 with testify/require equivalents for
fatal checks, using require.True, require.False, require.Equal, require.NotNil,
or require.NoError as appropriate; use assert only for non-fatal checks, and add
the required import. In service/retry_after_test.go lines 10-116, replace every
t.Fatalf with testify/require assertions comparing expected values from
ParseRetryAfterSeconds, and add the required import.
In `@model/channel_failover_test.go`:
- Around line 50-210: Update model/channel_failover_test.go lines 50-210 to use
testify/require for NoError, Equal, Nil, and NotNil assertions in the channel
failover tests. Update controller/record_channel_failure_test.go lines 27-86 to
use require.True/False for exclusion checks and assert.Equal for counter checks.
Update controller/should_retry_test.go lines 45-73 to use require.False/True for
retry expectations and explicitly initialize retry status-code settings and
relevant test state instead of relying on global defaults.
- Around line 13-48: Update setupChannelCache to reset the channelKeyCooldown
global when initializing the fixture and restore its previous value in the
returned cleanup function, alongside the other cache globals, so tests reusing
channel IDs remain isolated and deterministic.
In `@relaykit/types/error.go`:
- Around line 127-144: Update NewAPIError.IsRateLimited to return true only when
StatusCode equals http.StatusTooManyRequests; remove the RetryAfterSeconds
condition so retry hints on 5xx or other responses cannot classify the error as
key-level rate limiting.
---
Nitpick comments:
In `@controller/relay.go`:
- Around line 384-396: Update the comment in shouldRetry to accurately describe
info.HasSendResponse() as indicating that the relay session has begun reporting
response timing, not that bytes were written to the client; leave the retry
guard behavior unchanged unless an existing writer/render-state signal is
already available.
In `@model/ability.go`:
- Around line 109-120: The candidate selection loop in the ability selection
flow must also exclude channels whose enabled keys are all cooling down when the
cache is disabled. Load candidate channels once, apply EnabledKeysAllCoolingDown
before appending candidates, and preserve the same full-tier fallback behavior
used by model/channel_cache.go so both cache modes select consistently.
🪄 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 Plus
Run ID: 75bd366c-3006-4518-b96c-25b9d9579d51
📒 Files selected for processing (16)
VERSIONcontroller/record_channel_failure_test.gocontroller/relay.gocontroller/should_retry_test.gomodel/ability.gomodel/channel.gomodel/channel_cache.gomodel/channel_cooldown.gomodel/channel_cooldown_test.gomodel/channel_failover_test.gorelaykit/types/error.goservice/channel_select.goservice/error.goservice/retry_after_test.gosetting/operation_setting/status_code_ranges.gosetting/operation_setting/status_code_ranges_test.go
Comment on lines
+69
to
83
| func GetChannel(group string, model string, requestPath string, excludeChannels map[int]bool) (*Channel, error) { | ||
| var abilities []Ability | ||
|
|
||
| // Load all enabled abilities for this group/model ordered by priority so we | ||
| // can pick the highest tier that still has non-excluded channels. | ||
| err := DB.Where(commonGroupCol+" = ? and model = ? and enabled = ?", group, model, true). | ||
| Order("priority DESC"). | ||
| Find(&abilities).Error | ||
| if err != nil { | ||
| // 处理错误 | ||
| return 0, err | ||
| } | ||
|
|
||
| if len(priorities) == 0 { | ||
| // 如果没有查询到优先级，则返回错误 | ||
| return 0, errors.New("数据库一致性被破坏") | ||
| return nil, err | ||
| } | ||
|
|
||
| // 确定要使用的优先级 | ||
| var priorityToUse int | ||
| if retry >= len(priorities) { | ||
| // 如果重试次数大于优先级数，则使用最小的优先级 | ||
| priorityToUse = priorities[len(priorities)-1] | ||
| } else { | ||
| priorityToUse = priorities[retry] | ||
| abilities = filterAbilitiesByRequestPathAndModel(abilities, requestPath, model) | ||
| if len(abilities) == 0 { | ||
| return nil, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify model-name resolution parity between the DB path and the cache path.
set -uo pipefail
# Where is FormatMatchingModelName used in model/?
rg -n --type=go 'FormatMatchingModelName' model/
# Inspect the ability query and any normalized retry in ability.go
ast-grep run --pattern 'func GetChannel($$$) { $$$ }' --lang go model/ability.goRepository: QuantumNous/new-api
Length of output: 616
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "== Locate ability.go and relevant files =="
fd -a 'ability\.go|channel_cache\.go|channel_select\.go|channel_satisfy\.go|.*ratio.*' . | sed 's#^\./##'
echo
echo "== model/ability.go outline/section =="
wc -l model/ability.go
sed -n '1,140p' model/ability.go | cat -n
echo
echo "== model/channel_cache.go relevant sections =="
wc -l model/channel_cache.go
sed -n '110,180p' model/channel_cache.go | cat -n
sed -n '260,315p' model/channel_cache.go | cat -n
echo
echo "== service/channel_select.go around hadChannels =="
wc -l service/channel_select.go
sed -n '1,180p' service/channel_select.go | cat -n
echo
echo "== model/channel_satisfy.go =="
wc -l model/channel_satisfy.go
sed -n '1,90p' model/channel_satisfy.go | cat -n
echo
echo "== FormatMatchingModelName definitions/usages =="
rg -n --type=go 'FormatMatchingModelName|type .*Ratio|OtherRatios|matching' .Repository: QuantumNous/new-api
Length of output: 31566
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "== model/ability.go lines 140-230 =="
sed -n '140,230p' model/ability.go | cat -n
echo
echo "== CountAvailableChannels full section with model fallback =="
sed -n '1,55p' model/channel_cache.go | cat -n
echo
echo "== FormatMatchingModelName definition =="
sed -n '700,740p' setting/ratio_setting/model_ratio.go | cat -nRepository: QuantumNous/new-api
Length of output: 7243
Add the normalized-model fallback to GetChannel.
model/channel_cache.go retries with ratio_setting.FormatMatchingModelName(model) when no channels match the exact model. Use the same fallback in model/ability.go before filterAbilitiesByRequestPathAndModel, so CountAvailableChannels and selection exit discovery misses together and do not stop auto-group routing on a false positive count.
🤖 Prompt for 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.
In `@model/ability.go` around lines 69 - 83, The GetChannel function currently
filters only abilities loaded for the exact model, causing discovery to miss
channels that match the normalized model name. Before
filterAbilitiesByRequestPathAndModel, apply the same
ratio_setting.FormatMatchingModelName(model) fallback used by
model/channel_cache.go when exact-model abilities produce no matches, and
preserve the existing ordering and empty-result behavior.
Comment on lines
+268
to
+296
| func CountAvailableChannels(group string, model string, requestPath string) int { | ||
| if !common.MemoryCacheEnabled { | ||
| var abilities []Ability | ||
| if err := DB.Where(commonGroupCol+" = ? and model = ? and enabled = ?", group, model, true). | ||
| Find(&abilities).Error; err != nil { | ||
| return 0 | ||
| } | ||
| abilities = filterAbilitiesByRequestPathAndModel(abilities, requestPath, model) | ||
| seen := make(map[int]struct{}, len(abilities)) | ||
| total := 0 | ||
| for _, ability := range abilities { | ||
| if _, ok := seen[ability.ChannelId]; ok { | ||
| continue | ||
| } | ||
| seen[ability.ChannelId] = struct{}{} | ||
| if channel, err := GetChannelById(ability.ChannelId, true); err == nil { | ||
| total += channel.CountEnabledKeys() | ||
| } else { | ||
| total++ | ||
| } | ||
| } | ||
| if total == 0 { | ||
| normalized := ratio_setting.FormatMatchingModelName(model) | ||
| if normalized != "" && normalized != model { | ||
| return CountAvailableChannels(group, normalized, requestPath) | ||
| } | ||
| } | ||
| return total | ||
| } |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Batch the channel lookups in the cache-disabled path.
The loop calls GetChannelById once per distinct ability, so this helper issues N+1 queries. It runs on the request path: controller/relay.go Line 346 calls it once per auto group for every request, and service/channel_select.go Line 112 calls it again per group on every retry. selectAll=true also selects the key column, so full key material is read only to count keys.
Load the candidate channels in one query instead.
♻️ Proposed fix: single batched query
abilities = filterAbilitiesByRequestPathAndModel(abilities, requestPath, model)
- seen := make(map[int]struct{}, len(abilities))
- total := 0
- for _, ability := range abilities {
- if _, ok := seen[ability.ChannelId]; ok {
- continue
- }
- seen[ability.ChannelId] = struct{}{}
- if channel, err := GetChannelById(ability.ChannelId, true); err == nil {
- total += channel.CountEnabledKeys()
- } else {
- total++
- }
- }
+ seen := make(map[int]struct{}, len(abilities))
+ channelIds := make([]int, 0, len(abilities))
+ for _, ability := range abilities {
+ if _, ok := seen[ability.ChannelId]; ok {
+ continue
+ }
+ seen[ability.ChannelId] = struct{}{}
+ channelIds = append(channelIds, ability.ChannelId)
+ }
+ total := 0
+ if len(channelIds) > 0 {
+ var channels []*Channel
+ if err := DB.Where("id IN ?", channelIds).Find(&channels).Error; err != nil {
+ // fall back to one attempt per channel
+ total = len(channelIds)
+ } else {
+ found := make(map[int]struct{}, len(channels))
+ for _, channel := range channels {
+ found[channel.Id] = struct{}{}
+ total += channel.CountEnabledKeys()
+ }
+ total += len(channelIds) - len(found)
+ }
+ }🤖 Prompt for 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.
In `@model/channel_cache.go` around lines 268 - 296, Update CountAvailableChannels
in the cache-disabled branch to replace per-ability GetChannelById calls with
one batched channel query using the distinct ChannelId values, selecting only
fields needed to count enabled keys rather than full key material. Preserve the
existing fallback count for missing channel records, deduplication, request-path
filtering, and normalized-model retry behavior.
Comment on lines
+11
to
+174
| func TestChannelKeyCooldown_MarkIsClear(t *testing.T) { | ||
| channelKeyCooldown.Range(func(k, _ any) bool { | ||
| channelKeyCooldown.Delete(k) | ||
| return true | ||
| }) | ||
|
|
||
| if IsChannelKeyCoolingDown(100, 0) { | ||
| t.Fatal("fresh key should not be cooling down") | ||
| } | ||
|
|
||
| MarkChannelKeyCooldown(100, 0, 30) | ||
| if !IsChannelKeyCoolingDown(100, 0) { | ||
| t.Fatal("key should be cooling down after Mark") | ||
| } | ||
| // Different key index of the same channel is independent. | ||
| if IsChannelKeyCoolingDown(100, 1) { | ||
| t.Fatal("unrelated key index should not be cooling down") | ||
| } | ||
|
|
||
| ClearChannelKeyCooldown(100, 0) | ||
| if IsChannelKeyCoolingDown(100, 0) { | ||
| t.Fatal("key should be selectable after Clear") | ||
| } | ||
| } | ||
|
|
||
| func TestChannelKeyCooldown_Expiry(t *testing.T) { | ||
| // A cooldown that already expired must read as not cooling down and be | ||
| // cleaned up lazily. | ||
| channelKeyCooldown.Store(channelKeyCooldownMapKey(101, 0), time.Now().Unix()-1) | ||
| if IsChannelKeyCoolingDown(101, 0) { | ||
| t.Fatal("expired cooldown should read as not cooling down") | ||
| } | ||
| if _, ok := channelKeyCooldown.Load(channelKeyCooldownMapKey(101, 0)); ok { | ||
| t.Fatal("expired entry should be cleaned up on read") | ||
| } | ||
| } | ||
|
|
||
| func TestChannelKeyCooldown_Clamp(t *testing.T) { | ||
| channelKeyCooldown.Range(func(k, _ any) bool { | ||
| channelKeyCooldown.Delete(k) | ||
| return true | ||
| }) | ||
|
|
||
| // Oversized hint is clamped so a hostile header cannot park a key forever. | ||
| MarkChannelKeyCooldown(102, 0, MaxChannelCooldownSeconds+10_000) | ||
| v, _ := channelKeyCooldown.Load(channelKeyCooldownMapKey(102, 0)) | ||
| until, _ := v.(int64) | ||
| if max := time.Now().Unix() + int64(MaxChannelCooldownSeconds); until > max { | ||
| t.Fatalf("cooldown not clamped: until=%d, max=%d", until, max) | ||
| } | ||
| } | ||
|
|
||
| func TestEnabledKeysAllCoolingDown_SingleKey(t *testing.T) { | ||
| channelKeyCooldown.Range(func(k, _ any) bool { | ||
| channelKeyCooldown.Delete(k) | ||
| return true | ||
| }) | ||
| ch := &Channel{Id: 200, Key: "k1"} | ||
|
|
||
| if ch.EnabledKeysAllCoolingDown() { | ||
| t.Fatal("single-key channel with no cooldown should be ready") | ||
| } | ||
| MarkChannelKeyCooldown(200, 0, 30) | ||
| if !ch.EnabledKeysAllCoolingDown() { | ||
| t.Fatal("single-key channel with its key cooling should report all-cooling") | ||
| } | ||
| } | ||
|
|
||
| func TestEnabledKeysAllCoolingDown_MultiKey(t *testing.T) { | ||
| channelKeyCooldown.Range(func(k, _ any) bool { | ||
| channelKeyCooldown.Delete(k) | ||
| return true | ||
| }) | ||
| ch := &Channel{Id: 201, Key: "k1\nk2\nk3"} | ||
| ch.ChannelInfo.IsMultiKey = true | ||
|
|
||
| // One of three keys cooling -> channel still has ready keys. | ||
| MarkChannelKeyCooldown(201, 0, 30) | ||
| if ch.EnabledKeysAllCoolingDown() { | ||
| t.Fatal("channel with 2 ready keys should not be all-cooling") | ||
| } | ||
| // All three cooling -> all-cooling. | ||
| MarkChannelKeyCooldown(201, 1, 30) | ||
| MarkChannelKeyCooldown(201, 2, 30) | ||
| if !ch.EnabledKeysAllCoolingDown() { | ||
| t.Fatal("channel with every key cooling should report all-cooling") | ||
| } | ||
| } | ||
|
|
||
| func TestEnabledKeysAllCoolingDown_SkipsDisabledKeys(t *testing.T) { | ||
| channelKeyCooldown.Range(func(k, _ any) bool { | ||
| channelKeyCooldown.Delete(k) | ||
| return true | ||
| }) | ||
| ch := &Channel{Id: 202, Key: "k1\nk2"} | ||
| ch.ChannelInfo.IsMultiKey = true | ||
| // Disable key 1; only key 0 counts. Cool key 0 -> all (enabled) cooling. | ||
| ch.ChannelInfo.MultiKeyStatusList = map[int]int{1: common.ChannelStatusAutoDisabled} | ||
| MarkChannelKeyCooldown(202, 0, 30) | ||
| if !ch.EnabledKeysAllCoolingDown() { | ||
| t.Fatal("only enabled key is cooling -> should report all-cooling") | ||
| } | ||
| } | ||
|
|
||
| // TestGetRandomSatisfiedChannel_SkipsCoolingChannel verifies selection prefers a | ||
| // channel with a ready key over one whose only key is cooling down, but falls | ||
| // back when every candidate in the tier is cooling. | ||
| func TestGetRandomSatisfiedChannel_SkipsCoolingChannel(t *testing.T) { | ||
| channelKeyCooldown.Range(func(k, _ any) bool { | ||
| channelKeyCooldown.Delete(k) | ||
| return true | ||
| }) | ||
| cleanup := setupChannelCache(t, "default", "gpt-x", [][3]int{ | ||
| {1, 10, 0}, | ||
| {2, 10, 0}, | ||
| }) | ||
| defer cleanup() | ||
|
|
||
| // Cool ch1's key -> selection must return ch2 every time. | ||
| MarkChannelKeyCooldown(1, 0, 30) | ||
| for i := 0; i < 20; i++ { | ||
| ch, err := GetRandomSatisfiedChannel("default", "gpt-x", "", nil) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if ch == nil || ch.Id != 2 { | ||
| t.Fatalf("expected ch2 (ch1 cooling), got %v", ch) | ||
| } | ||
| } | ||
|
|
||
| // Cool ch2 as well -> both cooling, must fall back (never deny service). | ||
| MarkChannelKeyCooldown(2, 0, 30) | ||
| ch, err := GetRandomSatisfiedChannel("default", "gpt-x", "", nil) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if ch == nil { | ||
| t.Fatal("expected a fallback channel when all cooling, got nil") | ||
| } | ||
| } | ||
|
|
||
| // TestGetNextEnabledKey_SkipsCoolingKey verifies multi-key selection avoids a | ||
| // cooling key when a ready one exists. | ||
| func TestGetNextEnabledKey_SkipsCoolingKey(t *testing.T) { | ||
| channelKeyCooldown.Range(func(k, _ any) bool { | ||
| channelKeyCooldown.Delete(k) | ||
| return true | ||
| }) | ||
| ch := &Channel{Id: 300, Key: "k1\nk2"} | ||
| ch.ChannelInfo.IsMultiKey = true | ||
| ch.ChannelInfo.MultiKeyMode = constant.MultiKeyModeRandom | ||
|
|
||
| // Cool key 0 -> every selection must land on key 1. | ||
| MarkChannelKeyCooldown(300, 0, 30) | ||
| for i := 0; i < 20; i++ { | ||
| _, idx, apiErr := ch.GetNextEnabledKey() | ||
| if apiErr != nil { | ||
| t.Fatalf("unexpected error: %v", apiErr) | ||
| } | ||
| if idx != 1 { | ||
| t.Fatalf("expected key idx 1 (idx 0 cooling), got %d", idx) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Both new test files use raw testing assertions instead of the required testify library.
Both files are new Go backend test files. Both use t.Fatal/t.Fatalf directly instead of testify/require and testify/assert. Convert the fatal assertions to require calls and any non-fatal checks to assert calls.
model/channel_cooldown_test.go#L11-L174: replace eacht.Fatal/t.Fatalfwithrequire.False/require.True/require.Equal/require.Nil, etc., fromtestify/require.service/retry_after_test.go#L10-L116: replace eacht.Fatalfwithrequire.Equal(t, expected, ParseRetryAfterSeconds(h))(or equivalent), pulling intestify/require.
As per coding guidelines, "New or substantially rewritten Go backend tests must use testify/require for setup and fatal assertions and testify/assert for non-fatal checks."
📍 Affects 2 files
model/channel_cooldown_test.go#L11-L174(this comment)service/retry_after_test.go#L10-L116
🤖 Prompt for 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.
In `@model/channel_cooldown_test.go` around lines 11 - 174, Replace raw testing
assertions in model/channel_cooldown_test.go lines 11-174 with testify/require
equivalents for fatal checks, using require.True, require.False, require.Equal,
require.NotNil, or require.NoError as appropriate; use assert only for non-fatal
checks, and add the required import. In service/retry_after_test.go lines
10-116, replace every t.Fatalf with testify/require assertions comparing
expected values from ParseRetryAfterSeconds, and add the required import.
Source: Coding guidelines
Comment on lines
+13
to
+48
| func setupChannelCache(t *testing.T, group, modelName string, entries [][3]int) func() { | ||
| t.Helper() | ||
|
|
||
| oldMemoryCache := common.MemoryCacheEnabled | ||
| oldGroup2model := group2model2channels | ||
| oldChannelsIDM := channelsIDM | ||
| oldAdvanced := channel2advancedCustomConfig | ||
|
|
||
| common.MemoryCacheEnabled = true | ||
|
|
||
| channelsIDM = make(map[int]*Channel) | ||
| ids := make([]int, 0, len(entries)) | ||
| for _, e := range entries { | ||
| id, priority, weight := e[0], e[1], e[2] | ||
| p := int64(priority) | ||
| w := uint(weight) | ||
| channelsIDM[id] = &Channel{ | ||
| Id: id, | ||
| Status: common.ChannelStatusEnabled, | ||
| Priority: &p, | ||
| Weight: &w, | ||
| } | ||
| ids = append(ids, id) | ||
| } | ||
| group2model2channels = map[string]map[string][]int{ | ||
| group: {modelName: ids}, | ||
| } | ||
| channel2advancedCustomConfig = make(map[int]*dto.AdvancedCustomConfig) | ||
|
|
||
| return func() { | ||
| common.MemoryCacheEnabled = oldMemoryCache | ||
| group2model2channels = oldGroup2model | ||
| channelsIDM = oldChannelsIDM | ||
| channel2advancedCustomConfig = oldAdvanced | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reset the key cooldown map in the fixture.
setupChannelCache restores the cache globals but not channelKeyCooldown. model/channel_cooldown_test.go TestGetRandomSatisfiedChannel_SkipsCoolingChannel marks channels 1 and 2 in cooldown for 30 seconds and does not clear them, and these tests reuse the same channel IDs. The assertions currently still pass, but only through the "all cooling → use the full tier" fallback, so the results depend on test order.
Clear the cooldown map in the fixture to make selection deterministic.
💚 Proposed fix
common.MemoryCacheEnabled = true
+ channelKeyCooldown.Range(func(k, _ any) bool {
+ channelKeyCooldown.Delete(k)
+ return true
+ })
+
channelsIDM = make(map[int]*Channel)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func setupChannelCache(t *testing.T, group, modelName string, entries [][3]int) func() { | |
| t.Helper() | |
| oldMemoryCache := common.MemoryCacheEnabled | |
| oldGroup2model := group2model2channels | |
| oldChannelsIDM := channelsIDM | |
| oldAdvanced := channel2advancedCustomConfig | |
| common.MemoryCacheEnabled = true | |
| channelsIDM = make(map[int]*Channel) | |
| ids := make([]int, 0, len(entries)) | |
| for _, e := range entries { | |
| id, priority, weight := e[0], e[1], e[2] | |
| p := int64(priority) | |
| w := uint(weight) | |
| channelsIDM[id] = &Channel{ | |
| Id: id, | |
| Status: common.ChannelStatusEnabled, | |
| Priority: &p, | |
| Weight: &w, | |
| } | |
| ids = append(ids, id) | |
| } | |
| group2model2channels = map[string]map[string][]int{ | |
| group: {modelName: ids}, | |
| } | |
| channel2advancedCustomConfig = make(map[int]*dto.AdvancedCustomConfig) | |
| return func() { | |
| common.MemoryCacheEnabled = oldMemoryCache | |
| group2model2channels = oldGroup2model | |
| channelsIDM = oldChannelsIDM | |
| channel2advancedCustomConfig = oldAdvanced | |
| } | |
| } | |
| func setupChannelCache(t *testing.T, group, modelName string, entries [][3]int) func() { | |
| t.Helper() | |
| oldMemoryCache := common.MemoryCacheEnabled | |
| oldGroup2model := group2model2channels | |
| oldChannelsIDM := channelsIDM | |
| oldAdvanced := channel2advancedCustomConfig | |
| common.MemoryCacheEnabled = true | |
| channelKeyCooldown.Range(func(k, _ any) bool { | |
| channelKeyCooldown.Delete(k) | |
| return true | |
| }) | |
| channelsIDM = make(map[int]*Channel) | |
| ids := make([]int, 0, len(entries)) | |
| for _, e := range entries { | |
| id, priority, weight := e[0], e[1], e[2] | |
| p := int64(priority) | |
| w := uint(weight) | |
| channelsIDM[id] = &Channel{ | |
| Id: id, | |
| Status: common.ChannelStatusEnabled, | |
| Priority: &p, | |
| Weight: &w, | |
| } | |
| ids = append(ids, id) | |
| } | |
| group2model2channels = map[string]map[string][]int{ | |
| group: {modelName: ids}, | |
| } | |
| channel2advancedCustomConfig = make(map[int]*dto.AdvancedCustomConfig) | |
| return func() { | |
| common.MemoryCacheEnabled = oldMemoryCache | |
| group2model2channels = oldGroup2model | |
| channelsIDM = oldChannelsIDM | |
| channel2advancedCustomConfig = oldAdvanced | |
| } | |
| } |
🤖 Prompt for 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.
In `@model/channel_failover_test.go` around lines 13 - 48, Update
setupChannelCache to reset the channelKeyCooldown global when initializing the
fixture and restore its previous value in the returned cleanup function,
alongside the other cache globals, so tests reusing channel IDs remain isolated
and deterministic.
Comment on lines
+50
to
+210
| func TestGetRandomSatisfiedChannel_ExcludesFailedChannels(t *testing.T) { | ||
| cleanup := setupChannelCache(t, "default", "gpt-x", [][3]int{ | ||
| {1, 0, 0}, | ||
| {2, 0, 0}, | ||
| {3, 0, 0}, | ||
| }) | ||
| defer cleanup() | ||
|
|
||
| // exclude ch1 -> must return ch2 or ch3, never ch1 | ||
| for i := 0; i < 20; i++ { | ||
| ch, err := GetRandomSatisfiedChannel("default", "gpt-x", "", map[int]bool{1: true}) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if ch == nil { | ||
| t.Fatal("expected a channel, got nil") | ||
| } | ||
| if ch.Id == 1 { | ||
| t.Fatalf("excluded channel #1 was returned") | ||
| } | ||
| } | ||
|
|
||
| // exclude ch1+ch2 -> must return ch3 | ||
| ch, err := GetRandomSatisfiedChannel("default", "gpt-x", "", map[int]bool{1: true, 2: true}) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if ch == nil || ch.Id != 3 { | ||
| t.Fatalf("expected ch3, got %v", ch) | ||
| } | ||
|
|
||
| // exclude all -> no more channels to try | ||
| ch, err = GetRandomSatisfiedChannel("default", "gpt-x", "", map[int]bool{1: true, 2: true, 3: true}) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if ch != nil { | ||
| t.Fatalf("expected nil when all channels excluded, got #%d", ch.Id) | ||
| } | ||
| } | ||
|
|
||
| // TestGetRandomSatisfiedChannel_PriorityDescentAfterExhaustion verifies bug #6: | ||
| // same-priority channels must all be tried before descending to a lower | ||
| // priority. ch1/ch2 share priority 10, ch3 has priority 5. | ||
| func TestGetRandomSatisfiedChannel_PriorityDescentAfterExhaustion(t *testing.T) { | ||
| cleanup := setupChannelCache(t, "default", "gpt-x", [][3]int{ | ||
| {1, 10, 0}, | ||
| {2, 10, 0}, | ||
| {3, 5, 0}, | ||
| }) | ||
| defer cleanup() | ||
|
|
||
| // exclude ch1 -> must still return ch2 (same top priority), not descend to ch3 | ||
| for i := 0; i < 20; i++ { | ||
| ch, err := GetRandomSatisfiedChannel("default", "gpt-x", "", map[int]bool{1: true}) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if ch == nil || ch.Id != 2 { | ||
| t.Fatalf("expected ch2 (same priority), got %v", ch) | ||
| } | ||
| } | ||
|
|
||
| // exclude ch1+ch2 -> descend to ch3 | ||
| ch, err := GetRandomSatisfiedChannel("default", "gpt-x", "", map[int]bool{1: true, 2: true}) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if ch == nil || ch.Id != 3 { | ||
| t.Fatalf("expected ch3 after top priority exhausted, got %v", ch) | ||
| } | ||
| } | ||
|
|
||
| // TestGetRandomSatisfiedChannel_NilExcludeBackwardCompat verifies that with no | ||
| // exclusion (first attempt), the top priority channel pool is used. | ||
| func TestGetRandomSatisfiedChannel_NilExcludeBackwardCompat(t *testing.T) { | ||
| cleanup := setupChannelCache(t, "default", "gpt-x", [][3]int{ | ||
| {1, 10, 0}, | ||
| {2, 5, 0}, | ||
| }) | ||
| defer cleanup() | ||
|
|
||
| for i := 0; i < 20; i++ { | ||
| ch, err := GetRandomSatisfiedChannel("default", "gpt-x", "", nil) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if ch == nil || ch.Id != 1 { | ||
| t.Fatalf("expected top-priority ch1, got %v", ch) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // TestCountAvailableChannels verifies the adaptive retry budget helper counts | ||
| // every distinct channel that can serve the group/model, across priority tiers. | ||
| // With single-key channels the count equals the number of channels. | ||
| func TestCountAvailableChannels(t *testing.T) { | ||
| cleanup := setupChannelCache(t, "default", "gpt-x", [][3]int{ | ||
| {1, 10, 0}, | ||
| {2, 10, 0}, | ||
| {3, 5, 0}, | ||
| }) | ||
| defer cleanup() | ||
|
|
||
| if n := CountAvailableChannels("default", "gpt-x", ""); n != 3 { | ||
| t.Fatalf("expected 3 available channels, got %d", n) | ||
| } | ||
| if n := CountAvailableChannels("default", "nonexistent", ""); n != 0 { | ||
| t.Fatalf("expected 0 for unknown model, got %d", n) | ||
| } | ||
| if n := CountAvailableChannels("nonexistent", "gpt-x", ""); n != 0 { | ||
| t.Fatalf("expected 0 for unknown group, got %d", n) | ||
| } | ||
| } | ||
|
|
||
| // TestCountEnabledKeys verifies the per-channel enabled-key count used to size | ||
| // how many times a multi-key channel may be re-selected within one request. | ||
| func TestCountEnabledKeys(t *testing.T) { | ||
| single := &Channel{Id: 1, Key: "k1"} | ||
| if n := single.CountEnabledKeys(); n != 1 { | ||
| t.Fatalf("single-key channel: expected 1, got %d", n) | ||
| } | ||
|
|
||
| multi := &Channel{Id: 2, Key: "k1\nk2\nk3"} | ||
| multi.ChannelInfo.IsMultiKey = true | ||
| if n := multi.CountEnabledKeys(); n != 3 { | ||
| t.Fatalf("multi-key all enabled: expected 3, got %d", n) | ||
| } | ||
|
|
||
| // Disable one key via status list -> count drops to 2. | ||
| multi.ChannelInfo.MultiKeyStatusList = map[int]int{1: common.ChannelStatusAutoDisabled} | ||
| if n := multi.CountEnabledKeys(); n != 2 { | ||
| t.Fatalf("multi-key one disabled: expected 2, got %d", n) | ||
| } | ||
|
|
||
| empty := &Channel{Id: 3} | ||
| empty.ChannelInfo.IsMultiKey = true | ||
| if n := empty.CountEnabledKeys(); n != 0 { | ||
| t.Fatalf("multi-key no keys: expected 0, got %d", n) | ||
| } | ||
| } | ||
|
|
||
| // TestCountAvailableChannels_MultiKeyWeighted verifies the retry budget counts a | ||
| // multi-key channel once per enabled key, so every key can be tried before the | ||
| // channel is excluded from retry. | ||
| func TestCountAvailableChannels_MultiKeyWeighted(t *testing.T) { | ||
| cleanup := setupChannelCache(t, "default", "gpt-x", [][3]int{ | ||
| {1, 10, 0}, | ||
| {2, 10, 0}, | ||
| }) | ||
| defer cleanup() | ||
|
|
||
| // Make ch1 a 3-key channel; ch2 stays single-key. Expected budget: 3 + 1 = 4. | ||
| ch1 := channelsIDM[1] | ||
| ch1.Key = "k1\nk2\nk3" | ||
| ch1.ChannelInfo.IsMultiKey = true | ||
|
|
||
| if n := CountAvailableChannels("default", "gpt-x", ""); n != 4 { | ||
| t.Fatalf("expected budget 4 (3 keys + 1), got %d", n) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
New Go tests do not use testify. All three new test files assert with t.Fatal/t.Fatalf only, while the repository requires testify/require for setup and fatal assertions and testify/assert for non-fatal checks.
model/channel_failover_test.go#L50-L210: replace the error and identity checks withrequire.NoErrorandrequire.Equal, and userequire.Nil/require.NotNilfor the channel results.controller/record_channel_failure_test.go#L27-L86: replace the exclusion checks withrequire.True/require.Falseand the counter checks withassert.Equal.controller/should_retry_test.go#L45-L73: replace the retry expectations withrequire.False/require.True, and initialize the retry status-code settings explicitly instead of relying on global defaults.
As per coding guidelines: "New or substantially rewritten Go backend tests must use testify/require for setup and fatal assertions and testify/assert for non-fatal checks; initialize database, request, user, settings, and cache state explicitly".
📍 Affects 3 files
model/channel_failover_test.go#L50-L210(this comment)controller/record_channel_failure_test.go#L27-L86controller/should_retry_test.go#L45-L73
🤖 Prompt for 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.
In `@model/channel_failover_test.go` around lines 50 - 210, Update
model/channel_failover_test.go lines 50-210 to use testify/require for NoError,
Equal, Nil, and NotNil assertions in the channel failover tests. Update
controller/record_channel_failure_test.go lines 27-86 to use require.True/False
for exclusion checks and assert.Equal for counter checks. Update
controller/should_retry_test.go lines 45-73 to use require.False/True for retry
expectations and explicitly initialize retry status-code settings and relevant
test state instead of relying on global defaults.
Source: Coding guidelines
Comment on lines
+127
to
+144
| // IsRateLimited reports whether the failure is a per-key rate-limit signal: an | ||
| // HTTP 429 or any response carrying an upstream Retry-After / X-RateLimit-Reset | ||
| // hint. This is the single source of truth that distinguishes key-level from | ||
| // channel-level failures during retry: | ||
| // | ||
| // - rate-limited (true): only the key just used is throttled; the channel's | ||
| // other keys are likely fine, so retry should rotate to the next key and | ||
| // the key should enter cooldown for the hinted duration. | ||
| // - not rate-limited (false): the failure comes from the channel's upstream | ||
| // backend (e.g. 5xx, auth_unavailable). Every key of that channel faces the | ||
| // same broken backend, so retry should exclude the whole channel and fail | ||
| // over immediately instead of burning the remaining keys. | ||
| func (e *NewAPIError) IsRateLimited() bool { | ||
| if e == nil { | ||
| return false | ||
| } | ||
| return e.StatusCode == http.StatusTooManyRequests || e.RetryAfterSeconds > 0 | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files matching error.go =="
fd 'error\.go$' . | sed 's#^\./##' | sort
echo "== relaykit/types/error.go outline/section =="
if [ -f relaykit/types/error.go ]; then
wc -l relaykit/types/error.go
sed -n '1,180p' relaykit/types/error.go | cat -n
fi
echo "== service/error.go relevant sections =="
if [ -f service/error.go ]; then
wc -l service/error.go
rg -n "RelayErrorHandler|RetryAfterSeconds|Retry-After|X-RateLimit-Reset|StatusCode" service/error.go -C 4
fi
echo "== IsRateLimited usages =="
rg -n "IsRateLimited\(" . -g '*.{go,ts,tsx}' -C 3Repository: QuantumNous/new-api
Length of output: 15600
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ParseRetryAfterSeconds implementation =="
sed -n '88,152p' service/error.go | cat -n
echo "== RelayErrorHandler end and rate-limit guard if present =="
sed -n '154,240p' service/error.go | cat -n | sed -n '/func RelayErrorHandler/,$p'
echo "== controller/relay.go failure fallback and channel exclusion sections =="
sed -n '410,480p' controller/relay.go | cat -n
echo "== static verifier: IsRateLimited status/RetryAfter truth table for representative inputs =="
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`
class NewAPIError:
StatusCode: int
RetryAfterSeconds: int
def is_rate_limited(e):
return e.StatusCode == 429 or e.RetryAfterSeconds > 0
cases = [
# status, retry_after, expected_doc_from_issue
(429, 0, "true: 429"),
(503, 60, "true-current: 5xx with header"),
(503, 0, "false-current: 5xx without header"),
(401, 60, "true-current: auth with header"),
(502, 30, "true-current: 5xx with header"),
]
for status, retry_after, note in cases:
e = NewAPIError(StatusCode=status, RetryAfterSeconds=retry_after)
print(f"{status} retry_after={retry_after} -> IsRateLimited()={is_rate_limited(e)} ; {note}")
PYRepository: QuantumNous/new-api
Length of output: 10026
Restrict IsRateLimited() to 429 responses
IsRateLimited() returns true for any error with RetryAfterSeconds > 0, even when StatusCode is 5xx or another non-429 status. RelayErrorHandler sets this field for any response that carries Retry-After or X-RateLimit-Reset, so HTTP-date headers and timestamp-style X-RateLimit-Reset* values also trigger it. These inputs return true while the value may not be a valid cooldown duration.
🐛 Proposed fix to align with the documented rate-limit contract
func (e *NewAPIError) IsRateLimited() bool {
if e == nil {
return false
}
- return e.StatusCode == http.StatusTooManyRequests || e.RetryAfterSeconds > 0
+ return e.StatusCode == http.StatusTooManyRequests
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // IsRateLimited reports whether the failure is a per-key rate-limit signal: an | |
| // HTTP 429 or any response carrying an upstream Retry-After / X-RateLimit-Reset | |
| // hint. This is the single source of truth that distinguishes key-level from | |
| // channel-level failures during retry: | |
| // | |
| // - rate-limited (true): only the key just used is throttled; the channel's | |
| // other keys are likely fine, so retry should rotate to the next key and | |
| // the key should enter cooldown for the hinted duration. | |
| // - not rate-limited (false): the failure comes from the channel's upstream | |
| // backend (e.g. 5xx, auth_unavailable). Every key of that channel faces the | |
| // same broken backend, so retry should exclude the whole channel and fail | |
| // over immediately instead of burning the remaining keys. | |
| func (e *NewAPIError) IsRateLimited() bool { | |
| if e == nil { | |
| return false | |
| } | |
| return e.StatusCode == http.StatusTooManyRequests || e.RetryAfterSeconds > 0 | |
| } | |
| // IsRateLimited reports whether the failure is a per-key rate-limit signal: an | |
| // HTTP 429 or any response carrying an upstream Retry-After / X-RateLimit-Reset | |
| // hint. This is the single source of truth that distinguishes key-level from | |
| // channel-level failures during retry: | |
| // | |
| // - rate-limited (true): only the key just used is throttled; the channel's | |
| // other keys are likely fine, so retry should rotate to the next key and | |
| // the key should enter cooldown for the hinted duration. | |
| // - not rate-limited (false): the failure comes from the channel's upstream | |
| // backend (e.g. 5xx, auth_unavailable). Every key of that channel faces the | |
| // same broken backend, so retry should exclude the whole channel and fail | |
| // over immediately instead of burning the remaining keys. | |
| func (e *NewAPIError) IsRateLimited() bool { | |
| if e == nil { | |
| return false | |
| } | |
| return e.StatusCode == http.StatusTooManyRequests | |
| } |
🤖 Prompt for 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.
In `@relaykit/types/error.go` around lines 127 - 144, Update
NewAPIError.IsRateLimited to return true only when StatusCode equals
http.StatusTooManyRequests; remove the RetryAfterSeconds condition so retry
hints on 5xx or other responses cannot classify the error as key-level rate
limiting.
14 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Improves relay reliability: exclude-driven channel failover, adaptive retry budget, and a rate-limit cooldown circuit breaker that respects upstream
Retry-Afterhints. Also fixes a data-corruption bug where a retry could append a second response after SSE bytes were already flushed to the client.Fixes the long-standing "no available channel" failures where healthy channels were never tried before the request failed (see #852 — automatic disable of slow channels), plus several related root causes.
Changes
1. Exclude-driven channel failover + adaptive retry
GetRandomSatisfiedChannel/GetChanneltake anexcludeChannelsset; selection is rewritten to be exclude-driven (group non-excluded candidates by priority, pick the highest tier that still has channels, weighted-random within it). Returns(nil,nil)when the pool is exhausted so the caller stops cleanly.AutomaticRetryStatusCodeRangesis authoritative (defaults to full 5xx).max(RetryTimes, availableChannels-1)so every channel can be tried even whenRetryTimesis smaller than the pool. Pinned-channel requests stay at 1.2. Failover tech-debt paydown (multi-key aware)
retryparam,priorityRetryindex, retry-counter manipulation,resetNextTry, deadAutoGroupRetryIndexwrites.GetNextEnabledKey).CountEnabledKeys), so every key of a multi-key channel fits within the adaptive cap.shouldRetryto useretryCapinstead ofcommon.RetryTimes(adaptive cap was silently clamped).3. Rate-limit cooldown circuit breaker
Retry-After/X-RateLimit-Resetheaders, including OpenAI Go-duration formats ("6m0s","1s","88ms"); previouslyParseFloatfailed on these and the precise upstream hint was silently dropped to the default cooldown. Sub-second resets round up to 1s.Retry-After), key-aware; channel selection prefers non-cooling keys/channels with fallback, so cooldown never denies service.processChannelError), so admin channel tests never pollute live rate-limit state.4. Channel-level failure excludes the whole channel
NewAPIError.IsRateLimitedintroduced as the single source of truth (shared with cooldown);recordChannelFailureexcludes the channel immediately on channel-level error, while rate-limits still rotate keys until all are throttled. Applied to both sync relay and async task loops.5. Fix: never retry after response bytes sent to client
shouldRetrynow bails out whenrelayInfo.HasSendResponse()is true. A mid-stream upstream error (e.g. Claude emitting an error event aftermessage_starthas already flushed SSE bytes) could previously trigger a retry that appended a second response onto the partial output on the samehttp.ResponseWriter, corrupting/duplicating what the client saw.Testing
channel_failover_test.go,channel_cooldown_test.go,record_channel_failure_test.go,should_retry_test.go,retry_after_test.go, updatedstatus_code_ranges_test.go.go build,go vet, andgo test ./controller/... ./model/... ./service/... ./setting/...all pass on top of current upstreammain.Summary by CodeRabbit
v1.0.0-rc.21-erdev.2added.