fix: use singleflight and add backoff for token refresh - #56
Conversation
Replace RWMutex double-checked lock with singleflight.Group so concurrent callers share a single in-flight refresh without holding an exclusive lock during the HTTP round-trip. Add exponential backoff (1s to 30s cap) after failed refreshes to prevent token endpoint outage amplification.
|
Warning Review limit reached
Next review available in: 13 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe token refresh path now shares concurrent refreshes with ChangesToken refresh behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Callers
participant refreshableAuth
participant singleflight
participant TokenSource
Callers->>refreshableAuth: Request token
refreshableAuth->>singleflight: Share refresh operation
singleflight->>TokenSource: Token()
TokenSource-->>singleflight: Token or error
singleflight-->>refreshableAuth: Shared result
refreshableAuth-->>Callers: Cached or refreshed token
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openshell/v1/auth_refresh.go (1)
99-130: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUpdate retry state once for each shared refresh.
Lines 106-117 run once for every waiter after
singleflight.Group.Doreturns a shared error. One failedsource.Token()call can therefore increasebackofffrom 1 second to 30 seconds when several callers wait on it.Move the retry-state update into the
group.Doclosure. RechecknextRetryin that closure before callingsource.Token(). Add a concurrent failure test that verifies one source call produces one backoff increment.Also applies to: 140-141
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshell/v1/auth_refresh.go` around lines 99 - 130, Update the refresh flow around group.Do so retry state is changed only inside its closure for the single shared source.Token call. Recheck nextRetry there before invoking the source, and remove the outer per-waiter backoff update while preserving cached-token fallback and successful-state reset. Add a concurrent failure test verifying multiple waiters produce one source call and one backoff increment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openshell/v1/auth_refresh_test.go`:
- Around line 337-344: Strengthen the setup assertions around
provider.GetRequestMetadata in the backoff test: require both initial calls to
return no error, then assert beforeCount equals 2 before making the final call.
Keep the existing final metadata, no-error, and unchanged-call-count assertions.
---
Outside diff comments:
In `@openshell/v1/auth_refresh.go`:
- Around line 99-130: Update the refresh flow around group.Do so retry state is
changed only inside its closure for the single shared source.Token call. Recheck
nextRetry there before invoking the source, and remove the outer per-waiter
backoff update while preserving cached-token fallback and successful-state
reset. Add a concurrent failure test verifying multiple waiters produce one
source call and one backoff increment.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d9885d9e-cb30-4c7d-97d1-e4bfb8484818
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modopenshell/v1/auth_refresh.goopenshell/v1/auth_refresh_test.go
When multiple goroutines coalesce on a failed singleflight refresh, each would independently double the backoff. Guard the update so only the first goroutine from a batch applies it. Add test verifying a single coalesced failure sets backoff to initialBackoff, not 30s.
Use a channel barrier to synchronize goroutine start and increase the mock token fetch delay to 100ms so all goroutines enter singleflight before the first returns. Reduce goroutine count to 20 since the barrier ensures they all arrive together.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
=======================================
Coverage 89.67% 89.67%
=======================================
Files 78 78
Lines 4967 4989 +22
=======================================
+ Hits 4454 4474 +20
- Misses 346 348 +2
Partials 167 167 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR improves RefreshableToken concurrency and resilience by coalescing concurrent refresh attempts with singleflight and introducing an exponential backoff after refresh failures, while continuing to serve a stale cached token during backoff windows when available.
Changes:
- Replace the prior double-checked locking refresh path with
singleflight.Groupto share one in-flight refresh across callers. - Add exponential backoff state (
nextRetry,backoff) with a 30s cap after refresh failures. - Add/adjust unit tests for singleflight behavior and backoff semantics; add
golang.org/x/syncdependency.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| openshell/v1/auth_refresh.go | Switch refresh coalescing to singleflight and add backoff state/logic around refresh failures. |
| openshell/v1/auth_refresh_test.go | Update concurrency test to synchronize starts; add backoff-focused unit tests. |
| go.mod | Add golang.org/x/sync dependency. |
| go.sum | Add checksums for golang.org/x/sync. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…test) Add defensive nil/type check on singleflight return value to prevent panic if TokenSource returns (nil, nil). Strengthen backoff test with explicit nextRetry and precondition assertions per CodeRabbit and Copilot review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openshell/v1/auth_refresh_test.go`:
- Around line 428-437: Update both singleflight tests in
openshell/v1/auth_refresh_test.go:134-150 and
openshell/v1/auth_refresh_test.go:428-437 to use explicit contention barriers.
Gate all goroutines before invoking the provider request, wait until every
caller reaches the release gate, and in the failed-refresh case hold Token()
until callers contend; preserve the existing assertions while ensuring
successful and failed refreshes test actual singleflight joining rather than
scheduler timing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e92a2dc-0d89-4d23-ab06-35aa44f4238f
📒 Files selected for processing (2)
openshell/v1/auth_refresh.goopenshell/v1/auth_refresh_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openshell/v1/auth_refresh.go
Use the same channel barrier pattern as the success singleflight test to ensure deterministic goroutine contention per CodeRabbit review.
Summary
sync.RWMutexdouble-checked lock withsingleflight.Groupso concurrent callers share a single in-flight token refresh without holding an exclusive lock during HTTP round-tripsCloses #50
Test plan
-racemise run lintpasses (0 issues)Summary by CodeRabbit