Fix runtime skip for deactivated accounts#463
Conversation
📝 WalkthroughWalkthroughadds http 402 support for deactivated workspace detection. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy as Runtime Rotation Proxy
participant Upstream
Client->>Proxy: Request (account selected)
Proxy->>Upstream: Forward request
Upstream-->>Proxy: HTTP 402/403 + error body
Proxy->>Proxy: extractErrorCodeFromBody()<br/>parse code field
Proxy->>Proxy: isWorkspaceDisabledError()<br/>check deactivated_workspace
alt Deactivated Workspace Detected
Proxy->>Proxy: Disable account
Proxy->>Proxy: Clear session affinity
Proxy->>Proxy: Increment rotation counter
Proxy->>Proxy: Mark exhaustion reason
Proxy->>Proxy: Select next enabled account
Proxy->>Upstream: Retry request (different account)
else Unrelated Error
Proxy-->>Client: Forward 402/403 to client
Proxy->>Proxy: Record as failure
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes review flags:
Suggested labels
🚥 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. ✨ 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 |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/runtime-rotation-proxy.test.ts`:
- Around line 831-941: Add a deterministic vitest that covers the 403
passthrough regression: create an AccountManager (e.g., new
AccountManager(undefined, createStorage(now, 2))) and a createRecordingFetch
implementation that returns a 403 JSON error (non-workspace_disabled) on the
first attempt; start the proxy with startProxy({ accountManager, fetchImpl });
POST via postResponses and assert the proxied response status/body are forwarded
unchanged, that only one upstream call was made (calls.length === 1), and that
accountManager.getAccountByIndex(0)?.enabled and getAccountByIndex(1)?.enabled
remain true; place the test near the other rotation tests (around the block
covering 922-941) to exercise the branch in lib/runtime-rotation-proxy.ts (the
disable-or-forward logic at ~1085-1117) and ensure determinism with vitest
utilities already used in the file.
🪄 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: 265fbfb5-c3c5-46bd-a865-dc92b34140f8
📒 Files selected for processing (4)
lib/request/fetch-helpers.tslib/runtime-rotation-proxy.tstest/fetch-helpers.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)
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/fetch-helpers.test.tstest/runtime-rotation-proxy.test.ts
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/runtime-rotation-proxy.tslib/request/fetch-helpers.ts
🔇 Additional comments (3)
lib/request/fetch-helpers.ts (1)
331-344: 402 deactivated workspace detection looks correct.the new 402 branch in
lib/request/fetch-helpers.ts:331-344cleanly matchesdeactivated_workspacevia normalized code, tokenized code, and body text, and keeps 403 logic intact. coverage is present intest/fetch-helpers.test.ts:691-706.test/fetch-helpers.test.ts (1)
691-706: good regression coverage for 402 token-gated classification.the cases in
test/fetch-helpers.test.ts:691-706correctly prove that 402 alone is not enough and thatdeactivated_workspaceis required, matchinglib/request/fetch-helpers.ts:338-344.lib/runtime-rotation-proxy.ts (1)
948-959: the transient-budget vs deactivated-skip split is implemented correctly.the loop/accounting updates in
lib/runtime-rotation-proxy.ts:948-959andlib/runtime-rotation-proxy.ts:1213-1224, plus the disable+retry branch inlib/runtime-rotation-proxy.ts:1085-1117, match the intended behavior for mixed transient and deactivated pools.Also applies to: 1085-1117, 1213-1224
1 participant
Summary:
402deactivated_workspaceas a disabled workspace signal while preserving existing403workspace-disabled handling.enabled=false, clear session affinity for the request, persist the pool, and retry on another account.taskkillcleanup output in TUI-facing flows without exposing process output.What changed:
402 deactivated_workspacebefore generic error forwarding and retries the request on another enabled account.enabled=falsestorage and skipped after reload.codex_runtime_rotation_pool_exhaustedJSON withreason: "deactivated"when all usable accounts are deactivated.402and403responses still pass through unchanged.cmd.exe /d /s /c taskkill ...with ignored stdio to avoid noisySUCCESS: The process with PID ... has been terminated.output.typecheck:scriptsgreen.Verification:
npm.cmd test -- test/runtime-rotation-proxy.test.ts test/fetch-helpers.test.ts test/auto-update-checker.test.ts test/test-model-matrix-script.test.tsnpm.cmd test -- test/codex-routing.test.ts test/package-bin.test.tsnpm.cmd test -- test/index-retry.test.tsnpm.cmd run lintnpm.cmd run typechecknpm.cmd run typecheck:scriptsnpm.cmd run buildnpm.cmd testnpm.cmd run pack:checkStress notes:
npm.cmd testpassed with 257 files and 3872 tests.npm.cmd test+npm.cmd run pack:checktimeout was fixed intest/index-retry.test.ts; the concurrent stress condition now passes.Risk:
Rollback:
fix/runtime-skip-deactivated-accounts.Docs:
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
this pr adds 402
deactivated_workspacefailover to the runtime rotation proxy: deactivated accounts are disabled, session affinity is cleared, and the proxy retries on the remaining pool, with pool exhaustion emitting a structuredcodex_runtime_rotation_pool_exhaustedresponse. the transient-vs-deactivated attempt tracking and the post-loop exhaustion reason promotion are well-designed and the concurrent deactivation guard (accountWasEnabled) correctly prevents double-recording.Confidence Score: 5/5
safe to merge — all new paths are guarded, tested, and have no P0/P1 issues
no P0 or P1 issues found; concurrency guard, exhaustion reason promotion, and transient-vs-deactivated budget split are all correct; coverage now includes 402 failover, 403 failover, concurrent deactivation, mixed exhaustion, persistence, and passthrough — only minor P2 style observations remain
lib/runtime-rotation-proxy.ts and lib/request/fetch-helpers.ts are the highest-impact files but reviewed clean
Important Files Changed
transientAttemptsbudget split, and post-loop exhaustion reason promotion — logic is sound and well-guarded against concurrent deactivationisWorkspaceDisabledErrorto handle 402 status withdeactivated_workspacetoken or body-text regex; 403 handling is preserved unchangeddeactivated_workspacepath and verifies the false-positive guard forpayment_requiredand plain body texttaskkillincmd.exe /d /s /cto silence the SUCCESS stdout noise on windows; no logic changerunQuietWindowsTaskkillusing the samecmd.exe /d /s /cpattern; consistent with the auto-update-checker changevi.useFakeTimers()from a test that doesn't need fake timer control, eliminating the source of flakiness under concurrent loadnormalizeStandaloneArgsto keeptypecheck:scriptsgreen; no logic changenormalizeAuthAliasandshouldHandleMultiAuthAuth; no logic changeFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Incoming request] --> B{chooseAccount} B -- no account --> Z[break: pool exhausted] B -- account selected --> C[consumeToken] C --> D[ensureFreshAccessToken] D -- not ok retryable --> E[transientAttempts++ exhaustionReason=auth-failure] D -- not ok non-retryable --> E2[exhaustionReason=auth-failure continue] D -- ok --> F[fetch upstream] F -- network error --> G[transientAttempts++ exhaustionReason=network-error continue] F -- rate-limit 429 --> H[transientAttempts++ exhaustionReason=rate-limit continue] F -- 402 or 403 --> I{isWorkspaceDisabledError?} I -- yes --> J[refundToken recordFailure if accountWasEnabled setAccountEnabled false forgetSession exhaustionReason=deactivated continue] I -- no --> K[forward bodyText to client record usage return] F -- 401 --> L[transientAttempts++ exhaustionReason=auth-failure continue] F -- 5xx --> M[transientAttempts++ exhaustionReason=server-error continue] F -- 200 OK --> N[stream forward to client return] E --> LOOP{while: attemptedIndexes lt accountCount AND transientAttempts lt transientAttemptLimit} J --> LOOP G --> LOOP H --> LOOP L --> LOOP M --> LOOP E2 --> LOOP LOOP -- continue --> B LOOP -- exit --> P{post-loop reason} P -- transientAttempts gte limit AND size lt count --> Q[exhaustionReason=budget] P -- exhaustionReason==deactivated AND transientReason set --> R[exhaustionReason=transientExhaustionReason] P --> S[writePoolExhausted] Z --> SPrompt To Fix All With AI
Reviews (5): Last reviewed commit: "stabilize index retry stress test" | Re-trigger Greptile