connect: don't answer our own cluster update with another state update - #1741
Open
ralph wants to merge 2 commits into
Open
connect: don't answer our own cluster update with another state update#1741ralph wants to merge 2 commits into
ralph wants to merge 2 commits into
Conversation
Every state update we PUT comes back down the dealer socket as a cluster update naming our own device, and `handle_cluster_update` sets `update_state = true` for any update that arrives while we are active. So the echo schedules another PUT, which echoes again. The period is the round trip; measured against a Spotify Connect session it runs at roughly 420 ms and keeps going until the server answers 429 Too Many Requests. In two captured sessions every burst ended that way — five out of five — so the loop has no brake of its own. The requests it wastes are the device state PUTs, the ones that tell Spotify this device exists and what it is playing, so the ones dropped are the ones worth keeping. `ClusterUpdate.devices_that_changed` already distinguishes the cases and is already logged just above. Across 449 cluster updates captured from real sessions, not one named more than a single device, and the ones caused by our own PUT named us. Kept as narrow as the evidence. A cluster update still reaches the workaround unless it is a DEVICE_STATE_CHANGED naming this device and nothing else: "named only us" implies "caused by us" for a state change, not for a device appearing, disappearing or changing volume. Updates naming us alongside another device were never observed, and they fall through to the workaround too. The workaround stays because the comment says it earns its place — session id, playback id and track metadata were all tried against the underlying de-sync. This does not touch the case it was written for. Another device's *commands* never came through here in the first place; they arrive on the connect-state request stream and set `update_state` from `handle_connect_state_request`, and updates genuinely originating from another device still set it here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR prevents a state-update feedback loop in the Connect/Spirc cluster-update handling by ignoring cluster updates that are clearly echoes of this device’s own DEVICE_STATE_CHANGED PUT while the device is active.
Changes:
- Add
ClusterUpdateReasonusage and detect “own echo” cluster updates viaupdate_reason+devices_that_changed == [self.device_id]. - Skip the existing
update_state = trueworkaround when the cluster update is identified as an own-echo, avoiding repeated state PUTs and eventual429 Too Many Requests. - Document the fix in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| connect/src/spirc.rs | Filters out self-echo DEVICE_STATE_CHANGED cluster updates to prevent the state-update loop while preserving the existing workaround for other updates. |
| CHANGELOG.md | Adds an unreleased changelog entry describing the loop prevention and its impact (avoids 429s). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
While an active Connect device, librespot answers its own cluster update with another state
update, and the reply comes back as another cluster update. The result is a state-update loop
that runs at the round-trip period and only stops when Spotify answers
429 Too Many Requests.The loop
Every
notify()PUTs the device state. Spotify pushes the resulting cluster update back downthe dealer socket — including the one our own PUT just caused — and
handle_cluster_updatesets
update_state = truefor any update that arrives while we are active, without checkingwhere it came from:
That schedules another
notify()afterUPDATE_STATE_DELAY, which PUTs, which echoes. Tracedagainst a real session it runs at a median of 418 ms:
The loop has no brake of its own. Across two captured sessions, every burst ended in a
429 — five out of five. The only gaps not preceded by one are the idle stretch before playback
starts. The requests it burns are the device-state PUTs, so the ones dropped are the ones that
tell Spotify the device exists and what it is playing.
The change
ClusterUpdate.devices_that_changedalready carries what is needed to tell the cases apart,and
handle_cluster_updatealready logs it a few lines above. Skip the update when it namesonly us:
The
fixmeworkaround stays. The comment records that session id, playback id and trackmetadata were all tried against the underlying de-sync and none helped, which is evidence
against removing it. Anything that is not our own echo still reaches it.
Kept deliberately narrow, in two ways that matter:
device appearing, disappearing, or changing volume. Without this gate the change swallows a
DEVICE_VOLUME_CHANGEDecho that should reach the workaround — which happened in testing,so this is not hypothetical.
falls through to the workaround unchanged.
Commands from other devices were never affected either way: those arrive on the connect-state
request stream and set
update_statefromhandle_connect_state_request.Verification
Measured on macOS against a real Spotify account, comparing device-state PUTs in the 60
seconds after
PlayerEvent::Playing:Across a whole 2m38s session after the change: 17 device-state PUTs, 11 echoes suppressed, no
429 in a scenario that produced them on every previous run.
Legitimate state updates still go out — the count is small, not zero.
The workaround's case still works. With a second device sending pause, resume and volume
while librespot was the active device, the position did not de-sync:
Paused and resumed on the same millisecond, held flat across the state updates in between, and
tracked real time correctly afterwards. Volume propagated in both directions.
On the premise
The filter assumes an echo names only the device that caused it. Checked against 449 cluster
updates captured from real sessions before writing anything: not one named more than a single
device. Updates from other devices do appear while active, which is what the workaround
branch still handles.
Checks
cargo fmt --all -- --check,cargo build,cargo clippy --all-targets(zero warnings) andcargo test --workspace(23 passed) all clean. Note thatconnect/has no tests nearspirc.rs, so those are a don't-regress gate rather than coverage of this change; themeasurements above are the verification.
🤖 Generated with Claude Code