forked from NousResearch/hermes-agent
-
Notifications
You must be signed in to change notification settings - Fork 0
fix(auth): serialize Codex OAuth pool refresh under the auth-store lock #240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟢 Codex pool refresh computes timeout but does not propagate it to refresh_codex_oauth_pure (bug)
In
agent/credential_pool.py, the new Codex OAuth lock path in_refresh_entryreadsHERMES_CODEX_REFRESH_TIMEOUT_SECONDSto computerefresh_timeout_seconds(line 976) and correctly sizes the lock timeout torefresh_timeout_seconds + 5(line 979-982). However, it calls_refresh_entry_impl(line 989) without passing this timeout through. The_refresh_entry_implmethod (line 992) does not accept a timeout parameter, and its openai-codex branch (line 1030) callsauth_mod.refresh_codex_oauth_pure()using the defaulttimeout_seconds=20.0. The singleton refresh path inhermes_cli/auth.py(resolve_codex_runtime_credentials→_refresh_codex_auth_tokens) correctly passestimeout_secondsthrough. When a user increasesHERMES_CODEX_REFRESH_TIMEOUT_SECONDSfor slow networks, the pool-held lock waits for the full extended duration but the HTTP call still times out at 20s, defeating the purpose of the configuration.💡 Suggestion: Thread
refresh_timeout_secondsthrough to_refresh_entry_impland on torefresh_codex_oauth_pure. Add a*, timeout_seconds: float = 20.0parameter to_refresh_entry_impland pass it to therefresh_codex_oauth_purecall at line 1030. Update both call sites (lines 989 and 990) to pass the appropriate timeout value.📋 Prompt for AI Agents
In agent/credential_pool.py: (1) At line 989, change
return self._refresh_entry_impl(entry, force=force)toreturn self._refresh_entry_impl(entry, force=force, timeout_seconds=refresh_timeout_seconds). (2) At line 992, change the signature todef _refresh_entry_impl(self, entry: PooledCredential, *, force: bool, timeout_seconds: float = 20.0) -> Optional[PooledCredential]:. (3) At line 1030-1032, changerefreshed = auth_mod.refresh_codex_oauth_pure(entry.access_token, entry.refresh_token,)torefreshed = auth_mod.refresh_codex_oauth_pure(entry.access_token, entry.refresh_token, timeout_seconds=timeout_seconds,). (4) Line 990's non-codex fallthrough call already passes the default 20.0 which is correct for other providers.