feat: add channel auto reset usage option - #6030
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds coordinated Codex 429 recovery that consumes reset credit and retries buffered requests once. The channel setting is wired through backend and frontend forms, exposed for Codex channels, tested across local and Redis coordination paths, and localized in seven languages. ChangesCodex Auto Reset Usage
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Adaptor
participant CodexUpstream
participant ResetCoordinator
participant ResetCreditService
Client->>Adaptor: Send request
Adaptor->>CodexUpstream: Forward buffered request
CodexUpstream-->>Adaptor: 429 Too Many Requests
Adaptor->>ResetCoordinator: Coordinate reset by snapshot
ResetCoordinator->>ResetCreditService: Check eligibility and consume credit
ResetCreditService-->>ResetCoordinator: Reset result
ResetCoordinator-->>Adaptor: Reset success
Adaptor->>CodexUpstream: Retry request once
CodexUpstream-->>Adaptor: Return response
Adaptor-->>Client: Return final response
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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
relay/channel/codex/adaptor_test.go (1)
22-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider covering the failure/disabled branches too.
This test only exercises the happy path (429 → credit consumed → retry succeeds). The PR also introduces two other behaviorally distinct branches: reset-credit consumption failing (should return the original 429 without a second call) and
CodexAutoResetUsagedisabled (should skip buffering and callDoApiRequestdirectly once). Both are real contract branches worth locking in.Want me to draft these two additional test cases?
🤖 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 `@relay/channel/codex/adaptor_test.go` around lines 22 - 89, Add two tests around Adaptor.DoRequest to cover the new branches in TestCodexAutoResetUsageRetriesAfterRateLimit’s flow: one where the /backend-api/wham/rate-limit-reset-credits/consume call fails and the response request should return the original 429 without retrying, and one where ChannelOtherSettings.CodexAutoResetUsage is false and DoRequest should bypass buffering/reset logic and call DoApiRequest once directly. Use the existing Codex adaptor setup, httptest upstream server, and shared symbols like Adaptor, DoRequest, and DoApiRequest to keep the cases easy to locate.
🤖 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.
Nitpick comments:
In `@relay/channel/codex/adaptor_test.go`:
- Around line 22-89: Add two tests around Adaptor.DoRequest to cover the new
branches in TestCodexAutoResetUsageRetriesAfterRateLimit’s flow: one where the
/backend-api/wham/rate-limit-reset-credits/consume call fails and the response
request should return the original 429 without retrying, and one where
ChannelOtherSettings.CodexAutoResetUsage is false and DoRequest should bypass
buffering/reset logic and call DoApiRequest once directly. Use the existing
Codex adaptor setup, httptest upstream server, and shared symbols like Adaptor,
DoRequest, and DoApiRequest to keep the cases easy to locate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66109299-58ea-405e-bb33-ccb8ac234b78
📒 Files selected for processing (14)
dto/channel_settings.gorelay/channel/codex/adaptor.gorelay/channel/codex/adaptor_test.goweb/default/src/features/channels/components/drawers/channel-mutate-drawer.tsxweb/default/src/features/channels/lib/channel-form-errors.tsweb/default/src/features/channels/lib/channel-form.tsweb/default/src/features/channels/types.tsweb/default/src/i18n/locales/en.jsonweb/default/src/i18n/locales/fr.jsonweb/default/src/i18n/locales/ja.jsonweb/default/src/i18n/locales/ru.jsonweb/default/src/i18n/locales/vi.jsonweb/default/src/i18n/locales/zh-TW.jsonweb/default/src/i18n/locales/zh.json
exherb
changed the title
2ed310c to
bf4272e
Compare
exherb
changed the title
|
@seefs001 有空帮忙 review 下？🙏 |
|
这个功能可以加，但是我倾向于仅针对codex单渠道类型的单个渠道级别的配置，codex渠道仅供个人使用所以不会有很多账号，用户可以手动配置是否允许某个账号自动重置。 |
|
好的，了解，我挪到渠道配置里 |
f2ad4b2 to
1495f01
Compare
exherb
changed the title
1495f01 to
8f62600
Compare
|
@seefs001 改完啦 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/default/src/features/channels/components/drawers/channel-mutate-drawer.tsx (1)
4466-4565: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMove
auto_reset_usage_enabledinto Channel Extra Settings.
It’s currently nested under theMODEL_FETCHABLE_TYPES-gated “Upstream Model Detection Settings” block, so type 57 never shows this switch even thoughextraSettingsConfiguredalready treats it as part of “Channel Extra Settings.” Move it alongsidedisable_task_polling_sleepso the nav item and the actual control line up.🤖 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 `@web/default/src/features/channels/components/drawers/channel-mutate-drawer.tsx` around lines 4466 - 4565, The auto_reset_usage_enabled toggle is incorrectly placed inside the MODEL_FETCHABLE_TYPES-gated Upstream Model Detection Settings section, which hides it for type 57 and mismatches the Channel Extra Settings nav. Move the FormField for auto_reset_usage_enabled out of the upstream model block in channel-mutate-drawer.tsx and render it with the other extra settings, alongside disable_task_polling_sleep, so extraSettingsConfigured and the visible control stay aligned.
🧹 Nitpick comments (1)
web/default/src/features/channels/lib/channel-form.ts (1)
563-588: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating the new type-57 branch with the existing one.
The
auto_reset_usage_enabledset/delete logic (lines 583-588) duplicates the sameformData.type === 57condition already checked at line 563 fordisable_store/allow_safety_identifier/etc. Merging it into that existing block would avoid a second identical type check.♻️ Suggested consolidation
if (formData.type === 1 || formData.type === 57) { settingsObj.disable_store = formData.disable_store === true settingsObj.allow_safety_identifier = formData.allow_safety_identifier === true settingsObj.allow_include_obfuscation = formData.allow_include_obfuscation === true settingsObj.allow_inference_geo = formData.allow_inference_geo === true + if (formData.type === 57) { + settingsObj.auto_reset_usage_enabled = + formData.auto_reset_usage_enabled === true + } } else { if ('disable_store' in settingsObj) delete settingsObj.disable_store ... + if ('auto_reset_usage_enabled' in settingsObj) { + delete settingsObj.auto_reset_usage_enabled + } } - - if (formData.type === 57) { - settingsObj.auto_reset_usage_enabled = - formData.auto_reset_usage_enabled === true - } else if ('auto_reset_usage_enabled' in settingsObj) { - delete settingsObj.auto_reset_usage_enabled - }🤖 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 `@web/default/src/features/channels/lib/channel-form.ts` around lines 563 - 588, Consolidate the duplicated type-57 handling in channel-form’s settings update logic: the `auto_reset_usage_enabled` set/delete branch should be folded into the existing `formData.type === 1 || formData.type === 57` block instead of checking `formData.type === 57` again. Update the `settingsObj` mutation logic in that section so all type-57-specific fields are handled together and the fallback deletion path remains in the `else` branch.
🤖 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.
Outside diff comments:
In
`@web/default/src/features/channels/components/drawers/channel-mutate-drawer.tsx`:
- Around line 4466-4565: The auto_reset_usage_enabled toggle is incorrectly
placed inside the MODEL_FETCHABLE_TYPES-gated Upstream Model Detection Settings
section, which hides it for type 57 and mismatches the Channel Extra Settings
nav. Move the FormField for auto_reset_usage_enabled out of the upstream model
block in channel-mutate-drawer.tsx and render it with the other extra settings,
alongside disable_task_polling_sleep, so extraSettingsConfigured and the visible
control stay aligned.
---
Nitpick comments:
In `@web/default/src/features/channels/lib/channel-form.ts`:
- Around line 563-588: Consolidate the duplicated type-57 handling in
channel-form’s settings update logic: the `auto_reset_usage_enabled` set/delete
branch should be folded into the existing `formData.type === 1 || formData.type
=== 57` block instead of checking `formData.type === 57` again. Update the
`settingsObj` mutation logic in that section so all type-57-specific fields are
handled together and the fallback deletion path remains in the `else` branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b2f265c-6a9b-4af7-aa77-98568be8321b
📒 Files selected for processing (14)
dto/channel_settings.gorelay/channel/codex/adaptor.gorelay/channel/codex/adaptor_test.goweb/default/src/features/channels/components/drawers/channel-mutate-drawer.tsxweb/default/src/features/channels/lib/channel-form-errors.tsweb/default/src/features/channels/lib/channel-form.tsweb/default/src/features/channels/types.tsweb/default/src/i18n/locales/en.jsonweb/default/src/i18n/locales/fr.jsonweb/default/src/i18n/locales/ja.jsonweb/default/src/i18n/locales/ru.jsonweb/default/src/i18n/locales/vi.jsonweb/default/src/i18n/locales/zh-TW.jsonweb/default/src/i18n/locales/zh.json
✅ Files skipped from review due to trivial changes (10)
- web/default/src/features/channels/types.ts
- web/default/src/i18n/locales/fr.json
- dto/channel_settings.go
- web/default/src/features/channels/lib/channel-form-errors.ts
- web/default/src/i18n/locales/ja.json
- web/default/src/i18n/locales/zh.json
- web/default/src/i18n/locales/zh-TW.json
- web/default/src/i18n/locales/ru.json
- web/default/src/i18n/locales/vi.json
- web/default/src/i18n/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
- relay/channel/codex/adaptor_test.go
- relay/channel/codex/adaptor.go
|
@seefs001 再帮忙看看还有啥问题吧？ |
|
缺少一些关键的部分 不能只看429，需要看还有没有重置次数，需要看周额度是否被用完，调用时需要有ctx timeout管控，需要做并发管控。 ui展示也是有问题的，看代码是展示不出来设置项的。 |
8f62600 to
27fd63f
Compare
@seefs001 幸苦再抽空给看看 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
relay/channel/codex/adaptor.go (1)
172-207: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftAvoid the Redis lookup on the non-429 path
relay/channel/codex/adaptor.go:189,503-517
codexAutoResetSnapshotdoes a RedisGETbefore every auto-reset-enabled request, even when the upstream call succeeds and never needs a retry. A local fast path/cache here would remove that round trip from the hot path.🤖 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 `@relay/channel/codex/adaptor.go` around lines 172 - 207, The auto-reset flow performs the Redis-backed codexAutoResetSnapshot lookup before knowing whether a retry is needed. Defer or locally cache that snapshot so the initial channel.DoApiRequest path avoids the Redis GET, while still obtaining the correct snapshot before consumeCodexResetCredit when the response is HTTP 429 and preserving existing retry behavior.relay/channel/codex/adaptor_test.go (3)
676-704: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winExact TTL equality is timing-sensitive.
assert.Equal(t, codexAutoResetLockTTL, redisServer.TTL(lockHook.lockKey))(line 701) compares the lock's remaining TTL to the full lock-TTL constant.miniredis.TTL()returns the actual remaining duration computed from real elapsed time since theSET ... PXcall, so any scheduling delay between acquiring the lock and this read can make the assertion intermittently fail. Prefer bounding it instead of exact equality.♻️ Proposed fix
- assert.Equal(t, codexAutoResetLockTTL, redisServer.TTL(lockHook.lockKey)) + ttl := redisServer.TTL(lockHook.lockKey) + assert.Greater(t, ttl, time.Duration(0)) + assert.LessOrEqual(t, ttl, codexAutoResetLockTTL)Please confirm miniredis'
TTL()resolution/precision behavior to validate this flakiness risk.🤖 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 `@relay/channel/codex/adaptor_test.go` around lines 676 - 704, Replace the exact TTL assertion in the runCodexAutoReset test with a bounded check that accepts a slightly reduced remaining TTL while still verifying the lock was created with the expected codexAutoResetLockTTL. Keep the existing lock-key lookup and subsequent state assertions unchanged.
332-377: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWall-clock assertion risks CI flakiness.
assert.Less(t, time.Since(started), 250*time.Millisecond)(line 373) pins pass/fail to real elapsed time, which is the kind of timing-dependent test the repo guideline calls out to avoid — it can fail under CI load unrelated to any real regression. The functional assertions already present (usageCalls == 1,resetCalls == 0) capture the actual timeout behavior; consider dropping or substantially loosening the duration check.♻️ Proposed fix
- assert.Less(t, time.Since(started), 250*time.Millisecond) assert.Equal(t, http.StatusTooManyRequests, resp.StatusCode) assert.Equal(t, int32(1), usageCalls.Load()) assert.Zero(t, resetCalls.Load())As per coding guidelines: "avoid coverage-only, implementation-detail, duplicate, fake fuzz/stress, timing, and log-only tests."
🤖 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 `@relay/channel/codex/adaptor_test.go` around lines 332 - 377, Remove the wall-clock assertion on time.Since(started) from TestAutoResetUsageHasTimeout, since it introduces CI-dependent flakiness and duplicates the functional timeout checks. Keep the existing usageCalls, resetCalls, and response-status assertions that verify the intended behavior.Source: Coding guidelines
379-448: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBlocking channel receives have no timeout guard.
Several concurrency tests block on unbuffered channels with no deadline:
<-bothInitial(lines 404, 410),<-secondInitial/<-resetComplete(lines 475, 481),<-continueUsage(line 586),<-consumeStarted/<-lockHook.secondAttempt(lines 700, 706). If the coordination behavior under test regresses (e.g. the leader never reaches the expected call), these tests hang until the wholego testbinary times out instead of failing fast with a clear message, stalling CI. Wrapping these waits in aselectwith a short timer (orrequire.Eventually) would fail quickly with a clear diagnostic on regression.Also applies to: 450-527, 562-644, 646-723
🤖 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 `@relay/channel/codex/adaptor_test.go` around lines 379 - 448, Update the affected concurrency tests, including TestAutoResetUsageCoalescesConcurrentResets and the scenarios around secondInitial, resetComplete, continueUsage, consumeStarted, and lockHook.secondAttempt, so every blocking channel receive has a bounded timeout or require.Eventually assertion. Preserve the existing synchronization behavior while failing promptly with a clear test diagnostic when the expected signal is never delivered.
🤖 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 `@relay/channel/codex/adaptor.go`:
- Around line 296-299: Update the ineligibility/error branch around
checkCodexAutoResetEligibility to record the same local and Codex cooldown used
by the reset-failure paths before returning. Preserve the existing
codexAutoResetResult values and eligibilityErr, and apply this behavior to the
corresponding branch noted around the later eligibility check as well.
In `@web/default/src/i18n/locales/zh-TW.json`:
- Line 4881: Update the Traditional Chinese translation for the rate-limit
reset-credit message in the locale entry so its scope explicitly means the
behavior applies only to supported channels, using wording equivalent to
“僅對受支援的渠道生效” while preserving the rest of the translation.
---
Nitpick comments:
In `@relay/channel/codex/adaptor_test.go`:
- Around line 676-704: Replace the exact TTL assertion in the runCodexAutoReset
test with a bounded check that accepts a slightly reduced remaining TTL while
still verifying the lock was created with the expected codexAutoResetLockTTL.
Keep the existing lock-key lookup and subsequent state assertions unchanged.
- Around line 332-377: Remove the wall-clock assertion on time.Since(started)
from TestAutoResetUsageHasTimeout, since it introduces CI-dependent flakiness
and duplicates the functional timeout checks. Keep the existing usageCalls,
resetCalls, and response-status assertions that verify the intended behavior.
- Around line 379-448: Update the affected concurrency tests, including
TestAutoResetUsageCoalescesConcurrentResets and the scenarios around
secondInitial, resetComplete, continueUsage, consumeStarted, and
lockHook.secondAttempt, so every blocking channel receive has a bounded timeout
or require.Eventually assertion. Preserve the existing synchronization behavior
while failing promptly with a clear test diagnostic when the expected signal is
never delivered.
In `@relay/channel/codex/adaptor.go`:
- Around line 172-207: The auto-reset flow performs the Redis-backed
codexAutoResetSnapshot lookup before knowing whether a retry is needed. Defer or
locally cache that snapshot so the initial channel.DoApiRequest path avoids the
Redis GET, while still obtaining the correct snapshot before
consumeCodexResetCredit when the response is HTTP 429 and preserving existing
retry behavior.
🪄 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: 1d481ef0-5efb-4e14-a307-3bf7a12681ed
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
dto/channel_settings.gogo.modrelay/channel/codex/adaptor.gorelay/channel/codex/adaptor_test.goweb/default/src/features/channels/components/drawers/channel-mutate-drawer.tsxweb/default/src/features/channels/lib/channel-form-errors.tsweb/default/src/features/channels/lib/channel-form.tsweb/default/src/features/channels/types.tsweb/default/src/i18n/locales/en.jsonweb/default/src/i18n/locales/fr.jsonweb/default/src/i18n/locales/ja.jsonweb/default/src/i18n/locales/ru.jsonweb/default/src/i18n/locales/vi.jsonweb/default/src/i18n/locales/zh-TW.jsonweb/default/src/i18n/locales/zh.json
🚧 Files skipped from review as they are similar to previous changes (10)
- web/default/src/features/channels/types.ts
- web/default/src/features/channels/lib/channel-form-errors.ts
- dto/channel_settings.go
- web/default/src/i18n/locales/ru.json
- web/default/src/i18n/locales/fr.json
- web/default/src/i18n/locales/ja.json
- web/default/src/i18n/locales/vi.json
- web/default/src/i18n/locales/zh.json
- web/default/src/features/channels/lib/channel-form.ts
- web/default/src/features/channels/components/drawers/channel-mutate-drawer.tsx
27fd63f to
ea458c6
Compare
ea458c6 to
ef49d01
Compare
|
@seefs001 抽空帮忙再看看，感谢。 |
|
@Calcium-Ion 有空看下这个 pull request 么？ |
|
太难了，先关闭了 @seefs001 |
2 participants
Important
📝 变更描述 / Description
在渠道额外设置
ChannelOtherSettings中新增通用配置auto_reset_usage_enabled。渠道编辑抽屉只对 ChatGPT Subscription (Codex，type 57) 展示Auto reset usage开关；其他渠道保存时会移除该字段，尚未实现自动重置的适配器不受影响。启用后，Codex adapter 仅在首次上游请求返回
429时进入自动重置流程。流程会在受限 context 内读取 WHAM usage 和 reset credits，确认周额度已耗尽且available_count > 0后才调用 consume；只有响应确认windows_reset > 0，才复用请求体重试一次。资格不足、调用失败、超时或结果未确认时均保留原始429。同一进程使用 singleflight 合并请求；Redis 可用时再使用按渠道地址和账号隔离的
SET NX锁，锁 TTL 为 15 分钟，避免并发或紧随其后的请求重复消耗 reset credit。Redis 未启用或锁调用失败时退化为进程内 singleflight。🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
Summary by CodeRabbit
New Features
Bug Fixes
Tests