fix(auth): coordinate refresh across CLI processes - #1457
jeffpignataro wants to merge 3 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Aspect Workflows Tasks📅 Tue Sep 22 18:10:23 UTC 2026 Task Results
⏱ Last updated Tue Sep 22 18:17:05 UTC 2026 · 📊 GitHub API quota 1,598/7,700 (21% used, resets in 18m) |
![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: 392167c308
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let worker_grants = grants.clone(); | ||
| let worker_stop = stop.clone(); | ||
| let worker_bearer = bearer.clone(); | ||
| let thread = std::thread::spawn(move || { |
There was a problem hiding this comment.
i don't quite understand the use of thread here if this code meant to be single threaded.
There was a problem hiding this comment.
The thread isn't there to serve requests in parallel. It allows the main thread to keep going while a connection is parked. I'll adjust the wording though.
🤖 details:
wait_for_grant() returns once child #1's grant reaches the issuer, and the issuer then blocks on release_rx holding that connection open. Meanwhile the main thread spawns the second helper and the AXL child, which is exactly the window the test needs: both competing processes hit the locked refresh transaction while #1's grant is still in flight, and their requests queue on the listener. Without coordination they'd submit the old refresh token and the issuer would 401 them — that's the assertion grants == 1 is protecting.
On one thread you'd need a state machine interleaving accept polling with Command::spawn to get the same interleaving. I'll move the rationale up to the spawn so it's not buried in the loop body.
…g failures The rationale for the loopback issuer's background thread sat inside the loop body and only covered why responses are served serially, leaving the `thread::spawn` itself unexplained. Move it above the spawn and say what the thread is actually for: the test drives the other half of the exchange, holding the first grant open while it starts the competing processes. `Drop` also re-raised a worker panic through `join().unwrap()`. That drop runs while unwinding from the test's own failure, so the second panic aborted the process and the original assertion went unreported. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015XYrr1YGB4TDzf3nfP4c9K
This branch has not been deployed
2 participants
When multiple CLI processes encounter an expired bearer, they can submit the same single-use refresh token. One refresh succeeds; the others receive a rejection and report that the session expired. This can make a normal bearer expiration look like a requirement to log in again.
Coordinate refresh through one transaction shared by MCP, the Bazel credential helper, and AXL credential lookup. Waiting callers re-read the store and adopt the replacement token. A store-wide advisory lock also protects login, logout, and deployment removal from lost updates to the shared credential map. File-backed credentials use private temporary files and atomic replacement.
Lock acquisition is bounded to 60 seconds and automatic refresh to 30 seconds. Refresh rejection is distinguished from transient failures, which advise retrying and preserve the stored credentials; raw token-endpoint response bodies are not exposed.
On Unix, keyring locks use a private, ownership-checked directory keyed by the effective OS user ID, independent of
HOME,TMPDIR, andXDG_RUNTIME_DIR. Windows uses the OS known-folder API.Validation
cargo test -p axl-runtime --lib engine::aspect::: 132 passed.cargo test -p aspect-cli --test auth_refresh: 3 passed. Separate credential-helper and AXL processes share exactly one grant; logout is not overwritten by a pending refresh; failed refreshes preserve credentials.v2026.37.6with the originalsession expiredmessage.cargo fmt --all --check, BUILD formatting, andgit diff --checkpassed.bazel test //crates/aspect-cli:auth_refresh_test --test_output=errors: passed at392167c3(final test result reused from cache).bazel test //crates/axl-runtime:test --test_filter=engine::aspect::credential_store:: --test_output=errors: passed ata01a773b.Scope and rollout
Tests use synthetic credentials and a loopback issuer. Live keychain integration and Frontegg session policies were not tested. This addresses the reproducible refresh race; it does not establish the cause of a particular 24-hour logout or extend provider session lifetimes.
Restart long-running MCP processes after upgrading: older binaries do not participate in the lock. Independently copied credential stores remain separate and cannot safely share a rotating refresh token.