mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-01 07:21:17 +00:00
fix(album): make cover cache keys immune to future file mtimes
All cover cache keys used max(updated_at, cover_art_updated_at), so an album whose updated_at is in the future (bad file mtimes) would mask a cover edit entirely — id unchanged, reader cache key unchanged, UI URL unchanged — and keep serving the old image. Treat the keys as discriminators instead of dates: the album artwork id adds the two timestamps, the reader cache keys (album/disc/mediafile) carry the cover stamp as a separate component, and the UI cache-buster joins both timestamps instead of picking the newest.
This commit is contained in:
parent
0ffcb38eae
commit
f498dfde7d
@ -27,6 +27,14 @@ func (k *cacheKey) Key() string {
|
||||
)
|
||||
}
|
||||
|
||||
// coverStamp renders the album's manual-cover timestamp for cache keys (0 when unset).
|
||||
func coverStamp(t *time.Time) int64 {
|
||||
if t == nil {
|
||||
return 0
|
||||
}
|
||||
return t.UnixMilli()
|
||||
}
|
||||
|
||||
type imageCache struct {
|
||||
cache.FileCache
|
||||
}
|
||||
|
||||
@ -70,11 +70,14 @@ func (a *albumArtworkReader) Key() string {
|
||||
hashInput = conf.Server.Agents + hashInput
|
||||
}
|
||||
hash := md5.Sum([]byte(hashInput))
|
||||
// coverStamp is a separate component: folded into lastUpdate it could be masked
|
||||
// by a newer updated_at (files with future mtimes).
|
||||
return fmt.Sprintf(
|
||||
"%s.%x.%t",
|
||||
"%s.%x.%t.%d",
|
||||
a.cacheKey.Key(),
|
||||
hash,
|
||||
conf.Server.EnableExternalServices,
|
||||
coverStamp(a.album.CoverArtUpdatedAt),
|
||||
)
|
||||
}
|
||||
func (a *albumArtworkReader) LastUpdated() time.Time {
|
||||
|
||||
@ -119,9 +119,10 @@ func newDiscArtworkReader(ctx context.Context, a *artwork, artID model.ArtworkID
|
||||
func (d *discArtworkReader) Key() string {
|
||||
hash := md5.Sum([]byte(conf.Server.DiscArtPriority))
|
||||
return fmt.Sprintf(
|
||||
"%s.%x",
|
||||
"%s.%x.%d",
|
||||
d.cacheKey.Key(),
|
||||
hash,
|
||||
coverStamp(d.album.CoverArtUpdatedAt),
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
@ -57,9 +57,10 @@ func newMediafileArtworkReader(ctx context.Context, artwork *artwork, artID mode
|
||||
|
||||
func (a *mediafileArtworkReader) Key() string {
|
||||
return fmt.Sprintf(
|
||||
"%s.%t",
|
||||
"%s.%t.%d",
|
||||
a.cacheKey.Key(),
|
||||
conf.Server.EnableMediaFileCoverArt,
|
||||
coverStamp(a.album.CoverArtUpdatedAt),
|
||||
)
|
||||
}
|
||||
func (a *mediafileArtworkReader) LastUpdated() time.Time {
|
||||
|
||||
@ -3,6 +3,7 @@ package model_test
|
||||
import (
|
||||
"encoding/json"
|
||||
"path/filepath"
|
||||
"time"
|
||||
|
||||
"github.com/navidrome/navidrome/conf"
|
||||
"github.com/navidrome/navidrome/conf/configtest"
|
||||
@ -39,6 +40,17 @@ var _ = Describe("Album", func() {
|
||||
Expect(a.UploadedImagePath()).To(Equal(filepath.Join("/data", "artwork", "album", "al-1_cover.jpg")))
|
||||
})
|
||||
})
|
||||
|
||||
Describe("CoverArtID", func() {
|
||||
It("changes when a cover is uploaded, even if updated_at is in the future", func() {
|
||||
future := time.Now().Add(365 * 24 * time.Hour)
|
||||
stamp := time.Now()
|
||||
a := Album{ID: "al-1", UpdatedAt: future}
|
||||
before := a.CoverArtID().String()
|
||||
a.CoverArtUpdatedAt = &stamp
|
||||
Expect(a.CoverArtID().String()).ToNot(Equal(before))
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
var _ = Describe("Albums", func() {
|
||||
|
||||
@ -112,11 +112,11 @@ func ParseDiscArtworkID(id string) (albumID string, discNumber int, err error) {
|
||||
}
|
||||
|
||||
func artworkIDFromAlbum(al Album) ArtworkID {
|
||||
// A manual cover edit bumps cover_art_updated_at (not updated_at), so fold it
|
||||
// in here to refresh clients without disturbing Recently Added ordering.
|
||||
// The suffix is a cache discriminator, not a date: cover edits are added (not max'd)
|
||||
// so the id changes even when updated_at is newer, e.g. files with future mtimes.
|
||||
lastUpdate := al.UpdatedAt
|
||||
if al.CoverArtUpdatedAt != nil && al.CoverArtUpdatedAt.After(lastUpdate) {
|
||||
lastUpdate = *al.CoverArtUpdatedAt
|
||||
if al.CoverArtUpdatedAt != nil {
|
||||
lastUpdate = time.Unix(max(al.UpdatedAt.Unix(), 0)+al.CoverArtUpdatedAt.Unix(), 0)
|
||||
}
|
||||
return ArtworkID{
|
||||
Kind: KindAlbumArtwork,
|
||||
|
||||
@ -557,9 +557,7 @@ var _ = Describe("MediaFile", func() {
|
||||
Expect(album.CoverArtID().String()).To(HaveSuffix("_0"))
|
||||
disc.CoverArtUpdatedAt = &stamp
|
||||
album.CoverArtUpdatedAt = &stamp
|
||||
Expect(disc.CoverArtID().LastUpdate).To(Equal(stamp))
|
||||
Expect(disc.CoverArtID().String()).ToNot(HaveSuffix("_0"))
|
||||
Expect(album.CoverArtID().LastUpdate).To(Equal(stamp))
|
||||
Expect(album.CoverArtID().String()).ToNot(HaveSuffix("_0"))
|
||||
})
|
||||
})
|
||||
|
||||
@ -80,12 +80,11 @@ const getAvatarUrl = (username, size) =>
|
||||
}),
|
||||
)
|
||||
|
||||
// Newest of updatedAt / coverArtUpdatedAt (album cover uploads bump the latter).
|
||||
// Cache-buster from both timestamps (cover uploads bump coverArtUpdatedAt); joined,
|
||||
// not max'd, so the URL changes even when updatedAt is newer (future file mtimes).
|
||||
const artCacheKey = (record) =>
|
||||
[record.updatedAt, record.coverArtUpdatedAt]
|
||||
.filter(Boolean)
|
||||
.sort((a, b) => new Date(a) - new Date(b))
|
||||
.pop()
|
||||
[record.updatedAt, record.coverArtUpdatedAt].filter(Boolean).join('|') ||
|
||||
undefined
|
||||
|
||||
const getCoverArtUrl = (record, size, square) => {
|
||||
const cacheKey = artCacheKey(record)
|
||||
|
||||
@ -79,7 +79,7 @@ describe('getCoverArtUrl', () => {
|
||||
expect(url).toContain('square=true')
|
||||
})
|
||||
|
||||
it('should bust the cache on coverArtUpdatedAt when it is newer', () => {
|
||||
it('should include coverArtUpdatedAt in the cache key', () => {
|
||||
const albumRecord = {
|
||||
id: 'album-123',
|
||||
albumArtist: 'Test Artist',
|
||||
@ -90,8 +90,9 @@ describe('getCoverArtUrl', () => {
|
||||
const url = subsonic.getCoverArtUrl(albumRecord)
|
||||
|
||||
expect(url).toContain('al-album-123')
|
||||
expect(url).toContain('_=2024-06-01T00%3A00%3A00Z')
|
||||
expect(url).not.toContain('_=2023-01-01')
|
||||
expect(url).toContain(
|
||||
'_=2023-01-01T00%3A00%3A00Z%7C2024-06-01T00%3A00%3A00Z',
|
||||
)
|
||||
})
|
||||
|
||||
it('should return media file cover art URL for records with album', () => {
|
||||
@ -191,7 +192,7 @@ describe('getDiscCoverArtUrl', () => {
|
||||
expect(url).not.toContain('size=')
|
||||
})
|
||||
|
||||
it('should bust the cache on coverArtUpdatedAt when it is newer', () => {
|
||||
it('should include coverArtUpdatedAt in the cache key', () => {
|
||||
const url = subsonic.getDiscCoverArtUrl({
|
||||
albumId: 'album-123',
|
||||
discNumber: 1,
|
||||
@ -200,8 +201,9 @@ describe('getDiscCoverArtUrl', () => {
|
||||
})
|
||||
|
||||
expect(url).toContain('id=dc-album-123%3A1')
|
||||
expect(url).toContain('_=2024-06-01T00%3A00%3A00Z')
|
||||
expect(url).not.toContain('_=2023-01-01')
|
||||
expect(url).toContain(
|
||||
'_=2023-01-01T00%3A00%3A00Z%7C2024-06-01T00%3A00%3A00Z',
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user