Fix/explore art scanner requests #21

Merged
yonlu merged 9 commits from fix/explore-art-scanner-requests into main 2026-08-18 13:48:37 +00:00
Owner

Eight fixes found while looking at why Explore had no album art. They are
independent of each other; the branch name is the first one.

Explore's album art was almost entirely missing

5 of 24 cards on the shelves had a cover, and those five were the ones
already on disk. The Cover Art Archive answers front-250 with a 307 to
an Internet Archive storage node, and those nodes are slow — measured
against Explore's own twelve shelf albums, a successful fetch took 14–16 s
and a failing one 13–17 s, against a client timeout of 10. So every live
fetch died, and a timeout writes nothing and says nothing, which is why
this reads as "Explore has no album art" rather than as a slow upstream.
The timeout is 30 s, clearing the measured range: the fetch is off the
critical path, so waiting costs nothing and giving up early costs the
whole page.

Two things found beside it. writeCache(mbid, nil) has recorded "the
archive has no art for this" as an empty file since it was written and
nothing ever read it back, so every art-less release group was re-fetched
on every render — a third of the shelves, each spending a live request to
be told again what the last one said. And the frontend marked a failed
fetch as permanently answered for the session, so a timed-out cover never
retried within it.

23 of 24 after.

The scanner was reading the wrong disk's profile

deviceForPath scanned /sys/block comparing device numbers and, failing
an exact match, took the first entry whose major agreed. Every SATA disk
is major 8, and a filesystem's st_dev is its partition, so the exact
match never hits for anything on one and the fallback resolved /dev/sdb3
to whatever /sys/block listed first — alphabetically, sda.

On the machine this was found on that is an SSD sitting next to the 6 TB
spinning disk the library is actually on, so IsRotationalDisk answered
false and the scanner ran one worker per core across a drive with one
head. It goes through /sys/dev/block/<major>:<minor> now and climbs to
the parent when that is a partition: one readlink, no scan, no ambiguity.

With the right disk identified, the scan is sized to it. In-flight reads
were a flat 2 for anything rotational, which is a pre-NCQ assumption — a
modern SATA disk reports a queue depth of 32 and reorders outstanding
reads into the order its head passes over them. It gets 4; a drive
reporting a queue depth of 1 keeps 2, because there every extra worker is
one more seek competing for one head. A prefetch stage then issues
POSIX_FADV_WILLNEED over the first 512 KB of each file, running 16 files
ahead of the workers, so the read a worker needs has been in flight for
sixteen files' worth of parsing by the time it asks. Rotational only.

Also: ScanConcurrency has been a validated three-value config field with
exactly one caller, passing the constant auto — so choosing ssd or
hdd by hand did nothing at all.

Downloads: guardrails in a unit that can mean something

MinKbps/MaxKbps/PreferredKbps replace the megabyte bounds on
auto-pick. 300 MB is a generous FLAC single and a suspiciously small
boxset, and whoever fills the field in has no idea which release the
pipeline will apply it to; dividing by runtime gives one number that holds
across a nine-minute EP and a three-hour opera. Artwork is out of the
numerator, or a folder with 30 MB of scans reads as a better rip. An
unknown runtime passes the window rather than failing it — the window is
a statement about quality, and refusing everything the moment MusicBrainz
lacks a track length would be a silent embargo. MaxFileSizeMB survives
as a separate ceiling, still in megabytes, because it is a question about
disk space.

The feature is called requests, and now the copy is too

The badge on every Explore card and track row still said "Want track X",
the album page's button read "Want this" / "Wanted", and the Downloads
empty state pointed at a control by a name nothing rendered. The queued
badge is a bookmark rather than an hourglass: an hourglass says "wait,
this is under way", which overstates a request — nothing may be
downloading, nothing may ever be found. The backend's 'wanted' state is
deliberately untouched; it is a stored enum, not copy.

This also removes a dead duplicate branch in the badge's render(): the
first if (this.actionable) returned before the ring was built, so a
partly-held album that could still be requested drew a plus instead of its
progress arc.

