yonlu/yellowjacket
1
1
Fork 0
Code Issues 55 Pull Requests 6 Actions Packages Projects Releases 12 Wiki Activity

onPlaybackFinished emits its state outside the lock, so a stale 'stopped' can overtake a live 'playing' #123

New Issue
Closed
opened 2026-08-19 12:37:54 +00:00 by yonlu · 1 comment
yonlu commented 2026-08-19 12:37:54 +00:00
Owner
Copy Link
Copy Source

Findings

onPlaybackFinished (backend/player/player.go:492-531) releases p.mu and then emits the two events that tell the frontend what happened:

p.mu.Lock()
p.state = Stopped
...
p.mu.Unlock()

// Emit Wails events outside the lock — these are non-blocking
// calls that don't need player state.
p.emitPlaybackFinished()

events.Emit(
	p.ctx,
	events.PlaybackStateChanged,
	map[string]string{"state": string(Stopped)},
)

Every other emit in this file happens under p.mu (:414, :597, :676, :705, :772). This one does not, and the window is real: a user pressing Play, an MPRIS OnPlay, or the queue's own auto-advance can take p.mu in that gap, drive the player to Playing and emit PlaybackStateChanged(playing) first. The stale stopped then lands last and the frontend's isPlaying is wrong — inverted against what the player is actually doing, until the next transition. player-store keys entirely off the last event received; there is no sequence number on this event the way there is on PositionInfo.

Two smaller things in the same function, both consequences of the same "the callback does not know which chain it came from" gap:

  • No guard that the finished chain is still current. The callback is dispatched as go p.onPlaybackFinished() (:481) and then has to queue for p.mu. If it queues behind a slow loadFileLocked (a decode plus a DB read), it wakes up holding the mutex for a player that has since loaded and started a different track — and calls p.rewindLocked() and p.emitPositionLocked() against it, then hands a stale finish to the queue's auto-advance. A generation counter (the existing trackChangeID would do) captured when the chain is registered and compared here is the fix.
  • emitPositionLocked is called at :504 while the state has already been set to Stopped, which is correct, but the ordering of the three emits (position, finished, state) is only meaningful if they cannot be interleaved — which is the same fix.

Direction

Emit both events while still holding p.mu, as every other path does, and capture the chain's trackChangeID in the beep.Callback closure so a stale callback returns without touching anything. backend/player/emit_test.go already has the recorder pattern for asserting the payload order.

**Findings** `onPlaybackFinished` (`backend/player/player.go:492-531`) releases `p.mu` and *then* emits the two events that tell the frontend what happened: ```go p.mu.Lock() p.state = Stopped ... p.mu.Unlock() // Emit Wails events outside the lock — these are non-blocking // calls that don't need player state. p.emitPlaybackFinished() events.Emit( p.ctx, events.PlaybackStateChanged, map[string]string{"state": string(Stopped)}, ) ``` Every other emit in this file happens under `p.mu` (`:414`, `:597`, `:676`, `:705`, `:772`). This one does not, and the window is real: a user pressing Play, an MPRIS `OnPlay`, or the queue's own auto-advance can take `p.mu` in that gap, drive the player to `Playing` and emit `PlaybackStateChanged(playing)` **first**. The stale `stopped` then lands last and the frontend's `isPlaying` is wrong — inverted against what the player is actually doing, until the next transition. `player-store` keys entirely off the last event received; there is no sequence number on this event the way there is on `PositionInfo`. Two smaller things in the same function, both consequences of the same "the callback does not know which chain it came from" gap: - **No guard that the finished chain is still current.** The callback is dispatched as `go p.onPlaybackFinished()` (`:481`) and then has to queue for `p.mu`. If it queues behind a slow `loadFileLocked` (a decode plus a DB read), it wakes up holding the mutex for a player that has since loaded and started a *different* track — and calls `p.rewindLocked()` and `p.emitPositionLocked()` against it, then hands a stale finish to the queue's auto-advance. A generation counter (the existing `trackChangeID` would do) captured when the chain is registered and compared here is the fix. - `emitPositionLocked` is called at `:504` while the state has already been set to `Stopped`, which is correct, but the *ordering* of the three emits (position, finished, state) is only meaningful if they cannot be interleaved — which is the same fix. **Direction** Emit both events while still holding `p.mu`, as every other path does, and capture the chain's `trackChangeID` in the `beep.Callback` closure so a stale callback returns without touching anything. `backend/player/emit_test.go` already has the recorder pattern for asserting the payload order.
yonlu self-assigned this 2026-08-19 12:39:05 +00:00
yonlu added the
Status
In Progress
 label 2026-08-19 12:39:06 +00:00
yonlu commented 2026-08-19 12:39:06 +00:00
Author
Owner
Copy Link
Copy Source

Working this as part of a batch off fix/player-playing-state (#122, #123, #124, #125, #126) — all five are in backend/player and backend/queue, and #124 and #125 are the two halves of the same fallback expression, so splitting them across branches would mean three passes over the same twenty lines.

Approach:

  • #122 — done = true on every readAhead exit; bound the underrun fill and return 0, false past it; give the finished path a way to tell "drained" from "failed" so PlaybackFailed is emitted instead of a silent auto-advance.
  • #123 — move the two emits back under p.mu, and capture the chain's trackChangeID in the beep.Callback closure so a stale callback returns without touching the player.
  • #124/#125 — assign p.format in loadFileLocked, reset p.trackLengthMs there, take the replay path's rate from p.format.
  • #126 — bounds-check currentIndex the way loadCurrentTrack already does.

Verified with make test (all three build configurations) plus new unit tests per exit path; the audible half of #124 needs a 48 kHz fixture.

Working this as part of a batch off `fix/player-playing-state` (#122, #123, #124, #125, #126) — all five are in `backend/player` and `backend/queue`, and #124 and #125 are the two halves of the same fallback expression, so splitting them across branches would mean three passes over the same twenty lines. Approach: - **#122** — `done = true` on every `readAhead` exit; bound the underrun fill and return `0, false` past it; give the finished path a way to tell "drained" from "failed" so `PlaybackFailed` is emitted instead of a silent auto-advance. - **#123** — move the two emits back under `p.mu`, and capture the chain's `trackChangeID` in the `beep.Callback` closure so a stale callback returns without touching the player. - **#124/#125** — assign `p.format` in `loadFileLocked`, reset `p.trackLengthMs` there, take the replay path's rate from `p.format`. - **#126** — bounds-check `currentIndex` the way `loadCurrentTrack` already does. Verified with `make test` (all three build configurations) plus new unit tests per exit path; the audible half of #124 needs a 48 kHz fixture.
logan closed this issue 2026-08-19 14:53:46 +00:00
gitea-actions bot removed the
Status
In Progress
 label 2026-08-19 14:54:42 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Area/Design
Area/Downloads
Area/Explore
Area/Library-UI
Area/Metadata
Area/Packaging
Area/Player
Area/Queue
Area/Settings
Area/Shell-Nav
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
 Platform/Android
Platform/Desktop
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Medium
3
The priority is medium
Priority
Low
4
The priority is low
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
In Progress
Somebody is actively working on this right now
No labels Area/Player Kind/Bug Platform/Android Platform/Desktop
Priority
Medium
3
Reviewed
Confirmed
1
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logan yonlu
No Assignees yonlu
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: yonlu/yellowjacket#123