fix(rotation): persist rate-limit window in short-retry 429 path - #609
Conversation
The short-retry branch of the runtime fetch loop in index.ts marks the account rate-limited via `markRateLimitedWithReason` (which mutates the disk-serialized `rateLimitResetTimes`) and then sleeps + retries, but never called `saveToDiskDebounced()` — unlike the sibling full-rotation branch directly below it, which persists at line ~2327. A crash during the retry sleep (or before any later save) lost the rate-limit reset time; on restart the account was immediately re-selected, defeating the cooldown. This is the same durability gap class as PR #608 (runtime-rotation-proxy.ts) and PR #607, in a third location. Add the missing `saveToDiskDebounced()` after `recordRateLimit()` in the short-retry branch, mirroring the full-rotation branch. Found by a pre-release deep stress-test sweep. Regression test drives a 429 with a sub-threshold cooldown into the short-retry path and asserts the save is scheduled; it fails without the fix (verified by mutation). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary: This PR fixes a data-loss durability gap in the rate-limit persistence logic for the short-retry (429 with sub-threshold cooldown) path in Changes:
Risk Assessment: Low-risk, internal consistency fix. Regression test is included and all 4,920 tests pass. No public API or architecture changes. Classified as major severity due to data-loss risk, but minimal in scope and implementation. Walkthroughin the short-cooldown 429 retry path of the oauth plugin, ChangesShort 429 retry disk persistence
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. 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 |
Promote the 2.3.0-beta line to stable and ship three runtime-rotation durability fixes landed after beta.3: - #607: break stale-recovery deadlock on persisted transient account state (fixes #606) - #608: persist cooldown when an account has no resolvable accountId - #609: persist rate-limit window in the short-retry 429 path Version-coupled manifests bumped 2.3.0-beta.3 -> 2.3.0 (package.json, package-lock.json, .codex-plugin/plugin.json, AGENTS.md), release portal links updated in README.md and docs/README.md (v2.3.0 current stable, beta.3/beta.2 demoted to prior prerelease), CHANGELOG entry added, and docs/releases/v2.3.0.md created. documentation.test.ts coupling assertions green. Full suite 4920 pass / 3 skip / 0 fail; lint + tsc clean; pack budget ok (codex-multi-auth@2.3.0, 1062597 bytes / 1201 files). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
index.ts.markRateLimitedWithReason(which mutates the disk-serializedrateLimitResetTimes) andrecordRateLimit, then sleeps and retries — but never calledsaveToDiskDebounced(). The sibling full-rotation branch directly below it does persist (line ~2327).What Changed
index.ts— addaccountManager.saveToDiskDebounced()afterrecordRateLimit()in the short-retry branch, mirroring the full-rotation branch. One line + explanatory comment.test/index.test.ts— regression test in the fetch-handler suite: a 429 with a sub-threshold cooldown drives the short-retry path, the retry returns 200, and the test assertssaveToDiskDebouncedwas called once. Fails without the fix (verified by mutation).Severity
Low–medium. Like #608, this is a durability gap, not a live crash — the in-memory cooldown still works within a process. It only manifests if the process restarts inside the retry window. Fixing it makes the short-retry branch consistent with the full-rotation branch and the #607/#608 precedent.
Validation
npm run lintnpm run typechecknpm test(4920 passed, 3 skipped, 0 failures)npm test -- test/documentation.test.tsnpm run buildDocs and Governance Checklist
docs/getting-started.mdupdated (if onboarding flow changed)docs/features.mdupdated (if capability surface changed)docs/reference/*pages updated (if commands/settings/paths changed)docs/upgrade.mdupdated (if migration behavior changed)SECURITY.mdandCONTRIBUTING.mdreviewed for alignmentNo user-visible surface changed — internal fetch-loop consistency fix.
Risk and Rollback
🤖 Generated with Claude Code
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
adds
saveToDiskDebounced()to the short-retry 429 branch inindex.ts, closing a durability gap where a crash during the retry sleep could lose the rate-limit window and allow the account to be immediately re-selected on restart.index.ts: one-line fix placed afterrecordRateLimit()in the short-retry branch, mirroring the identical call sequence already present in the full-rotation branch at line ~2333.test/index.test.ts: regression test drives the short-retry path with a 1000 ms cooldown, lets it succeed on the second fetch, and assertssaveToDiskDebouncedwas called exactly once.Confidence Score: 4/5
safe to merge — one-line change on a well-understood path, mirrors the sibling branch exactly, and is covered by a targeted regression test.
the fix is minimal and correct. the only observation is that the new regression test (and its sibling) sleep ~1 second in real time because
sleepis not mocked, making the suite slightly slower and non-deterministic under jitter. no functional or correctness issues found.test/index.test.ts — the new test and its sibling at line 5470 both incur a real ~1 s sleep; fake timers would align them with the timer-controlled tests already present in the same describe block.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[fetch loop: 429 response] --> B{cooldownMs <= shortRetryThreshold\nAND retryCount < MAX_SHORT_RETRY?} B -- yes: short-retry branch --> C[markRateLimitedWithReason] C --> D[recordRateLimit] D --> E["saveToDiskDebounced() ✅ NEW"] E --> F[sleep + continue] F --> A B -- no: full-rotation branch --> G[markRateLimitedWithReason] G --> H[recordRateLimit] H --> I[saveToDiskDebounced existing] I --> J[break — rotate account]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(rotation): persist rate-limit window..." | Re-trigger Greptile