Fix playback stop on server-initiated reconnection#202
Open
teancom wants to merge 3 commits intoSendspin:mainfrom
Open
Fix playback stop on server-initiated reconnection#202teancom wants to merge 3 commits intoSendspin:mainfrom
teancom wants to merge 3 commits intoSendspin:mainfrom
Conversation
When the server reconnects to the daemon (same client_id, new websocket), the old code sent a goodbye message before handshaking the new connection. This caused the server to remove the client from its group and stop playback, requiring manual intervention to resume. Reorder _handle_server_connection to handshake the new connection first, then tear down the old one. The server's attach_connection() atomically replaces the websocket for the same client_id, so the client never leaves the group and playback continues uninterrupted. Also drop the RuntimeError in _on_audio_chunk when the audio worker isn't running — just log at debug level and drop the chunk, since this is a benign race during connection transitions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
On server-initiated reconnection, the old client's audio chunk listeners were never unregistered. This left a brief window where chunks from the old WebSocket could arrive and hit the audio handler after the worker was stopped. Call detach_client() before _handle_disconnect() to cleanly unsubscribe the old listeners first. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts the daemon’s server-initiated reconnection flow to avoid playback stopping when the server reconnects with the same client_id, and softens a benign audio-worker race by dropping chunks instead of raising.
Changes:
- Reordered server-initiated reconnection handling to handshake the new websocket before tearing down the previous connection.
- Removed explicit “goodbye” message send on same-
client_idreconnection to avoid the server removing the client from its playback group. - Changed audio chunk handling to drop chunks (debug log) when the audio worker isn’t running, instead of raising.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sendspin/daemon/daemon.py | Reorders server-initiated connection replacement to preserve group membership/playback across same-server reconnects. |
| sendspin/audio_connector.py | Makes _on_audio_chunk tolerant of worker stop/start races by dropping chunks instead of raising. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
|
Thanks @teancom ! |
Instead of logging every dropped chunk at debug level (which can be extremely noisy during reconnection races), log only when entering the dropping state and reset when chunks flow again. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
Sounds good! I added a commit for the audio chunk debug logging. Now to go back to sendspin-rs stuff. 😄 |
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
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.
I had what appeared to be a bit of a network 'blip', which my MA server noticed and attempted a reconnect, while my sendspin-cli instance was still under the impression that everything was fine. That actually caused my sendspin-cli to reset the connection on its side and playback to stop. After doing some digging, I figured that if the same server was reconnecting, if we swapped the order of 'tear down' and 'reconnect', then playback could continue (with a little audio hiccup, because there was an issue in the first place). If it's an ongoing issue, well then the server couldn't reconnect and we just stop (as the only option).
When the server reconnects to the daemon (same client_id, new websocket), the old code sent a goodbye message before handshaking the new connection. This caused the server to remove the client from its group and stop playback, requiring manual intervention to resume.
Reorder _handle_server_connection to handshake the new connection first, then tear down the old one. The server's attach_connection() atomically replaces the websocket for the same client_id, so the client never leaves the group and playback continues uninterrupted.
Also drop the RuntimeError in _on_audio_chunk when the audio worker isn't running — just log at debug level and drop the chunk, since this is a benign race during connection transitions.