From 516413f4ca5474b9b762acd67b2ee47c363912e4 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 11 Sep 2026 20:15:35 -0400 Subject: [PATCH] fix(admin): re-acquisition settings take effect without a restart, and say why a save was refused (#3936, #3937) #3936: Router() built a reacquisition.SettingsService of its own, so a save from the admin card refreshed that instance's cache while the sweeper in main.go kept serving what it loaded at boot. The card showed the new policy, the feature ran the old one, and only a restart reconciled them. main.go now hands its instance to the server (srv.ReacqSettings), as it already did for RecSettings, TagSettings and FingerprintSettings, and Router() constructs one only when that field is nil. The regression test saves through the router and reads the sweeper's instance. #3937: the card's catch tested `e instanceof Error`, but api.put throws a plain {code, message, status} object, so every reason the server gave was discarded in favour of "Couldn't save settings." It now uses errMessage, which appends the server's message for invalid_setting. Its test rejected with an Error no code path produces, so it passed throughout; it now rejects with what the client actually throws. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- cmd/minstrel/main.go | 3 + internal/server/server.go | 24 ++-- internal/server/server_test.go | 106 ++++++++++++++++++ .../ReacquisitionSettingsCard.svelte | 7 +- .../ReacquisitionSettingsCard.test.ts | 17 ++- 5 files changed, 144 insertions(+), 13 deletions(-) diff --git a/cmd/minstrel/main.go b/cmd/minstrel/main.go index ed6dd522..d447a7bf 100644 --- a/cmd/minstrel/main.go +++ b/cmd/minstrel/main.go @@ -377,6 +377,9 @@ func run() error { srv.RecSettings = recSettings srv.TagSettings = tagSettings srv.FingerprintSettings = fpSettings + // The sweeper above holds this same instance, so a save from the admin + // card changes what it does on its next tick (#3936). + srv.ReacqSettings = reacqSettings srv.StreamSecret = cfg.StreamSecret httpServer := &http.Server{ Addr: cfg.Server.Address, diff --git a/internal/server/server.go b/internal/server/server.go index c7eafbac..94c59ddd 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -105,6 +105,11 @@ type Server struct { // fingerprint workers, so a save from the admin card reaches them without a // restart. Router() constructs a fallback when nil (tests). FingerprintSettings *library.FingerprintSettingsService + // ReacqSettings is the DB-backed missing-file re-acquisition policy + // (milestone #290) — the same instance the sweeper in cmd/minstrel/main.go + // reads, so a save from the admin card reaches it without a restart + // (#3936). Router() constructs a fallback when nil (tests). + ReacqSettings *reacquisition.SettingsService // StreamSecret is the HMAC key used by /api/cast/stream-token to // mint signed UPnP / Sonos stream URLs and by /api/tracks/{id}/stream // to verify them. Sourced from config.Config.StreamSecret. Tests that @@ -157,13 +162,18 @@ func (s *Server) Router() http.Handler { return lidarr.NewClient(cfg.BaseURL, cfg.APIKey) } lidarrReqs := lidarrrequests.NewService(s.Pool, lidarrCfg, lidarrClientFn, nil) - // Always usable even when the load fails — it falls back to the - // shipped defaults rather than leaving the admin card unable to - // render (same posture as netsettings above). - reacqSettings, raErr := reacquisition.NewSettingsService( - context.Background(), s.Pool, s.Logger) - if raErr != nil { - s.Logger.Warn("reacquisition settings unavailable; serving defaults", "err", raErr) + reacqSettings := s.ReacqSettings + if reacqSettings == nil { + // Test contexts construct Server without main.go's boot wiring. + // Always usable even when the load fails — it falls back to the + // shipped defaults rather than leaving the admin card unable to + // render (same posture as netsettings above). + var raErr error + reacqSettings, raErr = reacquisition.NewSettingsService( + context.Background(), s.Pool, s.Logger) + if raErr != nil { + s.Logger.Warn("reacquisition settings unavailable; serving defaults", "err", raErr) + } } lidarrQuar := lidarrquarantine.NewService(s.Pool, lidarrCfg, lidarrClientFn, s.DataDir) tracksSvc := tracks.NewService(s.Pool, s.Logger, lidarrUnmonitorAdapter{fn: lidarrClientFn}, s.DataDir) diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 22e352a3..29c0eaf1 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -1,6 +1,7 @@ package server import ( + "bytes" "context" "encoding/json" "io" @@ -20,6 +21,7 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/db" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/library" + "git.fabledsword.com/bvandeusen/minstrel/internal/reacquisition" "git.fabledsword.com/bvandeusen/minstrel/internal/subsonic" ) @@ -242,6 +244,110 @@ func TestRouter_AdminSubtreeNotShadowed(t *testing.T) { } } +// TestRouter_ReacquisitionSettingsSavedThroughTheAPIReachTheSweeper is a +// regression test for #3936. Router() used to construct a +// reacquisition.SettingsService of its own, so a save from the admin card +// refreshed THAT instance's cache while the sweeper in cmd/minstrel/main.go +// kept serving what it had loaded at boot. The card showed the new policy, the +// feature kept running the old one, and only a restart reconciled them — the +// exact thing rule 25 says a setting must not need. +// +// The assertion is made against the instance main.go hands the sweeper: save +// through the router, then read that instance. A second service leaves it stale. +func TestRouter_ReacquisitionSettingsSavedThroughTheAPIReachTheSweeper(t *testing.T) { + dsn := os.Getenv("MINSTREL_TEST_DATABASE_URL") + if dsn == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + if err := db.Migrate(dsn, logger); err != nil { + t.Fatalf("migrate: %v", err) + } + pool, err := pgxpool.New(context.Background(), dsn) + if err != nil { + t.Fatalf("pool: %v", err) + } + t.Cleanup(pool.Close) + + ctx := context.Background() + q := dbq.New(pool) + _, _ = pool.Exec(ctx, "DELETE FROM sessions WHERE user_agent = 'reacq-settings-test'") + _, _ = pool.Exec(ctx, "DELETE FROM users WHERE username = 'test-reacq-settings-admin'") + user, err := q.CreateUser(ctx, dbq.CreateUserParams{ + Username: "test-reacq-settings-admin", + PasswordHash: "x", + ApiToken: "test-reacq-settings-token", + IsAdmin: true, + }) + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + t.Cleanup(func() { _, _ = pool.Exec(ctx, "DELETE FROM users WHERE id = $1", user.ID) }) + token := "reacq-settings-test-" + time.Now().Format("20060102150405.000000") + tokenHash := auth.HashSessionToken(token) + if _, err := pool.Exec(ctx, + "INSERT INTO sessions (user_id, token_hash, user_agent) VALUES ($1, $2, 'reacq-settings-test')", + user.ID, tokenHash[:], + ); err != nil { + t.Fatalf("insert session: %v", err) + } + + // The sweeper's service. Nothing else in the process may write to the + // settings for the assertion below to mean what it says. + sweeperSettings, err := reacquisition.NewSettingsService(ctx, pool, logger) + if err != nil { + t.Fatalf("reacquisition settings: %v", err) + } + before := sweeperSettings.Get() + t.Cleanup(func() { + if _, err := sweeperSettings.Set(context.Background(), before); err != nil { + t.Errorf("restore reacquisition settings: %v", err) + } + }) + wantGrace := before.GraceHours + 1 + if wantGrace > 720 { + wantGrace = before.GraceHours - 1 + } + + s := New(logger, pool, stubScanner{}, subsonic.Config{}, config.EventsConfig{}, + config.RecommendationConfig{}, "", config.BrandingConfig{}, nil, nil, nil, library.RunScanConfig{}) + s.ReacqSettings = sweeperSettings + ts := httptest.NewServer(s.Router()) + defer ts.Close() + + body, err := json.Marshal(map[string]any{ + "enabled": before.Enabled, + "grace_hours": wantGrace, + "backoff_base_hours": before.BackoffBaseHours, + "backoff_max_hours": before.BackoffMaxHours, + "max_attempts": before.MaxAttempts, + "max_per_pass": before.MaxPerPass, + "auto_approve": before.AutoApprove, + }) + if err != nil { + t.Fatalf("marshal: %v", err) + } + req, err := http.NewRequest(http.MethodPut, ts.URL+"/api/admin/library/reacquisition", bytes.NewReader(body)) + if err != nil { + t.Fatalf("build request: %v", err) + } + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Content-Type", "application/json") + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("PUT reacquisition settings: %v", err) + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusOK { + t.Fatalf("PUT reacquisition settings: status = %d, want 200", resp.StatusCode) + } + + if got := sweeperSettings.Get().GraceHours; got != wantGrace { + t.Fatalf("the sweeper's settings hold grace_hours = %d after the save, want %d — "+ + "the API wrote through a different service instance", got, wantGrace) + } +} + // stubScanner is a no-op ScanTrigger used only to make Server.Router() // register /api/admin/scan. Its Scan method must never be called by the // route-presence assertions in this file. diff --git a/web/src/lib/components/ReacquisitionSettingsCard.svelte b/web/src/lib/components/ReacquisitionSettingsCard.svelte index 9f37b1dc..2ae8c00a 100644 --- a/web/src/lib/components/ReacquisitionSettingsCard.svelte +++ b/web/src/lib/components/ReacquisitionSettingsCard.svelte @@ -6,6 +6,7 @@ updateReacquisitionSettings, type ReacquisitionSettings } from '$lib/api/admin'; + import { errMessage } from '$lib/api/errors'; import { pushToast } from '$lib/stores/toast.svelte'; // Policy for turning a missing file back into a Lidarr request @@ -60,8 +61,10 @@ } catch (e) { // The server validates the same ranges the database CHECKs enforce and // names the offending field, so surface its message rather than a - // generic failure. - pushToast(e instanceof Error ? e.message : "Couldn't save settings.", 'error'); + // generic failure. errMessage, not `e.message`: the API client throws a + // plain {code, message} object, never an Error, so an instanceof check + // here silently discarded every reason the server gave (#3937). + pushToast(errMessage(e), 'error'); } finally { saving = false; } diff --git a/web/src/lib/components/ReacquisitionSettingsCard.test.ts b/web/src/lib/components/ReacquisitionSettingsCard.test.ts index 7cfea1dd..2ede3ea9 100644 --- a/web/src/lib/components/ReacquisitionSettingsCard.test.ts +++ b/web/src/lib/components/ReacquisitionSettingsCard.test.ts @@ -1,6 +1,7 @@ import { afterEach, describe, expect, test, vi } from 'vitest'; import { render, screen, fireEvent, waitFor } from '@testing-library/svelte'; import type { ReacquisitionSettings } from '$lib/api/admin'; +import { ERROR_COPY } from '$lib/api/error-copy'; vi.mock('$lib/api/admin', () => ({ getReacquisitionSettings: vi.fn(), @@ -80,10 +81,18 @@ describe('ReacquisitionSettingsCard', () => { // The server names the offending field ("grace_hours must be 1-720"); a // generic "couldn't save" would throw that away. + // + // Rejects with what api.put actually throws — a plain {code, message, status} + // object, not an Error. The old version of this test rejected with an Error, + // which no code path produces, and so passed while the card was discarding + // every server message it was handed (#3937). test('a rejected save surfaces the server message', async () => { - vi.mocked(updateReacquisitionSettings).mockRejectedValue( - new Error('grace_hours must be 1-720') - ); + const message = 'grace_hours must be 1-720'; + vi.mocked(updateReacquisitionSettings).mockRejectedValue({ + code: 'invalid_setting', + message, + status: 400 + }); await renderCard(); const grace = screen.getByRole('spinbutton', { name: /wait before the first attempt/i }); @@ -91,7 +100,7 @@ describe('ReacquisitionSettingsCard', () => { await fireEvent.click(await screen.findByRole('button', { name: /save/i })); await waitFor(() => - expect(pushToast).toHaveBeenCalledWith('grace_hours must be 1-720', 'error') + expect(pushToast).toHaveBeenCalledWith(`${ERROR_COPY.invalid_setting} ${message}`, 'error') ); });