Skip to content

fix(auth): serialize concurrent xAI OAuth write-through to global root - #47460

Closed
sprmn24 wants to merge 1 commit into
NousResearch:mainfrom
sprmn24:fix/xai-oauth-write-through-race
Closed

fix(auth): serialize concurrent xAI OAuth write-through to global root#47460
sprmn24 wants to merge 1 commit into
NousResearch:mainfrom
sprmn24:fix/xai-oauth-write-through-race

Conversation

@sprmn24

@sprmn24 sprmn24 commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

_write_through_xai_oauth_to_global_root() syncs a freshly-refreshed xAI OAuth token set from a profile store back to the global root auth.json. The function held the profile-scoped file lock while doing so, but performed the read-modify-write on the global auth.json without acquiring any lock on that file.

When two profiles refresh their xAI tokens concurrently:

  1. Profile A and Profile B each read the global auth.json (both see refresh_token_old)
  2. Profile A writes refresh_token_A → global file
  3. Profile B writes refresh_token_B → global file (overwrites A's token)
  4. Profile A's in-memory state has refresh_token_A but the file now has refresh_token_B
  5. Next refresh attempt by Profile A: invalid_grant → forced re-authentication

Related Issue

Adjacent to the already-fixed concurrent refresh race in #10147 / #15120 (Nous OAuth). Same class of bug, new code path.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • 🔒 Security fix

Changes Made

  • hermes_cli/auth.py: Added _global_root_lock_holder = threading.local() (separate from _auth_lock_holder to prevent _file_lock's reentrancy depth counter from silently skipping the global lock when called from within a profile lock). Wrapped the entire read-modify-write in _write_through_xai_oauth_to_global_root() inside _file_lock(global_path.with_suffix(".lock"), _global_root_lock_holder, AUTH_LOCK_TIMEOUT_SECONDS, ...), matching the .lock sidecar convention used by _auth_lock_path().

  • tests/hermes_cli/test_xai_oauth_writethrough.py: Added test_concurrent_write_through_does_not_lose_second_token — fires two threads simultaneously via threading.Barrier(2), asserts the final global auth.json holds exactly one complete token set (not a corrupted mix of access_token from A and refresh_token from B).

How to Test

pytest tests/hermes_cli/test_xai_oauth_writethrough.py -v

All 5 tests pass.

Checklist

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes
  • I've tested on my platform: Windows 11 + WSL2 Ubuntu

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard provider/xai xAI (Grok) area/auth Authentication, OAuth, credential pools labels Jun 16, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for targeting a real current-main race in the xAI root write-through path.

Problems

  • The new lock covers only hermes_cli/auth.py:4041-4054. The credential-pool companion path still updates the same global file without a lock at agent/credential_pool.py:529-537, and calls it for xai-oauth at agent/credential_pool.py:975-995. A pool refresh can still race this path.
  • The added assertion at tests/hermes_cli/test_xai_oauth_writethrough.py:229 accepts either complete final token set. Since _save_auth_store already atomically replaces a complete JSON payload (hermes_cli/auth.py:1137-1153), the unlocked last-writer-wins implementation also satisfies that assertion.

Suggested changes

  • Share one global-root auth-store locking helper between both write-through paths.
  • Add a deterministic cross-path concurrency test that proves independent root-store updates are preserved, or directly instruments critical-section exclusion.

Automated hermes-sweeper review.

Comment thread hermes_cli/auth.py
return
_store_provider_state(global_store, "xai-oauth", dict(state), set_active=False)
_save_auth_store(global_store, global_path)
with _file_lock(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lock protects only the non-pool helper. agent/credential_pool.py:529-537 performs the same global-root read-modify-write for xai-oauth without taking this sidecar lock, so a pool refresh can still race this code. Please share the global-root locking mechanism with that helper.


token_a = {"access_token": "token-A-access", "refresh_token": "token-A-refresh"}
token_b = {"access_token": "token-B-access", "refresh_token": "token-B-refresh"}
assert final_tokens == token_a or final_tokens == token_b, (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This accepts the pre-fix last-writer-wins result as well: _save_auth_store already atomically replaces a complete JSON payload. Please make the test observe mutual exclusion or assert preservation of independent concurrent root-store updates.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 14, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing as superseded: PR #65656 (salvage of #65264 by @dschnurbusch, merged today) generalized this fix — global-root write-through is now serialized under a lock keyed to the target store's canonical path via _persist_provider_state_to_store(), covering the xAI path this PR addressed plus the Codex/credential-pool sibling sites, and fixing the shared-threading.local reentrancy mis-keying that a single _global_root_lock_holder would have carried. You spotted this lost-update window first — thanks for the report and the fix; the merged version credits the same bug class.

@teknium1 teknium1 closed this Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists provider/xai xAI (Grok) sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants