mirror of
https://github.com/coder/coder.git
synced 2026-09-21 20:51:01 +08:00
J. Scott Miller
3da8226876
`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.*