perf(codex): single-flight CodexAuthStore refresh - #168
Conversation
Concurrent requests hitting an expired ChatGPT credential each raced their own refresh call, causing redundant upstream token exchanges and risking a cancelled caller stranding a rotated refresh token before writeback. Guard refresh_and_write_back with a process-wide async mutex and detach the refresh+writeback sequence onto a spawned task so cancellation of one waiter's future can't interrupt another's in-flight refresh; waiters that lose the race simply re-read the credential persisted by the winner.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a single-flight mechanism for ChatGPT OAuth token refreshes using a static Mutex and detached background tasks to ensure cancellation safety during token rotation and writeback. The reviewer feedback points out a concurrency issue in force_refresh where multiple concurrent calls can cause redundant sequential refreshes, and suggests implementing a double-checked locking pattern to prevent this.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a path-keyed single-flight registry to serialize concurrent ChatGPT OAuth token refreshes for the same credential file, while allowing different files to refresh in parallel. It also wraps the refresh and writeback sequence in a detached tokio::spawn task to ensure cancellation safety. The review feedback points out a medium-severity issue where token refresh errors inside the spawned task could be silently lost if the caller's future is cancelled before completion, and suggests wrapping the sequence in an async block to guarantee all errors are logged.
|
/gemini review |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Code Review
This pull request introduces a single-flight mechanism for ChatGPT OAuth token refreshes in CodexAuthStore to prevent concurrent redundant refreshes for the same credential file while allowing parallel refreshes for different accounts. It also ensures that token writebacks are run in detached tasks so they are not lost if the caller's future is cancelled, and adds a conditional force-refresh method to skip rotation if another request has already updated the token. Extensive concurrent and cancellation tests are added to verify these behaviors. No review comments were provided, so there is no additional feedback.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces path-keyed single-flighting for ChatGPT OAuth token refreshes in CodexAuthStore to serialize concurrent refreshes of the same credential file while allowing different accounts to refresh in parallel. It also ensures that the refresh and writeback sequence runs in a detached task so that dropping the caller's future does not lose a rotated token. Feedback was provided regarding the synchronous path.canonicalize() call inside normalized_auth_path which is executed within an async context and could block the async executor thread.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements single-flighting and path normalization for ChatGPT OAuth token refreshes in CodexAuthStore to prevent redundant rotations and handle request cancellations safely. The review feedback highlights a potential panic in chatgpt_access_token when encountering non-OAuth credentials, suggesting to return an Option<&str> and handle it with a safe fallback at the call sites.
|
/gemini review |
There was a problem hiding this comment.
Code Review
본 풀 리퀘스트는 ChatGPT OAuth 토큰 갱신 메커니즘의 취소 안전성(cancellation safety)과 단일 실행(single-flight) 보장을 개선합니다. 동일한 자격 증명 파일에 대한 동시 갱신을 직렬화하기 위해 경로별 단일 실행 레지스트리를 도입하고, 호출자의 퓨처가 취소되더라도 토큰 갱신 및 파일 쓰기 작업이 완료되도록 tokio::spawn을 통해 비동기 작업을 분리했습니다. 리뷰어의 피드백은 이미 구현된 변경 사항을 중복 제안하고 있어 제외하였으며, 이에 따라 추가로 제공할 피드백은 없습니다.
Greptile SummaryThis PR adds single-flight coalescing to
Confidence Score: 5/5Safe to merge — the single-flight logic is correct, lock ordering is consistent, and the cancellation-safety guarantee is validated by tests. The core invariants all hold: REFRESH_LOCKS serializes concurrent refreshes for the same credential file, the check-lock-check pattern in get_valid_chatgpt and force_refresh_if_access_token ensures waiters re-read the winner's persisted credential rather than issuing a redundant exchange, and the detached spawn correctly owns the OwnedMutexGuard through writeback so task cancellation cannot expose a half-rotated token. All new behavior paths are covered by wiremock-backed tests. The two P2 observations are clarifications rather than correctness defects. No files require special attention — the changes are self-contained to auth/codex/auth.rs and the pool/inbound adapter helpers. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant A as Caller A (get_valid_chatgpt)
participant B as Caller B (get_valid_chatgpt)
participant RL as REFRESH_LOCKS[path]
participant Auth as Auth File
participant UP as OAuth Upstream
A->>Auth: read (before lock) → expired
B->>Auth: read (before lock) → expired
A->>RL: lock_owned() → acquired
B->>RL: lock_owned() → blocks
A->>Auth: read (after lock) → still expired
A->>UP: spawn detached task: refresh_tokens()
Note over A,UP: Task holds OwnedMutexGuard<()>
UP-->>A: new access_token + refresh_token
A->>Auth: write_refreshed_auth (atomic)
A->>RL: drop guard (task complete)
B->>RL: lock_owned() → acquired
B->>Auth: read (after lock) → new token valid!
B-->>B: return cached credential (no upstream call)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant A as Caller A (get_valid_chatgpt)
participant B as Caller B (get_valid_chatgpt)
participant RL as REFRESH_LOCKS[path]
participant Auth as Auth File
participant UP as OAuth Upstream
A->>Auth: read (before lock) → expired
B->>Auth: read (before lock) → expired
A->>RL: lock_owned() → acquired
B->>RL: lock_owned() → blocks
A->>Auth: read (after lock) → still expired
A->>UP: spawn detached task: refresh_tokens()
Note over A,UP: Task holds OwnedMutexGuard<()>
UP-->>A: new access_token + refresh_token
A->>Auth: write_refreshed_auth (atomic)
A->>RL: drop guard (task complete)
B->>RL: lock_owned() → acquired
B->>Auth: read (after lock) → new token valid!
B-->>B: return cached credential (no upstream call)
Reviews (2): Last reviewed commit: "fix(codex): handle unexpected refresh cr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a single-flight registry for ChatGPT OAuth token refreshes to serialize concurrent refreshes of the same credential file while allowing parallel refreshes for different accounts. It also adds robust tests to verify concurrent behavior, path normalization, and cancellation safety. The feedback points out a compilation error in normalized_auth_path where a closure is incorrectly passed to OnceCell::get_or_try_init instead of a Future directly, which must be resolved to compile successfully.
|
1 participant
Summary
Applies single-flight coalescing to
CodexAuthStore::refresh_and_write_backso concurrent requests hitting an expired ChatGPT credential no longer each race their own upstream token refresh. Refresh + writeback is detached onto a spawned task guarded by a process-wide async mutex, so a cancelled waiter can't interrupt another waiter's in-flight refresh; waiters that lose the race simply re-read the credential persisted by the winner. Adds a concurrent-refresh test covering this path.Milestone / spec
N/A — internal performance/correctness hardening of the existing Codex auth refresh path (no spec deviation).
Checklist
cargo buildpassescargo testpasses (new behavior is covered; tests run without network/loopback where possible)cargo clippy --all-targets -- -D warningscleancargo fmt --all --checkcleansrc/auth/codex/auth.rswas already over 500 lines before this change; not introduced by this PRdocs/updated if this change deviates from it — N/A, no spec deviationNotes for reviewers
Please look closely at the credential-handling path: the spawned refresh task must complete writeback even if the original caller is cancelled, and losing waiters must correctly re-read the winner's persisted credential rather than retrying their own refresh.
Related Issues
No linked issue.
Summary by cubic
Add single-flight coalescing to Codex ChatGPT OAuth refresh with a canonicalized, path-keyed lock to eliminate redundant token exchanges. Refresh + writeback runs in a detached, cancellation-safe task; paths are canonicalized for both locking and persistence, and different accounts refresh in parallel.
chatgpt_access_token(); if not ChatGPT, cooldown the account and warn instead of refreshing.Written for commit a99c3c1. Summary will update on new commits.