fix(coderd): dedupe concurrent OIDC token refresh in ValidateAPIKey by sav-labs · Pull Request #25301 · coder/coder

sav-labs · 2026-05-13T16:13:37Z

Symptom

When Coder is configured with Keycloak SSO, refreshing the dashboard after
the OIDC access token expires fires 2–5 concurrent grant_type=refresh_token
requests to Keycloak per user. With refresh-token rotation enabled, only one
wins; the rest fail with invalid_grant.

Root cause

ValidateAPIKey in coderd/httpmw/apikey.go performs the OIDC token refresh
inline on every HTTP request whose user_links.oauth_expiry is in the past.
Because the frontend dashboard issues several parallel API calls on load,
each goroutine independently observes the expired token and calls
oauthConfig.TokenSource(...).Token() with no in-process synchronisation.

Fix

Two complementary layers:

In-process singleflight — a package-level
singleflight.Group[string, database.UserLink] keyed by
userID:loginType deduplicates concurrent refreshes inside one coderd
process. Different users do not block each other.
Cross-replica optimistic lock — a new
UpdateUserLinkRefreshToken query updates user_links only when the
stored refresh token still matches the one the caller read. Losers of
the race observe sql.ErrNoRows, re-read the row written by the winner,
and reuse its tokens instead of erroring.

The helper refreshOAuthLink also re-reads the link inside the singleflight
closure, so callers queued behind the leader return immediately without
hitting the IdP at all.

Verification

A regression test, OAuthRefreshSingleflight in
coderd/httpmw/apikey_test.go, launches 5 concurrent ValidateAPIKey calls
for the same user with an expired OAuth token and asserts the IdP mock
recorded exactly one refresh attempt.

End-to-end evidence from the issue reporter (Keycloak event log with
access-token-lifespan = 2m, before vs. after this PR, showing one
REFRESH_TOKEN per expiry instead of 2–5) will be attached as a comment.

github-actions · 2026-05-13T16:14:01Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

sav-labs · 2026-05-13T16:18:51Z

I have read the CLA Document and I hereby sign the CLA

sav-labs · 2026-05-13T16:22:56Z

recheck

… lock Adds a new query for updating user_links token columns that only writes when the refresh token currently stored in the database still matches the one the caller read before contacting the IdP. Callers should treat sql.ErrNoRows as 'another caller refreshed first' and re-read the row. This is a prerequisite for deduplicating concurrent OIDC refresh requests in ValidateAPIKey across coderd replicas.

Emyrk · 2026-05-14T16:39:41Z

+// oidcRefreshGroup deduplicates concurrent OIDC/GitHub login refresh
+// attempts inside a single coderd process. Multiple parallel HTTP requests
+// from the same user that hit ValidateAPIKey after the OAuth access token
+// has expired would otherwise each independently call the IdP with the
+// same single-use refresh token; only one would win and the rest would
+// fail with invalid_grant. The singleflight key is "<userID>:<loginType>"
+// so refreshes for different users do not block each other. Across
+// replicas, the UpdateUserLinkRefreshToken query provides optimistic
+// locking that converts cross-process races into harmless re-reads.
+var oidcRefreshGroup singleflight.Group[string, database.UserLink]


We should not use globals.

Emyrk

"Cross-replica optimistic lock" will probably return the same refresh token if the api is loading many requests with the same auth.

Maybe we should hold a postgres lock when the token is expired

…PIKey When the OAuth access token expires and a user's dashboard refresh fires multiple parallel API requests, each goroutine independently called the IdP with the same single-use refresh token. The first won; the rest got invalid_grant and could overwrite the valid token with stale state on the losing path. Wraps the refresh in a singleflight group keyed by user ID and login type so concurrent goroutines in the same coderd process share one IdP call. Persists the new tokens via UpdateUserLinkRefreshToken, whose optimistic-lock predicate converts cross-replica races into sql.ErrNoRows, which the helper resolves by re-reading the row written by the winner. Closes coder#25275.

