diff --git a/internal/db/dbq/library_changes.sql.go b/internal/db/dbq/library_changes.sql.go index de326379..0eeda6c4 100644 --- a/internal/db/dbq/library_changes.sql.go +++ b/internal/db/dbq/library_changes.sql.go @@ -102,3 +102,26 @@ func (q *Queries) InsertLibraryChange(ctx context.Context, arg InsertLibraryChan _, err := q.db.Exec(ctx, insertLibraryChange, arg.EntityType, arg.EntityID, arg.Op) return err } + +const insertLibraryChanges = `-- name: InsertLibraryChanges :exec +INSERT INTO library_changes (entity_type, entity_id, op) +SELECT $1::text, + unnest($2::text[]), + $3::text +` + +type InsertLibraryChangesParams struct { + EntityType string + EntityIds []string + Op string +} + +// Batch form, for a mutation that touches many rows at once (#2704: the scan's +// reconcile pass can mark up to a quarter of a library missing in one go). +// Every caller before this one changed a single entity, so per-row inserts +// were the right shape; a loop here would be thousands of round-trips inside +// a scan that is already the slow path. +func (q *Queries) InsertLibraryChanges(ctx context.Context, arg InsertLibraryChangesParams) error { + _, err := q.db.Exec(ctx, insertLibraryChanges, arg.EntityType, arg.EntityIds, arg.Op) + return err +} diff --git a/internal/db/queries/library_changes.sql b/internal/db/queries/library_changes.sql index 463e48e0..86a17848 100644 --- a/internal/db/queries/library_changes.sql +++ b/internal/db/queries/library_changes.sql @@ -2,6 +2,17 @@ INSERT INTO library_changes (entity_type, entity_id, op) VALUES ($1, $2, $3); +-- name: InsertLibraryChanges :exec +-- Batch form, for a mutation that touches many rows at once (#2704: the scan's +-- reconcile pass can mark up to a quarter of a library missing in one go). +-- Every caller before this one changed a single entity, so per-row inserts +-- were the right shape; a loop here would be thousands of round-trips inside +-- a scan that is already the slow path. +INSERT INTO library_changes (entity_type, entity_id, op) +SELECT sqlc.arg(entity_type)::text, + unnest(sqlc.arg(entity_ids)::text[]), + sqlc.arg(op)::text; + -- name: GetLibraryChangesSince :many SELECT id, entity_type, entity_id, op, changed_at FROM library_changes diff --git a/internal/library/reconcile.go b/internal/library/reconcile.go index 6c406cf2..534b42e8 100644 --- a/internal/library/reconcile.go +++ b/internal/library/reconcile.go @@ -9,6 +9,7 @@ import ( "github.com/jackc/pgx/v5/pgtype" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync" ) // trackReconciler is the slice of dbq.Queries reconcileMissing needs. Narrowed @@ -18,6 +19,7 @@ type trackReconciler interface { ListTrackPathsForReconcile(ctx context.Context) ([]dbq.ListTrackPathsForReconcileRow, error) MarkTracksMissing(ctx context.Context, ids []pgtype.UUID) (int64, error) ClearTracksMissing(ctx context.Context, ids []pgtype.UUID) (int64, error) + InsertLibraryChanges(ctx context.Context, arg dbq.InsertLibraryChangesParams) error } // Reconcile marks tracks whose files have disappeared (#2523). @@ -91,6 +93,9 @@ func (s *Scanner) reconcileMissing( // otherwise a library that tripped the cap once could never recover its // marks even after the mount came back. if len(toClear) > 0 { + if err := logTrackChanges(ctx, q, toClear); err != nil { + return fmt.Errorf("log restored changes: %w", err) + } n, err := q.ClearTracksMissing(ctx, toClear) if err != nil { return fmt.Errorf("clear missing marks: %w", err) @@ -110,6 +115,9 @@ func (s *Scanner) reconcileMissing( ) } + if err := logTrackChanges(ctx, q, toMark); err != nil { + return fmt.Errorf("log missing changes: %w", err) + } n, err := q.MarkTracksMissing(ctx, toMark) if err != nil { return fmt.Errorf("mark tracks missing: %w", err) @@ -123,6 +131,41 @@ func (s *Scanner) reconcileMissing( return nil } +// logTrackChanges tells the delta sync that these tracks changed, so clients +// pick up the missing mark (#2704). +// +// Without this the mark was invisible to every client: MarkTracksMissing and +// ClearTracksMissing are plain UPDATEs, and /api/library/sync is a change-log +// feed — a row that never produces a change is never re-sent, so a client +// would keep its stale copy until an unrelated edit touched the track or the +// cursor fell out of the retention window. +// +// Logged BEFORE the mutation, deliberately, which is the opposite of the +// scanner's log-after-success pattern. The two failure modes are not +// symmetric: log-then-fail-to-mark makes clients re-fetch a track that has +// not changed, which costs one wasted read. Mark-then-fail-to-log leaves the +// mark in place with no change row, and because both statements are +// idempotent (`missing_since IS NULL` / `IS NOT NULL` guards) the next scan +// will not retry the pair — so the client never learns, permanently. A +// spurious re-read is the cheaper mistake. +func logTrackChanges(ctx context.Context, q trackReconciler, ids []pgtype.UUID) error { + if len(ids) == 0 { + return nil + } + strIDs := make([]string, 0, len(ids)) + for _, id := range ids { + strIDs = append(strIDs, syncpkg.FormatUUID(id)) + } + // OpUpsert, not OpDelete: the track still exists and keeps its history — + // only its playability changed. A delete would tell clients to drop the + // row, which is the behaviour #2704 deliberately rejected. + return q.InsertLibraryChanges(ctx, dbq.InsertLibraryChangesParams{ + EntityType: string(syncpkg.EntityTrack), + EntityIds: strIDs, + Op: string(syncpkg.OpUpsert), + }) +} + // verifyRootsPresent is the first and most important guard. If a configured root // doesn't resolve to a readable directory, the walk beneath it found nothing and // every row under it would look deleted. An unmounted media volume is the diff --git a/internal/library/reconcile_test.go b/internal/library/reconcile_test.go index a5248f73..55cb8e45 100644 --- a/internal/library/reconcile_test.go +++ b/internal/library/reconcile_test.go @@ -26,6 +26,8 @@ type fakeReconciler struct { listErr error markErr error clearErr error + // Change rows the reconcile pass asked the delta sync to emit (#2704). + loggedChanges []dbq.InsertLibraryChangesParams } func (f *fakeReconciler) ListTrackPathsForReconcile(context.Context) ([]dbq.ListTrackPathsForReconcileRow, error) { @@ -40,6 +42,13 @@ func (f *fakeReconciler) MarkTracksMissing(_ context.Context, ids []pgtype.UUID) return int64(len(ids)), nil } +func (f *fakeReconciler) InsertLibraryChanges( + _ context.Context, arg dbq.InsertLibraryChangesParams, +) error { + f.loggedChanges = append(f.loggedChanges, arg) + return nil +} + func (f *fakeReconciler) ClearTracksMissing(_ context.Context, ids []pgtype.UUID) (int64, error) { if f.clearErr != nil { return 0, f.clearErr @@ -324,3 +333,68 @@ func TestVerifyRootsPresent_NoRootsConfigured(t *testing.T) { t.Fatal("expected an error with no scan roots configured") } } + +// Without a change row the mark is invisible to every client: +// /api/library/sync is a change-log feed, so a plain UPDATE never reaches +// anyone. This was the gap that made #2704's wire field inert — the flag +// existed and nothing ever told a client to re-read the track. +func TestReconcileMissing_MarkingEmitsSyncChanges(t *testing.T) { + s := testScanner(t, populatedRoot(t)) + + // 10 rows, 2 absent — under the cap, so this exercises marking. + rows := make([]dbq.ListTrackPathsForReconcileRow, 0, 10) + seen := map[string]struct{}{} + for i := 0; i < 10; i++ { + path := fmt.Sprintf("/music/track-%02d.mp3", i) + rows = append(rows, row(byte(i), path, false)) + if i >= 2 { + seen[path] = struct{}{} + } + } + q := &fakeReconciler{rows: rows} + var stats Stats + + if err := s.reconcileMissing(context.Background(), q, seen, &stats); err != nil { + t.Fatalf("reconcile: %v", err) + } + + if len(q.loggedChanges) != 1 { + t.Fatalf("want one batch of change rows, got %d", len(q.loggedChanges)) + } + got := q.loggedChanges[0] + if got.EntityType != "track" { + t.Errorf("entity_type = %q, want track", got.EntityType) + } + // Upsert, not delete: the track still exists and keeps its history. A + // delete would tell clients to drop the row, which is the behaviour + // #2704 deliberately rejected. + if got.Op != "upsert" { + t.Errorf("op = %q, want upsert — the row survives, only its state changed", got.Op) + } + if len(got.EntityIds) != 2 { + t.Errorf("want both missing tracks logged, got %d ids", len(got.EntityIds)) + } +} + +// A file coming back must reach clients too, or a restored track stays +// greyed out on every device until something unrelated touches it. +func TestReconcileMissing_RestoringEmitsSyncChanges(t *testing.T) { + s := testScanner(t, populatedRoot(t)) + q := &fakeReconciler{rows: []dbq.ListTrackPathsForReconcileRow{ + row(1, "/music/back.mp3", true), + }} + var stats Stats + + if err := s.reconcileMissing( + context.Background(), q, map[string]struct{}{"/music/back.mp3": {}}, &stats, + ); err != nil { + t.Fatalf("reconcile: %v", err) + } + + if len(q.loggedChanges) != 1 { + t.Fatalf("a restored file must produce a change row, got %d batches", len(q.loggedChanges)) + } + if q.loggedChanges[0].Op != "upsert" { + t.Errorf("op = %q, want upsert", q.loggedChanges[0].Op) + } +} diff --git a/internal/sync/changes.go b/internal/sync/changes.go index 1734e611..a14b617d 100644 --- a/internal/sync/changes.go +++ b/internal/sync/changes.go @@ -36,6 +36,29 @@ func LogChange(ctx context.Context, dbtx dbq.DBTX, entityType EntityType, entity return nil } +// LogChanges is LogChange for a set of entities of one type sharing one op. +// +// Exists because the scan's reconcile pass can mark a quarter of a library +// missing in a single sweep (#2704), and a per-row loop there would be +// thousands of round-trips inside an operation that is already slow. Every +// other caller mutates one entity and should keep using LogChange. +// +// A no-op on an empty set, so callers don't have to guard. +func LogChanges(ctx context.Context, dbtx dbq.DBTX, entityType EntityType, entityIDs []string, op Op) error { + if len(entityIDs) == 0 { + return nil + } + q := dbq.New(dbtx) + if err := q.InsertLibraryChanges(ctx, dbq.InsertLibraryChangesParams{ + EntityType: string(entityType), + EntityIds: entityIDs, + Op: string(op), + }); err != nil { + return fmt.Errorf("sync.LogChanges(%s, %d ids, %s): %w", entityType, len(entityIDs), op, err) + } + return nil +} + // FormatUUID renders a pgtype.UUID as the canonical 8-4-4-4-12 hex form. // Returns "" if the UUID is not valid. func FormatUUID(u pgtype.UUID) string {