fix(cli): serialize token refresh across processes#34
Conversation
- Take a cross-process advisory lock (via gofrs/flock) around the refresh-and-save critical section so concurrent CLI invocations cannot spend the same refresh token twice - Re-read the store under the lock and return a peer process's fresh token when it has already completed a refresh, avoiding an unnecessary network call and a guaranteed invalid_grant on rotation servers - Change refreshAccessToken to accept the full stale Token so peer-refresh detection can compare access and refresh token fields - Expose TokenFile on AppConfig so the lock path is known regardless of the active storage backend - Add a test that pre-populates the store with a peer-refreshed token and asserts refreshAccessToken returns it without hitting the network
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR prevents concurrent authgate-cli processes from racing during refresh-token rotation by serializing the refresh-and-save critical section using a cross-process advisory lock, and adds coverage to ensure peer-refreshed tokens short-circuit the network call.
Changes:
- Add
lockTokenStore(gofrs/flock-based) to serialize refresh across CLI processes. - Update
refreshAccessTokento accept the full stale token, re-load the store after lock acquisition, and return peer-refreshed tokens without a refresh request. - Add a new test ensuring no network call occurs when a peer already refreshed, and update existing tests/call sites to the new refresh API.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| main_test.go | Updates test config to include TokenFile, adjusts refresh tests to new signature, and adds peer-refresh short-circuit test. |
| main.go | Updates refresh call site to pass the full stored token. |
| auth.go | Implements lock + peer-check logic in refreshAccessToken; updates auto-refresh call site. |
| config.go | Adds TokenFile to AppConfig and wires it through config/store initialization. |
| lock.go | Introduces cross-process advisory lock implementation used by refresh flow. |
| go.mod | Adds github.com/gofrs/flock dependency. |
| go.sum | Adds checksums for the new dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func lockTokenStore(ctx context.Context, tokenFile, clientID string) (io.Closer, error) { | ||
| if tokenFile == "" { | ||
| return nil, errors.New("lock: tokenFile is empty") | ||
| } | ||
| if clientID == "" { | ||
| return nil, errors.New("lock: clientID is empty") | ||
| } | ||
|
|
||
| dir := filepath.Dir(tokenFile) | ||
| if err := os.MkdirAll(dir, 0o700); err != nil { | ||
| return nil, fmt.Errorf("create lock directory %q: %w", dir, err) | ||
| } |
There was a problem hiding this comment.
Lock placement currently depends on cfg.TokenFile, whose default is a relative path (.authgate-tokens.json). For keyring/auto (keyring available) this introduces a new requirement that the current working directory be writable (so the lock file can be created), otherwise refresh will fail even though the keyring backend itself doesn’t need filesystem writes. Consider choosing a lock location that’s reliably writable (e.g., under os.UserCacheDir()/os.UserConfigDir()) when the token file path is relative or when using keyring storage.
| lockPath := filepath.Join(dir, filepath.Base(tokenFile)+"."+clientID+".lock") | ||
| fl := flock.New(lockPath) | ||
| locked, err := fl.TryLockContext(ctx, lockRetryInterval) |
There was a problem hiding this comment.
lockPath is built by concatenating clientID directly into the filename. If clientID contains path separators or .. segments, filepath.Join will clean the path and can place the lock file outside the intended directory (and on Windows some characters can make the filename invalid). Consider deriving a filesystem-safe lock name (e.g., hash/hex of clientID, or base64-url encoding, and/or rejecting/escaping path separators) before constructing the path.
Summary
authgate-cliinvocations that both see an expired access token currently race to call the refresh endpoint with the same refresh token. On servers that rotate refresh tokens (the go-authgate default), the first request consumes the token and the second getsinvalid_grant, forcing the second CLI back into an interactive login.github.com/gofrs/flock), which relies on kernelfcntl/LockFileExand is automatically released if the holder crashes — no stale-lock heuristics required.<tokenFile>.<clientID>.lock) and applies to every backend — keyring-backed runs need the coordination too, because the race is in the refresh flow, not the storage layer.Test plan
go test ./...passes (incl. existingTestRefreshAccessToken_RotationMode)TestRefreshAccessToken_PeerAlreadyRefreshedasserts no network call is made when the store already contains a peer-refreshed tokenmake lintis clean🤖 Generated with Claude Code