Skip to content

fix(auth): coordinate OAuth2 refresh across processes - #103

Open
pddkhanh wants to merge 2 commits into
xdevplatform:mainfrom
pddkhanh:fix/shared-oauth-refresh
Open

pddkhanh wants to merge 2 commits into
xdevplatform:mainfrom
pddkhanh:fix/shared-oauth-refresh

Conversation

@pddkhanh

@pddkhanh pddkhanh commented Oct 14, 2026 •

Copy link
Copy Markdown

When multiple xurl mcp processes share one login on the same machine, each keeps its own cached token-store snapshot. If the provider rotates a refresh token, two processes can redeem the same old grant: one succeeds, another fails and may enter browser login at startup.

Closes #102.

Change

  • Hold a cross-process sidecar file lock through token reload, refresh, and persistence. Unix uses flock; Windows uses LockFileEx.
  • Reload the shared store inside the lock, so a waiting process reuses the replacement grant instead of redeeming the old one.
  • Reuse a sibling's replacement access token on forced refresh, while still refreshing a replacement that is already expired.
  • Replace token-store files atomically so concurrent readers do not see truncated YAML. A removed store does not resurrect cached credentials.
  • Bound refresh requests to 20 seconds and lock acquisition to 30 seconds.

The lock coordinates cooperating refresh operations using the same local token-store path. It does not synchronize independent stores on separate hosts.

The lock and reload deliberately run even while a cached access token is valid. This makes removal or replacement of stored credentials visible on the next request. A valid reader can wait behind another process's refresh, up to the bounded lock timeout. An mtime-only fast path could miss replacement on a filesystem with coarse timestamp resolution.

Removing the last app is a valid empty-store state. Running bridges report a missing token without restoring the removed credentials. Unix lock support includes FreeBSD, NetBSD, OpenBSD, and DragonFly BSD.

If an earlier race already invalidated a refresh token, reauthenticate once after upgrading; this change cannot revive an invalidated grant.

Remaining persistence limitation

Interactive OAuth2 login, app-management commands, bearer/OAuth1 saves, and startup credential backfill still write outside the refresh lock. An administrative save concurrent with refresh can overwrite a newer token with its cached snapshot. Closing that separate gap requires routing every mutation through lock → reload → apply → atomic write. This PR does not claim to serialize those other writers.

Regression evidence

TestSharedRefreshAcrossProcesses launches four independent processes that all load an expired grant before a barrier opens. A local fake provider accepts that refresh token exactly once, rotates it, and rejects stale reuse. The test fails on stock v1.3.4. With this change, all processes succeed and the provider receives one refresh request. No real X credentials or requests are needed.

Additional tests cover forced-refresh reuse, removal of a persisted grant, and removal of the last registered app while a bridge still caches its credentials.

Validation

  • go vet ./... passes on macOS arm64.
  • go test -race ./... passes, including the new last-app-removal regression.
  • GOOS=windows GOARCH=amd64 CGO_ENABLED=0 go build ./... passes.
  • GOOS=linux GOARCH=arm64 CGO_ENABLED=0 go build ./... passes.
  • FreeBSD, NetBSD, OpenBSD, and DragonFly amd64 builds (CGO_ENABLED=0 go build ./...) pass.
  • gofmt and git diff --check pass.

Windows and BSD runtime behavior has not been exercised locally. Repeated browser pages motivated the report; the deterministic test proves the refresh-coordination defect without asserting that it explains every observed login prompt.

@CLAassistant

CLAassistant commented Oct 14, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@maartengoet maartengoet left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tested this branch (abe35b2) against the exact scenario from #102 — works; a few non-blocking notes below.

Real-world confirmation. macOS arm64, xurl 1.3.4 via npx -y @xdevplatform/xurl mcp https://api.x.com/mcp. Codex (ChatGPT desktop) keeps one bridge per thread alive for days (3 running here) plus one bridge per Claude Code session, all sharing ~/.xurl/auth.yml. Claude Code's MCP logs show the bootstrap fallback (no valid OAuth2 token; opening the browser to sign in) at new-session start, while the already-running bridges silently fail their refresh. Reading 1.3.4 matches the analysis in the issue: store.TokenStore is loaded once, saveToFile rewrites the whole snapshot with os.WriteFile, and mcpBridge.bootstrap() treats any GetValidOAuth2Token error as "no token".

What I ran on this branch (Go 1.27.1 darwin/arm64):

  • go vet ./... clean
  • go test -race -count=1 ./auth/ ./store/ → ok, including the 4-process TestSharedRefreshAcrossProcesses
  • GOOS=windows GOARCH=amd64 CGO_ENABLED=0 go build ./... and GOOS=linux GOARCH=arm64 CGO_ENABLED=0 go build ./... build
  • gofmt -l and go mod tidy -diff both empty
  • A build of this branch is now the xapi bridge for both Claude Code and Codex on this machine: a fresh bridge start no longer opens the browser, and auth.yml.refresh.lock shows up as soon as two bridges run. I'll report back if anything shows up across the 2-hour rotations.

Review notes (none blocking):

  1. Lock + Reload() now run on every GetValidOAuth2Token call, i.e. on every forwarded MCP request, not only when the cached token is expired. The cost is small (flock + a few hundred bytes of YAML), but a bridge holding a still-valid token can wait up to the 30s lock timeout while a sibling is inside a refresh (20s network timeout). I assume this is deliberate so a removed store takes effect immediately in running bridges (TestRefreshDoesNotResurrectRemovedGrant). If so, a comment in refreshOAuth2Token would help; otherwise an os.Stat mtime/size check before the full reload would keep the hot path lock-free.
  2. The other persistence paths (OAuth2Flow → saveOAuth2Token, auth apps add/update, bearer/OAuth1 saves, the startup credential backfill in NewTokenStoreWithCredentials) still write the whole in-memory snapshot without lock/reload. Because refresh now reloads first, the blast radius is much smaller, but a CLI command that saves while a bridge is rotating can still persist a stale refresh token. A follow-up that routes every saveToFile through lock → reload → apply → atomic write would close that.
  3. Reload() returns "empty token store" for a file with zero apps, where loadFromData treats it as empty. Reachable via xurl auth apps remove of the last app while bridges run; cosmetic (the bridge falls back to the browser either way), but xurl token then shows a store error instead of TokenNotFound.
  4. //go:build darwin || linux covers all goreleaser targets; //go:build unix would also keep go build ./... working on the BSDs.
  5. Worth a line in the CHANGELOG: a store whose refresh token already died under the old race will trigger exactly one more browser login after upgrading; after that the grant stays consistent.

Nice touches: flock on a sidecar (crash-safe, inode stable across the atomic rename), temp + fsync + rename, and the "sibling already replaced it" short-circuit on forced refresh. Would be great to see this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OAuth2 refresh races between MCP processes sharing one token store

3 participants