Fix/skip command ack - #372
Open
SEKY443 wants to merge 2 commits into
Open
Conversation
Root cause of the disconnect: loading a new track (audio key fetch, CDN storage resolve) is a real network round trip with no cached fast path on a manual skip - unlike the natural end-of-track advance, which usually has a prefetched stream ready. The dealer's reply to the skip command was only sent after that load finished, and left waiting long enough, Spotify's backend has been observed to consider the connection unresponsive and reset it with a "request disconnect" close frame - seen as a brief Connect disconnect/reconnect on every manual skip (confirmed from a client log showing exactly that close reason). This is a different mechanism from c40ee45 (decoupling the WebSocket read loop from request handling, so a slow command couldn't also starve pong replies and stall reconnection for up to 15 minutes) - that fix is intact after the merge and still doing its job. This is the layer above it: even with recvLoop free to keep reading, the reply itself was still gated on the load. handlePlayerCommand's skip_prev/skip_next cases now call reply(true) immediately, before running the actual skip, and return the new sentinel error errAlreadyReplied so the caller in Run() knows not to reply a second time (which would block forever - nothing reads a dealer.Request's response channel twice). Any failure past that point is only logged, the same trade-off already established for OptimisticPlaybackReplies: connect-state still gets pushed with the real outcome either way, so the UI self-corrects even on the rare skip that fails after being acknowledged. Scoped to skip_prev/skip_next specifically, since that's what's actually been observed and reported; API/MPRIS-triggered skips are untouched, since they call skipNext/skipPrev directly rather than through this dispatch and have no dealer reply to race against.
The skip_next/skip_prev early-reply fix didn't cover the command that
actually starts a DJ session (or any new context): "play". Confirmed from a
live log - the dealer connection got closed by Spotify ("request disconnect")
while a reply to "hm://connect-state/v1/player/command" was in flight, right
as a DJ session was starting.
"play" and "transfer" both end up in the same loadContext/loadCurrentTrackOrSkip
chain skip_next/skip_prev do, except with more ahead of them: resolving the
context itself plus loading its first track, with nothing prefetched yet for
a session that's only just starting - if anything a worse case than a skip.
Same treatment: reply immediately after the cheap synchronous state setup,
before the network-bound load, and return errAlreadyReplied so the load
failure (if any) is only logged rather than reported through a second reply.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: acknowledge skip/play/transfer before loading, not after
Every manual skip (and, more visibly, every "play" that starts a brand new
context — DJ especially, since it has nothing prefetched yet) causes a
brief Spotify Connect disconnect/reconnect. It's fast enough to be easy to
write off as nothing, but it's real and reproducible on demand.
Caught it directly in the logs:
StatusNormalClosure+"Request disconnect"is Spotify's backenddeliberately closing the dealer connection, not a network hiccup on our
end. The second line is our reply to the in-flight player command failing
to send because the socket is already gone by the time we try.
Root cause
handlePlayerCommand'sskip_next,skip_prev,play, andtransfercases all reply to the dealer request only after the case function
returns — and for all four of these, that means only after a real network
round trip: an audio key fetch and a CDN storage resolve for the track
being loaded, plus (for
play/transfer) resolving the context itselffirst. A natural end-of-track advance usually has a stream already
prefetched and skips most of this; none of these four do.
Left unacknowledged long enough, Spotify's backend appears to consider the
dealer connection unresponsive and resets it. The reconnect itself is fast
here (the WebSocket read loop is already decoupled from request handling,
so a slow command can't also stall reading the next frame off the wire),
so in practice this shows up as a sub-second blip rather than anything
that visibly breaks playback — easy to miss unless you're watching the
log at the right moment.
Fix
For these four cases, reply immediately after the cheap, synchronous part
of handling the command (state bookkeeping - no network involved), before
the network-bound load. The load itself is unchanged; only when the
dealer gets told the command was accepted moves earlier. A new sentinel
error,
errAlreadyReplied, tells the caller inRun()not to reply asecond time - replying twice would block forever, since nothing reads a
dealer.Request's response channel more than once.Any failure past that point (context/track fails to load, etc.) is now
only logged rather than reported back through the reply, since the reply
already went out. This is the same trade-off the existing
OptimisticPlaybackRepliesoption already makes at the output layer, justapplied here to the dealer reply itself. Connect-state still gets pushed
with the real outcome regardless, so the UI self-corrects even on the rare
command that fails after being acknowledged.
Scoped to exactly these four cases - the ones that can trigger a real
network-bound load - not every command.
Happy to split
play/transferinto a separate PR if you'd rather reviewthe two halves independently, same as the raw-payload split last time.