diff --git a/cmd/minstrel/main.go b/cmd/minstrel/main.go index 6ad3e0ec..ed6dd522 100644 --- a/cmd/minstrel/main.go +++ b/cmd/minstrel/main.go @@ -122,7 +122,15 @@ func run() error { } defer pool.Close() - scanner := library.New(pool, logger, cfg.Library.ScanPaths) + // Fingerprinting settings (M400 #3913): one instance, shared by the scanner, + // the fingerprint backfill, the duplicate sweep and the admin API, so a save + // reaches all of them without a restart. A load failure is logged, not fatal: + // the service falls back to the shipped defaults. + fpSettings, fpErr := library.NewFingerprintSettingsService(ctx, pool) + if fpErr != nil { + logger.Warn("fingerprint settings: using defaults", "err", fpErr) + } + scanner := library.New(pool, logger, cfg.Library.ScanPaths, fpSettings) contact := cfg.Library.ContactEmail if contact == "" { @@ -218,12 +226,12 @@ func run() error { // will — everything imported before fingerprinting existed, and rows derived // by an older method. A worker of its own rather than a scan stage; see // internal/library/fingerprint_backfill.go for why. - go library.NewFingerprintBackfillWorker(pool, logger.With("component", "fingerprint_backfill")).Run(ctx) + go library.NewFingerprintBackfillWorker(pool, logger.With("component", "fingerprint_backfill"), fpSettings).Run(ctx) // Duplicate sweep (M400 #3910): proposes groups of tracks holding one // recording, from the fingerprints above. Sweeps only when fingerprints have // changed since the last sweep. - go library.NewDuplicateSweepWorker(pool, logger.With("component", "duplicate_sweep")).Run(ctx) + go library.NewDuplicateSweepWorker(pool, logger.With("component", "duplicate_sweep"), fpSettings).Run(ctx) // Start the tag-enrichment worker (#1490). Reconciles the compiled-in // tag providers with tag_provider_settings, bumps the sources version if @@ -368,6 +376,7 @@ func run() error { srv.PlaylistScheduler = playlistScheduler srv.RecSettings = recSettings srv.TagSettings = tagSettings + srv.FingerprintSettings = fpSettings srv.StreamSecret = cfg.StreamSecret httpServer := &http.Server{ Addr: cfg.Server.Address, diff --git a/internal/api/admin_coverage.go b/internal/api/admin_coverage.go index 3c5e0d58..7bbe259c 100644 --- a/internal/api/admin_coverage.go +++ b/internal/api/admin_coverage.go @@ -1,6 +1,7 @@ package api import ( + "context" "net/http" "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" @@ -40,12 +41,32 @@ func (h *handlers) handleGetLibraryCoverage(w http.ResponseWriter, r *http.Reque // fingerprintCoverageResp is the wire shape for GET /api/admin/library/fingerprints. // fingerprinted + rejected + pending = total. Missing tracks are not counted: -// there is no file to fingerprint. +// there is no file to fingerprint. Enabled travels with the counts because with +// fingerprinting off (#3913) pending never shrinks, and a gauge that implies +// progress would be promising work nothing is doing. type fingerprintCoverageResp struct { Total int64 `json:"total"` Fingerprinted int64 `json:"fingerprinted"` Rejected int64 `json:"rejected"` Pending int64 `json:"pending"` + Enabled bool `json:"enabled"` +} + +// fingerprintCoverage reads the gauge against the current settings: a print at +// another length counts as pending, because the backfill will re-derive it. +func (h *handlers) fingerprintCoverage(ctx context.Context) (fingerprintCoverageResp, error) { + cfg := h.fingerprintSettings.Get() + row, err := library.FingerprintCoverage(ctx, h.pool, cfg) + if err != nil { + return fingerprintCoverageResp{}, err + } + return fingerprintCoverageResp{ + Total: row.Total, + Fingerprinted: row.Fingerprinted, + Rejected: row.Rejected, + Pending: row.Pending, + Enabled: cfg.Enabled, + }, nil } // handleGetFingerprintCoverage implements GET /api/admin/library/fingerprints: @@ -53,15 +74,10 @@ type fingerprintCoverageResp struct { // worker spanning many passes, with no scan run to attach a tally to, so its // progress is read live here. Always 200; zeros on an empty library. func (h *handlers) handleGetFingerprintCoverage(w http.ResponseWriter, r *http.Request) { - row, err := library.FingerprintCoverage(r.Context(), h.pool) + cov, err := h.fingerprintCoverage(r.Context()) if err != nil { writeErrWithLog(w, h.logger, "admin: get fingerprint coverage", apierror.InternalMsg("lookup failed", err)) return } - writeJSON(w, http.StatusOK, fingerprintCoverageResp{ - Total: row.Total, - Fingerprinted: row.Fingerprinted, - Rejected: row.Rejected, - Pending: row.Pending, - }) + writeJSON(w, http.StatusOK, cov) } diff --git a/internal/api/admin_duplicates.go b/internal/api/admin_duplicates.go index db319b3a..cbe2809d 100644 --- a/internal/api/admin_duplicates.go +++ b/internal/api/admin_duplicates.go @@ -95,7 +95,7 @@ func (h *handlers) handleListDuplicates(w http.ResponseWriter, r *http.Request) return } - cov, err := library.FingerprintCoverage(ctx, h.pool) + cov, err := h.fingerprintCoverage(ctx) if err != nil { h.logger.Error("admin: fingerprint coverage", "err", err) writeAdminJSONErr(w, http.StatusInternalServerError, "server_error") @@ -117,14 +117,12 @@ func (h *handlers) handleListDuplicates(w http.ResponseWriter, r *http.Request) } writeJSON(w, http.StatusOK, adminDuplicatesResponse{ - Sweep: sweep, - Fingerprints: fingerprintCoverageResp{ - Total: cov.Total, Fingerprinted: cov.Fingerprinted, Rejected: cov.Rejected, Pending: cov.Pending, - }, - Total: total, - Limit: limit, - Offset: offset, - Groups: foldDuplicateGroups(rows), + Sweep: sweep, + Fingerprints: cov, + Total: total, + Limit: limit, + Offset: offset, + Groups: foldDuplicateGroups(rows), }) } @@ -194,8 +192,10 @@ func foldDuplicateGroups(rows []dbq.ListPendingDuplicateGroupMembersRow) []dupli // The sweep outlives the request, so it runs on a background context, as // handleTriggerScan's scan does. func (h *handlers) handleRunDuplicateSweep(w http.ResponseWriter, _ *http.Request) { + // Runs whatever the sweep interval says: the interval paces the automatic + // sweep, and an operator pressing the button has already decided. started, err := library.TryStartDuplicateSweep( - context.Background(), h.pool, h.logger.With("source", "manual"), + context.Background(), h.pool, h.logger.With("source", "manual"), h.fingerprintSettings.Get(), ) if err != nil { h.logger.Error("admin: start duplicate sweep", "err", err) diff --git a/internal/api/admin_fingerprint_settings.go b/internal/api/admin_fingerprint_settings.go new file mode 100644 index 00000000..bf049b68 --- /dev/null +++ b/internal/api/admin_fingerprint_settings.go @@ -0,0 +1,67 @@ +package api + +import ( + "encoding/json" + "errors" + "net/http" + + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" + "git.fabledsword.com/bvandeusen/minstrel/internal/library" +) + +// fingerprintSettingsBody is the wire shape for GET and PUT +// /api/admin/library/fingerprint-settings (M400 #3913). The threshold travels as +// the bit-error rate the matcher uses; the card presents it as a match percentage. +type fingerprintSettingsBody struct { + Enabled bool `json:"enabled"` + ChromaprintLengthSec int32 `json:"chromaprint_length_sec"` + AcousticMaxBitErrorRate float64 `json:"acoustic_max_bit_error_rate"` + BackfillConcurrency int32 `json:"backfill_concurrency"` + SweepIntervalHours int32 `json:"sweep_interval_hours"` +} + +func fingerprintSettingsBodyOf(s library.FingerprintSettings) fingerprintSettingsBody { + return fingerprintSettingsBody{ + Enabled: s.Enabled, + ChromaprintLengthSec: s.ChromaprintLengthSec, + AcousticMaxBitErrorRate: s.AcousticMaxBitErrorRate, + BackfillConcurrency: s.BackfillConcurrency, + SweepIntervalHours: s.SweepIntervalHours, + } +} + +// handleGetFingerprintSettings implements GET /api/admin/library/fingerprint-settings. +func (h *handlers) handleGetFingerprintSettings(w http.ResponseWriter, _ *http.Request) { + writeJSON(w, http.StatusOK, fingerprintSettingsBodyOf(h.fingerprintSettings.Get())) +} + +// handleUpdateFingerprintSettings implements PUT /api/admin/library/fingerprint-settings. +// +// A whole-row write. A body that leaves a field out decodes it as zero, which no +// field accepts, so a partial save is refused rather than zeroing what it omitted. +// The saved settings reach the scanner and both workers at once: they share the +// service instance. +func (h *handlers) handleUpdateFingerprintSettings(w http.ResponseWriter, r *http.Request) { + var req fingerprintSettingsBody + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + writeErr(w, apierror.BadRequest("invalid_body", "malformed JSON")) + return + } + saved, err := h.fingerprintSettings.Set(r.Context(), library.FingerprintSettings{ + Enabled: req.Enabled, + ChromaprintLengthSec: req.ChromaprintLengthSec, + AcousticMaxBitErrorRate: req.AcousticMaxBitErrorRate, + BackfillConcurrency: req.BackfillConcurrency, + SweepIntervalHours: req.SweepIntervalHours, + }) + if err != nil { + // Validation mirrors migration 0061's CHECKs and names the field. + if errors.Is(err, library.ErrFingerprintSettingOutOfRange) { + writeErr(w, apierror.BadRequest("invalid_setting", err.Error())) + return + } + writeErrWithLog(w, h.logger, "admin fingerprint settings: update failed", apierror.Internal(err)) + return + } + writeJSON(w, http.StatusOK, fingerprintSettingsBodyOf(saved)) +} diff --git a/internal/api/admin_fingerprint_settings_test.go b/internal/api/admin_fingerprint_settings_test.go new file mode 100644 index 00000000..056447ea --- /dev/null +++ b/internal/api/admin_fingerprint_settings_test.go @@ -0,0 +1,55 @@ +package api + +import ( + "encoding/json" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "git.fabledsword.com/bvandeusen/minstrel/internal/library" +) + +func TestGetFingerprintSettings_ServesDefaultsWithoutAService(t *testing.T) { + h := &handlers{logger: slog.New(slog.NewTextHandler(io.Discard, nil))} + rec := httptest.NewRecorder() + h.handleGetFingerprintSettings(rec, httptest.NewRequest(http.MethodGet, "/api/admin/library/fingerprint-settings", nil)) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + var got fingerprintSettingsBody + if err := json.NewDecoder(rec.Body).Decode(&got); err != nil { + t.Fatalf("decode: %v", err) + } + if want := fingerprintSettingsBodyOf(library.DefaultFingerprintSettings); got != want { + t.Fatalf("body = %+v, want the defaults %+v", got, want) + } +} + +func TestUpdateFingerprintSettings_Rejects(t *testing.T) { + h := &handlers{logger: slog.New(slog.NewTextHandler(io.Discard, nil))} + for name, tc := range map[string]struct { + body string + code string + mentions string + }{ + "a value out of range, naming the field": { + body: `{"enabled":true,"chromaprint_length_sec":5,"acoustic_max_bit_error_rate":0.15,"backfill_concurrency":2,"sweep_interval_hours":1}`, + code: "invalid_setting", + mentions: "chromaprint_length_sec", + }, + // A partial body would otherwise zero every field it left out. + "a body missing fields": {body: `{"enabled":false}`, code: "invalid_setting"}, + "malformed JSON": {body: `{"enabled":`, code: "invalid_body"}, + } { + rec := httptest.NewRecorder() + h.handleUpdateFingerprintSettings(rec, httptest.NewRequest( + http.MethodPut, "/api/admin/library/fingerprint-settings", strings.NewReader(tc.body))) + body := rec.Body.String() + if rec.Code != http.StatusBadRequest || !strings.Contains(body, `"`+tc.code+`"`) || !strings.Contains(body, tc.mentions) { + t.Errorf("%s: status %d body %s; want 400 %s mentioning %q", name, rec.Code, body, tc.code, tc.mentions) + } + } +} diff --git a/internal/api/api.go b/internal/api/api.go index 85647a5c..6f21467c 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -33,30 +33,31 @@ import ( // Mount attaches /api/* handlers to r. Public endpoints (login) are outside // RequireUser; everything else is gated by the middleware. The events writer // is shared with the Subsonic mount so /rest/scrobble feeds the same store. -func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playevents.Writer, recCfg config.RecommendationConfig, recSettings *recsettings.Service, lidarrCfg *lidarrconfig.Service, lidarrReqs *lidarrrequests.Service, lidarrQuar *lidarrquarantine.Service, tracksSvc *tracks.Service, playlistsSvc *playlists.Service, coverEnricher *coverart.Enricher, coverSettings *coverart.SettingsService, tagSettings *tags.SettingsService, scanner *library.Scanner, scanCfg library.RunScanConfig, dataDir string, sender mailer.Sender, bus *eventbus.Bus, playlistScheduler *playlists.Scheduler, streamSecret []byte, netSettings *netsettings.Service, reacqSettings *reacquisition.SettingsService) { +func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playevents.Writer, recCfg config.RecommendationConfig, recSettings *recsettings.Service, lidarrCfg *lidarrconfig.Service, lidarrReqs *lidarrrequests.Service, lidarrQuar *lidarrquarantine.Service, tracksSvc *tracks.Service, playlistsSvc *playlists.Service, coverEnricher *coverart.Enricher, coverSettings *coverart.SettingsService, tagSettings *tags.SettingsService, scanner *library.Scanner, scanCfg library.RunScanConfig, dataDir string, sender mailer.Sender, bus *eventbus.Bus, playlistScheduler *playlists.Scheduler, streamSecret []byte, netSettings *netsettings.Service, reacqSettings *reacquisition.SettingsService, fpSettings *library.FingerprintSettingsService) { rng := rand.New(rand.NewSource(rand.Int63())) h := &handlers{ pool: pool, logger: logger, events: events, recCfg: recCfg, - recSettings: recSettings, - rng: rng.Float64, - lidarrCfg: lidarrCfg, - lidarrRequests: lidarrReqs, - lidarrQuarantine: lidarrQuar, - tracks: tracksSvc, - playlists: playlistsSvc, - coverart: coverEnricher, - coverSettings: coverSettings, - tagSettings: tagSettings, - scanner: scanner, - scanCfg: scanCfg, - dataDir: dataDir, - mailer: sender, - eventbus: bus, - playlistScheduler: playlistScheduler, - streamSecret: streamSecret, - netSettings: netSettings, - reacqSettings: reacqSettings, - librarySize: recommendation.NewLibrarySize(nil), + recSettings: recSettings, + rng: rng.Float64, + lidarrCfg: lidarrCfg, + lidarrRequests: lidarrReqs, + lidarrQuarantine: lidarrQuar, + tracks: tracksSvc, + playlists: playlistsSvc, + coverart: coverEnricher, + coverSettings: coverSettings, + tagSettings: tagSettings, + scanner: scanner, + scanCfg: scanCfg, + dataDir: dataDir, + mailer: sender, + eventbus: bus, + playlistScheduler: playlistScheduler, + streamSecret: streamSecret, + netSettings: netSettings, + reacqSettings: reacqSettings, + fingerprintSettings: fpSettings, + librarySize: recommendation.NewLibrarySize(nil), } r.Route("/api", func(api chi.Router) { @@ -216,6 +217,8 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev admin.Get("/library/coverage", h.handleGetLibraryCoverage) admin.Get("/library/fingerprints", h.handleGetFingerprintCoverage) + admin.Get("/library/fingerprint-settings", h.handleGetFingerprintSettings) + admin.Put("/library/fingerprint-settings", h.handleUpdateFingerprintSettings) // Duplicates report (#3912): proposals from the duplicate sweep, a // trigger to sweep now, dismissal, and the merge (#3911), which deletes // the removed copies' files after moving their history onto the kept one. @@ -306,6 +309,10 @@ type handlers struct { // missing files (milestone #290) — grace window, backoff, attempt caps. // Cached in the service, so the admin card reads it without a query. reacqSettings *reacquisition.SettingsService + // fingerprintSettings is the fingerprinting policy (M400 #3913), the same + // instance the scanner and the fingerprint workers read, so a save from the + // admin card reaches them without a restart. Nil serves the defaults. + fingerprintSettings *library.FingerprintSettingsService // netSettings caches the trusted reverse-proxy depth read by the auth // middleware on every request and edited from the admin network card. netSettings *netsettings.Service diff --git a/internal/db/dbq/duplicates.sql.go b/internal/db/dbq/duplicates.sql.go index 0f979e34..c3dab489 100644 --- a/internal/db/dbq/duplicates.sql.go +++ b/internal/db/dbq/duplicates.sql.go @@ -211,16 +211,21 @@ SELECT t.id, t.duration_ms, f.audio_stream_sha256, f.chromaprint WHERE t.missing_since IS NULL AND f.fingerprint_version >= $1 AND f.chromaprint IS NOT NULL - AND (t.duration_ms, t.id) > ($2::integer, $3::uuid) + -- Only chromaprints taken at the current length: prints at two lengths are not + -- comparable, and after a length change the backfill is still re-deriving the + -- rest (#3913). + AND f.chromaprint_length_sec = $2 + AND (t.duration_ms, t.id) > ($3::integer, $4::uuid) ORDER BY t.duration_ms, t.id - LIMIT $4 + LIMIT $5 ` type ListDuplicateCandidatesParams struct { - CurrentVersion int16 - AfterDurationMs int32 - AfterID pgtype.UUID - PageLimit int32 + CurrentVersion int16 + ChromaprintLengthSec int32 + AfterDurationMs int32 + AfterID pgtype.UUID + PageLimit int32 } type ListDuplicateCandidatesRow struct { @@ -237,6 +242,7 @@ type ListDuplicateCandidatesRow struct { func (q *Queries) ListDuplicateCandidates(ctx context.Context, arg ListDuplicateCandidatesParams) ([]ListDuplicateCandidatesRow, error) { rows, err := q.db.Query(ctx, listDuplicateCandidates, arg.CurrentVersion, + arg.ChromaprintLengthSec, arg.AfterDurationMs, arg.AfterID, arg.PageLimit, diff --git a/internal/db/dbq/fingerprint_settings.sql.go b/internal/db/dbq/fingerprint_settings.sql.go new file mode 100644 index 00000000..ab1ab634 --- /dev/null +++ b/internal/db/dbq/fingerprint_settings.sql.go @@ -0,0 +1,72 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: fingerprint_settings.sql + +package dbq + +import ( + "context" +) + +const getFingerprintSettings = `-- name: GetFingerprintSettings :one +SELECT id, enabled, chromaprint_length_sec, acoustic_max_bit_error_rate, backfill_concurrency, sweep_interval_hours, updated_at FROM fingerprint_settings WHERE id = true +` + +func (q *Queries) GetFingerprintSettings(ctx context.Context) (FingerprintSetting, error) { + row := q.db.QueryRow(ctx, getFingerprintSettings) + var i FingerprintSetting + err := row.Scan( + &i.ID, + &i.Enabled, + &i.ChromaprintLengthSec, + &i.AcousticMaxBitErrorRate, + &i.BackfillConcurrency, + &i.SweepIntervalHours, + &i.UpdatedAt, + ) + return i, err +} + +const updateFingerprintSettings = `-- name: UpdateFingerprintSettings :one +UPDATE fingerprint_settings + SET enabled = $1, + chromaprint_length_sec = $2, + acoustic_max_bit_error_rate = $3, + backfill_concurrency = $4, + sweep_interval_hours = $5, + updated_at = now() + WHERE id = true +RETURNING id, enabled, chromaprint_length_sec, acoustic_max_bit_error_rate, backfill_concurrency, sweep_interval_hours, updated_at +` + +type UpdateFingerprintSettingsParams struct { + Enabled bool + ChromaprintLengthSec int32 + AcousticMaxBitErrorRate float64 + BackfillConcurrency int32 + SweepIntervalHours int32 +} + +// Whole-row write from the admin card; migration 0061's CHECKs are the backstop +// behind the service's own validation. +func (q *Queries) UpdateFingerprintSettings(ctx context.Context, arg UpdateFingerprintSettingsParams) (FingerprintSetting, error) { + row := q.db.QueryRow(ctx, updateFingerprintSettings, + arg.Enabled, + arg.ChromaprintLengthSec, + arg.AcousticMaxBitErrorRate, + arg.BackfillConcurrency, + arg.SweepIntervalHours, + ) + var i FingerprintSetting + err := row.Scan( + &i.ID, + &i.Enabled, + &i.ChromaprintLengthSec, + &i.AcousticMaxBitErrorRate, + &i.BackfillConcurrency, + &i.SweepIntervalHours, + &i.UpdatedAt, + ) + return i, err +} diff --git a/internal/db/dbq/fingerprints.sql.go b/internal/db/dbq/fingerprints.sql.go index ce1bdf03..f92f0cb2 100644 --- a/internal/db/dbq/fingerprints.sql.go +++ b/internal/db/dbq/fingerprints.sql.go @@ -27,20 +27,29 @@ const getFingerprintCoverage = `-- name: GetFingerprintCoverage :one SELECT count(*)::bigint AS total, count(*) FILTER ( WHERE f.fingerprint_version >= $1 + AND f.chromaprint_length_sec = $2 AND f.audio_stream_sha256 IS NOT NULL AND f.chromaprint IS NOT NULL )::bigint AS fingerprinted, count(*) FILTER ( WHERE f.fingerprint_version >= $1 + AND f.chromaprint_length_sec = $2 AND (f.audio_stream_sha256 IS NULL OR f.chromaprint IS NULL) )::bigint AS rejected, count(*) FILTER ( - WHERE f.track_id IS NULL OR f.fingerprint_version < $1 + WHERE f.track_id IS NULL + OR f.fingerprint_version < $1 + OR f.chromaprint_length_sec <> $2 )::bigint AS pending FROM tracks t LEFT JOIN track_fingerprints f ON f.track_id = t.id WHERE t.missing_since IS NULL ` +type GetFingerprintCoverageParams struct { + CurrentVersion int16 + ChromaprintLengthSec int32 +} + type GetFingerprintCoverageRow struct { Total int64 Fingerprinted int64 @@ -49,11 +58,13 @@ type GetFingerprintCoverageRow struct { } // The admin gauge for the backfill. fingerprinted + rejected + pending = total. -// rejected is a row at the current version with a NULL half: a tool ran and -// refused the file, which is settled rather than waiting. Missing tracks are -// excluded, or the gauge could never reach the end. -func (q *Queries) GetFingerprintCoverage(ctx context.Context, currentVersion int16) (GetFingerprintCoverageRow, error) { - row := q.db.QueryRow(ctx, getFingerprintCoverage, currentVersion) +// "Current" means derived by the current method AT the current length: a row at +// another length is pending, because the backfill will re-derive it. rejected is +// a current row with a NULL half: a tool ran and refused the file, which is +// settled rather than waiting. Missing tracks are excluded, or the gauge could +// never reach the end. +func (q *Queries) GetFingerprintCoverage(ctx context.Context, arg GetFingerprintCoverageParams) (GetFingerprintCoverageRow, error) { + row := q.db.QueryRow(ctx, getFingerprintCoverage, arg.CurrentVersion, arg.ChromaprintLengthSec) var i GetFingerprintCoverageRow err := row.Scan( &i.Total, @@ -69,16 +80,21 @@ SELECT t.id, t.file_path FROM tracks t LEFT JOIN track_fingerprints f ON f.track_id = t.id WHERE t.missing_since IS NULL - AND (f.track_id IS NULL OR f.fingerprint_version < $1) - AND t.id > $2 + -- A row taken at another length is as stale as one from an older method: + -- chromaprints at two lengths cannot be compared (#3913). + AND (f.track_id IS NULL + OR f.fingerprint_version < $1 + OR f.chromaprint_length_sec <> $2) + AND t.id > $3 ORDER BY t.id - LIMIT $3 + LIMIT $4 ` type ListTracksNeedingFingerprintParams struct { - CurrentVersion int16 - AfterID pgtype.UUID - BatchLimit int32 + CurrentVersion int16 + ChromaprintLengthSec int32 + AfterID pgtype.UUID + BatchLimit int32 } type ListTracksNeedingFingerprintRow struct { @@ -93,7 +109,12 @@ type ListTracksNeedingFingerprintRow struct { // and retried in a tight loop. Missing tracks are skipped — there is no file to // read. func (q *Queries) ListTracksNeedingFingerprint(ctx context.Context, arg ListTracksNeedingFingerprintParams) ([]ListTracksNeedingFingerprintRow, error) { - rows, err := q.db.Query(ctx, listTracksNeedingFingerprint, arg.CurrentVersion, arg.AfterID, arg.BatchLimit) + rows, err := q.db.Query(ctx, listTracksNeedingFingerprint, + arg.CurrentVersion, + arg.ChromaprintLengthSec, + arg.AfterID, + arg.BatchLimit, + ) if err != nil { return nil, err } @@ -114,23 +135,25 @@ func (q *Queries) ListTracksNeedingFingerprint(ctx context.Context, arg ListTrac const upsertTrackFingerprint = `-- name: UpsertTrackFingerprint :exec INSERT INTO track_fingerprints ( - track_id, audio_stream_sha256, chromaprint, fingerprint_version + track_id, audio_stream_sha256, chromaprint, fingerprint_version, chromaprint_length_sec ) VALUES ( $1, $2, $3, - $4 + $4, $5 ) ON CONFLICT (track_id) DO UPDATE SET - audio_stream_sha256 = EXCLUDED.audio_stream_sha256, - chromaprint = EXCLUDED.chromaprint, - fingerprint_version = EXCLUDED.fingerprint_version, - computed_at = now() + audio_stream_sha256 = EXCLUDED.audio_stream_sha256, + chromaprint = EXCLUDED.chromaprint, + fingerprint_version = EXCLUDED.fingerprint_version, + chromaprint_length_sec = EXCLUDED.chromaprint_length_sec, + computed_at = now() ` type UpsertTrackFingerprintParams struct { - TrackID pgtype.UUID - AudioStreamSha256 []byte - Chromaprint []int32 - FingerprintVersion int16 + TrackID pgtype.UUID + AudioStreamSha256 []byte + Chromaprint []int32 + FingerprintVersion int16 + ChromaprintLengthSec int32 } // Written whenever a track's fingerprint is derived: by the scan when a file is @@ -143,6 +166,7 @@ func (q *Queries) UpsertTrackFingerprint(ctx context.Context, arg UpsertTrackFin arg.AudioStreamSha256, arg.Chromaprint, arg.FingerprintVersion, + arg.ChromaprintLengthSec, ) return err } diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index cddadd0c..17a9c89d 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -323,6 +323,16 @@ type DuplicateSweep struct { ErrorMessage *string } +type FingerprintSetting struct { + ID bool + Enabled bool + ChromaprintLengthSec int32 + AcousticMaxBitErrorRate float64 + BackfillConcurrency int32 + SweepIntervalHours int32 + UpdatedAt pgtype.Timestamptz +} + type GeneralLike struct { UserID pgtype.UUID TrackID pgtype.UUID @@ -694,11 +704,12 @@ type Track struct { } type TrackFingerprint struct { - TrackID pgtype.UUID - AudioStreamSha256 []byte - Chromaprint []int32 - FingerprintVersion int16 - ComputedAt pgtype.Timestamptz + TrackID pgtype.UUID + AudioStreamSha256 []byte + Chromaprint []int32 + FingerprintVersion int16 + ComputedAt pgtype.Timestamptz + ChromaprintLengthSec int32 } type TrackSimilarity struct { diff --git a/internal/db/migrations/0061_fingerprint_settings.down.sql b/internal/db/migrations/0061_fingerprint_settings.down.sql new file mode 100644 index 00000000..31d6d9cc --- /dev/null +++ b/internal/db/migrations/0061_fingerprint_settings.down.sql @@ -0,0 +1,2 @@ +ALTER TABLE track_fingerprints DROP COLUMN chromaprint_length_sec; +DROP TABLE fingerprint_settings; diff --git a/internal/db/migrations/0061_fingerprint_settings.up.sql b/internal/db/migrations/0061_fingerprint_settings.up.sql new file mode 100644 index 00000000..021f85af --- /dev/null +++ b/internal/db/migrations/0061_fingerprint_settings.up.sql @@ -0,0 +1,50 @@ +-- 0061_fingerprint_settings.up.sql — fingerprinting's knobs, in admin Settings +-- (Scribe #3913, milestone #400). Rule 25: anything an operator might tune is a +-- database row, changed without a restart. Singleton in the style of +-- reacquisition_settings (0056). +CREATE TABLE fingerprint_settings ( + id boolean PRIMARY KEY DEFAULT true, + + -- Fingerprinting new files, the backfill, and the duplicate sweep. Off stops + -- the decode work entirely — the reason to turn it off is a slow NAS, and + -- that is the operator's call. On by default: a library that cannot tell its + -- duplicates apart is what milestone #400 exists to end. + enabled boolean NOT NULL DEFAULT true, + + -- Seconds of audio fpcalc fingerprints. Chromaprints taken at different + -- lengths cannot be compared, which is why track_fingerprints records the + -- length each row was taken at (below): change this and every chromaprint is + -- re-derived, and until then only rows at the new length are compared. + chromaprint_length_sec integer NOT NULL DEFAULT 120, + + -- The most disagreement two aligned fingerprints may show and still be + -- proposed as one recording. Unrelated audio sits near 0.5, so the ceiling + -- stays well clear of it. + acoustic_max_bit_error_rate double precision NOT NULL DEFAULT 0.15, + + -- Files the backfill decodes at once. Decoding competes with playback + -- transcoding for CPU and with streaming for the mount. + backfill_concurrency integer NOT NULL DEFAULT 2, + + -- The least time between duplicate sweeps. A sweep still runs only when + -- fingerprints have changed since the last one. + sweep_interval_hours integer NOT NULL DEFAULT 1, + + -- When the settings were last saved. A new threshold or length can change + -- what a sweep finds, so a save makes a sweep due. + updated_at timestamptz NOT NULL DEFAULT now(), + + CONSTRAINT fingerprint_settings_singleton CHECK (id = true), + CONSTRAINT fingerprint_settings_length_range + CHECK (chromaprint_length_sec >= 30 AND chromaprint_length_sec <= 600), + CONSTRAINT fingerprint_settings_threshold_range + CHECK (acoustic_max_bit_error_rate >= 0.01 AND acoustic_max_bit_error_rate <= 0.35), + CONSTRAINT fingerprint_settings_concurrency_range + CHECK (backfill_concurrency >= 1 AND backfill_concurrency <= 8), + CONSTRAINT fingerprint_settings_sweep_interval_range + CHECK (sweep_interval_hours >= 1 AND sweep_interval_hours <= 168) +); +INSERT INTO fingerprint_settings (id) VALUES (true) ON CONFLICT (id) DO NOTHING; + +-- Every row written so far was taken at fpcalc's default length. +ALTER TABLE track_fingerprints ADD COLUMN chromaprint_length_sec integer NOT NULL DEFAULT 120; diff --git a/internal/db/queries/duplicates.sql b/internal/db/queries/duplicates.sql index 22c3b1fe..9150ccc5 100644 --- a/internal/db/queries/duplicates.sql +++ b/internal/db/queries/duplicates.sql @@ -53,6 +53,10 @@ SELECT t.id, t.duration_ms, f.audio_stream_sha256, f.chromaprint WHERE t.missing_since IS NULL AND f.fingerprint_version >= sqlc.arg(current_version) AND f.chromaprint IS NOT NULL + -- Only chromaprints taken at the current length: prints at two lengths are not + -- comparable, and after a length change the backfill is still re-deriving the + -- rest (#3913). + AND f.chromaprint_length_sec = sqlc.arg(chromaprint_length_sec) AND (t.duration_ms, t.id) > (sqlc.arg(after_duration_ms)::integer, sqlc.arg(after_id)::uuid) ORDER BY t.duration_ms, t.id LIMIT sqlc.arg(page_limit); diff --git a/internal/db/queries/fingerprint_settings.sql b/internal/db/queries/fingerprint_settings.sql new file mode 100644 index 00000000..f1022bb1 --- /dev/null +++ b/internal/db/queries/fingerprint_settings.sql @@ -0,0 +1,15 @@ +-- name: GetFingerprintSettings :one +SELECT * FROM fingerprint_settings WHERE id = true; + +-- name: UpdateFingerprintSettings :one +-- Whole-row write from the admin card; migration 0061's CHECKs are the backstop +-- behind the service's own validation. +UPDATE fingerprint_settings + SET enabled = sqlc.arg(enabled), + chromaprint_length_sec = sqlc.arg(chromaprint_length_sec), + acoustic_max_bit_error_rate = sqlc.arg(acoustic_max_bit_error_rate), + backfill_concurrency = sqlc.arg(backfill_concurrency), + sweep_interval_hours = sqlc.arg(sweep_interval_hours), + updated_at = now() + WHERE id = true +RETURNING *; diff --git a/internal/db/queries/fingerprints.sql b/internal/db/queries/fingerprints.sql index 90a8a978..ae68f233 100644 --- a/internal/db/queries/fingerprints.sql +++ b/internal/db/queries/fingerprints.sql @@ -4,16 +4,17 @@ -- older method. Replaces the row wholesale — a fingerprint of the old bytes has -- no standing once the file has changed. INSERT INTO track_fingerprints ( - track_id, audio_stream_sha256, chromaprint, fingerprint_version + track_id, audio_stream_sha256, chromaprint, fingerprint_version, chromaprint_length_sec ) VALUES ( sqlc.arg(track_id), sqlc.narg(audio_stream_sha256), sqlc.narg(chromaprint), - sqlc.arg(fingerprint_version) + sqlc.arg(fingerprint_version), sqlc.arg(chromaprint_length_sec) ) ON CONFLICT (track_id) DO UPDATE SET - audio_stream_sha256 = EXCLUDED.audio_stream_sha256, - chromaprint = EXCLUDED.chromaprint, - fingerprint_version = EXCLUDED.fingerprint_version, - computed_at = now(); + audio_stream_sha256 = EXCLUDED.audio_stream_sha256, + chromaprint = EXCLUDED.chromaprint, + fingerprint_version = EXCLUDED.fingerprint_version, + chromaprint_length_sec = EXCLUDED.chromaprint_length_sec, + computed_at = now(); -- name: DeleteTrackFingerprint :exec -- A file changed but could not be fingerprinted, for a reason unrelated to the @@ -32,27 +33,37 @@ SELECT t.id, t.file_path FROM tracks t LEFT JOIN track_fingerprints f ON f.track_id = t.id WHERE t.missing_since IS NULL - AND (f.track_id IS NULL OR f.fingerprint_version < sqlc.arg(current_version)) + -- A row taken at another length is as stale as one from an older method: + -- chromaprints at two lengths cannot be compared (#3913). + AND (f.track_id IS NULL + OR f.fingerprint_version < sqlc.arg(current_version) + OR f.chromaprint_length_sec <> sqlc.arg(chromaprint_length_sec)) AND t.id > sqlc.arg(after_id) ORDER BY t.id LIMIT sqlc.arg(batch_limit); -- name: GetFingerprintCoverage :one -- The admin gauge for the backfill. fingerprinted + rejected + pending = total. --- rejected is a row at the current version with a NULL half: a tool ran and --- refused the file, which is settled rather than waiting. Missing tracks are --- excluded, or the gauge could never reach the end. +-- "Current" means derived by the current method AT the current length: a row at +-- another length is pending, because the backfill will re-derive it. rejected is +-- a current row with a NULL half: a tool ran and refused the file, which is +-- settled rather than waiting. Missing tracks are excluded, or the gauge could +-- never reach the end. SELECT count(*)::bigint AS total, count(*) FILTER ( WHERE f.fingerprint_version >= sqlc.arg(current_version) + AND f.chromaprint_length_sec = sqlc.arg(chromaprint_length_sec) AND f.audio_stream_sha256 IS NOT NULL AND f.chromaprint IS NOT NULL )::bigint AS fingerprinted, count(*) FILTER ( WHERE f.fingerprint_version >= sqlc.arg(current_version) + AND f.chromaprint_length_sec = sqlc.arg(chromaprint_length_sec) AND (f.audio_stream_sha256 IS NULL OR f.chromaprint IS NULL) )::bigint AS rejected, count(*) FILTER ( - WHERE f.track_id IS NULL OR f.fingerprint_version < sqlc.arg(current_version) + WHERE f.track_id IS NULL + OR f.fingerprint_version < sqlc.arg(current_version) + OR f.chromaprint_length_sec <> sqlc.arg(chromaprint_length_sec) )::bigint AS pending FROM tracks t LEFT JOIN track_fingerprints f ON f.track_id = t.id diff --git a/internal/dbtest/reset.go b/internal/dbtest/reset.go index 25e0b076..a5b2380e 100644 --- a/internal/dbtest/reset.go +++ b/internal/dbtest/reset.go @@ -130,4 +130,15 @@ func ResetDB(t *testing.T, pool *pgxpool.Pool) { ); err != nil { t.Fatalf("dbtest.ResetDB reset tag-sources version: %v", err) } + // Fingerprinting settings (M400 #3913), a singleton like the counters above. + // Every column goes back to its migration default rather than to literals + // written here, so a test can pin the Go defaults to the migration's. + if _, err := pool.Exec(ctx, ` + UPDATE fingerprint_settings + SET enabled = DEFAULT, chromaprint_length_sec = DEFAULT, + acoustic_max_bit_error_rate = DEFAULT, backfill_concurrency = DEFAULT, + sweep_interval_hours = DEFAULT, updated_at = DEFAULT`, + ); err != nil { + t.Fatalf("dbtest.ResetDB reset fingerprint settings: %v", err) + } } diff --git a/internal/library/duplicate_sweep.go b/internal/library/duplicate_sweep.go index f1c3cf0f..34dd2cb8 100644 --- a/internal/library/duplicate_sweep.go +++ b/internal/library/duplicate_sweep.go @@ -32,10 +32,17 @@ import ( // carries a ~4 KB fingerprint, so a page is about 2 MB. const duplicateCandidatePage = 500 -// duplicateSweepTick is how often the worker checks for anything new to sweep. -// With nothing new, a tick is two cheap aggregate queries. +// duplicateSweepTick is how often the worker checks whether a sweep is due. The +// operator's sweep interval (#3913) is the least time between sweeps; the tick +// only bounds how late past it one starts. With nothing due, a tick is two cheap +// aggregate queries. const duplicateSweepTick = time.Hour +// sweepIntervalSlack absorbs the moment between a tick and the sweep it starts +// stamping started_at. Without it a one-hour interval checked on a one-hour tick +// would find the last sweep a moment under an hour old, and skip every other tick. +const sweepIntervalSlack = 5 * time.Minute + // staleDuplicateSweepThreshold is the age past which an in-flight sweep is // assumed dead — a crash mid-sweep leaves finished_at NULL for ever — and another // may start. Twice the library scan's threshold, because a sweep compares @@ -58,13 +65,16 @@ type DuplicateSweepResult struct { Oversize int // acoustic clusters too large to propose } -// RunDuplicateSweep runs one sweep and records it in duplicate_sweeps. -func RunDuplicateSweep(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger) (DuplicateSweepResult, error) { - return runDuplicateSweep(ctx, pool, logger, duplicateCandidatePage) +// RunDuplicateSweep runs one sweep and records it in duplicate_sweeps. cfg is a +// snapshot: one sweep applies one threshold and one length throughout. +func RunDuplicateSweep( + ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, cfg FingerprintSettings, +) (DuplicateSweepResult, error) { + return runDuplicateSweep(ctx, pool, logger, cfg, duplicateCandidatePage) } func runDuplicateSweep( - ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, pageSize int32, + ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, cfg FingerprintSettings, pageSize int32, ) (DuplicateSweepResult, error) { q := dbq.New(pool) sweep, err := q.StartDuplicateSweep(ctx) @@ -72,7 +82,7 @@ func runDuplicateSweep( return DuplicateSweepResult{}, fmt.Errorf("start duplicate sweep: %w", err) } - res, runErr := sweepDuplicates(ctx, q, sweep.ID, pageSize) + res, runErr := sweepDuplicates(ctx, q, sweep.ID, cfg, pageSize) finishCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), duplicateSweepFinishTimeout) defer cancel() @@ -98,7 +108,7 @@ func runDuplicateSweep( } func sweepDuplicates( - ctx context.Context, q *dbq.Queries, sweepID pgtype.UUID, pageSize int32, + ctx context.Context, q *dbq.Queries, sweepID pgtype.UUID, cfg FingerprintSettings, pageSize int32, ) (DuplicateSweepResult, error) { var res DuplicateSweepResult @@ -119,9 +129,12 @@ func sweepDuplicates( // Acoustic tier, streamed in duration order. The first member of an exact // group the stream meets stands in for the whole group; the rest are skipped. - grouper := newStreamGrouper(defaultAcousticMaxBitErrorRate) + // Only prints at the current length are streamed: a print at another length + // cannot be compared, and is waiting on the backfill to be re-derived. + grouper := newStreamGrouper(cfg.AcousticMaxBitErrorRate) params := dbq.ListDuplicateCandidatesParams{ - CurrentVersion: fingerprintVersion, + CurrentVersion: fingerprintVersion, + ChromaprintLengthSec: cfg.ChromaprintLengthSec, // Durations are never negative, and the all-zero uuid sorts first: every // row is after this cursor. Valid must be true, or "> NULL" matches nothing. AfterDurationMs: -1, @@ -262,7 +275,9 @@ func formatUUIDs(ids []pgtype.UUID) []string { // running, reaping a sweep that has been in flight past // staleDuplicateSweepThreshold. Mirrors TryStartScan. The sweep runs on ctx, so // a caller answering an HTTP request must pass a context that outlives it. -func TryStartDuplicateSweep(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger) (bool, error) { +func TryStartDuplicateSweep( + ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, cfg FingerprintSettings, +) (bool, error) { q := dbq.New(pool) row, err := q.GetInFlightDuplicateSweep(ctx) switch { @@ -282,23 +297,28 @@ func TryStartDuplicateSweep(ctx context.Context, pool *pgxpool.Pool, logger *slo } go func() { - if _, err := RunDuplicateSweep(ctx, pool, logger); err != nil { + if _, err := RunDuplicateSweep(ctx, pool, logger, cfg); err != nil { logger.Warn("duplicate sweep failed", "err", err) } }() return true, nil } -// DuplicateSweepWorker sweeps whenever fingerprints have changed. +// DuplicateSweepWorker sweeps whenever its input has changed, at most once per +// the operator's sweep interval. type DuplicateSweepWorker struct { - pool *pgxpool.Pool - logger *slog.Logger - tick time.Duration + pool *pgxpool.Pool + logger *slog.Logger + settings *FingerprintSettingsService + tick time.Duration } -// NewDuplicateSweepWorker builds a worker with the production cadence. -func NewDuplicateSweepWorker(pool *pgxpool.Pool, logger *slog.Logger) *DuplicateSweepWorker { - return &DuplicateSweepWorker{pool: pool, logger: logger, tick: duplicateSweepTick} +// NewDuplicateSweepWorker builds a worker with the production cadence. settings +// is shared with the admin API; nil runs on defaults. +func NewDuplicateSweepWorker( + pool *pgxpool.Pool, logger *slog.Logger, settings *FingerprintSettingsService, +) *DuplicateSweepWorker { + return &DuplicateSweepWorker{pool: pool, logger: logger, settings: settings, tick: duplicateSweepTick} } // Run blocks until ctx is cancelled, checking once at start and then each tick. @@ -323,7 +343,8 @@ func (w *DuplicateSweepWorker) tickOnce(ctx context.Context) { w.logger.Error("duplicate sweep: tick panicked", "panic", r) } }() - due, err := duplicateSweepDue(ctx, dbq.New(w.pool)) + cfg := w.settings.Get() + due, err := duplicateSweepDue(ctx, dbq.New(w.pool), cfg, time.Now()) if err != nil { if ctx.Err() == nil { w.logger.Warn("duplicate sweep: due check failed", "err", err) @@ -333,28 +354,45 @@ func (w *DuplicateSweepWorker) tickOnce(ctx context.Context) { if !due { return } - if _, err := TryStartDuplicateSweep(ctx, w.pool, w.logger); err != nil { + if _, err := TryStartDuplicateSweep(ctx, w.pool, w.logger, cfg); err != nil { w.logger.Warn("duplicate sweep: start failed", "err", err) } } -// duplicateSweepDue reports whether any fingerprint was written after the latest -// sweep started. Fingerprints are the sweep's only input, so nothing else can -// change its answer; while the backfill is running this is true every tick. -func duplicateSweepDue(ctx context.Context, q *dbq.Queries) (bool, error) { +// duplicateSweepDue reads what sweepIsDue decides on. +func duplicateSweepDue(ctx context.Context, q *dbq.Queries, cfg FingerprintSettings, now time.Time) (bool, error) { latest, err := q.GetLatestFingerprintComputedAt(ctx) if err != nil { return false, fmt.Errorf("latest fingerprint: %w", err) } - if !latest.Valid { - return false, nil // nothing fingerprinted yet - } + var lastStart pgtype.Timestamptz last, err := q.GetLatestDuplicateSweep(ctx) - if errors.Is(err, pgx.ErrNoRows) { - return true, nil - } - if err != nil { + switch { + case err == nil: + lastStart = last.StartedAt + case !errors.Is(err, pgx.ErrNoRows): return false, fmt.Errorf("latest duplicate sweep: %w", err) } - return latest.Time.After(last.StartedAt.Time), nil + return sweepIsDue(latest, lastStart, cfg, now), nil +} + +// sweepIsDue reports whether a sweep should start: something it reads has +// changed since the last sweep started, and the operator's interval has passed. +// +// Two things can change its answer. Fingerprints are its input, so any written +// after the last sweep began count; while the backfill runs that is true every +// tick, which is what the interval is for. And a settings save counts, because a +// new threshold or length changes what the same fingerprints group into. +func sweepIsDue(latestFingerprint, lastSweepStart pgtype.Timestamptz, cfg FingerprintSettings, now time.Time) bool { + if !latestFingerprint.Valid { + return false // nothing fingerprinted yet + } + if !lastSweepStart.Valid { + return true // never swept + } + interval := time.Duration(cfg.SweepIntervalHours) * time.Hour + if now.Sub(lastSweepStart.Time) < interval-sweepIntervalSlack { + return false + } + return latestFingerprint.Time.After(lastSweepStart.Time) || cfg.UpdatedAt.After(lastSweepStart.Time) } diff --git a/internal/library/duplicate_sweep_test.go b/internal/library/duplicate_sweep_test.go index 9f30c778..15bc478b 100644 --- a/internal/library/duplicate_sweep_test.go +++ b/internal/library/duplicate_sweep_test.go @@ -9,6 +9,9 @@ import ( "sort" "strings" "testing" + "time" + + "github.com/jackc/pgx/v5/pgtype" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync" @@ -58,6 +61,7 @@ func TestDuplicateSweep_Integration(t *testing.T) { } if err := q.UpsertTrackFingerprint(ctx, dbq.UpsertTrackFingerprintParams{ TrackID: tr.ID, AudioStreamSha256: sum, Chromaprint: print, FingerprintVersion: fingerprintVersion, + ChromaprintLengthSec: defaultChromaprintLengthSec, }); err != nil { t.Fatalf("fingerprint %s: %v", name, err) } @@ -105,7 +109,7 @@ func TestDuplicateSweep_Integration(t *testing.T) { } // 1. A page size of one forces the keyset cursor across every candidate. - res, err := runDuplicateSweep(ctx, pool, logger, 1) + res, err := runDuplicateSweep(ctx, pool, logger, DefaultFingerprintSettings, 1) if err != nil { t.Fatalf("first sweep: %v", err) } @@ -133,7 +137,7 @@ func TestDuplicateSweep_Integration(t *testing.T) { if _, err := pool.Exec(ctx, "UPDATE duplicate_groups SET status = 'dismissed' WHERE member_key = $1", acousticKey); err != nil { t.Fatalf("dismiss: %v", err) } - res, err = runDuplicateSweep(ctx, pool, logger, duplicateCandidatePage) + res, err = runDuplicateSweep(ctx, pool, logger, DefaultFingerprintSettings, duplicateCandidatePage) if err != nil { t.Fatalf("second sweep: %v", err) } @@ -150,7 +154,7 @@ func TestDuplicateSweep_Integration(t *testing.T) { "DELETE FROM track_fingerprints f USING tracks t WHERE f.track_id = t.id AND t.file_path LIKE '%www-02.mp3'"); err != nil { t.Fatalf("drop fingerprint: %v", err) } - res, err = runDuplicateSweep(ctx, pool, logger, duplicateCandidatePage) + res, err = runDuplicateSweep(ctx, pool, logger, DefaultFingerprintSettings, duplicateCandidatePage) if err != nil { t.Fatalf("third sweep: %v", err) } @@ -170,4 +174,77 @@ func TestDuplicateSweep_Integration(t *testing.T) { if !last.FinishedAt.Valid || last.ErrorMessage != nil { t.Fatalf("latest sweep = %+v, want finished without error", last) } + + // 5. Prints taken at another length are never compared (#3913). Every print + // here was taken at the default length, so a sweep at 60s has nothing to read, + // rather than scoring 120s prints against each other as if they were 60s ones. + atOtherLength := DefaultFingerprintSettings + atOtherLength.ChromaprintLengthSec = 60 + res, err = runDuplicateSweep(ctx, pool, logger, atOtherLength, duplicateCandidatePage) + if err != nil { + t.Fatalf("sweep at another length: %v", err) + } + if res.Candidates != 0 || res.Groups != 0 { + t.Fatalf("sweep at another length = %+v, want no candidates and no groups", res) + } + + // 6. The sweep applies the threshold it is given. The recording's two copies + // disagree on about 5% of their bits: grouped at the default, not at 1%. + if _, err := pool.Exec(ctx, "DELETE FROM duplicate_groups"); err != nil { + t.Fatalf("clear groups: %v", err) + } + strict := DefaultFingerprintSettings + strict.AcousticMaxBitErrorRate = 0.01 + res, err = runDuplicateSweep(ctx, pool, logger, strict, duplicateCandidatePage) + if err != nil { + t.Fatalf("strict sweep: %v", err) + } + if res.Groups != 0 { + t.Fatalf("sweep at a 1%% threshold = %+v, want the copies 5%% apart left ungrouped", res) + } + res, err = runDuplicateSweep(ctx, pool, logger, DefaultFingerprintSettings, duplicateCandidatePage) + if err != nil { + t.Fatalf("default sweep: %v", err) + } + if got := groups(); res.Groups != 1 || got[acousticKey] != (stored{"acoustic", "pending"}) { + t.Fatalf("sweep at the default threshold = %+v, groups %+v; want the recording's copies proposed", res, got) + } +} + +func TestSweepIsDue(t *testing.T) { + now := time.Date(2026, 9, 11, 12, 0, 0, 0, time.UTC) + at := func(ago time.Duration) pgtype.Timestamptz { + return pgtype.Timestamptz{Time: now.Add(-ago), Valid: true} + } + never := pgtype.Timestamptz{} + hourly := DefaultFingerprintSettings + daily := DefaultFingerprintSettings + daily.SweepIntervalHours = 24 + savedAgo := func(ago time.Duration) FingerprintSettings { + s := DefaultFingerprintSettings + s.UpdatedAt = now.Add(-ago) + return s + } + for _, tc := range []struct { + name string + latestPrint, lastSweep pgtype.Timestamptz + cfg FingerprintSettings + want bool + }{ + {"nothing fingerprinted", never, never, hourly, false}, + {"never swept", at(time.Minute), never, hourly, true}, + {"new fingerprints since the last sweep", at(10 * time.Minute), at(2 * time.Hour), hourly, true}, + {"nothing new since the last sweep", at(3 * time.Hour), at(2 * time.Hour), hourly, false}, + // The sweep started a moment after the previous tick, so one tick later + // it is a moment under an hour old. Without the slack this is false. + {"one tick after an hourly sweep", at(time.Minute), at(time.Hour - 2*time.Second), hourly, true}, + {"new fingerprints inside the interval", at(time.Minute), at(3 * time.Hour), daily, false}, + {"new fingerprints past the interval", at(time.Minute), at(25 * time.Hour), daily, true}, + {"settings saved since the last sweep", at(3 * time.Hour), at(2 * time.Hour), savedAgo(time.Hour), true}, + {"settings saved before the last sweep", at(3 * time.Hour), at(2 * time.Hour), savedAgo(4 * time.Hour), false}, + } { + if got := sweepIsDue(tc.latestPrint, tc.lastSweep, tc.cfg, now); got != tc.want { + t.Errorf("%s: sweepIsDue = %v, want %v", tc.name, got, tc.want) + } + } } diff --git a/internal/library/fingerprint.go b/internal/library/fingerprint.go index ebd6b3db..4a33117d 100644 --- a/internal/library/fingerprint.go +++ b/internal/library/fingerprint.go @@ -50,21 +50,28 @@ const fingerprintTimeout = 60 * time.Second const fingerprintWaitDelay = 5 * time.Second // fingerprintVersion stamps how a track_fingerprints row was derived. Bump it -// whenever the derivation changes — the hash arguments, fpcalc's flags or its -// length — and the backfill re-derives every row below it. Fingerprints taken -// by two methods are not comparable, and nothing else would reveal that the -// library held a mix. +// whenever the derivation changes — the hash arguments or fpcalc's flags — and +// the backfill re-derives every row below it. Fingerprints taken by two methods +// are not comparable, and nothing else would reveal that the library held a mix. +// +// The length is deliberately not part of it: it is an operator setting (#3913), +// so each row records the length it was taken at and readers compare only rows +// at the current one. See fingerprint_settings.go. const fingerprintVersion int16 = 1 // errFingerprintTimeout marks a tool that ran out of time. Distinct from a // failed exit because a stall is a fact about the mount, not about the file. var errFingerprintTimeout = errors.New("fingerprint tool timed out") -// defaultChromaprintLengthSec is how many seconds of audio fpcalc fingerprints. -// 120 is fpcalc's own default. Fingerprints taken at different lengths are not -// comparable, so changing this has to re-derive every stored one. +// defaultChromaprintLengthSec is the shipped value of the length setting (#3913): +// how many seconds of audio fpcalc fingerprints. 120 is fpcalc's own default. const defaultChromaprintLengthSec = 120 +// errChromaprintSkipped marks a chromaprint not taken because fingerprinting is +// switched off. Inconclusive rather than a verdict: nothing was learned about +// the file. +var errChromaprintSkipped = errors.New("chromaprint skipped: fingerprinting is off") + // fpcalcStderrTail caps how much of a failing tool's stderr reaches the log. const fpcalcStderrTail = 512 @@ -113,11 +120,24 @@ type fingerprintResult struct { printErr error } -// computeFingerprint derives both halves for the file at path. -func computeFingerprint(ctx context.Context, path string) fingerprintResult { +// fingerprintOptions is what the settings decide for one attempt. Captured once +// per file, so the length a chromaprint was taken at is the length stored with it +// even if the setting changes mid-attempt. +type fingerprintOptions struct { + lengthSec int32 + // chromaprint false takes the stream hash alone: a demux, no decode. + chromaprint bool +} + +// computeFingerprint derives the halves opts asks for, for the file at path. +func computeFingerprint(ctx context.Context, path string, opts fingerprintOptions) fingerprintResult { var r fingerprintResult r.streamSHA256, r.hashErr = computeAudioStreamSHA256(ctx, path) - r.chromaprint, r.printErr = computeChromaprint(ctx, path, defaultChromaprintLengthSec) + if !opts.chromaprint { + r.printErr = errChromaprintSkipped + return r + } + r.chromaprint, r.printErr = computeChromaprint(ctx, path, opts.lengthSec) return r } @@ -130,11 +150,13 @@ func (r fingerprintResult) inconclusive() bool { } // isInconclusive names the failures that are not a verdict on the file: a -// stall, a cancelled scan, and a tool that is not installed. The last matters -// outside the image — a dev binary run without fpcalc on PATH must not stamp -// every track in the library as unfingerprintable. +// stall, a cancelled scan, a tool that is not installed, and a chromaprint +// skipped because fingerprinting is off. A missing tool matters outside the +// image — a dev binary run without fpcalc on PATH must not stamp every track in +// the library as unfingerprintable. func isInconclusive(err error) bool { return errors.Is(err, errFingerprintTimeout) || + errors.Is(err, errChromaprintSkipped) || errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) || errors.Is(err, exec.ErrNotFound) @@ -142,11 +164,11 @@ func isInconclusive(err error) bool { // fingerprintFile runs the scanner's fingerprinter. A Scanner built without New // gets the real tools rather than a nil-func panic halfway through a scan. -func (s *Scanner) fingerprintFile(ctx context.Context, path string) fingerprintResult { +func (s *Scanner) fingerprintFile(ctx context.Context, path string, opts fingerprintOptions) fingerprintResult { if s.fingerprint == nil { - return computeFingerprint(ctx, path) + return computeFingerprint(ctx, path, opts) } - return s.fingerprint(ctx, path) + return s.fingerprint(ctx, path, opts) } // fingerprintOutcome is what storeFingerprint did with one attempt. @@ -163,9 +185,11 @@ const ( // the backfill (#3908) alike, so there is one rule for what gets written. It // never fails its caller: a missing fingerprint only keeps a track out of // duplicate detection, which is not worth dropping a scan or a pass over. +// +// lengthSec is the length fp's chromaprint was taken at, stored with it (#3913). func storeFingerprint( ctx context.Context, q *dbq.Queries, logger *slog.Logger, - trackID pgtype.UUID, path string, fp fingerprintResult, + trackID pgtype.UUID, path string, fp fingerprintResult, lengthSec int32, ) fingerprintOutcome { if fp.hashErr != nil { logger.Warn("fingerprint: audio stream hash failed", "path", path, "err", fp.hashErr) @@ -186,10 +210,11 @@ func storeFingerprint( // is stamped at the current version so the backfill does not retry it on // every pass. It is retried when the file changes. if err := q.UpsertTrackFingerprint(ctx, dbq.UpsertTrackFingerprintParams{ - TrackID: trackID, - AudioStreamSha256: fp.streamSHA256, - Chromaprint: fp.chromaprint, - FingerprintVersion: fingerprintVersion, + TrackID: trackID, + AudioStreamSha256: fp.streamSHA256, + Chromaprint: fp.chromaprint, + FingerprintVersion: fingerprintVersion, + ChromaprintLengthSec: lengthSec, }); err != nil { logger.Warn("fingerprint: storing fingerprint failed", "path", path, "err", err) return outcomeStoreFailed @@ -211,8 +236,8 @@ func computeAudioStreamSHA256(ctx context.Context, path string) ([]byte, error) // computeChromaprint returns the raw acoustic fingerprint of the first // lengthSec seconds of the file. -func computeChromaprint(ctx context.Context, path string, lengthSec int) ([]int32, error) { - out, err := runFingerprintTool(ctx, "fpcalc", fpcalcArgs(path, lengthSec)) +func computeChromaprint(ctx context.Context, path string, lengthSec int32) ([]int32, error) { + out, err := runFingerprintTool(ctx, "fpcalc", fpcalcArgs(path, int(lengthSec))) if err != nil { return nil, err } diff --git a/internal/library/fingerprint_backfill.go b/internal/library/fingerprint_backfill.go index 316a335b..eb19c1c8 100644 --- a/internal/library/fingerprint_backfill.go +++ b/internal/library/fingerprint_backfill.go @@ -40,10 +40,11 @@ const fingerprintBackfillTick = time.Hour // so tracks the scan adds mid-pass are not stuck behind one enormous page. const fingerprintBackfillBatch = 50 -// fingerprintBackfillConcurrency is how many files are decoded at once. Two is -// deliberately low: fpcalc and the stream hash compete with playback transcoding -// for CPU and with streaming for the mount, and a backfill that makes playback -// stutter is worse than one that takes longer. Operator-tunable in #3913. +// fingerprintBackfillConcurrency is the shipped value of the concurrency setting +// (#3913): how many files are decoded at once. Two is deliberately low: fpcalc +// and the stream hash compete with playback transcoding for CPU and with +// streaming for the mount, and a backfill that makes playback stutter is worse +// than one that takes longer. const fingerprintBackfillConcurrency = 2 // BackfillFingerprintsResult tallies one pass. @@ -68,24 +69,27 @@ func (r *BackfillFingerprintsResult) add(o fingerprintOutcome) { // FingerprintBackfillWorker fingerprints the tracks the scan never will. type FingerprintBackfillWorker struct { - pool *pgxpool.Pool - logger *slog.Logger - tick time.Duration - batch int32 - concurrency int + pool *pgxpool.Pool + logger *slog.Logger + settings *FingerprintSettingsService + tick time.Duration + batch int32 // fingerprint is a field for the same reason as Scanner.fingerprint: an // integration test pins which tracks a pass touches, not what the tools print. - fingerprint func(ctx context.Context, path string) fingerprintResult + fingerprint func(ctx context.Context, path string, opts fingerprintOptions) fingerprintResult } // NewFingerprintBackfillWorker builds a worker with the production cadence. -func NewFingerprintBackfillWorker(pool *pgxpool.Pool, logger *slog.Logger) *FingerprintBackfillWorker { +// settings is shared with the scanner and the admin API; nil runs on defaults. +func NewFingerprintBackfillWorker( + pool *pgxpool.Pool, logger *slog.Logger, settings *FingerprintSettingsService, +) *FingerprintBackfillWorker { return &FingerprintBackfillWorker{ pool: pool, logger: logger, + settings: settings, tick: fingerprintBackfillTick, batch: fingerprintBackfillBatch, - concurrency: fingerprintBackfillConcurrency, fingerprint: computeFingerprint, } } @@ -129,6 +133,12 @@ func (w *FingerprintBackfillWorker) runOnce(ctx context.Context) { // cursor is what lets a pass end: an inconclusive attempt writes no row, so a // file that keeps timing out would otherwise be listed again immediately and // retried forever within the pass. +// +// Settings are read before every batch, so a save takes effect within a batch +// rather than an hour (#3913): switching fingerprinting off ends the pass, a new +// concurrency applies to the next batch, and a new length restarts the walk from +// the top at that length, because every row written at the old one went stale +// the moment it changed. func (w *FingerprintBackfillWorker) pass(ctx context.Context) (BackfillFingerprintsResult, error) { q := dbq.New(w.pool) var ( @@ -137,15 +147,25 @@ func (w *FingerprintBackfillWorker) pass(ctx context.Context) (BackfillFingerpri ) // The all-zero uuid sorts before every real id. Valid must be true: a NULL // cursor would make "id > NULL" match nothing and every pass a silent no-op. - after := pgtype.UUID{Valid: true} + start := pgtype.UUID{Valid: true} + after := start + lengthSec := w.settings.Get().ChromaprintLengthSec for { if err := ctx.Err(); err != nil { return res, err } + cfg := w.settings.Get() + if !cfg.Enabled { + return res, nil + } + if cfg.ChromaprintLengthSec != lengthSec { + lengthSec, after = cfg.ChromaprintLengthSec, start + } rows, err := q.ListTracksNeedingFingerprint(ctx, dbq.ListTracksNeedingFingerprintParams{ - CurrentVersion: fingerprintVersion, - AfterID: after, - BatchLimit: w.batch, + CurrentVersion: fingerprintVersion, + ChromaprintLengthSec: lengthSec, + AfterID: after, + BatchLimit: w.batch, }) if err != nil { return res, fmt.Errorf("list tracks needing fingerprint: %w", err) @@ -154,7 +174,10 @@ func (w *FingerprintBackfillWorker) pass(ctx context.Context) (BackfillFingerpri return res, nil } - sem := make(chan struct{}, w.concurrency) + opts := fingerprintOptions{lengthSec: lengthSec, chromaprint: true} + // Validation keeps concurrency at one or more; the floor guards a zero + // that would block the first send for ever. + sem := make(chan struct{}, max(1, int(cfg.BackfillConcurrency))) var wg sync.WaitGroup for _, row := range rows { if ctx.Err() != nil { @@ -170,7 +193,7 @@ func (w *FingerprintBackfillWorker) pass(ctx context.Context) (BackfillFingerpri w.logger.Error("fingerprint backfill: track panicked", "path", path, "panic", r) } }() - outcome := storeFingerprint(ctx, q, w.logger, trackID, path, w.fingerprintFile(ctx, path)) + outcome := storeFingerprint(ctx, q, w.logger, trackID, path, w.fingerprintFile(ctx, path, opts), lengthSec) mu.Lock() res.add(outcome) mu.Unlock() @@ -181,16 +204,21 @@ func (w *FingerprintBackfillWorker) pass(ctx context.Context) (BackfillFingerpri } } -func (w *FingerprintBackfillWorker) fingerprintFile(ctx context.Context, path string) fingerprintResult { +func (w *FingerprintBackfillWorker) fingerprintFile(ctx context.Context, path string, opts fingerprintOptions) fingerprintResult { if w.fingerprint == nil { - return computeFingerprint(ctx, path) + return computeFingerprint(ctx, path, opts) } - return w.fingerprint(ctx, path) + return w.fingerprint(ctx, path, opts) } // FingerprintCoverage reports how much of the library carries a current // fingerprint, for the admin gauge. It lives here, beside the backfill, so the -// version it counts against is the one the backfill writes. -func FingerprintCoverage(ctx context.Context, pool *pgxpool.Pool) (dbq.GetFingerprintCoverageRow, error) { - return dbq.New(pool).GetFingerprintCoverage(ctx, fingerprintVersion) +// version and length it counts against are the ones the backfill writes. +func FingerprintCoverage( + ctx context.Context, pool *pgxpool.Pool, cfg FingerprintSettings, +) (dbq.GetFingerprintCoverageRow, error) { + return dbq.New(pool).GetFingerprintCoverage(ctx, dbq.GetFingerprintCoverageParams{ + CurrentVersion: fingerprintVersion, + ChromaprintLengthSec: cfg.ChromaprintLengthSec, + }) } diff --git a/internal/library/fingerprint_backfill_test.go b/internal/library/fingerprint_backfill_test.go index 8b5b81c9..fb3a166a 100644 --- a/internal/library/fingerprint_backfill_test.go +++ b/internal/library/fingerprint_backfill_test.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "log/slog" + "maps" "path/filepath" "sync" "testing" @@ -48,7 +49,7 @@ func TestFingerprintBackfill_Integration(t *testing.T) { } { if err := q.UpsertTrackFingerprint(ctx, dbq.UpsertTrackFingerprintParams{ TrackID: seed.track.ID, AudioStreamSha256: sum, Chromaprint: []int32{1}, - FingerprintVersion: seed.version, + FingerprintVersion: seed.version, ChromaprintLengthSec: defaultChromaprintLengthSec, }); err != nil { t.Fatalf("seed fingerprint: %v", err) } @@ -59,13 +60,24 @@ func TestFingerprintBackfill_Integration(t *testing.T) { var mu sync.Mutex calls := map[string]int{} - w := NewFingerprintBackfillWorker(pool, slog.New(slog.NewTextHandler(io.Discard, nil))) + settings, err := NewFingerprintSettingsService(ctx, pool) + if err != nil { + t.Fatalf("fingerprint settings: %v", err) + } + t.Cleanup(func() { + if _, err := settings.Set(context.Background(), DefaultFingerprintSettings); err != nil { + t.Errorf("restore fingerprint settings: %v", err) + } + }) + w := NewFingerprintBackfillWorker(pool, slog.New(slog.NewTextHandler(io.Discard, nil)), settings) // A batch of one forces the keyset cursor across several queries in a pass. w.batch = 1 - w.fingerprint = func(_ context.Context, path string) fingerprintResult { + lengths := map[int32]int{} + w.fingerprint = func(_ context.Context, path string, opts fingerprintOptions) fingerprintResult { name := filepath.Base(path) mu.Lock() calls[name]++ + lengths[opts.lengthSec]++ mu.Unlock() switch name { case "stall.mp3": @@ -126,7 +138,7 @@ func TestFingerprintBackfill_Integration(t *testing.T) { } // 4. The gauge counts what the passes wrote, and its buckets add up. - cov, err := FingerprintCoverage(ctx, pool) + cov, err := FingerprintCoverage(ctx, pool, settings.Get()) if err != nil { t.Fatalf("coverage: %v", err) } @@ -138,6 +150,64 @@ func TestFingerprintBackfill_Integration(t *testing.T) { if cov.Fingerprinted+cov.Rejected+cov.Pending != cov.Total { t.Errorf("coverage buckets %+v do not sum to the total", cov) } + + // 5. Changing the length (#3913) makes every stored row stale at once — the + // gauge shows the whole library pending before the backfill has touched a + // file — and the next pass re-derives each at the new length. The failure + // this prevents is invisible from the UI: prints at two lengths that silently + // never match. + shorter := DefaultFingerprintSettings + shorter.ChromaprintLengthSec = 60 + if _, err := settings.Set(ctx, shorter); err != nil { + t.Fatalf("change length: %v", err) + } + cov, err = FingerprintCoverage(ctx, pool, settings.Get()) + if err != nil { + t.Fatalf("coverage after length change: %v", err) + } + if cov.Total != 5 || cov.Pending != 5 { + t.Fatalf("coverage after length change = %+v, want all 5 tracks pending", cov) + } + mu.Lock() + clear(lengths) + mu.Unlock() + res, err = w.pass(ctx) + if err != nil { + t.Fatalf("new-length pass: %v", err) + } + if res.Processed != 5 { + t.Fatalf("new-length pass = %+v, want all 5 present tracks re-derived", res) + } + mu.Lock() + asked := maps.Clone(lengths) + mu.Unlock() + if len(asked) != 1 || asked[60] != 5 { + t.Fatalf("new-length pass asked for lengths %v, want 60s for all 5", asked) + } + var atOldLength int + if err := pool.QueryRow(ctx, + "SELECT count(*) FROM track_fingerprints WHERE chromaprint_length_sec <> 60").Scan(&atOldLength); err != nil { + t.Fatalf("count old-length rows: %v", err) + } + if atOldLength != 0 { + t.Fatalf("%d fingerprints are still at the old length after a complete pass", atOldLength) + } + + // 6. Switched off, the backfill does nothing, even with work waiting. + off := DefaultFingerprintSettings + off.Enabled = false + if _, err := settings.Set(ctx, off); err != nil { + t.Fatalf("switch fingerprinting off: %v", err) + } + addTrack("later") + res, err = w.pass(ctx) + if err != nil { + t.Fatalf("pass with fingerprinting off: %v", err) + } + if res.Processed != 0 || callCount("later.mp3") != 0 { + t.Fatalf("pass with fingerprinting off = %+v (later.mp3 tried %d times), want nothing done", + res, callCount("later.mp3")) + } } func TestBackfillFingerprintsResult_Add(t *testing.T) { diff --git a/internal/library/fingerprint_scan_test.go b/internal/library/fingerprint_scan_test.go index b2edb800..1e4c1d0a 100644 --- a/internal/library/fingerprint_scan_test.go +++ b/internal/library/fingerprint_scan_test.go @@ -60,9 +60,24 @@ func TestScanner_FingerprintsOnlyNewOrChangedBytes_Integration(t *testing.T) { result := fingerprintResult{streamSHA256: sum, chromaprint: chroma} calls := map[string]int{} - scanner := New(pool, logger, []string{root}) - scanner.fingerprint = func(_ context.Context, path string) fingerprintResult { + settings, err := NewFingerprintSettingsService(ctx, pool) + if err != nil { + t.Fatalf("fingerprint settings: %v", err) + } + if _, err := settings.Set(ctx, DefaultFingerprintSettings); err != nil { + t.Fatalf("reset fingerprint settings: %v", err) + } + t.Cleanup(func() { + if _, err := settings.Set(context.Background(), DefaultFingerprintSettings); err != nil { + t.Errorf("restore fingerprint settings: %v", err) + } + }) + + var lastOpts fingerprintOptions + scanner := New(pool, logger, []string{root}, settings) + scanner.fingerprint = func(_ context.Context, path string, opts fingerprintOptions) fingerprintResult { calls[path]++ + lastOpts = opts return result } @@ -78,14 +93,15 @@ func TestScanner_FingerprintsOnlyNewOrChangedBytes_Integration(t *testing.T) { sha []byte chroma []int32 version int16 + length int32 } stored := func(path string) (row, bool) { t.Helper() var r row err := pool.QueryRow(ctx, ` - SELECT f.audio_stream_sha256, f.chromaprint, f.fingerprint_version + SELECT f.audio_stream_sha256, f.chromaprint, f.fingerprint_version, f.chromaprint_length_sec FROM track_fingerprints f JOIN tracks t ON t.id = f.track_id - WHERE t.file_path = $1`, path).Scan(&r.sha, &r.chroma, &r.version) + WHERE t.file_path = $1`, path).Scan(&r.sha, &r.chroma, &r.version, &r.length) if errors.Is(err, pgx.ErrNoRows) { return row{}, false } @@ -113,8 +129,13 @@ func TestScanner_FingerprintsOnlyNewOrChangedBytes_Integration(t *testing.T) { if !ok { t.Fatal("first scan stored no fingerprint") } - if !bytes.Equal(got.sha, sum) || !slices.Equal(got.chroma, chroma) || got.version != fingerprintVersion { - t.Fatalf("stored %+v, want sha %x chromaprint %v version %d", got, sum, chroma, fingerprintVersion) + if !bytes.Equal(got.sha, sum) || !slices.Equal(got.chroma, chroma) || got.version != fingerprintVersion || + got.length != defaultChromaprintLengthSec { + t.Fatalf("stored %+v, want sha %x chromaprint %v version %d length %d", + got, sum, chroma, fingerprintVersion, defaultChromaprintLengthSec) + } + if !lastOpts.chromaprint || lastOpts.lengthSec != defaultChromaprintLengthSec { + t.Fatalf("first scan asked for %+v, want a chromaprint at the default length", lastOpts) } // 2. A tag-repair pass re-reads every unchanged file and must not @@ -171,4 +192,39 @@ func TestScanner_FingerprintsOnlyNewOrChangedBytes_Integration(t *testing.T) { if got.sha != nil || got.chroma != nil || got.version != fingerprintVersion { t.Fatalf("rejected file stored %+v, want both halves NULL at version %d", got, fingerprintVersion) } + + // 6. With fingerprinting off (#3913) the scan decodes nothing. It still asks + // for the stream hash — a demux, and what recognises a moved file — but stores + // no row, and a changed file's old row goes: it describes bytes that are gone. + result = fingerprintResult{streamSHA256: sum, chromaprint: chroma} + off := DefaultFingerprintSettings + off.Enabled = false + if _, err := settings.Set(ctx, off); err != nil { + t.Fatalf("switch fingerprinting off: %v", err) + } + touch(a, 4*time.Hour) + scan("fingerprinting-off scan") + if calls[a] != 5 || lastOpts.chromaprint { + t.Fatalf("fingerprinting-off scan: calls = %v, last options %+v; want a fifth call asking for no chromaprint", + calls, lastOpts) + } + if _, ok := stored(a); ok { + t.Fatal("with fingerprinting off, a changed file kept the previous bytes' fingerprint") + } + if _, ok := stored(b); !ok { + t.Fatal("with fingerprinting off, an unchanged file lost its fingerprint") + } + + // 7. The length setting reaches the scan, and is stored with the row. + longer := DefaultFingerprintSettings + longer.ChromaprintLengthSec = 90 + if _, err := settings.Set(ctx, longer); err != nil { + t.Fatalf("change length: %v", err) + } + touch(a, 5*time.Hour) + scan("new-length scan") + got, ok = stored(a) + if !ok || got.length != 90 || lastOpts.lengthSec != 90 || !lastOpts.chromaprint { + t.Fatalf("new-length scan stored %+v (present %v) after asking for %+v; want a chromaprint at 90s", got, ok, lastOpts) + } } diff --git a/internal/library/fingerprint_settings.go b/internal/library/fingerprint_settings.go new file mode 100644 index 00000000..fa971b80 --- /dev/null +++ b/internal/library/fingerprint_settings.go @@ -0,0 +1,163 @@ +package library + +import ( + "context" + "errors" + "fmt" + "sync" + "time" + + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// Fingerprinting settings (M400 #3913). +// +// Rule 25: what an operator might tune lives in the database and changes without +// a restart. One service instance is shared by the scanner, the backfill, the +// duplicate sweep and the admin API, so a save reaches all of them at once. +// +// The length is the dangerous knob: chromaprints taken at two lengths cannot be +// compared. Rather than bumping fingerprintVersion when it changes, every row +// records the length it was taken at (migration 0061) and every reader filters +// on the current one. A row at another length is stale to the backfill, pending +// to the gauge and invisible to the sweep, which is the effect a version bump +// would have, with one difference worth having: setting the length back makes +// rows not yet re-derived current again, instead of redoing the library twice. + +// Bounds for each setting, mirrored by migration 0061's CHECKs. +const ( + // Below 30s a print has too few items to align at the matcher's larger + // offsets and still overlap by minOverlapItems. Above 600s each print passes + // 20 KB and a candidate page of them grows past what one sweep should hold. + minChromaprintLengthSec = 30 + maxChromaprintLengthSec = 600 + // Unrelated audio sits near 0.5; a ceiling of 0.35 keeps a loosened + // threshold well clear of proposing noise. + minAcousticMaxBitErrorRate = 0.01 + maxAcousticMaxBitErrorRate = 0.35 + minBackfillConcurrency = 1 + maxBackfillConcurrency = 8 + minSweepIntervalHours = 1 + maxSweepIntervalHours = 168 + + defaultSweepIntervalHours = 1 +) + +// FingerprintSettings mirrors the fingerprint_settings row. +type FingerprintSettings struct { + // Enabled off stops every decode: the scan takes only the stream hash (a + // demux, and what recognises a moved file) and stores nothing, and the + // backfill idles. + Enabled bool + ChromaprintLengthSec int32 + AcousticMaxBitErrorRate float64 + BackfillConcurrency int32 + SweepIntervalHours int32 + // UpdatedAt is when the settings were last saved. Set by the database; + // ignored by Set. + UpdatedAt time.Time +} + +// DefaultFingerprintSettings mirrors migration 0061's column defaults, so a +// database that cannot be read still fingerprints the way a fresh install does. +var DefaultFingerprintSettings = FingerprintSettings{ + Enabled: true, + ChromaprintLengthSec: defaultChromaprintLengthSec, + AcousticMaxBitErrorRate: defaultAcousticMaxBitErrorRate, + BackfillConcurrency: fingerprintBackfillConcurrency, + SweepIntervalHours: defaultSweepIntervalHours, +} + +// ErrFingerprintSettingOutOfRange is returned by Set for a value migration +// 0061's CHECKs would reject, so the API answers 400 naming the field rather +// than surfacing a constraint violation. +var ErrFingerprintSettingOutOfRange = errors.New("fingerprint setting out of range") + +// FingerprintSettingsService caches the settings and owns their persistence. +// Cached because the scanner reads them for every file it fingerprints. +type FingerprintSettingsService struct { + pool *pgxpool.Pool + + mu sync.RWMutex + cur FingerprintSettings +} + +// NewFingerprintSettingsService loads once and caches. It always returns a +// usable service, holding the defaults when the load fails; the error says so. +func NewFingerprintSettingsService(ctx context.Context, pool *pgxpool.Pool) (*FingerprintSettingsService, error) { + s := &FingerprintSettingsService{pool: pool, cur: DefaultFingerprintSettings} + row, err := dbq.New(pool).GetFingerprintSettings(ctx) + if err != nil { + return s, fmt.Errorf("fingerprint settings: load: %w", err) + } + s.cur = fingerprintSettingsFromRow(row) + return s, nil +} + +// Get returns the cached settings. A nil service answers with the defaults, so +// a Scanner or worker built without one fingerprints as a fresh install would. +func (s *FingerprintSettingsService) Get() FingerprintSettings { + if s == nil { + return DefaultFingerprintSettings + } + s.mu.RLock() + defer s.mu.RUnlock() + return s.cur +} + +// Set validates, persists and re-caches. +func (s *FingerprintSettingsService) Set(ctx context.Context, in FingerprintSettings) (FingerprintSettings, error) { + if err := validateFingerprintSettings(in); err != nil { + return FingerprintSettings{}, err + } + if s == nil { + return FingerprintSettings{}, errors.New("fingerprint settings: no settings service") + } + row, err := dbq.New(s.pool).UpdateFingerprintSettings(ctx, dbq.UpdateFingerprintSettingsParams{ + Enabled: in.Enabled, + ChromaprintLengthSec: in.ChromaprintLengthSec, + AcousticMaxBitErrorRate: in.AcousticMaxBitErrorRate, + BackfillConcurrency: in.BackfillConcurrency, + SweepIntervalHours: in.SweepIntervalHours, + }) + if err != nil { + return FingerprintSettings{}, fmt.Errorf("fingerprint settings: save: %w", err) + } + out := fingerprintSettingsFromRow(row) + s.mu.Lock() + s.cur = out + s.mu.Unlock() + return out, nil +} + +func validateFingerprintSettings(in FingerprintSettings) error { + switch { + case in.ChromaprintLengthSec < minChromaprintLengthSec || in.ChromaprintLengthSec > maxChromaprintLengthSec: + return fmt.Errorf("%w: chromaprint_length_sec must be %d-%d", + ErrFingerprintSettingOutOfRange, minChromaprintLengthSec, maxChromaprintLengthSec) + // Written as a negated range so NaN, which fails every comparison, is refused. + case !(in.AcousticMaxBitErrorRate >= minAcousticMaxBitErrorRate && in.AcousticMaxBitErrorRate <= maxAcousticMaxBitErrorRate): + return fmt.Errorf("%w: acoustic_max_bit_error_rate must be %.2f-%.2f", + ErrFingerprintSettingOutOfRange, minAcousticMaxBitErrorRate, maxAcousticMaxBitErrorRate) + case in.BackfillConcurrency < minBackfillConcurrency || in.BackfillConcurrency > maxBackfillConcurrency: + return fmt.Errorf("%w: backfill_concurrency must be %d-%d", + ErrFingerprintSettingOutOfRange, minBackfillConcurrency, maxBackfillConcurrency) + case in.SweepIntervalHours < minSweepIntervalHours || in.SweepIntervalHours > maxSweepIntervalHours: + return fmt.Errorf("%w: sweep_interval_hours must be %d-%d", + ErrFingerprintSettingOutOfRange, minSweepIntervalHours, maxSweepIntervalHours) + } + return nil +} + +func fingerprintSettingsFromRow(row dbq.FingerprintSetting) FingerprintSettings { + return FingerprintSettings{ + Enabled: row.Enabled, + ChromaprintLengthSec: row.ChromaprintLengthSec, + AcousticMaxBitErrorRate: row.AcousticMaxBitErrorRate, + BackfillConcurrency: row.BackfillConcurrency, + SweepIntervalHours: row.SweepIntervalHours, + UpdatedAt: row.UpdatedAt.Time, + } +} diff --git a/internal/library/fingerprint_settings_test.go b/internal/library/fingerprint_settings_test.go new file mode 100644 index 00000000..95d5837b --- /dev/null +++ b/internal/library/fingerprint_settings_test.go @@ -0,0 +1,146 @@ +package library + +import ( + "context" + "errors" + "math" + "testing" + "time" +) + +func TestValidateFingerprintSettings(t *testing.T) { + with := func(edit func(*FingerprintSettings)) FingerprintSettings { + s := DefaultFingerprintSettings + edit(&s) + return s + } + valid := map[string]FingerprintSettings{ + "defaults": DefaultFingerprintSettings, + "shortest length": with(func(s *FingerprintSettings) { s.ChromaprintLengthSec = minChromaprintLengthSec }), + "longest length": with(func(s *FingerprintSettings) { s.ChromaprintLengthSec = maxChromaprintLengthSec }), + "strictest match": with(func(s *FingerprintSettings) { s.AcousticMaxBitErrorRate = minAcousticMaxBitErrorRate }), + "loosest match": with(func(s *FingerprintSettings) { s.AcousticMaxBitErrorRate = maxAcousticMaxBitErrorRate }), + "fewest at once": with(func(s *FingerprintSettings) { s.BackfillConcurrency = minBackfillConcurrency }), + "most at once": with(func(s *FingerprintSettings) { s.BackfillConcurrency = maxBackfillConcurrency }), + "shortest interval": with(func(s *FingerprintSettings) { s.SweepIntervalHours = minSweepIntervalHours }), + "longest interval": with(func(s *FingerprintSettings) { s.SweepIntervalHours = maxSweepIntervalHours }), + "switched off": with(func(s *FingerprintSettings) { s.Enabled = false }), + } + for name, s := range valid { + if err := validateFingerprintSettings(s); err != nil { + t.Errorf("%s: rejected: %v", name, err) + } + } + invalid := map[string]FingerprintSettings{ + "length too short": with(func(s *FingerprintSettings) { s.ChromaprintLengthSec = minChromaprintLengthSec - 1 }), + "length too long": with(func(s *FingerprintSettings) { s.ChromaprintLengthSec = maxChromaprintLengthSec + 1 }), + "match too strict": with(func(s *FingerprintSettings) { s.AcousticMaxBitErrorRate = minAcousticMaxBitErrorRate - 0.001 }), + "match too loose": with(func(s *FingerprintSettings) { s.AcousticMaxBitErrorRate = maxAcousticMaxBitErrorRate + 0.001 }), + "match not a number": with(func(s *FingerprintSettings) { s.AcousticMaxBitErrorRate = math.NaN() }), + "none at once": with(func(s *FingerprintSettings) { s.BackfillConcurrency = 0 }), + "too many at once": with(func(s *FingerprintSettings) { s.BackfillConcurrency = maxBackfillConcurrency + 1 }), + "no interval": with(func(s *FingerprintSettings) { s.SweepIntervalHours = 0 }), + "interval too long": with(func(s *FingerprintSettings) { s.SweepIntervalHours = maxSweepIntervalHours + 1 }), + } + for name, s := range invalid { + if err := validateFingerprintSettings(s); !errors.Is(err, ErrFingerprintSettingOutOfRange) { + t.Errorf("%s: err = %v, want ErrFingerprintSettingOutOfRange", name, err) + } + } +} + +func TestFingerprintSettingsService_NilServesDefaults(t *testing.T) { + var s *FingerprintSettingsService + if got := s.Get(); got != DefaultFingerprintSettings { + t.Fatalf("nil service Get = %+v, want the defaults", got) + } + // Validation still runs first, so a bad value is named rather than hidden + // behind the missing service. + bad := DefaultFingerprintSettings + bad.BackfillConcurrency = 0 + if _, err := s.Set(context.Background(), bad); !errors.Is(err, ErrFingerprintSettingOutOfRange) { + t.Fatalf("nil service Set of a bad value: err = %v, want ErrFingerprintSettingOutOfRange", err) + } +} + +func TestFingerprintSettingsService_Integration(t *testing.T) { + pool := newPool(t) + ctx := context.Background() + reload := func(step string) FingerprintSettings { + t.Helper() + fresh, err := NewFingerprintSettingsService(ctx, pool) + if err != nil { + t.Fatalf("%s: load: %v", step, err) + } + return fresh.Get() + } + withoutTime := func(s FingerprintSettings) FingerprintSettings { + s.UpdatedAt = time.Time{} + return s + } + + svc, err := NewFingerprintSettingsService(ctx, pool) + if err != nil { + t.Fatalf("load: %v", err) + } + // ResetDB puts every column back to its migration default, so this pins the + // Go defaults to migration 0061's. Were they to drift, a database that could + // not be read would fingerprint differently from one that could. + loaded := svc.Get() + if loaded.UpdatedAt.IsZero() { + t.Fatal("loaded settings carry no updated_at") + } + if withoutTime(loaded) != DefaultFingerprintSettings { + t.Fatalf("stored defaults = %+v, want the Go defaults %+v", withoutTime(loaded), DefaultFingerprintSettings) + } + + // Every bound the service accepts, the table accepts too. A CHECK tighter + // than validate would turn a value the card allows into a 500. + lowest := FingerprintSettings{ + Enabled: false, + ChromaprintLengthSec: minChromaprintLengthSec, + AcousticMaxBitErrorRate: minAcousticMaxBitErrorRate, + BackfillConcurrency: minBackfillConcurrency, + SweepIntervalHours: minSweepIntervalHours, + } + highest := FingerprintSettings{ + Enabled: true, + ChromaprintLengthSec: maxChromaprintLengthSec, + AcousticMaxBitErrorRate: maxAcousticMaxBitErrorRate, + BackfillConcurrency: maxBackfillConcurrency, + SweepIntervalHours: maxSweepIntervalHours, + } + for _, step := range []struct { + name string + want FingerprintSettings + }{{"lowest", lowest}, {"highest", highest}} { + saved, err := svc.Set(ctx, step.want) + if err != nil { + t.Fatalf("%s: save: %v", step.name, err) + } + // A save must move updated_at forward: it is what makes a sweep due. + if !saved.UpdatedAt.After(loaded.UpdatedAt) { + t.Fatalf("%s: updated_at %v did not move past %v", step.name, saved.UpdatedAt, loaded.UpdatedAt) + } + if withoutTime(saved) != step.want || withoutTime(svc.Get()) != step.want { + t.Fatalf("%s: saved %+v, cached %+v, want %+v", step.name, saved, svc.Get(), step.want) + } + if got := withoutTime(reload(step.name)); got != step.want { + t.Fatalf("%s: table holds %+v, want %+v", step.name, got, step.want) + } + } + + // An out-of-range save changes nothing, in the cache or the table. + before := svc.Get() + bad := highest + bad.ChromaprintLengthSec = maxChromaprintLengthSec + 1 + if _, err := svc.Set(ctx, bad); !errors.Is(err, ErrFingerprintSettingOutOfRange) { + t.Fatalf("out-of-range save: err = %v, want ErrFingerprintSettingOutOfRange", err) + } + if svc.Get() != before { + t.Fatalf("out-of-range save changed the cache to %+v", svc.Get()) + } + if got := reload("after out-of-range save"); got != before { + t.Fatalf("out-of-range save changed the table to %+v", got) + } +} diff --git a/internal/library/scanner.go b/internal/library/scanner.go index 7b32507a..f2c00c32 100644 --- a/internal/library/scanner.go +++ b/internal/library/scanner.go @@ -79,11 +79,14 @@ type Scanner struct { // integration test can substitute a deterministic one: CI has no real audio // to fingerprint, and what the test pins is WHEN the scan fingerprints, not // what the tools print. Call it through fingerprintFile. - fingerprint func(ctx context.Context, path string) fingerprintResult + fingerprint func(ctx context.Context, path string, opts fingerprintOptions) fingerprintResult + // settings is the operator's fingerprinting policy (#3913), shared with the + // workers and the admin API. Nil fingerprints with the defaults. + settings *FingerprintSettingsService } -func New(pool *pgxpool.Pool, logger *slog.Logger, paths []string) *Scanner { - return &Scanner{pool: pool, logger: logger, paths: paths, fingerprint: computeFingerprint} +func New(pool *pgxpool.Pool, logger *slog.Logger, paths []string, settings *FingerprintSettingsService) *Scanner { + return &Scanner{pool: pool, logger: logger, paths: paths, fingerprint: computeFingerprint, settings: settings} } // Scan walks every configured root and upserts any audio file whose mtime is @@ -313,10 +316,18 @@ func (s *Scanner) scanFile( // // Computed before move adoption so adoption can match on the audio hash // (#3914); stored after the upsert, once the row id is known. + // + // With fingerprinting switched off (#3913) the scan decodes nothing, but it + // still takes the stream hash: a demux rather than a decode, and the only + // thing that recognises a moved file with no MBID. var fp fingerprintResult + fpCfg := s.settings.Get() fingerprinted := !unchanged if fingerprinted { - fp = s.fingerprintFile(ctx, path) + fp = s.fingerprintFile(ctx, path, fingerprintOptions{ + lengthSec: fpCfg.ChromaprintLengthSec, + chromaprint: fpCfg.Enabled, + }) } // A path we've never seen might not be a new track — it might be one that @@ -384,7 +395,13 @@ func (s *Scanner) scanFile( s.logger.Warn("library scan: LogChange track upsert failed", "track_id", track.ID, "err", err) } if fingerprinted { - storeFingerprint(ctx, q, s.logger, track.ID, path, fp) + if fpCfg.Enabled { + storeFingerprint(ctx, q, s.logger, track.ID, path, fp, fpCfg.ChromaprintLengthSec) + } else if err := q.DeleteTrackFingerprint(ctx, track.ID); err != nil { + // Off, nothing is stored — but a row describing the previous bytes + // must not outlive them, or the sweep would compare audio that is gone. + s.logger.Warn("fingerprint: clearing stale fingerprint failed", "path", path, "err", err) + } } if knownTrack { diff --git a/internal/library/scanner_test.go b/internal/library/scanner_test.go index bcc56a79..b17741a9 100644 --- a/internal/library/scanner_test.go +++ b/internal/library/scanner_test.go @@ -81,7 +81,7 @@ func TestScanner_Integration(t *testing.T) { "TIT2": "Solo", "TPE1": "The Artist Y", "TALB": "Y Album", "TRCK": "1", }) - scanner := New(pool, logger, []string{root}) + scanner := New(pool, logger, []string{root}, nil) stats, err := scanner.Scan(ctx, nil) if err != nil { t.Fatalf("first scan: %v", err) @@ -259,7 +259,7 @@ func TestScanner_AdoptsMovedFile_Integration(t *testing.T) { }) } - scanner := New(pool, logger, []string{root}) + scanner := New(pool, logger, []string{root}, nil) if _, err := scanner.Scan(ctx, nil); err != nil { t.Fatalf("first scan: %v", err) } diff --git a/internal/server/server.go b/internal/server/server.go index 37b13da2..c7eafbac 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -100,6 +100,11 @@ type Server struct { // and serves the admin tuning endpoints from it. Router() constructs // a fallback when nil (tests). RecSettings *recsettings.Service + // FingerprintSettings is the DB-backed fingerprinting policy (M400 #3913). + // Constructed in cmd/minstrel/main.go and shared with the scanner and the + // fingerprint workers, so a save from the admin card reaches them without a + // restart. Router() constructs a fallback when nil (tests). + FingerprintSettings *library.FingerprintSettingsService // 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 @@ -186,7 +191,17 @@ func (s *Server) Router() http.Handler { s.Logger.Error("server: recsettings boot failed", "err", err) } } - api.Mount(r, s.Pool, s.Logger, writer, s.RecommendationCfg, recSettings, lidarrCfg, lidarrReqs, lidarrQuar, tracksSvc, playlistsSvc, s.CoverEnricher, s.CoverSettings, s.TagSettings, s.LibraryScanner, s.ScanCfg, s.DataDir, smtpSender, bus, s.PlaylistScheduler, s.StreamSecret, netSettings, reacqSettings) + fpSettings := s.FingerprintSettings + if fpSettings == nil { + // Test contexts construct Server without main.go's wiring. Always + // usable: a failed load serves the defaults. + var err error + fpSettings, err = library.NewFingerprintSettingsService(context.Background(), s.Pool) + if err != nil { + s.Logger.Warn("fingerprint settings unavailable; serving defaults", "err", err) + } + } + api.Mount(r, s.Pool, s.Logger, writer, s.RecommendationCfg, recSettings, lidarrCfg, lidarrReqs, lidarrQuar, tracksSvc, playlistsSvc, s.CoverEnricher, s.CoverSettings, s.TagSettings, s.LibraryScanner, s.ScanCfg, s.DataDir, smtpSender, bus, s.PlaylistScheduler, s.StreamSecret, netSettings, reacqSettings, fpSettings) // /api/admin/scan is the only admin route owned by the server package // (it needs the Scanner). Register it as a single inline-middleware // route — using r.Route("/api/admin", ...) here would create a second diff --git a/web/src/lib/api/admin.fingerprints.test.ts b/web/src/lib/api/admin.fingerprints.test.ts index 9442fe45..1ae3669c 100644 --- a/web/src/lib/api/admin.fingerprints.test.ts +++ b/web/src/lib/api/admin.fingerprints.test.ts @@ -1,8 +1,14 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { getFingerprintCoverage, type FingerprintCoverage } from './admin'; +import { + getFingerprintCoverage, + getFingerprintSettings, + updateFingerprintSettings, + type FingerprintCoverage, + type FingerprintSettings +} from './admin'; vi.mock('./client', () => ({ - api: { get: vi.fn(), post: vi.fn() } + api: { get: vi.fn(), post: vi.fn(), put: vi.fn() } })); import { api } from './client'; @@ -15,7 +21,8 @@ describe('admin fingerprint coverage API', () => { total: 18026, fingerprinted: 9400, rejected: 12, - pending: 8614 + pending: 8614, + enabled: true }; (api.get as unknown as ReturnType).mockResolvedValueOnce(sample); const got = await getFingerprintCoverage(); @@ -24,9 +31,31 @@ describe('admin fingerprint coverage API', () => { }); it('buckets sum to the total', async () => { - const sample: FingerprintCoverage = { total: 10, fingerprinted: 6, rejected: 1, pending: 3 }; + const sample: FingerprintCoverage = { + total: 10, + fingerprinted: 6, + rejected: 1, + pending: 3, + enabled: true + }; (api.get as unknown as ReturnType).mockResolvedValueOnce(sample); const got = await getFingerprintCoverage(); expect(got.fingerprinted + got.rejected + got.pending).toBe(got.total); }); + + it('reads and saves the fingerprinting settings at one path', async () => { + const settings: FingerprintSettings = { + enabled: true, + chromaprint_length_sec: 120, + acoustic_max_bit_error_rate: 0.15, + backfill_concurrency: 2, + sweep_interval_hours: 1 + }; + (api.get as unknown as ReturnType).mockResolvedValueOnce(settings); + (api.put as unknown as ReturnType).mockResolvedValueOnce(settings); + await getFingerprintSettings(); + await updateFingerprintSettings(settings); + expect(api.get).toHaveBeenCalledWith('/api/admin/library/fingerprint-settings'); + expect(api.put).toHaveBeenCalledWith('/api/admin/library/fingerprint-settings', settings); + }); }); diff --git a/web/src/lib/api/admin.ts b/web/src/lib/api/admin.ts index 07e3940e..8f97e103 100644 --- a/web/src/lib/api/admin.ts +++ b/web/src/lib/api/admin.ts @@ -322,6 +322,9 @@ export type FingerprintCoverage = { fingerprinted: number; rejected: number; pending: number; + // False when the operator has switched fingerprinting off (#3913): pending + // then never shrinks, and nothing should read as progress. + enabled: boolean; }; export async function getFingerprintCoverage(): Promise { @@ -339,6 +342,28 @@ export function createFingerprintCoverageQuery() { }); } +// Fingerprinting settings (#3913) ------------------------------------------ + +export type FingerprintSettings = { + enabled: boolean; + chromaprint_length_sec: number; + // The share of fingerprint bits two copies may disagree on and still be + // proposed as one recording. The card shows it as a match percentage. + acoustic_max_bit_error_rate: number; + backfill_concurrency: number; + sweep_interval_hours: number; +}; + +export async function getFingerprintSettings(): Promise { + return api.get('/api/admin/library/fingerprint-settings'); +} + +export async function updateFingerprintSettings( + s: FingerprintSettings +): Promise { + return api.put('/api/admin/library/fingerprint-settings', s); +} + // Cover-art providers ------------------------------------------------------ export type CoverProviderCapability = 'album_cover' | 'artist_thumb' | 'artist_fanart'; diff --git a/web/src/lib/api/errors.test.ts b/web/src/lib/api/errors.test.ts index d728c1fc..fc874041 100644 --- a/web/src/lib/api/errors.test.ts +++ b/web/src/lib/api/errors.test.ts @@ -75,6 +75,13 @@ describe('errMessage detail codes (#3918)', () => { ); }); + test('invalid_setting appends the field and range the server names', () => { + const msg = 'fingerprint setting out of range: chromaprint_length_sec must be 30-600'; + expect(errMessage({ code: 'invalid_setting', message: msg })).toBe( + `${ERROR_COPY.invalid_setting} ${msg}` + ); + }); + // Server messages are usually internal detail. Appending them for every code // would leak things like driver errors into toasts; this pins the scope. test('other codes never carry the server message', () => { diff --git a/web/src/lib/api/errors.ts b/web/src/lib/api/errors.ts index 1f09f55d..3de3b71b 100644 --- a/web/src/lib/api/errors.ts +++ b/web/src/lib/api/errors.ts @@ -15,7 +15,12 @@ export function errCode(err: unknown): string { * server messages are internal detail and must never reach a toast. Mirrored * in Android's ErrorCopy. */ -const DETAIL_CODES: ReadonlySet = new Set(['library_not_writable', 'file_delete_failed']); +const DETAIL_CODES: ReadonlySet = new Set([ + 'library_not_writable', + 'file_delete_failed', + // The server names the field and its range (#3913). + 'invalid_setting' +]); /** * Returns user-facing copy for an unknown error value. Looks up the diff --git a/web/src/lib/api/types.ts b/web/src/lib/api/types.ts index 76c5a645..4648ee5d 100644 --- a/web/src/lib/api/types.ts +++ b/web/src/lib/api/types.ts @@ -471,7 +471,13 @@ export type MergeDuplicateResult = { export type AdminDuplicatesResponse = { sweep: AdminDuplicateSweep; - fingerprints: { total: number; fingerprinted: number; rejected: number; pending: number }; + fingerprints: { + total: number; + fingerprinted: number; + rejected: number; + pending: number; + enabled: boolean; + }; total: number; limit: number; offset: number; diff --git a/web/src/lib/components/FingerprintSettingsCard.svelte b/web/src/lib/components/FingerprintSettingsCard.svelte new file mode 100644 index 00000000..85e766da --- /dev/null +++ b/web/src/lib/components/FingerprintSettingsCard.svelte @@ -0,0 +1,218 @@ + + +
+
+

