Fix cross-process CLI token refresh races

[brynary]

Summary

• serialize CLI OAuth refreshes across processes with an advisory sidecar lock
• re-read the auth store after acquiring the lock and adopt tokens refreshed by another CLI
• keep blocking file-lock waits off Tokio runtime workers

Tests

• cargo nextest run -p fabro-client
• cargo nextest run -p fabro-cli auth_login_refresh_logout_flow
• cargo +nightly-2026-04-14 clippy -p fabro-client --all-targets -- -D warnings
• cargo +nightly-2026-04-14 fmt --check --all
• cargo build --workspace

[Copilot]

Pull request overview

This PR addresses cross-process races during CLI OAuth token refresh by introducing an advisory, cross-process lock to serialize refresh-token rotation and by adopting tokens refreshed by another concurrent CLI process.

Changes:

• Add a cross-process “refresh” sidecar lock (*.refresh.lock) in AuthStore, acquired via spawn_blocking to keep blocking waits off Tokio worker threads.
• In the client refresh path, acquire the refresh lock before rotating tokens and re-read the auth store to adopt tokens refreshed by another process.
• Add a concurrency test ensuring two clients only perform a single refresh call for rotating refresh tokens.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
lib/foundation/fabro-client/src/client.rs	Serializes refresh-token rotation across processes and adopts refreshed tokens from the auth store; adds a concurrent refresh test.
lib/foundation/fabro-client/src/auth_store.rs	Implements an advisory cross-process refresh lock using a sidecar lockfile and spawn_blocking.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[brynary]

Ran a simplification pass over this branch (69976fc3).

The cross-process refresh lock had introduced a third copy of the same open-file → try-lock → block-on-contention sequence already present in with_shared_lock and with_exclusive_lock. Collapsed all three into one open_locked_file helper parameterized by a LockMode enum, which let open_lock_file and lock_error go away. Net 20 lines lighter in auth_store.rs despite adding the helper and docs.

Two smaller things:

• Lock calls are now qualified as FileExt:: calls throughout. std::fs::File has inherent locking methods with different return types, and inherent methods take precedence over trait methods, so the previous mix of FileExt::try_lock_exclusive(&f) and f.lock_exclusive() was resolving to two different implementations.
• acquire_refresh_lock has one signature on all platforms now. It previously returned Result<(), _> on non-Unix and Result<RefreshLockGuard, _> on Unix; RefreshLockGuard is defined for non-Unix too.

Behavior is unchanged. The try-then-block sequence is preserved inside the shared helper — it is deliberate (added in 6ac8bf6 for filesystem-lock error classification) and now documented, since only genuine contention reports WouldBlock while a filesystem that cannot lock reports EOPNOTSUPP/ENOLCK immediately.

Considered and rejected:

• Dropping the in-process refresh_lock mutex. The file lock does subsume it for correctness, since flock conflicts across distinct fds even within one process. But the mutex is a useful fast path: when several in-flight requests on one client get a 401 together, the ones that lose the mutex return on the in-memory token check without any filesystem work.
• Folding auth.refresh.lock into the existing auth.lock. Holding the auth lock across the refresh request would block unrelated shared-lock readers such as fabro auth status for up to the 30s request timeout. Two locks with the documented one-way ordering is the right call.
• A timeout on the lock wait. Hold time is already bounded by DEFAULT_CONTROL_PLANE_REQUEST_TIMEOUT (30s), and flock releases if the holder dies.

Verification: cargo nextest run -p fabro-client (40 passed), cargo nextest run -p fabro-cli auth_login_refresh_logout_flow (2 passed), cargo build --workspace, cargo +nightly-2026-04-14 clippy -p fabro-client --all-targets -- -D warnings, cargo +nightly-2026-04-14 fmt --check --all.

One caveat: the non-Unix branches are not compiled anywhere. CI is Linux and macOS only, and no Windows target is installed locally, so that change is unverified by compilation.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

lib/foundation/fabro-client/src/auth_store.rs:112

• The new LockError::Task message says “auth store lock”, but this error is emitted while waiting for the refresh sidecar lock (*.refresh.lock). This is confusing when debugging lock contention.

    #[error("failed to wait for auth store lock at {path}: {source}")]

lib/foundation/fabro-client/src/auth_store.rs:231

• On non-Unix targets, acquire_refresh_lock() returns UnsupportedPlatform, but Client::refresh_access_token() now calls it unconditionally. This can break the dev-token “adopt updated token from auth store” path (and yields a misleading OAuth-login error) even though reading the auth store is still supported on non-Unix.

    #[cfg(not(unix))]
    pub(crate) async fn acquire_refresh_lock(&self) -> Result<RefreshLockGuard, AuthStoreError> {
        Err(AuthStoreError::UnsupportedPlatform)
    }

