From 9b0bfc606bc5978032702a259de8defd820e474d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Fri, 10 Apr 2026 19:29:20 -0400 Subject: [PATCH] fix(subsonic): always emit required `created` field on AlbumID3 (#5340) * fix(subsonic): always emit required `created` field on AlbumID3 Strict OpenSubsonic clients (e.g. Navic via dev.zt64.subsonic) reject search3/getAlbum/getAlbumList2 responses that omit the `created` field, which the spec marks as required. Navidrome was dropping it whenever the album's CreatedAt was zero. Root cause was threefold: 1. buildAlbumID3/childFromAlbum conditionally emitted `created`, so a zero CreatedAt became a missing JSON key. 2. ToAlbum's `older()` helper treated a zero BirthTime as the minimum, so a single track with missing filesystem birth time could poison the album aggregation. 3. phase_1_folders' CopyAttributes copied `created_at` from the previous album row unconditionally, propagating an already-zero value forward on every metadata-driven album ID change. Since sql_base_repository drops `created_at` on UPDATE, a poisoned row could never self-heal. Fixes: - Always emit `created`, falling back to UpdatedAt/ImportedAt when CreatedAt is zero. Adds albumCreatedAt() helper used by both buildAlbumID3 and childFromAlbum. - Guard `older()` against a zero second argument. - Skip the CopyAttributes call in phase_1_folders when the previous album's created_at is zero, so the freshly-computed value survives. - New migration backfills existing broken rows from media_file.birth_time (falling back to updated_at). Tested against a real DB: repaired 605/6922 affected rows, no side effects on healthy rows. Signed-off-by: Deluan * refactor(subsonic): return albumCreatedAt by value to avoid heap escape Returning *time.Time from albumCreatedAt caused Go escape analysis to move the entire model.Album parameter to the heap, since the returned pointer aliased a field of the value receiver. For hot endpoints like getAlbumList2 and search3, this meant one full-struct heap allocation per album result. Return time.Time by value and let callers wrap it with gg.P() to take the address locally. Only the small time.Time value escapes; the model.Album struct stays on the stack. Also corrects the doc comment to reflect the actual guarantee ("best-effort" rather than "non-zero"), matching the test case that exercises the all-zero fallback. --------- Signed-off-by: Deluan --- ...260410201914_fix_zero_album_created_at.sql | 22 +++++++++++++ model/mediafile.go | 3 ++ model/mediafile_test.go | 14 ++++++++ persistence/album_repository.go | 12 ++++++- persistence/album_repository_test.go | 26 +++++++++++++++ server/subsonic/helpers.go | 22 ++++++++++--- server/subsonic/helpers_test.go | 32 +++++++++++++++++++ 7 files changed, 126 insertions(+), 5 deletions(-) create mode 100644 db/migrations/20260410201914_fix_zero_album_created_at.sql diff --git a/db/migrations/20260410201914_fix_zero_album_created_at.sql b/db/migrations/20260410201914_fix_zero_album_created_at.sql new file mode 100644 index 000000000..ff47eb95f --- /dev/null +++ b/db/migrations/20260410201914_fix_zero_album_created_at.sql @@ -0,0 +1,22 @@ +-- +goose Up + +-- Backfill album.created_at for rows poisoned by early scanner versions or +-- propagated via CopyAttributes during metadata-driven ID changes. Prefer the +-- oldest valid birth_time from the album's media files, fall back to updated_at. +UPDATE album +SET created_at = COALESCE( + (SELECT MIN(birth_time) + FROM media_file + WHERE media_file.album_id = album.id + AND birth_time IS NOT NULL + AND birth_time != '' + AND birth_time NOT LIKE '0001-%'), + updated_at +) +WHERE created_at IS NULL + OR created_at = '' + OR created_at LIKE '0001-%'; + +-- +goose Down + +SELECT 1; diff --git a/model/mediafile.go b/model/mediafile.go index ec83b76fd..6be8402ae 100644 --- a/model/mediafile.go +++ b/model/mediafile.go @@ -361,6 +361,9 @@ func older(t1, t2 time.Time) time.Time { if t1.IsZero() { return t2 } + if t2.IsZero() { + return t1 + } if t1.After(t2) { return t2 } diff --git a/model/mediafile_test.go b/model/mediafile_test.go index 038ac93d5..8b0c13da2 100644 --- a/model/mediafile_test.go +++ b/model/mediafile_test.go @@ -119,6 +119,20 @@ var _ = Describe("MediaFiles", func() { Expect(a.MinYear).To(Equal(1999)) }) }) + Context("CreatedAt aggregation", func() { + It("ignores zero BirthTime values when computing the oldest", func() { + mfs = MediaFiles{ + {BirthTime: t("2022-12-19 08:30")}, + {BirthTime: time.Time{}}, + {BirthTime: t("2022-12-18 10:00")}, + } + Expect(mfs.ToAlbum().CreatedAt).To(Equal(t("2022-12-18 10:00"))) + }) + It("returns zero when all BirthTime values are zero", func() { + mfs = MediaFiles{{BirthTime: time.Time{}}, {BirthTime: time.Time{}}} + Expect(mfs.ToAlbum().CreatedAt).To(BeZero()) + }) + }) }) When("we have multiple songs with same dates", func() { BeforeEach(func() { diff --git a/persistence/album_repository.go b/persistence/album_repository.go index c51a5beb1..99ed10877 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -252,7 +252,17 @@ func (r *albumRepository) CopyAttributes(fromID, toID string, columns ...string) } to := make(map[string]any) for _, col := range columns { - to[col] = from[col] + v := from[col] + // created_at is aggregated from song birth_times and must never be + // overwritten with a zero/poisoned value, or it propagates forward on + // every metadata-driven album ID change. + if col == "created_at" && (!v.Valid || v.String == "" || strings.HasPrefix(v.String, "0001-")) { + continue + } + to[col] = v + } + if len(to) == 0 { + return nil } _, err = r.executeSQL(Update(r.tableName).SetMap(to).Where(Eq{"id": toID})) return err diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go index 2792cec97..a6270933f 100644 --- a/persistence/album_repository_test.go +++ b/persistence/album_repository_test.go @@ -41,6 +41,32 @@ var _ = Describe("AlbumRepository", func() { }) }) + Describe("CopyAttributes", func() { + var srcTime, dstTime time.Time + BeforeEach(func() { + srcTime = time.Date(2020, 1, 2, 3, 4, 5, 0, time.UTC) + dstTime = time.Date(2024, 6, 7, 8, 9, 10, 0, time.UTC) + Expect(albumRepo.Put(&model.Album{ID: "copy-src", Name: "src", LibraryID: 1, CreatedAt: srcTime})).To(Succeed()) + Expect(albumRepo.Put(&model.Album{ID: "copy-dst", Name: "dst", LibraryID: 1, CreatedAt: dstTime})).To(Succeed()) + Expect(albumRepo.Put(&model.Album{ID: "copy-zero", Name: "zero", LibraryID: 1})).To(Succeed()) + DeferCleanup(func() { + _, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": []string{"copy-src", "copy-dst", "copy-zero"}})) + }) + }) + It("copies a valid created_at from source to destination", func() { + Expect(albumRepo.CopyAttributes("copy-src", "copy-dst", "created_at")).To(Succeed()) + got, err := albumRepo.Get("copy-dst") + Expect(err).ToNot(HaveOccurred()) + Expect(got.CreatedAt).To(BeTemporally("~", srcTime, time.Second)) + }) + It("leaves destination untouched when source created_at is zero", func() { + Expect(albumRepo.CopyAttributes("copy-zero", "copy-dst", "created_at")).To(Succeed()) + got, err := albumRepo.Get("copy-dst") + Expect(err).ToNot(HaveOccurred()) + Expect(got.CreatedAt).To(BeTemporally("~", dstTime, time.Second)) + }) + }) + Describe("GetAll", func() { var GetAll = func(opts ...model.QueryOptions) (model.Albums, error) { albums, err := albumRepo.GetAll(opts...) diff --git a/server/subsonic/helpers.go b/server/subsonic/helpers.go index e930aa630..ffa10898e 100644 --- a/server/subsonic/helpers.go +++ b/server/subsonic/helpers.go @@ -10,6 +10,7 @@ import ( "slices" "sort" "strings" + "time" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" @@ -17,6 +18,7 @@ import ( "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/subsonic/responses" + . "github.com/navidrome/navidrome/utils/gg" "github.com/navidrome/navidrome/utils/number" "github.com/navidrome/navidrome/utils/req" "github.com/navidrome/navidrome/utils/slice" @@ -317,6 +319,20 @@ func sanitizeSlashes(target string) string { return strings.ReplaceAll(target, "/", "_") } +// albumCreatedAt returns a best-effort timestamp for the album's `created` +// field, which is required by the OpenSubsonic spec but may be zero on legacy +// DB rows. Falls back to UpdatedAt → ImportedAt; can still return zero if all +// three are unset. +func albumCreatedAt(al model.Album) time.Time { + if !al.CreatedAt.IsZero() { + return al.CreatedAt + } + if !al.UpdatedAt.IsZero() { + return al.UpdatedAt + } + return al.ImportedAt +} + func childFromAlbum(ctx context.Context, al model.Album) responses.Child { child := responses.Child{} child.Id = al.ID @@ -329,7 +345,7 @@ func childFromAlbum(ctx context.Context, al model.Album) responses.Child { child.Year = int32(cmp.Or(al.MaxOriginalYear, al.MaxYear)) child.Genre = al.Genre child.CoverArt = al.CoverArtID().String() - child.Created = &al.CreatedAt + child.Created = P(albumCreatedAt(al)) child.Parent = al.AlbumArtistID child.ArtistId = al.AlbumArtistID child.Duration = int32(al.Duration) @@ -421,9 +437,7 @@ func buildAlbumID3(ctx context.Context, album model.Album) responses.AlbumID3 { dir.PlayCount = album.PlayCount dir.Year = int32(cmp.Or(album.MaxOriginalYear, album.MaxYear)) dir.Genre = album.Genre - if !album.CreatedAt.IsZero() { - dir.Created = &album.CreatedAt - } + dir.Created = P(albumCreatedAt(album)) if album.Starred { dir.Starred = album.StarredAt } diff --git a/server/subsonic/helpers_test.go b/server/subsonic/helpers_test.go index 4eb756b98..abf6116f3 100644 --- a/server/subsonic/helpers_test.go +++ b/server/subsonic/helpers_test.go @@ -571,6 +571,38 @@ var _ = Describe("helpers", func() { }) }) + Describe("buildAlbumID3 Created field", func() { + It("uses CreatedAt when set", func() { + t := time.Date(2020, 1, 2, 3, 4, 5, 0, time.UTC) + al := model.Album{ID: "a1", Name: "A", CreatedAt: t} + dir := buildAlbumID3(ctx, al) + Expect(dir.Created).ToNot(BeNil()) + Expect(*dir.Created).To(Equal(t)) + }) + + It("falls back to UpdatedAt when CreatedAt is zero", func() { + updated := time.Date(2019, 5, 6, 7, 8, 9, 0, time.UTC) + al := model.Album{ID: "a2", Name: "A", UpdatedAt: updated} + dir := buildAlbumID3(ctx, al) + Expect(dir.Created).ToNot(BeNil()) + Expect(*dir.Created).To(Equal(updated)) + }) + + It("falls back to ImportedAt when CreatedAt and UpdatedAt are zero", func() { + imported := time.Date(2021, 8, 9, 10, 11, 12, 0, time.UTC) + al := model.Album{ID: "a3", Name: "A", ImportedAt: imported} + dir := buildAlbumID3(ctx, al) + Expect(dir.Created).ToNot(BeNil()) + Expect(*dir.Created).To(Equal(imported)) + }) + + It("never leaves Created nil even when all timestamps are zero", func() { + al := model.Album{ID: "a4", Name: "A"} + dir := buildAlbumID3(ctx, al) + Expect(dir.Created).ToNot(BeNil()) + }) + }) + Describe("EnableAverageRating config", func() { It("excludes averageRating when disabled", func() { conf.Server.Subsonic.EnableAverageRating = false