navidrome/persistence/folder_repository_test.go
Deluan Quintão 53d54baef0
fix(jellyfin): stream collection responses to prevent OOM on large libraries (#5783)
* fix(jellyfin): stream /Items responses to prevent OOM on large libraries

Finamp's library sync issues an unbounded GET /Items?IncludeItemTypes=Audio
with Fields=MediaSources and no Limit. On a large library this built the whole
result set — every MediaFile and every BaseItemDto, each fat with MediaSources —
in memory, and json.Encoder then buffered the entire ~200MB response before
writing a byte. Measured on a 96k-track library, one such request peaked near
1.6GB RSS; in a memory-limited container the resulting slowness made clients
retry, stacking concurrent full-library builds until the process was OOM-killed.

Stream the song listing straight from a DB cursor (MediaFileRepository.GetCursor),
mapping and encoding one row at a time, so peak memory is bounded to about one
item regardless of library size. The cursor is opened lazily, when streaming
begins, so it doesn't hold a DB connection across the CountAll and ServerId
lookups that run first (ServerId can write on first use and would deadlock
against an open reader). Pagination still works — the cursor query carries
LIMIT/OFFSET/ORDER BY from StartIndex/Limit/SortBy — and TotalRecordCount stays
the full count. Other materialized responses stream their JSON too, avoiding the
encoder's buffer-the-whole-output cost.

Also bound artwork image concurrency with the same server.ThrottleBacklog that
Subsonic's getCoverArt uses, so a burst of image requests during sync can't
exhaust memory through concurrent decode/resize.

Verified against a copy of a 96k-track production database: byte-for-byte
identical response, peak RSS 1593MB -> 53MB, time-to-first-byte 8.1s -> 0.2s.

* fix(jellyfin): fail loudly on /Items streaming errors instead of a truncated 200

Addresses code review: a streamed /Items response commits HTTP 200 before an
error can occur, so failures were surfacing as misleadingly successful bodies.

- A cursor-open failure (e.g. a busy DB under connection pressure) is now
  surfaced before the first byte is written: the cursor open is deferred into
  the item source and run in writeItems after the ServerId lookup but before the
  envelope, returning a clean 500 the client can retry — not a 200 with an empty
  item list.
- A mid-stream (row-scan) error now aborts without closing the JSON envelope,
  leaving the body malformed. A truncated-but-valid response would let a sync
  client (Finamp) treat the short list as the whole library and prune local
  tracks; malformed JSON forces the client's parser to fail and retry.

* refactor(jellyfin): stream every collection endpoint from a DB cursor

Streaming was applied only to the song listing, which left two write paths, two
ServerId stamping mechanisms and a listXxx return-type split ("songs is
special") that made the code hard to follow.

Add GetCursor to the album, artist, genre and playlist repositories, mirroring
MediaFileRepository.GetCursor: same select builder as GetAll, so each cursor
yields identical, fully-hydrated rows (hydration happens per-row in PostScan,
and toModels is only a deref loop). The playlist cursor keeps GetAll's
owner/public visibility filter. Each repository test asserts the cursor yields
exactly what GetAll returns, including Max/Offset.

With cursors everywhere, every listXxx returns itemsResult and every collection
streams through one writer:

- listAlbums, listArtists and listPlaylists now stream from their cursors;
  search paths stay materialized (Search returns a slice).
- listGenres stays materialized: its total is the length of the full list and it
  paginates in memory, so there is nothing for a cursor to page over.
- api.ok's QueryResult case delegates to writeItems, so the ~9 inline callers
  funnel into the same writer; streamQueryResult and wrapResult are gone.
- /Items/Latest streams as a bare JSON array (its Jellyfin wire shape) via
  writeItemsArray, which shares the item loop and stamping. That also closes the
  same unbounded hole /Items had: limit=0 disabled its LIMIT.
- ServerId is now stamped in exactly one place, so api.ok only handles single,
  non-collection payloads.

Verified against a copy of a 96k-track production database: every endpoint
byte-for-byte identical to master. Unbounded /Items peak RSS 1691MB -> 54MB;
time-to-first-byte 6.3s -> 0.19s, /Artists 1.14s -> 0.13s.

* refactor(jellyfin): route every response through api.ok

Handlers were split between api.ok and api.writeItems with no clear rule for
which to call. api.ok now accepts itemsResult too, so it is the single entry
point: callers hand it whatever they have and it routes collections (cursor
-backed or materialized) to the streaming writer. writeItems is reached only
through api.ok now. The one exception is /Items/Latest, which returns a bare
JSON array rather than a QueryResult envelope and so writes directly — noted in
both doc comments.

Also drop GenreRepository.GetCursor: listGenres derives its total from the
length of the full list and paginates in memory, so there is nothing for a
cursor to page over, leaving the method unused.

* fix(jellyfin): stream the unbounded multi-type /Items merge

The multi-type merge capped each per-type query at offset+limit only when a
Limit was given. Without one (Finamp's favorites screen sends multi-type), every
type ran an unbounded query and collect() drained each cursor into a slice — so
/Items?IncludeItemTypes=MusicAlbum,Audio with no Limit materialized every album
and every song, the same OOM class this branch set out to fix. The comment
claiming the set was "capped at offset+limit per type" was wrong for limit=0.

Without a limit the merged page is just each type's rows in order minus the
first offset, which is exactly what chaining the per-type cursors yields, so
stream that instead of merging in memory. The bounded path is unchanged: with a
Limit each type holds at most offset+limit rows, so merging and paginating
across the combined list is safe. Cursors are opened one at a time (each pins a
DB connection until drained), with the first opened eagerly so the usual failure
is still a clean error before any byte is written.

Measured on a 96k-track library, /Items?IncludeItemTypes=MusicAlbum,Audio with
no Limit (103,393 items): peak RSS 612MB -> 53MB, time-to-first-byte 4.9s ->
0.6s, response byte-for-byte identical.

* refactor(jellyfin): parse /Items params into a struct

The /Items dispatcher was a single 90-line block that parsed a dozen params and
then threaded them positionally through three layers — queryItemsOfType took 11
arguments, listSongs and listAlbums 9 each — so reading any one of them meant
decoding a long argument list.

Parse once into an itemsQuery and pass that instead. queryItems now reads as the
four things it actually does: parse, the id/playlists-folder/playlist-parent
special cases, single-type dispatch, multi-type merge — with the playlist-parent
and merge bodies moved to playlistTracks and mergeTypes. Every listXxx takes
(ctx, opts, q).

No behavior change: parsing, ordering and the entityParent rule are unchanged,
and /Artists still passes favOnly=false explicitly by building the subset of the
query it uses.

* docs(jellyfin): trim comments on the streaming path

Cut the comments back to the non-obvious why: the deferred cursor open (the
ServerId write would deadlock against an open reader), the deliberate malformed
JSON on a mid-stream error, why chained opens one cursor at a time, why
listGenres stays materialized, and why the cursor helpers take the underlying
func type. Dropped the rest — restatements of the code, doc comments that
repeated a test's own name, and the GetCursor interface comments that the
existing MediaFileRepository.GetCursor does without.

* refactor(persistence): fold the cursor wrappers into a generic wrapCursor

wrapAlbumCursor, wrapArtistCursor, wrapMediaFileCursor and wrapFolderCursor were
the same twelve lines four times over, differing only in the type name, and the
playlist cursor had its own inline copy of the loop.

Add one generic wrapCursor. It takes an extractor func rather than a method on
an interface: a type parameter can't reach an embedded field, and methods that
only satisfy a generic constraint are reported by the unused linter. The
extractor also lets both type parameters be inferred, so call sites need no
explicit instantiation. Each entity keeps its named wrapper as a one-liner,
since the model cursor types are defined types and need the conversion — and the
existing wrapper tests call them directly.

The nil-row error now names the model type via %T ("unexpected nil model.Album")
instead of a hand-written per-entity string; the three tests asserting that
message are updated.

* fix(jellyfin): bound concurrent collection streams

A streamed collection holds a DB cursor — and its pooled connection — for the whole client-paced
response, where the old materialize-then-write path released it as soon as the query finished. The
pool is shared with the scanner, Subsonic, the native API and the UI, so enough slow clients take
every connection and everything else blocks waiting for one. Measured against a copy of a 96k-track
production DB, 20 concurrent unbounded streams against a pool of 16 stalled a write for 16.2s (the
other 14 writes in the run took ~2ms — the signature of connection starvation, not lock contention).

Cap concurrent streams at half the pool. Excess requests queue rather than fail, so no client is
rejected: the same 20 streams now all complete and the worst write is 2.5ms.

- conf.MaxOpenConns() now owns the pool sizing (db calls it). It belongs in conf: the pool is a
  tunable, db already imports conf, and putting it in db would force server/jellyfin to import db
  just to size the cap against it. Expressing the cap as MaxOpenConns()/2 also keeps the two from
  drifting apart.
- Uses chi's ThrottleBacklog, not server.ThrottleBacklog: the latter buffers the whole response to
  release its token early, which is right for artwork but would undo the streaming. chi's panics on a
  non-positive limit, so throttleStreams guards it — setting MaxConcurrentStreams=0 disables the cap
  instead of crashing the server at startup.

* refactor(jellyfin): move throttleStreams to middlewares.go

It's a middleware, so it belongs beside normalizeQueryKeys, authenticate and
withPlayer rather than in api.go. Its tests move to middlewares_test.go with it,
keeping one test file per production file.

* fix(jellyfin): abandon the scan when the client stops reading

encodeItems discarded its write errors and relied on the final Flush to report
them, so once bufio's buffer filled and the flush failed, the loop still pulled
every remaining row through the cursor and serialized it for a client that was
gone. A test with a failing writer confirms it: all 20k items were drained.

That wastes CPU, and holds the cursor's pooled DB connection and a stream slot
(now a capped resource) for the length of a full scan nobody is reading. Check
the per-item writes so the first failure ends the scan; the fixed envelope
writes stay unchecked, since bufio latches for them anyway.
2026-07-15 14:07:00 -04:00

353 lines
13 KiB
Go

package persistence
import (
"context"
"errors"
"fmt"
"github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/model/request"
"github.com/navidrome/navidrome/tests"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/pocketbase/dbx"
)
var _ = Describe("FolderRepository", func() {
var repo model.FolderRepository
var ctx context.Context
var conn *dbx.DB
var testLib, otherLib model.Library
BeforeEach(func() {
ctx = request.WithUser(log.NewContext(context.TODO()), model.User{ID: "userid"})
conn = GetDBXBuilder()
repo = newFolderRepository(ctx, conn)
// Use existing library ID 1 from test fixtures
libRepo := NewLibraryRepository(ctx, conn)
lib, err := libRepo.Get(1)
Expect(err).ToNot(HaveOccurred())
testLib = *lib
// Create a second library with its own folder to verify isolation
otherLib = model.Library{Name: "Other Library", Path: "/other/path"}
Expect(libRepo.Put(&otherLib)).To(Succeed())
})
AfterEach(func() {
// Clean up only test folders created by our tests (paths starting with "Test")
// This prevents interference with fixture data needed by other tests
_, _ = conn.NewQuery("DELETE FROM folder WHERE library_id = 1 AND path LIKE 'Test%'").Execute()
_, _ = conn.NewQuery(fmt.Sprintf("DELETE FROM library WHERE id = %d", otherLib.ID)).Execute()
})
Describe("GetFolderUpdateInfo", func() {
Context("with no target paths", func() {
It("returns all folders in the library", func() {
// Create test folders with unique names to avoid conflicts
folder1 := model.NewFolder(testLib, "TestGetLastUpdates/Folder1")
folder2 := model.NewFolder(testLib, "TestGetLastUpdates/Folder2")
err := repo.Put(folder1)
Expect(err).ToNot(HaveOccurred())
err = repo.Put(folder2)
Expect(err).ToNot(HaveOccurred())
otherFolder := model.NewFolder(otherLib, "TestOtherLib/Folder")
err = repo.Put(otherFolder)
Expect(err).ToNot(HaveOccurred())
// Query all folders (no target paths) - should only return folders from testLib
results, err := repo.GetFolderUpdateInfo(testLib)
Expect(err).ToNot(HaveOccurred())
// Should include folders from testLib
Expect(results).To(HaveKey(folder1.ID))
Expect(results).To(HaveKey(folder2.ID))
// Should NOT include folders from other library
Expect(results).ToNot(HaveKey(otherFolder.ID))
})
})
Context("with specific target paths", func() {
It("returns folder info for existing folders", func() {
// Create test folders with unique names
folder1 := model.NewFolder(testLib, "TestSpecific/Rock")
folder2 := model.NewFolder(testLib, "TestSpecific/Jazz")
folder3 := model.NewFolder(testLib, "TestSpecific/Classical")
err := repo.Put(folder1)
Expect(err).ToNot(HaveOccurred())
err = repo.Put(folder2)
Expect(err).ToNot(HaveOccurred())
err = repo.Put(folder3)
Expect(err).ToNot(HaveOccurred())
// Query specific paths
results, err := repo.GetFolderUpdateInfo(testLib, "TestSpecific/Rock", "TestSpecific/Classical")
Expect(err).ToNot(HaveOccurred())
Expect(results).To(HaveLen(2))
// Verify folder IDs are in results
Expect(results).To(HaveKey(folder1.ID))
Expect(results).To(HaveKey(folder3.ID))
Expect(results).ToNot(HaveKey(folder2.ID))
// Verify update info is populated
Expect(results[folder1.ID].UpdatedAt).ToNot(BeZero())
Expect(results[folder1.ID].Hash).To(Equal(folder1.Hash))
})
It("includes all child folders when querying parent", func() {
tests.SkipOnWindows("path storage (#TBD-path-sep-persistence)")
// Create a parent folder with multiple children
parent := model.NewFolder(testLib, "TestParent/Music")
child1 := model.NewFolder(testLib, "TestParent/Music/Rock/Queen")
child2 := model.NewFolder(testLib, "TestParent/Music/Jazz")
otherParent := model.NewFolder(testLib, "TestParent2/Music/Jazz")
Expect(repo.Put(parent)).To(Succeed())
Expect(repo.Put(child1)).To(Succeed())
Expect(repo.Put(child2)).To(Succeed())
// Query the parent folder - should return parent and all children
results, err := repo.GetFolderUpdateInfo(testLib, "TestParent/Music")
Expect(err).ToNot(HaveOccurred())
Expect(results).To(HaveLen(3))
Expect(results).To(HaveKey(parent.ID))
Expect(results).To(HaveKey(child1.ID))
Expect(results).To(HaveKey(child2.ID))
Expect(results).ToNot(HaveKey(otherParent.ID))
})
It("excludes children from other libraries", func() {
tests.SkipOnWindows("path storage (#TBD-path-sep-persistence)")
// Create parent in testLib
parent := model.NewFolder(testLib, "TestIsolation/Parent")
child := model.NewFolder(testLib, "TestIsolation/Parent/Child")
Expect(repo.Put(parent)).To(Succeed())
Expect(repo.Put(child)).To(Succeed())
// Create similar path in other library
otherParent := model.NewFolder(otherLib, "TestIsolation/Parent")
otherChild := model.NewFolder(otherLib, "TestIsolation/Parent/Child")
Expect(repo.Put(otherParent)).To(Succeed())
Expect(repo.Put(otherChild)).To(Succeed())
// Query should only return folders from testLib
results, err := repo.GetFolderUpdateInfo(testLib, "TestIsolation/Parent")
Expect(err).ToNot(HaveOccurred())
Expect(results).To(HaveLen(2))
Expect(results).To(HaveKey(parent.ID))
Expect(results).To(HaveKey(child.ID))
Expect(results).ToNot(HaveKey(otherParent.ID))
Expect(results).ToNot(HaveKey(otherChild.ID))
})
It("excludes missing children when querying parent", func() {
tests.SkipOnWindows("path storage (#TBD-path-sep-persistence)")
// Create parent and children, mark one as missing
parent := model.NewFolder(testLib, "TestMissingChild/Parent")
child1 := model.NewFolder(testLib, "TestMissingChild/Parent/Child1")
child2 := model.NewFolder(testLib, "TestMissingChild/Parent/Child2")
child2.Missing = true
Expect(repo.Put(parent)).To(Succeed())
Expect(repo.Put(child1)).To(Succeed())
Expect(repo.Put(child2)).To(Succeed())
// Query parent - should only return parent and non-missing child
results, err := repo.GetFolderUpdateInfo(testLib, "TestMissingChild/Parent")
Expect(err).ToNot(HaveOccurred())
Expect(results).To(HaveLen(2))
Expect(results).To(HaveKey(parent.ID))
Expect(results).To(HaveKey(child1.ID))
Expect(results).ToNot(HaveKey(child2.ID))
})
It("handles mix of existing and non-existing target paths", func() {
tests.SkipOnWindows("path storage (#TBD-path-sep-persistence)")
// Create folders for one path but not the other
existingParent := model.NewFolder(testLib, "TestMixed/Exists")
existingChild := model.NewFolder(testLib, "TestMixed/Exists/Child")
Expect(repo.Put(existingParent)).To(Succeed())
Expect(repo.Put(existingChild)).To(Succeed())
// Query both existing and non-existing paths
results, err := repo.GetFolderUpdateInfo(testLib, "TestMixed/Exists", "TestMixed/DoesNotExist")
Expect(err).ToNot(HaveOccurred())
Expect(results).To(HaveLen(2))
Expect(results).To(HaveKey(existingParent.ID))
Expect(results).To(HaveKey(existingChild.ID))
})
It("handles empty folder path as root", func() {
// Test querying for root folder without creating it (fixtures should have one)
rootFolderID := model.FolderID(testLib, ".")
results, err := repo.GetFolderUpdateInfo(testLib, "")
Expect(err).ToNot(HaveOccurred())
// Should return the root folder if it exists
if len(results) > 0 {
Expect(results).To(HaveKey(rootFolderID))
}
})
It("returns empty map for non-existent folders", func() {
results, err := repo.GetFolderUpdateInfo(testLib, "NonExistent/Path")
Expect(err).ToNot(HaveOccurred())
Expect(results).To(BeEmpty())
})
It("skips missing folders", func() {
// Create a folder and mark it as missing
folder := model.NewFolder(testLib, "TestMissing/Folder")
folder.Missing = true
err := repo.Put(folder)
Expect(err).ToNot(HaveOccurred())
results, err := repo.GetFolderUpdateInfo(testLib, "TestMissing/Folder")
Expect(err).ToNot(HaveOccurred())
Expect(results).To(BeEmpty())
})
})
})
Describe("HasAudioOutsideFolders", func() {
var albumRoot, disc1, disc2 *model.Folder
// TestHasAudio/Album/
// ├── CD1/ (audio, belongs to the album)
// └── CD2/ (audio, belongs to the album)
BeforeEach(func() {
albumRoot = model.NewFolder(testLib, "TestHasAudio/Album")
disc1 = model.NewFolder(testLib, "TestHasAudio/Album/CD1")
disc1.NumAudioFiles = 5
disc2 = model.NewFolder(testLib, "TestHasAudio/Album/CD2")
disc2.NumAudioFiles = 5
for _, f := range []*model.Folder{albumRoot, disc1, disc2} {
Expect(repo.Put(f)).To(Succeed())
}
})
It("returns false when all audio under the parent belongs to the given folders", func() {
Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeFalse())
})
It("returns true when another folder under the parent has audio", func() {
bonus := model.NewFolder(testLib, "TestHasAudio/Album/Bonus")
bonus.NumAudioFiles = 1
Expect(repo.Put(bonus)).To(Succeed())
Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeTrue())
})
It("returns true when the parent itself contains audio files", func() {
albumRoot.NumAudioFiles = 2
Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeTrue())
})
It("ignores audio outside the parent's subtree", func() {
other := model.NewFolder(testLib, "TestHasAudio/Other Album")
other.NumAudioFiles = 10
Expect(repo.Put(other)).To(Succeed())
Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeFalse())
})
It("ignores missing folders", func() {
gone := model.NewFolder(testLib, "TestHasAudio/Album/Gone")
gone.NumAudioFiles = 3
gone.Missing = true
Expect(repo.Put(gone)).To(Succeed())
Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeFalse())
})
It("does not treat LIKE wildcards in the parent path as patterns", func() {
// "TestHas_udio" would LIKE-match "TestHasAudio" if "_" were not escaped
wildcardRoot := model.NewFolder(testLib, "TestHas_udio/Album")
Expect(repo.Put(wildcardRoot)).To(Succeed())
Expect(repo.HasAudioOutsideFolders(*wildcardRoot, []string{"none"})).To(BeFalse())
})
})
Describe("wrapFolderCursor", func() {
It("does not panic when the cursor yields a dbFolder with nil Folder", func() {
// Simulate what queryWithStableResults does on the rows.Err() path:
// it yields a zero-value dbFolder (where Folder is nil) with an error.
dbErr := fmt.Errorf("database is locked")
cursor := func(yield func(dbFolder, error) bool) {
var empty dbFolder // Folder pointer is nil
yield(empty, dbErr)
}
// wrapFolderCursor should handle the nil Folder without panicking
wrappedCursor := wrapFolderCursor(cursor)
var gotErr error
Expect(func() {
for _, err := range wrappedCursor {
gotErr = err
}
}).ToNot(Panic())
Expect(gotErr).To(HaveOccurred())
Expect(gotErr.Error()).To(ContainSubstring("unexpected nil model.Folder"))
Expect(errors.Is(gotErr, dbErr)).To(BeTrue(), "should wrap the original cursor error")
})
It("yields folders from a valid cursor", func() {
folder := &model.Folder{ID: "f1", Name: "Test"}
cursor := func(yield func(dbFolder, error) bool) {
yield(dbFolder{Folder: folder}, nil)
}
wrappedCursor := wrapFolderCursor(cursor)
var folders []model.Folder
for f, err := range wrappedCursor {
Expect(err).ToNot(HaveOccurred())
folders = append(folders, f)
}
Expect(folders).To(HaveLen(1))
Expect(folders[0].ID).To(Equal("f1"))
})
})
Describe("GetAllWithPlaylists", func() {
It("returns all non-missing folders with playlists, ignoring the scan-timestamp gate", func() {
withPls := model.NewFolder(testLib, "TestAllPls/WithPls")
withPls.NumPlaylists = 2
noPls := model.NewFolder(testLib, "TestAllPls/NoPls")
noPls.NumPlaylists = 0
missingWithPls := model.NewFolder(testLib, "TestAllPls/Missing")
missingWithPls.NumPlaylists = 1
missingWithPls.Missing = true
Expect(repo.Put(withPls)).To(Succeed())
Expect(repo.Put(noPls)).To(Succeed())
Expect(repo.Put(missingWithPls)).To(Succeed())
// Force the folder's updated_at to the past so GetTouchedWithPlaylists
// (which gates on updated_at > last_scan_at) would NOT return it.
_, err := conn.NewQuery("UPDATE folder SET updated_at = {:t} WHERE id = {:id}").
Bind(dbx.Params{"t": "2000-01-01 00:00:00", "id": withPls.ID}).Execute()
Expect(err).ToNot(HaveOccurred())
var ids []string
cursor, err := repo.GetAllWithPlaylists()
Expect(err).ToNot(HaveOccurred())
for f, err := range cursor {
Expect(err).ToNot(HaveOccurred())
ids = append(ids, f.ID)
}
Expect(ids).To(ConsistOf(withPls.ID)) // only the non-missing folder with playlists
})
})
})