diff --git a/adapters/lastfm/agent_test.go b/adapters/lastfm/agent_test.go
index 6e94a075d..94024b9ab 100644
--- a/adapters/lastfm/agent_test.go
+++ b/adapters/lastfm/agent_test.go
@@ -576,10 +576,8 @@ var _ = Describe("lastfmAgent", func() {
httpClient.Res = http.Response{Body: io.NopCloser(bytes.NewBufferString(lastfmError6)), StatusCode: 200}
_, err := agent.GetAlbumInfo(ctx, "123", "U2", "mbid-1234")
Expect(err).To(HaveOccurred())
- // error 6 is a definitive not-found, so it must satisfy the shared sentinel (else it
- // would trip the artwork worker's circuit breaker and be retried as a transient fault).
+ // A definitive not-found must satisfy the sentinel, or the artwork worker retries it.
Expect(errors.Is(err, agents.ErrNotFound)).To(BeTrue())
- // No MBID retry: album.getInfo is queried by name+artist only, in a single call.
Expect(httpClient.RequestCount).To(Equal(1))
Expect(httpClient.SavedRequest.URL.Query().Get("mbid")).To(BeEmpty())
})
@@ -615,8 +613,7 @@ var _ = Describe("lastfmAgent", func() {
It("maps a Last.fm error 6 (artist not found) to the shared not-found sentinel", func() {
apiClient.Res = http.Response{Body: io.NopCloser(bytes.NewBufferString(lastfmError6)), StatusCode: 200}
_, err := agent.GetArtistImages(ctx, "123", "Nonexistent Artist", "")
- // Definitive not-found, not a fault — must satisfy the sentinel through the %w wrap so
- // runs of missing artists never trip the artwork worker's circuit breaker.
+ // Not a fault: runs of missing artists must not trip the worker's circuit breaker.
Expect(errors.Is(err, agents.ErrNotFound)).To(BeTrue())
})
diff --git a/cmd/root.go b/cmd/root.go
index c4f6e9473..1fa173619 100644
--- a/cmd/root.go
+++ b/cmd/root.go
@@ -358,7 +358,7 @@ func startArtworkWorker(ctx context.Context, worker *artwork.Worker) func() erro
}
// scheduleArtworkHousekeeping runs the startup fingerprint backfill and registers the
-// recurring stale-absent recheck and prune jobs. Scan-triggered prune lands in a later phase.
+// recurring stale-absent recheck and prune jobs.
func scheduleArtworkHousekeeping(ctx context.Context, worker *artwork.Worker) func() error {
return func() error {
schedulerInstance := scheduler.GetInstance()
diff --git a/core/artwork/agent_images.go b/core/artwork/agent_images.go
index 681490936..bc4d0735a 100644
--- a/core/artwork/agent_images.go
+++ b/core/artwork/agent_images.go
@@ -13,9 +13,7 @@ import (
"github.com/navidrome/navidrome/utils/str"
)
-// externalName applies the DevPreserveUnicodeInExternalCalls normalization the aggregate
-// provider used, so agent searches match the same way (typographic quotes/dashes cleared
-// unless preserved).
+// externalName mirrors the normalization the aggregate provider applies, so agent searches match.
func externalName(name string) string {
if conf.Server.DevPreserveUnicodeInExternalCalls {
return name
@@ -23,9 +21,8 @@ func externalName(name string) string {
return str.Clear(name)
}
-// bestImageURL returns the largest-Size image URL, skipping empty or unparseable
-// URLs; nil when none qualifies. Parsing happens per candidate so a malformed largest
-// URL never shadows a valid smaller one.
+// bestImageURL returns the largest parseable image URL, so a malformed candidate never
+// shadows a smaller valid one.
func bestImageURL(imgs []agents.ExternalImage) *url.URL {
var best *url.URL
var bestSize int
@@ -44,12 +41,10 @@ func bestImageURL(imgs []agents.ExternalImage) *url.URL {
return best
}
-// fetchArtistImage tries each enabled artist-image agent in order, each under its own gate.
-// Returns the winning reader + agent name; extErr is true only when NO agent succeeded and
-// at least one failed transiently (a later success beats an earlier agent error).
+// fetchArtistImage tries each enabled artist-image agent in order. extErr is true only when no
+// agent succeeded and at least one failed transiently.
func fetchArtistImage(ctx context.Context, ag *agents.Agents, gate gateFunc, ar model.Artist) (r io.ReadCloser, agentName string, extErr bool) {
- // Synthetic artists have no real external image; mirror Agents.GetArtistImages' guard so a
- // direct retriever call can't assign an unrelated result to Unknown/Various Artists.
+ // Synthetic artists would otherwise get an unrelated agent result assigned to them.
switch ar.ID {
case consts.UnknownArtistID, consts.VariousArtistsID:
return nil, "", false
@@ -71,7 +66,7 @@ func fetchArtistImage(ctx context.Context, ag *agents.Agents, gate gateFunc, ar
return reader, a.Name, false
}
if isTransientExternal(err) {
- extErr = true // includes errBreakerOpen and download failures: retry via the next agent
+ extErr = true
log.Debug(ctx, "Artwork: External artist-image lookup failed", "agent", a.Name, "artist", ar.Name, err)
}
}
diff --git a/core/artwork/agent_images_test.go b/core/artwork/agent_images_test.go
index 8525ab377..f57b3c19a 100644
--- a/core/artwork/agent_images_test.go
+++ b/core/artwork/agent_images_test.go
@@ -54,9 +54,8 @@ func (f *fakeImageAgent) GetAlbumImages(_ context.Context, name, _, _ string) ([
return f.imgs, f.err
}
-// imageAgents registers the fakes as built-in agents (ignoring the DataStore) and
-// enables them in order, returning the process-wide Agents. Because the fakes ignore
-// ds, reusing the GetAgents singleton across tests is safe.
+// imageAgents registers the fakes as built-in agents and enables them in order. The fakes
+// ignore the DataStore, so reusing the process-wide GetAgents singleton across tests is safe.
func imageAgents(fakes ...*fakeImageAgent) *agents.Agents {
names := make([]string, 0, len(fakes))
for _, f := range fakes {
diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go
index 1f75b8550..6cf466c64 100644
--- a/core/artwork/artwork.go
+++ b/core/artwork/artwork.go
@@ -20,32 +20,30 @@ import (
var ErrUnavailable = errors.New("artwork unavailable")
-// errStaleSource signals that a backing file's mtime no longer matches the state
-// row's RefMtime: the stored hash may be stale, so the load is aborted (dangling).
+// errStaleSource means the backing file's mtime no longer matches RefMtime, so the stored hash may be stale.
var errStaleSource = errors.New("artwork: source file changed since resolution")
// Image is one servable artwork response.
type Image struct {
io.ReadCloser
- Hash string // pixel-identity hash (immutable URL match); "" for placeholders
- ETag string // served-representation validator; "" falls back to Hash (full-size original)
- LastUpdated time.Time // zero for placeholders
+ Hash string // pixel identity; "" for placeholders
+ ETag string // representation validator; "" means Hash applies (full-size original)
+ LastUpdated time.Time
Placeholder bool
}
-// representationTag identifies a served resized representation for HTTP validation: it changes with
-// the dimensions and the encode settings (CoverArtQuality/EnableWebPEncoding), so a config change
-// invalidates a revalidating client's cache even though the pixel hash is unchanged.
+// representationTag varies with dimensions and encode settings, so a config change invalidates
+// a revalidating client's cache even though the pixel hash is unchanged.
func representationTag(hash string, size int, square bool) string {
return fmt.Sprintf("%s.%d.%v.%s", hash, size, square, formatQualityTag())
}
type Artwork interface {
- // Get serves resolved/provisional artwork; ErrUnavailable or model.ErrNotFound when
- // there is nothing to serve (absent, pending, dangling) — caller picks placeholder vs 404.
+ // Get returns ErrUnavailable when there is nothing to serve and model.ErrNotFound when
+ // the id resolves to nothing, so the caller can pick placeholder vs 404.
Get(ctx context.Context, artID model.ArtworkID, size int, square bool) (*Image, error)
- // GetOrPlaceholder parses a raw id token (raw entity ids accepted, as today) and falls
- // back to the kind's placeholder image (never resized, Placeholder=true).
+ // GetOrPlaceholder accepts an artwork token or a raw entity id, falling back to the
+ // kind's placeholder image (never resized, Placeholder=true).
GetOrPlaceholder(ctx context.Context, id string, size int, square bool) (*Image, error)
}
@@ -55,7 +53,6 @@ func NewArtwork(ds model.DataStore, cache cache.FileCache, store *ImageStore, ff
// EntityExists reports whether the entity an artwork id points at is still there: state rows
// outlive a deleted entity until the next prune, so a servable row is not evidence of its owner.
-// The repositories are ctx-scoped, so a request context makes this a visibility check too.
func EntityExists(ctx context.Context, ds model.DataStore, artID model.ArtworkID) bool {
var found bool
var err error
@@ -95,8 +92,8 @@ func (s *service) GetOrPlaceholder(ctx context.Context, id string, size int, squ
if err == nil {
img, err = s.Get(ctx, artID, size, square)
}
- // Only a resolvable entity with no art gets the placeholder. An id that matches no entity
- // stays ErrNotFound, so getCoverArt keeps answering error 70 and Jellyfin keeps 404ing.
+ // Only a resolvable entity with no art gets a placeholder; an unknown id must stay
+ // ErrNotFound so callers can still answer 404 / Subsonic error 70.
if errors.Is(err, ErrUnavailable) {
return placeholderImage(artID.Kind), nil
}
@@ -108,7 +105,7 @@ func (s *service) Get(ctx context.Context, artID model.ArtworkID, size int, squa
return nil, ErrUnavailable
}
if size < 0 {
- size = 0 // a negative size is a full-size request, not a giant (OOM) resize rectangle
+ size = 0 // a negative size means full-size, not a giant (OOM) resize rectangle
}
switch artID.Kind {
case model.KindDiscArtwork:
@@ -120,13 +117,10 @@ func (s *service) Get(ctx context.Context, artID model.ArtworkID, size int, squa
}
}
-// requestRecheckAge throttles view-triggered rechecks of an absent entity so repeatedly opening a
-// genuinely-absent page can't hammer external services; below staleAbsentAge to catch younger absences.
+// requestRecheckAge throttles view-triggered rechecks so reopening a genuinely-absent page can't
+// hammer external services; below staleAbsentAge to catch younger absences.
const requestRecheckAge = time.Hour
-// serveEntity serves an entity whose state the worker owns (album/artist/playlist/radio):
-// found row serves its hash, absent row is unavailable (promoting a stale recheck on view),
-// missing row reads through provisionally.
func (s *service) serveEntity(ctx context.Context, artID model.ArtworkID, size int, square bool) (*Image, error) {
ia, err := s.ds.Artwork(ctx).GetItemArtwork(artID.Kind, artID.ID, model.ImageTypePrimary)
switch {
@@ -135,8 +129,8 @@ func (s *service) serveEntity(ctx context.Context, artID model.ArtworkID, size i
case err != nil:
return nil, err
case ia.Hash == "":
- // EnqueueBump preserves an existing backoff row's retry_at; for a settled absent row
- // (no queue row) it inserts a fresh, immediately-eligible recheck.
+ // EnqueueBump preserves an existing backoff row's retry_at, and inserts an
+ // immediately-eligible recheck for a settled absent row.
if time.Since(ia.AttemptedAt) > requestRecheckAge {
s.enqueue(ctx, artID, model.ArtworkPriorityBump)
}
@@ -146,10 +140,8 @@ func (s *service) serveEntity(ctx context.Context, artID model.ArtworkID, size i
}
}
-// serveSource is the one place bytes become an Image: a full-size request streams open()
-// directly, anything else goes through the resize cache under key. hash is the pixel identity
-// where one exists ("" for disc art, which has no state row) and doubles as the full-size
-// validator, so an ETag is only needed when the bytes are resized or the hash is missing.
+// serveSource is the one place bytes become an Image. hash is the pixel identity ("" for disc art)
+// and doubles as the full-size validator, so an ETag is only needed when resized or hash is "".
func (s *service) serveSource(ctx context.Context, key, hash string, lastUpdate time.Time,
size int, square bool, open func() (io.ReadCloser, error),
) (*Image, error) {
@@ -176,11 +168,10 @@ func (s *service) serveSource(ctx context.Context, key, hash string, lastUpdate
return &Image{ReadCloser: stream, Hash: hash, ETag: representationTag(key, size, square), LastUpdated: lastUpdate}, nil
}
-// serveHash serves the bytes of a found state row. A mismatch/open error is dangling (a warm
-// cache still serves), but a cancelled request is not: it must not enqueue a re-resolution.
+// serveHash serves the bytes of a found state row. A mismatch/open error is dangling, but a
+// cancelled request is not: it must not enqueue a re-resolution.
func (s *service) serveHash(ctx context.Context, artID model.ArtworkID, ia *model.ItemArtwork, size int, square bool) (*Image, error) {
- // Only this path can hand back a deleted entity's bytes: an absent row is already
- // unavailable, and the provisional and disc paths load their entity to resolve at all.
+ // Only this path can hand back a deleted entity's bytes; the others load their entity anyway.
if !EntityExists(ctx, s.ds, artID) {
return nil, ErrUnavailable
}
@@ -203,8 +194,7 @@ func (s *service) serveHash(ctx context.Context, artID model.ArtworkID, ia *mode
return img, nil
}
-// openOriginal opens the full-resolution bytes for a found state row, enforcing the
-// mtime invariant: bytes are never served under a hash they no longer match.
+// openOriginal enforces the mtime invariant: bytes are never served under a hash they no longer match.
func openOriginal(ia *model.ItemArtwork, mime string, store *ImageStore) (io.ReadCloser, error) {
if isFileBacked(ia.Source) {
f, err := os.Open(ia.SourcePath)
@@ -224,8 +214,7 @@ func openOriginal(ia *model.ItemArtwork, mime string, store *ImageStore) (io.Rea
}
return f, nil
}
- // Store-backed (embedded/external/generated): the bytes live in the content-addressed
- // store, but an embedded source still carries the audio file's mtime to detect edits.
+ // Store-backed bytes still carry the source's mtime, to detect edits to embedded art.
if ia.SourcePath != "" && ia.RefMtime != 0 {
info, err := os.Stat(ia.SourcePath)
if err != nil {
@@ -240,8 +229,8 @@ func openOriginal(ia *model.ItemArtwork, mime string, store *ImageStore) (io.Rea
return store.Open(ia.Hash, mime)
}
-// provisional does a local-only read-through for an entity with no state row: it enqueues
-// the worker (Bump) and serves any local bytes immediately, never writing a state row.
+// provisional serves local bytes for an entity with no state row, enqueuing the worker but
+// never writing a state row itself.
func (s *service) provisional(ctx context.Context, artID model.ArtworkID, size int, square bool) (*Image, error) {
item := model.ArtworkQueueItem{ItemKind: artID.Kind.Prefix(), ItemID: artID.ID, ImageType: model.ImageTypePrimary}
res, err := newLocalResolver(s.ds, s.ffmpeg).resolve(ctx, item)
@@ -254,8 +243,7 @@ func (s *service) provisional(ctx context.Context, artID model.ArtworkID, size i
return s.serveResolution(ctx, res, size, square)
}
-// serveResolution turns a local resolution's bytes into a servable Image (byte-hash
-// only, no decode). A resolution with no reader is unavailable.
+// serveResolution turns a local resolution's bytes into a servable Image (byte-hash only, no decode).
func (s *service) serveResolution(ctx context.Context, res resolution, size int, square bool) (*Image, error) {
if res.reader == nil {
return nil, ErrUnavailable
@@ -274,12 +262,9 @@ func (s *service) serveResolution(ctx context.Context, res resolution, size int,
func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(data)), nil })
}
-// serveMediaFile serves a track: own found art wins; an absent row delegates to the album;
-// a missing row extracts embedded art (if eligible, enqueuing) else delegates without enqueue.
func (s *service) serveMediaFile(ctx context.Context, artID model.ArtworkID, size int, square bool) (*Image, error) {
- // Per-track art can be disabled after mf rows were resolved (the setting is not in the
- // config fingerprint). Honor it at serve time so a direct mf- URL falls back to disc/album
- // instead of serving stale persisted embedded art.
+ // The setting is not in the config fingerprint, so honor it at serve time: a direct mf- URL
+ // must fall back to disc/album instead of serving stale persisted embedded art.
if !conf.Server.EnableMediaFileCoverArt {
mf, err := s.ds.MediaFile(ctx).Get(artID.ID)
if err != nil {
@@ -292,9 +277,9 @@ func (s *service) serveMediaFile(ctx context.Context, artID model.ArtworkID, siz
case err == nil && ia.Hash != "":
return s.serveHash(ctx, artID, ia, size, square)
case err == nil:
- // absent row → fall through to album delegation
+ // absent row: fall through
case errors.Is(err, model.ErrNotFound):
- // no row → fall through to embedded eligibility / album delegation
+ // no row: fall through
default:
return nil, err
}
@@ -307,13 +292,11 @@ func (s *service) serveMediaFile(ctx context.Context, artID model.ArtworkID, siz
if noRow && conf.Server.EnableMediaFileCoverArt && mf.HasCoverArt {
return s.provisionalEmbedded(ctx, artID, *mf, size, square)
}
- // Mirror MediaFile.CoverArtID's fallback: a multi-disc track defers to its disc art
- // (which itself falls back to the album), not straight to the album.
+ // Mirror MediaFile.CoverArtID: a track defers to its disc art, which falls back to the album.
return s.Get(ctx, mf.DiscCoverArtID(), size, square)
}
-// provisionalEmbedded extracts a track's embedded art for an immediate serve and always
-// enqueues the track (Bump) so the worker persists state; it never writes a state row.
+// provisionalEmbedded serves a track's embedded art immediately, leaving the state row to the worker.
func (s *service) provisionalEmbedded(ctx context.Context, artID model.ArtworkID, mf model.MediaFile, size int, square bool) (*Image, error) {
lib, err := loadLibraryView(ctx, s.ds, mf.LibraryID)
if err != nil {
@@ -322,31 +305,26 @@ func (s *service) provisionalEmbedded(ctx context.Context, artID model.ArtworkID
res, ok := resolveEmbedded(ctx, lib, s.ffmpeg, mf.Path)
s.enqueue(ctx, artID, model.ArtworkPriorityBump)
if !ok {
- // Eligible but unextractable (truncated frame, no ffmpeg): fall back the way
- // CoverArtID does rather than answer with a placeholder.
+ // Eligible but unextractable: fall back the way CoverArtID does, not to a placeholder.
return s.Get(ctx, mf.DiscCoverArtID(), size, square)
}
return s.serveResolution(ctx, res, size, square)
}
-// serveDisc serves disc-level artwork as a pure provisional read-through: no state rows,
-// no enqueue. It tries the disc-folder selection chain and falls back to the album cover.
+// serveDisc reads disc art through with no state row and no enqueue, falling back to the album cover.
func (s *service) serveDisc(ctx context.Context, artID model.ArtworkID, size int, square bool) (*Image, error) {
dr, err := newDiscArtworkReader(ctx, s.ds, artID)
if err != nil {
return nil, err
}
- // Single-disc albums run the chain too: a disc can carry its own art, distinct from the
- // album cover, and DiscArtPriority is what expresses that preference.
+ // Single-disc albums run the chain too: a disc can carry art distinct from the album cover.
selectImage := func() (io.ReadCloser, string, error) {
funcs := dr.fromDiscArtPriority(ctx, s.ffmpeg, conf.Server.DiscArtPriority)
return selectImageReader(ctx, artID, funcs...)
}
albumArtID := model.ArtworkID{Kind: model.KindAlbumArtwork, ID: dr.album.ID}
- // Disc art has no state row, so there is no stored content hash — to key the resize cache
- // on, or to fall back to as a validator. Keying on the id and the album's mtime, as the
- // legacy reader did, lets a warm cache answer without touching the filesystem; the chain
- // runs only on a miss. The key carries DiscArtPriority so changing it invalidates.
+ // Disc art has no state row, hence no content hash: keying on id, album mtime and
+ // DiscArtPriority lets a warm cache answer without running the chain or touching the disk.
key := fmt.Sprintf("%s|%d|%s", artID.ID, dr.cacheTime().UnixNano(), conf.Server.DiscArtPriority)
img, err := s.serveSource(ctx, key, "", dr.cacheTime(), size, square,
func() (io.ReadCloser, error) { rc, _, err := selectImage(); return rc, err })
@@ -359,16 +337,14 @@ func (s *service) serveDisc(ctx context.Context, artID model.ArtworkID, size int
return img, nil
}
-// dangling enqueues a re-resolution at Scan priority and reports the artwork as
-// unavailable, leaving the state row untouched.
+// dangling enqueues a re-resolution and reports unavailable, leaving the state row untouched.
func (s *service) dangling(ctx context.Context, artID model.ArtworkID) (*Image, error) {
log.Debug(ctx, "Artwork: State row points at bytes we cannot serve, re-resolving", "artID", artID)
s.enqueue(ctx, artID, model.ArtworkPriorityScan)
return nil, ErrUnavailable
}
-// enqueue schedules a request-triggered re-resolution. It uses EnqueueBump so an incidental
-// read-through never resets a failed resolution's backoff (unlike scan/manual re-resolve).
+// enqueue uses EnqueueBump so an incidental read-through never resets a failed resolution's backoff.
func (s *service) enqueue(ctx context.Context, artID model.ArtworkID, priority int) {
err := s.ds.ArtworkQueue(ctx).EnqueueBump(model.ArtworkQueueItem{
ItemKind: artID.Kind.Prefix(),
@@ -390,8 +366,8 @@ func placeholderImage(kind model.Kind) *Image {
return &Image{ReadCloser: r, Placeholder: true}
}
-// PlaceholderFor returns the kind-appropriate placeholder for an artwork id, for callers that must
-// serve a placeholder without consulting persisted state (e.g. an access-control denial).
+// PlaceholderFor returns the kind-appropriate placeholder for an artwork id, for callers that
+// must not consult persisted state (e.g. an access-control denial).
func PlaceholderFor(id string) *Image {
artID, _ := model.ParseArtworkID(id)
return placeholderImage(artID.Kind)
@@ -401,8 +377,7 @@ type coverArtIDGetter interface {
CoverArtID() model.ArtworkID
}
-// parseArtworkID ports the legacy getArtworkId: parse the token, and if it is a raw
-// entity id, resolve the entity and take its CoverArtID.
+// parseArtworkID accepts an artwork token or a raw entity id, resolving the latter to its CoverArtID.
func (s *service) parseArtworkID(ctx context.Context, id string) (model.ArtworkID, error) {
if id == "" {
return model.ArtworkID{}, ErrUnavailable
diff --git a/core/artwork/artwork_suite_test.go b/core/artwork/artwork_suite_test.go
index b3a76a768..0f93f49a1 100644
--- a/core/artwork/artwork_suite_test.go
+++ b/core/artwork/artwork_suite_test.go
@@ -38,7 +38,6 @@ func TestArtwork(t *testing.T) {
}
// osDirFS wraps os.DirFS as a storage.MusicFS for integration tests.
-// ReadTags is not exercised by these tests, so it is left as a stub.
type osDirFS struct{ fs.FS }
func (o osDirFS) ReadTags(...string) (map[string]metadata.Info, error) { return nil, nil }
diff --git a/core/artwork/artwork_test.go b/core/artwork/artwork_test.go
index 4ee99edec..59429d8b4 100644
--- a/core/artwork/artwork_test.go
+++ b/core/artwork/artwork_test.go
@@ -41,8 +41,6 @@ var _ = Describe("Artwork", func() {
primaryKey := func(kind, id string) string { return kind + "|" + id + "|" + model.ImageTypePrimary }
- // seedFoundStore installs a store-backed found state (bytes in the content-addressed
- // store, no backing file) and returns the hash.
seedFoundStore := func(kind, id string, imgBytes []byte) string {
hash, err := hashImage(bytes.NewReader(imgBytes))
Expect(err).ToNot(HaveOccurred())
@@ -53,8 +51,7 @@ var _ = Describe("Artwork", func() {
return hash
}
- // seedEntity registers the owning entity, without which the state row describes something
- // that no longer exists and the service correctly refuses to serve it.
+ // Without its owning entity, a state row is not served at all.
seedEntity = func(kind, id string) {
GinkgoHelper()
switch kind {
@@ -119,8 +116,7 @@ var _ = Describe("Artwork", func() {
img, err := svc.Get(ctx, model.MustParseArtworkID("al-al1"), 100, false)
Expect(err).ToNot(HaveOccurred())
- // A resized response versions its validator with the encode settings, distinct from
- // the pixel hash, so a CoverArtQuality/WebP change invalidates client caches.
+ // A resized response versions its ETag with the encode settings, not the pixel hash.
Expect(img.ETag).To(Equal(representationTag(img.Hash, 100, false)))
Expect(img.ETag).ToNot(Equal(img.Hash))
resized := readAll(img)
@@ -128,8 +124,7 @@ var _ = Describe("Artwork", func() {
Expect(err).ToNot(HaveOccurred())
Expect(cfg.Width).To(Equal(100))
- // Delete the store file: a warm resize-cache entry must keep serving without
- // ever touching the original (the stale-serve self-heal).
+ // Deleting the store file proves the warm entry serves without touching the original.
hash, _ := hashImage(bytes.NewReader(coverBytes))
Expect(store.Remove(hash, "image/jpeg", time.Now().Add(time.Hour))).To(Succeed())
Eventually(func(g Gomega) {
@@ -199,8 +194,7 @@ var _ = Describe("Artwork", func() {
Expect(queueRepo.Data[primaryKey("al", "al3b")].Priority).To(Equal(model.ArtworkPriorityScan))
})
- // State rows and their bytes outlive a deleted entity until the next prune, so serving
- // straight from the row would keep handing out a removed entity's image.
+ // State rows outlive a deleted entity until the next prune.
It("refuses to serve a found row whose entity is gone", func() {
hash := seedFoundStore("al", "alzz", coverBytes)
Expect(hash).ToNot(BeEmpty())
@@ -228,7 +222,6 @@ var _ = Describe("Artwork", func() {
_, err := svc.Get(ctx, model.MustParseArtworkID("al-al4b"), 0, false)
Expect(err).To(MatchError(ErrUnavailable))
Expect(queueRepo.Data[primaryKey("al", "al4b")].Priority).To(Equal(model.ArtworkPriorityBump))
- // The absent state row is left intact; only a recheck is scheduled.
ia, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "al4b", model.ImageTypePrimary)
Expect(err).ToNot(HaveOccurred())
Expect(ia.Hash).To(BeEmpty())
@@ -272,7 +265,6 @@ var _ = Describe("Artwork", func() {
It("ignores a resolved mf row and delegates to the album when per-track art is disabled", func() {
conf.Server.EnableMediaFileCoverArt = false
- // A resolved mf row exists (from when the setting was on) but must not be served.
seedFoundStore("mf", "mf7", []byte("stale embedded track art"))
seedFoundStore("al", "albz", coverBytes)
mfRepo.SetData(model.MediaFiles{{ID: "mf7", AlbumID: "albz"}})
@@ -322,7 +314,6 @@ var _ = Describe("Artwork", func() {
})
It("delegates a multi-disc track to its disc art, not straight to the album", func() {
- // Not embedded-eligible: the fallback must mirror CoverArtID (disc first, then album).
folderRepo.result = []model.Folder{{Path: "tests/fixtures/artist/an-album", ImageFiles: []string{"cover.jpg"}}}
albumRepo.SetData(model.Albums{{ID: "aldd", Name: "Album", FolderIDs: []string{"f1"}, Discs: model.Discs{1: "One", 2: "Two"}}})
seedFoundStore("al", "aldd", []byte("album-art-distinct")) // album's own found art differs
@@ -330,15 +321,13 @@ var _ = Describe("Artwork", func() {
img, err := svc.Get(ctx, model.MustParseArtworkID("mf-mf5"), 0, false)
Expect(err).ToNot(HaveOccurred())
- // The disc-folder image wins over the album's found art, proving it routed via serveDisc.
+ // The disc-folder image, not the album's own art: proof it routed through serveDisc.
Expect(readAll(img)).To(Equal(coverBytes))
})
- // Jellyfin clients are now told to request the track image before it resolves, so an
- // unextractable frame must not answer with a placeholder where the album has art.
It("falls back to the album when an eligible track's embedded art will not extract", func() {
conf.Server.EnableMediaFileCoverArt = true
- // HasCoverArt is set, but the file yields no extractable image (it is not audio).
+ // HasCoverArt is set, but the file is not audio, so nothing extracts.
mfRepo.SetData(model.MediaFiles{{
ID: "mfbad", AlbumID: "albad", LibraryID: 0, HasCoverArt: true,
Path: "tests/fixtures/artist/an-album/front.png",
@@ -352,8 +341,7 @@ var _ = Describe("Artwork", func() {
})
It("routes a single-disc track through disc resolution too", func() {
- // A single disc can carry its own art: DiscArtPriority still applies, so the disc
- // image is served even when the album has different found art.
+ // DiscArtPriority applies to single-disc albums too, over the album's own found art.
folderRepo.result = []model.Folder{{Path: "tests/fixtures/artist/an-album", ImageFiles: []string{"cover.jpg"}}}
albumRepo.SetData(model.Albums{{ID: "alsd", Name: "Album", FolderIDs: []string{"f1"}, Discs: model.Discs{1: ""}}})
seedFoundStore("al", "alsd", []byte("album-art-distinct"))
@@ -375,9 +363,8 @@ var _ = Describe("Artwork", func() {
Expect(readAll(img)).To(Equal(coverBytes))
})
- // The resize cache is keyed on the id and the album's mtime, not on the image bytes, so a
- // warm hit needs no filesystem access. Dropping the source between the two requests is
- // how that shows: re-reading would fall back to the album instead.
+ // The resize cache keys on id + album mtime, not on the bytes, so dropping the source
+ // between the two requests is what shows a warm hit never touches the filesystem.
It("serves a sized disc image from cache without re-reading the source", func() {
folderRepo.result = []model.Folder{{Path: "tests/fixtures/artist/an-album", ImageFiles: []string{"cover.jpg"}}}
albumRepo.SetData(model.Albums{{ID: "aldc3", Name: "Album", FolderIDs: []string{"f1"}}})
@@ -388,8 +375,6 @@ var _ = Describe("Artwork", func() {
warmed := readAll(first)
Expect(warmed).ToNot(BeEmpty())
- // With the source gone and the album holding no art of its own, a re-read would
- // fall through to the album and fail.
folderRepo.result = nil
second, err := svc.Get(ctx, discID, 64, false)
@@ -397,8 +382,7 @@ var _ = Describe("Artwork", func() {
Expect(readAll(second)).To(Equal(warmed), "a warm sized request must not touch the source")
})
- // A disc image can be replaced without the album row changing, so the key folds in the
- // folder's ImagesUpdatedAt; keying on album.UpdatedAt alone would serve the old image.
+ // A disc image can change without the album row changing, so the key folds in ImagesUpdatedAt.
It("invalidates the cached image when the folder's images change", func() {
folderRepo.result = []model.Folder{{
Path: "tests/fixtures/artist/an-album", ImageFiles: []string{"cover.jpg"},
@@ -412,7 +396,6 @@ var _ = Describe("Artwork", func() {
firstKey := first.ETag
readAll(first)
- // The image was replaced: same album row, newer folder images timestamp.
folderRepo.result[0].ImagesUpdatedAt = time.Now()
second, err := svc.Get(ctx, discID, 64, false)
Expect(err).ToNot(HaveOccurred())
@@ -420,8 +403,7 @@ var _ = Describe("Artwork", func() {
Expect(second.ETag).ToNot(Equal(firstKey), "a replaced image must not keep the old cache entry")
})
- // Disc art has no content hash to fall back to, so without an explicit validator the
- // full-size response would carry an empty ETag — identical for every disc image.
+ // Disc art has no content hash, so without an explicit validator every ETag would be empty.
It("gives a full-size disc image a validator that tracks the source", func() {
folderRepo.result = []model.Folder{{
Path: "tests/fixtures/artist/an-album", ImageFiles: []string{"cover.jpg"},
@@ -490,8 +472,7 @@ var _ = Describe("Artwork", func() {
Expect(readAll(img)).To(Equal(phBytes))
})
- // An entity that has no art and an id that names no entity are different answers:
- // Subsonic reports error 70 for the latter, and Jellyfin 404s.
+ // "No art" and "no such entity" are different answers: clients 404 only on the latter.
It("reports not-found rather than a placeholder for an id with no entity", func() {
_, err := svc.GetOrPlaceholder(ctx, "al-nosuchalbum", 0, false)
Expect(err).To(MatchError(model.ErrNotFound))
diff --git a/core/artwork/blurhash/blurhash.go b/core/artwork/blurhash/blurhash.go
index 302ec83c0..3fe8efbec 100644
--- a/core/artwork/blurhash/blurhash.go
+++ b/core/artwork/blurhash/blurhash.go
@@ -1,5 +1,5 @@
-// Package blurhash implements the blurhash encoding algorithm (https://github.com/woltapp/blurhash),
-// matching Jellyfin's parameters so clients tuned against Jellyfin see equivalent hashes.
+// Package blurhash implements the blurhash encoding (https://github.com/woltapp/blurhash),
+// parameterized to match Jellyfin so clients see equivalent hashes.
package blurhash
import (
@@ -15,10 +15,10 @@ import (
const alphabet = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz#$%*+,-.:;=?@[]^_{|}~"
-// maxInputSize matches Jellyfin: larger inputs are slower with no visually discernible difference.
+// maxInputSize: larger inputs are slower with no visible difference in the result.
const maxInputSize = 128
-// Components picks x/y component counts for an image, targeting ~16 near-square tiles (Jellyfin's formula).
+// Components picks x/y component counts targeting ~16 near-square tiles.
func Components(width, height int) (int, int) {
if width <= 0 || height <= 0 {
return 0, 0
@@ -109,8 +109,7 @@ func Encode(img image.Image, xComp, yComp int) (string, error) {
return sb.String(), nil
}
-// toRGBA gives the pixel loop direct Pix access, avoiding a per-pixel allocation through the
-// image.At interface (~16k allocs per encode).
+// toRGBA gives the pixel loop direct Pix access, avoiding a per-pixel allocation via image.At.
func toRGBA(img image.Image) *image.RGBA {
if rgba, ok := img.(*image.RGBA); ok {
return rgba
@@ -165,8 +164,7 @@ func linearToSRGB(v float64) int {
return int((1.055*math.Pow(v, 1/2.4)-0.055)*255 + 0.5)
}
-// Encode83 encodes value as a fixed-width, big-endian base83 string of the given length, using the
-// blurhash spec's alphabet.
+// Encode83 encodes value as a fixed-width, big-endian base83 string of the given length.
func Encode83(value, length int) string {
b := make([]byte, length)
for i := length - 1; i >= 0; i-- {
diff --git a/core/artwork/blurhash/blurhash_test.go b/core/artwork/blurhash/blurhash_test.go
index 18dd86b99..b457eac64 100644
--- a/core/artwork/blurhash/blurhash_test.go
+++ b/core/artwork/blurhash/blurhash_test.go
@@ -103,7 +103,6 @@ var _ = Describe("Encode", func() {
})
It("downscales large images internally without changing the result materially", func() {
- // A 1000px solid image must encode fine and carry the same DC as its small version.
big, err := blurhash.Encode(solidImage(1000, 1000, color.NRGBA{R: 60, G: 120, B: 180, A: 255}), 5, 5)
Expect(err).ToNot(HaveOccurred())
small, err := blurhash.Encode(solidImage(16, 16, color.NRGBA{R: 60, G: 120, B: 180, A: 255}), 5, 5)
diff --git a/core/artwork/disc.go b/core/artwork/disc.go
index 84d4543b3..049d2bf3f 100644
--- a/core/artwork/disc.go
+++ b/core/artwork/disc.go
@@ -27,14 +27,13 @@ type discArtworkReader struct {
isMultiFolder bool
firstTrackRel string // library-relative; for fromTag / ffmpeg via lib.Abs
lib libraryView
- // imagesUpdatedAt is the newest ImagesUpdatedAt across the album's and this disc's folders.
- // An image can be replaced without the album row changing, so this is what makes a cache
- // key notice it.
+ // Newest ImagesUpdatedAt across the album's and this disc's folders: an image can be
+ // replaced without the album row changing, so this is what makes a cache key notice it.
imagesUpdatedAt time.Time
}
// cacheTime is the disc image's validity stamp: any of these moving means the selection may
-// have changed. Mirrors what the legacy reader folded into its cache key.
+// have changed.
func (d *discArtworkReader) cacheTime() time.Time {
return utils.TimeNewest(d.album.UpdatedAt, d.album.ImportedAt, d.imagesUpdatedAt)
}
@@ -161,19 +160,12 @@ func (d *discArtworkReader) fromDiscSubtitle(ctx context.Context, subtitle strin
}
}
-// globMetaChars holds the substitution metacharacters understood by
-// filepath.Match. The '\' escape character is intentionally excluded:
-// disc art patterns come from user config and never include escaped
-// metachars in practice, and treating '\' as a metachar would misalign
-// the literal-prefix extraction in extractDiscNumber.
+// filepath.Match's '\' escape is excluded on purpose: treating it as a metachar
+// would misalign the literal-prefix extraction in extractDiscNumber.
const globMetaChars = "*?["
-// extractDiscNumber parses the disc number from a filename matched by a
-// filepath.Match-style glob pattern.
-//
-// Both pattern and filename must already be lowercased by the caller, which
-// is also expected to have verified that filepath.Match(pattern, filename)
-// is true before calling this function.
+// extractDiscNumber parses the disc number from a filename matched by a filepath.Match-style
+// glob. Caller must lowercase both args and have already verified the match.
func extractDiscNumber(pattern, filename string) (int, bool) {
metaIdx := strings.IndexAny(pattern, globMetaChars)
if metaIdx < 0 {
@@ -199,9 +191,8 @@ func extractDiscNumber(pattern, filename string) (int, bool) {
return num, true
}
-// fromExternalFile returns a sourceFunc that matches image files against a glob
-// pattern. A numbered filename whose number equals the target disc wins over
-// any unnumbered candidate; callers must pass a lowercase pattern.
+// fromExternalFile matches image files against a (lowercase) glob pattern. A numbered
+// filename whose number equals the target disc wins over any unnumbered candidate.
func (d *discArtworkReader) fromExternalFile(ctx context.Context, pattern string) sourceFunc {
isLiteral := !strings.ContainsAny(pattern, globMetaChars)
return func() (io.ReadCloser, string, error) {
diff --git a/core/artwork/e2e/acquire_serve_test.go b/core/artwork/e2e/acquire_serve_test.go
index 61f37e0bd..4e2540ead 100644
--- a/core/artwork/e2e/acquire_serve_test.go
+++ b/core/artwork/e2e/acquire_serve_test.go
@@ -20,9 +20,7 @@ import (
. "github.com/onsi/gomega"
)
-// These specs wire the real Worker and Service over the same ImageStore and mock repositories,
-// then drive the full enqueue → drain → serve loop. They assert the integration of the chain, not
-// the per-source resolution rules (which the unit suites in package artwork already cover).
+// Covers the enqueue → drain → serve chain; per-source resolution rules live in the unit suites.
var _ = Describe("Acquisition → serve loop", func() {
var (
ctx context.Context
@@ -42,7 +40,6 @@ var _ = Describe("Acquisition → serve loop", func() {
coverBytes []byte
)
- // itemFound reports whether the worker has persisted a resolved (hash-bearing) state row.
itemFound := func(kind model.Kind, id string) func() bool {
return func() bool {
ia, err := artRepo.GetItemArtwork(kind, id, model.ImageTypePrimary)
@@ -66,7 +63,7 @@ var _ = Describe("Acquisition → serve loop", func() {
conf.Server.CacheFolder = conf.NewDir(GinkgoT().TempDir())
conf.Server.DataFolder = conf.NewDir(GinkgoT().TempDir())
conf.Server.CoverArtPriority = "cover.jpg"
- conf.Server.ArtistArtPriority = "artist.png" // upload wins first; kept offline as a safety net
+ conf.Server.ArtistArtPriority = "artist.png" // keeps artist resolution offline
conf.Server.EnableMediaFileCoverArt = true
conf.Server.ArtworkWorkerConcurrency = 1
@@ -94,8 +91,7 @@ var _ = Describe("Acquisition → serve loop", func() {
}
ffm := tests.NewMockFFmpeg("")
store = artwork.NewImageStore(GinkgoT().TempDir())
- // size=0 requests stream originals and never touch the resize cache, so this reader is a
- // compile-time stand-in only; the resize path is covered by the package's serving_test.
+ // size=0 requests stream originals, so this reader is never called (serving_test covers resizing).
imgCache := cache.NewFileCache("ArtworkPipelineE2E", "100MB", "images", 0,
func(context.Context, cache.Item) (io.Reader, error) {
return nil, errors.New("resize not exercised in e2e")
@@ -106,8 +102,6 @@ var _ = Describe("Acquisition → serve loop", func() {
worker = artwork.NewWorker(ds, store, agents.GetAgents(ds, nil), ffm, events.NoopBroker(), imgCache)
})
- // seedFolderAlbum wires an album backed by the real fixture folder cover, shared by the album
- // and playlist-grid scenarios.
seedFolderAlbum := func(albumID string) {
folderRepo.result = []model.Folder{{Path: albumFolderPath, ImageFiles: []string{"cover.jpg"}}}
albumRepo.SetData(model.Albums{{ID: albumID, Name: "Album", FolderIDs: []string{"f1"}, LibraryID: 0}})
@@ -159,7 +153,6 @@ var _ = Describe("Acquisition → serve loop", func() {
img, err := svc.Get(ctx, model.MustParseArtworkID("pl-pl1"), 0, false)
Expect(err).ToNot(HaveOccurred())
Expect(img.Hash).To(Equal(ia.Hash))
- // The generated grid is a fresh PNG placed in the content-addressed store.
art, err := artRepo.GetImage(ia.Hash)
Expect(err).ToNot(HaveOccurred())
Expect(art.Mime).To(Equal("image/png"))
@@ -187,7 +180,6 @@ var _ = Describe("Acquisition → serve loop", func() {
ID: "mf1", AlbumID: "al1", HasCoverArt: true, LibraryID: 0, Path: mp3Fixture,
}})
- // First read: no state row yet → extract embedded art provisionally and enqueue the track.
provisional, err := svc.Get(ctx, model.MustParseArtworkID("mf-mf1"), 0, false)
Expect(err).ToNot(HaveOccurred())
Expect(provisional.Placeholder).To(BeFalse())
@@ -198,14 +190,13 @@ var _ = Describe("Acquisition → serve loop", func() {
_, err = artRepo.GetItemArtwork(model.KindMediaFileArtwork, "mf1", model.ImageTypePrimary)
Expect(err).To(MatchError(model.ErrNotFound), "provisional serving must not write a state row")
- // The provisional read enqueued a Bump; drain it and confirm the persisted hash matches.
+ // The provisional read enqueued a Bump; drain it.
runWorkerUntil(ctx, worker, itemFound(model.KindMediaFileArtwork, "mf1"))
ia, err := artRepo.GetItemArtwork(model.KindMediaFileArtwork, "mf1", model.ImageTypePrimary)
Expect(err).ToNot(HaveOccurred())
Expect(ia.Source).To(Equal("embedded"))
Expect(ia.Hash).To(Equal(provisional.Hash))
- // Second read: now served from the persisted state row / store, same bytes.
resolved, err := svc.Get(ctx, model.MustParseArtworkID("mf-mf1"), 0, false)
Expect(err).ToNot(HaveOccurred())
Expect(resolved.Hash).To(Equal(ia.Hash))
@@ -225,7 +216,6 @@ var _ = Describe("Acquisition → serve loop", func() {
Expect(art.Width).To(BeNumerically(">", 0))
Expect(art.Height).To(BeNumerically(">", 0))
Expect(art.SizeBytes).To(BeNumerically("==", len(coverBytes)))
- // Never a synthesized value: the blurhash is encoded from the real pixels.
Expect(art.BlurHash).ToNot(BeEmpty())
})
@@ -273,17 +263,16 @@ var _ = Describe("Acquisition → serve loop", func() {
Expect(err).ToNot(HaveOccurred())
staleHash := ia.Hash
- // Replace the backing file with different bytes and a newer mtime.
path := model.UploadedImagePath(consts.EntityRadio, name)
Expect(os.WriteFile(path, readFixture(artistPngFixture), 0o600)).To(Succeed())
newer := time.Now().Add(2 * time.Second)
Expect(os.Chtimes(path, newer, newer)).To(Succeed())
- // The mtime no longer matches the state row, so the old hash's bytes are never served.
+ // The mtime no longer matches the state row, so the stale bytes are not served.
_, err = svc.Get(ctx, model.MustParseArtworkID("ra-ra1"), 0, false)
Expect(err).To(MatchError(artwork.ErrUnavailable))
- // That read enqueued a re-resolution; draining it republishes the new bytes.
+ // That failed read enqueued a re-resolution.
runWorkerUntil(ctx, worker, func() bool {
cur, gerr := artRepo.GetItemArtwork(model.KindRadioArtwork, "ra1", model.ImageTypePrimary)
return gerr == nil && cur.Hash != "" && cur.Hash != staleHash
@@ -307,15 +296,14 @@ var _ = Describe("Acquisition → serve loop", func() {
})
})
-// mustGet unwraps a Service.Get result for inline byte assertions.
func mustGet(img *artwork.Image, err error) *artwork.Image {
GinkgoHelper()
Expect(err).ToNot(HaveOccurred())
return img
}
-// gifFixture is a 4x4 GIF held as raw bytes on purpose: encoding one would import image/gif into
-// this test binary and register the decoder, masking the production import the spec above guards.
+// Raw bytes on purpose: encoding a GIF here would register image/gif in the test binary, masking
+// the production import the spec above guards.
var gifFixture = []byte{
0x47, 0x49, 0x46, 0x38, 0x39, 0x61, 0x04, 0x00, 0x04, 0x00, 0x80, 0x00,
0x00, 0x2e, 0x86, 0xc1, 0xf4, 0xd0, 0x3f, 0x2c, 0x00, 0x00, 0x00, 0x00,
diff --git a/core/artwork/e2e/album_test.go b/core/artwork/e2e/album_test.go
index 92fabf9fc..c27f672e3 100644
--- a/core/artwork/e2e/album_test.go
+++ b/core/artwork/e2e/album_test.go
@@ -9,10 +9,8 @@ import (
. "github.com/onsi/gomega"
)
-// Folder-backed album art is served via os.Open(SourcePath), which the in-memory library FS
-// cannot satisfy; the worker's persisted state row (Source + SourcePath) is the resolver's
-// selection, so folder scenarios assert on it. Embedded art lands in the content-addressed store
-// and is asserted byte-for-byte; a no-art album settles absent.
+// The in-memory library FS cannot satisfy the os.Open(SourcePath) used to serve folder art, so
+// folder scenarios assert on the worker's state row (Source + SourcePath) instead of the bytes.
var _ = Describe("Album artwork resolution", func() {
BeforeEach(func() {
setupResolutionHarness()
@@ -354,8 +352,7 @@ var _ = Describe("Album artwork resolution", func() {
})
scan()
- // Album B first: a drain settles every ready item, and folder art is only
- // byte-servable while the album still has no state row.
+ // Album B first: the acquire in expectAbsent would settle Album B too.
expectFolderCover(albumByName("Album B"), "Artist/Album B/cover.jpg")
expectAbsent(albumByName("Album A"))
})
@@ -389,11 +386,10 @@ var _ = Describe("Album artwork resolution", func() {
})
})
- // albumRootParent refuses the library root as an album root (parent.ParentID == ""), so a
- // stray image at the top of the library never becomes some album's cover.
+ // albumRootParent refuses the library root as an album root (parent.ParentID == "").
When("a multi-disc album sits directly at the library root with a cover.jpg beside it", func() {
// (library root)
- // ├── cover.jpg ← must NOT be adopted: the root is never an album root
+ // ├── cover.jpg ← must NOT be adopted
// ├── CD1/
// │ └── 01 - Track.mp3
// └── CD2/
@@ -410,15 +406,14 @@ var _ = Describe("Album artwork resolution", func() {
})
})
- // compareImageFiles prefers shallower paths on a basename tie, so an artist-folder cover.jpg
- // would outrank the album's own if the parent folder were ever considered here. It is not:
- // albumRootParent skips the parent for a single-folder album that has images of its own.
+ // The shallower artist-folder cover.jpg would win the basename tie, but albumRootParent skips
+ // the parent folder for a single-folder album that has images of its own.
When("a single-folder album has its own cover.jpg and the artist folder has one too", func() {
// Artist/
// ├── cover.jpg ← shallower, but must NOT win
// └── Album/
// ├── 01 - Track.mp3
- // └── cover.jpg ← should win (the album has images of its own)
+ // └── cover.jpg ← should win
It("prefers the album's own cover over the shallower artist-folder cover", func() {
conf.Server.CoverArtPriority = defaultCoverPriority
setLayout(fstest.MapFS{
@@ -441,8 +436,7 @@ var _ = Describe("Album artwork resolution", func() {
// ├── Album A bonus/
// │ └── 02 - Track.mp3 (album: "Album A")
// └── Album B/
- // └── 01 - Track.mp3 other-album audio, so the artist folder is
- // correctly rejected as Album A's root
+ // └── 01 - Track.mp3 (other-album audio: rejects the artist folder as a root)
It("prefers the album's own art over the artist image", func() {
conf.Server.CoverArtPriority = defaultCoverPriority
setLayout(fstest.MapFS{
diff --git a/core/artwork/e2e/disc_test.go b/core/artwork/e2e/disc_test.go
index f869cadae..5fdd5f27b 100644
--- a/core/artwork/e2e/disc_test.go
+++ b/core/artwork/e2e/disc_test.go
@@ -11,8 +11,7 @@ import (
)
// Disc art is a serve-time read through the library FS (no worker state row), so per-disc images
-// are asserted byte-for-byte. Single-disc albums run the disc chain too — a disc can carry art
-// distinct from the album cover. Album-root covers are folder-backed and asserted on the state row.
+// are asserted byte-for-byte, while album-root covers are asserted on the state row.
var _ = Describe("Disc artwork resolution", func() {
BeforeEach(func() {
setupResolutionHarness()
diff --git a/core/artwork/e2e/e2e_suite_test.go b/core/artwork/e2e/e2e_suite_test.go
index bec7b12cf..8881da3a6 100644
--- a/core/artwork/e2e/e2e_suite_test.go
+++ b/core/artwork/e2e/e2e_suite_test.go
@@ -1,6 +1,5 @@
-// Package e2e exercises the artwork pipeline end to end: it enqueues real entities, drives the
-// real Worker to drain the queue, and serves the result through the real Service, over a real
-// ImageStore and real library files.
+// Package e2e exercises the artwork pipeline end to end: the real Worker drains the queue and the
+// real Service serves the result, over a real ImageStore and real library files.
package e2e
import (
@@ -36,7 +35,6 @@ const (
albumFolderPath = "tests/fixtures/artist/an-album"
)
-// readFixture returns the raw bytes of a project-relative fixture file.
func readFixture(rel string) []byte {
GinkgoHelper()
data, err := os.ReadFile(rel)
@@ -44,7 +42,6 @@ func readFixture(rel string) []byte {
return data
}
-// readAll drains an artwork image to bytes and closes it.
func readAll(img *artwork.Image) []byte {
GinkgoHelper()
Expect(img).ToNot(BeNil())
@@ -54,8 +51,6 @@ func readAll(img *artwork.Image) []byte {
return data
}
-// runWorkerUntil starts the real worker loop, waits for a condition, then cancels and joins it,
-// mirroring how cmd drives Worker.Run in production.
func runWorkerUntil(ctx context.Context, worker *artwork.Worker, until func() bool) {
GinkgoHelper()
runCtx, cancel := context.WithCancel(ctx)
@@ -66,8 +61,6 @@ func runWorkerUntil(ctx context.Context, worker *artwork.Worker, until func() bo
Eventually(done, 2*time.Second).Should(Receive(BeNil()))
}
-// fakeFolderRepo is the minimal FolderRepository the album/playlist resolution chains touch:
-// GetAll yields the seeded folders and the album-root parent lookup finds nothing.
type fakeFolderRepo struct {
model.FolderRepository
result []model.Folder
@@ -81,8 +74,6 @@ func (f *fakeFolderRepo) HasAudioOutsideFolders(model.Folder, []string) (bool, e
func (f *fakeFolderRepo) Get(string) (*model.Folder, error) { return nil, model.ErrNotFound }
-// writeUpload copies a fixture into the per-entity upload folder under the data dir and returns
-// the bare filename UploadedImagePath expects.
func writeUpload(entityType, name, srcFixture string) string {
GinkgoHelper()
dst := model.UploadedImagePath(entityType, name)
diff --git a/core/artwork/e2e/mediafile_test.go b/core/artwork/e2e/mediafile_test.go
index 6667b8dc2..d26756dcd 100644
--- a/core/artwork/e2e/mediafile_test.go
+++ b/core/artwork/e2e/mediafile_test.go
@@ -126,8 +126,7 @@ var _ = Describe("MediaFile artwork resolution", func() {
scan()
replaceWithRealMP3("Artist/Album/01 - Track.mp3")
- // The setting is not part of the artwork fingerprint, so a direct mf- request must
- // honor it at serve time rather than serving previously-eligible embedded art.
+ // The setting is not part of the artwork fingerprint, so it must be honored at serve time.
conf.Server.EnableMediaFileCoverArt = false
mf := mediafileOn("Artist/Album/01 - Track.mp3")
trackArtID := model.NewArtworkID(model.KindMediaFileArtwork, mf.ID, nil)
diff --git a/core/artwork/e2e/playlist_test.go b/core/artwork/e2e/playlist_test.go
index 2db8d4dda..2de601877 100644
--- a/core/artwork/e2e/playlist_test.go
+++ b/core/artwork/e2e/playlist_test.go
@@ -169,7 +169,6 @@ var _ = Describe("Playlist artwork resolution", func() {
// ├── AlbumB/{01 - Track.mp3, cover.png} ← tile 2
// ├── AlbumC/{01 - Track.mp3, cover.png} ← tile 3
// └── AlbumD/{01 - Track.mp3, cover.png} ← tile 4
- // Four distinct tiles fill the grid outright, with no mirroring.
It("fills all four grid quadrants with distinct album art", func() {
conf.Server.CoverArtPriority = "cover.*"
layout := fstest.MapFS{}
diff --git a/core/artwork/e2e/resolution_harness_test.go b/core/artwork/e2e/resolution_harness_test.go
index d79cf498c..ed27cd36b 100644
--- a/core/artwork/e2e/resolution_harness_test.go
+++ b/core/artwork/e2e/resolution_harness_test.go
@@ -38,12 +38,6 @@ import (
"go.senan.xyz/taglib"
)
-// This harness restores the pre-cutover artwork resolution edge-case coverage, but drives it
-// through the real pipeline: a real scanner populates the folder graph from an in-memory library,
-// the real Worker drains the queue to resolve/persist state, and the real Service serves it.
-// It documents the folder-selection rules (album/disc/artist priority, #5376/#5456/#5451/#5457)
-// that the lightweight acquire_serve_test.go intentionally leaves to this suite.
-
const fakeLibScheme = "artworkfake"
const fakeLibPath = fakeLibScheme + ":///music"
@@ -61,9 +55,8 @@ var (
fakeFS *storagetest.FakeFS
)
-// The DB file lives in a suite-level tempdir: the go-sqlite3 singleton keeps the file open for the
-// whole suite, and Ginkgo's per-spec TempDir cleanup can't unlink a file with a live handle on
-// Windows. A suite-level tempdir plus an AfterSuite close avoids the lock conflict.
+// The go-sqlite3 singleton holds the file open for the whole suite, and Windows cannot unlink a
+// file with a live handle, so the DB cannot live in Ginkgo's per-spec TempDir.
var suiteDBTempDir string
// Migrating the schema costs ~400ms, so it runs once per suite and specs reset by truncating.
@@ -117,8 +110,7 @@ func setupResolutionHarness() {
ffm := tests.NewMockFFmpeg("")
rstore = artwork.NewImageStore(filepath.Join(tempDir, "store"))
- // size=0 requests stream originals and never touch the resize cache, so this reader is a
- // compile-time stand-in only; the resize path is covered by the package's serving_test.
+ // size=0 requests stream originals, so this reader is never called (serving_test covers resizing).
imgCache := cache.NewFileCache("ArtworkResolutionE2E", "100MB", "images", 0,
func(context.Context, cache.Item) (io.Reader, error) {
return nil, fmt.Errorf("resize not exercised in e2e")
@@ -129,7 +121,7 @@ func setupResolutionHarness() {
rworker = artwork.NewWorker(rds, rstore, agents.GetAgents(rds, nil), ffm, events.NoopBroker(), imgCache)
}
-// setLayout populates the fake library. All paths must be forward-slash and relative.
+// setLayout paths must be relative and forward-slash.
func setLayout(files fstest.MapFS) {
GinkgoHelper()
fakeFS.SetFiles(files)
@@ -143,8 +135,6 @@ func scan() {
Expect(err).ToNot(HaveOccurred())
}
-// acquire drives the worker to resolve one entity and returns its persisted state row. It fails if
-// the worker never settles (found or absent) within the timeout.
func acquire(kind model.Kind, id string) model.ItemArtwork {
GinkgoHelper()
rworker.Bump(kind.Prefix(), id)
@@ -170,7 +160,6 @@ func runResolutionWorkerUntil(until func() bool) {
Eventually(done, 2*time.Second).Should(Receive(BeNil()))
}
-// serveBytes reads an artwork ID through the real Service at full size and returns its bytes.
func serveBytes(artID model.ArtworkID) []byte {
GinkgoHelper()
img, err := rsvc.Get(rctx, artID, 0, false)
@@ -189,7 +178,6 @@ func serveErr(artID model.ArtworkID) error {
return err
}
-// libFileBytes returns the contents of the one library file whose path ends with suffix.
func libFileBytes(suffix string) []byte {
GinkgoHelper()
var match string
@@ -203,10 +191,8 @@ func libFileBytes(suffix string) []byte {
return fakeFS.MapFS[match].Data
}
-// expectAlbumFolderCover asserts the album resolves to the library image at the given path suffix,
-// byte-for-byte. The serve happens before acquisition on purpose: with no state row the request
-// path resolves locally through the library FS, whereas a settled folder row is file-backed and
-// read with os.Open, which the in-memory FS cannot satisfy.
+// Serving before acquiring is deliberate: with no state row the request resolves through the
+// library FS, while a settled folder row is read with os.Open, which the in-memory FS cannot serve.
func expectAlbumFolderCover(al model.Album, suffix string) {
GinkgoHelper()
requireNoStateRow(model.KindAlbumArtwork, al.ID)
@@ -216,9 +202,7 @@ func expectAlbumFolderCover(al model.Album, suffix string) {
Expect(filepath.ToSlash(ia.SourcePath)).To(HaveSuffix(suffix))
}
-// requireNoStateRow guards the byte-level folder assertions: a drain resolves every ready queue
-// row, so acquiring one entity can settle others. Once settled, folder art is file-backed and the
-// in-memory FS cannot serve it — so these assertions must come before any acquire in a spec.
+// A drain settles every ready item, so byte-level folder assertions must precede any acquire.
func requireNoStateRow(kind model.Kind, id string) {
GinkgoHelper()
_, err := rds.Artwork(rctx).GetItemArtwork(kind, id, model.ImageTypePrimary)
@@ -226,7 +210,6 @@ func requireNoStateRow(kind model.Kind, id string) {
"assert %s %q before acquiring any other entity in this spec", kind, id)
}
-// expectAlbumAbsent asserts the album settled absent (no source resolved) and serves unavailable.
func expectAlbumAbsent(al model.Album) {
GinkgoHelper()
ia := acquire(model.KindAlbumArtwork, al.ID)
@@ -234,8 +217,6 @@ func expectAlbumAbsent(al model.Album) {
Expect(serveErr(al.CoverArtID())).To(MatchError(artwork.ErrUnavailable))
}
-// expectArtistFolder asserts the worker selected a library folder image (artist.*, album/artist.*)
-// as the artist image; like album folder art it is file-backed, so it is asserted on the state row.
func expectArtistFolder(ar model.Artist, suffix string) {
GinkgoHelper()
requireNoStateRow(model.KindArtistArtwork, ar.ID)
@@ -245,8 +226,6 @@ func expectArtistFolder(ar model.Artist, suffix string) {
Expect(filepath.ToSlash(ia.SourcePath)).To(HaveSuffix(suffix))
}
-// writeUploadedImage drops raw bytes into the per-entity upload folder under DataFolder, matching
-// the layout model.UploadedImagePath expects. Uploads are real files on disk, so they serve back.
func writeUploadedImage(entity, filename string, data []byte) {
GinkgoHelper()
dst := model.UploadedImagePath(entity, filename)
@@ -254,21 +233,17 @@ func writeUploadedImage(entity, filename string, data []byte) {
Expect(os.WriteFile(dst, data, 0o600)).To(Succeed())
}
-// discArtID is the artwork ID for one disc of an album.
func discArtID(al model.Album, disc int) model.ArtworkID {
return model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, disc), &al.UpdatedAt)
}
-// expectDiscImage asserts a multi-disc album serves the given disc's art byte-for-byte. Disc art
-// is a pure serve-time read through the library FS (no worker/state row), so this serves it live.
+// Disc art is a pure serve-time read through the library FS: no worker, no state row.
func expectDiscImage(al model.Album, disc int, label string) {
GinkgoHelper()
Expect(serveBytes(discArtID(al, disc))).To(Equal(pngBytes(label)))
}
-// gridQuadrants decodes a generated 2x2 playlist cover and samples the center of each quadrant,
-// in rect() order: top-left, top-right, bottom-left, bottom-right. Each tile is a solid color, so
-// the samples identify which album art landed where (and whether tiles were mirrored).
+// Samples in rect() order: top-left, top-right, bottom-left, bottom-right.
func gridQuadrants(data []byte) [4]color.RGBA {
GinkgoHelper()
img, _, err := image.Decode(bytes.NewReader(data))
@@ -282,9 +257,7 @@ func gridQuadrants(data []byte) [4]color.RGBA {
return [4]color.RGBA{at(qw, qh), at(3*qw, qh), at(qw, 3*qh), at(3*qw, 3*qh)}
}
-// storedBytes returns the bytes the worker placed in the content-addressed store for a
-// store-backed resolution (embedded/generated). Folder/upload sources are file-backed and are
-// not in the store; assert those on ia.SourcePath instead.
+// Store-backed sources only (embedded/generated); file-backed ones assert on ia.SourcePath.
func storedBytes(ia model.ItemArtwork) []byte {
GinkgoHelper()
art, err := rds.Artwork(rctx).GetImage(ia.Hash)
@@ -297,9 +270,7 @@ func storedBytes(ia model.ItemArtwork) []byte {
return data
}
-// smallPNG builds a tiny valid PNG whose pixel color is derived from label, so the bytes are
-// distinct per label (a resolver picking a different file yields a different hash/path) while
-// still decoding cleanly for the worker's blurhash step.
+// The pixel color derives from label, so each label yields distinct, still-decodable bytes.
func smallPNG(label string) *fstest.MapFile {
h := fnv.New32a()
_, _ = h.Write([]byte(label))
@@ -316,13 +287,11 @@ func smallPNG(label string) *fstest.MapFile {
return &fstest.MapFile{Data: buf.Bytes()}
}
-// pngBytes returns the bytes smallPNG(label) writes, for byte-for-byte serve assertions.
func pngBytes(label string) []byte {
GinkgoHelper()
return smallPNG(label).Data
}
-// trackFile builds a fake MP3 entry with optional tag overrides (album, disc, discsubtitle, ...).
func trackFile(num int, title string, extra ...map[string]any) *fstest.MapFile {
tags := storagetest.Track(num, title)
for _, e := range extra {
@@ -333,10 +302,8 @@ func trackFile(num int, title string, extra ...map[string]any) *fstest.MapFile {
return storagetest.MP3(tags)
}
-// embeddedArtFixture is a real MP3 with an embedded picture; FakeFS's JSON-encoded tags aren't
-// taglib-readable, so embedded-art scenarios swap these bytes in after scanning. embeddedArtBytes
-// is the exact image taglib extracts from it. Both load lazily (after tests.Init chdirs to the
-// project root and registers Gomega), via loadEmbeddedFixture from setupResolutionHarness.
+// FakeFS's JSON-encoded tags aren't taglib-readable, so embedded-art specs swap in these real MP3
+// bytes after scanning. Loaded lazily: tests.Init must chdir to the project root first.
var (
embeddedFixtureOnce sync.Once
embeddedArtFixture []byte
@@ -367,8 +334,6 @@ func extractEmbeddedArt(mp3 []byte) []byte {
return data
}
-// replaceWithRealMP3 swaps the fake entry at relPath for the real embedded-art MP3, so the
-// library FS returns a taglib-parseable stream during resolution.
func replaceWithRealMP3(relPath string) {
GinkgoHelper()
fakeFS.MapFS[relPath] = &fstest.MapFile{Data: embeddedArtFixture}
diff --git a/core/artwork/folders_artist.go b/core/artwork/folders_artist.go
index e411cdf9b..9c27ac9ac 100644
--- a/core/artwork/folders_artist.go
+++ b/core/artwork/folders_artist.go
@@ -21,8 +21,6 @@ import (
)
const (
- // maxArtistFolderTraversalDepth defines how many directory levels to search
- // when looking for artist images (artist folder + parent directories)
maxArtistFolderTraversalDepth = 3
)
@@ -35,8 +33,7 @@ func fromArtistFolder(ctx context.Context, libFS fs.FS, libPath, artistFolder, p
if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
return nil, "", fmt.Errorf(`artist folder '%s' is outside library '%s'`, artistFolder, libPath)
}
- // fs.Glob / path.Join below expect forward-slash paths; filepath.Rel may
- // return backslash separators on Windows.
+ // fs.Glob needs forward slashes; filepath.Rel returns backslashes on Windows.
rel = filepath.ToSlash(rel)
current := artistFolder
var unreadable error
@@ -49,7 +46,7 @@ func fromArtistFolder(ctx context.Context, libFS fs.FS, libPath, artistFolder, p
unreadable = err
}
if rel == "." {
- break // reached library root; don't traverse above it
+ break // reached library root
}
rel = path.Dir(rel)
current = filepath.Dir(current)
@@ -61,10 +58,8 @@ func fromArtistFolder(ctx context.Context, libFS fs.FS, libPath, artistFolder, p
}
}
-// findImageInFolder globs libFS at relFolder for pattern and returns the first
-// matching image. absFolder is used only for the returned display path and log
-// messages so callers see absolute-looking paths consistent with the rest of
-// the artwork pipeline.
+// findImageInFolder returns the first image matching pattern; absFolder is only used for
+// the returned display path and log messages.
func findImageInFolder(ctx context.Context, libFS fs.FS, relFolder, absFolder, pattern string) (io.ReadCloser, string, error) {
log.Trace(ctx, "Artwork: Looking for artist image", "pattern", pattern, "folder", absFolder)
globPattern := pattern
@@ -77,7 +72,6 @@ func findImageInFolder(ctx context.Context, libFS fs.FS, relFolder, absFolder, p
return nil, "", err
}
- // Filter to valid image files
var imagePaths []string
for _, m := range matches {
if !model.IsImageFile(m) {
@@ -86,8 +80,7 @@ func findImageInFolder(ctx context.Context, libFS fs.FS, relFolder, absFolder, p
imagePaths = append(imagePaths, m)
}
- // Sort image files by prioritizing base filenames without numeric
- // suffixes (e.g., artist.jpg before artist.1.jpg)
+ // Prefer base filenames over numeric-suffixed ones (artist.jpg before artist.1.jpg)
slices.SortFunc(imagePaths, compareImageFiles)
var openErr error
@@ -125,7 +118,7 @@ func loadArtistFolder(ctx context.Context, ds model.DataStore, albums model.Albu
if len(albums) == 0 {
return "", time.Time{}, nil
}
- libID := albums[0].LibraryID // Just need one of the albums, as they should all be in the same Library - for now! TODO: Support multiple libraries
+ libID := albums[0].LibraryID // TODO: Support albums spanning multiple libraries
folderPath := str.LongestCommonPrefix(paths)
if !strings.HasSuffix(folderPath, string(filepath.Separator)) {
@@ -133,15 +126,13 @@ func loadArtistFolder(ctx context.Context, ds model.DataStore, albums model.Albu
}
folderPath = filepath.Dir(folderPath)
- // Manipulate the path to get the folder ID
- // TODO: This is a bit hacky, but it's the easiest way to get the folder ID, ATM
+ // TODO: Hacky, but the easiest way to get the folder ID ATM
libPath := core.AbsolutePath(ctx, ds, libID, "")
folderID := model.FolderID(model.Library{ID: libID, Path: libPath}, folderPath)
log.Trace(ctx, "Artwork: Calculating artist folder details", "folderPath", folderPath, "folderID", folderID,
"libPath", libPath, "libID", libID, "albumPaths", paths)
- // Get the last update time for the folder
folders, err := ds.Folder(ctx).GetAll(model.QueryOptions{Filters: squirrel.Eq{"folder.id": folderID, "missing": false}})
if err != nil || len(folders) == 0 {
log.Warn(ctx, "Artwork: Could not find folder for artist", "folderPath", folderPath, "id", folderID,
@@ -151,8 +142,7 @@ func loadArtistFolder(ctx context.Context, ds model.DataStore, albums model.Albu
return folderPath, folders[0].ImagesUpdatedAt, nil
}
-// findImageInArtistFolder scans a folder for an image file matching the artist's MBID or name
-// (case-insensitive). Returns the full path, or empty string if not found.
+// findImageInArtistFolder matches an image by MBID or artist name (case-insensitive), "" if none.
func findImageInArtistFolder(folder, mbzArtistID, artistName string) string {
entries, err := os.ReadDir(folder)
if err != nil {
diff --git a/core/artwork/folders_artist_test.go b/core/artwork/folders_artist_test.go
index 27b52c9e4..a35e40232 100644
--- a/core/artwork/folders_artist_test.go
+++ b/core/artwork/folders_artist_test.go
@@ -10,9 +10,8 @@ import (
. "github.com/onsi/gomega"
)
-// unreadableFS globs like its embedded MapFS but refuses to open anything, standing in for a
-// stale mount or a permissions failure. Injecting the error keeps this independent of the
-// filesystem: os.Chmod does not restrict read access on Windows.
+// unreadableFS globs like its embedded MapFS but refuses to open anything. Injecting the error
+// keeps this independent of the filesystem: os.Chmod does not restrict read access on Windows.
type unreadableFS struct{ fstest.MapFS }
func (u unreadableFS) Open(string) (fs.File, error) { return nil, fs.ErrPermission }
diff --git a/core/artwork/gate.go b/core/artwork/gate.go
index 68c281284..096b1120d 100644
--- a/core/artwork/gate.go
+++ b/core/artwork/gate.go
@@ -21,36 +21,33 @@ const (
var errBreakerOpen = errors.New("artwork: external circuit breaker open")
// gateFunc gates one named external fetch (rate limit + circuit breaker per name).
-// resolveItem defaults to passthroughGate; the worker injects the per-agent gate.
type gateFunc = func(name string, f func() (io.ReadCloser, string, error)) (io.ReadCloser, string, error)
func passthroughGate(_ string, f func() (io.ReadCloser, string, error)) (io.ReadCloser, string, error) {
return f()
}
-// isTransientExternal reports whether an external step failed in a way worth retrying;
-// a not-found (from either package) is a definitive answer, not a fault.
+// isTransientExternal reports whether an external failure is worth retrying; a not-found
+// (from either package) is a definitive answer, not a fault.
func isTransientExternal(err error) bool {
return err != nil && !errors.Is(err, agents.ErrNotFound) && !errors.Is(err, model.ErrNotFound)
}
-// extGate is one agent's rate limiter + circuit breaker; each external agent gets its
-// own so a provider whose API or CDN is down backs off in isolation from the others.
+// extGate is one agent's rate limiter and circuit breaker, so a failing provider backs off
+// in isolation from the others.
type extGate struct {
limiter *rate.Limiter
breaker *breaker
}
-// gate wraps a named external step with that agent's own rate limiter and circuit
-// breaker, matching gateFunc so it can be handed to the processor's resolver.
+// gate runs a named external step through that agent's rate limiter and circuit breaker.
func (w *Worker) gate(name string, f func() (io.ReadCloser, string, error)) (io.ReadCloser, string, error) {
g := w.gateFor(name)
if !g.breaker.allow() {
log.Debug(w.runCtx, "Artwork: Skipping agent, circuit breaker open", "agent", name)
return nil, "", errBreakerOpen
}
- // Waiting for the rate-limit permit is counted separately: a slow agent and a throttled one
- // look identical from the drain, but only one of them is the provider's fault.
+ // Timed separately so a throttled agent isn't mistaken for a slow provider.
waitStart := time.Now()
if err := g.limiter.Wait(w.runCtx); err != nil {
return nil, "", err
@@ -63,8 +60,7 @@ func (w *Worker) gate(name string, f func() (io.ReadCloser, string, error)) (io.
return r, path, err
}
-// gateFor lazily creates the per-name gate on first use, each with its own limiter at
-// ArtworkExternalMaxRPS and its own breaker.
+// gateFor lazily creates the per-name gate on first use.
func (w *Worker) gateFor(name string) *extGate {
w.gatesMu.Lock()
defer w.gatesMu.Unlock()
@@ -81,8 +77,8 @@ func (w *Worker) gateFor(name string) *extGate {
return g
}
-// breaker opens after breakerThreshold consecutive external errors and admits a
-// single probe once breakerProbeAfter has elapsed; a success re-closes it.
+// breaker opens after breakerThreshold consecutive errors and admits a single probe once
+// breakerProbeAfter has elapsed; a success re-closes it.
type breaker struct {
mu sync.Mutex
failures int
@@ -107,8 +103,7 @@ func (b *breaker) allow() bool {
func (b *breaker) record(name string, err error) {
b.mu.Lock()
defer b.mu.Unlock()
- // A not-found (from either package) is a definitive answer, not a fault; only real
- // errors trip the breaker. Must stay consistent with isTransientExternal.
+ // A not-found is a definitive answer, not a fault; keep in sync with isTransientExternal.
if err == nil || errors.Is(err, model.ErrNotFound) || errors.Is(err, agents.ErrNotFound) {
if b.failures >= breakerThreshold {
log.Info("Artwork: Circuit breaker closed for agent", "agent", name)
diff --git a/core/artwork/housekeeping.go b/core/artwork/housekeeping.go
index f5da3d0cc..398256d03 100644
--- a/core/artwork/housekeeping.go
+++ b/core/artwork/housekeeping.go
@@ -15,28 +15,23 @@ import (
"github.com/navidrome/navidrome/model"
)
-// staleAbsentAge is how old an absent resolution must be before the recheck job retries it.
const staleAbsentAge = 24 * time.Hour
-// recheckKinds are the item kinds eligible for the periodic recheck jobs (stale-absent and
-// missing-row). Media files are excluded: they resolve embedded-only, at scan or on view.
+// recheckKinds omits media files: they resolve embedded-only, at scan or on view.
var recheckKinds = []model.Kind{
model.KindArtistArtwork, model.KindAlbumArtwork, model.KindPlaylistArtwork, model.KindRadioArtwork,
}
-// hasRecheckPath reports whether a periodic job will revisit this kind, which is what makes
-// settling absent on an exhausted retry budget recoverable rather than permanent.
+// hasRecheckPath reports whether a periodic job will revisit this kind, making an absent settle recoverable.
func hasRecheckPath(prefix string) bool {
kind, ok := model.ParseKind(prefix)
return ok && slices.Contains(recheckKinds, kind)
}
-// artworkEpoch invalidates all resolution state when bumped; bump it in the same change that
-// alters resolution semantics. Deliberately not the server version, which changes every build.
+// artworkEpoch invalidates all resolution state when bumped; bump it whenever resolution semantics change.
const artworkEpoch = 1
-// fingerprint summarizes the inputs that affect artwork resolution outcomes; a
-// change means previously resolved (or absent) state may no longer be correct.
+// fingerprint covers the inputs that affect resolution outcomes; a change invalidates stored state.
func fingerprint() string {
raw := fmt.Sprintf("%s|%s|%s|%s|%t|%t|%d",
conf.Server.CoverArtPriority, conf.Server.ArtistArtPriority, conf.Server.ArtistImageFolder,
@@ -45,8 +40,7 @@ func fingerprint() string {
return hex.EncodeToString(sum[:])
}
-// backfill enqueues artwork resolution for every entity when the config fingerprint changed
-// (or was never stored), artists first so those pages resolve before the larger backlog.
+// backfill enqueues artwork resolution for every entity when the config fingerprint changed.
func backfill(ctx context.Context, ds model.DataStore) (bool, error) {
start := time.Now()
ctx = auth.WithAdminUser(ctx, ds)
@@ -60,7 +54,7 @@ func backfill(ctx context.Context, ds model.DataStore) (bool, error) {
return false, nil
}
- // Artists first: few entities, most external-dependent, so they get queue headstart.
+ // Artists first: few entities, most external-dependent, so they get a queue headstart.
kinds := []struct {
kind model.Kind
fetch func() ([]string, error)
@@ -100,8 +94,6 @@ func enqueueBackfillKind(ctx context.Context, ds model.DataStore, kind model.Kin
return ds.ArtworkQueue(ctx).Enqueue(items...)
}
-// enqueueStaleAbsentAll requeues absent-state entries older than staleAbsentAge, across
-// every artwork-bearing kind, for the periodic recheck job.
func enqueueStaleAbsentAll(ctx context.Context, ds model.DataStore) error {
cutoff := time.Now().Add(-staleAbsentAge)
queue := ds.ArtworkQueue(ctx)
@@ -113,8 +105,7 @@ func enqueueStaleAbsentAll(ctx context.Context, ds model.DataStore) error {
return nil
}
-// enqueueMissingAll requeues entities that have no item_artwork row yet, across every recheck
-// kind: the safety net for entities a scan never enqueued (added between scans, or scanner off).
+// enqueueMissingAll is the safety net for entities a scan never enqueued (added between scans, or scanner off).
func enqueueMissingAll(ctx context.Context, ds model.DataStore) error {
queue := ds.ArtworkQueue(ctx)
for _, kind := range recheckKinds {
@@ -125,8 +116,7 @@ func enqueueMissingAll(ctx context.Context, ds model.DataStore) error {
return nil
}
-// Refresh clears an item's resolved artwork state and re-queues it at Bump priority, so a
-// deliberate refresh (image upload or manual re-resolve) drops the current pick and re-resolves it.
+// Refresh drops an item's resolved artwork state and re-queues it at Bump priority.
func Refresh(ctx context.Context, ds model.DataStore, kind model.Kind, id string) error {
if err := ds.Artwork(ctx).DeleteForItem(kind, id); err != nil {
return fmt.Errorf("clearing artwork state: %w", err)
diff --git a/core/artwork/housekeeping_test.go b/core/artwork/housekeeping_test.go
index f556579b8..cadb6dbfd 100644
--- a/core/artwork/housekeeping_test.go
+++ b/core/artwork/housekeeping_test.go
@@ -213,9 +213,7 @@ var _ = Describe("Housekeeping", func() {
artRepo.ItemData["al-stale"] = model.ItemArtwork{ItemKind: "al", ItemID: "al1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old}
artRepo.ItemData["pl-stale"] = model.ItemArtwork{ItemKind: "pl", ItemID: "pl1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old}
artRepo.ItemData["ra-stale"] = model.ItemArtwork{ItemKind: "ra", ItemID: "ra1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old}
- // Not stale: too recent.
artRepo.ItemData["ar-recent"] = model.ItemArtwork{ItemKind: "ar", ItemID: "ar2", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: recent}
- // Not absent: has a resolved hash.
artRepo.ItemData["al-resolved"] = model.ItemArtwork{ItemKind: "al", ItemID: "al2", ImageType: model.ImageTypePrimary, Hash: "somehash", AttemptedAt: old}
err := enqueueStaleAbsentAll(ctx, ds)
@@ -250,7 +248,6 @@ var _ = Describe("Housekeeping", func() {
})
It("enqueues only entities that have no item_artwork row, across all kinds", func() {
- // al1 is already resolved and ar1 already absent: both must be skipped.
artRepo.ItemData["al-resolved"] = model.ItemArtwork{ItemKind: "al", ItemID: "al1", ImageType: model.ImageTypePrimary, Hash: "somehash", AttemptedAt: time.Now()}
artRepo.ItemData["ar-absent"] = model.ItemArtwork{ItemKind: "ar", ItemID: "ar1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: time.Now()}
diff --git a/core/artwork/image_store.go b/core/artwork/image_store.go
index bfbc5a9bc..bac20360f 100644
--- a/core/artwork/image_store.go
+++ b/core/artwork/image_store.go
@@ -16,8 +16,7 @@ import (
"github.com/zeebo/xxh3"
)
-// ImageStore is the content-addressed store for artwork images that have no
-// library file backing them (external downloads, embedded extractions, generated).
+// ImageStore is the content-addressed store for artwork images with no library file backing them.
type ImageStore struct {
root string
}
@@ -26,14 +25,12 @@ func NewImageStore(rootDir string) *ImageStore {
return &ImageStore{root: rootDir}
}
-// GetImageStore roots the store in its own subtree under the data folder, so
-// Prune's recursive sweep never reaches the per-entity upload folders next to it.
+// GetImageStore roots the store in its own subtree so Prune's sweep never reaches the upload folders beside it.
func GetImageStore() *ImageStore {
return NewImageStore(filepath.Join(conf.Server.DataFolder.String(), consts.ArtworkFolder, "store"))
}
-// extForMime is deliberately NOT mime.ExtensionsByType: extensions are baked into
-// content-addressed paths and re-derived on Open, so they must be stable across OSes.
+// extForMime must stay stable across OSes: extensions are baked into stored paths and re-derived on Open.
func extForMime(m string) string {
switch m {
case "image/jpeg":
@@ -56,8 +53,7 @@ func hashImage(r io.Reader) (string, error) {
return fmt.Sprintf("%016x", d.Sum64()), nil
}
-// validHash rejects anything but 16 lowercase hex chars: known-absent states carry "",
-// and malformed persisted hashes must never reach path sharding (slice panics, separators).
+// validHash guards path sharding: a malformed hash would slice-panic or inject path separators.
func validHash(hash string) bool {
if len(hash) != 16 {
return false
@@ -85,7 +81,7 @@ func (s *ImageStore) Write(hash, mimeType string, r io.Reader) error {
if err := os.Chtimes(dst, now, now); err == nil {
return nil
}
- // touch failed (file likely pruned concurrently) — fall through and write it
+ // touch failed (likely pruned concurrently) — fall through and rewrite it
}
if err := os.MkdirAll(filepath.Dir(dst), 0755); err != nil {
return err
@@ -112,8 +108,7 @@ func (s *ImageStore) Open(hash, mimeType string) (io.ReadCloser, error) {
return os.Open(s.path(hash, mimeType))
}
-// Remove deletes the store file unless it is newer than olderThan, in which case
-// an overlapping acquisition may have just touched it and be about to commit its row.
+// Remove skips files newer than olderThan: an overlapping acquisition may not have committed its row yet.
func (s *ImageStore) Remove(hash, mimeType string, olderThan time.Time) error {
if !validHash(hash) {
return fmt.Errorf("imagestore: invalid hash %q", hash)
@@ -136,9 +131,8 @@ func (s *ImageStore) Remove(hash, mimeType string, olderThan time.Time) error {
return err
}
-// Sweep removes store files not accepted by keep. Files modified after cutoff
-// (including temp files) are always kept: their acquisition row may not be committed yet.
-// The walk is cancellable: it runs under the worker's prune lock, which shutdown waits on.
+// Sweep removes store files not accepted by keep. Files modified after cutoff are always
+// kept: their acquisition row may not be committed yet.
func (s *ImageStore) Sweep(ctx context.Context, cutoff time.Time, keep func(hash, ext string) bool) (int, error) {
removed := 0
err := filepath.WalkDir(s.root, func(path string, d fs.DirEntry, err error) error {
diff --git a/core/artwork/playlist_cover.go b/core/artwork/playlist_cover.go
index 43047a6c6..68f4c4e10 100644
--- a/core/artwork/playlist_cover.go
+++ b/core/artwork/playlist_cover.go
@@ -18,9 +18,7 @@ import (
const tileSize = 600
-// findPlaylistSidecarPath scans the directory of the playlist file for a sidecar
-// image file with the same base name (case-insensitive). Returns empty string if
-// no matching image is found or if plsPath is empty.
+// findPlaylistSidecarPath finds an image beside plsPath with the same base name (case-insensitive).
func findPlaylistSidecarPath(ctx context.Context, plsPath string) string {
if plsPath == "" {
return ""
@@ -59,25 +57,21 @@ func rect(pos int) image.Rectangle {
return r
}
-// fillCenter crops the source image from the center and scales it to fill dstW x dstH exactly,
-// equivalent to imaging.Fill with Center anchor.
+// fillCenter center-crops src and scales it to fill dstW x dstH exactly.
func fillCenter(src image.Image, dstW, dstH int) image.Image {
srcBounds := src.Bounds()
srcW := srcBounds.Dx()
srcH := srcBounds.Dy()
- // Calculate crop rectangle (center crop to match destination aspect ratio)
srcAspect := float64(srcW) / float64(srcH)
dstAspect := float64(dstW) / float64(dstH)
var cropRect image.Rectangle
if srcAspect > dstAspect {
- // Source is wider — crop horizontally
cropW := int(float64(srcH) * dstAspect)
cropX := (srcW - cropW) / 2
cropRect = image.Rect(srcBounds.Min.X+cropX, srcBounds.Min.Y, srcBounds.Min.X+cropX+cropW, srcBounds.Max.Y)
} else {
- // Source is taller — crop vertically
cropH := int(float64(srcW) / dstAspect)
cropY := (srcH - cropH) / 2
cropRect = image.Rect(srcBounds.Min.X, srcBounds.Min.Y+cropY, srcBounds.Max.X, srcBounds.Min.Y+cropY+cropH)
@@ -88,10 +82,7 @@ func fillCenter(src image.Image, dstW, dstH int) image.Image {
return dst
}
-// decodeTile and assembleTiles mirror playlistArtworkReader's createTile/
-// createTiledImage, reusing the same rect/fillCenter cropping helpers.
-// decodeTile runs on every sampled album's resolved bytes before the processor's
-// own maxImageBytes/maxImagePixels guards apply, so it enforces them itself too.
+// decodeTile runs before the processor's size guards apply, so it enforces the caps itself.
func decodeTile(r io.ReadCloser) (image.Image, error) {
data, err := readCapped(r)
if err != nil {
diff --git a/core/artwork/processor.go b/core/artwork/processor.go
index af589991a..d6d078fa4 100644
--- a/core/artwork/processor.go
+++ b/core/artwork/processor.go
@@ -18,14 +18,13 @@ import (
xdraw "golang.org/x/image/draw"
)
-// outcome tells the worker what to do with the queue row: found/absent
-// delete it, failed reschedules it via MarkFailed.
+// outcome tells the worker whether to delete the queue row (found/absent) or reschedule it.
type outcome int
const (
outcomeFound outcome = iota
// outcomeFoundStale: state was written and is served, but a higher-priority external
- // step failed, so the row must retry (via MarkFailed) to give that source another chance.
+ // source failed, so the row must retry to give it another chance.
outcomeFoundStale
outcomeAbsent
outcomeFailed
@@ -47,12 +46,12 @@ func (o outcome) String() string {
// thumbnailSize is the max dimension fed to blurhash.
const thumbnailSize = 128
-// maxImageBytes caps a resolved image read: a user-editable ExternalImageURL could
-// point at an arbitrarily large endpoint, and 20MB is generous for any real cover.
+// maxImageBytes caps a resolved image read: a user-editable ExternalImageURL could point at
+// an arbitrarily large endpoint.
const maxImageBytes = 20 << 20
-// maxImagePixels caps declared dimensions: a tiny compressed file can declare a
-// huge canvas that image.Decode would expand into gigabytes (decompression bomb).
+// maxImagePixels guards against decompression bombs: a tiny file can declare a canvas that
+// image.Decode would expand into gigabytes.
const maxImagePixels = 64 << 20
// acquired is what a successful acquire persisted, handed back so the caller can warm the resize
@@ -63,8 +62,7 @@ type acquired struct {
data []byte
}
-// processor turns one queue item into stored artwork. It owns acquisition only — settling the
-// queue row afterwards is the Worker's job. pruneLock is nil in tests, where nothing prunes.
+// processor turns one queue item into stored artwork; settling the queue row is the Worker's job.
type processor struct {
ds model.DataStore
store *ImageStore
@@ -76,8 +74,6 @@ type processor struct {
// blurhash it, place its bytes, and persist the resulting state.
func (p *processor) acquire(ctx context.Context, item model.ArtworkQueueItem) (out outcome, got *acquired) {
repo := p.ds.Artwork(ctx)
- // Timed on every exit, failures included: an item that is slow is usually one that failed
- // slowly, and the outcome alone doesn't say whether the cost was the network or the decode.
start := time.Now()
defer func() {
log.Debug(ctx, "Artwork: Acquisition finished", "kind", item.ItemKind, "id", item.ItemID,
@@ -91,8 +87,7 @@ func (p *processor) acquire(ctx context.Context, item model.ArtworkQueueItem) (o
}
if res.reader == nil {
if res.extError || res.localError {
- // A source errored/timed out rather than answering "no image": never settle on
- // absent, keep serving old state.
+ // A fault is not a definitive "no image": never settle absent, keep serving old state.
log.Debug(ctx, "Artwork: No image, but a source faulted; keeping previous state",
"kind", item.ItemKind, "id", item.ItemID, "extError", res.extError, "localError", res.localError)
return outcomeFailed, nil
@@ -101,7 +96,6 @@ func (p *processor) acquire(ctx context.Context, item model.ArtworkQueueItem) (o
}
defer res.reader.Close()
- // Times the download too: res.reader is the provider's response body for external sources.
readStart := time.Now()
data, err := readCapped(res.reader)
if err != nil {
@@ -123,7 +117,6 @@ func (p *processor) acquire(ctx context.Context, item model.ArtworkQueueItem) (o
art, err := repo.GetImage(hash)
switch {
case err == nil:
- // Dedup hit: identical bytes already known, reuse dims/mime/blurhash.
log.Debug(ctx, "Artwork: Reusing a known image, skipping decode", "kind", item.ItemKind,
"id", item.ItemID, "hash", hash)
case errors.Is(err, model.ErrNotFound):
@@ -148,7 +141,6 @@ func (p *processor) acquire(ctx context.Context, item model.ArtworkQueueItem) (o
}
got = &acquired{ia: ia, mime: art.Mime, data: data}
if res.extError {
- // Served, but a higher-priority external source errored: the pick may improve on retry.
log.Debug(ctx, "Artwork: Serving a lower-priority source after an external failure",
"kind", item.ItemKind, "id", item.ItemID, "source", res.source)
return outcomeFoundStale, got
@@ -156,9 +148,8 @@ func (p *processor) acquire(ctx context.Context, item model.ArtworkQueueItem) (o
return outcomeFound, got
}
-// persist places the bytes and commits the rows referencing them. Only this window excludes
-// Prune, which reclaims store files no row points at; resolution stays outside so a slow fetch
-// cannot hold prune off.
+// persist places the bytes and commits the rows referencing them, excluding Prune for that
+// window only so a slow resolution can never hold it off.
func (p *processor) persist(repo model.ArtworkRepository, item model.ArtworkQueueItem,
art *model.Artwork, res resolution, data []byte,
) (*model.ItemArtwork, error) {
@@ -183,14 +174,14 @@ func (p *processor) persist(repo model.ArtworkRepository, item model.ArtworkQueu
RefMtime: refMtime,
AttemptedAt: time.Now(),
}
- // PutItemArtwork stamps UpdatedAt on ia, so what it holds now matches the persisted row.
+ // PutItemArtwork stamps UpdatedAt on ia, so the returned struct matches the persisted row.
if err := repo.PutItemArtwork(ia); err != nil {
return nil, fmt.Errorf("persisting item artwork state: %w", err)
}
return ia, nil
}
-// writeAbsent records a known-absent state: every local/external source answered definitively "no".
+// writeAbsent records a known-absent state: every source answered definitively "no".
func writeAbsent(ctx context.Context, repo model.ArtworkRepository, item model.ArtworkQueueItem) outcome {
err := repo.PutItemArtwork(&model.ItemArtwork{
ItemKind: item.ItemKind,
@@ -207,7 +198,6 @@ func writeAbsent(ctx context.Context, repo model.ArtworkRepository, item model.A
return outcomeAbsent
}
-// readCapped reads r, rejecting anything over maxImageBytes.
func readCapped(r io.Reader) ([]byte, error) {
data, err := io.ReadAll(io.LimitReader(r, maxImageBytes+1))
if err != nil {
@@ -219,15 +209,13 @@ func readCapped(r io.Reader) ([]byte, error) {
return data, nil
}
-// decodeCapped rejects declared dimensions over maxImagePixels BEFORE the
-// full-decode allocation, then decodes.
+// decodeCapped rejects declared dimensions over maxImagePixels before the full-decode allocation.
func decodeCapped(data []byte) (image.Image, string, error) {
cfg, format, err := image.DecodeConfig(bytes.NewReader(data))
if err != nil {
return nil, "", fmt.Errorf("decode image config: %w", err)
}
- // Compared by division so the cap holds for any dimensions a decoder might report, without
- // depending on a multiplication staying inside int64.
+ // Compared by division so the cap cannot be defeated by an int64 overflow.
if cfg.Width <= 0 || cfg.Height <= 0 || cfg.Width > maxImagePixels/cfg.Height {
return nil, "", fmt.Errorf("image dimensions %dx%d exceed pixel cap %d", cfg.Width, cfg.Height, maxImagePixels)
}
@@ -238,8 +226,7 @@ func decodeCapped(data []byte) (image.Image, string, error) {
return img, format, nil
}
-// decodeArtwork builds a new Artwork row from raw bytes: dimensions, mime and a
-// blurhash computed from a downscaled thumbnail.
+// decodeArtwork builds a new Artwork row from raw bytes: dimensions, mime and blurhash.
func decodeArtwork(ctx context.Context, hash string, data []byte) (*model.Artwork, error) {
img, format, err := decodeCapped(data)
if err != nil {
@@ -263,8 +250,7 @@ func decodeArtwork(ctx context.Context, hash string, data []byte) (*model.Artwor
}, nil
}
-// makeThumbnail downscales img to fit within maxSize on its longest side.
-// Images within bounds are returned as-is (no upscaling).
+// makeThumbnail downscales img to fit within maxSize on its longest side; it never upscales.
func makeThumbnail(img image.Image, maxSize int) image.Image {
b := img.Bounds()
w, h := b.Dx(), b.Dy()
@@ -277,14 +263,14 @@ func makeThumbnail(img image.Image, maxSize int) image.Image {
return dst
}
-// isFileBacked reports whether a resolution's bytes already live in a library/upload
-// file, so the acquisition must not duplicate them into the content-addressed store.
+// isFileBacked reports whether the bytes already live in a library/upload file, so the
+// content-addressed store must not duplicate them.
func isFileBacked(source string) bool {
return source == "folder" || source == "upload"
}
-// placeBytes reports the item's backing-file provenance (folder/upload: image, embedded: audio,
-// external/generated: none) and writes the bytes into the store for the non-file-backed sources.
+// placeBytes reports the item's backing-file provenance and writes the bytes into the store
+// for the sources that have none.
func placeBytes(store *ImageStore, art *model.Artwork, res resolution, data []byte) (sourcePath string, refMtime int64, err error) {
if isFileBacked(res.source) {
return res.sourcePath, res.refMtime, nil
@@ -295,8 +281,7 @@ func placeBytes(store *ImageStore, art *model.Artwork, res resolution, data []by
return sourcePath, refMtime, store.Write(art.Hash, art.Mime, bytes.NewReader(data))
}
-// mimeForFormat maps an image.Decode format name to its MIME type; extForMime
-// in image_store.go performs the inverse for content-addressed file paths.
+// mimeForFormat maps an image.Decode format name to its MIME type; extForMime is the inverse.
func mimeForFormat(format string) string {
switch format {
case "jpeg":
diff --git a/core/artwork/processor_test.go b/core/artwork/processor_test.go
index fcdd9de8a..14f516d58 100644
--- a/core/artwork/processor_test.go
+++ b/core/artwork/processor_test.go
@@ -22,8 +22,7 @@ import (
. "github.com/onsi/gomega"
)
-// pngHeaderWithDims builds just a PNG signature + IHDR chunk declaring w×h. DecodeConfig
-// reads the header without touching pixel data, so the body can be omitted entirely.
+// DecodeConfig reads only the header, so the pixel data can be omitted entirely.
func pngHeaderWithDims(w, h uint32) []byte {
ihdr := make([]byte, 13)
binary.BigEndian.PutUint32(ihdr[0:], w)
@@ -124,8 +123,8 @@ var _ = Describe("processor.acquire", func() {
Expect(lock.held()).To(BeFalse(), "the window must close before acquire returns")
})
- // Resolution can reach the network under its own timeout, so holding the lock across it
- // would let one slow provider block prune, and every drain queued behind prune's writer.
+ // Resolution can reach the network, so holding the lock across it would let one slow
+ // provider block prune, and every drain queued behind prune's writer.
It("never takes it while only resolving", func() {
folderRepo.result = nil
ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{
@@ -179,8 +178,7 @@ var _ = Describe("processor.acquire", func() {
It("failed-on-unreadable-local: a listed cover that will not open never records absent", func() {
conf.Server.CoverArtPriority = "cover.jpg"
- // A healthy library whose folder listing names a cover the FS will not hand over —
- // what a stale mount looks like from here.
+ // A folder listing that names a cover the FS will not hand over: a stale mount.
libRoot := GinkgoT().TempDir()
Expect(os.MkdirAll(filepath.Join(libRoot, "an-album"), 0o755)).To(Succeed())
libRepo.SetData(model.Libraries{{ID: 0, Path: testFileLibPath(libRoot)}})
@@ -196,12 +194,10 @@ var _ = Describe("processor.acquire", func() {
Expect(err).To(MatchError(model.ErrNotFound), "an I/O fault must not be recorded as absent")
})
- // An upload outranks every other source, so an unreadable one must neither settle absent
- // nor let a lower-priority image take its place.
+ // An upload outranks every source, so an unreadable one must not let a lower one take over.
It("failed-on-unreadable-upload: an upload that will not open never records absent", func() {
if runtime.GOOS == "windows" {
- // os.Chmod cannot revoke read access there, so the file would open and the spec
- // would pass on the decode error instead of the unreadable source.
+ // The file would still open, so the spec would pass on the decode error instead.
Skip("chmod does not restrict read access on Windows")
}
radioRepo := tests.CreateMockedRadioRepo()
@@ -277,7 +273,6 @@ var _ = Describe("processor.acquire", func() {
Expect(ia.Source).To(Equal("external:deezerFake"))
Expect(ia.Hash).ToNot(BeEmpty())
- // External art is content-addressed into the store, not file-backed.
art, err := artRepo.GetImage(ia.Hash)
Expect(err).ToNot(HaveOccurred())
rc, err := store.Open(ia.Hash, art.Mime)
@@ -300,8 +295,7 @@ var _ = Describe("processor.acquire", func() {
ia1, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "al5", model.ImageTypePrimary)
Expect(err).ToNot(HaveOccurred())
- // Poison the stored blurhash: if the second item re-decodes instead of
- // deduping on hash, this sentinel gets overwritten by a real computed value.
+ // A re-decode instead of a hash dedup would overwrite this sentinel.
poisoned := artRepo.Data[ia1.Hash]
poisoned.BlurHash = "SENTINEL"
artRepo.Data[ia1.Hash] = poisoned
@@ -318,8 +312,7 @@ var _ = Describe("processor.acquire", func() {
})
It("two items, two files, identical bytes: each item keeps its own provenance; the shared artwork row is written once", func() {
- // Two distinct library files with byte-identical content resolve to the same
- // hash. Provenance is per-item, so neither file's path may overwrite the other.
+ // Byte-identical files share one hash, but provenance is per item.
libRoot := GinkgoT().TempDir()
imgBytes, err := os.ReadFile(filepath.Join(repoRoot, "tests/fixtures/artist/an-album/cover.jpg"))
Expect(err).ToNot(HaveOccurred())
@@ -345,7 +338,7 @@ var _ = Describe("processor.acquire", func() {
Expect(filepath.ToSlash(iaA.SourcePath)).To(HaveSuffix("album-a/cover.jpg"))
Expect(iaA.RefMtime).To(Equal(time.Unix(1000, 0).UnixNano()))
- // Poison the shared row's blurhash: the second item must dedup on hash, not re-decode.
+ // A re-decode instead of a hash dedup would overwrite this sentinel.
poisoned := artRepo.Data[iaA.Hash]
poisoned.BlurHash = "SENTINEL"
artRepo.Data[iaA.Hash] = poisoned
@@ -359,13 +352,11 @@ var _ = Describe("processor.acquire", func() {
Expect(filepath.ToSlash(iaB.SourcePath)).To(HaveSuffix("album-b/cover.jpg"))
Expect(iaB.RefMtime).To(Equal(time.Unix(2000, 0).UnixNano()))
- // The first item's provenance survives the second item processing identical bytes.
iaAafter, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "alA", model.ImageTypePrimary)
Expect(err).ToNot(HaveOccurred())
Expect(filepath.ToSlash(iaAafter.SourcePath)).To(HaveSuffix("album-a/cover.jpg"))
Expect(iaAafter.RefMtime).To(Equal(time.Unix(1000, 0).UnixNano()))
- // One shared artwork row, and dedup preserved it untouched.
Expect(artRepo.Data).To(HaveLen(1))
reused, err := artRepo.GetImage(iaA.Hash)
Expect(err).ToNot(HaveOccurred())
@@ -411,9 +402,8 @@ var _ = Describe("processor.acquire", func() {
Expect(err).To(MatchError(model.ErrNotFound))
})
- // The cap is compared by division, so it cannot be slipped by dimensions whose product
- // would overflow. No supported format can declare such dimensions today — image/png caps
- // them at 2^30-1 and the rest are 16-bit — so this pins the arithmetic, not a live hole.
+ // The cap is compared by division so an overflowing product cannot slip past it; no supported
+ // format can declare these dimensions, so this pins the arithmetic, not a live hole.
DescribeTable("rejects out-of-range declared dimensions",
func(w, h uint32) {
_, err := decodeArtwork(ctx, "bomb", pngHeaderWithDims(w, h))
@@ -451,8 +441,6 @@ var _ = Describe("processor.acquire", func() {
})
})
-// countingLocker stands in for the worker's prune read-lock, recording how often and how long
-// acquire holds it.
type countingLocker struct {
locks int
unlocks int
diff --git a/core/artwork/prune.go b/core/artwork/prune.go
index 88ef0177a..2146e3778 100644
--- a/core/artwork/prune.go
+++ b/core/artwork/prune.go
@@ -33,8 +33,7 @@ func prune(ctx context.Context, ds model.DataStore, store *ImageStore) error {
log.Info(ctx, "Artwork: Purged dangling queue rows", "count", queuePurged)
}
- // One grace cutoff for both the DB orphan check and the file sweep: files younger
- // than the window may belong to acquisitions whose rows aren't committed yet.
+ // Files younger than the grace window may belong to acquisitions whose rows aren't committed yet.
cutoff := time.Now().Add(-pruneMinAge)
candidates, err := repo.GetOrphanHashes(cutoff)
if err != nil {
@@ -48,8 +47,7 @@ func prune(ctx context.Context, ds model.DataStore, store *ImageStore) error {
if err := repo.DeleteOrphans(cutoff, candidates); err != nil {
return err
}
- // DeleteOrphans may spare candidates reacquired since the snapshot; only remove files
- // for rows actually gone (absent from the post-delete re-read).
+ // DeleteOrphans may spare candidates reacquired since the snapshot; only drop files whose rows are gone.
survivors, err := repo.GetImages(candidates)
if err != nil {
return err
@@ -59,8 +57,6 @@ func prune(ctx context.Context, ds model.DataStore, store *ImageStore) error {
if _, ok := survivors[h]; ok {
continue
}
- // A spared fresh file is at worst a stray a later sweep reclaims;
- // Worker.RunPrune serializes prune against in-flight acquisitions.
if err := store.Remove(h, arts[h].Mime, cutoff); err != nil {
log.Warn(ctx, "Artwork: Could not remove orphan file", "hash", h, err)
}
diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go
index 4afdc6ba9..348c7d15e 100644
--- a/core/artwork/prune_test.go
+++ b/core/artwork/prune_test.go
@@ -107,7 +107,6 @@ var _ = Describe("Prune", func() {
Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg"})).To(Succeed())
ageArtwork(h, time.Now().Add(-2*time.Hour))
awRepo.OrphanHashes = []string{h}
- // Reacquisition: an item now references the hash the snapshot flagged as orphan.
Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: "a1",
ImageType: model.ImageTypePrimary, Hash: h, Source: "folder"})).To(Succeed())
@@ -144,8 +143,7 @@ var _ = Describe("Prune", func() {
Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg"})).To(Succeed())
ageArtwork(h, time.Now().Add(-2*time.Hour))
awRepo.OrphanHashes = []string{h}
- // The row is legitimately orphaned, but a concurrent acquisition just touched the
- // file's mtime (duplicate Write) and is about to commit a row referencing it.
+ // The row is orphaned, but a concurrent acquisition just touched the file's mtime.
Expect(prune(context.Background(), ds, store)).To(Succeed())
@@ -213,24 +211,19 @@ var _ = Describe("Prune", func() {
Expect(os.Chmod(shardDir, 0500)).To(Succeed())
DeferCleanup(func() { _ = os.Chmod(shardDir, 0755) })
- // hb (blocked) is processed first: if store.Remove's failure aborted the loop
- // instead of warning and continuing, hg would never be reached.
+ // hb is processed first: aborting on its Remove failure would leave hg unreached.
awRepo.OrphanHashes = []string{hb, hg}
- // Prune still errors: Sweep independently revisits hb's leftover file and,
- // unlike the loop below, has no warn-and-continue fallback of its own.
+ // Prune still errors: Sweep revisits hb's leftover file with no warn-and-continue of its own.
err := prune(context.Background(), ds, store)
Expect(err).To(HaveOccurred())
- // hg: reached and fully pruned despite being queued after the failing hb -
- // proof the loop didn't return/break on the first Remove error.
_, err = awRepo.GetImage(hg)
Expect(err).To(MatchError(model.ErrNotFound))
_, err = store.Open(hg, "image/jpeg")
Expect(os.IsNotExist(err)).To(BeTrue())
- // hb: row still purged (DeleteOrphans doesn't depend on file removal), but the
- // file itself survives since store.Remove failed and only warned.
+ // The row purge does not depend on file removal, so only the file survives.
_, err = awRepo.GetImage(hb)
Expect(err).To(MatchError(model.ErrNotFound))
rc, err := store.Open(hb, "image/jpeg")
diff --git a/core/artwork/resize.go b/core/artwork/resize.go
index 58c8a2796..b64ad1adb 100644
--- a/core/artwork/resize.go
+++ b/core/artwork/resize.go
@@ -21,12 +21,8 @@ import (
func init() {
conf.AddHook(func() {
- // gen2brain/webp selects native (purego/libwebp) vs WASM in its own
- // package init() and exposes the result only via webp.Dynamic(); there is
- // no runtime way to switch back. On 32-bit ARM/x86 the purego callback path
- // crashes (issue #5597), so those builds must be compiled with the
- // "nodynamic" tag (see Dockerfile), which makes webp.Dynamic() report an
- // error here and forces the safe WASM path.
+ // gen2brain/webp picks native vs WASM in its own init(), with no way to switch at
+ // runtime: 32-bit builds need the "nodynamic" tag (see Dockerfile) to force WASM.
if err := webp.Dynamic(); err != nil {
log.Debug("Artwork: Using WASM WebP encoder/decoder", "reason", err)
} else {
@@ -49,7 +45,6 @@ func resizeImageData(ctx context.Context, ffm ffmpeg.FFmpeg, data []byte, size i
log.Trace(ctx, "Artwork: Resized image", "bytes", len(data), "size", size, "square", square,
"elapsed", time.Since(start))
}()
- // Preserve animation for animated images
if isAnimatedGIF(data) {
if ffm.IsAvailable() {
// Animated GIF: convert to animated WebP via ffmpeg (with optional resize)
@@ -67,11 +62,8 @@ func resizeImageData(ctx context.Context, ffm ffmpeg.FFmpeg, data []byte, size i
return resizeStaticImage(data, size, square)
}
-// toFastScaleType converts images whose concrete type has no optimized scaler
-// in x/image/draw (e.g. *image.NYCbCrA from WebP, *image.Paletted from indexed
-// PNGs) into *image.RGBA, which has a fast path. Without this, CatmullRom.Scale
-// falls back to a generic per-pixel At()/RGBA() loop that is several times
-// slower. Fast-path types are returned unchanged.
+// toFastScaleType converts types x/image/draw has no optimized scaler for (e.g. *image.NYCbCrA,
+// *image.Paletted) to *image.RGBA, avoiding CatmullRom.Scale's generic per-pixel fallback.
func toFastScaleType(img image.Image) image.Image {
switch img.(type) {
case *image.RGBA, *image.NRGBA, *image.Gray, *image.YCbCr:
diff --git a/core/artwork/resolve.go b/core/artwork/resolve.go
index 5826c913c..e50532d7b 100644
--- a/core/artwork/resolve.go
+++ b/core/artwork/resolve.go
@@ -24,21 +24,19 @@ type resolution struct {
source string // model.ItemArtwork.Source value: "folder", "embedded", "external", "upload", "generated"
sourcePath string // backing library/upload file (folder/upload: the image; embedded: the audio file); "" otherwise
refMtime int64 // sourcePath mtime (unix-nanoseconds) at resolution; 0 when no sourcePath
- // external source errored/timed out. With no reader: forces failed (never absent).
- // On a hit: a higher-priority external step failed—serve this, but retry later.
+ // external source errored/timed out. With no reader it forces failed (never absent);
+ // on a hit a higher-priority external step failed—serve this, but retry later.
extError bool
- // a local source that should have been readable wasn't (stale mount, permissions).
- // With no reader: forces failed, so a transient I/O fault never records absent.
+ // a local source that should have been readable wasn't. With no reader it forces failed,
+ // so a transient I/O fault never records absent.
localError bool
}
// chainState carries what a priority walk has seen so far. A hit takes extErr with it so a
-// transient external failure still schedules a retry, but not localErr: a local candidate a
-// later source recovered from is re-listed by the scanner when it actually changes.
+// transient external failure still retries; localErr is dropped, as the scanner re-lists changes.
type chainState struct{ extErr, localErr bool }
-// try reports a hit, stamping the accumulated external failure onto it, and otherwise records
-// the miss. Folding both into one call is what keeps the OR from being forgotten at a new site.
+// try stamps the accumulated external failure onto a hit, and records the miss otherwise.
func (c *chainState) try(res resolution, ok bool) (resolution, bool) {
if ok {
res.extError = c.extErr
@@ -53,15 +51,13 @@ func (c *chainState) exhausted() resolution {
return resolution{extError: c.extErr, localError: c.localErr}
}
-// externalSource is the ability to reach the network: which agents to ask, and the per-agent
-// rate limiter and circuit breaker to ask them through.
+// externalSource holds the agents to ask and the rate limiter/circuit breaker to ask them through.
type externalSource struct {
agents *agents.Agents
gate gateFunc
}
-// resolver walks a kind's priority chain and returns the first hit. A nil ext means local-only,
-// so "may this resolution go external" is one fact rather than two that could disagree.
+// resolver walks a kind's priority chain and returns the first hit; a nil ext means local-only.
type resolver struct {
ds model.DataStore
ffmpeg ffmpeg.FFmpeg
@@ -75,8 +71,8 @@ func newResolver(ds model.DataStore, ag *agents.Agents, ffm ffmpeg.FFmpeg, gate
return &resolver{ds: ds, ffmpeg: ffm, ext: &externalSource{agents: ag, gate: gate}}
}
-// newLocalResolver offers no parameter through which an external source could be supplied, so
-// a request can neither reach the network nor sample album art for the worker-built grid.
+// newLocalResolver builds a resolver that can neither reach the network nor sample album art
+// for the worker-built grid.
func newLocalResolver(ds model.DataStore, ffm ffmpeg.FFmpeg) *resolver {
return &resolver{ds: ds, ffmpeg: ffm}
}
@@ -115,8 +111,7 @@ func (r *resolver) fetchExternalArtist(ctx context.Context, ar model.Artist) (io
return fetchArtistImage(ctx, r.ext.agents, r.ext.gate, ar)
}
-// resolveAlbum ports the folder/embedded/external selection from
-// reader_album.go, walking conf.Server.CoverArtPriority.
+// resolveAlbum walks conf.Server.CoverArtPriority over the folder, embedded and external sources.
func (r *resolver) resolveAlbum(ctx context.Context, albumID string) (resolution, error) {
al, err := r.ds.Album(ctx).Get(albumID)
if err != nil {
@@ -154,8 +149,7 @@ func (r *resolver) resolveAlbum(ctx context.Context, albumID string) (resolution
return chain.exhausted(), nil
}
-// resolveArtist ports the upload/folder/external selection from
-// reader_artist.go: upload always wins, then conf.Server.ArtistArtPriority.
+// resolveArtist tries the uploaded image first, then walks conf.Server.ArtistArtPriority.
func (r *resolver) resolveArtist(ctx context.Context, artistID string) (resolution, error) {
ar, err := r.ds.Artist(ctx).Get(artistID)
if err != nil {
@@ -171,7 +165,7 @@ func (r *resolver) resolveArtist(ctx context.Context, artistID string) (resoluti
return upload, nil
}
- // Only consider albums where the artist is the sole album artist, same as reader_artist.go.
+ // Only consider albums where the artist is the sole album artist.
als, err := r.ds.Album(ctx).GetAll(model.QueryOptions{
Filters: squirrel.And{
squirrel.Eq{"album_artist_id": artistID},
@@ -230,8 +224,7 @@ func (r *resolver) resolveArtist(ctx context.Context, artistID string) (resoluti
return chain.exhausted(), nil
}
-// resolvePlaylist ports reader_playlist.go's chain: uploaded image, sidecar,
-// ExternalImageURL, then the generated 2x2 grid sourced through resolveAlbum.
+// resolvePlaylist tries the uploaded image, the sidecar and ExternalImageURL, then a generated grid.
func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (resolution, error) {
pl, err := r.ds.Playlist(ctx).Get(playlistID)
if err != nil {
@@ -252,8 +245,7 @@ func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (reso
return res, nil
}
}
- // A local ExternalImageURL is a file-backed reference: serve it in place (staleness-checked,
- // and available even on the request path). Only http(s) URLs need the gated remote fetch.
+ // A local ExternalImageURL is file-backed and served in place; only http(s) needs the gated fetch.
localImg, remoteImg := classifyPlaylistImage(pl.ExternalImageURL)
if localImg != "" {
res, ok := resolveLocalFile(localImg, "folder")
@@ -265,8 +257,7 @@ func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (reso
}
}
if r.ext == nil {
- // The remote ExternalImageURL fetch and the 2x2 grid are worker-only; a request must
- // not fetch remotely nor sample album art synchronously.
+ // The remote fetch and the generated grid are worker-only; a request must do neither.
return resolution{}, nil
}
if remoteImg != nil && conf.Server.EnableM3UExternalAlbumArt {
@@ -309,14 +300,13 @@ func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (reso
}
}
if len(tiles) == 0 {
- // A tile-level failure must never resolve as a clean absent: propagate
- // internal errors, and force extError for external ones.
+ // A tile-level failure must never resolve as a clean absent.
if tileErr != nil {
return resolution{}, fmt.Errorf("resolvePlaylist: sampled album art failed: %w", tileErr)
}
return resolution{extError: extErr}, nil
}
- // Grow to 4 tiles by repeating what we have, mirroring reader_playlist.go's loadTiles.
+ // Grow to 4 tiles by repeating what we have.
switch len(tiles) {
case 2:
tiles = append(tiles, tiles[1], tiles[0])
@@ -330,7 +320,7 @@ func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (reso
return resolution{reader: grid, source: "generated", extError: extErr}, nil
}
-// resolveRadio ports reader_radio.go: only an uploaded image, no fallback.
+// resolveRadio serves only an uploaded image; there is no fallback.
func (r *resolver) resolveRadio(ctx context.Context, radioID string) (resolution, error) {
radio, err := r.ds.Radio(ctx).Get(radioID)
if err != nil {
@@ -340,8 +330,8 @@ func (r *resolver) resolveRadio(ctx context.Context, radioID string) (resolution
return res, nil
}
-// resolveMediaFile resolves a track's own embedded art only; there is no folder or
-// external fallback, so disabled/missing cover art is a definitive absent.
+// resolveMediaFile resolves a track's own embedded art only, so disabled or missing cover art
+// is a definitive absent.
func (r *resolver) resolveMediaFile(ctx context.Context, id string) (resolution, error) {
mf, err := r.ds.MediaFile(ctx).Get(id)
if err != nil {
@@ -358,9 +348,8 @@ func (r *resolver) resolveMediaFile(ctx context.Context, id string) (resolution,
return res, nil
}
-// resolveExternalStep runs a single external sourceFunc through the named gate; used by
-// the playlist ExternalImageURL step. ok reports a hit; extErr reports a non-not-found
-// error (a not-found is a definitive "no", not a failure).
+// resolveExternalStep runs a single external sourceFunc through the named gate. extErr excludes
+// a not-found, which is a definitive "no" rather than a failure.
func resolveExternalStep(gate gateFunc, name string, sf sourceFunc) (res resolution, ok bool, extErr bool) {
r, path, err := gate(name, sf)
if r != nil {
@@ -369,8 +358,8 @@ func resolveExternalStep(gate gateFunc, name string, sf sourceFunc) (res resolut
return resolution{}, false, err != nil && !errors.Is(err, model.ErrNotFound)
}
-// classifyPlaylistImage splits a playlist ExternalImageURL into a local filesystem path
-// (served file-backed) or a remote http(s) URL (fetched and stored); at most one is set.
+// classifyPlaylistImage splits a playlist ExternalImageURL into a local filesystem path or a
+// remote http(s) URL; at most one is set.
func classifyPlaylistImage(imageURL string) (localPath string, remote *url.URL) {
if imageURL == "" {
return "", nil
@@ -429,8 +418,8 @@ func resolveArtistFolderPattern(ctx context.Context, lib libraryView, artistFold
return resolution{reader: r, source: "folder", sourcePath: path, refMtime: mtimeOf(path)}, true
}
-// resolveLocalFile opens an absolute path directly (uploads, image-folder). A missing path is
-// "no source"; any other open failure says nothing about whether the image exists.
+// resolveLocalFile opens an absolute path directly. A missing path is "no source"; any other
+// open failure says nothing about whether the image exists.
func resolveLocalFile(path, source string) (resolution, bool) {
if path == "" {
return resolution{}, false
@@ -450,8 +439,7 @@ func mtimeOf(path string) int64 {
return info.ModTime().UnixNano()
}
-// mtimeViaFS stats through the library FS instead of a joined absolute path,
-// since library roots in tests may not be real OS paths (e.g. testfile://).
+// mtimeViaFS stats through the library FS, since library roots in tests may not be real OS paths.
func mtimeViaFS(fsys fs.FS, name string) int64 {
if fsys == nil || name == "" {
return 0
diff --git a/core/artwork/resolve_test.go b/core/artwork/resolve_test.go
index 1459d7da8..fc1a6a3ff 100644
--- a/core/artwork/resolve_test.go
+++ b/core/artwork/resolve_test.go
@@ -387,8 +387,7 @@ var _ = Describe("resolveItem", func() {
Expect(img.Bounds().Dx()).To(Equal(expectedSize))
Expect(img.Bounds().Dy()).To(Equal(expectedSize))
},
- // tileSize-1: the 4-tile canvas is built as [0, tileSize-1], matching
- // reader_playlist.go's createTiledImage exactly.
+ // tileSize-1: the 4-tile canvas is built as [0, tileSize-1].
Entry("1 album -> single tile", []string{"t1"}, tileSize/2),
Entry("2 albums -> duplicated to 4 tiles", []string{"t1", "t2"}, tileSize-1),
Entry("3 albums -> duplicated to 4 tiles", []string{"t1", "t2", "t3"}, tileSize-1),
@@ -508,8 +507,7 @@ var _ = Describe("resolveItem", func() {
Expect(res.extError).To(BeFalse())
})
- // A local resolver holds no agents, and the external branch dereferences them before the
- // gate is ever consulted, so reaching it at all would panic rather than degrade.
+ // A local resolver holds no agents: reaching the external branch would panic, not degrade.
It("skips the external step instead of dereferencing absent agents", func() {
conf.Server.CoverArtPriority = "external"
ds.MockedAlbum = tests.CreateMockAlbumRepo()
@@ -521,8 +519,7 @@ var _ = Describe("resolveItem", func() {
Expect(res.extError).To(BeFalse(), "a skipped step is not a failed one")
})
- // The request path must never reach the network nor sample album art synchronously. The
- // worker resolving the same playlist is asserted alongside, so this cannot pass vacuously.
+ // The worker resolving the same playlist is asserted alongside, so this cannot pass vacuously.
It("resolves a playlist locally without fetching remotely or building the grid", func() {
conf.Server.EnableM3UExternalAlbumArt = true
var hits atomic.Int32
@@ -587,7 +584,6 @@ var _ = Describe("resolveItem", func() {
})
It("skips a grid tile whose declared dimensions are a decompression bomb", func() {
- // End-to-end regression: a bomb-declaring tile must not break the grid.
libRoot := GinkgoT().TempDir()
Expect(os.MkdirAll(filepath.Join(libRoot, "bomb"), 0755)).To(Succeed())
Expect(os.WriteFile(filepath.Join(libRoot, "bomb", "cover.jpg"), pngHeaderWithDims(50000, 50000), 0600)).To(Succeed())
@@ -606,8 +602,7 @@ var _ = Describe("resolveItem", func() {
})
It("does not resolve as absent when every sampled album fails to resolve", func() {
- // "missing1"/"missing2" are not in MockAlbumRepo's data, so resolveAlbum
- // returns a genuine (non-external) error for every sampled tile.
+ // The album ids are absent from MockAlbumRepo, so every tile fails non-externally.
plRepo := tests.CreateMockPlaylistRepo()
plRepo.SetData(model.Playlists{{ID: "pl3", Name: "Playlist"}})
plRepo.TracksRepo = &tests.MockPlaylistTrackRepo{AlbumIDs: []string{"missing1", "missing2"}}
@@ -620,8 +615,7 @@ var _ = Describe("resolveItem", func() {
})
})
-// decodeTile runs on every sampled album's resolved bytes before the processor's
-// own guards apply, so it must enforce the same caps independently.
+// decodeTile runs before the processor's own guards, so it must enforce the caps itself.
var _ = Describe("decodeTile", func() {
It("rejects a decompression bomb before the full decode", func() {
data := pngHeaderWithDims(50000, 50000) // 2.5 gigapixels, far above the cap
diff --git a/core/artwork/sources.go b/core/artwork/sources.go
index b2cd5be44..885ca03cf 100644
--- a/core/artwork/sources.go
+++ b/core/artwork/sources.go
@@ -187,9 +187,8 @@ func fromURL(ctx context.Context, imageUrl *url.URL) (io.ReadCloser, string, err
if err != nil {
return nil, "", err
}
- // A dead image URL is a definitive miss, not a transient fault: agents (e.g. Last.fm) can
- // advertise an image URL that 404s. Map it to ErrNotFound so it settles absent instead of
- // retrying forever and tripping the artwork breaker.
+ // An agent-advertised URL that 404s is a definitive miss, not a fault: settle absent
+ // instead of retrying forever and tripping the artwork breaker.
if resp.StatusCode == http.StatusNotFound || resp.StatusCode == http.StatusGone {
resp.Body.Close()
return nil, "", model.ErrNotFound
diff --git a/core/artwork/uploader.go b/core/artwork/uploader.go
index 2c3c380dd..9c38ae2e5 100644
--- a/core/artwork/uploader.go
+++ b/core/artwork/uploader.go
@@ -15,8 +15,7 @@ import (
"github.com/navidrome/navidrome/utils"
)
-// MaxImageUploadSize returns the configured MaxImageUploadSize in bytes, or the built-in default
-// when it's unset/invalid. Shared by every API that accepts image uploads.
+// MaxImageUploadSize returns the configured max upload size in bytes, or the built-in default.
func MaxImageUploadSize() int64 {
if size, err := humanize.ParseBytes(conf.Server.MaxImageUploadSize); err == nil && size > 0 {
return int64(size)
@@ -25,18 +24,15 @@ func MaxImageUploadSize() int64 {
return int64(size)
}
-// Uploader stores a user-uploaded entity image and invalidates that entity's artwork state so the
-// upload becomes the served cover.
+// Uploader stores a user-uploaded entity image and invalidates that entity's artwork state.
type Uploader interface {
SetImage(ctx context.Context, entityType string, entityID string, name string, oldPath string, reader io.Reader, ext string) (filename string, err error)
RemoveImage(ctx context.Context, path string) error
- // EnqueueArtwork clears an item's resolved state and re-queues it at Bump priority. Callers
- // must invoke it AFTER persisting the new filename, so the worker never resolves the old one.
+ // EnqueueArtwork re-resolves the item's artwork. Call it AFTER persisting the new
+ // filename, or the worker resolves the old one.
EnqueueArtwork(ctx context.Context, entityType, entityID string)
}
-// uploadEntityKind maps an upload's entity type to its artwork kind prefix, so a
-// successful upload can clear and re-queue that item's artwork state.
var uploadEntityKind = map[string]model.Kind{
consts.EntityArtist: model.KindArtistArtwork,
consts.EntityPlaylist: model.KindPlaylistArtwork,
@@ -59,14 +55,12 @@ func (s *uploader) SetImage(ctx context.Context, entityType string, entityID str
return "", fmt.Errorf("creating image directory: %w", err)
}
- // Remove old image if it exists
if oldPath != "" {
if err := os.Remove(oldPath); err != nil && !os.IsNotExist(err) {
log.Warn(ctx, "Artwork: Failed to remove old image", "path", oldPath, err)
}
}
- // Save new image
f, err := os.Create(absPath)
if err != nil {
return "", fmt.Errorf("creating image file: %w", err)
@@ -79,8 +73,6 @@ func (s *uploader) SetImage(ctx context.Context, entityType string, entityID str
return filename, nil
}
-// EnqueueArtwork clears the item's resolved state and re-queues it at Bump priority: the
-// upload is now the top-priority source, so the worker re-resolves and the UI swaps.
func (s *uploader) EnqueueArtwork(ctx context.Context, entityType, id string) {
kind, ok := uploadEntityKind[entityType]
if !ok {
diff --git a/core/artwork/worker.go b/core/artwork/worker.go
index 1e0a33444..c994e3320 100644
--- a/core/artwork/worker.go
+++ b/core/artwork/worker.go
@@ -22,13 +22,13 @@ import (
const (
workerPollInterval = 5 * time.Second
backoffBase = 5 * time.Second
- // giveUpAfter bounds the retry budget from enqueue: past it the worker stops retrying and
- // hands the item to the periodic stale-absent recheck (settling absent on a bare failure).
+ // giveUpAfter bounds the retry budget from enqueue; past it the item falls to the
+ // periodic stale-absent recheck.
giveUpAfter = 12 * time.Hour
)
-// drainPool drains one class of work with its own slot budget, so a kind whose resolution
-// blocks cannot occupy slots another kind needs.
+// drainPool drains one class of work with its own slot budget, so a blocking kind cannot
+// occupy slots another kind needs.
type drainPool struct {
name string
kinds []string
@@ -36,9 +36,8 @@ type drainPool struct {
wake chan struct{}
}
-// Worker drains the artwork queue through the processor: each external agent is rate-limited
-// and circuit-broken independently, and prune is serialized against the store-write window
-// via pruneMu.
+// Worker drains the artwork queue: each external agent is rate-limited and circuit-broken
+// independently, and pruneMu serializes prune against the store-write window.
type Worker struct {
proc *processor
cache cache.FileCache
@@ -67,8 +66,8 @@ func NewWorker(ds model.DataStore, store *ImageStore, ag *agents.Agents, ffmpeg
return w
}
-// newDrainPools splits the drain by what bounds it: gate() waits for its rate-limit permit
-// while holding a slot, so a sleeping lookup would otherwise crowd out a cover sitting on disk.
+// newDrainPools splits the drain by what bounds it: gate() holds a slot while waiting for its
+// rate-limit permit, so a sleeping lookup would crowd out a cover sitting on disk.
func newDrainPools() []*drainPool {
budget := conf.MaxOpenConns() // floored at 4, so both remainders below stay positive
local := min(max(1, conf.Server.ArtworkWorkerConcurrency), budget-1)
@@ -80,8 +79,7 @@ func newDrainPools() []*drainPool {
}
}
-// Kind is a proxy for cost: an album whose chain reaches "external" still costs a local slot,
-// but only the few with no local art do.
+// Kind is a proxy for cost: an album that reaches an external agent still costs a local slot.
var (
externalDrainKinds = []string{model.KindArtistArtwork.Prefix()}
localDrainKinds = []string{
@@ -92,8 +90,7 @@ var (
}
)
-// Run blocks draining the queue until ctx is cancelled. It exits cleanly with no
-// leaked goroutines: each drain waits for its batch before the loop can return.
+// Run blocks draining the queue until ctx is cancelled.
func (w *Worker) Run(ctx context.Context) error {
w.runCtx = ctx
var wg sync.WaitGroup
@@ -104,7 +101,6 @@ func (w *Worker) Run(ctx context.Context) error {
return nil
}
-// runPool drains one pool's kinds until ctx is cancelled.
func (w *Worker) runPool(ctx context.Context, p *drainPool) {
ticker := time.NewTicker(workerPollInterval)
defer ticker.Stop()
@@ -117,7 +113,7 @@ func (w *Worker) runPool(ctx context.Context, p *drainPool) {
return
}
if n > 0 {
- continue // keep draining while this pool has ready work
+ continue
}
select {
case <-ctx.Done():
@@ -128,8 +124,7 @@ func (w *Worker) runPool(ctx context.Context, p *drainPool) {
}
}
-// Bump enqueues an item at the highest priority and wakes the drain loop. It is
-// non-blocking: a wake already pending is enough.
+// Bump enqueues an item at the highest priority and wakes the drain loops.
func (w *Worker) Bump(kind, id string) {
item := model.ArtworkQueueItem{
ItemKind: kind,
@@ -141,8 +136,7 @@ func (w *Worker) Bump(kind, id string) {
log.Warn("Artwork: Could not bump queue item", "kind", kind, "id", id, err)
return
}
- // Waking all beats routing by kind: a spurious wake costs one empty dequeue, while an
- // unrouted kind would never wake at all.
+ // Wake every pool: a spurious wake only costs an empty dequeue.
for _, p := range w.pools {
select {
case p.wake <- struct{}{}:
@@ -159,27 +153,25 @@ func (w *Worker) RunPrune(ctx context.Context) error {
return prune(ctx, w.proc.ds, w.proc.store)
}
-// Backfill enqueues every entity for re-resolution when the artwork config fingerprint has
-// changed since the last run, artists first. It reports whether anything was enqueued.
+// Backfill enqueues every entity for re-resolution when the artwork config fingerprint changed,
+// artists first. It reports whether anything was enqueued.
func (w *Worker) Backfill(ctx context.Context) (bool, error) {
return backfill(ctx, w.proc.ds)
}
-// EnqueueStaleAbsentAll requeues known-absent entries older than staleAbsentAge, so artwork
-// that appeared since the last attempt is eventually picked up.
+// EnqueueStaleAbsentAll requeues known-absent entries older than staleAbsentAge.
func (w *Worker) EnqueueStaleAbsentAll(ctx context.Context) error {
return enqueueStaleAbsentAll(ctx, w.proc.ds)
}
-// EnqueueMissingAll requeues entities that have no artwork state row at all: the safety net
-// for anything a scan never enqueued.
+// EnqueueMissingAll requeues entities with no artwork state row: the safety net for anything
+// a scan never enqueued.
func (w *Worker) EnqueueMissingAll(ctx context.Context) error {
return enqueueMissingAll(ctx, w.proc.ds)
}
func (w *Worker) drain(ctx context.Context, concurrency int, kinds ...string) (int, error) {
- // Dequeue well past the worker pool so a slow item (an external lookup burning its
- // timeout) never idles the other slots: the pool stays fed until the batch runs out.
+ // Dequeue well past the pool size so a slow external lookup never idles the other slots.
// DequeueBatch does not mark rows taken, so this is one query per pass, not per slot.
items, err := w.proc.ds.ArtworkQueue(ctx).DequeueBatch(max(16, 4*concurrency), kinds...)
if err != nil {
@@ -189,8 +181,8 @@ func (w *Worker) drain(ctx context.Context, concurrency int, kinds ...string) (i
return 0, nil
}
drainStart := time.Now()
- // Resolved only once there is work, and per drain rather than per item: the worker needs an
- // admin identity for private playlists, and can start before any admin exists.
+ // Private playlists need an admin identity; resolved per drain because the worker can
+ // start before any admin exists.
ctx = auth.WithAdminUser(ctx, w.proc.ds)
sem := make(chan struct{}, concurrency)
var wg sync.WaitGroup
@@ -206,16 +198,15 @@ func (w *Worker) drain(ctx context.Context, concurrency int, kinds ...string) (i
wg.Go(func() {
defer func() { <-sem }()
out, got := w.process(ctx, item)
- // Refresh clients on any visible state change: found/foundStale (new art) and absent
- // (removed art — clients must drop a previously-served immutable cover). foundStale
- // also wrote a served state row.
+ // Absent counts as a visible change too: clients must drop a previously-served
+ // immutable cover.
if out == outcomeFound || out == outcomeFoundStale || out == outcomeAbsent {
refreshMu.Lock()
refresh = append(refresh, item)
refreshMu.Unlock()
}
- // Precache only actual images. Post-outcome only: the queue row was already settled
- // by process, so warming the resize cache here can never block or alter queue ops.
+ // Post-outcome: the queue row is already settled, so warming the cache can't
+ // block or alter queue ops.
if got != nil {
w.precache(ctx, got)
}
@@ -228,8 +219,8 @@ func (w *Worker) drain(ctx context.Context, concurrency int, kinds ...string) (i
return len(items), nil
}
-// artworkKindToResource maps an artwork kind to the UI resource name carried in the refresh
-// event; note media_file maps to "song", so this can't derive from Kind.String().
+// artworkKindToResource maps a kind to its UI resource name; media_file maps to "song", so
+// this can't derive from Kind.String().
var artworkKindToResource = map[model.Kind]string{
model.KindAlbumArtwork: "album",
model.KindArtistArtwork: "artist",
@@ -238,8 +229,8 @@ var artworkKindToResource = map[model.Kind]string{
model.KindMediaFileArtwork: "song",
}
-// broadcastRefresh emits one coalesced RefreshResource for the batch's newly-acquired
-// artwork, so connected UIs re-fetch the affected records (and pick up the new coverArt id).
+// broadcastRefresh emits one coalesced RefreshResource for the batch, so UIs re-fetch the
+// affected records and pick up the new coverArt id.
func (w *Worker) broadcastRefresh(ctx context.Context, found []model.ArtworkQueueItem) {
if len(found) == 0 {
return
@@ -270,16 +261,16 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) (outc
queue := w.proc.ds.ArtworkQueue(ctx)
switch out {
case outcomeFound, outcomeAbsent:
- // DeleteIfUnchanged, not Delete: a scan that re-enqueued this row mid-flight reset
- // its retry_at, so the row survives here and the next drain re-resolves it.
+ // A scan that re-enqueued this row mid-flight reset its retry_at, so the row survives
+ // here and the next drain re-resolves it.
if err := queue.DeleteIfUnchanged(item.ItemKind, item.ItemID, item.ImageType, item.RetryAt); err != nil {
log.Warn(ctx, "Artwork: Could not delete processed queue item", "kind", item.ItemKind, "id", item.ItemID, err)
}
case outcomeFoundStale, outcomeFailed:
retryAt := time.Now().Add(backoff(item.Attempts))
if retryAt.Before(item.EnqueuedAt.Add(giveUpAfter)) {
- // MarkFailedIfUnchanged, not MarkFailed: a scan that re-enqueued this row mid-flight reset
- // retry_at, so stale backoff must not stomp its fresh, immediate eligibility.
+ // A mid-flight re-enqueue reset retry_at; stale backoff must not stomp its
+ // fresh, immediate eligibility.
if err := queue.MarkFailedIfUnchanged(item.ItemKind, item.ItemID, item.ImageType, item.RetryAt, retryAt); err != nil {
log.Warn(ctx, "Artwork: Could not reschedule failed queue item", "kind", item.ItemKind, "id", item.ItemID, err)
}
@@ -288,10 +279,8 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) (outc
"budgetLeft", time.Until(item.EnqueuedAt.Add(giveUpAfter)))
break
}
- // Retry budget exhausted: stop retrying. Absent is only recoverable where a periodic
- // recheck will revisit it, so kinds without one keep no row at all; and art already
- // being served is kept, since exhaustion means the source stayed unreachable rather
- // than that the entity lost its cover.
+ // Absent is only recoverable where a periodic recheck revisits it, so other kinds keep
+ // no row; art already being served is kept, as exhaustion means unreachable, not removed.
settled := "kept previous state"
if out == outcomeFailed && hasRecheckPath(item.ItemKind) && !w.hasResolvedArtwork(ctx, item) {
writeAbsent(ctx, w.proc.ds.Artwork(ctx), item)
@@ -306,7 +295,6 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) (outc
return out, got
}
-// hasResolvedArtwork reports whether the item already has a hash-bearing state row.
func (w *Worker) hasResolvedArtwork(ctx context.Context, item model.ArtworkQueueItem) bool {
kind, ok := model.ParseKind(item.ItemKind)
if !ok {
@@ -317,15 +305,14 @@ func (w *Worker) hasResolvedArtwork(ctx context.Context, item model.ArtworkQueue
}
// precache warms the resize cache at the UI cover size from the bytes just acquired, so the
-// first UI request is a cache hit without re-reading the rows or the file. Skipped when
-// disabled; failures are debug-only.
+// first UI request hits without re-reading the rows or the file.
func (w *Worker) precache(ctx context.Context, got *acquired) {
if !conf.Server.EnableArtworkPrecache || w.cache == nil || w.cache.Disabled(ctx) {
return
}
precacheStart := time.Now()
- // Same key as the serving path (hash/size/square); only the source of the bytes differs.
- // square matches what the list surfaces request, otherwise this warms a key nothing reads.
+ // Same key as the serving path: square must match what the list surfaces request, or this
+ // warms a key nothing reads.
item := &resizedItem{
hash: got.ia.Hash,
size: conf.Server.UICoverArtSize,
diff --git a/core/artwork/worker_soak_test.go b/core/artwork/worker_soak_test.go
index e905664a4..eb7346102 100644
--- a/core/artwork/worker_soak_test.go
+++ b/core/artwork/worker_soak_test.go
@@ -17,13 +17,10 @@ import (
. "github.com/onsi/gomega"
)
-// soakCycles is deliberately >2000: this is a leak regression guard, not a
-// performance benchmark, so it favors a stable signal over raw speed.
+// Deliberately >2000: a leak guard favors a stable signal over speed.
const soakCycles = 2200
var _ = Describe("Worker soak", func() {
- // Runs acquisition over many cycles across a mix of sources, asserting
- // goroutines/heap plateau instead of growing unbounded (a leak guard). Skipped under -short.
It("does not leak goroutines, heap, or fds over many acquisition cycles", func() {
if testing.Short() {
Skip("skipping soak test in short mode")
@@ -57,8 +54,7 @@ var _ = Describe("Worker soak", func() {
proc := &processor{ds: ds, store: store, resolver: newResolver(ds, ag, ffm, nil)}
conf.Server.CoverArtPriority = "cover.jpg, embedded"
- // Dangling refs (al/ra ids the repos don't know about) mirror an entity
- // deleted after being enqueued; ds.Radio auto-provisions an empty mock repo.
+ // Dangling refs mirror an entity deleted after being enqueued.
items := []model.ArtworkQueueItem{
{ItemKind: "al", ItemID: "al-folder"},
{ItemKind: "al", ItemID: "al-embed"},
@@ -78,8 +74,7 @@ var _ = Describe("Worker soak", func() {
}
settleGoroutines := func() int {
- // Background goroutines (GC workers, etc.) can take a moment to wind down;
- // poll for two consecutive equal samples instead of trusting a single one.
+ // Background goroutines wind down late, so poll for two consecutive equal samples.
prev := -1
for range 100 {
runtime.GC()
@@ -102,8 +97,7 @@ var _ = Describe("Worker soak", func() {
it := items[i%len(items)]
out, _ := proc.acquire(context.Background(), it)
- // "Serve-adjacent" read-back: exercise the Phase 2 surfaces a caller would
- // use after acquisition, not the old serving pipeline.
+ // Read-back exercises the surfaces a caller would use after acquisition.
if out == outcomeFound {
kind, _ := model.ParseKind(it.ItemKind)
ia, err := artRepo.GetItemArtwork(kind, it.ItemID, model.ImageTypePrimary)
diff --git a/core/artwork/worker_test.go b/core/artwork/worker_test.go
index 903a1e86f..d2a2f0dac 100644
--- a/core/artwork/worker_test.go
+++ b/core/artwork/worker_test.go
@@ -22,8 +22,6 @@ import (
"go.uber.org/goleak"
)
-// recordingCache captures the keys passed to Get so precache warming can be asserted,
-// and can be forced Disabled to exercise the skip path.
type recordingCache struct {
cache.FileCache
mu sync.Mutex
@@ -48,8 +46,8 @@ func (c *recordingCache) getKeys() []string {
return append([]string(nil), c.keys...)
}
-// reenqueueOnDequeue simulates a concurrent scan Enqueue between DequeueBatch and the
-// worker's delete by bumping retry_at, so a DeleteIfUnchanged on the dequeued value no-ops.
+// Simulates a concurrent Enqueue between DequeueBatch and the worker's delete, so
+// DeleteIfUnchanged on the dequeued value no-ops.
type reenqueueOnDequeue struct {
*tests.MockArtworkQueueRepo
done bool
@@ -303,8 +301,7 @@ var _ = Describe("Worker", func() {
Expect(err).ToNot(HaveOccurred())
Expect(n).To(Equal(1))
- // The concurrent re-enqueue reset retry_at; the failure path must not stomp it
- // with stale backoff nor bump attempts, so the row stays immediately eligible.
+ // The re-enqueue reset retry_at; the failure path must not stomp it nor bump attempts.
it := findQueued(queueRepo, "al", "al8")
Expect(it).ToNot(BeNil())
Expect(it.Attempts).To(BeZero())
@@ -317,7 +314,7 @@ var _ = Describe("Worker", func() {
imageAgents(&fakeImageAgent{name: "failAgent", err: errors.New("agent timed out")})
w = NewWorker(ds, store, ag, ffm, broker, imgCache)
Expect(queueRepo.Enqueue(model.ArtworkQueueItem{ItemKind: "al", ItemID: "al9"})).To(Succeed())
- // Age the row past the budget so the next retry would land beyond enqueued_at+giveUpAfter.
+ // Age the row past the retry budget.
for k, v := range queueRepo.Data {
if v.ItemID == "al9" {
v.EnqueuedAt = time.Now().Add(-(giveUpAfter + time.Hour))
@@ -329,7 +326,6 @@ var _ = Describe("Worker", func() {
Expect(err).ToNot(HaveOccurred())
Expect(n).To(Equal(1))
- // Row removed (stops retrying) and the failure settles absent for the periodic sweep.
Expect(findQueued(queueRepo, "al", "al9")).To(BeNil())
ia, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "al9", model.ImageTypePrimary)
Expect(err).ToNot(HaveOccurred())
@@ -363,8 +359,8 @@ var _ = Describe("Worker", func() {
Expect(ia.Hash).To(Equal("cafebabe"), "a persistent outage must not discard served art")
})
- // Media files are excluded from recheckKinds, so an absent row written here would never
- // be revisited: a transient read error would look like "this track has no cover" forever.
+ // Media files are excluded from recheckKinds, so an absent row here would never be
+ // revisited: a transient read error would look permanent.
It("does not settle absent on exhaustion for a kind with no recheck path", func() {
conf.Server.EnableMediaFileCoverArt = true
ds.MockedMediaFile = tests.CreateMockMediaFileRepo()
@@ -403,7 +399,6 @@ var _ = Describe("Worker", func() {
Expect(err).ToNot(HaveOccurred())
Expect(n).To(Equal(1))
- // Resolved as absent (no art) and removed — not stuck failing on ErrNotFound forever.
Expect(findQueued(queueRepo, "pl", "plPriv")).To(BeNil())
ia, err := artRepo.GetItemArtwork(model.KindPlaylistArtwork, "plPriv", model.ImageTypePrimary)
Expect(err).ToNot(HaveOccurred())
@@ -526,9 +521,8 @@ var _ = Describe("Worker", func() {
})
It("does not open the breaker on a run of agent not-found misses", func() {
- // Regression: agents.ErrNotFound is a definitive miss, not a fault. A run of
- // artless items must never trip the breaker, or they'd loop in retry instead of
- // settling absent. Uses the real gate, not passthroughGate.
+ // agents.ErrNotFound is a definitive miss, not a fault: artless items must not
+ // trip the breaker, or they would loop in retry instead of settling absent.
notFound := func() (io.ReadCloser, string, error) { return nil, "", agents.ErrNotFound }
for range breakerThreshold + 3 {
_, _, err := w.gate("A", notFound)
@@ -601,8 +595,7 @@ var _ = Describe("Worker", func() {
Expect(imgCache.getKeys()).To(BeEmpty())
})
- // It warms from the bytes the acquisition already held, so no state row or store file
- // needs to exist for it to work.
+ // It warms from the bytes acquisition already held, so no state row or store file is needed.
It("warms from the acquired bytes without reading them back", func() {
conf.Server.EnableArtworkPrecache = true
ia := &model.ItemArtwork{
@@ -615,9 +608,8 @@ var _ = Describe("Worker", func() {
w.precache(ctx, &acquired{ia: ia, mime: "image/jpeg", data: data})
- // Nothing backs that hash on disk or in the store, so the entry can only have come
- // from the bytes handed in. Probing with a source that refuses to open proves it
- // is really cached rather than re-read on demand.
+ // Nothing backs that hash on disk, so a hit can only come from the bytes handed in;
+ // the probe refuses to open, proving nothing is re-read.
probe := &resizedItem{
hash: ia.Hash, size: 300, square: true, ffmpeg: ffm,
open: func() (io.ReadCloser, error) { return nil, errors.New("precache must not re-read the source") },
@@ -642,9 +634,8 @@ var _ = Describe("Worker", func() {
Expect(pooled).To(HaveLen(len(artworkKindToResource)), "a kind is claimed by more than one pool")
})
- // A first backfill enqueues artists before albums, and artists resolve through a
- // rate-limited agent that holds its slot while waiting. Sharing one pool let ~29k
- // sleeping lookups sit in front of every album for hours.
+ // Artists resolve through a rate-limited agent that holds its slot while waiting, so one
+ // shared pool would park every album behind them.
It("resolves albums while artists are stuck on a slow agent", func() {
conf.Server.CoverArtPriority = "cover.jpg"
conf.Server.ArtistArtPriority = "external"
@@ -677,15 +668,13 @@ var _ = Describe("Worker", func() {
runCtx, cancel := context.WithCancel(ctx)
done := make(chan struct{})
go func() { defer close(done); _ = w.Run(runCtx) }()
- // Wait for Run to return: a leaked pool goroutine outlives the spec and races the
- // config snapshot Ginkgo restores on cleanup.
+ // Join Run: a leaked pool goroutine would race the config snapshot Ginkgo restores.
DeferCleanup(func() {
cancel()
close(block) // unpark the blocked lookups so the pools can unwind
<-done
})
- // The album must land while every artist is still parked in the agent.
Eventually(func() bool {
ia, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "alx", model.ImageTypePrimary)
return err == nil && ia.Hash != ""
@@ -698,8 +687,6 @@ var _ = Describe("Worker", func() {
})
Describe("batching", func() {
- // A cancelled drain leaves its undispatched rows untouched in the queue, so a later
- // drain picks them up unchanged.
It("leaves undispatched items queued when cancelled mid-batch", func() {
for i := range 8 {
id := fmt.Sprintf("alc%d", i)
@@ -720,8 +707,6 @@ var _ = Describe("Worker", func() {
}
})
- // The pool is fed from one dequeue per pass: a batch sized to the pool would make a
- // single slow item idle the other slots for as long as it runs.
It("dequeues past the worker pool so one drain covers many items", func() {
for i := range 16 {
ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{{ID: fmt.Sprintf("alb%d", i), Name: "Album"}})
diff --git a/core/artwork/worker_timing_test.go b/core/artwork/worker_timing_test.go
index 9b64fc53a..f7315b20a 100644
--- a/core/artwork/worker_timing_test.go
+++ b/core/artwork/worker_timing_test.go
@@ -41,8 +41,6 @@ func TestArtworkBreakerHalfOpen(t *testing.T) {
})
}
-// Drives the worker's per-name gate map with the fake clock: one agent's open breaker
-// must neither block another agent nor short-circuit the other's probe recovery.
func TestArtworkGatePerAgentBreakerIsolation(t *testing.T) {
synctest.Test(t, func(t *testing.T) {
g := NewWithT(t)
diff --git a/model/artwork.go b/model/artwork.go
index 204fe5ebb..0c8ce3a00 100644
--- a/model/artwork.go
+++ b/model/artwork.go
@@ -15,20 +15,17 @@ type Artwork struct {
const ImageTypePrimary = "primary"
-// ItemImage is per-entity artwork state hydrated at query time; never persisted
-// (structs:"-" keeps it out of upserts).
+// ItemImage is per-entity artwork state hydrated at query time; never persisted.
type ItemImage struct {
ImageHash string `structs:"-" json:"imageHash,omitempty"`
ImageAbsent bool `structs:"-" json:"imageAbsent,omitempty"`
BlurHash string `structs:"-" json:"blurHash,omitempty"`
- // Dimensions of the original image. A blurhash carries no aspect ratio, so a client
- // needs these to decode the placeholder into the shape the real image will occupy.
+ // A blurhash carries no aspect ratio, so clients need these to shape the placeholder.
ImageWidth int `structs:"-" json:"imageWidth,omitempty"`
ImageHeight int `structs:"-" json:"imageHeight,omitempty"`
}
-// AspectRatio is the image's width/height, or nil when there is no image or its dimensions are
-// unknown (an unresolved item). Never guesses: a wrong ratio mis-shapes a client's placeholder.
+// AspectRatio is the image's width/height, or nil when the image or its dimensions are unknown.
func (i ItemImage) AspectRatio() *float64 {
if i.ImageAbsent || i.ImageWidth <= 0 || i.ImageHeight <= 0 {
return nil
@@ -47,8 +44,7 @@ type ItemArtwork struct {
SourcePath string `structs:"source_path"`
// RefMtime is SourcePath's mtime (unix-nanoseconds) at resolution; 0 when there is no SourcePath.
RefMtime int64 `structs:"ref_mtime"`
- // attempted_at/updated_at are nullable in the schema but always set by PutItemArtwork;
- // raw inserts must set them too, since these non-pointer time.Time fields fail to scan NULL.
+ // Nullable in the schema, but every insert must set them: these non-pointer fields cannot scan NULL.
AttemptedAt time.Time `structs:"attempted_at"`
UpdatedAt time.Time `structs:"updated_at"`
}
@@ -65,8 +61,7 @@ type ItemArtworkInfo struct {
// Absent reports a known-absent artwork state (resolved, no image).
func (i ItemArtworkInfo) Absent() bool { return i.Hash == "" }
-// Image projects the hydration entry onto the entity-facing struct, so every hydration
-// site copies the same set of fields.
+// Image projects the hydration entry onto the entity-facing struct.
func (i ItemArtworkInfo) Image() ItemImage {
return ItemImage{
ImageHash: i.Hash,
@@ -96,50 +91,43 @@ const (
)
type ArtworkRepository interface {
- // Image identity (artwork table)
GetImage(hash string) (*Artwork, error)
PutImage(a *Artwork) error
GetImages(hashes []string) (map[string]Artwork, error)
// GetOrphanHashes returns hashes referenced by no item_artwork row and older than cutoff.
GetOrphanHashes(createdBefore time.Time) ([]string, error)
- // DeleteOrphans deletes the given hashes only if still unreferenced and older than cutoff (atomic re-check).
+ // DeleteOrphans deletes the given hashes only if still unreferenced and older than cutoff.
DeleteOrphans(createdBefore time.Time, hashes []string) error
- // Per-item state (item_artwork table)
GetItemArtwork(kind Kind, id, imageType string) (*ItemArtwork, error)
PutItemArtwork(ia *ItemArtwork) error
DeleteForItem(kind Kind, id string) error
- // DeleteForItems removes state rows for the given ids of one kind, in chunks.
DeleteForItems(kind Kind, ids []string) error
- // GetInfoForItems hydrates a page: one batched query, item_artwork joined to artwork.
+ // GetInfoForItems hydrates a page in one batched query.
GetInfoForItems(kind Kind, ids []string) (map[string]ItemArtworkInfo, error)
- // GetAllMimes returns hash -> current mime for every stored artwork, for sweep retention checks.
+ // GetAllMimes returns hash -> current mime for every stored artwork.
GetAllMimes() (map[string]string, error)
// PurgeDanglingItemArtwork removes state rows whose entity no longer exists.
PurgeDanglingItemArtwork() (int64, error)
}
type ArtworkQueueRepository interface {
- // Enqueue upserts; an existing row keeps the higher of the two priorities and has its
- // retry_at reset (a detected change wants immediate re-resolution).
+ // Enqueue upserts; an existing row keeps the higher priority and has its retry_at reset.
Enqueue(items ...ArtworkQueueItem) error
// EnqueueBump upserts like Enqueue but preserves an existing row's retry_at, so a
// request-triggered read-through never resets a failed resolution's backoff.
EnqueueBump(items ...ArtworkQueueItem) error
// DequeueBatch returns up to n items with retry_at <= now, priority desc, enqueued_at asc.
- // Restricted to the given item kinds when any are passed, so a drain pool sees only its own
- // work and cannot be held up behind another kind's backlog.
+ // Restricted to the given kinds when any are passed, so one kind cannot block another's drain.
DequeueBatch(n int, kinds ...string) ([]ArtworkQueueItem, error)
// MarkFailedIfUnchanged applies the failure backoff only while retry_at still matches
- // seenRetryAt; a concurrent re-enqueue (which resets retry_at) keeps its fresh eligibility.
+ // seenRetryAt, so a concurrent re-enqueue keeps its fresh eligibility.
MarkFailedIfUnchanged(kind, id, imageType string, seenRetryAt, retryAt time.Time) error
- // DeleteIfUnchanged deletes the row only if its retry_at still matches retryAt, so a
- // concurrent re-enqueue (which resets retry_at) survives instead of being erased.
+ // DeleteIfUnchanged deletes only while retry_at still matches, sparing a concurrent re-enqueue.
DeleteIfUnchanged(kind, id, imageType string, retryAt time.Time) error
Count() (int64, error)
// EnqueueStaleAbsent inserts queue rows (priority Recheck) for absent states older than cutoff.
EnqueueStaleAbsent(kind Kind, attemptedBefore time.Time) (int64, error)
- // EnqueueMissing inserts queue rows (priority Recheck) for entities of the kind that have no
- // item_artwork row at all, so a never-processed entity is eventually resolved even without a scan.
+ // EnqueueMissing inserts queue rows (priority Recheck) for entities with no item_artwork row.
EnqueueMissing(kind Kind) (int64, error)
// PurgeDangling removes queue rows whose entity no longer exists.
PurgeDangling() (int64, error)
diff --git a/model/artwork_test.go b/model/artwork_test.go
index ecb89625b..a6a3fc701 100644
--- a/model/artwork_test.go
+++ b/model/artwork_test.go
@@ -23,8 +23,7 @@ var _ = Describe("ItemImage JSON", func() {
Expect(out).To(HaveKeyWithValue("blurHash", "LEHV6nWB2yk8"))
})
- // Clients decode the blurhash into a bitmap of their choosing, so without the dimensions they
- // cannot know the placeholder's shape and default to a square.
+ // Without the dimensions, clients cannot know the placeholder's shape and default to a square.
It("exposes the image dimensions alongside the blurhash", func() {
al := model.Album{ID: "al-3", Name: "Album"}
al.BlurHash = "LEHV6nWB2yk8"
diff --git a/persistence/album_repository.go b/persistence/album_repository.go
index 75f98d5d9..66632bdac 100644
--- a/persistence/album_repository.go
+++ b/persistence/album_repository.go
@@ -202,8 +202,7 @@ func (r *albumRepository) CountAll(options ...model.QueryOptions) (int64, error)
}
func (r *albumRepository) Exists(id string) (bool, error) {
- // Filtered like CountAll: the plain exists() helper applies no library filter, so it
- // would report a row in a library the caller cannot see.
+ // The exists() helper applies no library filter, so it would report rows the caller cannot see.
c, err := r.count(r.applyLibraryFilter(r.newSelect().Where(Eq{"album.id": id})))
return c > 0, err
}
@@ -259,7 +258,6 @@ func (r *albumRepository) GetAll(options ...model.QueryOptions) (model.Albums, e
return albums, nil
}
-// hydrateArtwork fills each album's ImageHash/ImageAbsent from one batched item_artwork lookup.
func (r *albumRepository) hydrateArtwork(albums model.Albums) {
if len(albums) == 0 {
return
@@ -271,9 +269,7 @@ func (r *albumRepository) hydrateArtwork(albums model.Albums) {
}
}
-// GetAllIDs returns just the album IDs for the same row set as GetAll, skipping the
-// heavy column projection and JSON post-processing. Used by bulk enumeration (artwork backfill)
-// and as GetCursor's id pre-pass.
+// GetAllIDs returns the IDs of GetAll's row set, skipping its column projection and JSON decoding.
func (r *albumRepository) GetAllIDs(options ...model.QueryOptions) ([]string, error) {
sq := r.applyLibraryFilter(r.newSelect(options...).Columns("album.id"))
if filtersNeedAnnotation(sq) {
diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go
index 66acef072..32ee40e64 100644
--- a/persistence/album_repository_test.go
+++ b/persistence/album_repository_test.go
@@ -993,8 +993,7 @@ var _ = Describe("AlbumRepository", func() {
})
})
- // Exists used the unfiltered helper, so it reported albums in libraries the caller
- // cannot see -- the same leak Get/GetAll/CountAll already guard against.
+ // Exists must apply the same library filter as Get/GetAll/CountAll.
Describe("Exists library visibility", func() {
It("hides an album the user has no library access to", func() {
Expect(albumRepo.Put(&model.Album{ID: "vis-album", Name: "Vis", LibraryID: 1})).To(Succeed())
diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go
index 00aa75c81..c5c3a2444 100644
--- a/persistence/artist_repository.go
+++ b/persistence/artist_repository.go
@@ -266,8 +266,7 @@ func (r *artistRepository) GetAll(options ...model.QueryOptions) (model.Artists,
}
// GetAllIDs returns just the artist IDs for the same row set as GetAll, skipping the
-// heavy stats columns and JSON post-processing. Used by bulk enumeration (artwork backfill)
-// and as GetCursor's id pre-pass.
+// heavy stats columns and JSON post-processing.
func (r *artistRepository) GetAllIDs(options ...model.QueryOptions) ([]string, error) {
sq := r.applyLibraryFilterToArtistQuery(r.newSelect(options...).Columns("artist.id")).GroupBy("artist.id")
if filtersNeedAnnotation(sq) {
diff --git a/persistence/artwork_hydration.go b/persistence/artwork_hydration.go
index 8bc6d579b..0e8edce6e 100644
--- a/persistence/artwork_hydration.go
+++ b/persistence/artwork_hydration.go
@@ -12,12 +12,12 @@ import (
"github.com/pocketbase/dbx"
)
-// artworkChunkSize bounds each chunk fetch's id IN-list (under SQLite's bound-parameter limit); a
-// whole multiple of artworkBatchSize so a page re-chunks into even hydration batches.
+// Keeps the id IN-list under SQLite's bound-parameter limit; a whole multiple of artworkBatchSize
+// so a page re-chunks into even hydration batches.
const artworkChunkSize = artworkBatchSize * 3
-// streamByIDs yields the rows of ids in chunks, fetching each chunk through the caller's hydrating
-// fetch. Resolving ids first keeps OFFSET out of the joined query (spec §6).
+// streamByIDs yields rows in id chunks through the caller's hydrating fetch. Resolving ids first
+// keeps OFFSET out of the joined query.
func streamByIDs[S ~[]T, T any](ids []string, fetch func(chunk []string) (S, error)) iter.Seq2[T, error] {
return func(yield func(T, error) bool) {
for chunk := range slices.Chunk(ids, artworkChunkSize) {
@@ -36,8 +36,8 @@ func streamByIDs[S ~[]T, T any](ids []string, fetch func(chunk []string) (S, err
}
}
-// chunkOptions narrows the caller's options to a chunk of ids, dropping Max/Offset (the id pre-pass
-// already consumed them) and reusing its Sort, which may be a seeded random expression by now.
+// chunkOptions narrows options to a chunk of ids, dropping Max/Offset (the id pre-pass already
+// consumed them) and reusing Sort, which may be a seeded random expression by now.
func chunkOptions(options []model.QueryOptions, idField string) func([]string) model.QueryOptions {
var base model.QueryOptions
if len(options) > 0 {
@@ -53,8 +53,8 @@ func chunkOptions(options []model.QueryOptions, idField string) func([]string) m
}
}
-// hydrateItemImages returns per-item artwork info for a fetched page via one batched query per kind
-// (never a join, see spec §6). On error it logs and returns an empty map so the page still renders.
+// hydrateItemImages returns per-item artwork info in one batched query per kind. On error it logs
+// and returns an empty map, so the page still renders.
func hydrateItemImages(ctx context.Context, db dbx.Builder, kind model.Kind, ids []string) map[string]model.ItemArtworkInfo {
if len(ids) == 0 {
return map[string]model.ItemArtworkInfo{}
@@ -75,7 +75,7 @@ func applyItemImage(infos map[string]model.ItemArtworkInfo, id string, img *mode
}
// hydrateMediaFileArtwork mirrors MediaFile.CoverArtID: an embedded-eligible file with resolved own
-// art uses it, else it falls back to the album's. Two batched item_artwork lookups, never a join.
+// art uses it, else it falls back to the album's.
func hydrateMediaFileArtwork(ctx context.Context, db dbx.Builder, mfs model.MediaFiles) {
if len(mfs) == 0 {
return
@@ -96,26 +96,20 @@ func hydrateMediaFileArtwork(ctx context.Context, db dbx.Builder, mfs model.Medi
eligible := mf.HasCoverArt && conf.Server.EnableMediaFileCoverArt
ownInfo, ownResolved := mfInfos[mf.ID]
if eligible && ownResolved && !ownInfo.Absent() {
- mf.ItemImage = ownInfo.Image() // own resolved art wins
+ mf.ItemImage = ownInfo.Image()
continue
}
ownWontResolve := !eligible || (ownResolved && ownInfo.Absent())
- // Fallback (see MediaFile.CoverArtID): inherit a found album hash for optimistic caching,
- // but only when the album's bytes are what serving will actually return. A multi-disc track
- // emits a dc- id served from disc art, and an eligible-but-unresolved track still extracts
- // its own embedded image — stamping the album hash on either advertises a content-version
- // (and a blurhash) belonging to a different image.
+ // Inherit the album hash only when serving returns those exact bytes: a multi-disc track is
+ // served disc art, and an eligible-but-unresolved one still extracts its own embedded image.
if album, ok := albumInfos[mf.AlbumID]; ok && !album.Absent() {
if mf.DiscNumber == 0 && ownWontResolve {
mf.ItemImage = album.Image()
}
continue
}
- // Nothing found. Mark absent only when serving would definitively yield a placeholder:
- // a single-disc track whose album is known-absent and whose own art won't resolve. A
- // multi-disc track resolves disc art provisionally (never known-absent), and an
- // eligible-but-unresolved track can still extract its own embedded art — both stay
- // requestable.
+ // Mark absent only when serving would definitively yield a placeholder; disc art and
+ // still-extractable embedded art both keep a track requestable.
if mf.DiscNumber > 0 {
continue
}
@@ -125,8 +119,7 @@ func hydrateMediaFileArtwork(ctx context.Context, db dbx.Builder, mfs model.Medi
}
}
-// hydrateCursor buffers a streamed page into batches and hydrates each before yielding, so a
-// cursor carries the same artwork state as a fetched page without a per-row query.
+// hydrateCursor hydrates a streamed cursor in batches, avoiding a per-row query.
func hydrateCursor[T any](cursor iter.Seq2[T, error], hydrate func([]T)) iter.Seq2[T, error] {
return func(yield func(T, error) bool) {
buf := make([]T, 0, artworkBatchSize)
@@ -157,8 +150,7 @@ func hydrateCursor[T any](cursor iter.Seq2[T, error], hydrate func([]T)) iter.Se
}
}
-// hydratePlaylistTrackArtwork hydrates the MediaFile embedded in each playlist track, so a track
-// reached through a playlist carries the same artwork state as one reached through the songs list.
+// hydratePlaylistTrackArtwork hydrates the MediaFile embedded in each playlist track.
func hydratePlaylistTrackArtwork(ctx context.Context, db dbx.Builder, tracks model.PlaylistTracks) {
if len(tracks) == 0 {
return
diff --git a/persistence/artwork_hydration_test.go b/persistence/artwork_hydration_test.go
index 28678700b..adf0e7e53 100644
--- a/persistence/artwork_hydration_test.go
+++ b/persistence/artwork_hydration_test.go
@@ -18,8 +18,7 @@ import (
"github.com/pocketbase/dbx"
)
-// seedAnnotations gives each id a distinct rating, play count, play date and starred_at for the
-// current user, dropping the rows again when the spec ends. It writes them directly, so that
+// seedAnnotations gives each id distinct annotation values, writing them directly so that
// SetRating's average_rating update does not outlive the cleanup.
func seedAnnotations(itemType string, ids ...string) {
GinkgoHelper()
@@ -103,7 +102,6 @@ var _ = Describe("Artwork hydration", func() {
al.ImageAbsent = true
Expect(repo.(*albumRepository).Put(&al)).To(Succeed())
- // No item_artwork rows exist, so a fresh read must observe zero values.
got, err := repo.Get(al.ID)
Expect(err).ToNot(HaveOccurred())
Expect(got.ImageHash).To(BeEmpty())
@@ -179,8 +177,6 @@ var _ = Describe("Artwork hydration", func() {
Expect(got.ImageHash).To(Equal("plget8888888888"))
})
- // A track reached through a playlist must carry the same artwork state as one reached
- // through the songs list, or its cover id has no hash to serve immutably or blur.
It("hydrates the tracks reached through a playlist", func() {
Expect(aw.PutImage(&model.Artwork{Hash: "pltrackhash1234", Mime: "image/jpeg", BlurHash: "LPLBLURhash"})).To(Succeed())
putInfo("al", songDayInALife.AlbumID, "pltrackhash1234")
@@ -279,16 +275,13 @@ var _ = Describe("Artwork hydration", func() {
byID := getByID()
- // eligible + own hash -> own hash
Expect(byID["1001"].ImageHash).To(Equal("mfh1001xxxxxxxx"))
Expect(byID["1001"].ImageAbsent).To(BeFalse())
- // eligible + embedded absent -> falls through to album 102 info
Expect(byID["1002"].ImageHash).To(Equal("alh102xxxxxxxxxx"))
Expect(byID["1002"].ImageAbsent).To(BeFalse())
- // not eligible (no embedded cover) -> album 103 info (known-absent)
Expect(byID["1003"].ImageHash).To(BeEmpty())
Expect(byID["1003"].ImageAbsent).To(BeTrue())
- // not eligible, album has no row -> zero values (unresolved)
+ // 2002: not eligible, and its album has no row at all -> unresolved
Expect(byID["2002"].ImageHash).To(BeEmpty())
Expect(byID["2002"].ImageAbsent).To(BeFalse())
})
@@ -304,15 +297,12 @@ var _ = Describe("Artwork hydration", func() {
byID := getByID()
- // own-art-wins: ImageHash is the track's own, but AlbumImage still carries the album's.
Expect(byID["1001"].ImageHash).To(Equal("mfh1001albimgxxx"))
Expect(byID["1001"].AlbumImage.ImageHash).To(Equal("alh101albimgxxxx"))
- // single-disc album-inheritance: not eligible, so ImageHash mirrors the album's hash.
Expect(byID["1002"].ImageHash).To(Equal("alh102albimgxxxx"))
Expect(byID["1002"].AlbumImage.ImageHash).To(Equal("alh102albimgxxxx"))
- // multi-disc bailout: ImageHash stays bare, but AlbumImage still reflects the absence.
Expect(byID["2002"].ImageHash).To(BeEmpty())
Expect(byID["2002"].ImageAbsent).To(BeFalse())
Expect(byID["2002"].AlbumImage.ImageAbsent).To(BeTrue())
@@ -341,18 +331,16 @@ var _ = Describe("Artwork hydration", func() {
})
It("keeps an eligible file optimistic when its own art is unresolved, even if the album is absent", func() {
- // 1004 is eligible (has embedded cover) with no mf state row yet; its album (103) is absent.
setCover("1004", true)
DeferCleanup(func() { setCover("1004", false) })
putInfo("al", "103", "") // album known-absent
byID := getByID()
- // The track's own embedded art is still unresolved, so it must NOT inherit the album's
- // absence: coverArt stays requestable so serving can extract the embedded art.
+ // 1004's own embedded art is still unresolved, so coverArt must stay requestable.
Expect(byID["1004"].ImageAbsent).To(BeFalse())
Expect(byID["1004"].ImageHash).To(BeEmpty())
- // A non-eligible sibling on the same absent album still inherits the absence.
+ // 1003 is not eligible, so it still inherits the album's absence.
Expect(byID["1003"].ImageAbsent).To(BeTrue())
})
@@ -361,8 +349,6 @@ var _ = Describe("Artwork hydration", func() {
byID := getByID()
- // 2002 is multi-disc (DiscNumber>0); CoverArtID emits a dc- id served from disc art of
- // unknown identity, so it must not advertise the album's hash as its content-version.
Expect(byID["2002"].ImageHash).To(BeEmpty())
Expect(byID["2002"].ImageAbsent).To(BeFalse())
})
@@ -372,14 +358,12 @@ var _ = Describe("Artwork hydration", func() {
byID := getByID()
- // 2002 is a multi-disc track (DiscNumber>0); CoverArtID points at disc art, which
- // resolves provisionally, so it must never inherit the album's absence.
Expect(byID["2002"].ImageAbsent).To(BeFalse())
Expect(byID["2002"].ImageHash).To(BeEmpty())
})
- // serveMediaFile extracts this track's own embedded art (provisionalEmbedded), so the
- // album's hash would advertise a content-version for bytes nobody will serve.
+ // serveMediaFile serves this track's own embedded art, so the album's hash would
+ // advertise a content-version for bytes nobody will serve.
It("leaves the hash bare for an eligible file whose own art is unresolved", func() {
setCover("1004", true)
DeferCleanup(func() { setCover("1004", false) })
@@ -438,16 +422,15 @@ var _ = Describe("Artwork hydration", func() {
albumRadioactivity.ID, albumMultiDisc.ID, albumCJK.ID, albumPunctuation.ID}}
onlyArtists = squirrel.Eq{"artist.id": []string{artistKraftwerk.ID, artistBeatles.ID,
artistCJK.ID, artistPunctuation.ID}}
- // Both fixture playlists belong to the same owner, so the owner_name sort would have a
- // single value to order by. This one is also private and owned by a third user, which
- // is what the non-admin visibility spec needs to see hidden.
+ // Both fixture playlists share an owner, leaving the owner_name sort a single value to
+ // order by; this one is also private, which the non-admin visibility spec needs.
foreign := model.Playlist{Name: "Foreign", OwnerID: thirdUser.ID, OwnerName: thirdUser.UserName}
Expect(playlistRepo.Put(&foreign)).To(Succeed())
DeferCleanup(func() { Expect(playlistRepo.Delete(foreign.ID)).To(Succeed()) })
onlyPlaylists = squirrel.Eq{"playlist.id": []string{plsBest.ID, plsCool.ID, foreign.ID}}
- // The suite annotates a single album and artist, so the annotation-backed sorts would
- // have nothing to order. Seed distinct values, and drop them again after each spec.
+ // The suite annotates a single album and artist, leaving the annotation-backed sorts
+ // nothing to order.
seedAnnotations("album", albumSgtPeppers.ID, albumAbbeyRoad.ID)
seedAnnotations("artist", artistKraftwerk.ID, artistCJK.ID)
@@ -515,8 +498,7 @@ var _ = Describe("Artwork hydration", func() {
To(Equal(slice.Map(want, func(a model.Album) string { return a.ID })))
})
- // The sorts and filters the Jellyfin list endpoints issue: the id pre-pass must resolve each
- // of them exactly like the full query. Comparing sort keys, not ids, keeps ties out of it.
+ // The sorts the Jellyfin list endpoints issue; comparing sort keys, not ids, keeps ties out.
DescribeTable("orders albums like GetAll",
func(opts model.QueryOptions, key func(model.Album) string) {
opts = scoped(opts, onlyAlbums)
@@ -612,8 +594,7 @@ var _ = Describe("Artwork hydration", func() {
repo := NewPlaylistRepository(otherCtx, GetDBXBuilder())
opts := model.QueryOptions{Sort: "name", Filters: onlyPlaylists}
- // Both phases must filter on their own: the id pre-pass, and the chunk fetch that would
- // otherwise re-read an id whose visibility changed in between.
+ // Both phases must filter on their own: the id pre-pass and the chunk fetch.
Expect(repo.GetAllIDs(opts)).To(ConsistOf(plsBest.ID))
all, err := repo.GetAll(model.QueryOptions{Filters: onlyPlaylists})
Expect(err).ToNot(HaveOccurred())
@@ -633,15 +614,13 @@ var _ = Describe("Artwork hydration", func() {
BeforeEach(func() {
mfRepo = NewMediaFileRepository(ctx, GetDBXBuilder())
putInfo("al", albumSgtPeppers.ID, "curhash11111111")
- // Distinct titles keep the ordering/paging specs tie-free; other fixture songs share
- // titles (e.g. "Antenna") or albums, which would make row order ambiguous.
+ // Distinct titles only: other fixture songs share titles (e.g. "Antenna" x3), which
+ // would make the positional comparisons against GetAll pass by tie-order coincidence.
onlySongs = squirrel.Eq{"media_file.id": []string{songDayInALife.ID, songComeTogether.ID,
songRadioactivity.ID, songAntenna.ID, songDisc1Track01.ID, songCJK.ID, songPunctuation.ID}}
})
It("hydrates artwork onto every streamed track, unlike GetCursor", func() {
- // Scoped to onlySongs (distinct titles): the full fixture has title ties (e.g. "Antenna"
- // x3), so positional comparison against GetAll would only pass by tie-order coincidence.
opts := model.QueryOptions{Sort: "title", Filters: onlySongs}
want, err := mfRepo.GetAll(opts)
Expect(err).ToNot(HaveOccurred())
diff --git a/persistence/artwork_queue_repository.go b/persistence/artwork_queue_repository.go
index e3e1f735b..5da7f343c 100644
--- a/persistence/artwork_queue_repository.go
+++ b/persistence/artwork_queue_repository.go
@@ -11,7 +11,7 @@ import (
"github.com/pocketbase/dbx"
)
-// enqueueChunkSize keeps each multi-row insert under SQLite's bind-variable limit (7 cols -> 700 vars).
+// Keeps each multi-row insert under SQLite's bind-variable limit (7 cols -> 700 vars).
const enqueueChunkSize = 100
type artworkQueueRepository struct {
@@ -26,16 +26,13 @@ func NewArtworkQueueRepository(ctx context.Context, db dbx.Builder) model.Artwor
return r
}
-// Enqueue also restarts the retry budget the worker measures from enqueued_at, so a fresh
-// request never inherits an old row's spent window and give up on its first attempt.
+// Enqueue also resets enqueued_at, so a fresh request does not inherit an old row's spent retry budget.
func (r *artworkQueueRepository) Enqueue(items ...model.ArtworkQueueItem) error {
return r.enqueue(`ON CONFLICT (item_kind, item_id, image_type) DO UPDATE SET
priority = MAX(priority, excluded.priority), retry_at = excluded.retry_at,
attempts = 0, enqueued_at = excluded.enqueued_at`, items)
}
-// EnqueueBump raises priority like Enqueue but leaves an existing row's retry_at intact, so a
-// request-triggered read-through never resets a failed resolution's backoff. New rows insert eligible.
func (r *artworkQueueRepository) EnqueueBump(items ...model.ArtworkQueueItem) error {
return r.enqueue(`ON CONFLICT (item_kind, item_id, image_type) DO UPDATE SET
priority = MAX(priority, excluded.priority)`, items)
@@ -72,8 +69,6 @@ func (r *artworkQueueRepository) DequeueBatch(n int, kinds ...string) ([]model.A
return res, err
}
-// MarkFailedIfUnchanged applies the backoff only while retry_at still equals seenRetryAt;
-// a concurrent Enqueue resets retry_at, so its fresh eligibility survives untouched.
func (r *artworkQueueRepository) MarkFailedIfUnchanged(kind, id, imageType string, seenRetryAt, retryAt time.Time) error {
upd := Update(r.tableName).
Set("attempts", Expr("attempts + 1")).
@@ -83,13 +78,10 @@ func (r *artworkQueueRepository) MarkFailedIfUnchanged(kind, id, imageType strin
return err
}
-// DeleteIfUnchanged deletes the row only while its retry_at still equals the dequeued
-// value; a concurrent Enqueue resets retry_at, so the row survives to be re-resolved.
func (r *artworkQueueRepository) DeleteIfUnchanged(kind, id, imageType string, retryAt time.Time) error {
return r.delete(Eq{"item_kind": kind, "item_id": id, "image_type": imageType, "retry_at": retryAt})
}
-// PurgeDangling removes queue rows whose entity no longer exists, per kind.
func (r *artworkQueueRepository) PurgeDangling() (int64, error) {
return purgeDangling(r.executeSQL, r.tableName)
}
diff --git a/persistence/artwork_queue_repository_test.go b/persistence/artwork_queue_repository_test.go
index a2843bf14..5baa482b6 100644
--- a/persistence/artwork_queue_repository_test.go
+++ b/persistence/artwork_queue_repository_test.go
@@ -19,8 +19,8 @@ var _ = Describe("ArtworkQueueRepository", func() {
ImageType: model.ImageTypePrimary, Priority: prio}
}
- // backOff puts a row into the state a failed resolution leaves behind. Production only ever
- // reaches that state through MarkFailedIfUnchanged, which needs the retry_at it dequeued.
+ // Writes the post-failure state directly: the production path, MarkFailedIfUnchanged, needs
+ // a retry_at only a dequeue can hand it.
backOff := func(kind, id string, retryAt time.Time) {
GinkgoHelper()
r := repo.(*artworkQueueRepository)
@@ -63,11 +63,9 @@ var _ = Describe("ArtworkQueueRepository", func() {
It("EnqueueBump raises priority without resetting a backing-off row's retry_at", func() {
Expect(repo.Enqueue(item("al", "b1", model.ArtworkPriorityScan))).To(Succeed())
- // Push retry_at into the future so the row is backing off and hidden from dequeue.
backOff("al", "b1", time.Now().Add(time.Hour))
Expect(repo.DequeueBatch(10)).To(BeEmpty())
- // A request-triggered bump raises priority but must leave the backoff intact.
Expect(repo.EnqueueBump(item("al", "b1", model.ArtworkPriorityBump))).To(Succeed())
Expect(repo.DequeueBatch(10)).To(BeEmpty(), "bump must not reset retry_at")
@@ -101,7 +99,7 @@ var _ = Describe("ArtworkQueueRepository", func() {
It("MarkFailedIfUnchanged applies backoff only while retry_at is unchanged", func() {
Expect(repo.Enqueue(item("al", "m1", model.ArtworkPriorityScan))).To(Succeed())
- // Anchor retry_at in the past (attempts -> 1) so it can never collide with the re-enqueue's now.
+ // Anchor retry_at in the past so it can never collide with the re-enqueue's now.
backOff("al", "m1", time.Now().Add(-time.Hour))
got, err := repo.DequeueBatch(10)
Expect(err).ToNot(HaveOccurred())
@@ -111,7 +109,6 @@ var _ = Describe("ArtworkQueueRepository", func() {
// A concurrent scan re-enqueues, resetting retry_at to now.
Expect(repo.Enqueue(item("al", "m1", model.ArtworkPriorityScan))).To(Succeed())
- // Failing with the stale retry_at is a no-op: the re-enqueued row keeps its fresh state.
future := time.Now().Add(48 * time.Hour)
Expect(repo.MarkFailedIfUnchanged("al", "m1", model.ImageTypePrimary, original, future)).To(Succeed())
got, _ = repo.DequeueBatch(10)
@@ -119,7 +116,6 @@ var _ = Describe("ArtworkQueueRepository", func() {
Expect(got[0].Attempts).To(BeZero(), "re-enqueue clears attempts, and the stale failure must not bump them")
current := got[0].RetryAt
- // Failing with the current retry_at applies the backoff and bumps attempts.
Expect(repo.MarkFailedIfUnchanged("al", "m1", model.ImageTypePrimary, current, future)).To(Succeed())
got, _ = repo.DequeueBatch(10)
Expect(got).To(BeEmpty(), "backed-off row is hidden until the future retry_at")
@@ -171,7 +167,6 @@ var _ = Describe("ArtworkQueueRepository", func() {
n, _ := repo.Count()
Expect(n).To(Equal(int64(1)))
- // Deleting with the current retry_at removes it.
got, _ = repo.DequeueBatch(10)
Expect(got).To(HaveLen(1))
Expect(repo.DeleteIfUnchanged("al", "d1", model.ImageTypePrimary, got[0].RetryAt)).To(Succeed())
@@ -225,8 +220,6 @@ var _ = Describe("ArtworkQueueRepository", func() {
It("enqueues entities that have no item_artwork row at all", func() {
awRepo := NewArtworkRepository(context.Background(), GetDBXBuilder())
- // albumSgtPeppers has a resolved row and albumAbbeyRoad an absent row: both already
- // processed, so neither should be enqueued as "missing".
Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: albumSgtPeppers.ID, ImageType: model.ImageTypePrimary, Hash: "hX", AttemptedAt: time.Now()})).To(Succeed())
Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: albumAbbeyRoad.ID, ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: time.Now()})).To(Succeed())
diff --git a/persistence/artwork_repository_test.go b/persistence/artwork_repository_test.go
index 19b7219fe..3910a5575 100644
--- a/persistence/artwork_repository_test.go
+++ b/persistence/artwork_repository_test.go
@@ -11,7 +11,6 @@ import (
"github.com/pocketbase/dbx"
)
-// clearArtworkTables resets the shared test DB's artwork tables so specs don't leak state.
func clearArtworkTables() {
db := GetDBXBuilder()
for _, t := range []string{"artwork_queue", "item_artwork", "artwork"} {
diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go
index 6b47b5228..df1472c2a 100644
--- a/persistence/mediafile_repository.go
+++ b/persistence/mediafile_repository.go
@@ -163,8 +163,7 @@ func (r *mediaFileRepository) CountBySuffix(options ...model.QueryOptions) (map[
}
func (r *mediaFileRepository) Exists(id string) (bool, error) {
- // Filtered like CountAll: the plain exists() helper applies no library filter, so it
- // would report a row in a library the caller cannot see.
+ // The exists() helper applies no library filter, so it would report rows the caller cannot see.
c, err := r.count(r.applyLibraryFilter(r.newSelect().Where(Eq{"media_file.id": id})))
return c > 0, err
}
@@ -226,7 +225,6 @@ func (r *mediaFileRepository) GetAll(options ...model.QueryOptions) (model.Media
return mfs, nil
}
-// hydrateArtwork hydrates a fetched page in place.
func (r *mediaFileRepository) hydrateArtwork(mfs model.MediaFiles) {
hydrateMediaFileArtwork(r.ctx, r.db, mfs)
}
@@ -301,8 +299,7 @@ func (r *mediaFileRepository) GetCursor(options ...model.QueryOptions) (model.Me
return wrapMediaFileCursor(cursor), nil
}
-// GetAllIDs returns just the media_file IDs for the same row set as GetAll, skipping the heavy
-// column projection. Used as GetCursorWithArtwork's id pre-pass.
+// GetAllIDs returns the IDs of GetAll's row set, skipping its wide column projection.
func (r *mediaFileRepository) GetAllIDs(options ...model.QueryOptions) ([]string, error) {
sq := r.applyLibraryFilter(r.newSelect(options...).Columns("media_file.id"))
if filtersNeedAnnotation(sq) {
@@ -313,8 +310,7 @@ func (r *mediaFileRepository) GetAllIDs(options ...model.QueryOptions) ([]string
return ids, err
}
-// GetCursorWithArtwork streams the same rows as GetCursor, hydrated, via the id pre-pass used by
-// the other cursors, rather than the scanner's bounded, unhydrated GetCursor itself.
+// GetCursorWithArtwork streams the same rows as GetCursor, hydrated, via an id pre-pass.
func (r *mediaFileRepository) GetCursorWithArtwork(options ...model.QueryOptions) (model.MediaFileCursor, error) {
ids, err := r.GetAllIDs(options...)
if err != nil {
diff --git a/persistence/mediafile_repository_test.go b/persistence/mediafile_repository_test.go
index d0e7071cf..b9cba03ff 100644
--- a/persistence/mediafile_repository_test.go
+++ b/persistence/mediafile_repository_test.go
@@ -1119,8 +1119,7 @@ var _ = Describe("MediaRepository", func() {
})
})
- // Exists used the unfiltered helper, so it reported tracks in libraries the caller
- // cannot see -- the same leak Get/GetAll/CountAll already guard against.
+ // Exists must apply the same library filter as Get/GetAll/CountAll.
Describe("Exists library visibility", func() {
It("hides a track the user has no library access to", func() {
restricted := model.User{ID: "restricted_mf_user", UserName: "rm", Name: "RM", Email: "rm@t.com"}
diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go
index a40635e21..1ae1ce43e 100644
--- a/persistence/playlist_repository.go
+++ b/persistence/playlist_repository.go
@@ -134,8 +134,8 @@ func (r *playlistRepository) Put(p *model.Playlist, cols ...string) error {
}
pls.ID = id // r.put assigns the generated id to p, not to this copy
if isNew {
- // A brand-new playlist has art to find even with no tracks (an imported m3u can carry an
- // ExternalImageURL). An update landing here changed only metadata, so leave its cover be.
+ // Even a trackless new playlist has art to find (an imported m3u can carry an
+ // ExternalImageURL); an update landing here changed only metadata, so leave its cover be.
r.enqueueCoverRebuild(id)
}
return r.refreshCounters(&pls.Playlist)
@@ -184,7 +184,6 @@ func (r *playlistRepository) findBy(sql Sqlizer) (*model.Playlist, error) {
return &list[0], nil
}
-// hydrateArtwork fills each playlist's ImageHash/ImageAbsent from one batched item_artwork lookup.
func (r *playlistRepository) hydrateArtwork(playlists model.Playlists) {
if len(playlists) == 0 {
return
@@ -211,12 +210,10 @@ func (r *playlistRepository) GetAll(options ...model.QueryOptions) (model.Playli
return playlists, err
}
-// GetAllIDs returns just the playlist IDs for the same row set as GetAll (honoring userFilter),
-// skipping its per-row processing. Used by bulk enumeration (artwork backfill) and as GetCursor's
-// id pre-pass.
+// GetAllIDs returns the IDs of GetAll's row set, skipping its per-row processing.
func (r *playlistRepository) GetAllIDs(options ...model.QueryOptions) ([]string, error) {
- // Joins a projection of user, not the table: its name/created_at columns would otherwise make
- // an ORDER BY on the playlist's own ambiguous.
+ // Joins a projection of user, not the table: its name/created_at columns would make an ORDER BY
+ // on the playlist's own ambiguous.
sq := r.newSelect(options...).Columns("playlist.id", "user.user_name as owner_name").
Join("(select id, user_name from user) user on user.id = owner_id").Where(r.userFilter())
if filtersNeedAnnotation(sq) {
@@ -228,8 +225,7 @@ func (r *playlistRepository) GetAllIDs(options ...model.QueryOptions) ([]string,
}
func (r *playlistRepository) GetCursor(options ...model.QueryOptions) (model.PlaylistCursor, error) {
- // GetAllIDs and GetAll both apply userFilter: a cursor must not widen visibility beyond
- // public/owned playlists, even if visibility changes between the two queries.
+ // Both passes apply userFilter, so a visibility change between them cannot widen the cursor.
ids, err := r.GetAllIDs(options...)
if err != nil {
return nil, err
@@ -338,10 +334,8 @@ func (r *playlistRepository) refreshCounters(pls *model.Playlist) error {
return nil
}
-// enqueueCoverRebuild re-resolves the generated 2x2 grid, which depends on the track set. Called
-// only where that set actually changes: the grid samples albums at random, so rebuilding after a
-// mere rename would hand the playlist a different cover. No clear -- the old cover keeps serving
-// until the worker rebuilds, so there is no flicker.
+// enqueueCoverRebuild re-resolves the generated 2x2 grid. Call it only when the track set changes:
+// the grid samples albums at random, so rebuilding after a mere rename would change the cover.
func (r *playlistRepository) enqueueCoverRebuild(id string) {
item := model.ArtworkQueueItem{ItemKind: model.KindPlaylistArtwork.Prefix(), ItemID: id,
ImageType: model.ImageTypePrimary, Priority: model.ArtworkPriorityScan}
diff --git a/persistence/playlist_repository_test.go b/persistence/playlist_repository_test.go
index a60c8281a..f900e65ef 100644
--- a/persistence/playlist_repository_test.go
+++ b/persistence/playlist_repository_test.go
@@ -273,8 +273,7 @@ var _ = Describe("PlaylistRepository", func() {
Expect(queued).ToNot(ContainElement(HaveField("ItemID", "")), "must not enqueue an empty playlist id")
})
- // The grid samples albums at random, so re-resolving after a rename would silently hand the
- // playlist a different cover.
+ // The grid samples albums at random, so re-resolving after a rename would change the cover.
It("does not enqueue artwork when only metadata changes", func() {
ctx := request.WithUser(log.NewContext(GinkgoT().Context()), model.User{ID: "userid", UserName: "userid", IsAdmin: true})
newPls := model.Playlist{Name: "Rename Me", OwnerID: "userid"}
@@ -411,9 +410,8 @@ var _ = Describe("PlaylistRepository", func() {
})
})
- // Exists is ctx-sensitive through userFilter: a private playlist is invisible to anyone but
- // its owner or an admin. Callers that only want "does it still exist" -- the public image
- // route serving a share -- must elevate, or a shared private playlist looks deleted.
+ // Exists is ctx-sensitive through userFilter, so callers that only want "does it still exist"
+ // -- the public image route serving a share -- must elevate, or a private playlist looks gone.
Describe("Exists visibility", func() {
It("hides a private playlist from an unauthenticated context", func() {
// "userid" is the fixture user; playlist.owner_id has a FK to user(id).
diff --git a/server/imghttp/headers.go b/server/imghttp/headers.go
index 8f017d60a..0277132fb 100644
--- a/server/imghttp/headers.go
+++ b/server/imghttp/headers.go
@@ -1,6 +1,5 @@
-// Package imghttp holds the shared HTTP caching contract for artwork responses, so the
-// subsonic, public, and jellyfin image handlers apply identical headers without importing
-// each other.
+// Package imghttp holds the HTTP caching contract shared by the subsonic, public, and jellyfin
+// image handlers, so they apply identical headers without importing each other.
package imghttp
import (
@@ -11,33 +10,30 @@ import (
)
// WriteImageHeaders applies the artwork caching contract and reports whether a 304 was written
-// (in which case the caller must not write a body). requestedHash is the hash the client asserted
-// (id suffix / JWT payload / jellyfin tag param), or "" when the request carried no hash.
+// (the caller must then not write a body). requestedHash is the hash the client asserted, or "".
func WriteImageHeaders(w http.ResponseWriter, r *http.Request, img *artwork.Image, requestedHash string) (wrote304 bool) {
h := w.Header()
- // Placeholders are transient stand-ins for not-yet-resolved art: never cached, no validators.
+ // Placeholders are transient stand-ins for unresolved art: never cached, no validators.
if img.Placeholder {
h.Set("Cache-Control", "no-store")
return false
}
- // The validator identifies the served representation (resized/re-encoded bytes version it via
- // ETag), so a CoverArtQuality/EnableWebPEncoding change invalidates a revalidating client's
- // cache. Falls back to the pixel hash for full-size originals (bytes == the hash).
+ // The ETag versions the served representation, so a re-encoding config change invalidates a
+ // revalidating client's cache; full-size originals are their own pixel hash.
etag := img.ETag
if etag == "" {
etag = img.Hash
}
- // An empty validator is not one: emitting it would hand every such response the same ETag,
- // and matching it would 304 a client that echoed it back even after the bytes changed.
+ // An empty validator would be shared by every such response, 304ing clients that echo it back.
if etag != "" {
h.Set("ETag", `"`+etag+`"`)
}
if !img.LastUpdated.IsZero() {
h.Set("Last-Modified", img.LastUpdated.UTC().Format(http.TimeFormat))
}
- // Immutable only when the client asked for the exact current pixel hash; bare/legacy/mismatched
- // requests get cheap ETag revalidation instead, which fixes stale art after re-resolution.
+ // Immutable only when the client asked for the current pixel hash; anything else revalidates,
+ // so art that gets re-resolved is never pinned.
if requestedHash != "" && requestedHash == img.Hash {
h.Set("Cache-Control", "public, max-age=31536000, immutable")
} else {
@@ -51,8 +47,7 @@ func WriteImageHeaders(w http.ResponseWriter, r *http.Request, img *artwork.Imag
return false
}
-// ifNoneMatch reports whether the If-None-Match header asserts the given hash, using weak
-// comparison (RFC 9110): "*" matches any current representation and W/ prefixes are ignored.
+// ifNoneMatch reports whether If-None-Match asserts hash, using RFC 9110 weak comparison.
func ifNoneMatch(header, hash string) bool {
header = strings.TrimSpace(header)
if header == "" {
diff --git a/server/imghttp/headers_test.go b/server/imghttp/headers_test.go
index 80baae47d..2e8313faf 100644
--- a/server/imghttp/headers_test.go
+++ b/server/imghttp/headers_test.go
@@ -30,8 +30,8 @@ func placeholder() *artwork.Image {
return &artwork.Image{ReadCloser: io.NopCloser(strings.NewReader("PH")), Placeholder: true}
}
-// resized carries a representation ETag distinct from the pixel hash (as a resized/re-encoded
-// response does), so the validator versions with the encode settings.
+// A re-encoded response carries a representation ETag distinct from the pixel hash, so the
+// validator versions with the encode settings.
func resized() *artwork.Image {
return &artwork.Image{
ReadCloser: io.NopCloser(strings.NewReader("IMG")),
@@ -41,7 +41,6 @@ func resized() *artwork.Image {
}
}
-// unvalidated stands for a response with no content hash and no representation tag.
func unvalidated() *artwork.Image {
return &artwork.Image{ReadCloser: io.NopCloser(strings.NewReader("IMG")), LastUpdated: lastMod}
}
@@ -107,8 +106,8 @@ var _ = Describe("WriteImageHeaders", func() {
Entry("placeholder ignores If-None-Match and never 304s",
testCase{img: placeholder(), ifNoneMatch: "*", want304: false, wantCache: "no-store"}),
- // An image with neither ETag nor Hash has no validator. Emitting one would give every
- // such response the same empty tag, and matching it would 304 changed bytes.
+ // With no validator, an emitted ETag would be the same empty tag on every such response,
+ // and matching it would 304 changed bytes.
Entry("omits the ETag entirely when there is no validator",
testCase{img: unvalidated(), wantCache: "public, no-cache", wantLastMod: true}),
Entry("never 304s an empty validator echoed back by the client",
diff --git a/server/jellyfin/dto/mappers.go b/server/jellyfin/dto/mappers.go
index 974a50bd0..352fce11d 100644
--- a/server/jellyfin/dto/mappers.go
+++ b/server/jellyfin/dto/mappers.go
@@ -193,17 +193,15 @@ func SongToBaseItem(mf model.MediaFile, fields Fields) BaseItemDto {
} else if mf.Genre != "" {
item.Genres = []string{mf.Genre}
}
- // A track's own cover wins in Finamp's precedence (ImageTags.Primary before AlbumId).
+ // Clients prefer ImageTags.Primary over AlbumId, so a track's own cover must win here.
if mf.ImageHash != "" && mf.ImageHash != mf.AlbumImage.ImageHash {
tag, blurs, ratio := primaryImage(mf.ItemImage, mf.ID, fields)
item.ImageTags = map[string]string{"Primary": tag}
item.ImageBlurHashes = blurs
item.PrimaryImageAspectRatio = ratio
} else if embeddedArtPending(mf) {
- // Nothing enqueues media files, so an unresolved track only resolves when someone asks
- // for its image. Advertising the id here is what makes a Jellyfin client ask; the
- // request extracts the embedded art and queues the track for the worker. No blurhash:
- // there is no resolved image to have one yet, and a fake would be cached forever.
+ // Nothing enqueues media files: advertising the id is what makes a client ask, and that
+ // request is what extracts the embedded art and queues the track.
item.ImageTags = map[string]string{"Primary": mf.ID}
} else if mf.AlbumID != "" {
if tag, blurs, ratio := primaryImage(mf.AlbumImage, mf.AlbumID, fields); tag != "" {
@@ -215,15 +213,13 @@ func SongToBaseItem(mf model.MediaFile, fields Fields) BaseItemDto {
return item
}
-// embeddedArtPending reports a track whose own art is eligible but not resolved yet, and not
-// known to be absent.
func embeddedArtPending(mf model.MediaFile) bool {
return mf.HasCoverArt && conf.Server.EnableMediaFileCoverArt &&
mf.ImageHash == "" && !mf.ItemImage.ImageAbsent
}
-// primaryImage derives all a mapper advertises about one image, so Primary is chosen once. It never
-// fakes a blurhash: Finamp keys its cover cache on the value, pinning a stale cover forever (#5798).
+// primaryImage never fakes a blurhash: clients key their cover cache on the value, which would
+// pin a stale cover forever.
func primaryImage(img model.ItemImage, fallback string, fields Fields) (tag string, blurs map[string]map[string]string, ratio *float64) {
if img.ImageAbsent {
return "", nil, nil
diff --git a/server/jellyfin/dto/mappers_test.go b/server/jellyfin/dto/mappers_test.go
index e7234a203..a7b381c5c 100644
--- a/server/jellyfin/dto/mappers_test.go
+++ b/server/jellyfin/dto/mappers_test.go
@@ -290,8 +290,6 @@ var _ = Describe("mappers", func() {
Expect(string(b)).ToNot(ContainSubstring("NormalizationGain"))
})
- // Real Jellyfin only attaches it when the client asks (DtoService.ContainsField), and derives
- // it from the image's real dimensions.
Describe("PrimaryImageAspectRatio", func() {
nonSquare := func() model.Album {
al := model.Album{ID: "al1", Name: "Album"}
@@ -457,9 +455,8 @@ var _ = Describe("mappers", func() {
Expect(after.ImageTags["Primary"]).To(Equal(before.ImageTags["Primary"]))
})
- // Nothing enqueues media files, so a track's own art only resolves when something requests
- // it. A Jellyfin client that is only told about the album image never asks, so the track's
- // own cover would stay unreachable for Jellyfin-only users.
+ // Nothing enqueues media files, so a track's own art only resolves when a client requests it;
+ // advertising just the album image would leave that cover unreachable.
Describe("unresolved embedded art", func() {
BeforeEach(func() {
DeferCleanup(configtest.SetupConfig())
diff --git a/server/jellyfin/e2e/playlists_test.go b/server/jellyfin/e2e/playlists_test.go
index 2ba4b1608..08d6f4f6f 100644
--- a/server/jellyfin/e2e/playlists_test.go
+++ b/server/jellyfin/e2e/playlists_test.go
@@ -215,8 +215,8 @@ var _ = Describe("Playlists", func() {
To(Equal(http.StatusNotImplemented))
})
- // Guards the whole chain: an upload must clear any previously-resolved artwork state, or a
- // stale tag stays live under clients' blurhash-keyed cover cache until the next scan (#5798).
+ // An upload must clear the resolved artwork state, or clients keep serving the stale cover
+ // from their tag-keyed cache until the next scan.
It("clears the resolved image tag after a cover upload", func() {
plID := createPlaylist("Cover Tag", nil)
Expect(ds.Artwork(ctx).PutItemArtwork(&model.ItemArtwork{
@@ -233,8 +233,7 @@ var _ = Describe("Playlists", func() {
Expect(upload(adminUser, "/Items/"+enc(plID)+"/Images/Primary", "image/jpeg", jpeg).Code).
To(Equal(http.StatusNoContent))
- // The upload re-queues resolution instead of resolving inline, so the tag falls back to
- // unresolved rather than carrying over the pre-upload hash.
+ // The upload re-queues resolution instead of resolving inline, so the tag goes bare.
Expect(imageTag()).ToNot(Equal("1111111111111111"))
})
})
diff --git a/server/jellyfin/images_test.go b/server/jellyfin/images_test.go
index f8cdf0701..bdd47e956 100644
--- a/server/jellyfin/images_test.go
+++ b/server/jellyfin/images_test.go
@@ -75,8 +75,7 @@ var _ = Describe("Images", func() {
Expect(fa.recvId).To(ContainSubstring("a1"))
})
- // resolveArtworkID probes the entity tables, so a deleted item cannot produce an artwork id
- // at all -- which is what stops artwork state outliving its entity from being served here.
+ // resolveArtworkID probes the entity tables, so a deleted item yields no artwork id at all.
It("asks for no artwork once the item is deleted, rather than its lingering state", func() {
ds := &tests.MockDataStore{} // no albums/artists/tracks/playlists at all
fa := &fakeArtwork{}
diff --git a/server/nativeapi/artwork.go b/server/nativeapi/artwork.go
index 1fd8af479..ac7890661 100644
--- a/server/nativeapi/artwork.go
+++ b/server/nativeapi/artwork.go
@@ -9,7 +9,6 @@ import (
"github.com/navidrome/navidrome/model"
)
-// refreshableArtworkKinds are the entity kinds a manual re-resolve accepts.
var refreshableArtworkKinds = map[model.Kind]bool{
model.KindAlbumArtwork: true,
model.KindArtistArtwork: true,
@@ -22,7 +21,6 @@ func (api *Router) addArtworkRoute(r chi.Router) {
r.Post("/artwork/{kind}/{id}/refresh", api.refreshArtwork())
}
-// refreshArtwork clears an item's resolved artwork state and re-queues it at Bump priority.
// State is deliberately cleared so a wrong pick disappears immediately (placeholder until re-resolved).
func (api *Router) refreshArtwork() http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
diff --git a/server/public/handle_images.go b/server/public/handle_images.go
index 1c45898c9..223dad5bb 100644
--- a/server/public/handle_images.go
+++ b/server/public/handle_images.go
@@ -39,9 +39,8 @@ func (pub *Router) handleImages(w http.ResponseWriter, r *http.Request) {
http.Error(w, "invalid request", http.StatusBadRequest)
return
}
- // Elevated like the Jellyfin image route: the token is the authorization, so the service's
- // entity check asks "is it still there", not "may this user see it" -- the latter would hide
- // a shared private playlist, which is the case shares exist to serve.
+ // The token is the authorization, so the entity check must ask "is it still there", not
+ // "may this user see it" -- the latter would hide a shared private playlist.
ctx = request.WithUser(ctx, model.User{IsAdmin: true})
size := p.IntOr("size", 0)
square := p.BoolOr("square", false)
diff --git a/server/public/handle_images_test.go b/server/public/handle_images_test.go
index cef4e0851..214108bef 100644
--- a/server/public/handle_images_test.go
+++ b/server/public/handle_images_test.go
@@ -53,8 +53,7 @@ var _ = Describe("handleImages", func() {
return httptest.NewRequest("GET", "/img?:id="+url.QueryEscape(token), nil)
}
- // The handler re-checks that the entity behind the token still exists, so every spec needs
- // a store where "1" is a live album.
+ // The handler re-checks that the entity behind the token still exists, so "1" must be live.
var ds *tests.MockDataStore
BeforeEach(func() {
diff --git a/server/subsonic/e2e/subsonic_artwork_test.go b/server/subsonic/e2e/subsonic_artwork_test.go
index 2679b4324..e757ebf14 100644
--- a/server/subsonic/e2e/subsonic_artwork_test.go
+++ b/server/subsonic/e2e/subsonic_artwork_test.go
@@ -43,9 +43,8 @@ import (
"go.senan.xyz/taglib"
)
-// The artwork serving path streams folder-backed originals via os.Open, which a fake FS cannot
-// back, so this suite scans a small REAL on-disk library and drives the real acquisition worker
-// and artwork.Artwork through the Subsonic and public image handlers.
+// The serving path streams folder-backed originals via os.Open, which a fake FS cannot back, so
+// this suite scans a small REAL on-disk library and drives the real worker and artwork.Artwork.
var _ = Describe("Artwork Serving", Ordered, func() {
var (
artRouter *subsonic.Router
@@ -105,9 +104,8 @@ var _ = Describe("Artwork Serving", Ordered, func() {
ds = &tests.MockDataStore{RealDS: persistence.New(db.Db())}
auth.Init(ds)
- // A re-scan of already-populated data deadlocks the parallel album-refresh phase at the
- // harness's MaxOpenConns=1, so wipe the golden content and import ONLY the real artwork
- // library fresh (the import path never enters that refresh phase).
+ // Re-scanning already-populated data deadlocks the parallel album-refresh phase at the
+ // harness's MaxOpenConns=1, so wipe the golden content and import this library fresh.
wipeScannedContent()
artLib := model.Library{Name: "Artwork Library", Path: musicDir}
Expect(ds.Library(ctx).Put(&artLib)).To(Succeed())
@@ -201,9 +199,8 @@ var _ = Describe("Artwork Serving", Ordered, func() {
Expect(w.Header().Get("Cache-Control")).To(Equal("no-store"))
})
- // The BeforeAll grants the artwork library to adminUser only, so regularUser cannot see
- // this album -- but its artwork state and bytes are perfectly servable by id. The service
- // resolves the entity through the caller's repositories, so the filter still applies.
+ // Only adminUser was granted this library, so regularUser must get the placeholder even
+ // though the artwork state and bytes are perfectly servable by id.
It("serves the placeholder for an album in a library the caller cannot see", func() {
w := httptest.NewRecorder()
artRouter.ServeHTTP(w, buildReq(regularUser, "getCoverArt", "id", "al-"+artfulID))
@@ -212,12 +209,10 @@ var _ = Describe("Artwork Serving", Ordered, func() {
Expect(w.Body.Bytes()).To(Equal(placeholder), "must not leak the real cover")
Expect(w.Header().Get("Cache-Control")).To(Equal("no-store"))
- // ...while the admin, who does have access, gets the real bytes for the same id.
Expect(getCover("id", "al-"+artfulID).Body.Bytes()).ToNot(Equal(placeholder))
})
- // An album with no art and an id naming no album are different answers; only the former
- // is a placeholder.
+ // An id naming no entity is a different answer from an album with no art: no placeholder.
It("answers error 70 for an id that matches no entity", func() {
w := getCover("id", "al-nosuchalbum")
Expect(w.Code).To(Equal(http.StatusOK))
@@ -261,8 +256,8 @@ func buildArtworkRouter(art artwork.Artwork) *subsonic.Router {
)
}
-// wipeScannedContent clears all scanned library content (including the golden fake library) so the
-// next scan is a clean import. FKs are disabled for the bulk delete, mirroring harness.Restore.
+// wipeScannedContent clears all scanned content so the next scan is a clean import. FKs are
+// disabled for the bulk delete, mirroring harness.Restore.
func wipeScannedContent() {
GinkgoHelper()
_, err := db.Db().Exec("PRAGMA foreign_keys = OFF")
@@ -283,8 +278,7 @@ func albumIDByName(name string) string {
return albums[0].ID
}
-// writeArtworkTrack lays down one tag-distinct track (and optionally a folder cover) so the scanner
-// creates a self-contained album; distinct ALBUM/ALBUMARTIST tags keep the two albums from merging.
+// Distinct ALBUM/ALBUMARTIST tags keep the generated albums from merging into one.
func writeArtworkTrack(root, artist, album, title string, withCover bool) {
GinkgoHelper()
dir := filepath.Join(root, artist, album)
@@ -306,9 +300,8 @@ func readArtworkFixture(name string) []byte {
return data
}
-// newDummyImageCache backs the artwork.Artwork's resize cache. size=0 requests stream originals
-// and never invoke the reader, so it only needs to satisfy the constructor; resize behavior is
-// covered by the artwork package's own suites.
+// size=0 requests stream originals and never invoke the reader, so this resize cache only has
+// to satisfy the constructor.
func newDummyImageCache(ctx context.Context) cache.FileCache {
GinkgoHelper()
c := cache.NewFileCache("SubsonicArtworkE2E", "100MB", "images", 0,
@@ -319,7 +312,6 @@ func newDummyImageCache(ctx context.Context) cache.FileCache {
return c
}
-// runWorkerUntil drives the real worker loop until a condition holds, then cancels and joins it.
func runWorkerUntil(ctx context.Context, worker *artwork.Worker, until func() bool) {
GinkgoHelper()
runCtx, cancel := context.WithCancel(ctx)
diff --git a/server/subsonic/helpers_test.go b/server/subsonic/helpers_test.go
index e437ada6c..51a92ac4c 100644
--- a/server/subsonic/helpers_test.go
+++ b/server/subsonic/helpers_test.go
@@ -349,8 +349,7 @@ var _ = Describe("helpers", func() {
})
Describe("childFromMediaFile", func() {
- // The album id carries the album's hash, which hydration puts in AlbumImage; the
- // track's own ItemImage describes its own art and must not be stamped onto an al- id.
+ // An al- id must carry the album's hash (AlbumImage), never the track's own.
It("suffixes coverArt with the album's content hash when resolved", func() {
mf := model.MediaFile{ID: "mf-1", AlbumID: "al-1", AlbumImage: model.ItemImage{ImageHash: hash}}
Expect(childFromMediaFile(ctx, mf).CoverArt).To(Equal("al-al-1_" + hash))
diff --git a/server/subsonic/media_retrieval_test.go b/server/subsonic/media_retrieval_test.go
index 63ceaa8fa..7610c866a 100644
--- a/server/subsonic/media_retrieval_test.go
+++ b/server/subsonic/media_retrieval_test.go
@@ -67,9 +67,8 @@ var _ = Describe("MediaRetrievalController", func() {
Expect(w.Body.String()).To(Equal(artwork.data))
})
- // Visibility now lives in the service, which resolves the entity through the
- // request-scoped repositories. The handler's whole contribution is handing over the
- // caller's context unchanged -- elevating here would bypass the library filter.
+ // The service applies the library filter from the caller's context, so elevating here
+ // would bypass it.
It("passes the caller's context to the service rather than elevating", func() {
r := newGetRequest("id=al-34")
usr := model.User{ID: "u1", UserName: "u1"}
diff --git a/tests/harness/harness.go b/tests/harness/harness.go
index b4da0377e..5949c4fae 100644
--- a/tests/harness/harness.go
+++ b/tests/harness/harness.go
@@ -86,9 +86,8 @@ func SetupDB(ctx context.Context, users ...*model.User) *DB {
return h
}
-// ResettableTables lists the tables a per-spec reset may write. FTS shadow tables are excluded;
-// they are kept in sync by their content tables' triggers, and writing them directly corrupts the
-// index.
+// ResettableTables lists the tables a per-spec reset may write. FTS shadow tables are excluded:
+// their content tables' triggers keep them in sync, and writing them directly corrupts the index.
func ResettableTables() []string {
rows, err := db.Db().Query("SELECT name FROM main.sqlite_master WHERE type='table' AND name NOT LIKE 'sqlite_%' AND name NOT LIKE '%_fts' AND name NOT LIKE '%_fts_%'")
Expect(err).ToNot(HaveOccurred())
@@ -103,9 +102,8 @@ func ResettableTables() []string {
return tables
}
-// TruncateDB empties every resettable table, leaving the migrated schema in place. Suites whose
-// specs each build their own library use this instead of a golden snapshot, since re-migrating the
-// schema per spec costs ~400ms.
+// TruncateDB empties every resettable table, leaving the migrated schema in place — for suites
+// whose specs each build their own library, so the schema is not re-migrated per spec.
func TruncateDB(tables []string) {
sqlDB := db.Db()
_, err := sqlDB.Exec("PRAGMA foreign_keys = OFF")
diff --git a/tests/mock_artwork_queue_repo.go b/tests/mock_artwork_queue_repo.go
index 0068d6087..82661d728 100644
--- a/tests/mock_artwork_queue_repo.go
+++ b/tests/mock_artwork_queue_repo.go
@@ -11,13 +11,13 @@ import (
type MockArtworkQueueRepo struct {
model.ArtworkQueueRepository
- // mu guards Data so the worker's concurrent drain can hit this mock race-free.
+ // mu guards Data: the worker drains this mock concurrently.
mu sync.Mutex
Data map[string]model.ArtworkQueueItem // keyed by iaKey(kind, id, imageType)
Err error
// ItemArtworkSource, when set, backs EnqueueStaleAbsent with real item_artwork state.
ItemArtworkSource *MockArtworkRepo
- // ExistingIDs, keyed by item_kind, backs PurgeDangling; a nil per-kind map keeps that kind.
+ // ExistingIDs is keyed by item_kind; a nil per-kind map means PurgeDangling keeps that kind.
ExistingIDs map[string]map[string]bool
}
@@ -191,8 +191,7 @@ func (m *MockArtworkQueueRepo) EnqueueStaleAbsent(kind model.Kind, attemptedBefo
return inserted, nil
}
-// EnqueueMissing enqueues entities in ExistingIDs[kind] that have no item_artwork row in
-// ItemArtworkSource and are not already queued, mirroring the SQL set-difference insert.
+// EnqueueMissing mirrors the SQL set-difference insert: ExistingIDs[kind] minus ItemArtworkSource.
func (m *MockArtworkQueueRepo) EnqueueMissing(kind model.Kind) (int64, error) {
m.mu.Lock()
defer m.mu.Unlock()
diff --git a/ui/src/album/AlbumGridView.jsx b/ui/src/album/AlbumGridView.jsx
index 983089416..e17fb617e 100644
--- a/ui/src/album/AlbumGridView.jsx
+++ b/ui/src/album/AlbumGridView.jsx
@@ -232,8 +232,7 @@ const LoadedAlbumGrid = ({ ids, data, basePath, width }) => {
}
const AlbumGridView = ({ albumListType, loaded, loading, seed, ...props }) => {
- // A re-roll replaces every album, so the previous roll must not linger while it loads. Blanking
- // on any load instead collapsed the grid to a spinner on each search keystroke.
+ // A re-roll replaces every album, so the previous roll must not linger while it loads.
const rerolling = useRollChanged(seed, loading) && albumListType === 'random'
const hide = rerolling || !props.data || !props.ids
return hide ? :
diff --git a/ui/src/album/useRollChanged.test.jsx b/ui/src/album/useRollChanged.test.jsx
index b7920a331..d0074a077 100644
--- a/ui/src/album/useRollChanged.test.jsx
+++ b/ui/src/album/useRollChanged.test.jsx
@@ -8,8 +8,8 @@ describe('useRollChanged', () => {
initialProps: props,
})
- // A re-roll redirects and remounts the grid, so "mounted with a load in flight" is exactly the
- // case where the store still holds the previous roll. Nothing is settled until that load lands.
+ // A re-roll remounts the grid, so on mount a load in flight means the store still holds the
+ // previous roll.
it('reports a change while a load is in flight on a fresh mount', () => {
const { result } = setup({ seed: 's1', loading: true })
expect(result.current).toBe(true)
@@ -20,15 +20,12 @@ describe('useRollChanged', () => {
expect(result.current).toBe(false)
})
- // Typing in the search box refetches with the same seed: the albums on screen still belong to
- // the roll being loaded, so they must stay put.
it('stays false while loading a filter change on the same roll', () => {
const { result, rerender } = setup({ seed: 's1', loading: false })
rerender({ seed: 's1', loading: true })
expect(result.current).toBe(false)
})
- // A new seed is a new roll, so what is on screen is about to be replaced wholesale.
it('goes true while loading after the seed changes', () => {
const { result, rerender } = setup({ seed: 's1', loading: false })
rerender({ seed: 's2', loading: true })
@@ -43,8 +40,7 @@ describe('useRollChanged', () => {
expect(result.current).toBe(false)
})
- // The seed can land a render before loading flips, which would otherwise record the new roll as
- // already shown and skip the blank entirely.
+ // The seed can land a render before loading flips; the new roll must not be recorded as shown.
it('still reports a change when the seed arrives before loading starts', () => {
const { result, rerender } = setup({ seed: 's1', loading: false })
rerender({ seed: 's2', loading: false })
diff --git a/ui/src/common/Artwork.jsx b/ui/src/common/Artwork.jsx
index 4c9398a4e..52d59f403 100644
--- a/ui/src/common/Artwork.jsx
+++ b/ui/src/common/Artwork.jsx
@@ -11,7 +11,7 @@ import { BlurHashCanvas } from './BlurHashCanvas'
const fadeMs = 500
const useStyles = makeStyles({
- // className supplies the size and shape; overflow:hidden clips the fills to a rounded shape.
+ // className supplies the size and shape; overflow:hidden clips the fills to it.
root: {
position: 'relative',
display: 'inline-flex',
@@ -34,9 +34,8 @@ const useStyles = makeStyles({
imgInstant: { opacity: 1, transition: 'none' },
})
-// Artwork renders an entity's cover through the shared useImageUrl blob cache, so it survives
-// React remounts without re-fetching. The blurhash is the loading placeholder; the image is only
-// mounted once its blob is ready, so an unresolved cover never renders as a broken ![]()
.
+// Renders a cover through the shared useImageUrl blob cache, so it survives remounts without
+// re-fetching. The image mounts only once its blob is ready, so it never renders broken.
export const Artwork = ({
record,
size = config.uiCoverArtSize,
@@ -50,8 +49,7 @@ export const Artwork = ({
const url = record ? subsonic.getCoverArtUrl(record, size, square) : ''
const { imgUrl } = useImageUrl(url)
- // A blob already cached when this instance mounted paints on the first frame, so it skips the
- // fade; anything fetched later cross-fades over the blurhash.
+ // A blob already cached at mount paints on the first frame, so it skips the fade.
const cachedOnMount = useRef(null)
if (cachedOnMount.current === null) {
cachedOnMount.current = !!imgUrl
@@ -63,8 +61,7 @@ export const Artwork = ({
setFaded(false)
}, [url])
- // Retire the blurhash on a timer rather than transitionend: under prefers-reduced-motion the
- // transition is none, so the event never fires and the placeholder would stay up forever.
+ // Timer, not transitionend: under prefers-reduced-motion there is no transition to end.
useEffect(() => {
if (!decoded || faded) return undefined
const timer = setTimeout(() => setFaded(true), fadeMs)
@@ -74,11 +71,9 @@ export const Artwork = ({
if (!record) return null
const instant = cachedOnMount.current
- // The blurhash stays mounted under the image until the fade ends. Swapping them the moment the
- // blob arrives would expose the empty container for the length of the fade.
+ // Kept mounted until the fade ends; swapping on blob arrival would flash an empty container.
const showBlurHash = !!record.blurHash && !instant && !faded
- // A square request is padded, not cropped, so its content is already aspect-fit inside the square
- // the server returns; `contain` is what keeps placeholder and image on the same pixels.
+ // A square request is padded, not cropped, so `contain` keeps placeholder and image aligned.
const effectiveFit = square ? 'contain' : fit
const ratio = record.imageWidth / record.imageHeight
const handleClick = imgUrl && onClick ? onClick : undefined
diff --git a/ui/src/common/Artwork.test.jsx b/ui/src/common/Artwork.test.jsx
index 7b9117860..22192bc6b 100644
--- a/ui/src/common/Artwork.test.jsx
+++ b/ui/src/common/Artwork.test.jsx
@@ -56,8 +56,7 @@ describe('Artwork', () => {
expect(canvas.style.objectFit).toBe('contain')
})
- // A square request is padded, not cropped, so the artwork still sits letterboxed inside the
- // square the server returns and the placeholder has to letterbox with it.
+ // A square request is padded, not cropped, so the placeholder has to letterbox with it.
it('letterboxes the blurhash when the server pads a non-square image to a square', () => {
useImageUrl.mockReturnValue({ imgUrl: null, loading: true })
const nonSquare = { ...withArt, imageWidth: 1200, imageHeight: 800 }
@@ -68,8 +67,7 @@ describe('Artwork', () => {
expect(canvas.style.objectFit).toBe('contain')
})
- // The padded square the server returns is aspect-fit, so cropping it would disagree with the
- // placeholder. Both renderers have to read `square` the same way.
+ // The padded square is aspect-fit, so `square` overrides fit="cover" as it does for the canvas.
it('fits the image itself with contain when the server padded to a square', () => {
useImageUrl.mockReturnValue({ imgUrl: 'blob:abc', loading: false })
const { container } = render(
@@ -109,15 +107,13 @@ describe('Artwork', () => {
useImageUrl.mockReturnValue({ imgUrl: null, loading: true })
const { container, rerender } = render()
- // Blob arrives: the image mounts transparent, with the blurhash still behind it.
useImageUrl.mockReturnValue({ imgUrl: 'blob:abc', loading: false })
rerender()
const img = container.querySelector('img')
expect(img).not.toBeNull()
expect(container.querySelector('canvas')).not.toBeNull()
- // Decoding starts the cross-fade; the blurhash only goes away once it finishes. The clock
- // drives it, not transitionend, which never fires under prefers-reduced-motion.
+ // The clock ends the fade, not transitionend, which never fires under reduced-motion.
act(() => {
fireEvent.load(img)
})
@@ -138,7 +134,6 @@ describe('Artwork', () => {
it('does not fade an image that was already cached on mount', () => {
useImageUrl.mockReturnValue({ imgUrl: 'blob:abc', loading: false })
const { container } = render()
- // No placeholder to cross-fade from, so it paints at once.
expect(container.querySelector('canvas')).toBeNull()
expect(container.querySelector('img').className).toContain('imgInstant')
})
diff --git a/ui/src/common/BlurHashCanvas.test.jsx b/ui/src/common/BlurHashCanvas.test.jsx
index c6f373c82..ebbe811b9 100644
--- a/ui/src/common/BlurHashCanvas.test.jsx
+++ b/ui/src/common/BlurHashCanvas.test.jsx
@@ -92,7 +92,6 @@ describe('BlurHashCanvas', () => {
rerender()
expect(ctxMock.clearRect).toHaveBeenCalledTimes(2)
- // No new pixels drawn after the clear, so the stale frame stays gone.
expect(ctxMock.putImageData).toHaveBeenCalledTimes(1)
})
})
diff --git a/ui/src/common/useScrollToTop.test.jsx b/ui/src/common/useScrollToTop.test.jsx
index 631d1362e..89bde3d37 100644
--- a/ui/src/common/useScrollToTop.test.jsx
+++ b/ui/src/common/useScrollToTop.test.jsx
@@ -12,8 +12,7 @@ describe('useScrollToTop', () => {
expect(window.scrollTo).toHaveBeenCalledWith({ top: 0 })
})
- // Navigating straight from one detail page to another (an album's artist link, say) reuses the
- // component, so only the key change tells us we are looking at something new.
+ // Detail-to-detail navigation reuses the component, so only the key change signals a new page.
it('scrolls again when the key changes', () => {
const { rerender } = renderHook(({ id }) => useScrollToTop(id), {
initialProps: { id: 'al-1' },
@@ -32,8 +31,7 @@ describe('useScrollToTop', () => {
expect(window.scrollTo).toHaveBeenCalledTimes(1)
})
- // The record arrives after the first render, so the key starts undefined; scrolling then would
- // fire before the page has its content and read as a no-op.
+ // The record arrives after the first render, so the key starts undefined.
it('waits for a key rather than scrolling on an empty record', () => {
const { rerender } = renderHook(({ id }) => useScrollToTop(id), {
initialProps: { id: undefined },
diff --git a/utils/cache/file_caches_test.go b/utils/cache/file_caches_test.go
index 4dd5bb626..7ce1b0393 100644
--- a/utils/cache/file_caches_test.go
+++ b/utils/cache/file_caches_test.go
@@ -342,8 +342,7 @@ var _ = Describe("File Caches", func() {
})
It("re-fetches when an adopted entry's data file vanished", func() {
- // Entries adopted on startup take a different code path than ones
- // created in-process, so cover both.
+ // Entries adopted on startup take a different code path than in-process ones.
var calls atomic.Int32
fc := callNewFileCache("test", "10MB", "test", 0, func(ctx context.Context, arg Item) (io.Reader, error) {
calls.Add(1)
diff --git a/utils/cache/spread_fs_test.go b/utils/cache/spread_fs_test.go
index 44b14db2b..d0b54567d 100644
--- a/utils/cache/spread_fs_test.go
+++ b/utils/cache/spread_fs_test.go
@@ -44,8 +44,7 @@ var _ = Describe("Spread FS", func() {
Describe("Create", func() {
It("leaves an already-open reader's bytes intact", func() {
- // A re-created entry must not shrink the file an older stream is still
- // serving: its reader would spin forever at the premature EOF.
+ // Shrinking a file an older stream still serves spins its reader at a premature EOF.
if runtime.GOOS == "windows" {
Skip("Windows cannot unlink a file with open handles, so Create reuses the inode")
}