An unfavourited track was a colour, not a shape

favCtrl.iconName returned the solid glyph in both states, so "not a
favourite" was a filled heart in a duller colour — the two states
separated by hue alone, which fails WCAG 1.4.1 outright and reads as
"everything is a favourite" to everyone else. iconFor(favorited) returns
the outline or the fill, and the nine call sites split into the two cases
they always were: the three showing a state pass it, the rest are
context-menu items, which are actions and take the outline.

A row's leftover space belongs to the gaps

The three card grids laid out with justify: 'center' and a fixed 8px
gap, which gives the row a fixed width and pushes everything left over to
the two margins — measured on a 1440px window, cards 16px apart inside
78px of nothing down each side. The outside was five times the inside.
utils/grid-spacing.ts computes one number from what the row could not
spend on another card: 30px outside against 34px between, holding at any
width.

The virtualizer's own space-evenly + gap: 'auto' cannot be used — it
fits floor(width / cardWidth) columns without reserving the gap it is
about to need, so a width one card short of exact leaves seven cards a
pixel apart.

A flaky test fixture that guarded nothing

newServiceFixture stops auto-pick from starting a grab, because a
detached go m.grab(...) racing t.TempDir()'s cleanup is how these
tests fail. It did that with MaxSizeMB: 1 — and the size gates read
Candidate.TotalSize, which real providers fill and the fake leaves at
zero. Zero is under every ceiling, so the guard never fired and the race
kept happening, roughly one run in fifteen. It is now a format the fake
never produces: thirty consecutive whole-package runs, clean.


main is merged in (it had moved four commits ahead — the release
automation fixes — with no file overlap).

Tests: make lint and make test across all three build configurations,
plus the Vitest component suite and an e2e spec for the requested badge.

