Repository navigation
Retry deactivated workspaces during rotation - #99
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. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📝 WalkthroughWalkthroughAdded a new traversal-control flag Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
index.ts (1)
2086-2136:⚠️ Potential issue | 🟠 MajorUse
removeAccountsWithSameRefreshToken()instead ofremoveAccount()for workspace deactivation.When a workspace is deactivated at line 2117, only the current account entry is removed via
removeAccount(). This diverges from the auth-failure handling at line 1878, which correctly usesremoveAccountsWithSameRefreshToken()to remove all workspace variants. If duplicate entries exist for the same workspace (samerefreshToken, different organization/account IDs), the unremoved variants can re-enter rotation. Additionally, the fallback tomarkAccountCoolingDown(..., "auth-failure")is semantically incorrect—a deactivated workspace should be permanently removed, not temporarily cooled down as an auth failure.Replace the
removeAccount()call withremoveAccountsWithSameRefreshToken()and remove or replace the cooldown fallback, since a deactivated workspace should not be eligible for retry after cooldown expiry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 2086 - 2136, The workspace deactivation branch currently calls accountManager.removeAccount(account) and then falls back to accountManager.markAccountCoolingDown(..., "auth-failure"); replace the single-account removal with accountManager.removeAccountsWithSameRefreshToken(account) to purge all variants sharing the same refreshToken (use the same account variable and keep accountLabel), retain accountManager.saveToDiskDebounced(), attempted.clear(), accountCount = accountManager.getAccountCount(), restartAccountTraversalAfterWorkspaceDeactivation = true and the showToast call, and remove the markAccountCoolingDown(...) fallback entirely so deactivated workspaces are permanently removed rather than cooled down; ensure any existing logic that depended on removeAccount's boolean return is adjusted to the new method's semantics (e.g., check whether any accounts were removed if needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@index.ts`:
- Around line 2086-2136: The workspace deactivation branch currently calls
accountManager.removeAccount(account) and then falls back to
accountManager.markAccountCoolingDown(..., "auth-failure"); replace the
single-account removal with
accountManager.removeAccountsWithSameRefreshToken(account) to purge all variants
sharing the same refreshToken (use the same account variable and keep
accountLabel), retain accountManager.saveToDiskDebounced(), attempted.clear(),
accountCount = accountManager.getAccountCount(),
restartAccountTraversalAfterWorkspaceDeactivation = true and the showToast call,
and remove the markAccountCoolingDown(...) fallback entirely so deactivated
workspaces are permanently removed rather than cooled down; ensure any existing
logic that depended on removeAccount's boolean return is adjusted to the new
method's semantics (e.g., check whether any accounts were removed if needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1626d4e-499a-4e19-89bd-01159b5e2df2
📒 Files selected for processing (2)
index.tstest/index.test.ts
There was a problem hiding this comment.
Pull request overview
Updates the account rotation logic to treat deactivated_workspace responses as retriable, enabling failover to sibling workspaces while still removing (or cooling down) the dead workspace from rotation.
Changes:
- Adjust test fixtures to return
detail.code = "deactivated_workspace"instead of the previouserror.codeshape. - Add a new restart path in the rotation traversal to restart selection after a workspace deactivation (including removal/cooldown + persistence).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/index.test.ts | Updates mocked API error payload shape to validate detail.code handling for deactivated workspaces. |
| index.ts | Adds a dedicated restart flag/path to retry rotation after detecting a deactivated workspace and updating the account pool. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return new Response(JSON.stringify({ detail: { code: "deactivated_workspace", message: "workspace dead" } }), { | ||
| status: 402, | ||
| }); |
There was a problem hiding this comment.
This change replaces the previously-covered { error: { code } } payload shape with { detail: { code } }, which reduces regression coverage for the earlier shape. If the production handler is intended to tolerate both formats (common with API gateways / different error envelopes), consider adding an additional test case (or parameterizing this one) that exercises the legacy error.code shape as well.
| return new Response(JSON.stringify({ detail: { code: "deactivated_workspace", message: "workspace dead" } }), { | |
| status: 402, | |
| }); | |
| return new Response( | |
| JSON.stringify({ | |
| error: { code: "deactivated_workspace", message: "workspace dead" }, | |
| detail: { code: "deactivated_workspace", message: "workspace dead" }, | |
| }), | |
| { | |
| status: 402, | |
| }, | |
| ); |
| let accountCount = accountManager.getAccountCount(); | ||
| const attempted = new Set<number>(); | ||
| let restartAccountTraversalWithFallback = false; | ||
| let restartAccountTraversalAfterWorkspaceDeactivation = false; | ||
|
|
||
| while (attempted.size < Math.max(1, accountCount)) { |
There was a problem hiding this comment.
The indentation/formatting in this hunk looks inconsistent (notably the while (attempted.size ...) line appearing flush-left relative to surrounding control flow). Even if it compiles, this makes it difficult to visually validate block structure and increases the risk of brace/control-flow mistakes. Running the project formatter (or adjusting indentation to match surrounding style) would make the traversal/restart logic much safer to review and maintain.
| if (restartAccountTraversalWithFallback) { | ||
| break; | ||
| } | ||
| if (restartAccountTraversalAfterWorkspaceDeactivation) { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (restartAccountTraversalWithFallback) { | ||
| continue; | ||
| } | ||
| if (restartAccountTraversalWithFallback) { | ||
| continue; | ||
| } | ||
| if (restartAccountTraversalAfterWorkspaceDeactivation) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The restart handling is duplicated in two places (a break gate and a continue gate) for both flags. This duplication is easy to accidentally diverge over time (e.g., adding a new restart reason but only updating one site). Consider consolidating into a single shouldRestartTraversal (or a single enum-like restartReason) and handling it once, so there’s only one authoritative place controlling the restart decision.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
index.ts (1)
2086-2140:⚠️ Potential issue | 🟠 MajorRestart traversal even when grouped removal returns
0.Because
accountManageris shared across fetches, another concurrent request can remove the same refresh-token group first. In that case this branch keeps the oldattempted/accountCountsnapshot and can skip an eligible sibling after indices shift instead of rotating to it.♻️ Proposed fix
const removedCount = accountManager.removeAccountsWithSameRefreshToken(account); if (removedCount > 0) { accountManager.saveToDiskDebounced(); - restartAccountTraversalAfterWorkspaceDeactivation = true; const removalMessage = removedCount > 1 ? `Workspace deactivated. Removed ${removedCount} related entries from rotation and switching accounts.` : `Workspace deactivated. Removed ${accountLabel} from rotation and switching accounts.`; await showToast( removalMessage, "warning", { duration: toastDurationMs }, ); - break; - } - - logWarn( - `[${PLUGIN_NAME}] Expected grouped account removal after workspace deactivation, but removed ${removedCount}.`, - ); + } else { + logWarn( + `[${PLUGIN_NAME}] Expected grouped account removal after workspace deactivation, but removed ${removedCount}. Restarting traversal anyway because the pool may already have changed.`, + ); + } + restartAccountTraversalAfterWorkspaceDeactivation = true; break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 2086 - 2140, When workspaceDeactivated is true we must always restart the account traversal even if accountManager.removeAccountsWithSameRefreshToken(account) returns 0 to avoid using a stale attempted/accountCount snapshot and skipping siblings after concurrent removals; update the branch that handles removedCount to set restartAccountTraversalAfterWorkspaceDeactivation = true before the removalCount check (or set it in both the removedCount > 0 and removedCount === 0 paths) and ensure showToast/early break logic still runs for the removedCount > 0 case while preserving the existing logWarn + break for the removedCount === 0 case; key symbols: workspaceDeactivated, accountManager.removeAccountsWithSameRefreshToken, restartAccountTraversalAfterWorkspaceDeactivation, showToast.
🧹 Nitpick comments (1)
index.ts (1)
1785-1788: Collapse the two traversal-restart flags into one.Both flags only drive the same
break/continuepath. A singlerestartAccountTraversalboolean would remove duplicated control flow and make future retry reasons easier to wire correctly.Also applies to: 2441-2454
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 1785 - 1788, Replace the two boolean flags restartAccountTraversalWithFallback and restartAccountTraversalAfterWorkspaceDeactivation with a single flag restartAccountTraversal; initialize restartAccountTraversal next to accountCount and attempted (where accountCount = accountManager.getAccountCount() is set), then update all control flow checks and assignments that reference either of the two old flags (including the other occurrence around the 2441-2454 block) to use restartAccountTraversal instead so the break/continue logic is driven by one unified boolean.
🤖 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/index.test.ts`:
- Around line 3207-3210: Update the 402 test fixture so that one mocked Response
only contains detail.code (and omits error.code) to ensure parsing relies on
detail; locate the Response JSON object created in the test (the object with
keys "error" and "detail" returned via new Response(JSON.stringify({...})) in
test/index.test.ts) and remove or null out the top-level error.code for at least
one of the 402 cases (keep detail.code present) so the test verifies handling
when only detail.code is provided.
---
Outside diff comments:
In `@index.ts`:
- Around line 2086-2140: When workspaceDeactivated is true we must always
restart the account traversal even if
accountManager.removeAccountsWithSameRefreshToken(account) returns 0 to avoid
using a stale attempted/accountCount snapshot and skipping siblings after
concurrent removals; update the branch that handles removedCount to set
restartAccountTraversalAfterWorkspaceDeactivation = true before the removalCount
check (or set it in both the removedCount > 0 and removedCount === 0 paths) and
ensure showToast/early break logic still runs for the removedCount > 0 case
while preserving the existing logWarn + break for the removedCount === 0 case;
key symbols: workspaceDeactivated,
accountManager.removeAccountsWithSameRefreshToken,
restartAccountTraversalAfterWorkspaceDeactivation, showToast.
---
Nitpick comments:
In `@index.ts`:
- Around line 1785-1788: Replace the two boolean flags
restartAccountTraversalWithFallback and
restartAccountTraversalAfterWorkspaceDeactivation with a single flag
restartAccountTraversal; initialize restartAccountTraversal next to accountCount
and attempted (where accountCount = accountManager.getAccountCount() is set),
then update all control flow checks and assignments that reference either of the
two old flags (including the other occurrence around the 2441-2454 block) to use
restartAccountTraversal instead so the break/continue logic is driven by one
unified boolean.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9570622b-769c-46e5-9942-bbfda0a04e9f
📒 Files selected for processing (2)
index.tstest/index.test.ts
| return new Response(JSON.stringify({ | ||
| error: { code: "deactivated_workspace", message: "workspace dead" }, | ||
| detail: { code: "deactivated_workspace", message: "workspace dead" }, | ||
| }), { |
There was a problem hiding this comment.
Make at least one 402 fixture detail.code-only to avoid false positives.
At Line 3207 and Line 4290, the mocked payload includes both error.code and detail.code. That can still pass if detail.code parsing breaks, because the legacy field remains available. For this PR objective, at least one path should only provide detail.code.
Suggested fixture adjustment
-return new Response(JSON.stringify({
- error: { code: "deactivated_workspace", message: "workspace dead" },
- detail: { code: "deactivated_workspace", message: "workspace dead" },
-}), {
+return new Response(JSON.stringify({
+ detail: { code: "deactivated_workspace", message: "workspace dead" },
+}), {
status: 402,
});Also applies to: 4290-4293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/index.test.ts` around lines 3207 - 3210, Update the 402 test fixture so
that one mocked Response only contains detail.code (and omits error.code) to
ensure parsing relies on detail; locate the Response JSON object created in the
test (the object with keys "error" and "detail" returned via new
Response(JSON.stringify({...})) in test/index.test.ts) and remove or null out
the top-level error.code for at least one of the 402 cases (keep detail.code
present) so the test verifies handling when only detail.code is provided.
…tation Retry deactivated workspaces during rotation
…tation Retry deactivated workspaces during rotation
Summary
deactivated_workspaceas a hard removal for every account entry sharing the same refresh token, then restart traversal onto a healthy remaining account0detail.codeand legacyerror.codeenvelopes in the touched deactivated-workspace testsVerification
Summary by CodeRabbit
Bug Fixes
Tests
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 upgrades the deactivated-workspace handler from single-account removal to grouped removal — all accounts sharing the same refresh token are evicted together via
removeAccountsWithSameRefreshToken. a newrestartAccountTraversalAfterWorkspaceDeactivationflag drives the outer-loop restart so the plugin correctly retries against the remaining healthy accounts rather than returning 503 immediately. a defensive fallback (cool-down +saveToDiskDebounced) handles the unexpected case where grouped removal returns 0.removeAccountsWithSameRefreshTokenreplaces the oldremoveAccountcall, evicting all stale entries sharing the deactivated refresh token in one shot.restartAccountTraversalWithFallbackpattern — sets on successful grouped removal, triggerscontinueon the outerwhile (true), resetsaccountCount/attemptedcleanly.removedCount === 0(should not happen in practice), the account is cooled down and disk is saved, but the windows/redaction comment present on the happy path is missing here — minor doc gap per the project's merge policy.removeAccountsWithSameRefreshTokenmock; new test covers theremovedCount === 0cool-down path; both touched fetch mocks now include the dualerror/detailenvelope to cover both code paths inisDeactivatedWorkspaceError.Confidence Score: 4/5
removedCount === 0fallback is good, and the dual envelope mock fix is correct. the only remaining item is adding the windows/redaction comment to the fallbacksaveToDiskDebouncedcall — a one-line doc fix that doesn't affect runtime correctness.saveToDiskDebouncedmissing windows/redaction comment)Important Files Changed
removeAccountwith groupedremoveAccountsWithSameRefreshToken, introducesrestartAccountTraversalAfterWorkspaceDeactivationflag to drive the outer-loop restart, and adds a defensive fallback (cool-down + debounced save) when grouped removal returns 0. logic is sound; the fallbacksaveToDiskDebouncedcall lacks the windows/redaction comment that the happy path carries.removeAccountsWithSameRefreshTokenmock, and dualerror/detailenvelope), and adds a new test for theremovedCount === 0cool-down path. coverage looks complete for the changed behaviour.Sequence Diagram
sequenceDiagram participant OL as outer while(true) participant IL as inner while(attempted) participant AM as accountManager OL->>OL: reset accountCount, attempted, flags IL->>AM: getCurrentOrNextForFamilyHybrid() AM-->>IL: deadWorkspace (shared-refresh) IL->>API: POST /chat [access-org-dead] API-->>IL: 402 deactivated_workspace IL->>AM: refundToken + recordFailure IL->>AM: withFlaggedAccountStorageTransaction (flag record) IL->>AM: removeAccountsWithSameRefreshToken(deadWorkspace) AM-->>IL: removedCount = 2 IL->>AM: saveToDiskDebounced() IL->>IL: restartAccountTraversalAfterWorkspaceDeactivation = true IL->>IL: break OL->>OL: if restartAccountTraversalAfterWorkspaceDeactivation → continue OL->>OL: reset accountCount=1, attempted=∅, flags=false IL->>AM: getCurrentOrNextForFamilyHybrid() AM-->>IL: healthyFallback (healthy-refresh) IL->>API: POST /chat [access-org-live] API-->>IL: 200 OK IL->>AM: recordSuccess IL-->>OL: return 200Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "fix(pr99):restore-zero-removal-fallback" | Re-trigger Greptile
Context used: