fix: prevent concurrent token refreshes - #26530
Conversation
This comment has been minimized.
This comment has been minimized.
71bc56f to
551304b
Compare
Emyrk
left a comment
Emyrk
left a comment
There was a problem hiding this comment.
Would this package make things easier?
https://pkg.go.dev/golang.org/x/sync/singleflight
As for the replicas. That remains open, but imo we can do the easy solve for a single instance, then work on the replica fix.
See #25301 for some more context on another attempt
4f68ded to
2d9539e
Compare
This can cause bad refresh token errors, since it can only be used once. Only covers a single instance.
2d9539e to
36997a7
Compare
There was a problem hiding this comment.
Right approach for DEVEX-498; blocking issues in the custom singleflight and key choice, details inline.
Replica question: yes, replicas can still race, singleflight is per-process. But the existing re-read fallback and OldOauthRefreshToken optimistic lock bound the damage to a transient one-off error. I'd ship per-process dedup, keep the re-read fallback tested, and revisit only if error rates stay material. No database-level lock needed now; avoid SELECT FOR UPDATE on the link row, it would serialize unrelated reads and hold a lock across an external HTTP call.
This review was drafted with the assistance of Coder Agents on behalf of @Emyrk.
ec2ace1 to
cd69fb3
Compare
cd69fb3 to
785b088
Compare
| // Use a detached context so if a request is canceled it does not cancel all | ||
| // the other requests as well. Preserve any original deadline. | ||
| rctx := context.WithoutCancel(ctx) | ||
| deadline, ok := ctx.Deadline() |
There was a problem hiding this comment.
Should we specify a specific deadline here? Like 10s? What is the deadline?
There was a problem hiding this comment.
Yeah I was not completely sure about this. I felt like if someone calls RefreshToken with a deadline (or not), then hopefully there was a sensible decision behind that (big if lol) and we should respect that. In tests for example I wanted to respect the test timeout.
I was thinking about a static deadline here but I was concerned that I would choose the wrong number; I am not sure what the upper bound is for completing the full refresh. Currently the refresh is bounded by 10 seconds, then another 10 seconds for updating the link, and the validation and the following update both have no separate deadline. Maybe 30s is reasonable? That seems like a LONG time to wait for a request though.
Edit to add that I think requests are currently not setting a deadline, at least the ones I have seen so far, so in production there would be no deadline. we have no reasonable way to guarantee a deadline would never be set or make sense for refreshing the token
There was a problem hiding this comment.
I went with retry timeout + 10 seconds.
I also removed the separate detached context for the database update, no need since it is already detached from the request context.
7e55096 to
c1395ed
Compare
c1395ed to
b484a70
Compare
2 participants
This can cause bad refresh token errors, since it can only be used once.
Looks like there was an attempt to fix this by checking the database after a failed refresh, but of course this depends on the first request having updated the database in time...
@Emyrk does my fix look like it makes sense? One thing I was not sure about is if there can be multiple replicas writing in which case I actually will need a database-level lock...
Closes https://linear.app/codercom/issue/DEVEX-498/fix-external-oauth-refresh-race
and the original error report is https://linear.app/codercom/issue/DEVEX-432/github-oauth-refresh-unreliability