diff --git a/Dockerfile b/Dockerfile index 7aef6079..55b3690d 100644 --- a/Dockerfile +++ b/Dockerfile @@ -33,8 +33,14 @@ RUN go build -trimpath \ -o /out/minstrel ./cmd/minstrel FROM debian:bookworm-slim +# ffmpeg: duration probes and the exact-tier audio hash (a SHA-256 of the +# encoded audio packets, so no decode). libchromaprint-tools: fpcalc, the +# acoustic fingerprint that tells the same recording at two bitrates apart +# from two different recordings (M400). Both are baked in at build time so a +# deployed instance never fetches either (rule 164); fpcalc is shelled out +# rather than bound because CGO_ENABLED=0 above rules out cgo. RUN apt-get update \ - && apt-get install -y --no-install-recommends ca-certificates ffmpeg \ + && apt-get install -y --no-install-recommends ca-certificates ffmpeg libchromaprint-tools \ && rm -rf /var/lib/apt/lists/* RUN groupadd --system --gid 1000 minstrel \ diff --git a/internal/db/dbq/fingerprints.sql.go b/internal/db/dbq/fingerprints.sql.go new file mode 100644 index 00000000..f1b5e244 --- /dev/null +++ b/internal/db/dbq/fingerprints.sql.go @@ -0,0 +1,59 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: fingerprints.sql + +package dbq + +import ( + "context" + + "github.com/jackc/pgx/v5/pgtype" +) + +const deleteTrackFingerprint = `-- name: DeleteTrackFingerprint :exec +DELETE FROM track_fingerprints WHERE track_id = $1 +` + +// A file changed but could not be fingerprinted, for a reason unrelated to the +// file. The stored row describes the OLD bytes, so it goes and the backfill +// re-derives it — nothing may keep trusting a stale identity. +func (q *Queries) DeleteTrackFingerprint(ctx context.Context, trackID pgtype.UUID) error { + _, err := q.db.Exec(ctx, deleteTrackFingerprint, trackID) + return err +} + +const upsertTrackFingerprint = `-- name: UpsertTrackFingerprint :exec +INSERT INTO track_fingerprints ( + track_id, audio_stream_sha256, chromaprint, fingerprint_version +) VALUES ( + $1, $2, $3, + $4 +) +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() +` + +type UpsertTrackFingerprintParams struct { + TrackID pgtype.UUID + AudioStreamSha256 []byte + Chromaprint []int32 + FingerprintVersion int16 +} + +// Written whenever a track's fingerprint is derived: by the scan when a file is +// new or its bytes changed, and by the backfill (#3908) for rows derived by an +// older method. Replaces the row wholesale — a fingerprint of the old bytes has +// no standing once the file has changed. +func (q *Queries) UpsertTrackFingerprint(ctx context.Context, arg UpsertTrackFingerprintParams) error { + _, err := q.db.Exec(ctx, upsertTrackFingerprint, + arg.TrackID, + arg.AudioStreamSha256, + arg.Chromaprint, + arg.FingerprintVersion, + ) + return err +} diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index d0d4a5f1..07ca6f75 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -667,6 +667,14 @@ type Track struct { MissingSince pgtype.Timestamptz } +type TrackFingerprint struct { + TrackID pgtype.UUID + AudioStreamSha256 []byte + Chromaprint []int32 + FingerprintVersion int16 + ComputedAt pgtype.Timestamptz +} + type TrackSimilarity struct { TrackAID pgtype.UUID TrackBID pgtype.UUID diff --git a/internal/db/migrations/0058_track_fingerprints.down.sql b/internal/db/migrations/0058_track_fingerprints.down.sql new file mode 100644 index 00000000..c04a359c --- /dev/null +++ b/internal/db/migrations/0058_track_fingerprints.down.sql @@ -0,0 +1 @@ +DROP TABLE track_fingerprints; diff --git a/internal/db/migrations/0058_track_fingerprints.up.sql b/internal/db/migrations/0058_track_fingerprints.up.sql new file mode 100644 index 00000000..aa0f8c9c --- /dev/null +++ b/internal/db/migrations/0058_track_fingerprints.up.sql @@ -0,0 +1,38 @@ +-- 0058_track_fingerprints.up.sql — an acoustic identity per track (Scribe +-- milestone #400: #3905, #3906). +-- +-- A table of its own rather than columns on tracks, for the hot path's sake: +-- tracks is read with SELECT * by eight queries, among them ListTracksByAlbum, +-- SearchTracks and GetTracksByIDs — album pages, search, the Subsonic surface. +-- A ~4 KB chromaprint column on tracks would be de-TOASTed on every one of +-- those reads to carry a value only the duplicate sweep ever looks at. +-- +-- What a row means, which the backfill depends on: +-- no row never fingerprinted +-- fingerprint_version < current derived by an older method; re-derive it +-- fingerprint_version = current attempted; a NULL value means that tool +-- failed on this file, and it is not retried +-- until the file changes +-- A failure that says nothing about the file — a timeout, a cancelled scan, a +-- missing binary — writes no row at all, so the backfill tries again. +CREATE TABLE track_fingerprints ( + -- CASCADE is right here, unlike for the likes and play history M400's + -- merge has to carry across: a fingerprint describes one file's bytes and + -- means nothing once that file's row is gone. + track_id uuid PRIMARY KEY REFERENCES tracks (id) ON DELETE CASCADE, + -- SHA-256 of the ENCODED audio packets (ffmpeg -c:a copy -f hash), not of + -- decoded samples. internal/library/fingerprint.go says why. + audio_stream_sha256 bytea + CHECK (audio_stream_sha256 IS NULL OR octet_length(audio_stream_sha256) = 32), + -- fpcalc -raw -signed: the same 32 bits per item, stored signed because + -- integer is. + chromaprint integer[], + fingerprint_version smallint NOT NULL, + computed_at timestamptz NOT NULL DEFAULT now() +); + +-- The exact duplicate tier is an equality match on this column. Partial +-- because a NULL is never looked up — it only means the hash was not taken. +CREATE INDEX track_fingerprints_audio_stream_sha256 + ON track_fingerprints (audio_stream_sha256) + WHERE audio_stream_sha256 IS NOT NULL; diff --git a/internal/db/queries/fingerprints.sql b/internal/db/queries/fingerprints.sql new file mode 100644 index 00000000..ef0276b5 --- /dev/null +++ b/internal/db/queries/fingerprints.sql @@ -0,0 +1,22 @@ +-- name: UpsertTrackFingerprint :exec +-- Written whenever a track's fingerprint is derived: by the scan when a file is +-- new or its bytes changed, and by the backfill (#3908) for rows derived by an +-- 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 +) VALUES ( + sqlc.arg(track_id), sqlc.narg(audio_stream_sha256), sqlc.narg(chromaprint), + sqlc.arg(fingerprint_version) +) +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(); + +-- name: DeleteTrackFingerprint :exec +-- A file changed but could not be fingerprinted, for a reason unrelated to the +-- file. The stored row describes the OLD bytes, so it goes and the backfill +-- re-derives it — nothing may keep trusting a stale identity. +DELETE FROM track_fingerprints WHERE track_id = $1; diff --git a/internal/dbtest/reset.go b/internal/dbtest/reset.go index c71b9aae..a99db739 100644 --- a/internal/dbtest/reset.go +++ b/internal/dbtest/reset.go @@ -87,6 +87,7 @@ var dataTables = []string{ // pristine Discover knobs rather than whatever a previous test tuned. "discover_tuning", "recommendation_tuning_audit", + "track_fingerprints", // M400 "tracks", "albums", "artists", diff --git a/internal/library/fingerprint.go b/internal/library/fingerprint.go new file mode 100644 index 00000000..59ac7cb7 --- /dev/null +++ b/internal/library/fingerprint.go @@ -0,0 +1,296 @@ +package library + +import ( + "bytes" + "context" + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "os/exec" + "strconv" + "strings" + "time" + + "github.com/jackc/pgx/v5/pgtype" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// Acoustic identity (M400). +// +// Two values per track, because they answer different questions: +// +// audio_stream_sha256 a SHA-256 of the ENCODED audio packets. Equal means the +// same audio bytes, whatever the tags or container around +// them say. No threshold and no false positives — this is +// what catches two copies of one MP3 that differ only in +// their ID3 (#3885). +// +// chromaprint fpcalc's raw fingerprint. Close means the same +// recording, even at another bitrate or in another codec +// — the case an exact hash cannot see. +// +// Both shell out, in the shape probeDurationMs already set: a deadline on every +// call, and a failure that leaves the value unset rather than failing the file. +// A track with no fingerprint is never a duplicate candidate; it is still a +// track. + +// fingerprintTimeout bounds one ffmpeg hash or fpcalc call. Longer than +// probeTimeout because both read the audio rather than a header: the hash reads +// every packet and fpcalc decodes up to its -length. 60s leaves room for a large +// lossless file on a slow network mount; a call needing more is a stall, not a +// big file. +const fingerprintTimeout = 60 * time.Second + +// fingerprintWaitDelay bounds how long Output may keep waiting on the tool's +// pipes after the deadline has killed it. Without it, a child that left a +// descendant holding stdout open would block the scan past its own timeout. +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. +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. +const defaultChromaprintLengthSec = 120 + +// fpcalcStderrTail caps how much of a failing tool's stderr reaches the log. +const fpcalcStderrTail = 512 + +// streamHashArgs hashes the encoded audio packets, never decoded samples. +// +// -c:a copy is the point, not an optimisation. A decoded hash of a lossy file +// depends on the decoder's float maths and sample conversion, which can move +// between ffmpeg releases — so an image upgrade could silently change every +// stored hash, and yesterday's duplicate would stop matching today's copy. +// Packet bytes do not move. It is also far cheaper: demux only, no decode. +// +// -map 0:a keeps embedded cover art (an attached-picture video stream) out of +// the hash, so two copies of one recording carrying different art still match. +func streamHashArgs(path string) []string { + return []string{ + "-v", "error", + "-i", path, + "-map", "0:a", + "-c:a", "copy", + "-f", "hash", "-hash", "sha256", + "-", + } +} + +// fpcalcArgs asks for the raw fingerprint as SIGNED integers. +// +// -raw because the matcher compares items bit by bit, which the compressed form +// cannot do without being unpacked first. -signed because the column is Postgres +// integer[], which is signed: fpcalc's default prints uint32, and half of those +// values do not fit. Signed output is the same 32 bits with no reinterpretation +// step left to get wrong. +func fpcalcArgs(path string, lengthSec int) []string { + return []string{ + "-raw", "-signed", + "-length", strconv.Itoa(lengthSec), + path, + } +} + +// fingerprintResult is one attempt at both halves of a track's identity. They +// fail independently: a file ffmpeg can demux may still defeat fpcalc. +type fingerprintResult struct { + streamSHA256 []byte + chromaprint []int32 + hashErr error + printErr error +} + +// computeFingerprint derives both halves for the file at path. +func computeFingerprint(ctx context.Context, path string) fingerprintResult { + var r fingerprintResult + r.streamSHA256, r.hashErr = computeAudioStreamSHA256(ctx, path) + r.chromaprint, r.printErr = computeChromaprint(ctx, path, defaultChromaprintLengthSec) + return r +} + +// inconclusive reports whether either half failed for a reason that says +// nothing about the file. Such a result must never be stored: stamped at the +// current version it would read as "tried, and this file cannot be +// fingerprinted", and the backfill would never try it again. +func (r fingerprintResult) inconclusive() bool { + return isInconclusive(r.hashErr) || isInconclusive(r.printErr) +} + +// 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. +func isInconclusive(err error) bool { + return errors.Is(err, errFingerprintTimeout) || + errors.Is(err, context.Canceled) || + errors.Is(err, context.DeadlineExceeded) || + errors.Is(err, exec.ErrNotFound) +} + +// 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 { + if s.fingerprint == nil { + return computeFingerprint(ctx, path) + } + return s.fingerprint(ctx, path) +} + +// storeFingerprint records one attempt for a track whose bytes are new or have +// changed. It never fails the scan: a missing fingerprint only keeps a track +// out of duplicate detection, which is not worth dropping the track over. +func (s *Scanner) storeFingerprint( + ctx context.Context, q *dbq.Queries, trackID pgtype.UUID, path string, fp fingerprintResult, +) { + if fp.hashErr != nil { + s.logger.Warn("library scan: audio stream hash failed", "path", path, "err", fp.hashErr) + } + if fp.printErr != nil { + s.logger.Warn("library scan: chromaprint failed", "path", path, "err", fp.printErr) + } + if fp.inconclusive() { + // Any row this track holds describes its PREVIOUS bytes. Drop it and + // leave the track to the backfill, rather than stamping a failure that + // says nothing about this file. + if err := q.DeleteTrackFingerprint(ctx, trackID); err != nil { + s.logger.Warn("library scan: clearing stale fingerprint failed", "path", path, "err", err) + } + return + } + // A NULL half here is a verdict — the tool ran and rejected this file — and + // is stamped at the current version so the backfill does not retry it on + // every boot. It is retried when the file changes. + if err := q.UpsertTrackFingerprint(ctx, dbq.UpsertTrackFingerprintParams{ + TrackID: trackID, + AudioStreamSha256: fp.streamSHA256, + Chromaprint: fp.chromaprint, + FingerprintVersion: fingerprintVersion, + }); err != nil { + s.logger.Warn("library scan: storing fingerprint failed", "path", path, "err", err) + } +} + +// computeAudioStreamSHA256 returns the SHA-256 of the file's encoded audio. +func computeAudioStreamSHA256(ctx context.Context, path string) ([]byte, error) { + out, err := runFingerprintTool(ctx, "ffmpeg", streamHashArgs(path)) + if err != nil { + return nil, err + } + return parseStreamHash(out) +} + +// 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)) + if err != nil { + return nil, err + } + return parseFpcalcRaw(out) +} + +// runFingerprintTool runs one tool under fingerprintTimeout. +// +// Any non-zero exit is an error, and that deliberately includes fpcalc's exit 3: +// "reading failed, but here is a fingerprint of what I got". A partial +// fingerprint of a damaged file is not that file's identity. Stored, it would +// score against a healthy copy over whatever prefix survived, and could group +// or fail to group either way. Absent is better than wrong. +func runFingerprintTool(ctx context.Context, name string, args []string) ([]byte, error) { + runCtx, cancel := context.WithTimeout(ctx, fingerprintTimeout) + defer cancel() + + cmd := exec.CommandContext(runCtx, name, args...) + cmd.WaitDelay = fingerprintWaitDelay + out, err := cmd.Output() + if err == nil { + return out, nil + } + // The caller gave up (a cancelled scan). Report that rather than the + // signal-killed exit it caused, so it is never mistaken for a verdict on + // the file. + if ctx.Err() != nil { + return nil, fmt.Errorf("%s: %w", name, ctx.Err()) + } + // Named separately so a stall reads as a stall, not as a crash. + if errors.Is(runCtx.Err(), context.DeadlineExceeded) { + return nil, fmt.Errorf("%s: no result within %s: %w", name, fingerprintTimeout, errFingerprintTimeout) + } + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + return nil, fmt.Errorf("%s exited %d: %s", name, exitErr.ExitCode(), stderrTail(exitErr.Stderr)) + } + return nil, fmt.Errorf("%s: %w", name, err) +} + +// stderrTail keeps the END of a failing tool's stderr. ffmpeg and fpcalc print +// the actual reason last, after any banner or per-frame warnings, so a cap that +// kept the head would log the noise and drop the cause. +func stderrTail(stderr []byte) []byte { + stderr = bytes.TrimSpace(stderr) + if len(stderr) > fpcalcStderrTail { + stderr = stderr[len(stderr)-fpcalcStderrTail:] + } + return stderr +} + +// parseStreamHash reads the ffmpeg hash muxer's "SHA256=" line. +func parseStreamHash(out []byte) ([]byte, error) { + for _, line := range strings.Split(string(out), "\n") { + hexed, ok := strings.CutPrefix(strings.TrimSpace(line), "SHA256=") + if !ok { + continue + } + sum, err := hex.DecodeString(hexed) + if err != nil { + return nil, fmt.Errorf("stream hash %q: %w", hexed, err) + } + if len(sum) != sha256.Size { + return nil, fmt.Errorf("stream hash is %d bytes, want %d", len(sum), sha256.Size) + } + return sum, nil + } + return nil, errors.New("ffmpeg printed no SHA256= line") +} + +// parseFpcalcRaw reads fpcalc's text output: +// +// DURATION= +// FINGERPRINT=,,... +func parseFpcalcRaw(out []byte) ([]int32, error) { + for _, line := range strings.Split(string(out), "\n") { + list, ok := strings.CutPrefix(strings.TrimSpace(line), "FINGERPRINT=") + if !ok { + continue + } + if list == "" { + return nil, errors.New("fpcalc returned an empty fingerprint") + } + items := strings.Split(list, ",") + fp := make([]int32, len(items)) + for i, item := range items { + // ParseInt at 32 bits, not ParseUint: a value past int32 means the + // output was unsigned — -signed went missing from the invocation — + // and nothing downstream would reinterpret it. Refuse it here. + v, err := strconv.ParseInt(item, 10, 32) + if err != nil { + return nil, fmt.Errorf("fingerprint item %d %q: %w", i, item, err) + } + fp[i] = int32(v) + } + return fp, nil + } + return nil, errors.New("fpcalc printed no FINGERPRINT= line") +} diff --git a/internal/library/fingerprint_scan_test.go b/internal/library/fingerprint_scan_test.go new file mode 100644 index 00000000..b2edb800 --- /dev/null +++ b/internal/library/fingerprint_scan_test.go @@ -0,0 +1,174 @@ +package library + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "log/slog" + "os" + "path/filepath" + "slices" + "testing" + "time" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db" +) + +// TestScanner_FingerprintsOnlyNewOrChangedBytes_Integration pins WHEN the scan +// fingerprints. The cost of getting it wrong is asymmetric and invisible: a +// scan that re-fingerprints unchanged files still produces correct rows, just +// by decoding the entire library on every tag-repair pass. +// +// The fingerprinter is stubbed. CI has no real audio, and the tools' output is +// covered by the parser tests; this covers the scan's decisions. +func TestScanner_FingerprintsOnlyNewOrChangedBytes_Integration(t *testing.T) { + if testing.Short() { + t.Skip("skipping scanner integration in -short mode") + } + dsn := os.Getenv("MINSTREL_TEST_DATABASE_URL") + if dsn == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + ctx := context.Background() + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + + if err := db.Migrate(dsn, logger); err != nil { + t.Fatalf("migrate: %v", err) + } + pool, err := pgxpool.New(ctx, dsn) + if err != nil { + t.Fatalf("pool: %v", err) + } + t.Cleanup(pool.Close) + if _, err := pool.Exec(ctx, "TRUNCATE tracks, albums, artists RESTART IDENTITY CASCADE"); err != nil { + t.Fatalf("truncate: %v", err) + } + + root := t.TempDir() + a := filepath.Join(root, "artist/album/01.mp3") + b := filepath.Join(root, "artist/album/02.mp3") + writeTestMP3(t, a, map[string]string{"TIT2": "One", "TPE1": "Artist", "TALB": "Album", "TRCK": "1"}) + writeTestMP3(t, b, map[string]string{"TIT2": "Two", "TPE1": "Artist", "TALB": "Album", "TRCK": "2"}) + + sum := bytes.Repeat([]byte{0xAB}, 32) + chroma := []int32{7, -7, 2147483647} + result := fingerprintResult{streamSHA256: sum, chromaprint: chroma} + calls := map[string]int{} + + scanner := New(pool, logger, []string{root}) + scanner.fingerprint = func(_ context.Context, path string) fingerprintResult { + calls[path]++ + return result + } + + scan := func(step string) Stats { + t.Helper() + st, err := scanner.Scan(ctx, nil) + if err != nil { + t.Fatalf("%s: scan: %v", step, err) + } + return st + } + type row struct { + sha []byte + chroma []int32 + version int16 + } + 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 + 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) + if errors.Is(err, pgx.ErrNoRows) { + return row{}, false + } + if err != nil { + t.Fatalf("read fingerprint for %s: %v", path, err) + } + return r, true + } + // A later step moves mtime forward past the row's updated_at, which is + // what the scan reads as "these bytes changed". + touch := func(path string, ahead time.Duration) { + t.Helper() + when := time.Now().Add(ahead) + if err := os.Chtimes(path, when, when); err != nil { + t.Fatalf("chtimes %s: %v", path, err) + } + } + + // 1. New files are fingerprinted, and stored at the current version. + scan("first scan") + if calls[a] != 1 || calls[b] != 1 { + t.Fatalf("first scan fingerprint calls = %v, want one per file", calls) + } + got, ok := stored(a) + 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) + } + + // 2. A tag-repair pass re-reads every unchanged file and must not + // fingerprint any of them again. + // + // The Updated count is what makes this able to fail. Without it, a scan + // that simply SKIPPED both files would also leave the call counts at one, + // and the assertion would pass without the re-read path ever running. + if _, err := pool.Exec(ctx, "UPDATE tracks SET duration_ms = 1000, tag_read_version = 0"); err != nil { + t.Fatalf("force tag re-read: %v", err) + } + if st := scan("tag-repair scan"); st.Updated != 2 || st.Skipped != 0 { + t.Fatalf("tag-repair scan stats = %+v, want both files re-read (Updated=2 Skipped=0)", st) + } + if calls[a] != 1 || calls[b] != 1 { + t.Fatalf("tag-repair scan re-fingerprinted unchanged files: calls = %v", calls) + } + if _, ok := stored(a); !ok { + t.Fatal("tag-repair scan dropped a stored fingerprint") + } + + // 3. Bytes that changed are fingerprinted again, and only those. + touch(a, time.Hour) + scan("changed-file scan") + if calls[a] != 2 || calls[b] != 1 { + t.Fatalf("changed-file scan calls = %v, want a=2 b=1", calls) + } + + // 4. A changed file whose attempt is inconclusive loses its old row: that + // row describes the previous bytes, and a stall says nothing about the new + // ones. + result = fingerprintResult{streamSHA256: sum, printErr: fmt.Errorf("fpcalc: %w", errFingerprintTimeout)} + touch(a, 2*time.Hour) + scan("inconclusive scan") + if _, ok := stored(a); ok { + t.Fatal("inconclusive attempt left the previous bytes' fingerprint in place") + } + if _, ok := stored(b); !ok { + t.Fatal("inconclusive attempt on one file removed another file's fingerprint") + } + + // 5. A file the tools reject gets a row at the current version with both + // halves NULL — a verdict, so the backfill does not retry it every boot. + result = fingerprintResult{ + hashErr: errors.New("ffmpeg exited 1"), + printErr: errors.New("fpcalc exited 2"), + } + touch(a, 3*time.Hour) + scan("rejected scan") + got, ok = stored(a) + if !ok { + t.Fatal("a file the tools rejected got no row, so the backfill would retry it forever") + } + 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) + } +} diff --git a/internal/library/fingerprint_test.go b/internal/library/fingerprint_test.go new file mode 100644 index 00000000..e4b5cee1 --- /dev/null +++ b/internal/library/fingerprint_test.go @@ -0,0 +1,194 @@ +package library + +import ( + "bytes" + "context" + "errors" + "fmt" + "os/exec" + "slices" + "strings" + "testing" +) + +func TestParseFpcalcRaw(t *testing.T) { + cases := []struct { + name string + out string + want []int32 + wantErr string + }{ + { + name: "signed output with negatives", + out: "DURATION=213\nFINGERPRINT=-1453821711,17,0,2147483647,-2147483648\n", + want: []int32{-1453821711, 17, 0, 2147483647, -2147483648}, + }, + { + name: "fingerprint line need not come second", + out: "FINGERPRINT=5,6\nDURATION=1\n", + want: []int32{5, 6}, + }, + { + // fpcalc's default is uint32. This value only appears when -signed + // is missing, and storing it would need a reinterpretation nothing + // performs. + name: "unsigned output is refused", + out: "DURATION=213\nFINGERPRINT=2841145585,17\n", + wantErr: "item 0", + }, + {name: "empty fingerprint", out: "DURATION=0\nFINGERPRINT=\n", wantErr: "empty fingerprint"}, + {name: "no fingerprint line", out: "DURATION=213\n", wantErr: "no FINGERPRINT= line"}, + {name: "non-numeric item", out: "FINGERPRINT=1,x,3\n", wantErr: "item 1"}, + {name: "trailing comma", out: "FINGERPRINT=1,2,\n", wantErr: "item 2"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := parseFpcalcRaw([]byte(tc.out)) + if tc.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("err = %v, want one containing %q", err, tc.wantErr) + } + return + } + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if !slices.Equal(got, tc.want) { + t.Fatalf("got %v, want %v", got, tc.want) + } + }) + } +} + +func TestParseStreamHash(t *testing.T) { + // The real value ffmpeg printed for both files of the #3885 pair. + const www = "24e2daa3b4a534ff1a8d1a76f67810205869daf89f728d83a16625da4d28a18e" + + got, err := parseStreamHash([]byte("SHA256=" + www + "\n")) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if len(got) != 32 || got[0] != 0x24 || got[31] != 0x8e { + t.Fatalf("decoded %x, want %s", got, www) + } + + for name, out := range map[string]string{ + "no hash line": "", + "other hash": "MD5=" + www[:32] + "\n", + "not hex": "SHA256=" + strings.Repeat("zz", 32) + "\n", + "short digest": "SHA256=" + www[:62] + "\n", + "odd hex chars": "SHA256=" + www[:63] + "\n", + } { + if _, err := parseStreamHash([]byte(out)); err == nil { + t.Errorf("%s: parsed %q without error", name, out) + } + } +} + +// followedBy reports whether flag appears in args immediately followed by value. +func followedBy(args []string, flag, value string) bool { + for i := 0; i+1 < len(args); i++ { + if args[i] == flag && args[i+1] == value { + return true + } + } + return false +} + +// The exact tier's stored hashes must stay comparable across ffmpeg upgrades, +// which only holds while the packets are copied rather than decoded. A decoded +// hash still matches within one ffmpeg build, so nothing else would notice the +// change until an image upgrade silently broke every stored value. +func TestStreamHashArgs_HashPacketsNotSamples(t *testing.T) { + args := streamHashArgs("/music/a.mp3") + for _, pair := range [][2]string{ + {"-c:a", "copy"}, // no decode + {"-map", "0:a"}, // audio only: cover art stays out of the hash + {"-f", "hash"}, // the hash muxer, not a file + {"-hash", "sha256"}, + {"-i", "/music/a.mp3"}, + } { + if !followedBy(args, pair[0], pair[1]) { + t.Errorf("streamHashArgs lacks %s %s: %v", pair[0], pair[1], args) + } + } +} + +func TestFpcalcArgs_RequestSignedRawOutput(t *testing.T) { + args := fpcalcArgs("/music/a.flac", 90) + for _, flag := range []string{"-raw", "-signed"} { + if !slices.Contains(args, flag) { + t.Errorf("fpcalcArgs lacks %s: %v", flag, args) + } + } + if !followedBy(args, "-length", "90") { + t.Errorf("fpcalcArgs does not pass the requested length: %v", args) + } + // fpcalc takes the file as its trailing positional argument. + if args[len(args)-1] != "/music/a.flac" { + t.Errorf("path is not last: %v", args) + } +} + +func TestStderrTail_KeepsTheCauseNotTheBanner(t *testing.T) { + banner := bytes.Repeat([]byte("warning: skipping frame\n"), fpcalcStderrTail) + got := stderrTail(append(banner, []byte("ERROR: could not decode\n")...)) + if len(got) != fpcalcStderrTail { + t.Fatalf("tail is %d bytes, want the %d-byte cap", len(got), fpcalcStderrTail) + } + if !bytes.HasSuffix(got, []byte("ERROR: could not decode")) { + t.Fatalf("tail dropped the final line: ...%q", got[len(got)-40:]) + } + + if got := stderrTail([]byte(" short \n")); string(got) != "short" { + t.Fatalf("short stderr = %q, want it trimmed and whole", got) + } +} + +// A stored failure is permanent until the file changes, so the classification +// decides whether a track is ever retried. Every inconclusive case here would, +// if misfiled as a verdict, silently exclude that track from duplicate +// detection for good. +func TestIsInconclusive(t *testing.T) { + notInstalled := fmt.Errorf("fpcalc: %w", &exec.Error{Name: "fpcalc", Err: exec.ErrNotFound}) + cases := []struct { + name string + err error + want bool + }{ + {"timeout", fmt.Errorf("fpcalc: no result: %w", errFingerprintTimeout), true}, + {"scan cancelled", fmt.Errorf("ffmpeg: %w", context.Canceled), true}, + {"caller deadline", fmt.Errorf("ffmpeg: %w", context.DeadlineExceeded), true}, + {"tool not installed", notInstalled, true}, + {"tool rejected the file", errors.New("fpcalc exited 2: could not decode"), false}, + {"unparseable output", errors.New("fpcalc printed no FINGERPRINT= line"), false}, + {"success", nil, false}, + } + for _, tc := range cases { + if got := isInconclusive(tc.err); got != tc.want { + t.Errorf("%s: isInconclusive = %v, want %v", tc.name, got, tc.want) + } + } +} + +// Either half being inconclusive taints the whole result: storing the half that +// succeeded would stamp the row at the current version with the other half +// NULL, and that NULL would then read as a verdict. +func TestFingerprintResult_InconclusiveIfEitherHalfIs(t *testing.T) { + stall := fmt.Errorf("fpcalc: %w", errFingerprintTimeout) + rejected := errors.New("fpcalc exited 2") + for name, tc := range map[string]struct { + r fingerprintResult + want bool + }{ + "both succeeded": {fingerprintResult{streamSHA256: []byte{1}, chromaprint: []int32{1}}, false}, + "hash ok, print stalled": {fingerprintResult{streamSHA256: []byte{1}, printErr: stall}, true}, + "hash stalled, print ok": {fingerprintResult{hashErr: stall, chromaprint: []int32{1}}, true}, + "hash ok, print rejected": {fingerprintResult{streamSHA256: []byte{1}, printErr: rejected}, false}, + "both rejected by the file": {fingerprintResult{hashErr: rejected, printErr: rejected}, false}, + } { + if got := tc.r.inconclusive(); got != tc.want { + t.Errorf("%s: inconclusive = %v, want %v", name, got, tc.want) + } + } +} diff --git a/internal/library/scanner.go b/internal/library/scanner.go index 634c45a4..c23dbd94 100644 --- a/internal/library/scanner.go +++ b/internal/library/scanner.go @@ -75,10 +75,15 @@ type Scanner struct { pool *pgxpool.Pool logger *slog.Logger paths []string + // fingerprint derives a file's acoustic identity (M400). A field so an + // 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 } func New(pool *pgxpool.Pool, logger *slog.Logger, paths []string) *Scanner { - return &Scanner{pool: pool, logger: logger, paths: paths} + return &Scanner{pool: pool, logger: logger, paths: paths, fingerprint: computeFingerprint} } // Scan walks every configured root and upserts any audio file whose mtime is @@ -295,6 +300,25 @@ func (s *Scanner) scanFile( durationMs = probed } + // Fingerprint only bytes this row has not seen: a new path, or a file whose + // mtime moved past the row's. An unchanged file re-read for a tag repair + // keeps its stored fingerprint, for the same reason it keeps its duration + // above — a tagReadVersion bump must stay bound by tag reads, not become a + // decode of the whole library. + // + // An unchanged file with NO fingerprint yet is the backfill's job (#3908), + // deliberately not the scan's. Folding it into the skip check would make + // the first scan after an upgrade re-decode every track and push a sync + // change to every client for each one. + // + // Computed before move adoption so adoption can match on the audio hash + // (#3914); stored after the upsert, once the row id is known. + var fp fingerprintResult + fingerprinted := !unchanged + if fingerprinted { + fp = s.fingerprintFile(ctx, path) + } + // A path we've never seen might not be a new track — it might be one that // moved or was renamed (#2528). Adopting re-points the existing row at this // path and clears its missing mark, so the UpsertTrack below conflicts on @@ -359,6 +383,9 @@ func (s *Scanner) scanFile( // touches this track will re-emit the change. s.logger.Warn("library scan: LogChange track upsert failed", "track_id", track.ID, "err", err) } + if fingerprinted { + s.storeFingerprint(ctx, q, track.ID, path, fp) + } if knownTrack { stats.Updated++