Skip to content

fix(rotation): persist rate-limit window in short-retry 429 path - #609

Merged
ndycode merged 1 commit into
mainfrom
fix/persist-cooldown-short-retry
Jun 14, 2026
Merged

fix(rotation): persist rate-limit window in short-retry 429 path#609
ndycode merged 1 commit into
mainfrom
fix/persist-cooldown-short-retry

Conversation

@ndycode

@ndycode ndycode commented Jun 14, 2026

Copy link
Copy Markdown
Owner

Summary

  • Found by a pre-release deep stress-test sweep (verification gauntlet + adversarial probes). The gauntlet was all-green; an adversarial persistence-audit probe found a third instance of the cooldown-persistence gap class fixed by fix(rotation): break stale-recovery deadlock on transient account state #607 and fix(rotation): persist cooldown when account has no resolvable accountId #608 — this one in index.ts.
  • The short-retry branch of the runtime fetch loop marks the account rate-limited via markRateLimitedWithReason (which mutates the disk-serialized rateLimitResetTimes) and recordRateLimit, then sleeps and retries — but never called saveToDiskDebounced(). The sibling full-rotation branch directly below it does persist (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.

What Changed

  • index.ts — add accountManager.saveToDiskDebounced() after recordRateLimit() 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 asserts saveToDiskDebounced was 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 lint
  • npm run typecheck
  • npm test (4920 passed, 3 skipped, 0 failures)
  • npm test -- test/documentation.test.ts
  • npm run build

Docs and Governance Checklist

  • README updated (if user-visible behavior changed)
  • docs/getting-started.md updated (if onboarding flow changed)
  • docs/features.md updated (if capability surface changed)
  • relevant docs/reference/* pages updated (if commands/settings/paths changed)
  • docs/upgrade.md updated (if migration behavior changed)
  • SECURITY.md and CONTRIBUTING.md reviewed for alignment

No user-visible surface changed — internal fetch-loop consistency fix.

Risk and Rollback

  • Risk level: low. Adds one debounced disk write on a path that previously skipped it; matches the established sibling-branch pattern.
  • Rollback plan: revert this commit.

🤖 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 in index.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 after recordRateLimit() 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 asserts saveToDiskDebounced was 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 sleep is 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

Filename Overview
index.ts adds saveToDiskDebounced() in the short-retry 429 branch, directly after recordRateLimit(), mirroring the full-rotation branch at line ~2333. change is minimal, placement is correct, comment is accurate.
test/index.test.ts regression test correctly validates saveToDiskDebounced is called once on the short-retry path. sleep is not mocked, so the test incurs a real ~1s wait; fake timers would be more consistent with other timer-controlled tests in the same suite.

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]
Loading

Fix All in Codex

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
test/index.test.ts:5549-5649
`sleep` is not mocked here, so the test incurs a real `~1 s` wait (`addJitter(Math.max(100, 1000), 0.2)` ≈ 800–1200 ms). other tests in this same describe block (e.g. the `useFakeTimers` + `advanceTimersByTimeAsync(1000)` pattern at line 5115) control time explicitly. the sibling test at line 5470 has the same gap — this PR doubles the accumulated wall-clock cost. consider wrapping the `sleep` call with fake timers to keep the suite deterministic and fast.

Reviews (1): Last reviewed commit: "fix(rotation): persist rate-limit window..." | Re-trigger Greptile

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>
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Summary: 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 index.ts. Without this fix, the account's rate-limit reset time can be lost if the process crashes during the retry sleep, allowing the account to be immediately re-selected on restart and defeating the intended cooldown mechanism. This is the third instance of this same durability issue (previously fixed in PRs #607 and #608).

Changes:

  • index.ts: Added missing accountManager.saveToDiskDebounced() call after rate-limit is recorded in the short-retry branch (+2 lines with explanatory comment), aligning with the existing full-rotation branch behavior
  • test/index.test.ts: Added regression test verifying the short-retry path triggers saveToDiskDebounced (+100 lines)

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.

Walkthrough

in the short-cooldown 429 retry path of the oauth plugin, accountManager.saveToDiskDebounced() is now called immediately after markRateLimitedWithReason, aligning with the full-rotation branch. a regression test verifies this call happens exactly once while the request still resolves with 200.

Changes

Short 429 retry disk persistence

Layer / File(s) Summary
saveToDiskDebounced call + regression test
index.ts, test/index.test.ts
index.ts:2285–2290 adds saveToDiskDebounced() with comments after markRateLimitedWithReason in the short retry branch. test/index.test.ts:5549–5648 adds a 100-line regression test mocking the branch, asserting the call fires exactly once and the outer fetch still resolves 200.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • ndycode/codex-multi-auth#608: directly related — pins saveToDiskDebounced() after cooldown mutation in a neighboring 429 branch (missing/unresolvable accountId rotation path), same persistence pattern at the same call site.
  • ndycode/codex-multi-auth#355: changes the same index.ts 429 retry/cooldown flow to persist rate-limit windows via saveToDiskDebounced, with accompanying test coverage for that behavior.
  • ndycode/codex-multi-auth#45: modifies the same short-window 429 rotation logic in index.ts around the rate-limit path this pr is fixing.

Suggested labels

bug

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed title follows conventional commits format (fix type, rotation scope, lowercase imperative), under 72 chars, and directly describes the main change.
Description check ✅ Passed description is complete: summary articulates the bug found in stress-testing, what changed section details both code and test additions, validation section fully checked, risk/rollback clear, no user-facing surface changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/persist-cooldown-short-retry
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch fix/persist-cooldown-short-retry

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ndycode
ndycode merged commit f9abbe0 into main Jun 14, 2026
1 of 2 checks passed
@ndycode ndycode mentioned this pull request Jun 14, 2026
12 tasks
ndycode added a commit that referenced this pull request Jun 14, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant