fix(scanner): add nil guards to cursor wrapping (#5139)
* fix(persistence): add nil guards to cursor wrapping in folder and mediafile repos Prevent SIGSEGV panic when queryWithStableResults yields a zero-value struct on the rows.Err() path (e.g., "database is locked" during concurrent scanning). Extract cursor wrapping into wrapFolderCursor and wrapMediaFileCursor with nil checks matching the existing pattern in album_repository.go. Fixes #5138 * fix(persistence): wrap original cursor error in nil guard messages Use %w to preserve the underlying error (e.g., "database is locked") so callers can use errors.Is/As for root cause analysis. Tests now verify the original error is accessible via errors.Is. * fix(persistence): add nil guards and error wrapping in album, folder, and mediafile cursor functions Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
This commit is contained in:
@@ -4,6 +4,7 @@ import (
|
|||||||
"context"
|
"context"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
"fmt"
|
"fmt"
|
||||||
|
"iter"
|
||||||
"maps"
|
"maps"
|
||||||
"slices"
|
"slices"
|
||||||
"strings"
|
"strings"
|
||||||
@@ -302,17 +303,21 @@ func (r *albumRepository) GetTouchedAlbums(libID int) (model.AlbumCursor, error)
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
|
return wrapAlbumCursor(cursor), nil
|
||||||
|
}
|
||||||
|
|
||||||
|
func wrapAlbumCursor(cursor iter.Seq2[dbAlbum, error]) model.AlbumCursor {
|
||||||
return func(yield func(model.Album, error) bool) {
|
return func(yield func(model.Album, error) bool) {
|
||||||
for a, err := range cursor {
|
for a, err := range cursor {
|
||||||
if a.Album == nil {
|
if a.Album == nil {
|
||||||
yield(model.Album{}, fmt.Errorf("unexpected nil album: %v", a))
|
yield(model.Album{}, fmt.Errorf("unexpected nil album (%v): %w", a, err))
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
if !yield(*a.Album, err) || err != nil {
|
if !yield(*a.Album, err) || err != nil {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}, nil
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// RefreshPlayCounts updates the play count and last play date annotations for all albums, based
|
// RefreshPlayCounts updates the play count and last play date annotations for all albums, based
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
package persistence
|
package persistence
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
@@ -743,6 +744,46 @@ var _ = Describe("AlbumRepository", func() {
|
|||||||
_, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": album.ID}))
|
_, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": album.ID}))
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
|
Describe("wrapAlbumCursor", func() {
|
||||||
|
It("does not panic when the cursor yields a dbAlbum with nil Album", func() {
|
||||||
|
// Simulate what queryWithStableResults does on the rows.Err() path:
|
||||||
|
// it yields a zero-value dbAlbum (where Album is nil) with an error.
|
||||||
|
dbErr := fmt.Errorf("database is locked")
|
||||||
|
cursor := func(yield func(dbAlbum, error) bool) {
|
||||||
|
var empty dbAlbum // Album pointer is nil
|
||||||
|
yield(empty, dbErr)
|
||||||
|
}
|
||||||
|
|
||||||
|
// wrapAlbumCursor should handle the nil Album without panicking
|
||||||
|
wrappedCursor := wrapAlbumCursor(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 album"))
|
||||||
|
Expect(errors.Is(gotErr, dbErr)).To(BeTrue(), "should wrap the original cursor error")
|
||||||
|
})
|
||||||
|
|
||||||
|
It("yields albums from a valid cursor", func() {
|
||||||
|
album := &model.Album{ID: "a1", Name: "Test"}
|
||||||
|
cursor := func(yield func(dbAlbum, error) bool) {
|
||||||
|
yield(dbAlbum{Album: album}, nil)
|
||||||
|
}
|
||||||
|
|
||||||
|
wrappedCursor := wrapAlbumCursor(cursor)
|
||||||
|
var albums []model.Album
|
||||||
|
for a, err := range wrappedCursor {
|
||||||
|
Expect(err).ToNot(HaveOccurred())
|
||||||
|
albums = append(albums, a)
|
||||||
|
}
|
||||||
|
Expect(albums).To(HaveLen(1))
|
||||||
|
Expect(albums[0].ID).To(Equal("a1"))
|
||||||
|
})
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
func _p(id, name string, sortName ...string) model.Participant {
|
func _p(id, name string, sortName ...string) model.Participant {
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ import (
|
|||||||
"context"
|
"context"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
"fmt"
|
"fmt"
|
||||||
|
"iter"
|
||||||
"maps"
|
"maps"
|
||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
@@ -218,13 +219,21 @@ func (r folderRepository) GetTouchedWithPlaylists() (model.FolderCursor, error)
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
|
return wrapFolderCursor(cursor), nil
|
||||||
|
}
|
||||||
|
|
||||||
|
func wrapFolderCursor(cursor iter.Seq2[dbFolder, error]) model.FolderCursor {
|
||||||
return func(yield func(model.Folder, error) bool) {
|
return func(yield func(model.Folder, error) bool) {
|
||||||
for f, err := range cursor {
|
for f, err := range cursor {
|
||||||
|
if f.Folder == nil {
|
||||||
|
yield(model.Folder{}, fmt.Errorf("unexpected nil folder (%v): %w", f, err))
|
||||||
|
return
|
||||||
|
}
|
||||||
if !yield(*f.Folder, err) || err != nil {
|
if !yield(*f.Folder, err) || err != nil {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}, nil
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
func (r folderRepository) purgeEmpty(libraryIDs ...int) error {
|
func (r folderRepository) purgeEmpty(libraryIDs ...int) error {
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ package persistence
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
|
|
||||||
"github.com/navidrome/navidrome/log"
|
"github.com/navidrome/navidrome/log"
|
||||||
@@ -210,4 +211,44 @@ var _ = Describe("FolderRepository", func() {
|
|||||||
})
|
})
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
|
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 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"))
|
||||||
|
})
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -3,6 +3,7 @@ package persistence
|
|||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
"fmt"
|
"fmt"
|
||||||
|
"iter"
|
||||||
"slices"
|
"slices"
|
||||||
"strconv"
|
"strconv"
|
||||||
"strings"
|
"strings"
|
||||||
@@ -231,17 +232,7 @@ func (r *mediaFileRepository) GetCursor(options ...model.QueryOptions) (model.Me
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
return func(yield func(model.MediaFile, error) bool) {
|
return wrapMediaFileCursor(cursor), nil
|
||||||
for m, err := range cursor {
|
|
||||||
if m.MediaFile == nil {
|
|
||||||
yield(model.MediaFile{}, fmt.Errorf("unexpected nil mediafile: %v", m))
|
|
||||||
return
|
|
||||||
}
|
|
||||||
if !yield(*m.MediaFile, err) || err != nil {
|
|
||||||
return
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}, nil
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// FindByPaths finds media files by their paths.
|
// FindByPaths finds media files by their paths.
|
||||||
@@ -371,13 +362,21 @@ func (r *mediaFileRepository) GetMissingAndMatching(libId int) (model.MediaFileC
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
|
return wrapMediaFileCursor(cursor), nil
|
||||||
|
}
|
||||||
|
|
||||||
|
func wrapMediaFileCursor(cursor iter.Seq2[dbMediaFile, error]) model.MediaFileCursor {
|
||||||
return func(yield func(model.MediaFile, error) bool) {
|
return func(yield func(model.MediaFile, error) bool) {
|
||||||
for m, err := range cursor {
|
for m, err := range cursor {
|
||||||
|
if m.MediaFile == nil {
|
||||||
|
yield(model.MediaFile{}, fmt.Errorf("unexpected nil mediafile (%v): %w", m, err))
|
||||||
|
return
|
||||||
|
}
|
||||||
if !yield(*m.MediaFile, err) || err != nil {
|
if !yield(*m.MediaFile, err) || err != nil {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}, nil
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// FindRecentFilesByMBZTrackID finds recently added files by MusicBrainz Track ID in other libraries
|
// FindRecentFilesByMBZTrackID finds recently added files by MusicBrainz Track ID in other libraries
|
||||||
|
|||||||
@@ -2,6 +2,8 @@ package persistence
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
|
"errors"
|
||||||
|
"fmt"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
"github.com/Masterminds/squirrel"
|
"github.com/Masterminds/squirrel"
|
||||||
@@ -711,4 +713,44 @@ var _ = Describe("MediaRepository", func() {
|
|||||||
Expect(results).To(BeEmpty())
|
Expect(results).To(BeEmpty())
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
|
Describe("wrapMediaFileCursor", func() {
|
||||||
|
It("does not panic when the cursor yields a dbMediaFile with nil MediaFile", func() {
|
||||||
|
// Simulate what queryWithStableResults does on the rows.Err() path:
|
||||||
|
// it yields a zero-value dbMediaFile (where MediaFile is nil) with an error.
|
||||||
|
dbErr := fmt.Errorf("database is locked")
|
||||||
|
cursor := func(yield func(dbMediaFile, error) bool) {
|
||||||
|
var empty dbMediaFile // MediaFile pointer is nil
|
||||||
|
yield(empty, dbErr)
|
||||||
|
}
|
||||||
|
|
||||||
|
// wrapMediaFileCursor should handle the nil MediaFile without panicking
|
||||||
|
wrappedCursor := wrapMediaFileCursor(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 mediafile"))
|
||||||
|
Expect(errors.Is(gotErr, dbErr)).To(BeTrue(), "should wrap the original cursor error")
|
||||||
|
})
|
||||||
|
|
||||||
|
It("yields mediafiles from a valid cursor", func() {
|
||||||
|
mf := &model.MediaFile{ID: "mf1", Title: "Test"}
|
||||||
|
cursor := func(yield func(dbMediaFile, error) bool) {
|
||||||
|
yield(dbMediaFile{MediaFile: mf}, nil)
|
||||||
|
}
|
||||||
|
|
||||||
|
wrappedCursor := wrapMediaFileCursor(cursor)
|
||||||
|
var mediafiles []model.MediaFile
|
||||||
|
for m, err := range wrappedCursor {
|
||||||
|
Expect(err).ToNot(HaveOccurred())
|
||||||
|
mediafiles = append(mediafiles, m)
|
||||||
|
}
|
||||||
|
Expect(mediafiles).To(HaveLen(1))
|
||||||
|
Expect(mediafiles[0].ID).To(Equal("mf1"))
|
||||||
|
})
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
Reference in New Issue
Block a user