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') ); });