fix(artwork): don't crash the server when a playlist's tracks can't be loaded (#6267)

Playlist().Tracks returns nil when its internal Get fails (for example when the
context is canceled at shutdown), and resolvePlaylist called GetAlbumIDs on it,
panicking with a nil pointer dereference. The artwork drain runs on a bare
goroutine, so the panic killed the whole server.

resolvePlaylist now returns an error when Tracks is nil, and the worker recovers
panics per item: it logs the panic with the item details and stack, and marks
the item as a failed attempt so the rest of the batch still runs.

Fixes #6266

core/artwork/resolve.go

@@ -374,8 +374,11 @@ func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (reso
 		}
 	}
 
-	albumIDs, err := r.ds.Playlist().Tracks(ctx, pl.ID, false).
-		GetAlbumIDs(ctx, model.QueryOptions{Max: PlaylistGridSamples, Sort: "random()"})
+	tracks := r.ds.Playlist().Tracks(ctx, pl.ID, false)
+	if tracks == nil {
+		return resolution{}, fmt.Errorf("resolvePlaylist: could not load tracks for playlist %s", pl.ID)
+	}
+	albumIDs, err := tracks.GetAlbumIDs(ctx, model.QueryOptions{Max: PlaylistGridSamples, Sort: "random()"})
 	if err != nil {
 		return resolution{}, err
 	}

core/artwork/resolve_test.go

@@ -707,6 +707,16 @@ var _ = Describe("resolveItem", func() {
 			Expect(err).To(HaveOccurred())
 			Expect(res).To(Equal(resolution{}))
 		})
+
+		It("returns an error when the playlist tracks cannot be loaded", func() {
+			plRepo := tests.CreateMockPlaylistRepo()
+			plRepo.SetData(model.Playlists{{ID: "pl4", Name: "Playlist"}})
+			ds.MockedPlaylist = plRepo
+
+			res, err := newResolver(ds, ag, ffm, nil).resolve(ctx, model.ArtworkQueueItem{ItemKind: "pl", ItemID: "pl4"})
+			Expect(err).To(HaveOccurred())
+			Expect(res).To(Equal(resolution{}))
+		})
 	})
 })
 

core/artwork/worker.go