Eight fixes found while looking at why Explore had no album art. They are independent of each other; the branch name is the first one. ## Explore's album art was almost entirely missing 5 of 24 cards on the shelves had a cover, and those five were the ones already on disk. The Cover Art Archive answers `front-250` with a 307 to an Internet Archive storage node, and those nodes are slow — measured against Explore's own twelve shelf albums, a successful fetch took 14–16 s and a failing one 13–17 s, against a client timeout of 10. So every live fetch died, and a timeout writes nothing and says nothing, which is why this reads as "Explore has no album art" rather than as a slow upstream. The timeout is 30 s, clearing the measured range: the fetch is off the critical path, so waiting costs nothing and giving up early costs the whole page. Two things found beside it. `writeCache(mbid, nil)` has recorded "the archive has no art for this" as an empty file since it was written and nothing ever read it back, so every art-less release group was re-fetched on every render — a third of the shelves, each spending a live request to be told again what the last one said. And the frontend marked a failed fetch as permanently answered for the session, so a timed-out cover never retried within it. **23 of 24 after.** ## The scanner was reading the wrong disk's profile `deviceForPath` scanned `/sys/block` comparing device numbers and, failing an exact match, took the first entry whose *major* agreed. Every SATA disk is major 8, and a filesystem's `st_dev` is its **partition**, so the exact match never hits for anything on one and the fallback resolved `/dev/sdb3` to whatever `/sys/block` listed first — alphabetically, `sda`. On the machine this was found on that is an SSD sitting next to the 6 TB spinning disk the library is actually on, so `IsRotationalDisk` answered false and the scanner ran one worker per core across a drive with one head. It goes through `/sys/dev/block/<major>:<minor>` now and climbs to the parent when that is a partition: one readlink, no scan, no ambiguity. With the right disk identified, the scan is sized to it. In-flight reads were a flat 2 for anything rotational, which is a pre-NCQ assumption — a modern SATA disk reports a queue depth of 32 and reorders outstanding reads into the order its head passes over them. It gets 4; a drive reporting a queue depth of 1 keeps 2, because there every extra worker is one more seek competing for one head. A prefetch stage then issues `POSIX_FADV_WILLNEED` over the first 512 KB of each file, running 16 files ahead of the workers, so the read a worker needs has been in flight for sixteen files' worth of parsing by the time it asks. Rotational only. Also: `ScanConcurrency` has been a validated three-value config field with exactly one caller, passing the constant `auto` — so choosing `ssd` or `hdd` by hand did nothing at all. ## Downloads: guardrails in a unit that can mean something `MinKbps`/`MaxKbps`/`PreferredKbps` replace the megabyte bounds on auto-pick. 300 MB is a generous FLAC single and a suspiciously small boxset, and whoever fills the field in has no idea which release the pipeline will apply it to; dividing by runtime gives one number that holds across a nine-minute EP and a three-hour opera. Artwork is out of the numerator, or a folder with 30 MB of scans reads as a better rip. An unknown runtime *passes* the window rather than failing it — the window is a statement about quality, and refusing everything the moment MusicBrainz lacks a track length would be a silent embargo. `MaxFileSizeMB` survives as a separate ceiling, still in megabytes, because it is a question about disk space. ## The feature is called requests, and now the copy is too The badge on every Explore card and track row still said "Want track X", the album page's button read "Want this" / "Wanted", and the Downloads empty state pointed at a control by a name nothing rendered. The `queued` badge is a bookmark rather than an hourglass: an hourglass says "wait, this is under way", which overstates a request — nothing may be downloading, nothing may ever be found. The backend's `'wanted'` state is deliberately untouched; it is a stored enum, not copy. This also removes a dead duplicate branch in the badge's `render()`: the first `if (this.actionable)` returned before the ring was built, so a partly-held album that could still be requested drew a plus instead of its progress arc. ## An unfavourited track was a colour, not a shape `favCtrl.iconName` returned the solid glyph in both states, so "not a favourite" was a filled heart in a duller colour — the two states separated by hue alone, which fails WCAG 1.4.1 outright and reads as "everything is a favourite" to everyone else. `iconFor(favorited)` returns the outline or the fill, and the nine call sites split into the two cases they always were: the three showing a *state* pass it, the rest are context-menu items, which are actions and take the outline. ## A row's leftover space belongs to the gaps The three card grids laid out with `justify: 'center'` and a fixed 8px gap, which gives the row a fixed width and pushes everything left over to the two margins — measured on a 1440px window, cards 16px apart inside 78px of nothing down each side. The outside was five times the inside. `utils/grid-spacing.ts` computes one number from what the row could not spend on another card: 30px outside against 34px between, holding at any width. The virtualizer's own `space-evenly` + `gap: 'auto'` cannot be used — it fits `floor(width / cardWidth)` columns without reserving the gap it is about to need, so a width one card short of exact leaves seven cards a pixel apart. ## A flaky test fixture that guarded nothing `newServiceFixture` stops auto-pick from starting a grab, because a detached `go m.grab(...)` racing `t.TempDir()`'s cleanup is how these tests fail. It did that with `MaxSizeMB: 1` — and the size gates read `Candidate.TotalSize`, which real providers fill and the fake leaves at zero. Zero is under every ceiling, so the guard never fired and the race kept happening, roughly one run in fifteen. It is now a format the fake never produces: thirty consecutive whole-package runs, clean. --- `main` is merged in (it had moved four commits ahead — the release automation fixes — with no file overlap). Tests: `make lint` and `make test` across all three build configurations, plus the Vitest component suite and an e2e spec for the requested badge.
yonlu added 8 commits 2026-08-18 05:54:45 +00:00
Explore's album art was almost entirely missing: 5 of 24 cards on the
shelves had a cover, and those five were the ones already on disk.

The Cover Art Archive answers `front-250` with a 307 to an Internet
Archive storage node, and those nodes are slow. Measured against the
twelve albums on Explore's own shelves, a successful fetch took 14-16 s
and a failing one 13-17 s, against a client timeout of 10. So every
live fetch died, and a timeout writes nothing and says nothing -- which
is why this reads as "Explore has no album art" rather than as a slow
upstream. The timeout is 30 s, chosen to clear the measured range: the
fetch is off the critical path, so waiting costs nothing and giving up
early costs the whole page.