Fingerprinting

+

+ How tracks are fingerprinted, and how alike two must sound to be proposed as the same + recording. Identical files are always found, whatever these say. +

+
+ + {#if loadError} +

+ Couldn't load fingerprinting settings. + +

+ {:else if form === null} +

Loading…

+ {:else} + + +
+ + + + + + + +
+ + {#if lengthChanged} +

+

+ {/if} + + {#if problems.length > 0} +
    + {#each problems as problem (problem)} +
  • {problem}
    • + {/each} +
+ {/if} + +
+ +
+ {/if} +
diff --git a/web/src/lib/components/FingerprintSettingsCard.test.ts b/web/src/lib/components/FingerprintSettingsCard.test.ts new file mode 100644 index 00000000..85ea770d --- /dev/null +++ b/web/src/lib/components/FingerprintSettingsCard.test.ts @@ -0,0 +1,141 @@ +import { afterEach, describe, expect, test, vi } from 'vitest'; +import { render, screen, fireEvent, waitFor } from '@testing-library/svelte'; +import type { FingerprintSettings } from '$lib/api/admin'; +import { ERROR_COPY } from '$lib/api/error-copy'; + +vi.mock('$lib/api/admin', () => ({ + getFingerprintSettings: vi.fn(), + updateFingerprintSettings: vi.fn() +})); + +vi.mock('$lib/stores/toast.svelte', () => ({ pushToast: vi.fn() })); + +import FingerprintSettingsCard from './FingerprintSettingsCard.svelte'; +import { getFingerprintSettings, updateFingerprintSettings } from '$lib/api/admin'; +import { pushToast } from '$lib/stores/toast.svelte'; + +const base: FingerprintSettings = { + enabled: true, + chromaprint_length_sec: 120, + acoustic_max_bit_error_rate: 0.15, + backfill_concurrency: 2, + sweep_interval_hours: 1 +}; + +afterEach(() => vi.clearAllMocks()); + +async function renderCard( + over: Partial = {}, + props: { libraryTotal?: number; onSaved?: () => void } = {} +) { + vi.mocked(getFingerprintSettings).mockResolvedValue({ ...base, ...over }); + const r = render(FingerprintSettingsCard, { props }); + await screen.findByRole('spinbutton', { name: /seconds of audio/i }); + return r; +} + +const saveButton = () => screen.getByRole('button', { name: /save/i }); + +describe('FingerprintSettingsCard', () => { + test('save is disabled until something changes', async () => { + await renderCard(); + expect(saveButton()).toHaveProperty('disabled', true); + await fireEvent.input(screen.getByRole('spinbutton', { name: /hours between sweeps/i }), { + target: { value: '6' } + }); + await waitFor(() => expect(saveButton()).toHaveProperty('disabled', false)); + }); + + // The matcher works in bit-error rates; the report shows match percentages. + // A card that showed 0.15 beside a report saying "96% match" would leave the + // operator converting in their head. + test('the threshold reads as a match percentage and saves as a bit-error rate', async () => { + vi.mocked(updateFingerprintSettings).mockResolvedValue({ + ...base, + acoustic_max_bit_error_rate: 0.1 + }); + await renderCard(); + const match = screen.getByRole('spinbutton', { name: /minimum match/i }) as HTMLInputElement; + expect(match.value).toBe('85'); + + await fireEvent.input(match, { target: { value: '90' } }); + await fireEvent.click(saveButton()); + await waitFor(() => + expect(updateFingerprintSettings).toHaveBeenCalledWith( + expect.objectContaining({ acoustic_max_bit_error_rate: 0.1 }) + ) + ); + }); + + // A new length re-fingerprints the whole library. Nothing else on the page + // would say so before the operator commits to it. + test('changing the length warns that every track is re-fingerprinted', async () => { + await renderCard({}, { libraryTotal: 18026 }); + expect(screen.queryByTestId('length-warning')).toBeNull(); + + await fireEvent.input(screen.getByRole('spinbutton', { name: /seconds of audio/i }), { + target: { value: '60' } + }); + const warning = await screen.findByTestId('length-warning'); + expect(warning.textContent).toMatch(/all\s+18,026\s+tracks/); + }); + + test('other changes carry no re-fingerprinting warning', async () => { + await renderCard({}, { libraryTotal: 18026 }); + await fireEvent.input(screen.getByRole('spinbutton', { name: /files fingerprinted at once/i }), { + target: { value: '4' } + }); + await waitFor(() => expect(saveButton()).toHaveProperty('disabled', false)); + expect(screen.queryByTestId('length-warning')).toBeNull(); + }); + + test('a value out of range blocks saving and says which', async () => { + await renderCard(); + await fireEvent.input(screen.getByRole('spinbutton', { name: /files fingerprinted at once/i }), { + target: { value: '12' } + }); + const problems = await screen.findByTestId('settings-problems'); + expect(problems.textContent).toMatch(/files fingerprinted at once must be from 1 to 8/i); + expect(saveButton()).toHaveProperty('disabled', true); + }); + + test('a rejected save surfaces the field the server names', async () => { + const message = 'fingerprint setting out of range: sweep_interval_hours must be 1-168'; + vi.mocked(updateFingerprintSettings).mockRejectedValue({ + code: 'invalid_setting', + message, + status: 400 + }); + await renderCard(); + await fireEvent.input(screen.getByRole('spinbutton', { name: /hours between sweeps/i }), { + target: { value: '6' } + }); + await fireEvent.click(saveButton()); + await waitFor(() => + expect(pushToast).toHaveBeenCalledWith(`${ERROR_COPY.invalid_setting} ${message}`, 'error') + ); + }); + + // Switching fingerprinting off changes what the page's counts mean. + test('a save tells the page, so its counts refresh', async () => { + const onSaved = vi.fn(); + vi.mocked(updateFingerprintSettings).mockResolvedValue({ ...base, enabled: false }); + await renderCard({}, { onSaved }); + + await fireEvent.click(screen.getByRole('checkbox', { name: /fingerprint tracks/i })); + await fireEvent.click(saveButton()); + await waitFor(() => expect(onSaved).toHaveBeenCalledTimes(1)); + expect(updateFingerprintSettings).toHaveBeenCalledWith( + expect.objectContaining({ enabled: false }) + ); + }); + + test('a failed load offers a retry rather than an empty card', async () => { + vi.mocked(getFingerprintSettings).mockRejectedValue(new Error('nope')); + render(FingerprintSettingsCard); + await waitFor(() => + expect(screen.getByText(/couldn't load fingerprinting settings/i)).toBeTruthy() + ); + expect(screen.getByRole('button', { name: /try again/i })).toBeTruthy(); + }); +}); diff --git a/web/src/lib/styles/error-copy.json b/web/src/lib/styles/error-copy.json index c18028a3..237d999a 100644 --- a/web/src/lib/styles/error-copy.json +++ b/web/src/lib/styles/error-copy.json @@ -45,6 +45,7 @@ "sweep_in_progress": "A duplicate sweep is already running.", "duplicate_group_not_pending": "That group has already been resolved.", "survivor_not_in_group": "That copy isn't part of this group any more.", + "invalid_setting": "That setting is out of range.", "album_not_found": "That album no longer exists.", "artist_not_found": "That artist no longer exists.", "playlist_not_found": "That playlist no longer exists.", diff --git a/web/src/routes/admin/+page.svelte b/web/src/routes/admin/+page.svelte index df5334ae..97feda2a 100644 --- a/web/src/routes/admin/+page.svelte +++ b/web/src/routes/admin/+page.svelte @@ -445,6 +445,11 @@ · {fingerprints.pending.toLocaleString()} pending {/if} + {#if fingerprints.enabled === false} + + · + fingerprinting is off + {/if} {#if fingerprints.rejected > 0} · 0} - {prints.pending.toLocaleString()} tracks are still waiting for a fingerprint and join - the comparison once they have one. + {#if prints.enabled === false} + Fingerprinting is off, so {prints.pending.toLocaleString()} tracks without a current + fingerprint aren't compared. + {:else} + {prints.pending.toLocaleString()} tracks are still waiting for a fingerprint and join + the comparison once they have one. + {/if} {/if}

{/if} @@ -191,8 +197,13 @@ {#if prints && prints.total > 0 && prints.fingerprinted === 0}

Nothing to compare yet.

- The library is still being fingerprinted — {prints.pending.toLocaleString()} tracks to go. - Duplicates appear here as the sweep finds them. + {#if prints.enabled === false} + Fingerprinting is off, so no track has a fingerprint to compare. Turn it on in the + settings below. + {:else} + The library is still being fingerprinted — {prints.pending.toLocaleString()} tracks to go. + Duplicates appear here as the sweep finds them. + {/if}

{:else if sweep?.state === 'never'}

The sweep hasn't run yet.

@@ -341,4 +352,8 @@ {/if} {/if} + + + query.refetch()} /> diff --git a/web/src/routes/admin/duplicates/duplicates.test.ts b/web/src/routes/admin/duplicates/duplicates.test.ts index 98d3c9d3..c7c7fa60 100644 --- a/web/src/routes/admin/duplicates/duplicates.test.ts +++ b/web/src/routes/admin/duplicates/duplicates.test.ts @@ -10,7 +10,17 @@ vi.mock('$lib/api/admin', () => ({ mergeDuplicateGroup: vi.fn().mockResolvedValue({ survivor_track_id: 'www-01', removed_paths: ['/music/Moe Shop/WWW (2020)/www-02.mp3'] - }) + }), + // The page embeds FingerprintSettingsCard, which loads its own settings from + // this module; the card has its own suite. + getFingerprintSettings: vi.fn().mockResolvedValue({ + enabled: true, + chromaprint_length_sec: 120, + acoustic_max_bit_error_rate: 0.15, + backfill_concurrency: 2, + sweep_interval_hours: 1 + }), + updateFingerprintSettings: vi.fn() })); import AdminDuplicatesPage from './+page.svelte'; @@ -33,7 +43,7 @@ const finishedSweep = { oversize_clusters: 0, error_message: null }; -const allFingerprinted = { total: 1200, fingerprinted: 1200, rejected: 0, pending: 0 }; +const allFingerprinted = { total: 1200, fingerprinted: 1200, rejected: 0, pending: 0, enabled: true }; function member(id: string, extra: Partial = {}) { return { @@ -120,12 +130,33 @@ describe('admin duplicates', () => { response({ groups: [], total: 0, - fingerprints: { total: 1200, fingerprinted: 0, rejected: 0, pending: 1200 } + fingerprints: { total: 1200, fingerprinted: 0, rejected: 0, pending: 1200, enabled: true } }) ); expect(text(screen.getByTestId('empty-state'))).toContain('still being fingerprinted'); }); + // Switched off, the backlog never shrinks. "Still being fingerprinted" would + // promise work nothing is doing (#3913). + test('with fingerprinting off the page says so instead of promising progress', () => { + renderWith( + response({ + groups: [], + total: 0, + fingerprints: { total: 1200, fingerprinted: 0, rejected: 0, pending: 1200, enabled: false } + }) + ); + const empty = text(screen.getByTestId('empty-state')); + expect(empty).toContain('Fingerprinting is off'); + expect(empty).not.toContain('still being fingerprinted'); + expect(text(screen.getByTestId('sweep-status'))).toContain('Fingerprinting is off'); + }); + + test('the fingerprinting settings are on this page', async () => { + renderWith(response()); + expect(await screen.findByRole('spinbutton', { name: /seconds of audio/i })).toBeTruthy(); + }); + test('empty before any sweep says the sweep has not run', () => { renderWith( response({