mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(coderd/externalauth): support disabling token refresh retries (#26231)
`TestRefreshToken/RefreshRetries` flakes on Windows. The subtest disables transient-failure refresh retries by setting `RefreshRetryTimeout = time.Nanosecond`, but a near-zero timeout cannot deterministically prevent a retry: on coarse-clock platforms the 1ns deadline may not register as expired until after the first refresh attempt completes, and `retry.Wait`'s first delay is zero, so an extra IDP refresh attempt slips through and the attempt-count assertion fails with `refreshCount = totalRefreshes + 1`. A negative `RefreshRetryTimeout` now disables transient-failure retries explicitly so exactly one refresh attempt is made, and the test sets `-1` instead of `time.Nanosecond`. The retry config fields are only set from tests, so default refresh behavior is unchanged. Closes https://github.com/coder/internal/issues/1550 (PLAT-293) <details> <summary>Root cause analysis</summary> 1. The test sets `RefreshRetryTimeout = time.Nanosecond` intending "no retries". 2. `refreshTokenWithRetry` creates `context.WithTimeout(ctx, 1ns)`. On Linux this context is canceled synchronously at creation: consecutive `time.Now()` reads differ by more than 1ns, so `context.WithDeadline` observes `time.Until(deadline) <= 0`. The `retryCtx.Err() != nil` guard then deterministically stops after one attempt. 3. On Windows, `time.Now()` is coarse, so both clock reads inside `WithTimeout` can return the same instant, and a real 1ns timer is scheduled instead of synchronous cancellation. 4. The fake IDP is served in-process, so the first refresh attempt can complete before that timer fires. `retryCtx.Err()` is still nil and `retry.Wait`'s first delay is zero, so a second refresh attempt happens. 5. `require.Equal(t, refreshCount, totalRefreshes)` then fails with `expected: 2, actual: 1` (or `4 vs 3` when the race hits a later loop iteration), matching all CI occurrences. Timing-based test-side mitigations cannot close this race, so the fix adds explicit retry-disable semantics instead. `RefreshRetries` passed 100 consecutive local runs with the change. </details> *This PR was generated by Coder Agents on behalf of @jscottmiller.*
This commit is contained in:
@@ -139,7 +139,8 @@ type Config struct {
|
||||
RefreshRetryMaxBackoff time.Duration
|
||||
// RefreshRetryTimeout overrides the total budget for retrying a transient
|
||||
// refresh failure across all attempts. A zero value applies
|
||||
// defaultRefreshRetryTimeout.
|
||||
// defaultRefreshRetryTimeout. A negative value disables transient-failure
|
||||
// retries entirely, so exactly one refresh attempt is made.
|
||||
RefreshRetryTimeout time.Duration
|
||||
}
|
||||
|
||||
@@ -389,9 +390,9 @@ validate:
|
||||
|
||||
// refreshTokenWithRetry exchanges the refresh token for a new access token,
|
||||
// retrying with exponential backoff on transient failures. Permanent
|
||||
// failures (as classified by isFailedRefresh) and the no-op case where no
|
||||
// refresh token is set bypass the retry loop so a doomed refresh is not
|
||||
// repeatedly attempted.
|
||||
// failures (as classified by isFailedRefresh), the no-op case where no
|
||||
// refresh token is set, and a negative RefreshRetryTimeout all bypass the
|
||||
// retry loop so a doomed or unwanted refresh is not repeatedly attempted.
|
||||
func (c *Config) refreshTokenWithRetry(ctx context.Context, existingToken *oauth2.Token) (*oauth2.Token, error) {
|
||||
// Without a refresh token the oauth2 library short-circuits with
|
||||
// "token expired and refresh token is not set". No retry can recover
|
||||
@@ -400,6 +401,12 @@ func (c *Config) refreshTokenWithRetry(ctx context.Context, existingToken *oauth
|
||||
return c.TokenSource(ctx, existingToken).Token()
|
||||
}
|
||||
|
||||
// A negative RefreshRetryTimeout disables retries entirely, so make a
|
||||
// single attempt and return.
|
||||
if c.RefreshRetryTimeout < 0 {
|
||||
return c.TokenSource(ctx, existingToken).Token()
|
||||
}
|
||||
|
||||
initial := c.RefreshRetryInitialBackoff
|
||||
if initial <= 0 {
|
||||
initial = defaultRefreshRetryInitialBackoff
|
||||
@@ -409,7 +416,7 @@ func (c *Config) refreshTokenWithRetry(ctx context.Context, existingToken *oauth
|
||||
maximum = defaultRefreshRetryMaxBackoff
|
||||
}
|
||||
total := c.RefreshRetryTimeout
|
||||
if total <= 0 {
|
||||
if total == 0 {
|
||||
total = defaultRefreshRetryTimeout
|
||||
}
|
||||
|
||||
@@ -430,7 +437,7 @@ func (c *Config) refreshTokenWithRetry(ctx context.Context, existingToken *oauth
|
||||
// retry.Wait selects between time.After(delay) and ctx.Done(); when
|
||||
// delay is zero and the context is already canceled the two cases
|
||||
// race nondeterministically, which would cause an unwanted extra
|
||||
// refresh attempt with a near-zero budget (notably in tests).
|
||||
// refresh attempt with a near-zero budget.
|
||||
if retryCtx.Err() != nil {
|
||||
return token, err
|
||||
}
|
||||
|
||||
@@ -156,9 +156,10 @@ func TestRefreshToken(t *testing.T) {
|
||||
// refresh attempts should ever happen. An invalid refresh token does
|
||||
// not magically become valid at some point in the future.
|
||||
//
|
||||
// Internal retries are disabled in this subtest via RefreshRetryTimeout
|
||||
// so each RefreshToken call results in exactly one IDP refresh attempt.
|
||||
// The RefreshTokenWithBackoff subtest covers the retry-with-backoff path.
|
||||
// Internal retries are disabled in this subtest via a negative
|
||||
// RefreshRetryTimeout so each RefreshToken call results in exactly one
|
||||
// IDP refresh attempt. The RefreshTokenWithBackoff subtest covers the
|
||||
// retry-with-backoff path.
|
||||
t.Run("RefreshRetries", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
@@ -182,9 +183,10 @@ func TestRefreshToken(t *testing.T) {
|
||||
}),
|
||||
},
|
||||
ExternalAuthOpt: func(cfg *externalauth.Config) {
|
||||
// Disable transient-error retries so the assertion below
|
||||
// (1 IDP call per RefreshToken) holds.
|
||||
cfg.RefreshRetryTimeout = time.Nanosecond
|
||||
// Negative timeout disables retries (1 IDP call per RefreshToken).
|
||||
// A tiny positive timeout is unreliable on coarse-clock platforms
|
||||
// (Windows).
|
||||
cfg.RefreshRetryTimeout = -1
|
||||
},
|
||||
})
|
||||
|
||||
|
||||
Reference in New Issue
Block a user