fix(coderd/externalauth): support disabling token refresh retries - #26231
Conversation
…hRetryTimeout TestRefreshToken/RefreshRetries flaked on Windows because it disabled transient-failure retries with RefreshRetryTimeout = time.Nanosecond. A near-zero timeout is not deterministic: 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 (refreshCount = totalRefreshes + 1). Make a negative RefreshRetryTimeout disable transient-failure retries explicitly so exactly one refresh attempt is made, and use -1 in the test instead of time.Nanosecond. Closes coder/internal#1550 (PLAT-293)
|
/coder-agents-review |
|
Chat: Review in progress | View chat deep-review v0.7.1 | Round 3 | Last posted: Round 3, 4 findings (2 P4, 2 Nit), APPROVE. Review Finding inventoryFindings
Round logRound 1Panel (12 reviewers). 0 P0-P2, 2 P4, 3 Nit (1 dropped). Reviewed against cfb03f5..cbb23fa. Round 2Panel (6 reviewers). All R1 findings verified fixed. No new findings. Reviewed against cfb03f5..0cbd71a. Round 3Panel (5 reviewers). 0 open from prior rounds. 2 new Nit (both dropped as diminishing returns on already-addressed CRF-1/CRF-2). Reviewed against cfb03f5..f6e6771. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Well-targeted fix for a Windows-specific flaky test. The root cause analysis correctly traces the nondeterminism to coarse clock resolution interacting with near-zero context timeouts, and the fix introduces an explicit retry-disable semantic rather than trying to make the timing tighter. 10 of 12 panel reviewers found no issues.
As Pariston put it: "I tried to build a case against this change and could not."
Severity summary: 2 P4, 3 Nit (1 Nit dropped as scope-adjacent).
coderd/externalauth/externalauth.go:391
Nit [CRF-4] This function doc lists two retry-bypass paths (permanent failures, no refresh token) but not the negative-RefreshRetryTimeout bypass this PR adds. The inline comment at line 404 covers the "why," but the function-level doc is the contract, and the contract is now incomplete.
Suggested addition (e.g. at the end of the existing doc comment):
...and the no-op case where no refresh token is set, and a negative RefreshRetryTimeout, all bypass the retry loop...
(Leorio)
🤖
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All R1 findings addressed. CRF-1/CRF-2 (verbose comments) condensed, CRF-4 (incomplete function doc) updated with the third bypass path, CRF-5 (commit subject length) fixed via PR title shortening for squash merge. 7 reviewers (Netero + 6 panel) verified the fixes and found no new issues.
As Pariston put it for the second round: "I tried to build a case against this change and could not."
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Round 3: all prior findings remain fixed. The R3 delta (commit f6e6771) rewrites the inline comment at line 404 to describe the behavior directly instead of referencing the flake. Gon re-flagged residual duplication between the inline comments and the field doc (2 Nits), but the comments are now 2-3 lines each, structurally consistent with the function's comment-per-early-return pattern, and the author has consciously refined them across two commits. Dropping as diminishing returns.
6 reviewers (Netero + 5 panel) found no actionable issues. Pariston, for the third time: "I tried to build a case against this change and couldn't."
🤖 This review was automatically generated with Coder Agents.
TestRefreshToken/RefreshRetriesflakes on Windows. The subtest disables transient-failure refresh retries by settingRefreshRetryTimeout = 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, andretry.Wait's first delay is zero, so an extra IDP refresh attempt slips through and the attempt-count assertion fails withrefreshCount = totalRefreshes + 1.A negative
RefreshRetryTimeoutnow disables transient-failure retries explicitly so exactly one refresh attempt is made, and the test sets-1instead oftime.Nanosecond. The retry config fields are only set from tests, so default refresh behavior is unchanged.Closes coder/internal#1550 (PLAT-293)
Root cause analysis
RefreshRetryTimeout = time.Nanosecondintending "no retries".refreshTokenWithRetrycreatescontext.WithTimeout(ctx, 1ns). On Linux this context is canceled synchronously at creation: consecutivetime.Now()reads differ by more than 1ns, socontext.WithDeadlineobservestime.Until(deadline) <= 0. TheretryCtx.Err() != nilguard then deterministically stops after one attempt.time.Now()is coarse, so both clock reads insideWithTimeoutcan return the same instant, and a real 1ns timer is scheduled instead of synchronous cancellation.retryCtx.Err()is still nil andretry.Wait's first delay is zero, so a second refresh attempt happens.require.Equal(t, refreshCount, totalRefreshes)then fails withexpected: 2, actual: 1(or4 vs 3when 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.
RefreshRetriespassed 100 consecutive local runs with the change.This PR was generated by Coder Agents on behalf of @jscottmiller.