[brynary]

Follow-up (58c8df59). Two review passes landed after my first comment and turned up a correctness bug plus several things worth tightening.

The staleness check could install an already-expired token. oauth_entry.access_token != failed_access_token treated "different from the token that failed" as "usable". A long-lived process (say fabro run streaming) could read an entry a sibling rotated an hour earlier whose access token had since expired, install it, and return Ok. The retry at client.rs:414-420 is single-shot and does not refresh again, so that surfaced a 401 to the user. Before this PR the same situation rotated normally and succeeded.

Fixed by requiring the stored token to be unexpired; an expired one falls through and rotates with the refresh token just read. Worth stressing that this check is an optimization, not the safety property — the lock plus the fresh read is what makes single-use rotation safe. Without the check the loser would rotate with the new refresh token it just read and still succeed, just burning one extra rotation. So it should not cost correctness.

Added refresh_access_token_rotates_when_the_stored_token_is_also_expired. I verified it is a real regression test: with the fix reverted it fails with "No request has been received by the mock server", because the client adopts the stale token instead of rotating.

Also in this commit:

• Non-Unix acquire_refresh_lock is now a no-op passthrough, matching with_shared_lock/with_exclusive_lock off Unix. Returning UnsupportedPlatform there broke re-installing a stored dev token, which needs no lock because it never writes. Restores pre-PR behavior on that path.
• Client::refresh_lock renamed to local_refresh_lock. The in-process mutex and the cross-process acquire_refresh_lock were sharing one word four lines apart.
• LockError::Task gated on Unix, where its only construction site is, letting the two tokio::task imports share one gate.
• The concurrency test now uses a no-proxy transport connector. Building clients without one routes through connect_target_transport, which does not call .no_proxy(), so the test was creating four proxy-discovering reqwest clients against localhost — contrary to the rule in CLAUDE.md, which warns it shows up as misleading nextest timeouts on macOS. The fabro-client suite went from 0.245s to 0.218s with 41 tests instead of 40.
• The test now asserts the rotated refresh token reaches the store. That it persisted, rather than just that both clients ended up with the new access token, is the actual invariant behind single-use rotation.

Two things I deliberately did not change:

• No ceiling on the flock wait, and no user-visible signal while waiting. Three terminals expiring together can sit ~90s printing nothing, and tracing::warn! would not help since the CLI routes logs to ~/.fabro/logs/cli.log rather than stderr. A fix means a deadline poll loop plus a new error variant, and the deadline has to live inside the blocking closure — tokio::time::timeout around spawn_blocking does not work, because the task cannot be aborted, so the parked thread would acquire the lock after the caller gave up and hand it to a dead caller. That is a design call for this PR rather than cleanup, so I left it. Happy to add it if you want it before merge.
• A cached HTTP client for the refresh POST. build_public_http_client() builds a fresh client per refresh, inside the lock, including root-cert load and a cold handshake. At one refresh per ten minutes a pooled connection would be idle-reaped anyway.

Verification: cargo nextest run -p fabro-client (41 passed), cargo nextest run -p fabro-cli auth_login_refresh_logout_flow (2 passed), cargo build --workspace, clippy with -D warnings, fmt --check --all.

The non-Unix caveat from my previous comment still applies: those branches compile nowhere, so the passthrough change is unverified.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

lib/foundation/fabro-client/src/auth_store.rs:283

• Same issue as with_shared_lock: with_exclusive_lock should also ensure the parent directory exists before opening/creating the lock file. Otherwise first-time writes to a new FABRO_AUTH_FILE location can fail earlier than intended when trying to open *.lock.

    #[cfg(unix)]
    fn with_exclusive_lock<T>(
        &self,
        f: impl FnOnce() -> Result<T, AuthStoreError>,
    ) -> Result<T, AuthStoreError> {
        let _lock = open_locked_file(self.lock_path(), LockMode::Exclusive)?;
        f()
    }

lib/foundation/fabro-client/src/auth_store.rs:266

• with_shared_lock opens/creates the lock file but does not ensure the auth store parent directory exists first. If FABRO_AUTH_FILE points at a path in a not-yet-created directory, read-only commands like fabro auth status/logout can fail with ENOENT when opening *.lock, even though a missing auth store should behave like “empty”.

This issue also appears on line 276 of the same file.

    fn with_shared_lock<T>(
        &self,
        f: impl FnOnce() -> Result<T, AuthStoreError>,
    ) -> Result<T, AuthStoreError> {
        let _lock = open_locked_file(self.lock_path(), LockMode::Shared)?;
        f()
    }