@@ -4,9 +4,11 @@ import (
 	"bytes"
 	"cmp"
 	"context"
+	"fmt"
 	"io"
 	"math"
 	"math/rand/v2"
+	"runtime/debug"
 	"sync"
 	"time"
 
@@ -244,7 +246,7 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) (outc
 	item.ImageType = cmp.Or(item.ImageType, model.ImageTypePrimary)
 	trace := &ChainTrace{}
 	ctx = withTrace(ctx, trace)
-	out, got, retryIn := w.proc.acquire(ctx, item)
+	out, got, retryIn := w.safeAcquire(ctx, item)
 
 	queue := w.proc.ds.ArtworkQueue()
 	switch out {
@@ -286,6 +288,20 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) (outc
 	return out, got
 }
 
+// safeAcquire turns a panic into a failed attempt: the drain runs on a bare goroutine, so an
+// unrecovered panic would crash the server, and the still-queued row would crash it again on restart.
+func (w *Worker) safeAcquire(ctx context.Context, item model.ArtworkQueueItem) (out outcome, got *acquired, retryIn time.Duration) {
+	defer func() {
+		if r := recover(); r != nil {
+			log.Error(ctx, "Artwork: Panic while processing item", "kind", item.ItemKind, "id", item.ItemID,
+				"imageType", item.ImageType, "attempts", item.Attempts, "panic", r, "stack", string(debug.Stack()))
+			traceStage(ctx, "panic", fmt.Errorf("%v", r))
+			out, got, retryIn = outcomeFailed, nil, 0
+		}
+	}()
+	return w.proc.acquire(ctx, item)
+}
+
 // recordGiveUp keeps the last failure on the state row after the queue row is deleted. An item
 // that never resolved has no row to update, and creating one would settle it absent.
 func (w *Worker) recordGiveUp(ctx context.Context, item model.ArtworkQueueItem, trace string) {

core/artwork/worker_test.go

@@ -142,6 +142,18 @@ func (v *visibilityPlaylistRepo) Get(ctx context.Context, id string) (*model.Pla
 	return v.MockPlaylistRepo.Get(ctx, id)
 }
 
+type panickingAlbumRepo struct {
+	*tests.MockAlbumRepo
+	panicID string
+}
+
+func (r *panickingAlbumRepo) Get(ctx context.Context, id string) (*model.Album, error) {
+	if id == r.panicID {
+		panic("boom")
+	}
+	return r.MockAlbumRepo.Get(ctx, id)
+}
+
 func adminUserRepo() *tests.MockedUserRepo {
 	repo := tests.CreateMockUserRepo()
 	Expect(repo.Put(GinkgoT().Context(), &model.User{ID: "admin", UserName: "admin", IsAdmin: true})).To(Succeed())
@@ -278,6 +290,36 @@ var _ = Describe("Worker", func() {
 			Expect(err).To(MatchError(model.ErrNotFound), "a timeout must never settle on absent")
 		})
 
+		It("fails an item that panics, without stopping the rest of the batch", func() {
+			folderRepo.result = []model.Folder{{
+				Path:       "tests/fixtures/artist/an-album",
+				ImageFiles: []string{"cover.jpg"},
+			}}
+			albums := tests.CreateMockAlbumRepo()
+			albums.SetData(model.Albums{
+				{ID: "alboom", Name: "Album", FolderIDs: []string{"f1"}},
+				{ID: "alok", Name: "Album", FolderIDs: []string{"f1"}},
+			})
+			ds.MockedAlbum = &panickingAlbumRepo{MockAlbumRepo: albums, panicID: "alboom"}
+			Expect(queueRepo.Enqueue(ctx, model.ArtworkQueueItem{ItemKind: "al", ItemID: "alboom"})).To(Succeed())
+			Expect(queueRepo.Enqueue(ctx, model.ArtworkQueueItem{ItemKind: "al", ItemID: "alok"})).To(Succeed())
+
+			n, err := w.drain(ctx, 1)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(n).To(Equal(2))
+
+			it := findQueued(queueRepo, "al", "alboom")
+			Expect(it).ToNot(BeNil(), "a panicking item must be rescheduled, not dropped")
+			Expect(it.Attempts).To(Equal(1))
+			Expect(it.RetryAt).To(BeTemporally(">", time.Now()))
+			Expect(it.Trace).To(ContainSubstring("boom"))
+
+			Expect(findQueued(queueRepo, "al", "alok")).To(BeNil())
+			ia, err := artRepo.GetItemArtwork(ctx, model.KindAlbumArtwork, "alok", model.ImageTypePrimary)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(ia.Source).To(Equal("folder"))
+		})
+
 		It("reschedules past the provider's requested delay when it exceeds the backoff", func() {
 			conf.Server.CoverArtPriority = "external"
 			ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "al9", Name: "Album"}})

ui/src/playlist/PlaylistList.jsx

@@ -95,6 +95,7 @@ export const ToggleField = ({ resource, source }) => {
   return (
     <Switch
       checked={record[source]}
+      color="primary"
       onClick={handleClick}
       disabled={!isWritable(record.ownerId)}
     />

ui/src/playlist/PlaylistList.test.jsx

@@ -2,6 +2,7 @@ import React from 'react'
 import { render, screen } from '@testing-library/react'
 import { describe, it, expect, vi } from 'vitest'
 import { TestContext } from 'ra-test'
+import { RecordContextProvider } from 'react-admin'
 import { PlaylistLove, ToggleField, ToggleAutoImport } from './PlaylistList'
 
 vi.mock('../config', () => ({
@@ -14,6 +15,7 @@ vi.mock('../common', () => ({
       {record?.starred ? 'starred' : 'not-starred'}
     </button>
   ),
+  isWritable: (ownerId) => ownerId === 'me',
 }))
 
 describe('<PlaylistLove />', () => {
@@ -55,3 +57,28 @@ describe('playlist toggles without a record', () => {
     expect(container.innerHTML).toBe('')
   })
 })
+
+// Secondary is a surface color in many themes, so these toggles must use primary
+describe('<ToggleField />', () => {
+  const renderToggle = (record) =>
+    render(
+      <TestContext>
+        <RecordContextProvider value={record}>
+          <ToggleField resource="playlist" source="public" />
+        </RecordContextProvider>
+      </TestContext>,
+    )
+
+  it.each([
+    ['owner', 'me', false],
+    ['non-owner', 'someone-else', true],
+  ])('renders a primary-colored switch for the %s', (_, ownerId, disabled) => {
+    renderToggle({ id: 'pl-1', public: true, ownerId })
+    const input = screen.getByRole('checkbox')
+    const switchBase = input.closest('.MuiSwitch-switchBase')
+    expect(input.checked).toBe(true)
+    expect(input.disabled).toBe(disabled)
+    expect(switchBase.classList).toContain('MuiSwitch-colorPrimary')
+    expect(switchBase.classList).not.toContain('MuiSwitch-colorSecondary')
+  })
+})

ui/src/themes/dracula.js

@@ -185,16 +185,6 @@ export default {
         color: `${foreground} !important`,
       },
     },
-    MuiSwitch: {
-      colorSecondary: {
-        '&$checked': {
-          color: green,
-        },
-        '&$checked + $track': {
-          backgroundColor: green,
-        },
-      },
-    },
     NDAlbumGridView: {
       albumName: {
         marginTop: '0.5rem',

ui/src/themes/gruvboxDark.js

@@ -121,16 +121,6 @@ export default {
         boxShadow: '3px 3px 5px #3c3836',
       },
     },
-    MuiSwitch: {
-      colorSecondary: {
-        '&$checked': {
-          color: '#458588',
-        },
-        '&$checked + $track': {
-          backgroundColor: '#458588',
-        },
-      },
-    },
     NDMobileArtistDetails: {
       bgContainer: {
         background:

ui/src/themes/tokyoNight.js

@@ -184,16 +184,6 @@ export default {
         color: `${foreground} !important`,
       },
     },
-    MuiSwitch: {
-      colorSecondary: {
-        '&$checked': {
-          color: blue,
-        },
-        '&$checked + $track': {
-          backgroundColor: blue,
-        },
-      },
-    },
     NDAlbumGridView: {
       albumName: {
         marginTop: '0.5rem',

ui/src/themes/tokyoNightLight.js

@@ -184,16 +184,6 @@ export default {
         color: `${foreground} !important`,
       },
     },
-    MuiSwitch: {
-      colorSecondary: {
-        '&$checked': {
-          color: blue,
-        },
-        '&$checked + $track': {
-          backgroundColor: blue,
-        },
-      },
-    },
     NDAlbumGridView: {
       albumName: {
         marginTop: '0.5rem',

ui/src/themes/useCurrentTheme.js

@@ -63,6 +63,8 @@ const useCurrentTheme = () => {
         ...theme.props,
         MuiUseMediaQuery: { noSsr: true },
         MuiPopover: { disableScrollLock: true },
+        // MUI defaults to secondary, which many themes use as a surface color
+        MuiSwitch: { color: 'primary' },
       },
     }),
     [theme],

ui/src/themes/useCurrentTheme.test.jsx

@@ -3,6 +3,10 @@ import { Provider } from 'react-redux'
 import { createStore } from 'redux'
 import mediaQuery from 'css-mediaquery'
 import { renderHook } from '@testing-library/react-hooks'
+import { render, screen } from '@testing-library/react'
+import { createMuiTheme, ThemeProvider } from '@material-ui/core/styles'
+import Switch from '@material-ui/core/Switch'
+import themes from './index'
 import useCurrentTheme from './useCurrentTheme'
 import { themeReducer } from '../reducers/themeReducer'
 import { AUTO_THEME_ID } from '../consts'
@@ -161,4 +165,27 @@ describe('useCurrentTheme', () => {
       expect(document.body.style.backgroundColor).toBe('rgb(18, 18, 18)')
     })
   })
+  describe('switch color', () => {
+    it.each(Object.keys(themes))(
+      'renders switches with the primary color in %s',
+      (theme) => {
+        const { result } = renderHook(() => useCurrentTheme(), {
+          wrapper: ({ children }) => (
+            <Provider store={createStore(themeReducer, { theme })}>
+              {children}
+            </Provider>
+          ),
+        })
+        render(
+          <ThemeProvider theme={createMuiTheme(result.current)}>
+            <Switch checked onChange={() => {}} />
+          </ThemeProvider>,
+        )
+        const switchBase = screen
+          .getByRole('checkbox')
+          .closest('.MuiSwitch-switchBase')
+        expect(switchBase.classList).toContain('MuiSwitch-colorPrimary')
+      },
+    )
+  })
 })