refactor(oauth): fix backoff bug and clean up error handling#17
refactor(oauth): fix backoff bug and clean up error handling#17
Conversation
- Use additive 5s backoff per RFC 8628 §3.5 instead of compounding multiplier - Replace direct error equality with errors.Is() for ErrRefreshTokenExpired - Remove duplicate APICallOK() call on re-auth retry path - Use full struct assignment instead of partial field copies in auto-refresh - Drain response body before 401 retry to allow connection pool reuse - Remove unused DeviceAuthURL and Scopes from oauth2.Config - Replace panic() with fmt.Fprintf + os.Exit(1) in initConfig() - Replace custom contains() helpers with strings.Contains() in tests - Update slow_down test for additive backoff behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR refines the OAuth device-flow polling and related error-handling behavior, aligning the slow_down handling with RFC 8628 guidance and improving robustness/clarity in a few areas.
Changes:
- Adjust
slow_downpolling behavior to use a fixed +5s interval increase (capped) rather than a multiplier-based approach. - Improve error comparison semantics by using
errors.IsforErrRefreshTokenExpired. - Simplify tests by replacing a custom substring helper with
strings.Contains, and update polling tests to match the newslow_downbehavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
polling_test.go |
Updates the slow_down test scenario/expectations to match the new +5s backoff behavior. |
main.go |
Changes polling backoff behavior, improves error handling, adjusts device flow config usage, and updates the 401 auto-refresh flow. |
main_test.go |
Replaces custom substring matching with strings.Contains. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
polling_test.go
Outdated
| // After 1 slow_down the interval becomes 1+5=6s, so we need ~8s total. | ||
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) |
There was a problem hiding this comment.
The timing estimate here looks incorrect and makes the test timeout very tight (potentially flaky). With Interval=1s, after the first slow_down the poll interval becomes 6s; with one authorization_pending before success, the third attempt won’t occur until ~1s + 6s + 6s ≈ 13s (plus overhead). Consider either increasing the context timeout buffer, or adjusting the mocked server sequence so success happens on the second attempt after slow_down if you want a shorter test.
| // After 1 slow_down the interval becomes 1+5=6s, so we need ~8s total. | |
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) | |
| // After 1 slow_down the interval becomes 1+5=6s; with an additional authorization_pending | |
| // before success, the third attempt occurs after ~1s + 6s + 6s ≈ 13s, so use a generous timeout. | |
| ctx, cancel := context.WithTimeout(context.Background(), 25*time.Second) |
| // If 401, drain and close the first response body to allow connection reuse, | ||
| // then refresh the token and retry. | ||
| if resp.StatusCode == http.StatusUnauthorized { | ||
| _, _ = io.Copy(io.Discard, resp.Body) | ||
| resp.Body.Close() | ||
|
|
There was a problem hiding this comment.
resp.Body.Close() is deferred earlier, but the 401 branch also explicitly closes the body after draining it. That can lead to a double-close of the original response body. Consider restructuring so the initial defer resp.Body.Close() is only set for the non-401 path (or removed and replaced with explicit closes) while still draining+closing before the retry to allow connection reuse.
| // Backoff state | ||
| pollInterval := time.Duration(interval) * time.Second | ||
| backoffMultiplier := 1.0 | ||
|
|
There was a problem hiding this comment.
This block now implements a linear +5s backoff on slow_down (per RFC 8628), but the surrounding function-level documentation still mentions “exponential backoff”. Please update the doc/comment wording to match the new behavior so future readers aren’t misled.
- Fix double-close of response body by using explicit close in 401 path and defer in else branch instead of unconditional defer - Increase slow_down test timeout from 15s to 25s to prevent flakiness - Update pollForTokenWithProgress doc comment to say "additive backoff" instead of "exponential backoff" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
errors.Is()instead of==forErrRefreshTokenExpiredat both call sites, so wrapped errors are handled correctlyAPICallOK()call, unusedDeviceAuthURL/Scopesfields, customcontains()test helpers (replaced withstrings.Contains), partial struct update (replaced with full assignment),panic()ininitConfig()(replaced with stderr + exit), and drain response body before 401 retry for connection reuseTest plan
make test— all tests pass (including updatedTestPollForToken_SlowDownfor additive backoff)make lint— 0 issuesmake build— binary builds cleanly🤖 Generated with Claude Code