Adds a regression test that spawns N concurrent ValidateAPIKey calls for one user with an expired OAuth access token and asserts the IdP mock observed exactly one refresh attempt.

sav-labs · 2026-05-14T19:03:36Z

"Cross-replica optimistic lock" will probably return the same refresh token if the api is loading many requests with the same auth.

Maybe we should hold a postgres lock when the token is expired

Thanks for the review! Addressed both:

Moved the singleflight group from a package var to a field on
OAuth2Configs.
Wrapped the refresh in db.InTx with AcquireLock (postgres advisory lock
keyed on user_id + login_type) so refreshes for a given user serialize across
replicas. The optimistic-lock predicate on UpdateUserLinkRefreshToken is now
defense-in-depth — happy to drop it if you prefer.

sav-labs · 2026-05-18T08:44:29Z

@Emyrk gentle nudge 🙏 Addressed your advisory-lock suggestion — refresh now runs in db.InTx with tx.AcquireLock keyed on user_id + login_type. CI is green. Another look when you have time would be great, thanks!

Emyrk · 2026-05-18T13:43:42Z

+	// RefreshGroup deduplicates concurrent OAuth refresh attempts inside a
+	// single coderd process. Multiple parallel HTTP requests from the same
+	// user that hit ValidateAPIKey after the OAuth access token has expired
+	// would otherwise each independently call the IdP with the same
+	// single-use refresh token; only one would win and the rest would fail
+	// with invalid_grant. The singleflight key is "<userID>:<loginType>" so
+	// refreshes for different users do not block each other. The zero value
+	// is ready to use.
+	RefreshGroup singleflight.Group[string, database.UserLink]


The singlefight here is good 👍

Emyrk · 2026-05-18T13:54:19Z

+	txErr := db.InTx(func(tx database.Store) error {
+		lockID := database.GenLockID(fmt.Sprintf("oauth-refresh:%s:%s", original.UserID, original.LoginType))
+		if err := tx.AcquireLock(dbauthz.AsSystemRestricted(ctx), lockID); err != nil {
+			return &ValidateAPIKeyError{
+				Code: http.StatusInternalServerError,
+				Response: codersdk.Response{
+					Message: internalErrorMessage,
+					Detail:  fmt.Sprintf("acquire oauth-refresh advisory lock: %s", err.Error()),
+				},
+				Hard: true,
+			}
+		}
+
+		link, err := tx.GetUserLinkByUserIDLoginType(dbauthz.AsSystemRestricted(ctx), database.GetUserLinkByUserIDLoginTypeParams{
+			UserID:    original.UserID,
+			LoginType: original.LoginType,
+		})
+		if err != nil {
+			return &ValidateAPIKeyError{
+				Code: http.StatusInternalServerError,
+				Response: codersdk.Response{
+					Message: "A database error occurred",
+					Detail:  fmt.Sprintf("re-read user_link for refresh: %s", err.Error()),
+				},
+				Hard: true,
+			}
+		}
+		// Another goroutine or replica already refreshed; reuse its result.
+		if !link.OAuthExpiry.IsZero() && link.OAuthExpiry.After(dbtime.Now()) {
+			result = link
+			return nil
+		}
+
+		token, err := oauthConfig.TokenSource(ctx, &oauth2.Token{
+			AccessToken:  link.OAuthAccessToken,
+			RefreshToken: link.OAuthRefreshToken,
+			Expiry:       link.OAuthExpiry,
+		}).Token()
+		if err != nil {
+			return &ValidateAPIKeyError{
+				Code: http.StatusUnauthorized,
+				Response: codersdk.Response{
+					Message: fmt.Sprintf(
+						"Could not refresh expired %s token. Try re-authenticating to resolve this issue.",
+						friendlyName),
+					Detail: err.Error(),
+				},
+				Hard: true,
+			}
+		}
+
+		updated, err := tx.UpdateUserLinkRefreshToken(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLinkRefreshTokenParams{
+			UserID:                 link.UserID,
+			LoginType:              link.LoginType,
+			OAuthAccessToken:       token.AccessToken,
+			OAuthAccessTokenKeyID:  sql.NullString{}, // dbcrypt will update as required
+			OAuthRefreshToken:      token.RefreshToken,
+			OAuthRefreshTokenKeyID: sql.NullString{}, // dbcrypt will update as required
+			OAuthExpiry:            token.Expiry,
+			// Refresh should keep the same debug context because we use
+			// the original claims for the group/role sync.
+			Claims:               link.Claims,
+			OldOauthRefreshToken: link.OAuthRefreshToken,
+		})
+		if err != nil {
+			return &ValidateAPIKeyError{
+				Code: http.StatusInternalServerError,
+				Response: codersdk.Response{
+					Message: internalErrorMessage,
+					Detail:  fmt.Sprintf("update user_link: %s.", err.Error()),
+				},
+				Hard: true,
+			}
+		}
+		result = updated
+		return nil
+	}, nil)
+	if txErr != nil {
+		var vErr *ValidateAPIKeyError
+		if errors.As(txErr, &vErr) {
+			return database.UserLink{}, vErr
+		}
+		return database.UserLink{}, &ValidateAPIKeyError{
+			Code: http.StatusInternalServerError,
+			Response: codersdk.Response{
+				Message: internalErrorMessage,
+				Detail:  fmt.Sprintf("refresh user_link transaction: %s", txErr.Error()),
+			},
+			Hard: true,
+		}
+	}
+	return result, nil
+}


