Repository navigation
Conversation
maartengoet
left a comment
There was a problem hiding this comment.
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 ./...cleango test -race -count=1 ./auth/ ./store/→ ok, including the 4-processTestSharedRefreshAcrossProcessesGOOS=windows GOARCH=amd64 CGO_ENABLED=0 go build ./...andGOOS=linux GOARCH=arm64 CGO_ENABLED=0 go build ./...buildgofmt -landgo mod tidy -diffboth empty- A build of this branch is now the
xapibridge for both Claude Code and Codex on this machine: a fresh bridge start no longer opens the browser, andauth.yml.refresh.lockshows up as soon as two bridges run. I'll report back if anything shows up across the 2-hour rotations.
Review notes (none blocking):
- Lock +
Reload()now run on everyGetValidOAuth2Tokencall, 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 inrefreshOAuth2Tokenwould help; otherwise anos.Statmtime/size check before the full reload would keep the hot path lock-free. - The other persistence paths (
OAuth2Flow→saveOAuth2Token,auth apps add/update, bearer/OAuth1 saves, the startup credential backfill inNewTokenStoreWithCredentials) 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 everysaveToFilethrough lock → reload → apply → atomic write would close that. Reload()returns"empty token store"for a file with zero apps, whereloadFromDatatreats it as empty. Reachable viaxurl auth apps removeof the last app while bridges run; cosmetic (the bridge falls back to the browser either way), butxurl tokenthen shows a store error instead ofTokenNotFound.//go:build darwin || linuxcovers all goreleaser targets;//go:build unixwould also keepgo build ./...working on the BSDs.- 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.
When multiple
xurl mcpprocesses 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
flock; Windows usesLockFileEx.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
TestSharedRefreshAcrossProcesseslaunches 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.CGO_ENABLED=0 go build ./...) pass.gofmtandgit diff --checkpass.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.