feat: remove blocking I/O from the player loop by devgianlu · Pull Request #379 · devgianlu/go-librespot

devgianlu · 2026-08-29T10:36:48Z

Removes blocking I/O from AppPlayer.Run, the single goroutine that owns all
player state. Fixes #300 at the root, and #372 falls out of it.

Why

Run owned every piece of state and did every network round trip inline.
Each handler wrapped itself in one 30s budget covering the context resolve,
the audio key, the CDN first chunk and the connect-state PUT at the end.

A CDN edge that stalls burns the whole budget, so that PUT fails instantly
with context deadline exceeded — not a network problem, an already-dead
context. flushState then dropped it for good: it cleared stateDirty
before attempting and only re-armed on a rate limit. Meanwhile every
command that arrived during the stall queued up and drained in a burst the
moment the loop freed.

Visible in the reporter's own logs in #300 (log2, 14:28:27 → 14:29:04): a
30s CDN stall, an instant PUT failure, three /player/play commands issued
during the stall, four tracks loaded in four seconds.

What it looks like now

Run() goroutine  ── owns all state, never blocks
   ├─ loader lane    one job at a time, owns the track list exclusively
   ├─ state lane     connect-state PUTs: own deadline, coalescing, retry
   └─ go ...         detached: resume reports, token renewal

Jobs come in three classes, because "newest wins" is only right for some of
them. A load cancels whatever is running and drops whatever is queued —
only the newest destination matters, and running the rest first is exactly
the burst above. Queue edits are never dropped; losing a queued track would
be visible. Prefetches yield to both.

The 30s handler wrappers are gone: deadlines belong to the job now, not to
whichever command happened to start it. The invariant is stated on
AppPlayer, where the next person adding a handler will read it.

Bugs found and fixed along the way

Each is its own commit, and each is a real bug independent of the refactor:

An accesspoint deadlock. ap.recvLoop fanned packets out over
unbuffered channels while also dispatching audio keys, so a consumer
waiting on a key blocked the loop delivering it. Broken only by the 15s
key timeout — the pong watchdog cannot help, since it reacts by failing a
read while the loop is blocked on a send.
A permanent loop wedge on logout. The dealer-initiated path had no
zeroconf guard, and outside zeroconf nothing reads that channel at all.
An mpris panic. Close closed the update channels under concurrent
senders — a live send on closed channel.
A volume deadlock. updateVolume open-coded the drain-then-send that
sendVolumeUpdate documents as unsafe to run concurrently, on a slot only
the loop drains, with the mixer as a second writer.
Stale queue on transfer. The early transfer claim published a new
context uri and track with the previous context's PrevTracks/
NextTracks/Index still attached. Previously masked because the
correcting push followed a few hundred ms later on the same goroutine.

Notes for review

tracks.Snapshot deep-copies. ContextTrackToProvidedTrack hands over the
ContextTrack's own metadata map (ids.go:94) and the list keeps writing
to it, so anything published across a goroutine boundary must own its copy.
Because the player is handed its stream from the lane, its playing event
can beat the load being recorded — those events are held until it lands. A
play/pause arriving mid-load is remembered and applied rather than failing
for want of a stream.
advanceNext walking unplayable tracks was fifty nested frames, each
getting a fresh 30s budget. It is now a sequence of cancellable jobs.
Behaviour change: /player/next and /player/prev return before the
track has loaded, so a client polling /status immediately sees the old
track. Both already returned an unconditional 200. /player/play still
reports a context it could not resolve. A superseded command answers 409.
Supersedes Fix/skip command ack #372. It gets there by not doing the work on the loop, so
the acknowledgement still reports whether the command was accepted — no
errAlreadyReplied, no swallowed errors.

Testing

./coverage.sh and go test -race -tags "test_unit test_integration" ./...
green, each commit independently. Daemon coverage 21.8%, from effectively
zero on this code. New tests pin the properties that matter: supersede drops
loads but never queue edits, submit never blocks on a stalled job, close
answers everything it owes, Emit does not block on a stalled listener, and
a snapshot shares nothing with the list it came from.

Not yet exercised against a real account — that is the main thing this needs
before leaving draft. Worth checking specifically: skip/seek/pause/transfer,
DJ narration, shuffle on a long context, and a deliberately stalled CDN.

Known residual

The player's own command channel. Player.SeekMs fetches a CDN chunk inside
manageLoop (vorbis/decoder.go:376) and playerCmdSet can open the audio
device, so a p.player.* call from the loop can still block behind those.
Fixing it means restructuring Player, not AppPlayer — separate change.

Closes #300
Closes #372

🤖 Generated with Claude Code

flushState cleared stateDirty before attempting the PUT and only re-armed the timer for a rate limit, so any other failure — a timeout, a 5xx, a network error — dropped that state push for good. Nothing resent it until some later transition happened to come along, leaving Spotify with a state the device had already moved on from. Clear the dirty flag on success instead, and reschedule every failure with an exponential backoff from the coalescing interval, capped and jittered. A rate limit still waits exactly the cooldown the backend asked for. See #300 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