Two things beside it, both found on the way.

`writeCache(mbid, nil)` has recorded "the archive has no art for this"
as an empty file since it was written, and nothing has ever read it
back: `readCache` returns "" for an empty file, which is
indistinguishable from a miss. So every art-less release group was
re-fetched from CAA on every render that asked about it. A third of the
shelves are art-less, so that was a third of the page spending a live
request to be told again what the last one said. `knownMissing` reads
it, on both the release-group and the release path.

And the frontend marked a failed fetch as permanently answered for the
session, so a timed-out cover never retried within it. It drops the
marker instead; a genuine 404 is now answered from disk, so re-asking
one costs nothing.

Measured after: 23 of 24.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MeQt5hgXg5YGoNZQ9ozG7L
The three card grids -- albums, artists, genres -- laid out with
`justify: 'center'` and a fixed 8px gap and padding, which gives the
row a fixed width and pushes everything left over to the two margins.
Measured on a 1440px window: cards 16px apart inside 78px of nothing
down each side. The outside was five times the inside.

`utils/grid-spacing.ts` computes one number instead, from what the row
could not spend on another card: the same value between two cards,
between two rows, and down each edge. That window now reads 30px
outside against 34px between, and it holds at any width.

The virtualizer has a word for this -- `justify: 'space-evenly'` with
`gap: 'auto'` -- and it cannot be used. It fits `floor(width /
cardWidth)` columns without reserving the gap it is about to need, so a
width one card short of exact leaves seven cards a pixel apart. On the
window above it would fit 7 columns with 1px between them. Deciding the
column count here is what puts a floor under the spacing.

Two consequences. The layout is rebuilt when the container width
changes the spacing rather than only when the cover size changes, so
each grid observes its own scroller -- keyed on the spacing, or every
pixel of a drag rebuilds a layout that comes out the same. And
`cover-grid`'s ScrollManager took `GRID_GAP`/`GRID_PADDING` as
constants, which stopped describing anything the moment the spacing
became elastic: it asks the host for the geometry now, since a scroll
position rebuilt from a stale 8px lands in the wrong row.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MeQt5hgXg5YGoNZQ9ozG7L
`favCtrl.iconName` returned the solid glyph in both states, so "not a
favourite" was a filled heart in a duller colour and the only thing
separating the two states was hue. That fails outright for anyone who
cannot tell the two colours apart (WCAG 1.4.1), and reads as
"everything is a favourite" to everyone else.

`iconFor(favorited)` returns the outline or the fill, and the nine
`<wa-icon>` call sites split into the two cases they always were. The
three that show a *state* -- the mini player, the phone's now-playing
view, and the sidebar's marker for the favourites playlist itself --
pass it. The rest are context-menu items, which are actions rather than
states and take the outline `iconName` still returns.

`track-list` and `album-dropdown` already had this right, from inline
SVG paths of their own; this is the same rule for the call sites that
go through the icon library. `regular/star` is vendored to go with
`regular/heart`, which was already there.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MeQt5hgXg5YGoNZQ9ozG7L
The feature was renamed to requests and the copy was not. The badge on
every Explore card and track row still offered "Want track X", the
album page's button read "Want this" / "Wanted", the artist page's
release menu said "Want This", and the Downloads empty state told the
user to look for a control by a name nothing rendered.

The `queued` badge is a bookmark rather than an hourglass. An hourglass
says "wait, this is under way", which overstates what a request is:
nothing may be downloading, nothing may ever be found, and the list is
somewhere a user can leave one indefinitely. A bookmark says the honest
thing -- it is on your list -- and reads as the opposite of the plus
that put it there, which is what a toggle's two states have to do.

The backend's `'wanted'` request state is deliberately untouched: it is
a stored enum, not copy.

Also removes a dead duplicate branch in the badge's `render()`. The
first `if (this.actionable)` returned before the ring was built, so a
partly-held album that could still be requested drew a plus instead of
its progress arc.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MeQt5hgXg5YGoNZQ9ozG7L
Three faults, one subsystem, and the middle one is why a request that
looked obviously satisfiable came back refused.

