fix(library): preserve playlist phantoms on incremental scan and removal
The incremental scan's orphan cleanup and RemoveFromLibrary deleted audio_files rows without first filling the playlist phantom columns, so a track removed from the library folder outside YellowJacket (or removed from the library) became a permanently empty playlist row that nothing could re-link — the same bug #183 fixed on the full-rescan and retire paths, on the two paths it missed. Add a scoped PreservePlaylistPhantomsForFiles and run it in the same transaction as the deletes on both paths. Closes #246
This commit is contained in:
+92
-30
@@ -911,30 +911,96 @@ func (l *Library) scanInternal(
|
||||
|
||||
orphanStart := time.Now()
|
||||
|
||||
// Snapshot the orphan set first. The playlist-phantom
|
||||
// preservation and the deletes are one transaction (the
|
||||
// preservation has to land before the ON DELETE SET NULL, and
|
||||
// both have to succeed or neither does), and the ids are what
|
||||
// scope that preservation to just these files instead of
|
||||
// rewriting every playlist row on a routine scan.
|
||||
var (
|
||||
orphans []sqlcgen.AudioFile
|
||||
orphanPaths []string
|
||||
)
|
||||
|
||||
existingPaths.Range(func(key, value any) bool {
|
||||
path := key.(string)
|
||||
audioFile := value.(sqlcgen.AudioFile)
|
||||
orphanPaths = append(orphanPaths, key.(string))
|
||||
orphans = append(orphans, value.(sqlcgen.AudioFile))
|
||||
|
||||
l.logger.Debug(
|
||||
"removing orphaned database entry",
|
||||
"path", path, "id", audioFile.ID,
|
||||
)
|
||||
return true
|
||||
})
|
||||
|
||||
if err := l.db.Queries.DeleteAudioFile(
|
||||
l.ctx, audioFile.ID,
|
||||
); err != nil {
|
||||
l.logger.Warn(
|
||||
"failed to delete orphaned audio file",
|
||||
"path", path,
|
||||
"id", audioFile.ID,
|
||||
"err", err,
|
||||
)
|
||||
deleted := make([]bool, len(orphans))
|
||||
|
||||
metrics.addWarning(path, "orphan", err)
|
||||
|
||||
return true
|
||||
if len(orphans) > 0 {
|
||||
orphanIDs := make([]int64, len(orphans))
|
||||
for i, f := range orphans {
|
||||
orphanIDs[i] = f.ID
|
||||
}
|
||||
|
||||
tx, beginErr := l.db.BeginTx()
|
||||
if beginErr != nil {
|
||||
metrics.addWarning("", "orphan", beginErr)
|
||||
} else {
|
||||
defer func() { _ = tx.Rollback() }() // no-op after commit
|
||||
|
||||
if err := database.PreservePlaylistPhantomsForFiles(
|
||||
l.ctx, tx, orphanIDs, l.logger,
|
||||
); err != nil {
|
||||
// Deleting without the phantoms is exactly the
|
||||
// playlist-emptying bug the preservation exists to
|
||||
// prevent, so leave the rows for the next scan
|
||||
// rather than empty the playlists now.
|
||||
l.logger.Error(
|
||||
"skipping orphan deletion: could not preserve "+
|
||||
"playlist entries",
|
||||
"err", err,
|
||||
)
|
||||
|
||||
metrics.addWarning("", "orphan", err)
|
||||
|
||||
_ = tx.Rollback()
|
||||
} else {
|
||||
txq := l.db.Queries.WithTx(tx)
|
||||
|
||||
for i, f := range orphans {
|
||||
if err := txq.DeleteAudioFile(
|
||||
l.ctx, f.ID,
|
||||
); err != nil {
|
||||
l.logger.Warn(
|
||||
"failed to delete orphaned audio file",
|
||||
"path", orphanPaths[i],
|
||||
"id", f.ID,
|
||||
"err", err,
|
||||
)
|
||||
|
||||
metrics.addWarning(orphanPaths[i], "orphan", err)
|
||||
|
||||
continue
|
||||
}
|
||||
|
||||
deleted[i] = true
|
||||
}
|
||||
|
||||
if err := tx.Commit(); err != nil {
|
||||
l.logger.Error(
|
||||
"could not commit orphan deletion",
|
||||
"err", err,
|
||||
)
|
||||
|
||||
metrics.addWarning("", "orphan", err)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Post-commit bookkeeping for the files that actually went.
|
||||
for i, f := range orphans {
|
||||
if !deleted[i] {
|
||||
continue
|
||||
}
|
||||
|
||||
path := orphanPaths[i]
|
||||
|
||||
// Keep the file's tagging group in sync: drop the group's
|
||||
// track count and clear it out once empty, mirroring the
|
||||
// bookkeeping maybeRebindTaggingGroup does for a group_key
|
||||
@@ -942,25 +1008,25 @@ func (l *Library) scanInternal(
|
||||
// and replaced leaves a stale tagging_items row behind —
|
||||
// its track_count still counts the deleted files, and it
|
||||
// never clears from the autotag queue.
|
||||
if audioFile.GroupKey != "" {
|
||||
if f.GroupKey != "" {
|
||||
if err := l.db.Queries.DecrementTaggingItemTrackCount(
|
||||
l.ctx, audioFile.GroupKey,
|
||||
l.ctx, f.GroupKey,
|
||||
); err != nil {
|
||||
l.logger.Warn(
|
||||
"failed to decrement tagging group for orphan",
|
||||
"path", path,
|
||||
"group_key", audioFile.GroupKey,
|
||||
"group_key", f.GroupKey,
|
||||
"err", err,
|
||||
)
|
||||
|
||||
metrics.addWarning(path, "orphan", err)
|
||||
} else if err := l.db.Queries.DeleteTaggingItemIfEmpty(
|
||||
l.ctx, audioFile.GroupKey,
|
||||
l.ctx, f.GroupKey,
|
||||
); err != nil {
|
||||
l.logger.Warn(
|
||||
"failed to clean up emptied tagging group for orphan",
|
||||
"path", path,
|
||||
"group_key", audioFile.GroupKey,
|
||||
"group_key", f.GroupKey,
|
||||
"err", err,
|
||||
)
|
||||
|
||||
@@ -969,12 +1035,10 @@ func (l *Library) scanInternal(
|
||||
}
|
||||
|
||||
// Remove from FTS5 search index.
|
||||
if err := l.db.DeleteSearchIndex(
|
||||
audioFile.ID,
|
||||
); err != nil {
|
||||
if err := l.db.DeleteSearchIndex(f.ID); err != nil {
|
||||
l.logger.Warn(
|
||||
"failed to delete FTS entry for orphan",
|
||||
"id", audioFile.ID,
|
||||
"id", f.ID,
|
||||
"err", err,
|
||||
)
|
||||
|
||||
@@ -982,9 +1046,7 @@ func (l *Library) scanInternal(
|
||||
}
|
||||
|
||||
removed.Add(1)
|
||||
|
||||
return true
|
||||
})
|
||||
}
|
||||
|
||||
metrics.OrphanCleanup = time.Since(orphanStart)
|
||||
|
||||
|
||||
@@ -94,6 +94,57 @@ func countRows(
|
||||
return n
|
||||
}
|
||||
|
||||
// RemoveFromLibrary is the one path that empties a track *deliberately*:
|
||||
// the file stays on disk but is excluded, so nothing re-imports it. The
|
||||
// playlist entry must still survive as a re-linkable phantom rather than
|
||||
// an empty row, because a later full rescan clears the exclusion and is
|
||||
// what re-links the entry then (#246).
|
||||
func TestRemoveFromLibrary_PreservesPlaylistPhantoms(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
lib, db := setupTestLibrary(t)
|
||||
|
||||
seedRemovableLibrary(t, lib, "/nonexistent/cover.jpg")
|
||||
|
||||
if _, err := lib.db.ExecContext(
|
||||
`INSERT INTO playlists (name) VALUES ('keepme')`,
|
||||
); err != nil {
|
||||
t.Fatalf("seed playlist: %v", err)
|
||||
}
|
||||
|
||||
playlistID := queryInt(
|
||||
t, db, `SELECT id FROM playlists WHERE name = 'keepme'`,
|
||||
)
|
||||
|
||||
if _, err := lib.db.ExecContext(
|
||||
`INSERT INTO playlist_tracks (playlist_id, audio_file_id, position)
|
||||
SELECT ?, id, 0 FROM audio_files
|
||||
WHERE file_path = '/music/song.mp3'`,
|
||||
playlistID,
|
||||
); err != nil {
|
||||
t.Fatalf("seed playlist_tracks: %v", err)
|
||||
}
|
||||
|
||||
if _, err := lib.RemoveFromLibrary([]string{"/music/song.mp3"}); err != nil {
|
||||
t.Fatalf("RemoveFromLibrary: %v", err)
|
||||
}
|
||||
|
||||
phantomPath := queryString(
|
||||
t, db,
|
||||
`SELECT phantom_file_path FROM playlist_tracks
|
||||
WHERE playlist_id = ?`,
|
||||
playlistID,
|
||||
)
|
||||
|
||||
if phantomPath != "/music/song.mp3" {
|
||||
t.Fatalf(
|
||||
"phantom_file_path is %q, want %q -- the entry cannot be "+
|
||||
"re-linked after a later full rescan",
|
||||
phantomPath, "/music/song.mp3",
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
// A library with tagging_items must still be removable. tagging_items
|
||||
// FK-references libraries with no ON DELETE clause, so leaving those
|
||||
// rows behind fails the DELETE and rolls back the entire removal.
|
||||
|
||||
@@ -4,6 +4,7 @@ import (
|
||||
"errors"
|
||||
"fmt"
|
||||
|
||||
"yellowjacket/backend/database"
|
||||
"yellowjacket/backend/events"
|
||||
)
|
||||
|
||||
@@ -57,6 +58,25 @@ func (l *Library) RemoveFromLibrary(filePaths []string) (*RemovalResult, error)
|
||||
|
||||
var result RemovalResult
|
||||
|
||||
// Preserve the playlist entries before the rows go, so they survive
|
||||
// as re-linkable phantoms rather than empty rows. The track is
|
||||
// excluded and will not be re-imported on its own, but a later full
|
||||
// rescan clears the exclusion and this is what lets the entry
|
||||
// re-link then — the same preservation every other path that empties
|
||||
// audio_files performs (#246).
|
||||
rowIDs := make([]int64, len(rows))
|
||||
for i, row := range rows {
|
||||
rowIDs[i] = row.ID
|
||||
}
|
||||
|
||||
if err := database.PreservePlaylistPhantomsForFiles(
|
||||
l.ctx, tx, rowIDs, l.logger,
|
||||
); err != nil {
|
||||
return nil, fmt.Errorf(
|
||||
"could not preserve playlist entries for removal: %w", err,
|
||||
)
|
||||
}
|
||||
|
||||
// Exclude every path the caller named, including one whose row has
|
||||
// already gone: the user asked for that file to stay out, and a row
|
||||
// that disappeared between the click and the commit is not a reason
|
||||
|
||||
@@ -231,3 +231,98 @@ func TestScan_MultipleDirectoriesDoNotCrossContaminate(t *testing.T) {
|
||||
t.Errorf("Album A and Album B must not share a group_key: %+v", keys)
|
||||
}
|
||||
}
|
||||
|
||||
// TestScan_OrphanCleanupPreservesPlaylistPhantoms guards #246: a file
|
||||
// deleted from the library folder *outside* YellowJacket is discovered
|
||||
// as an orphan by the next scan, and its playlist entry must survive as
|
||||
// a re-linkable phantom — the same preservation the full rescan and
|
||||
// stale-retire paths already perform, scoped here to just the orphaned
|
||||
// file. Before the fix the entry became an empty row (audio_file_id
|
||||
// NULL and no phantom_file_path), which nothing can ever re-link.
|
||||
func TestScan_OrphanCleanupPreservesPlaylistPhantoms(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
lib, db := setupTestLibrary(t)
|
||||
|
||||
root := t.TempDir()
|
||||
track := filepath.Join(root, "gone.mp3")
|
||||
|
||||
writeTestTrack(t, track, 0)
|
||||
|
||||
library, err := db.Queries.CreateLibrary(lib.ctx, sqlcgen.CreateLibraryParams{
|
||||
Name: "orphans",
|
||||
Path: root,
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("create library: %v", err)
|
||||
}
|
||||
|
||||
if metrics := lib.scanInternal(library.ID, library.Name, library.Path); metrics == nil {
|
||||
t.Fatal("first scan returned nil metrics")
|
||||
}
|
||||
|
||||
trackID := queryInt(
|
||||
t, db, "SELECT id FROM audio_files WHERE file_path = ?", track,
|
||||
)
|
||||
if trackID == 0 {
|
||||
t.Fatal("first scan did not import the track")
|
||||
}
|
||||
|
||||
if _, err := db.ExecContext(
|
||||
`INSERT INTO playlists (name) VALUES ('keepme')`,
|
||||
); err != nil {
|
||||
t.Fatalf("seed playlist: %v", err)
|
||||
}
|
||||
|
||||
playlistID := queryInt(
|
||||
t, db, "SELECT id FROM playlists WHERE name = 'keepme'",
|
||||
)
|
||||
|
||||
if _, err := db.ExecContext(
|
||||
`INSERT INTO playlist_tracks (playlist_id, audio_file_id, position)
|
||||
VALUES (?, ?, 0)`,
|
||||
playlistID, trackID,
|
||||
); err != nil {
|
||||
t.Fatalf("seed playlist_tracks: %v", err)
|
||||
}
|
||||
|
||||
// The file goes away outside the app.
|
||||
if err := os.Remove(track); err != nil {
|
||||
t.Fatalf("remove track: %v", err)
|
||||
}
|
||||
|
||||
if metrics := lib.scanInternal(library.ID, library.Name, library.Path); metrics == nil {
|
||||
t.Fatal("second scan returned nil metrics")
|
||||
}
|
||||
|
||||
if n := queryInt(
|
||||
t, db, "SELECT COUNT(*) FROM audio_files WHERE file_path = ?", track,
|
||||
); n != 0 {
|
||||
t.Fatalf("audio_files still holds the removed path: %d rows", n)
|
||||
}
|
||||
|
||||
if n := queryInt(
|
||||
t, db,
|
||||
"SELECT COUNT(*) FROM playlist_tracks WHERE playlist_id = ? "+
|
||||
"AND audio_file_id IS NULL",
|
||||
playlistID,
|
||||
); n != 1 {
|
||||
t.Fatalf(
|
||||
"playlist entry did not become a phantom: %d null-id rows, want 1",
|
||||
n,
|
||||
)
|
||||
}
|
||||
|
||||
phantomPath := queryString(
|
||||
t, db,
|
||||
"SELECT phantom_file_path FROM playlist_tracks WHERE playlist_id = ?",
|
||||
playlistID,
|
||||
)
|
||||
if phantomPath != track {
|
||||
t.Fatalf(
|
||||
"phantom_file_path = %q, want %q -- the entry cannot be "+
|
||||
"re-linked if the file comes back",
|
||||
phantomPath, track,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user