recvLoop is the single goroutine that reads the socket, answers pings and hands audio keys to the key provider, and it fanned packets out over unbuffered channels with a blocking send. A consumer that was itself waiting on an audio key therefore deadlocked the loop: the ProductInfo or CountryCode packet ahead of the key could not be delivered, so the key behind it was never dispatched. Only the 15s key timeout broke the cycle — the pong watchdog cannot, because it reacts by failing a read while the loop is blocked on a send. Buffer each receiver and give both fan-out sends an escape on done, so shutdown does not leave a wedged loop behind either. See #300 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Both logout paths sent the player straight down an unbuffered channel whose reader rebuilds a whole session synchronously, so the send held up the player loop for as long as that took. The dealer-initiated one was worse: it never checked whether zeroconf was enabled, and outside zeroconf nothing reads that channel at all, so a server-sent logout wedged the loop for good. Route both through requestLogout, which drops the request when zeroconf is disabled and otherwise hands over from its own goroutine, giving up if the player is cancelled first. It deliberately does not select on stop: that token has exactly one taker, and stealing it would leave Run running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Emit wrote to every listener inline, with a ten second timeout each, from whichever goroutine raised the event — in practice the player loop. One listener that stopped reading therefore stalled playback control for ten seconds per event, and several stalled it for longer still. Give each client its own queue and writer goroutine, so Emit only hands the event over. One goroutine per client rather than per event, because listeners rely on the order events arrive in: will_play before metadata, paused before playing. A client that falls far enough behind loses its oldest queued events rather than the newest, so it converges on the current state instead of replaying a stale backlog. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Close closed the two update channels while emitters were still sending on them, so a state update racing shutdown took the daemon down with "send on closed channel". The flag it set alongside was an unsynchronised bool read from the consumer goroutine, and the channels themselves were unbuffered, so every emit also parked the player loop until D-Bus had finished writing the properties out. Close a done channel instead, and make both channels latest-wins queues of depth one: only the most recent state is worth publishing, and the emitter never waits. The command reply channel is buffered for the same reason — the reply comes from the player loop and must not wait on a D-Bus caller. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Two problems on the same few lines. The state save is a temp file plus a rename, which on a Pi with a tired SD card routinely takes long enough to be heard, and it ran inline on the player loop; coalesce it onto its own goroutine instead, behind a mutex, since two AppPlayers overlap whenever a zeroconf session is replaced. The channel send was also a blocking send on a slot only the player loop drains. It open-coded the drain-then-send that sendVolumeUpdate documents as unsafe to run concurrently, and the mixer is a second writer: refill the freed slot from there and the loop waits forever on itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The connect-state PUT ran inline on the player loop, sharing one thirty second budget with everything else the handler did. A stalled CDN fetch spent that budget on the audio, so the state push at the end of the load failed the moment it was attempted, with an already dead context — the "context deadline exceeded" in #300 was never a network problem. Give the push a lane of its own, with its own deadline. The state is marshalled on the player loop so the lane carries bytes rather than protos the loop goes on mutating, and the cluster's view of our public address comes back the same way. Consecutive player-state pushes coalesce, since only the newest is worth sending; every other reason marks a transition the backend has to see, in order. The buffering state a load writes before fetching anything now reaches Spotify while the load is still running, rather than after it — which is what stops a controller deciding the device has gone unresponsive and re-issuing the command. Fixes #300 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The transfer handler claims the transfer early, pushing the new context uri and track before resolving anything. It left PrevTracks, NextTracks and Index untouched, so that push described the new context with the previous one's queue attached — they are only replaced later, once the track list has resolved. Until now the correcting push followed a few hundred milliseconds later on the same goroutine, which hid it. With the push on its own lane the inconsistent state genuinely reaches Spotify, so clear them as part of the claim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The daemon reached into the track list from a dozen places, and the four line Track/PrevTracks/NextTracks/Index assignment appeared six times. Give the list one accessor that produces everything the daemon publishes about it, so what the list holds and what the state reports stop being the same memory. That distinction is the point. ContextTrackToProvidedTrack hands over the ContextTrack's own metadata map, and the list keeps writing to it as queueing and autoplay flags change, so a track published anywhere it may outlive the call has to own its copy. AllTracks loses its context and gains a bound, which is what its one caller — the autoplay seed — actually wants: it is reached by having run out of context, so anything still unfetched is by definition not recently played, and it was the only unbounded page walk left in the package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Prefetching is a full track load — audio key, storage resolve, first chunk, and any narration wrapped around it — and it ran inline on the player loop, on a timer, while a track was playing. Up to thirty seconds during which nothing else could be served. Add the loader lane: one job at a time, off the loop, with results applied back on it. Jobs come in three classes because "newest wins" is only right for some of them. A load cancels whatever is running and drops whatever is queued, since only the newest destination matters and running the rest first is exactly the burst of skipping in #300. Queue edits are never dropped, because losing a queued track would be visible. Prefetches yield to both. A result that no longer describes what the daemon is trying to do is discarded rather than applied, and discarding closes the stream it built — that stream owns a CDN reader or a cache file nothing else will release. Only prefetching moves here for now. Picking which track to prefetch stays on the loop, because the track list is still read from there. NewStream reads the country code, which the loop writes when the accesspoint reports it, so that becomes an atomic rather than a pointer shared between the two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Loading a track is the longest thing the daemon does: metadata, an audio key, storage resolve, the first CDN chunk, any narration around it, and opening the output device. All of it ran inline on the player loop, so for its duration nothing else was served — not a dealer command, not an API request, not the player's own events. Commands that arrived meanwhile queued up and then ran back to back the moment it finished, which is the burst of skipping in #300. Split it: the bookkeeping and the buffering state stay on the loop, the fetching runs on the loader lane, and what it found is recorded back on the loop. A newer load cancels the one in flight rather than queueing behind it, so a stalled fetch delays only itself. The chain that used to unwind through return values now continues through callbacks — loadCurrentTrackOrSkip, advanceNext and the skip commands hand on what to do once the load lands. advanceNext walking a run of unplayable tracks is now a sequence of cancellable jobs rather than fifty nested frames, each of which used to get a fresh thirty second budget of its own. Two things follow from the player being handed its stream from elsewhere. Its playing event can reach the loop before the load has been recorded, so those events are held until it has; and a play or pause arriving mid-load is remembered and applied when the track lands, rather than failing for want of a stream. Episode resume reports go to their own goroutines: nothing waits on them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Starting a context and taking over a transfer both resolved the context, walked it to the starting track and shuffled it — up to two hundred and fifty six pages of it — before the player loop could do anything else. The episode resume lookup sat on the same path, another ten seconds of it. Both now claim the state they are about to describe and hand the rest to the loader lane. Claiming first is what matters: the context uri, the track and a buffering state reach controllers immediately, rather than after however long the resolve takes. Neither builds on a list the daemon already owns — the resolved list only becomes the daemon's when the result is applied — so nothing is shared across the boundary to get this wrong. The resume lookup moves into the load itself, where the position it finds is the position the stream is built at, rather than being written into the state beforehand and picked back up. Together with the previous commit this is what #372 asks for: skip, play and transfer are acknowledged after the bookkeeping, not after a network round trip, so the backend stops resetting the dealer connection. It gets there by not doing the work on the loop at all, so the acknowledgement still reports whether the command was accepted. Closes #372 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Three network calls were left on the loop: resolving the context for POST /player/play, renewing an access token for GET /token, and resolving an autoplay station when a context runs out. The first is the one that matters — it is on the path a controller hits for every track it starts, which for the Volumio plugin in #300 is constantly. They move to the lane, or to their own goroutine where nothing about the player is involved. The API request keeps waiting for its real answer, on the HTTP goroutine rather than this one, so /player/play still reports a context it could not resolve; the reply travels with the work and a guard makes sure it is sent exactly once. A command superseded before it ran now answers 409 rather than looking like a server error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The last blocking I/O on the loop was the track list itself. Every walk of it — peeking at what comes next, going to the next track, seeking to a chosen one, shuffling, editing the queue — can fetch a context page, and all of them ran inline. The list now belongs to the loader lane and is reached only through listJob, which gives a walk exclusive use of it and applies the snapshot it leaves behind on the loop. Neither the list nor the resolver behind it is safe to touch from two goroutines, so nothing else may hold it: the loop keeps the pointer to compare and replace, and never calls a method on it. Two rules this needs that the lane did not have. A mutation is applied even when superseded, because it already changed the list and refusing to report that would leave the state describing a list that no longer exists; and its commit checks the list is still the current one, or a queue edit that queued behind a context load would publish a snapshot of the wrong context. Shuffle is reported before the reordering rather than after: it is what the shuffle button binds to and the walk can cover every page, so it flips immediately and reverts if the walk fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Each handler wrapped itself in thirty seconds because it did the work itself and something had to bound it. Nothing reached from the loop does network, disk or peer I/O any more — it is all on a lane, with a deadline belonging to the job rather than borrowed from whichever command happened to start it — so those budgets bounded nothing, and the contexts they carried were dead weight threaded through a dozen signatures. Stating the invariant on AppPlayer instead, where the next person to add a handler will read it: nothing reached from Run's select may block. That is what lets all of this state be read and written without a single lock, and it is what #300 came down to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

devgianlu and others added 15 commits August 29, 2026 11:18

Navigation Menu

feat: remove blocking I/O from the player loop - #379

feat: remove blocking I/O from the player loop#379
devgianlu wants to merge 15 commits into
masterfrom
fix/nonblocking-player-loop

devgianlu commented Aug 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

devgianlu commented Aug 29, 2026

Why

What it looks like now

Bugs found and fixed along the way

Notes for review

Testing

Known residual

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant