Attempt to reload auth as a step in 401 recovery#8880
Conversation
…cess sync - Add AuthManager::auth_cached and move stale-token refresh logic into AuthManager - Make CodexAuth::get_token/get_token_data synchronous and remove CodexAuth::refresh_token - Change auth_provider_from_auth and BackendClient::from_auth to non-async - Replace direct CodexAuth usage with AuthManager where appropriate and use cached auth for UI/status
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56598f3193
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Tidy up auth recovery code: - Fix line formatting in UnauthorizedRecovery::has_next - Simplify Arc::clone usage in AuthManager::unauthorized_recovery - Use method pointer for unauthorized_recovery mapping in ModelClient - Reformat conditional let in handle_unauthorized and drop redundant return
Introduce ReloadOutcome and expected_account_id tracking in UnauthorizedRecovery to conditionally skip reloading auth when the on-disk account id differs from the cached one. Refactor AuthManager reload logic into load_auth_from_storage and store_auth helpers, add reload_if_account_id_matches with account id validation, and preserve cached auth when reload is skipped. Add a test to verify that recovery
…ad-auth-as-a-step-in-401-recovery # Conflicts: # codex-rs/core/src/client.rs
| } | ||
| } | ||
| UnauthorizedRecoveryStep::RefreshToken => { | ||
| self.manager.refresh_token().await?; |
There was a problem hiding this comment.
In this path, should we should avoid writing the refreshed token information back to the credential store? This will happen if the user has subsequently logged out or logged in to a different account. Overwriting that subsequent login info feels wrong. Then again, if we don't overwrite it, other instances that were created with the old token will fail if they later try to refresh their tokens.
- Log in with account A to instance 1
- Log in with account A to instance 2
- Log in with account B to instance 3
- Instance 1 gets 401 and refreshes its token. Do we write it back to auth.json? If we do, then instance 2 will see the refreshed token (good) but the next instance launched will use account A rather than B (bad). If we don't, then instance 2 will not see the refreshed token (bad).
Hmm, I don't see a way to handle all of these cases without persisting token state for multiple accounts, which gets even more complex.
There was a problem hiding this comment.
Chatted offline, decided to proceed with the status quo for now.
Add high-level docs for UnauthorizedRecovery state machine behavior. Rename store_auth() to set_auth() and use it consistently in reload paths. Simplify refresh_token() by using a single auth_cached() call.
pakrym-oai
deleted the
pakrym/attempt-to-reload-auth-as-a-step-in-401-recovery
branch
2 participants
When authentication fails, first attempt to reload the auth from file and then attempt to refresh it.