**The guardrails were in megabytes, which cannot mean anything.** 300 MB
is a generous FLAC single and a suspiciously small boxset, and whoever
fills the field in has no idea which release the pipeline will apply it
to. `MinKbps`/`MaxKbps`/`PreferredKbps` are the same statement divided
by how long the music is, so one number holds across a nine-minute EP
and a three-hour opera. The runtime comes from `Download.Expected`,
which every anchored request already carries, so this costs no lookup;
the rate is audio bytes over that, falling back to the mean stated
per-file bitrate when the runtime is unknown. Artwork is excluded from
the numerator, or a folder with 30 MB of scans reads as a better rip.

An unknown runtime *passes* the window rather than failing it: the
window is a statement about quality, and refusing everything the moment
MusicBrainz is missing a track length would be a silent embargo.
`MaxFileSizeMB` survives as a separate ceiling, still in megabytes on
purpose -- it is a question about disk space, and it has to apply to a
candidate whose bitrate cannot be worked out at all.

**Auto-pick required daylight over the runner-up**, 0.08 on the
combined score, and so fired hardest in the case it was never written
for: a popular album turns up five *correct* copies, all matching the
tracklist at 95%+ and differing only in format and seeders, their
scores land within a point of each other, and it refused forever on the
grounds that the choice was the user's. It was not. There was no
question about what to fetch, only about which copy -- and abundance is
the condition under which that matters least. A candidate no longer has
to beat the field, only clear the bars on its own terms; where several
do, ranking puts the one closest to the preferred bitrate first.

That tie-break needed the preference to carry weight or it would have
been decorative in a new unit: `BitrateFit` was 0.05 against format's
0.42, so asking for 320 and being handed a FLAC every time was the
designed behaviour. When a preference is set the weights shift to fit
0.40 / format 0.20 / bitrate 0.10, taking it off the two heuristics
that exist as stand-ins for the preference the user has now given.
Health and priority are untouched. And the fit spans 0.5 to 1.0 rather
than 0 to 1, so a preference can promote the copy that matches it and
can never push the others under `minQuality` -- turning "I like 320"
into "never take anything else" silently is what `MinKbps`/`MaxKbps`
are for, out loud.

**And a refusal quoted numbers that passed.** The request list built its
message from `ranked[0]` -- the best candidate *before* the guardrails
and before the lead check -- so a request killed by the size window, or
by having too many good copies, reported "best of 12 found is not a
confident enough match (match 96%, quality 88%)". `AutoPickVeto` names
the gate that actually refused, and `AutoPickable` is that returning
empty.

Existing configs: the old `MinFileSizeMB`/`PreferredFileSizeMB` are not
migrated. A number meaning "300 MB" cannot be reinterpreted as a rate
without knowing the album it was aimed at, so carrying it over would be
inventing an intent nobody expressed. Those two fall back to no window,
which is the permissive default and what a fresh install gets;
`MaxFileSizeMB` carries over unchanged, because a ceiling on bytes
still means exactly what it did.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MeQt5hgXg5YGoNZQ9ozG7L
`newServiceFixture` stops auto-pick from starting a grab, because none
of its tests is about the download and a detached `go m.grab(...)`
racing `t.TempDir()`'s cleanup is how they fail. It did that with
`MaxSizeMB: 1` -- and the size gates read `Candidate.TotalSize`, which
real providers fill and the fake leaves at zero. Zero is under every
ceiling, so the guard never fired and the race it was written to
prevent kept happening, roughly one run in fifteen:

    TempDir RemoveAll cleanup: unlinkat ... : directory not empty

The guard is a format the fake never produces. Thirty consecutive
whole-package runs, none.

