feat: customizable automatic retry status codes - #2663
Conversation
WalkthroughThis PR introduces configurable automatic retry status codes functionality that allows operators to define HTTP status code ranges that should trigger retry logic. The feature extends existing status code range parsing infrastructure with new retry-specific handling, wires configuration through controller options, and adds UI components for managing these settings. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Interface
participant Controller as Controller
participant OptionMap as Option Map
participant Relay as Relay Handler
User->>Controller: POST /api/option (AutomaticRetryStatusCodes)
Controller->>OptionMap: Parse and store AutomaticRetryStatusCodes
OptionMap->>OptionMap: Update AutomaticRetryStatusCodeRanges
Controller-->>User: Success response
Note over Relay: Request arrives with error
Relay->>Relay: Extract HTTP status code
Relay->>Relay: Call ShouldRetryByStatusCode(code)
Relay->>Relay: Match code against AutomaticRetryStatusCodeRanges
alt Code matches configured range
Relay->>Relay: Retry = true
else Code does not match
Relay->>Relay: Retry = false
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/settings/OperationSetting.jsx (1)
33-80: Ensure thesetInputs()merge strategy preserves defaults when API response is incomplete.The backend currently returns
AutomaticRetryStatusCodesvia/api/option/, but the frontend'ssetInputs(newInputs)approach (line 97) replaces the entire state, causing any missing keys from the response to becomeundefined. While the backend initializes this key in the OptionMap and includes it in the API response, this implementation is fragile to future changes or partial responses.The proposed defensive fix is sound:
Recommended change (merge defaults on refresh)
- setInputs(newInputs); + setInputs((prev) => ({ ...prev, ...newInputs }));This preserves frontend defaults if the backend ever omits a key during updates or migrations.
Also applies to: 84-100
🤖 Fix all issues with AI agents
In `@setting/operation_setting/status_code_ranges_test.go`:
- Around line 54-79: Tests mutate the global AutomaticRetryStatusCodeRanges
which will race if tests run in parallel; protect access by introducing a
package-level sync.RWMutex (e.g., automaticRetryMu) and update reads/writes:
wrap reads in ShouldRetryByStatusCode with a RLock/RUnlock and wrap assignments
(including in tests) with Lock/Unlock, or alternatively stop mutating the global
by providing a setter/getter that uses the mutex and update the tests to call
the setter and restore original via the getter; this ensures safe concurrent
access to AutomaticRetryStatusCodeRanges without relying on test ordering.
In `@setting/operation_setting/status_code_ranges.go`:
- Around line 17-27: AutomaticRetryStatusCodeRanges and
AutomaticDisableStatusCodeRanges are exported mutable globals causing a data
race and a broken sortedness invariant; add a package-level sync.RWMutex (e.g.,
statusCodeRangesMu) and use it to protect all reads/writes of these globals
(acquire write lock in SetAutomaticRetryStatusCodes and
SetAutomaticDisableStatusCodes, acquire read lock in ShouldDisableByStatusCode
and any readers). Also ensure setters validate/sort the slices (or store them in
a normalized form) so the sortedness invariant holds, and make
shouldMatchStatusCodeRanges robust to unsorted input by either sorting a copy
under the lock or by scanning all ranges instead of bailing early when code <
r.Start; update the SetAutomaticRetryStatusCodes/SetAutomaticDisableStatusCodes
and shouldMatchStatusCodeRanges implementations accordingly.
In `@web/src/components/settings/HttpStatusCodeRulesInput.jsx`:
- Around line 60-67: The current conditional {!parsed?.ok && (...)} shows errors
when parsed is null/undefined; update the guard to only render the error block
when parsed is present and indicates a real validation failure (for example:
parsed != null && parsed.ok === false) and ensure you access
parsed.invalidTokens safely (e.g., parsed.invalidTokens?.length) so
invalidText/invalidTokens are only shown when parsed exists and has invalid
tokens; change the conditional around the Text component (and any uses of
parsed.invalidTokens) accordingly.
🧹 Nitpick comments (1)
setting/operation_setting/status_code_ranges.go (1)
50-57: Recommend copying before publishing parsed ranges to avoid accidental external mutation.Even though
ParseHTTPStatusCodeRangesreturns a fresh slice today, copying on assignment makes the “treat as immutable” intent clearer and safer.Proposed change
func AutomaticRetryStatusCodesFromString(s string) error { ranges, err := ParseHTTPStatusCodeRanges(s) if err != nil { return err } - AutomaticRetryStatusCodeRanges = ranges + AutomaticRetryStatusCodeRanges = append([]StatusCodeRange(nil), ranges...) return nil }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
controller/option.gocontroller/relay.gomodel/option.gosetting/operation_setting/status_code_ranges.gosetting/operation_setting/status_code_ranges_test.goweb/src/components/settings/HttpStatusCodeRulesInput.jsxweb/src/components/settings/OperationSetting.jsxweb/src/i18n/locales/en.jsonweb/src/i18n/locales/zh.jsonweb/src/pages/Setting/Operation/SettingsMonitoring.jsx
🧰 Additional context used
🧬 Code graph analysis (5)
model/option.go (2)
common/constants.go (1)
OptionMap(37-37)setting/operation_setting/status_code_ranges.go (2)
AutomaticRetryStatusCodesToString(46-48)AutomaticRetryStatusCodesFromString(50-57)
setting/operation_setting/status_code_ranges_test.go (1)
setting/operation_setting/status_code_ranges.go (3)
AutomaticRetryStatusCodeRanges(19-27)StatusCodeRange(10-13)ShouldRetryByStatusCode(59-61)
controller/relay.go (1)
setting/operation_setting/status_code_ranges.go (1)
ShouldRetryByStatusCode(59-61)
web/src/pages/Setting/Operation/SettingsMonitoring.jsx (3)
web/src/helpers/statusCodeRules.js (1)
parseHttpStatusCodeRules(1-48)web/src/helpers/utils.jsx (1)
showError(122-151)web/src/components/settings/HttpStatusCodeRulesInput.jsx (1)
HttpStatusCodeRulesInput(23-70)
controller/option.go (1)
setting/operation_setting/status_code_ranges.go (1)
ParseHTTPStatusCodeRanges(93-145)
🔇 Additional comments (9)
web/src/i18n/locales/zh.json (1)
1914-1915: Translations for auto-retry status codes look consistent with the existing auto-disable wording.web/src/i18n/locales/en.json (1)
1928-1929: New en locale entries align with the new UI labels/errors.model/option.go (1)
147-147: LGTM!The new
AutomaticRetryStatusCodesoption is properly integrated into bothInitOptionMapandupdateOptionMap, following the same pattern as the existingAutomaticDisableStatusCodes. Error handling is correctly propagated.Also applies to: 451-452
controller/option.go (1)
190-198: LGTM!The validation case for
AutomaticRetryStatusCodescorrectly mirrors the existingAutomaticDisableStatusCodeshandling. Input is validated withParseHTTPStatusCodeRangesbefore persisting, ensuring invalid configurations are rejected early.web/src/pages/Setting/Operation/SettingsMonitoring.jsx (4)
49-49: LGTM!The default retry status codes are well-chosen. Notably excluding
400(Bad Request),408(Azure timeout),504/524(gateway timeouts) aligns with the previous hardcoded retry logic that explicitly skipped these codes.
58-60: LGTM!Parsing and validation logic for
AutomaticRetryStatusCodescorrectly mirrors the existingAutomaticDisableStatusCodeshandling, ensuring consistent error reporting before submission.Also applies to: 73-80
86-90: LGTM!The
normalizedMapapproach cleanly handles normalized value substitution for both status code fields, avoiding code duplication.
258-270: LGTM!The
HttpStatusCodeRulesInputcomponent is properly wired for the retry status codes with appropriate labels, placeholders, and validation feedback.controller/relay.go (1)
320-327: LGTM!The refactored
shouldRetrylogic is cleaner and correctly delegates to the configurableShouldRetryByStatusCodehelper. The guards for 2xx (success) and invalid status codes (<100 or >599) are sensible defaults.Note: The
shouldRetryTaskRelayfunction uses more granular hardcoded status code checks (including specific handling for timeouts like 504/524, 408 for Azure, and 307 redirects) compared to the new delegated approach inshouldRetry. Confirm whether these different semantics are intentional based on the distinct retry requirements for task relay operations.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| func TestShouldRetryByStatusCode(t *testing.T) { | ||
| orig := AutomaticRetryStatusCodeRanges | ||
| t.Cleanup(func() { AutomaticRetryStatusCodeRanges = orig }) | ||
|
|
||
| AutomaticRetryStatusCodeRanges = []StatusCodeRange{ | ||
| {Start: 429, End: 429}, | ||
| {Start: 500, End: 599}, | ||
| } | ||
|
|
||
| require.True(t, ShouldRetryByStatusCode(429)) | ||
| require.True(t, ShouldRetryByStatusCode(500)) | ||
| require.False(t, ShouldRetryByStatusCode(400)) | ||
| require.False(t, ShouldRetryByStatusCode(200)) | ||
| } | ||
|
|
||
| func TestShouldRetryByStatusCode_DefaultMatchesLegacyBehavior(t *testing.T) { | ||
| require.False(t, ShouldRetryByStatusCode(200)) | ||
| require.False(t, ShouldRetryByStatusCode(400)) | ||
| require.True(t, ShouldRetryByStatusCode(401)) | ||
| require.False(t, ShouldRetryByStatusCode(408)) | ||
| require.True(t, ShouldRetryByStatusCode(429)) | ||
| require.True(t, ShouldRetryByStatusCode(500)) | ||
| require.False(t, ShouldRetryByStatusCode(504)) | ||
| require.False(t, ShouldRetryByStatusCode(524)) | ||
| require.True(t, ShouldRetryByStatusCode(599)) | ||
| } |
There was a problem hiding this comment.
Global mutation in tests can become flaky if package tests are (now or later) run in parallel.
Consider either:
- documenting “do not use
t.Parallel()in this package/file because of global config”, or - protecting
AutomaticRetryStatusCodeRangeswith a package-level mutex and using it in tests + production.
🤖 Prompt for AI Agents
In `@setting/operation_setting/status_code_ranges_test.go` around lines 54 - 79,
Tests mutate the global AutomaticRetryStatusCodeRanges which will race if tests
run in parallel; protect access by introducing a package-level sync.RWMutex
(e.g., automaticRetryMu) and update reads/writes: wrap reads in
ShouldRetryByStatusCode with a RLock/RUnlock and wrap assignments (including in
tests) with Lock/Unlock, or alternatively stop mutating the global by providing
a setter/getter that uses the mutex and update the tests to call the setter and
restore original via the getter; this ensures safe concurrent access to
AutomaticRetryStatusCodeRanges without relying on test ordering.
| // Default behavior matches legacy hardcoded retry rules in controller/relay.go shouldRetry: | ||
| // retry for 1xx, 3xx, 4xx(except 400/408), 5xx(except 504/524), and no retry for 2xx. | ||
| var AutomaticRetryStatusCodeRanges = []StatusCodeRange{ | ||
| {Start: 100, End: 199}, | ||
| {Start: 300, End: 399}, | ||
| {Start: 401, End: 407}, | ||
| {Start: 409, End: 499}, | ||
| {Start: 500, End: 503}, | ||
| {Start: 505, End: 523}, | ||
| {Start: 525, End: 599}, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file in question
fd -type f status_code_ranges.goRepository: QuantumNous/new-api
Length of output: 233
🏁 Script executed:
# Search for usage of AutomaticRetryStatusCodeRanges and AutomaticDisableStatusCodeRanges
rg "AutomaticRetryStatusCodeRanges|AutomaticDisableStatusCodeRanges" --type go -B 2 -A 2Repository: QuantumNous/new-api
Length of output: 4279
🏁 Script executed:
# Search for operation settings updates and locking mechanisms
rg "operation.*[Ss]etting" --type go | head -30Repository: QuantumNous/new-api
Length of output: 2793
🏁 Script executed:
# Look for "shouldRetry" and references to controller/relay.go
rg "shouldRetry|controller/relay" --type go -B 2 -A 2 | head -50Repository: QuantumNous/new-api
Length of output: 1870
🏁 Script executed:
# Check if there's a config or settings struct with locks/atomics
rg "sync\.(Mutex|RWMutex|Atomic)" --type go -B 2 -A 2 | head -40Repository: QuantumNous/new-api
Length of output: 2242
🏁 Script executed:
# Read the complete status_code_ranges.go file
cat setting/operation_setting/status_code_ranges.goRepository: QuantumNous/new-api
Length of output: 4573
🏁 Script executed:
# Check for any locking mechanism specific to operation_setting package
rg "Mutex" setting/operation_setting/ --type goRepository: QuantumNous/new-api
Length of output: 45
Major: data race and sortedness invariant in global mutable status code ranges.
-
Data race:
AutomaticRetryStatusCodeRangesandAutomaticDisableStatusCodeRangesare unprotected exported globals. They are read in request paths (e.g.,service/channel.gocallsShouldDisableByStatusCode) and written viaSetAutomaticRetryStatusCodes/SetAutomaticDisableStatusCodes. Concurrent reads/writes are a Go data race. Other operation settings modules (e.g.,setting/ratio_setting/model_ratio.go,setting/user_usable_group.go) consistently protect mutable globals withsync.RWMutex. -
Sortedness invariant:
shouldMatchStatusCodeRangesrelies on sorted ranges—it returnsfalsewhencode < r.Start, skipping later ranges.ParseHTTPStatusCodeRangesenforces sorting, but the globals can be assigned directly (as tests do), bypassing this guarantee and silently breaking the matching logic.
Fix: Add sync.RWMutex around both globals (consistent with other settings), and make shouldMatchStatusCodeRanges order-independent:
Proposed changes
-func shouldMatchStatusCodeRanges(ranges []StatusCodeRange, code int) bool {
+func shouldMatchStatusCodeRanges(ranges []StatusCodeRange, code int) bool {
if code < 100 || code > 599 {
return false
}
for _, r := range ranges {
- if code < r.Start {
- return false
- }
- if code <= r.End {
+ if code >= r.Start && code <= r.End {
return true
}
}
return false
}Also applies to: 35-52 (update functions), 74-84 (shouldMatchStatusCodeRanges)
🤖 Prompt for AI Agents
In `@setting/operation_setting/status_code_ranges.go` around lines 17 - 27,
AutomaticRetryStatusCodeRanges and AutomaticDisableStatusCodeRanges are exported
mutable globals causing a data race and a broken sortedness invariant; add a
package-level sync.RWMutex (e.g., statusCodeRangesMu) and use it to protect all
reads/writes of these globals (acquire write lock in
SetAutomaticRetryStatusCodes and SetAutomaticDisableStatusCodes, acquire read
lock in ShouldDisableByStatusCode and any readers). Also ensure setters
validate/sort the slices (or store them in a normalized form) so the sortedness
invariant holds, and make shouldMatchStatusCodeRanges robust to unsorted input
by either sorting a copy under the lock or by scanning all ranges instead of
bailing early when code < r.Start; update the
SetAutomaticRetryStatusCodes/SetAutomaticDisableStatusCodes and
shouldMatchStatusCodeRanges implementations accordingly.
| {!parsed?.ok && ( | ||
| <Text type='danger' style={{ display: 'block', marginTop: 8 }}> | ||
| {invalidText} | ||
| {parsed?.invalidTokens && parsed.invalidTokens.length > 0 | ||
| ? `: ${parsed.invalidTokens.join(', ')}` | ||
| : ''} | ||
| </Text> | ||
| )} |
There was a problem hiding this comment.
Guard against undefined parsed prop to avoid false error display.
When parsed is undefined or null (e.g., before initial parsing), !parsed?.ok evaluates to true, which will display the error text even though no actual validation error occurred.
🐛 Proposed fix
- {!parsed?.ok && (
+ {parsed && !parsed.ok && (
<Text type='danger' style={{ display: 'block', marginTop: 8 }}>
{invalidText}
{parsed?.invalidTokens && parsed.invalidTokens.length > 0
? `: ${parsed.invalidTokens.join(', ')}`
: ''}
</Text>
)}📝 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.
| {!parsed?.ok && ( | |
| <Text type='danger' style={{ display: 'block', marginTop: 8 }}> | |
| {invalidText} | |
| {parsed?.invalidTokens && parsed.invalidTokens.length > 0 | |
| ? `: ${parsed.invalidTokens.join(', ')}` | |
| : ''} | |
| </Text> | |
| )} | |
| {parsed && !parsed.ok && ( | |
| <Text type='danger' style={{ display: 'block', marginTop: 8 }}> | |
| {invalidText} | |
| {parsed?.invalidTokens && parsed.invalidTokens.length > 0 | |
| ? `: ${parsed.invalidTokens.join(', ')}` | |
| : ''} | |
| </Text> | |
| )} |
🤖 Prompt for AI Agents
In `@web/src/components/settings/HttpStatusCodeRulesInput.jsx` around lines 60 -
67, The current conditional {!parsed?.ok && (...)} shows errors when parsed is
null/undefined; update the guard to only render the error block when parsed is
present and indicates a real validation failure (for example: parsed != null &&
parsed.ok === false) and ensure you access parsed.invalidTokens safely (e.g.,
parsed.invalidTokens?.length) so invalidText/invalidTokens are only shown when
parsed exists and has invalid tokens; change the conditional around the Text
component (and any uses of parsed.invalidTokens) accordingly.
…s-code feat: customizable automatic retry status codes
2 participants
#2659
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.