The issue we have here is the refresh happens inside the transaction.

We have a limited pg connection pool in coder. If an IdP is slow, we are now consuming the connection for the full duration of the request.

This gets more expensive when the pg_lock is contended, as each connection will block waiting for the first one to return. So a single slow refresh could now consume a pg connection per replica 🤔

I don't have any elegant solution in mind. Ideally we don't hold a pg connection or a pg lock during the refresh:

token, err := oauthConfig.TokenSource(ctx, &oauth2.Token{ AccessToken: link.OAuthAccessToken, RefreshToken: link.OAuthRefreshToken, Expiry: link.OAuthExpiry, }).Token()

It feels a bit sloppy, but maybe we can mark the row as being "in progress" in some fashion. So the top of this function acquiring the lock marks the row, then releases the transaction before the refresh.

Any competing replicas see the mark, and enter a holding pattern for some duration before failing.

Emyrk · 2026-05-18T13:58:43Z

To fully fix this, we actually might want to do this protection in the TokenSource wrapper? Or something akin to that, because we also refresh tokens in the provisionerdserver:

https://github.com/coder/coder/blob/main/coderd/provisionerdserver/provisionerdserver.go#L3186-L3220

Emyrk · 2026-05-18T14:00:37Z

+-- name: UpdateUserLinkRefreshToken :one
+-- Optimistic lock: only update the row if the refresh token in the database
+-- still matches the one we read before attempting the refresh. This prevents
+-- a concurrent caller that lost a token-refresh race (across replicas, where
+-- in-process deduplication via singleflight cannot reach) from overwriting a
+-- valid token stored by the winner. Callers should treat sql.ErrNoRows as
+-- "another caller refreshed first" and re-read the row rather than erroring.
+UPDATE
+	user_links
+SET
+	oauth_access_token = @oauth_access_token,
+	oauth_access_token_key_id = @oauth_access_token_key_id,
+	oauth_refresh_token = @oauth_refresh_token,
+	oauth_refresh_token_key_id = @oauth_refresh_token_key_id,
+	oauth_expiry = @oauth_expiry,
+	claims = @claims
+WHERE
+	user_id = @user_id
+AND
+	login_type = @login_type
+AND
+	oauth_refresh_token = @old_oauth_refresh_token
+RETURNING *;


👍

Also implement the encryption side of this query in dbcrypt

Example: https://github.com/coder/coder/blob/main/enterprise/dbcrypt/dbcrypt.go#L175-L175

Emyrk

Blocking for now until we resolve the postgres connection issue

github-actions Bot assigned sav-labs May 13, 2026

