fix(codex-limits): harden deduped usage refresh#71
Conversation
Co-authored-by: Codex <noreply@openai.com>
|
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 failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe changes enhance account credential management by implementing per-request timeout handling for API calls, tracking active accounts via refresh tokens, propagating rotated credentials across duplicate accounts, and optimizing storage synchronization through cache invalidation rather than in-place reloading. Tests are expanded to validate deduplication logic, active marker preservation, and token rotation propagation. Changes
Sequence DiagramsequenceDiagram
participant Client
participant AccountMgr as Account Manager
participant API as Codex API
participant Storage
Client->>AccountMgr: Fetch account limits
AccountMgr->>AccountMgr: Setup AbortController with timeout
AccountMgr->>API: Request usage data
alt Timeout occurs
API--xAccountMgr: Request aborted
AccountMgr->>AccountMgr: Clear timeout, throw timeout error
else Success
API-->>AccountMgr: Usage response
AccountMgr->>AccountMgr: Clear timeout
end
Client->>AccountMgr: Handle token rotation
AccountMgr->>Storage: Get duplicate accounts with old token
AccountMgr->>AccountMgr: Identify all accounts sharing refreshToken
AccountMgr->>AccountMgr: Update credentials for matching accounts
AccountMgr->>Storage: Save updated accounts
AccountMgr->>AccountMgr: Invalidate account manager cache
AccountMgr->>Client: Return refreshed state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (previousRefreshToken && refreshResult.refresh !== previousRefreshToken) { | ||
| for (const storedAccount of storage.accounts) { | ||
| if (storedAccount?.refreshToken === previousRefreshToken) { | ||
| applyRefreshedCredentials(storedAccount); | ||
| } | ||
| } | ||
| } else { | ||
| applyRefreshedCredentials(account); | ||
| } |
There was a problem hiding this comment.
Stale credentials persist for non-rotated refreshes
when the server issues a fresh accessToken but keeps the same refreshToken (no rotation), the else branch only updates the single account object in the current iteration. duplicate entries in storage.accounts that share previousRefreshToken are skipped by deduplication and never receive the new accessToken / expiresAt. those stale values are then written back to disk via saveAccounts, so any code path outside this tool that reads those duplicate account objects will still see an expired token.
the fan-out propagation already implemented in the rotation branch should apply unconditionally whenever previousRefreshToken is set, regardless of whether the token was rotated:
if (previousRefreshToken) {
for (const storedAccount of storage.accounts) {
if (storedAccount?.refreshToken === previousRefreshToken) {
applyRefreshedCredentials(storedAccount);
}
}
} else {
applyRefreshedCredentials(account);
}
this also closes a windows token-leakage window: stale tokens sitting on disk can be picked up by an antivirus scanner or a concurrent read before saveAccounts replaces them.
Prompt To Fix With AI
This is a comment left during a code review.
Path: index.ts
Line: 4390-4398
Comment:
**Stale credentials persist for non-rotated refreshes**
when the server issues a fresh `accessToken` but keeps the same `refreshToken` (no rotation), the `else` branch only updates the single `account` object in the current iteration. duplicate entries in `storage.accounts` that share `previousRefreshToken` are skipped by deduplication and never receive the new `accessToken` / `expiresAt`. those stale values are then written back to disk via `saveAccounts`, so any code path outside this tool that reads those duplicate account objects will still see an expired token.
the fan-out propagation already implemented in the rotation branch should apply unconditionally whenever `previousRefreshToken` is set, regardless of whether the token was rotated:
```
if (previousRefreshToken) {
for (const storedAccount of storage.accounts) {
if (storedAccount?.refreshToken === previousRefreshToken) {
applyRefreshedCredentials(storedAccount);
}
}
} else {
applyRefreshedCredentials(account);
}
```
this also closes a windows token-leakage window: stale tokens sitting on disk can be picked up by an antivirus scanner or a concurrent read before `saveAccounts` replaces them.
How can I resolve this? If you propose a fix, please make it concise.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| if (previousRefreshToken && refreshResult.refresh !== previousRefreshToken) { | ||
| for (const storedAccount of storage.accounts) { | ||
| if (storedAccount?.refreshToken === previousRefreshToken) { | ||
| applyRefreshedCredentials(storedAccount); | ||
| } | ||
| } | ||
| } else { | ||
| applyRefreshedCredentials(account); | ||
| } |
There was a problem hiding this comment.
When refresh tokens do NOT rotate, duplicate accounts are not updated with the new accessToken and expiresAt.
If the refresh succeeds but returns the same refresh token (line 4390 condition is false), only the current account object is updated (line 4397). However, other accounts in storage that share the same refresh token will retain their old expired accessToken and expiresAt values.
Fix by always updating all matching accounts:
if (previousRefreshToken && refreshResult.refresh !== previousRefreshToken) {
// Token rotated - update all accounts with old token
for (const storedAccount of storage.accounts) {
if (storedAccount?.refreshToken === previousRefreshToken) {
applyRefreshedCredentials(storedAccount);
}
}
} else {
// Token didn't rotate - still need to update all accounts with same token
const currentToken = refreshResult.refresh;
for (const storedAccount of storage.accounts) {
if (storedAccount?.refreshToken === currentToken) {
applyRefreshedCredentials(storedAccount);
}
}
}| if (previousRefreshToken && refreshResult.refresh !== previousRefreshToken) { | |
| for (const storedAccount of storage.accounts) { | |
| if (storedAccount?.refreshToken === previousRefreshToken) { | |
| applyRefreshedCredentials(storedAccount); | |
| } | |
| } | |
| } else { | |
| applyRefreshedCredentials(account); | |
| } | |
| if (previousRefreshToken && refreshResult.refresh !== previousRefreshToken) { | |
| // Token rotated - update all accounts with old token | |
| for (const storedAccount of storage.accounts) { | |
| if (storedAccount?.refreshToken === previousRefreshToken) { | |
| applyRefreshedCredentials(storedAccount); | |
| } | |
| } | |
| } else { | |
| // Token didn't rotate - still need to update all accounts with same token | |
| const currentToken = refreshResult.refresh; | |
| for (const storedAccount of storage.accounts) { | |
| if (storedAccount?.refreshToken === currentToken) { | |
| applyRefreshedCredentials(storedAccount); | |
| } | |
| } | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
1 participant
Summary
codex-limitsusage fetching with a timeoutContext
This PR supersedes #70.
I accidentally pushed the fix from #70 to
main, reverted that accidental push in6e18b35, and reapplied the intended change on top of currentmaininsupersede/pr70-codex-limitsso it can be reviewed normally here.Validation
npm run lintnpm run typechecknpm testSummary by CodeRabbit
Bug Fixes
Refactor
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 hardens the
codex-limitstool with three targeted fixes: a configurable fetch timeout viaAbortController, correct active-account labeling across deduplicated refresh-token views, and fan-out propagation of rotated credentials to all duplicate stored accounts. the changes are well-motivated and the new regression tests cover the main scenarios.key changes:
fetchUsagenow wraps every network call in anAbortControllertimeout sourced fromgetFetchTimeoutMs(loadPluginConfig())refreshTokento a string and treats falsy/empty tokens as non-deduplicable (each missing-token account appears separately)activeRefreshTokenis captured before the loop so that the[active]marker follows the credential identity, not just the array indexapplyRefreshedCredentialsfans out to every account sharingpreviousRefreshTokenin storageinvalidateAccountManagerCache()issues found:
applyRefreshedCredentialsonly fires whenrefreshResult.refresh !== previousRefreshToken(rotation). when a non-rotating refresh occurs (newaccessToken, samerefreshToken), duplicate accounts still get their staleaccessToken/expiresAtwritten back to disk viasaveAccounts, which can cause failures in code paths that access those duplicates directlyAbortControllerabort path — a regression in signal wiring won't be caught, especially on windows where timer behavior under load differssaveAccounts→invalidateAccountManagerCacheis not atomic; antivirus file locks or concurrentexecute()calls on windows can read stale tokens between the two operations; no in-code comment documents the redaction strategy forrefreshResult.refresh/refreshResult.accessin error pathsConfidence Score: 3/5
elsebranch inapplyRefreshedCredentialsleaves duplicate accounts with stale access tokens on disk after a non-rotating refresh — a real (if limited-blast-radius) logic bug. the missing timeout test and lack of windows concurrency commentary (per repo policy) also hold this back from a higher score.Important Files Changed
Sequence Diagram
sequenceDiagram participant E as execute() participant S as storage.accounts participant Q as queuedRefresh participant F as fetch (CODEX_BASE_URL) participant D as disk (saveAccounts) E->>S: loadAccounts() E->>E: deduplicate by refreshToken (skip empty tokens) E->>E: capture activeRefreshToken loop for each uniqueIndex i E->>S: read account[i] alt accessToken missing or expired E->>Q: queuedRefresh(account.refreshToken) Q-->>E: {type:"success", access, refresh, expires} alt refresh rotated (new != old) E->>S: fan-out applyRefreshedCredentials<br/>to ALL accounts with old refreshToken else no rotation E->>S: applyRefreshedCredentials(account[i] only) Note over S: ⚠️ duplicate accounts<br/>retain stale accessToken end end E->>F: GET /wham/usage (with AbortController timeout) alt timeout fires F-->>E: AbortError → "timed out after Xms" else response ok F-->>E: UsagePayload JSON end E->>E: format output lines end alt storageChanged E->>D: saveAccounts(storage) E->>E: invalidateAccountManagerCache() Note over D,E: ⚠️ not atomic on Windows endLast reviewed commit: bb446e5
Context used: