test: cover rotation proxy state init and stale-state recovery#572
Conversation
Direct coverage for the phase-2-extracted proxy state container: - createRotationProxyState seeds the known-manager set with the active manager and zeroes the status from the injected clock - recoverStaleRuntimeState reloads from disk, swaps the active manager while keeping the previous one known for in-flight requests, carries the routing-mutex mode onto the reloaded pool, and records the observability reset/reload markers - the 1s dedupe window returns the freshly reloaded manager without a second disk read; concurrent callers share one in-flight reload - a failed reload reports lastError, returns null, and neither arms the dedupe window nor leaks a stale promise, so the next call retries https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Warning Review limit reached
More reviews will be available in 28 minutes and 24 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
Adds the dashboard-settings-data, rotation-account-selection, rotation-token-refresh, and rotation-proxy-state suites delivered after the table was first written. https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| describe("recoverStaleRuntimeState", () => { | ||
| it("reloads from disk, swaps the active manager, and records observability", async () => { | ||
| const state = createRotationProxyState(stateInit()); | ||
| const previousManager = state.activeAccountManager; | ||
| const reloaded = new AccountManager(undefined, storageWith(2)); | ||
| loadFromDisk.mockResolvedValue(reloaded); | ||
|
|
||
| const result = await recoverStaleRuntimeState(state); | ||
|
|
||
| expect(result).toBe(reloaded); | ||
| expect(state.activeAccountManager).toBe(reloaded); | ||
| // The previous manager stays known so in-flight requests can finish. |
There was a problem hiding this comment.
AccountManager.resetVolatileRuntimeState() runs for real in these tests
only loadFromDisk is spied on; resetVolatileRuntimeState is not mocked. that means resetTrackers() and resetAllCircuitBreakers() execute against the module-level health-tracker and circuit-breaker singletons on every happy-path and concurrent test invocation. within a single vitest worker the state is isolated per file, so this doesn't contaminate sibling test files, but it does silently mutate global state inside this worker. if a future test in this same file relies on a pre-seeded tracker or circuit-breaker state (e.g. to test recovery when an account is already cooling down), the reset will silently erase that setup. adding a vi.spyOn(AccountManager, 'resetVolatileRuntimeState').mockReturnValue(undefined) in beforeEach would make the seam explicit and match the spirit of the PR description ("nothing touches disk").
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/rotation-proxy-state.test.ts
Line: 105-117
Comment:
**`AccountManager.resetVolatileRuntimeState()` runs for real in these tests**
only `loadFromDisk` is spied on; `resetVolatileRuntimeState` is not mocked. that means `resetTrackers()` and `resetAllCircuitBreakers()` execute against the module-level health-tracker and circuit-breaker singletons on every happy-path and concurrent test invocation. within a single vitest worker the state is isolated per file, so this doesn't contaminate sibling test files, but it does silently mutate global state inside this worker. if a future test in this same file relies on a pre-seeded tracker or circuit-breaker state (e.g. to test recovery when an account is already cooling down), the reset will silently erase that setup. adding a `vi.spyOn(AccountManager, 'resetVolatileRuntimeState').mockReturnValue(undefined)` in `beforeEach` would make the seam explicit and match the spirit of the PR description ("nothing touches disk").
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Declining this one: the resetVolatileRuntimeState() call inside recoverStaleRuntimeState is part of the recovery contract under test (the pool-exhausted reset of trackers and breakers is deliberate product behavior, paired with the recordRuntimeReset marker the test does assert). Stubbing the static would also stub that in-recovery call and hollow out the function being tested. The "nothing touches disk" claim still holds — these singletons are in-memory — and cross-test isolation is already guaranteed by the forks pool plus the explicit beforeEach reset. If a future test in this file needs pre-seeded tracker state, scoping a spy inside that one test is the right tool at that point.
Generated by Claude Code
Run the dedupe test on fake timers (the guard deliberately uses the real wall clock, not the injected now()), cover the window-expiry boundary where a fresh reload is allowed again, and assert a failed reload leaves lastStaleRuntimeReloadAt unarmed. https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
toStrictEqual so the lastAccountLabel/lastAccountId/ lastAccountUpdatedAt initials are asserted too. https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
2 participants
Summary
Twelfth suite in the direct-coverage wave (siblings: #559–#561, #563–#567, #569–#571; all independent, based on
main).lib/runtime/rotation-proxy-state.tsholds the proxy's per-instance state container and the pool-exhausted stale-state recovery. This addstest/rotation-proxy-state.test.ts(5 tests).A real
AccountManagerbacks the state;AccountManager.loadFromDiskis the one stubbed seam (static spy), and the observability reset/reload recorders are mocked so nothing touches disk.What the tests pin
createRotationProxyState: the known-manager set is seeded with the active manager and the status block starts zeroed withstartedAtfrom the injected clock.recoverStaleRuntimeStatereloads from disk, swapsactiveAccountManager, keeps the previous manager in the known set (so in-flight requests can still finish against it), carries the configured routing-mutex mode onto the reloaded pool, stampslastStaleRuntimeReloadAt, and records both observability markers withpool-exhausted-no-account.loadFromDiskso the window is genuinely open).status.lastError, and neither arms the dedupe window nor leaks a stale promise — the very next call retries the reload.Validation
vitest run test/rotation-proxy-state.test.ts— 5/5 passingnpm run typecheck— cleannpx eslint test/rotation-proxy-state.test.ts --max-warnings=0— cleanhttps://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
Generated by 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
test/rotation-proxy-state.test.ts— 5 tests covering the init shape, happy-path stale-state recovery, 1-second dedupe window (withvi.useFakeTimers()for determinism), concurrent-caller deduplication via a gatedloadFromDisk, and failure-path behaviour.toStrictEqualto pin the fullstatusobject (all 11 fields),knownAccountManagersseeding,lastGlobalAccountIndex, andlastStaleRuntimeReloadAtzero-value.knownAccountManagersaccumulation, observability call counts, dedupe window expiry aftervi.advanceTimersByTime(1_001), and that a failed reload surfacesstatus.lastError, leaveslastStaleRuntimeReloadAtat 0, and allows an immediate retry.Confidence Score: 5/5
test-only change; adds no production code and does not touch disk, tokens, or any live state.
the suite correctly exercises every branch of recoverStaleRuntimeState — happy path, concurrent deduplication, dedupe-window expiry, and failure recovery — using fake timers and a gate promise to keep assertions deterministic. no issues were found that would affect correctness or safety of the production module.
no files require special attention.
Important Files Changed
Sequence Diagram
sequenceDiagram participant C1 as Caller 1 participant C2 as Caller 2 participant R as recoverStaleRuntimeState participant S as RotationProxyState participant AM as AccountManager Note over R,S: dedupe check — Date.now() - lastStaleRuntimeReloadAt <= 1000? C1->>R: call R->>S: read lastStaleRuntimeReloadAt (0) R->>S: read staleRuntimeReloadPromise (null) R->>AM: resetVolatileRuntimeState() R->>AM: loadFromDisk() [async, gated] R->>S: set staleRuntimeReloadPromise C2->>R: call (concurrent) R->>S: read lastStaleRuntimeReloadAt (still 0) R->>S: read staleRuntimeReloadPromise (non-null) R-->>C2: return existing promise AM-->>R: reloaded AccountManager R->>S: "activeAccountManager = reloaded" R->>S: knownAccountManagers.add(reloaded) R->>S: "lastStaleRuntimeReloadAt = Date.now()" R->>S: "staleRuntimeReloadPromise = null (finally)" R-->>C1: reloaded R-->>C2: reloaded (same promise) Note over R,S: within 1s dedupe window C1->>R: call again R->>S: "read lastStaleRuntimeReloadAt (> 0, within window)" R-->>C1: return activeAccountManager (no disk read)Reviews (2): Last reviewed commit: "test: pin the full initial status shape ..." | Re-trigger Greptile