fix: reload stale provider state in already-open sessions#539
fix: reload stale provider state in already-open sessions#539Delqhi wants to merge 2 commits intoNoeFabris:mainfrom
Conversation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
WalkthroughThis PR implements dynamic reloading of OAuth and account state during request retries to fix stale authentication in already-open sessions. The plugin now detects when zero accounts or maximum rate-limits are reached, attempts to reload the latest state from disk via Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Additional ContextThe changes address the root cause documented in the linked issue: already-open sessions retain stale in-memory account and rate-limit state even after disk state changes. The solution introduces a provider-state reload + retry pattern similar to OpenCode's core auth recovery flow, along with proper storage merge logic to handle rate-limit reset time clearing. 🚥 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. 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 |
|
Additional validation detail from my local repro:\n\n- I reproduced this with an already-open tmux-backed OpenCode TUI session, not just fresh |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Review Summary: The PR implements three related fixes:
All changes are well-tested with new test coverage for the merge and save behaviors. The code follows existing patterns in the codebase and addresses the failure modes described in the PR. Reviewed by minimax-m2.5-20260211 · 414,943 tokens |
Greptile SummaryThis PR fixes stale Antigravity provider state in already-open sessions by reloading Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant C as Caller
participant F as fetch()
participant D as Disk (AccountManager)
participant L as retry loop
C->>F: request
F->>D: loadFromDisk(latestAuth)
D-->>F: fresh accountManager
F->>L: enter while(true)
L->>L: accountCount === 0?
alt no accounts → reload once
L->>D: reloadProviderState("no-accounts")<br/>getAuth() + loadFromDisk()
D-->>L: reloaded accountManager<br/>clears rateLimitState / emptyResponse / accountFailure maps
L->>L: continue (retry loop)
L->>L: accountCount still 0?
L-->>C: throw "No Antigravity accounts available"
else accounts available
L->>L: select account, prepare request
L->>L: strip x-goog-api-key from headers
L->>C: fetch(prepared.request, requestInit)
C-->>L: response
end
alt all-rate-limited AND waitMs > maxWaitMs → reload once
L->>D: reloadProviderState("all-rate-limited:family")<br/>getAuth() + loadFromDisk()
D-->>L: reloaded accountManager<br/>clears shared state maps
L->>L: continue (retry loop)
L->>L: still rate-limited?
L-->>C: throw "All N account(s) rate-limited"
end
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/plugin.ts
Line: 1488-1492
Comment:
**Clearing shared state affects concurrent in-flight requests**
`rateLimitStateByAccountQuota`, `emptyResponseAttempts`, `accountFailureState`, `rateLimitToastShown`, and `softQuotaToastShown` are all closure-level variables shared across every concurrent request in the same plugin instance. When one request triggers `reloadProviderState`, these maps are cleared globally, which means any other request that is mid-flight at the same moment loses its accumulated per-attempt tracking (e.g. backoff state, empty-response counters).
In practice the reload path is rare and the worst case is that a concurrent request retries against a previously-known rate-limited account once before re-discovering the limit. This is unlikely to be noticeable, but it's worth noting that the clear is not scoped to the triggering request.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/plugin.ts
Line: 1464-1465
Comment:
**Disk read on every request**
`AccountManager.loadFromDisk` is now called at the top of every `fetch` invocation, unconditionally. For heavy workloads (e.g. many parallel tool-call requests in a single agent turn) this multiplies disk reads proportionally.
Since the intent is only to avoid staleness between user interactions, it could be worth debouncing or caching the result for a short window (e.g. 1–5 s) so that a burst of concurrent requests within the same turn shares one disk read. This is not a correctness issue, but it may become noticeable at scale.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(plugin): reload stale provider state..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| rateLimitStateByAccountQuota.clear(); | ||
| emptyResponseAttempts.clear(); | ||
| accountFailureState.clear(); | ||
| rateLimitToastShown = false; | ||
| softQuotaToastShown = false; |
There was a problem hiding this comment.
rateLimitStateByAccountQuota, emptyResponseAttempts, accountFailureState, rateLimitToastShown, and softQuotaToastShown are all closure-level variables shared across every concurrent request in the same plugin instance. When one request triggers reloadProviderState, these maps are cleared globally, which means any other request that is mid-flight at the same moment loses its accumulated per-attempt tracking (e.g. backoff state, empty-response counters).
In practice the reload path is rare and the worst case is that a concurrent request retries against a previously-known rate-limited account once before re-discovering the limit. This is unlikely to be noticeable, but it's worth noting that the clear is not scoped to the triggering request.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin.ts
Line: 1488-1492
Comment:
**Clearing shared state affects concurrent in-flight requests**
`rateLimitStateByAccountQuota`, `emptyResponseAttempts`, `accountFailureState`, `rateLimitToastShown`, and `softQuotaToastShown` are all closure-level variables shared across every concurrent request in the same plugin instance. When one request triggers `reloadProviderState`, these maps are cleared globally, which means any other request that is mid-flight at the same moment loses its accumulated per-attempt tracking (e.g. backoff state, empty-response counters).
In practice the reload path is rare and the worst case is that a concurrent request retries against a previously-known rate-limited account once before re-discovering the limit. This is unlikely to be noticeable, but it's worth noting that the clear is not scoped to the triggering request.
How can I resolve this? If you propose a fix, please make it concise.| accountManager = await AccountManager.loadFromDisk(latestAuth); | ||
| activeAccountManager = accountManager; |
There was a problem hiding this comment.
AccountManager.loadFromDisk is now called at the top of every fetch invocation, unconditionally. For heavy workloads (e.g. many parallel tool-call requests in a single agent turn) this multiplies disk reads proportionally.
Since the intent is only to avoid staleness between user interactions, it could be worth debouncing or caching the result for a short window (e.g. 1–5 s) so that a burst of concurrent requests within the same turn shares one disk read. This is not a correctness issue, but it may become noticeable at scale.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin.ts
Line: 1464-1465
Comment:
**Disk read on every request**
`AccountManager.loadFromDisk` is now called at the top of every `fetch` invocation, unconditionally. For heavy workloads (e.g. many parallel tool-call requests in a single agent turn) this multiplies disk reads proportionally.
Since the intent is only to avoid staleness between user interactions, it could be worth debouncing or caching the result for a short window (e.g. 1–5 s) so that a burst of concurrent requests within the same turn shares one disk read. This is not a correctness issue, but it may become noticeable at scale.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugin.ts (1)
1855-1861:⚠️ Potential issue | 🟠 MajorSanitize the account-verification probe too.
This strips
x-goog-api-keyfrom warmup and main request paths, but the OAuth verification probe on Line 485 still spreadsgetAntigravityHeaders()without removing it.verify/verify-allcan therefore keep hitting the same project-mismatch failure that this patch fixes for normal requests.Also applies to: 2008-2013
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugin.ts` around lines 1855 - 1861, The OAuth/account-verification probe still spreads getAntigravityHeaders() without removing the x-goog-api-key, so calls like verify/verify-all can use the wrong API key; update the verification probe (the code that builds the verify/verify-all request — locate the verify handler that calls getAntigravityHeaders()) to create a headers copy, delete("x-goog-api-key") from that headers object prior to merging/spreading into the RequestInit (same approach used for warmupHeaders/warmupInit), and apply the same sanitization to the other similar blocks referenced around the verify probe (also the analogous blocks in the region flagged around lines ~2008-2013).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugin.ts`:
- Around line 1464-1465: The code currently hot-swaps the AccountManager by
assigning accountManager = await AccountManager.loadFromDisk(latestAuth) and
activeAccountManager = accountManager which breaks session-local state
(sessionOffsetApplied, toast debounce, pending save timer) and leaves
refreshQueue and maps like rateLimitStateByAccountQuota/accountFailureState
bound to the old instance; instead, implement a reload handover in the
AccountManager reload flow: call AccountManager.loadFromDisk(latestAuth) to
obtain newManager, transfer/merge session-local flags and timers from the
existing accountManager into newManager (or call a reconcileFrom(oldManager)
method on AccountManager), rebind refreshQueue and any external maps to
newManager, dispose the oldManager (cancel timers, clear debouncers, and stop
using its index-based keys), then set activeAccountManager to the
reconciled/newManager; ensure this same pattern is applied to the other reload
locations around lines 1477–1494.
In `@src/plugin/accounts.ts`:
- Line 1006: mergeAccountStorage treats an empty object for rateLimitResetTimes
as an explicit delete, but the save path is always emitting {} for empty
in-memory maps; change the persistence so it only writes the rateLimitResetTimes
property when this instance actually cleared limits or when the map is
non-empty: in the save/build object code that currently sets
rateLimitResetTimes: { ...a.rateLimitResetTimes }, update it to omit the
property if the map is empty (or write an explicit cleared sentinel only when an
explicitClear flag is set), and ensure saveToDisk()/mergeAccountStorage() uses
that presence/flag to perform deletes; refer to the rateLimitResetTimes field,
mergeAccountStorage(), and the saveToDisk()/save path to implement this
conditional write.
---
Outside diff comments:
In `@src/plugin.ts`:
- Around line 1855-1861: The OAuth/account-verification probe still spreads
getAntigravityHeaders() without removing the x-goog-api-key, so calls like
verify/verify-all can use the wrong API key; update the verification probe (the
code that builds the verify/verify-all request — locate the verify handler that
calls getAntigravityHeaders()) to create a headers copy,
delete("x-goog-api-key") from that headers object prior to merging/spreading
into the RequestInit (same approach used for warmupHeaders/warmupInit), and
apply the same sanitization to the other similar blocks referenced around the
verify probe (also the analogous blocks in the region flagged around lines
~2008-2013).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 87bea7d7-d91c-4086-8896-65ddb49e0fdc
📒 Files selected for processing (5)
src/plugin.tssrc/plugin/accounts.test.tssrc/plugin/accounts.tssrc/plugin/storage.test.tssrc/plugin/storage.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
| accountManager = await AccountManager.loadFromDisk(latestAuth); | ||
| activeAccountManager = accountManager; |
There was a problem hiding this comment.
Avoid replacing the AccountManager object mid-session.
On Line 1464 and Line 1486, this hot-swaps the manager instance instead of reconciling it in place. AccountManager carries session-local state (sessionOffsetApplied, toast debounce, pending save timer), so the swap can re-apply PID offset on later requests, re-fire “Using account...” toasts, and still let the old instance persist a stale snapshot after the reload. rateLimitStateByAccountQuota / accountFailureState are also keyed by account.index, and Line 1427 leaves refreshQueue bound to the previous manager. This needs a reload handover that disposes/rebinds the old state instead of just reassigning the variable.
Also applies to: 1477-1494
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugin.ts` around lines 1464 - 1465, The code currently hot-swaps the
AccountManager by assigning accountManager = await
AccountManager.loadFromDisk(latestAuth) and activeAccountManager =
accountManager which breaks session-local state (sessionOffsetApplied, toast
debounce, pending save timer) and leaves refreshQueue and maps like
rateLimitStateByAccountQuota/accountFailureState bound to the old instance;
instead, implement a reload handover in the AccountManager reload flow: call
AccountManager.loadFromDisk(latestAuth) to obtain newManager, transfer/merge
session-local flags and timers from the existing accountManager into newManager
(or call a reconcileFrom(oldManager) method on AccountManager), rebind
refreshQueue and any external maps to newManager, dispose the oldManager (cancel
timers, clear debouncers, and stop using its index-based keys), then set
activeAccountManager to the reconciled/newManager; ensure this same pattern is
applied to the other reload locations around lines 1477–1494.
| enabled: a.enabled, | ||
| lastSwitchReason: a.lastSwitchReason, | ||
| rateLimitResetTimes: Object.keys(a.rateLimitResetTimes).length > 0 ? a.rateLimitResetTimes : undefined, | ||
| rateLimitResetTimes: { ...a.rateLimitResetTimes }, |
There was a problem hiding this comment.
Don't treat every empty rateLimitResetTimes map as an explicit clear.
On Line 1006, every save from an account with no in-memory limits now emits {}. mergeAccountStorage() treats {} as “delete the stored limits”, so a stale writer can wipe a fresher rate-limit record from another session/process. A simple case is: session A still has {} in memory, session B persists a fresh 429 window, then A’s delayed saveToDisk() clears B’s limit again. This sentinel needs to be emitted only when this instance actually cleared limits, not for every empty map.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugin/accounts.ts` at line 1006, mergeAccountStorage treats an empty
object for rateLimitResetTimes as an explicit delete, but the save path is
always emitting {} for empty in-memory maps; change the persistence so it only
writes the rateLimitResetTimes property when this instance actually cleared
limits or when the map is non-empty: in the save/build object code that
currently sets rateLimitResetTimes: { ...a.rateLimitResetTimes }, update it to
omit the property if the map is empty (or write an explicit cleared sentinel
only when an explicitClear flag is set), and ensure
saveToDisk()/mergeAccountStorage() uses that presence/flag to perform deletes;
refer to the rateLimitResetTimes field, mergeAccountStorage(), and the
saveToDisk()/save path to implement this conditional write.
1 participant
Summary
No Antigravity accounts.../All accounts rate-limited...states in already-open sessionsrateLimitResetTimeson disk so stale quota blocks do not get merged back inx-goog-api-keyfrom Antigravity request headers so OAuth-based Antigravity calls do not inherit an unrelated Google API key from the global provider configProblem
Fresh
opencode runprocesses could recover after auth/account changes on disk, but already-open OpenCode sessions could stay stuck on stale Antigravity state until restart.Two concrete failure modes were reproducible:
No Antigravity accounts available. Run \opencode auth login`.`All 1 account(s) rate-limited for claude...There was also a related config interaction where an unrelated
provider.google.options.apiKeycould leak into Antigravity OAuth requests asx-goog-api-key, causing project mismatch failures.What changed
src/plugin.tsAccountManagerfrom the latest auth at request timex-goog-api-keyfrom Antigravity request headers and thinking warmup headerssrc/plugin/accounts.ts+src/plugin/storage.tsrateLimitResetTimesduring save/merge so clearing stale disk limits actually sticksTests
rateLimitResetTimespersistence and merge semanticsVerification
npm run buildnpm testantigravity-accounts.jsonon diskCloses #538