Repository navigation
fix(voice): recover connections with fresh sessions - #587
Conversation
|
I noticed it might partially conflict with #582. I am happy to make the changes. |
f539585 to
c44e810
Compare
There was a problem hiding this comment.
pulled the branch and actually ran things instead of just reading the diff — build/vet clean, tests pass with -race. also pointed our bot at this branch and ran the test we wrote for the exact bug we hit in prod, still passes fine
the part I actually care about: TestHandleGatewayCloseStartsFreshConnection's "resume attempts exhausted" case feeds it a plain error, same shape as a dns failure, and it correctly asks for a fresh session instead of looping forever. that's literally our bug
couple small things, not blockers - there's no test that goes through reconnect() -> 5 tries -> closeHandlerFunc in one shot, it's just tested in pieces (easy enough to verify by eye though). also tryOpenGateway's timeout went from 5s to 30s for every open attempt, not just recovery, so a join that's actually gonna fail takes longer to error now. probably fine
separate heads up, this touches the same handleGatewayClose that #580 and #583 both edit, so those are gonna conflict
approving, looks good to me
There was a problem hiding this comment.
found two things while we were going through the branch — left them as inline suggestions, both should be quick to apply
|
Thanks for your suggestion. I edited both sections reflecting your review. |
| return ctx.Err() | ||
| case <-timer.C: | ||
| func (g *gatewayImpl) doReconnect(ctx context.Context, state State, maximumAttempts int) error { | ||
| for attempt := 0; ; attempt++ { |
There was a problem hiding this comment.
not a fan of this for loop style
can we keep it a normal for one?
There was a problem hiding this comment.
as I described use a plain for loop like before for {}
| return 0 | ||
| } | ||
| // Exponentially backoff up to a limit of 10s | ||
| exponent := min(attempt-1, 4) |
There was a problem hiding this comment.
it was meant to limit the number of attempts, but it was quite meaningless since we already have maximumReconnectAttempts to limit the number of attempts. fixed it.
There was a problem hiding this comment.
why do you use a loop here now all of the sudden
there is no loop needed, just clamp it it goes over maximumConnectDelay
There was a problem hiding this comment.
you're right. fixed it
|
@topi314 thank you for the comments. I reviewed your points and fixed them. |
Summary
Problem
When a Voice Gateway endpoint became unavailable, the reconnect loop kept retrying the cached endpoint indefinitely. The previous delay calculation also multiplied the attempt count by an already increasing delay, allowing the interval to grow beyond the documented 10-second limit.
If those attempts eventually failed, the connection could not recover by requesting new voice credentials. This left the connection registered but unable to send speaking updates until the process was restarted.
The connection could also open as soon as a Voice Server Update supplied a token and endpoint, without waiting for the matching fresh session ID from a Voice State Update.
Changes
4021and4022so they do not start a new connection, matching Discord's documented behavior.Testing
go test -race ./voice.go test ./....go vet ./....