`TestManualDownloadSatisfiesRequestOnSuccess` was relying on the guard
being broken -- it is the one test here that wants the download -- so
it now clears the preferences itself rather than depending on a bug.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MeQt5hgXg5YGoNZQ9ozG7L
`deviceForPath` scanned `/sys/block` comparing device numbers and, when
no entry matched exactly, took the first one whose *major* agreed. Every
SATA disk is major 8. A filesystem's `st_dev` is its **partition**, so
the exact match never hits for anything on one, and the fallback then
resolved `/dev/sdb3` to whatever `/sys/block` listed first -- which is
alphabetical, which is `sda`.

On the machine this was found on that is a Samsung SSD sitting next to
the 6 TB spinning disk the library is actually on, so
`IsRotationalDisk` answered false and the scanner ran one worker per
core across a drive with one head. Matching on major alone cannot be
right on any machine with two disks, which is the case this exists for.

It goes through `/sys/dev/block/<major>:<minor>` instead -- a symlink
the kernel maintains to the device's own sysfs directory -- and climbs
to the parent when that turns out to be a partition. One readlink, no
scan, no ambiguity. The dev_t decode goes with it: Linux packs 12 bits
of major and 20 of minor split across the word, and masking the low
byte of each is right only for the first 256 of either.

`ProfileForPath` returns what the scanner needs to ask next, and the
new half is `queue_depth`: how many commands the drive will accept and
reorder at once. A SATA disk with NCQ enabled reports 31 or 32 and one
without reports 1, which is the difference between concurrency helping
and hurting. An absent file is read as "queues", because everything
that does not publish it -- NVMe, virtio, device-mapper -- is a device
where concurrency is fine.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MeQt5hgXg5YGoNZQ9ozG7L
perf(library): size the scan to the drive, and prefetch what it reads
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 2m47s
CI / e2e (pull_request) Successful in 5m59s
590a0d86dd
Every parser in `backend/metadata` is header-only -- a few hundred
bytes and return -- so on a spinning disk a scan is not waiting on CPU
or on bytes, it is waiting on the head to arrive. Two things follow,
and the drive says which.

**How many reads should be in flight.** This was a flat 2 for anything
rotational, which is a pre-NCQ assumption: a modern SATA disk reports a
queue depth of 32 and reorders outstanding reads into the order its
head passes over them, and was being handed a quarter of what it can
use. It gets 4 now. A drive that reports 1 -- a USB bridge, a pre-2004
disk -- services one command at a time in the order given, where every
extra worker is one more seek competing for one head and the scan gets
*slower* the harder it is pushed; that keeps 2.

**And that the next seek should already be queued.** A prefetch stage
between the walk and the workers issues `POSIX_FADV_WILLNEED` over the
first 512 KB of each file -- enough for an ID3v2 tag carrying cover
art, or FLAC's STREAMINFO and PICTURE blocks. The buffered channel *is*
the lookahead: the goroutine runs 16 files ahead of the workers,
hinting as it goes, so the read a worker needs has been in flight for
sixteen files' worth of parsing by the time it asks. Rotational only;
an SSD gets the channel back unwrapped and pays nothing, since it has
no seek to hide and already has one worker per core.

`workersForProfile` is the policy on its own so it can be tested
against drives this machine does not have, and the scan logs the
device, its rotational flag and its queue depth, so the decision is
inspectable rather than inferred.

Also: `ScanConcurrency` has been a validated three-value config field
with exactly one caller, passing the constant `auto` -- so choosing
`ssd` or `hdd` by hand did nothing at all. It reads the config now.
The two modes overrule detection about the *disk* and not about its
queue, since a user who picks `hdd` on a queueing drive still wants
that drive's queue used.

What is not here is inode-ordered dispatch. It needs the streaming walk
restructured to buffer per directory, and with queueing the drive is
already reordering what the hints put in front of it; that wants a
measurement on real hardware before the complexity.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MeQt5hgXg5YGoNZQ9ozG7L
yonlu added 1 commit 2026-08-18 11:45:00 +00:00
Merge remote-tracking branch 'origin/main' into fix/explore-art-scanner-requests
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 2m26s
CI / e2e (pull_request) Successful in 6m13s
48abecb830
yonlu merged commit 3bf27e3fd5 into main 2026-08-18 13:48:37 +00:00
Sign in to join this conversation.