github-actions Bot added the community Pull Requests and issues created by the community. label May 13, 2026

sav-labs changed the title ~~fix(coderd/httpmw): dedupe concurrent OIDC token refresh in ValidateAPIKey~~ fix(coderd): dedupe concurrent OIDC token refresh in ValidateAPIKey May 13, 2026

sav-labs force-pushed the fix/oidc-refresh-singleflight branch from 66be41b to 8134ae7 Compare May 13, 2026 16:32

cdrci2 added a commit to coder/cla that referenced this pull request May 13, 2026

@sav-labs has signed the CLA in coder/coder#25301

2cfdfc6

sav-labs force-pushed the fix/oidc-refresh-singleflight branch from 8134ae7 to 9a82cac Compare May 13, 2026 17:34

DevelopmentCats requested review from dannykopping and jdomeracki-coder May 13, 2026 20:51

dannykopping requested review from Emyrk and removed request for dannykopping May 14, 2026 07:52

sav-labs force-pushed the fix/oidc-refresh-singleflight branch from 9a82cac to 590c48a Compare May 14, 2026 08:43

Emyrk reviewed May 14, 2026

View reviewed changes

sav-labs added 2 commits May 14, 2026 20:13

test(coderd/httpmw): cover concurrent OIDC refresh deduplication

b1bd3e8

Adds a regression test that spawns N concurrent ValidateAPIKey calls for one user with an expired OAuth access token and asserts the IdP mock observed exactly one refresh attempt.

sav-labs force-pushed the fix/oidc-refresh-singleflight branch from 590c48a to b1bd3e8 Compare May 14, 2026 19:03

Emyrk reviewed May 18, 2026

View reviewed changes

Emyrk requested changes May 19, 2026

View reviewed changes

github-actions Bot added the stale This issue is like stale bread. label May 27, 2026

github-actions Bot closed this May 31, 2026

Emyrk mentioned this pull request Jun 22, 2026

fix: prevent concurrent token refreshes #26530

Merged

Navigation Menu

Uh oh!

fix(coderd): dedupe concurrent OIDC token refresh in ValidateAPIKey - #25301

fix(coderd): dedupe concurrent OIDC token refresh in ValidateAPIKey#25301
sav-labs wants to merge 3 commits into
coder:mainfrom
sav-labs:fix/oidc-refresh-singleflight

sav-labs commented May 13, 2026

Uh oh!

github-actions Bot commented May 13, 2026 •

edited

Loading

Uh oh!

sav-labs commented May 13, 2026

Uh oh!

sav-labs commented May 13, 2026

Uh oh!

Emyrk May 14, 2026

Uh oh!

Emyrk left a comment

Uh oh!

sav-labs commented May 14, 2026

Uh oh!

sav-labs commented May 18, 2026

Uh oh!

Emyrk May 18, 2026

Uh oh!

Emyrk May 18, 2026

Uh oh!

Emyrk May 18, 2026

Uh oh!

Emyrk commented May 18, 2026

Uh oh!

Emyrk May 18, 2026 •

edited

Loading

Uh oh!

Emyrk left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

sav-labs commented May 13, 2026

Symptom

Root cause

Fix

Verification

Uh oh!

github-actions Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sav-labs commented May 13, 2026

Uh oh!

sav-labs commented May 13, 2026

Uh oh!

Emyrk May 14, 2026

Choose a reason for hiding this comment

Uh oh!

Emyrk left a comment

Choose a reason for hiding this comment

Uh oh!

sav-labs commented May 14, 2026

Uh oh!

sav-labs commented May 18, 2026

Uh oh!

Emyrk May 18, 2026

Choose a reason for hiding this comment

Uh oh!

Emyrk May 18, 2026

Choose a reason for hiding this comment

Uh oh!

Emyrk May 18, 2026

Choose a reason for hiding this comment

Uh oh!

Emyrk commented May 18, 2026

Uh oh!

Emyrk May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Emyrk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented May 13, 2026 •

edited

Loading

Emyrk May 18, 2026 •

edited

Loading