Started from a desktop report that could not be reproduced: the play/pause button showing pause while the seek bar sat still. Auditing the play/pause and position path for states that produce exactly that turned up seven defects, six of them in backend/player.
pre-push ran the browser tier in parallel with -race
#123–#127 are one commit because they are one file's worth of tangled state and two of them do not compile apart. #122 is separate and self-contained.
The one that explains the report
Stream treated an empty ring as a momentary underrun and answered with silence and ok — correct only while the read-ahead is still going to deliver something, and two of its three exit paths left it never going to. Above that type nothing could tell the difference from healthy playback: the chain never ended, so the player stayed in Playing with the button showing pause, and the decoder's position never moved, so the 1 Hz report pinned the bar at a constant — and since every report resets the bar's interpolation, the report actively suppressed the only thing that would still have moved it.
Not proven to be what was seen, since it was never reproduced. It is a state that produces precisely those symptoms and had no way out.
Two findings worth reading
The first fix was wrong and a test caught it. The silence fill was bounded by a count of calls, reasoning from the speaker's ~200 ms per call. A caller draining in a tight loop makes hundreds in microseconds, so TestBufferedStreamer_BasicStream spent the whole budget before the read-ahead goroutine was scheduled once and ended a good stream at sample zero. The bound is a duration and a run of calls now; it needs both, for opposite reasons.
#127 was found by a test written for #123. The existing suite never loads a file outside YELLOWJACKET_INTEGRATION, so make test was green over a genuine race on every position emit — once a second for the whole of playback. srcMu is the lock that excludes the read-ahead goroutine and the position readers took the speaker lock, which that goroutine never holds. Taking it naively deadlocks, because seekLocked already holds it and then emits the landing position from inside that region; seekSourceLocked is that region extracted.
Verification
make lint — 0 issues, all three build configurations.
make test — green, all three passes, race detector on.
make bindings-check, make skill-check, make css-check, npx tsc --noEmit, make ui-test (898).
Every new test was confirmed to fail without its fix — reverted each fix in turn: index out of range [-1], p.format still holds the speaker rate (44100), a stalled source never ended the stream, WARNING: DATA RACE.
Not covered: the audible half of #124 (a finished track replayed at the wrong pitch) is asserted through p.format, not by listening; and #122's stall is reproduced with a synthetic source, since the real trigger is unknown.
Side-effects worth knowing
OnPlaybackFinished takes an error now and gains //wails:ignore — v3's binding generator segfaults on a bound method taking an error. It was never IPC, and this removes a binding the frontend could have called to force an auto-advance.
A track that fails mid-playback is no longer recorded as a play.
Filed and not fixed here: a comment on #53 with two findings about seek-bar, including that its stated hypothesis does not survive reading updated().
Started from a desktop report that could not be reproduced: the play/pause button showing **pause** while the seek bar sat still. Auditing the play/pause and position path for states that produce exactly that turned up seven defects, six of them in `backend/player`.
## Commits
| Commit | Issue | What |
|---|---|---|
| `282dab4` | #122 | `BufferedStreamer` streamed silence with no end condition |
| `2b84bc5` | #123 | Stale finished-callback; state emitted outside the lock; a failed source reported as a natural end |
| `2b84bc5` | #124 | `p.format` never updated on load |
| `2b84bc5` | #125 | `p.trackLengthMs` inherited across tracks |
| `2b84bc5` | #126 | `q.tracks[currentIndex]` with `currentIndex == -1` |
| `2b84bc5` | #127 | Data race: position readers guarded the decoder with the wrong lock |
| `61d549a` | #128 | `pre-push` ran the browser tier in parallel with `-race` |
#123–#127 are one commit because they are one file's worth of tangled state and two of them do not compile apart. #122 is separate and self-contained.
## The one that explains the report
`Stream` treated an empty ring as a momentary underrun and answered with silence and `ok` — correct only while the read-ahead is still going to deliver something, and **two of its three exit paths left it never going to**. Above that type nothing could tell the difference from healthy playback: the chain never ended, so the player stayed in `Playing` with the button showing pause, and the decoder's position never moved, so the 1 Hz report pinned the bar at a constant — and since every report resets the bar's interpolation, the report actively *suppressed* the only thing that would still have moved it.
Not proven to be what was seen, since it was never reproduced. It is a state that produces precisely those symptoms and had no way out.
## Two findings worth reading
**The first fix was wrong and a test caught it.** The silence fill was bounded by a count of calls, reasoning from the speaker's ~200 ms per call. A caller draining in a tight loop makes hundreds in microseconds, so `TestBufferedStreamer_BasicStream` spent the whole budget before the read-ahead goroutine was scheduled once and ended a good stream at sample zero. The bound is a duration **and** a run of calls now; it needs both, for opposite reasons.
**#127 was found by a test written for #123.** The existing suite never loads a file outside `YELLOWJACKET_INTEGRATION`, so `make test` was green over a genuine race on every position emit — once a second for the whole of playback. `srcMu` is the lock that excludes the read-ahead goroutine and the position readers took the *speaker* lock, which that goroutine never holds. Taking it naively deadlocks, because `seekLocked` already holds it and then emits the landing position from inside that region; `seekSourceLocked` is that region extracted.
## Verification
- `make lint` — 0 issues, all three build configurations.
- `make test` — green, all three passes, race detector on.
- `make bindings-check`, `make skill-check`, `make css-check`, `npx tsc --noEmit`, `make ui-test` (898).
- **Every new test was confirmed to fail without its fix** — reverted each fix in turn: `index out of range [-1]`, `p.format still holds the speaker rate (44100)`, `a stalled source never ended the stream`, `WARNING: DATA RACE`.
Not covered: the audible half of #124 (a finished track replayed at the wrong pitch) is asserted through `p.format`, not by listening; and #122's stall is reproduced with a synthetic source, since the real trigger is unknown.
## Side-effects worth knowing
- `OnPlaybackFinished` takes an `error` now and gains `//wails:ignore` — v3's binding generator **segfaults** on a bound method taking an `error`. It was never IPC, and this removes a binding the frontend could have called to force an auto-advance.
- A track that fails mid-playback is no longer recorded as a play.
Filed and not fixed here: a comment on #53 with two findings about `seek-bar`, including that its stated hypothesis does not survive reading `updated()`.
Closes #122
Closes #123
Closes #124
Closes #125
Closes #126
Closes #127
Closes #128
BufferedStreamer.Stream treated an empty ring buffer as a momentary
underrun and answered with silence and ok. That is right while the
read-ahead is still going to deliver something, and two of its three
exit paths left it never going to: a Close, and a source returning
(0, true) in a loop. Neither set done, so the ring drained and every
call after it was silence claiming to be audio, for the life of the
process.
Nothing above this type could tell that from healthy playback. The
beep.Seq chain never ended, so the player stayed in Playing with the
button showing pause; the decoder's position never moved, so the 1 Hz
report pinned the seek bar at a constant -- and since every report
resets the bar's interpolation, the report actively suppressed the one
thing that would still have moved it. A frozen bar over a track that
was not playing, with no watchdog anywhere to notice.
Every exit now marks the stream done, and the silence fill is bounded
by a duration *and* a run of calls. It needs both. Wall clock is the
real measure, because the speaker paces itself and a stall is a
question about time -- but a caller draining in a tight loop makes
hundreds of calls in microseconds and would outrun a duration alone.
A call count alone is the opposite failure, and not a hypothetical
one: the first attempt used one and spent the whole budget before the
read-ahead goroutine had been scheduled once, ending a perfectly good
stream at sample zero and breaking TestBufferedStreamer_BasicStream.
Err is plumbed out at the same time, because a drained source and a
failed one both arrive as (0, false) and are not the same event.
Reading it is a separate change; without it there is nothing to read.
Closes#122
Five faults found while auditing the play/pause and position path for
a desktop report of the pause icon showing over a seek bar that was
not moving. They are one commit because they are one file's worth of
tangled state, and two of them do not compile apart.
The finished callback did not know which chain it came from. It is
dispatched as a goroutine from the beep callback and then queues for
p.mu, so a user pressing Next in the last second of a track had it
wake up holding the lock for a player that had loaded something else
-- and rewind it, stop it, and hand a stale finish to the queue's
auto-advance. updateStreamers now stamps a chainID and the callback
carries the one it was registered with. (#123)
It also emitted PlaybackFinished and PlaybackStateChanged(stopped)
*after* releasing p.mu, alone in this file, so a Play() taking the
lock in that gap emitted `playing` first and the stale `stopped`
landed last -- the button showing play over a track that was audibly
running. Both emits are back under the lock. (#123)
A source that failed mid-track was reported to the queue as a natural
end, so a broken file auto-advanced in silence and was counted as
played. The handler takes the reason now: the player cannot name the
track, because the metadata is the queue's, so the queue emits
PlaybackFailed and skips recording the play. (#123)
p.format was assigned once, in the constructor, to the *speaker's*
rate, and never again -- so it claimed 44.1 kHz for every file. The
replay-after-finish path resamples from it, meaning a finished track
played a second time was resampled from a rate the decoder never
produced: audibly wrong speed and pitch, and the length and position
fallbacks wrong with it. The fixtures are 22050 Hz, which is what lets
a test see this at all. (#124)
p.trackLengthMs was written only when the database had a row and
cleared only by UnloadTrack, so a file with no row inherited the
previous track's duration -- and every position report is scaled by
it, so the bar reported one track's progress on another's scale.
(#125)
Queue.OnPlaybackFinished indexed q.tracks[currentIndex] having checked
only that the queue was non-empty. currentIndex is -1 whenever the
queue has been exhausted, and onQueueExhausted deliberately leaves the
finished track loaded -- so playing it from there and letting it end
panicked, on a goroutine with no caller to recover it. (#126)
The position readers guarded the decoder with the speaker lock, which
the read-ahead goroutine has no reason to hold and never takes -- so
Position() raced readAhead's Stream() on every position emit, once a
second for the whole of playback. srcMu is the lock that excludes that
goroutine, and taking it naively deadlocks, because seekLocked already
holds it and then emits the landing position from inside that region.
seekSourceLocked is that region extracted, so the lock is released
before anything is emitted. Found by the race detector, via the test
added here for the chain guard: the existing suite never loads a file
outside the integration guard, so make test was green over it. (#127)
OnPlaybackFinished picks up //wails:ignore along with its error
parameter: v3's generator segfaults on a bound method taking an error,
and this was never IPC. That removes a binding the frontend could have
called to force an auto-advance.
Closes#123Closes#124Closes#125Closes#126Closes#127
`go test -race ./...` saturates every core for ~47s, and the UI tier
it was sharing them with is a real Chromium with wall-clock timeouts.
So the browser lost, at random: setup took 106s inside the hook
against 63s standalone, and a different suite failed on each run --
three failing to fetch setup.ts from Vitest's own dev server once, a
15s "did not mount itself" the next time -- against a suite that
passes 898/898 five times running on its own.
That reads as "your branch broke the frontend" when nothing is wrong,
which is the most expensive kind of false negative: the next person
bisects a change that was never at fault. It cost two pushes here
before the summary line gave it away.
Sequential costs about 15s.
Closes#128
Reviewed the code rather than the description, and independently verified the
claims that carry the most risk. This is careful work; approving.
What I checked
go test -race -count=2 ./backend/player/... ./backend/queue/... — green.
Ran twice deliberately, since the race this fixes fired once a second during
playback and a single pass proves less than it looks.
A new test genuinely fails without its fix. Spot-checked #126 by
restoring the pre-fix condition (len(q.tracks) == 0): TestFinishedWithNoCurrentTrackDoesNotPanic panics with index out of range at handlers.go:30, on a goroutine with no caller to
recover it — exactly as described.
make lint — 0 issues across all three build configurations.
make bindings-check — current, and nothing in frontend/src, frontend/test or e2e/specs referenced the removed OnPlaybackFinished
export, so dropping it costs no caller.
The part worth praising
The seekSourceLocked extraction is the right shape and the reasoning is
right: srcMu is not reentrant, displayPositionSecsLocked takes it, and seekLocked used to emit the landing position from inside the region that
already held it. Taking the lock naively there really would deadlock.
What makes it safe rather than lucky is that the constraint is written down
where it would be violated — seekerLengthSecsLocked carries "it must not take
[the source lock] anyway: displayPositionSecsLocked calls this while holding
it". That is the comment that stops the next person reintroducing the deadlock
while fixing a different race, and it is in the callee rather than the caller,
which is where it can actually be read in time.
Same for chainID: capturing it at speaker.Play time rather than reading p.chainID in the callback is the whole fix, and the comment says so.
One observation, not a blocker
Closes #128 is honest about being a contention fix rather than a root-cause
one — the import race presumably still exists, it just stops being reachable
from the gate. I think that is the right call for a gate ("a gate that fails at
random is not a gate"), and I have a corroborating data point: my own push was
rejected by this hook today on a branch based on current main, and the
identical command passed standalone immediately after. So the flake is real,
still live on main, and this is the fix for the case that actually blocks
people.
Worth keeping in mind only if make ui-test starts flaking on its own under
some other load, since that path is unchanged.
All seven Closes footers are in the commit bodies, one per line, so they will
take. Merging.
Reviewed the code rather than the description, and independently verified the
claims that carry the most risk. This is careful work; approving.
## What I checked
- **`go test -race -count=2 ./backend/player/... ./backend/queue/...`** — green.
Ran twice deliberately, since the race this fixes fired once a second during
playback and a single pass proves less than it looks.
- **A new test genuinely fails without its fix.** Spot-checked #126 by
restoring the pre-fix condition (`len(q.tracks) == 0`):
`TestFinishedWithNoCurrentTrackDoesNotPanic` panics with
`index out of range` at `handlers.go:30`, on a goroutine with no caller to
recover it — exactly as described.
- **`make lint`** — 0 issues across all three build configurations.
- **`make bindings-check`** — current, and nothing in `frontend/src`,
`frontend/test` or `e2e/specs` referenced the removed `OnPlaybackFinished`
export, so dropping it costs no caller.
## The part worth praising
The `seekSourceLocked` extraction is the right shape and the reasoning is
right: `srcMu` is not reentrant, `displayPositionSecsLocked` takes it, and
`seekLocked` used to emit the landing position from inside the region that
already held it. Taking the lock naively there really would deadlock.
What makes it safe rather than lucky is that the constraint is written down
where it would be violated — `seekerLengthSecsLocked` carries "it must not take
[the source lock] anyway: displayPositionSecsLocked calls this while holding
it". That is the comment that stops the next person reintroducing the deadlock
while fixing a different race, and it is in the callee rather than the caller,
which is where it can actually be read in time.
Same for `chainID`: capturing it at `speaker.Play` time rather than reading
`p.chainID` in the callback is the whole fix, and the comment says so.
## One observation, not a blocker
`Closes #128` is honest about being a contention fix rather than a root-cause
one — the import race presumably still exists, it just stops being reachable
from the gate. I think that is the right call for a gate ("a gate that fails at
random is not a gate"), and I have a corroborating data point: my own push was
rejected by this hook today on a branch based on current `main`, and the
identical command passed standalone immediately after. So the flake is real,
still live on `main`, and this is the fix for the case that actually blocks
people.
Worth keeping in mind only if `make ui-test` starts flaking on its own under
some other load, since that path is unchanged.
All seven `Closes` footers are in the commit bodies, one per line, so they will
take. Merging.
logan
merged commit 3607fe445e into main2026-08-19 14:53:46 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
logan
Started from a desktop report that could not be reproduced: the play/pause button showing pause while the seek bar sat still. Auditing the play/pause and position path for states that produce exactly that turned up seven defects, six of them in
backend/player.Commits
282dab4BufferedStreamerstreamed silence with no end condition2b84bc52b84bc5p.formatnever updated on load2b84bc5p.trackLengthMsinherited across tracks2b84bc5q.tracks[currentIndex]withcurrentIndex == -12b84bc561d549apre-pushran the browser tier in parallel with-race#123–#127 are one commit because they are one file's worth of tangled state and two of them do not compile apart. #122 is separate and self-contained.
The one that explains the report
Streamtreated an empty ring as a momentary underrun and answered with silence andok— correct only while the read-ahead is still going to deliver something, and two of its three exit paths left it never going to. Above that type nothing could tell the difference from healthy playback: the chain never ended, so the player stayed inPlayingwith the button showing pause, and the decoder's position never moved, so the 1 Hz report pinned the bar at a constant — and since every report resets the bar's interpolation, the report actively suppressed the only thing that would still have moved it.Not proven to be what was seen, since it was never reproduced. It is a state that produces precisely those symptoms and had no way out.
Two findings worth reading
The first fix was wrong and a test caught it. The silence fill was bounded by a count of calls, reasoning from the speaker's ~200 ms per call. A caller draining in a tight loop makes hundreds in microseconds, so
TestBufferedStreamer_BasicStreamspent the whole budget before the read-ahead goroutine was scheduled once and ended a good stream at sample zero. The bound is a duration and a run of calls now; it needs both, for opposite reasons.#127 was found by a test written for #123. The existing suite never loads a file outside
YELLOWJACKET_INTEGRATION, somake testwas green over a genuine race on every position emit — once a second for the whole of playback.srcMuis the lock that excludes the read-ahead goroutine and the position readers took the speaker lock, which that goroutine never holds. Taking it naively deadlocks, becauseseekLockedalready holds it and then emits the landing position from inside that region;seekSourceLockedis that region extracted.Verification
make lint— 0 issues, all three build configurations.make test— green, all three passes, race detector on.make bindings-check,make skill-check,make css-check,npx tsc --noEmit,make ui-test(898).index out of range [-1],p.format still holds the speaker rate (44100),a stalled source never ended the stream,WARNING: DATA RACE.Not covered: the audible half of #124 (a finished track replayed at the wrong pitch) is asserted through
p.format, not by listening; and #122's stall is reproduced with a synthetic source, since the real trigger is unknown.Side-effects worth knowing
OnPlaybackFinishedtakes anerrornow and gains//wails:ignore— v3's binding generator segfaults on a bound method taking anerror. It was never IPC, and this removes a binding the frontend could have called to force an auto-advance.Filed and not fixed here: a comment on #53 with two findings about
seek-bar, including that its stated hypothesis does not survive readingupdated().Closes #122
Closes #123
Closes #124
Closes #125
Closes #126
Closes #127
Closes #128
Reviewed the code rather than the description, and independently verified the
claims that carry the most risk. This is careful work; approving.
What I checked
go test -race -count=2 ./backend/player/... ./backend/queue/...— green.Ran twice deliberately, since the race this fixes fired once a second during
playback and a single pass proves less than it looks.
restoring the pre-fix condition (
len(q.tracks) == 0):TestFinishedWithNoCurrentTrackDoesNotPanicpanics withindex out of rangeathandlers.go:30, on a goroutine with no caller torecover it — exactly as described.
make lint— 0 issues across all three build configurations.make bindings-check— current, and nothing infrontend/src,frontend/testore2e/specsreferenced the removedOnPlaybackFinishedexport, so dropping it costs no caller.
The part worth praising
The
seekSourceLockedextraction is the right shape and the reasoning isright:
srcMuis not reentrant,displayPositionSecsLockedtakes it, andseekLockedused to emit the landing position from inside the region thatalready held it. Taking the lock naively there really would deadlock.
What makes it safe rather than lucky is that the constraint is written down
where it would be violated —
seekerLengthSecsLockedcarries "it must not take[the source lock] anyway: displayPositionSecsLocked calls this while holding
it". That is the comment that stops the next person reintroducing the deadlock
while fixing a different race, and it is in the callee rather than the caller,
which is where it can actually be read in time.
Same for
chainID: capturing it atspeaker.Playtime rather than readingp.chainIDin the callback is the whole fix, and the comment says so.One observation, not a blocker
Closes #128is honest about being a contention fix rather than a root-causeone — the import race presumably still exists, it just stops being reachable
from the gate. I think that is the right call for a gate ("a gate that fails at
random is not a gate"), and I have a corroborating data point: my own push was
rejected by this hook today on a branch based on current
main, and theidentical command passed standalone immediately after. So the flake is real,
still live on
main, and this is the fix for the case that actually blockspeople.
Worth keeping in mind only if
make ui-teststarts flaking on its own undersome other load, since that path is unchanged.
All seven
Closesfooters are in the commit bodies, one per line, so they willtake. Merging.