Skip to content

fix(connect): keep spirc running after transient connection-id update failure#1716

Open
maresb wants to merge 1 commit into
librespot-org:devfrom
maresb:fix/spirc-no-fatal-shutdown-on-connection-id-failure
Open

fix(connect): keep spirc running after transient connection-id update failure#1716
maresb wants to merge 1 commit into
librespot-org:devfrom
maresb:fix/spirc-no-fatal-shutdown-on-connection-id-failure

Conversation

@maresb

@maresb maresb commented Jun 1, 2026

Copy link
Copy Markdown

Problem

In SpircTask::run, the connection_id_update arm of the main tokio::select! loop is the only arm that treats a handler error as fatal — it breaks out of the loop:

match |connection_id| if let Err(why) = self.handle_connection_id_update(connection_id).await {
    error!("failed handling connection id update: {why}");
    break;   // every other arm just logs and continues
}

Breaking leads straight to the unexpected shutdown path and terminates the spirc — but the librespot process stays alive. So process-level supervision (Restart=on-failure, etc.) never fires, and the device silently disappears from Spotify Connect until it is manually restarted.

The failure is usually transient: handle_connection_id_update issues the connect-state PUT (notify_new_device_appeared), which can race a dealer websocket reset and surface as SpircError::FailedDealerSetup.

Fix

Drop the break; so the arm logs and continues, identical in shape to the seven sibling arms in the same select!. The connection_id is delivered through a long-lived dealer subscription to hm://pusher/v1/connections/, and the pusher re-sends a fresh connection_id on every dealer reconnect — so a transient failure is retried on the next update instead of taking down the whole controller.

Real-world impact

A single transient failed to put connect state for new device left a raspotify instance wedged — process healthy, systemctl reporting the unit active, discovery socket still listening, but gone from Connect — for 13 days before it was noticed and restarted.

Notes

  • Intentionally minimal and independent of the broader dealer-reconnect work in fix: dealer reconnect and session loss recovery without playback interruption #1692 (which keeps the break and adds separate reconnect machinery). This can land as a small standalone robustness fix.
  • connect/src/spirc.rs has no test coverage around run(), so there is no unit test to update; verified by reproducing the wedge against a live instance.

🤖 Generated with Claude Code

The `connection_id_update` arm of the spirc `select!` loop is the only arm
that treats a handler error as fatal: it `break`s out of the loop, which
leads to the "unexpected shutdown" path and terminates the spirc while the
librespot process stays alive. The device then silently disappears from
Spotify Connect until the process is manually restarted.

Such failures are typically transient — `handle_connection_id_update`
issues the connect-state PUT (`notify_new_device_appeared`), which can race
a dealer websocket reset and surface as `SpircError::FailedDealerSetup`.
The connection_id arrives via a long-lived dealer subscription to
`hm://pusher/v1/connections/`, and the pusher re-sends a fresh
connection_id on every dealer reconnect, so logging and continuing lets the
next update retry registration — exactly like every other arm in this loop.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 1, 2026 10:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR changes Spirc’s behavior to avoid shutting down when handling a connection-id update fails, and documents the behavior change in the changelog.

Changes:

  • Stop breaking out of the Spirc loop on connection-id update handling errors.
  • Add a changelog entry describing the updated behavior and user-facing impact.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
connect/src/spirc.rs Removes the break on connection-id update handling failure so Spirc continues running.
CHANGELOG.md Documents the new behavior so releases capture the operational/user impact.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread connect/src/spirc.rs
@maresb

maresb commented Jun 1, 2026

Copy link
Copy Markdown
Author

Heads-up for reviewers: the red cargo clippy check is pre-existing on dev and unrelated to this changedev's own HEAD fails it too. The first error is clippy::unnecessary_sort_by at metadata/src/audio/item.rs:197 (newly enforced by clippy 1.96.0). This PR's cargo fmt, cargo +stable test, and all cross builds pass.

That's already addressed by the open clippy PRs — #1711 fixes this exact sort lint, and #1712 fixes a second stable-clippy lint (clippy aborts on the first error, so that one would surface next). Merging those would turn CI green here without touching this PR, so I've kept this diff to the single relevant line rather than duplicating their fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants