Singleflight key for upstream refresh must match the storage row, not the session

[jhrozek]

upstreamTokenRefresher.RefreshAndStore deduplicates concurrent refreshes on the caller's session id:

// pkg/authserver/refresher.go:67
key := sessionID + ":" + expired.ProviderID
result, err, _ := r.sfGroup.Do(key, func() (any, error) { ... })

That key is only correct while the storage row is itself keyed by session id. It is the key of the caller, not of the row the refresh reads and overwrites. A storage implementation that keys rows on anything else — for example on the user and provider, so a user's separate sessions share one stored credential — produces two different singleflight keys for one row. Both pass the gate, both redeem the same refresh token against the IdP, and the second redemption presents a token the first one already consumed.

This is a single process. Session affinity does not help: both requests already land on the same pod.

Why it matters

ToolHive is IdP-agnostic, so the worst behaviour applies. Okta revokes the whole grant family when a consumed refresh token is presented again — the user loses every session on that provider, not just the losing one. Auth0's reuse interval defaults to no grace. Keycloak gives no grace when rotation is enabled. Entra allows roughly ten seconds for confidential clients. Google does not rotate by default.

The losing goroutine also has a bad failure path of its own. If its redemption succeeds but the store then fails, refreshAndStore sees a rotated refresh token, deletes the row, and returns an error, forcing a full re-auth.

Relationship to #4122

Distinct, and #4122 depends on this being true. That issue is about coordinating refreshes across pods, and states that the in-process singleflight "deduplicates concurrent refresh attempts within a single pod". That is the assumption this issue disproves. Fixing the key here is a precondition for the Redis-lock and CAS tiers proposed there being worth building — a distributed lock keyed off the wrong identity has the same hole.

Why now

Planned work on non-interactive agent credentials (an agent reads a stored upstream credential using a delegated token) adds a user-keyed read, which makes several callers share one storage row by construction. Today the collision needs an unusual storage implementation. After that change it is the normal case. This has to land before any refresher runs against a user-keyed read.

Fix

The refresher needs to know where the row it is about to overwrite actually lives, and key singleflight on that. UpstreamTokenStorage should return the identity of the row a (sessionID, providerID) lookup resolved to, and the refresher keys on that identity. Correct for per-session keying and for any other scheme, at the cost of one breaking change to the interface.

Keying on the UserID and ProviderID already present on storage.UpstreamTokens is not a drop-in substitute: under per-session keying it is coarser than the row, so two sessions with genuinely separate rows would share one refresh result and only the winner's row would be written.

The change needs a concurrency test: N goroutines calling RefreshAndStore for the same row through different session ids, asserting the provider's RefreshTokens was called exactly once.

Three comments describe the current (session, provider) granularity as intended behaviour and need to change with the key — the upstreamTokenRefresher struct doc, the RefreshAndStore doc, and the inline note above the key.

Files

• pkg/authserver/refresher.go — the key, and refreshAndStore's rotation/persist handling
• pkg/authserver/storage/types.go — UpstreamTokenStorage, UpstreamTokenRefresher
• pkg/auth/upstreamtoken/service.go — InProcessService has no group of its own and delegates to the refresher, so it inherits the fix