fix: the archive write path loses tracks and destroys albums when it fails
- Two tracks sharing a track number and a title were both given the same target name, and the copy truncates, so one silently replaced the other. normalize.DetectCollisions groups by the title half of a name and folds repeats of a title together, so it never saw the pair, and verify compares each written file with the plan entry in the same position, so it passed. The plan now reports it and execute.Run refuses before the first copy. - Collision "both" filed the album at "<album> (2)" but the summary, the library built by the same run and the journal record all kept using the path the plan asked for. promote now returns where the album landed. - "replace" removed the destination before renaming the new album in, so a failed rename, a permission problem part-way through RemoveAll or a power cut left nothing behind. The old album is moved aside first and moved back if the new one cannot land. - "merge" checked for conflicts as it moved files, leaving the destination holding half of one album and half of another on a clash. Every name is checked before anything moves. - A staged file that could not be deleted after a successful promotion made Run fail, and FilePlan then forgot an album that is complete on disk. - cleanStaging walked up to four empty ancestor directories, so filing the last album could delete the staging directory itself. It now stops at the staging root and clears the album folder the scan named rather than the disc folder the first track sat in, so a CD1/CD2 rip is fully cleared. - A failed run left the empty artist folder it had created in the archive. - Run panicked on a plan with no tracks. Closes 5c83509
diff --git a/internal/execute/execute.go b/internal/execute/execute.go
@@ -21,7 +21,10 @@ type Options struct {
Collision string
CoverPath string
CoverName string
- Logf func(format string, args ...any)
+ // StagingRoot is the staging directory the album came from. Staging cleanup
+ // never removes it, however empty it becomes.
+ StagingRoot string
+ Logf func(format string, args ...any)
}
type Result struct {
@@ -30,6 +33,9 @@ type Result struct {
RemovedStaging bool
Bytes int64
StagingLeftovers []string
+ // StagingStuck names staged files that could not be removed. The album is
+ // archived either way.
+ StagingStuck []string
}
func Run(ctx context.Context, p plan.Plan, opts Options) (Result, error) {
@@ -55,9 +61,33 @@ func Run(ctx context.Context, p plan.Plan, opts Options) (Result, error) {
}
parent := filepath.Dir(p.ArchiveDir)
+ createdParent := false
+ if _, err := os.Stat(parent); os.IsNotExist(err) {
+ createdParent = true
+ }
if err := os.MkdirAll(parent, 0o755); err != nil {
return res, err
}
+ promoted := false
+ defer func() {
+ // An artist folder is only created so the album can go inside it. When
+ // the album never gets there -- a truncated download, a decode that
+ // fails verification -- the empty folder is left behind, and an empty
+ // artist directory in the archive is indistinguishable from one whose
+ // albums were lost. os.Remove refuses to touch a non-empty directory, so
+ // this can only ever undo the creation above.
+ if createdParent && !promoted {
+ os.Remove(parent)
+ }
+ }()
+
+ // Last line of defence against writing one track over another. The plan
+ // checks this too, but the plan is a value that any caller can assemble, and
+ // the cost of being wrong here is a quietly missing track rather than a
+ // refusal, so it is worth proving again immediately before the first copy.
+ if err := checkTargets(p); err != nil {
+ return res, err
+ }
tmp, err := os.MkdirTemp(parent, "."+filepath.Base(p.ArchiveDir)+".intake-")
if err != nil {
@@ -99,27 +129,44 @@ func Run(ctx context.Context, p plan.Plan, opts Options) (Result, error) {
return res, err
}
- if err := promote(p.ArchiveDir, tmp, opts.Collision, logf); err != nil {
+ finalDir, err := promote(p.ArchiveDir, tmp, opts.Collision, logf)
+ if err != nil {
return res, err
}
+ promoted = true
+ // "both" lands the album somewhere other than the path that was asked for,
+ // and everything downstream -- the summary, the journal, the library this
+ // same run builds -- has to follow the album to where it actually went.
+ res.ArchiveDir = finalDir
finalPaths := make([]string, 0, len(written))
for _, f := range written {
rel, err := filepath.Rel(tmp, f)
if err != nil {
rel = filepath.Base(f)
}
- finalPaths = append(finalPaths, filepath.Join(p.ArchiveDir, rel))
+ finalPaths = append(finalPaths, filepath.Join(finalDir, rel))
}
res.Files = finalPaths
if !opts.KeepStaging {
+ // The album is archived from here on. Failing to tidy up after it is a
+ // nuisance, not a failure: returning an error would make the caller
+ // treat a complete archive as broken, drop its journal record, and offer
+ // to file it all over again.
for _, t := range p.Tracks {
if err := os.Remove(t.Source); err != nil && !os.IsNotExist(err) {
- return res, fmt.Errorf("intake succeeded but could not remove staged file %s: %w", filepath.Base(t.Source), err)
+ res.StagingStuck = append(res.StagingStuck, filepath.Base(t.Source))
}
}
- albumDir := filepath.Dir(p.Tracks[0].Source)
- leftovers, err := cleanStaging(albumDir)
+ // The album folder to clear is the one the scan called the album, not the
+ // folder the first track happened to sit in: a CD1 / CD2 rip puts the
+ // tracks one level below the album, and clearing only CD1 would leave CD2
+ // and the album's own sidecars sitting in staging for ever.
+ albumDir := p.Album.Dir
+ if albumDir == "" {
+ albumDir = filepath.Dir(p.Tracks[0].Source)
+ }
+ leftovers, err := cleanStaging(albumDir, opts.StagingRoot)
if err != nil {
logf("note: could not fully clear %s: %v", albumDir, err)
}
@@ -127,7 +174,11 @@ func Run(ctx context.Context, p plan.Plan, opts Options) (Result, error) {
logf("removed non-audio leftovers from staging: %s", strings.Join(leftovers, ", "))
}
res.StagingLeftovers = leftovers
- res.RemovedStaging = true
+ res.RemovedStaging = len(res.StagingStuck) == 0
+ if len(res.StagingStuck) > 0 {
+ logf("note: the album is archived, but %d staged file(s) could not be removed: %s",
+ len(res.StagingStuck), strings.Join(res.StagingStuck, ", "))
+ }
}
for _, t := range res.Files {
if fi, err := os.Stat(t); err == nil {
@@ -137,6 +188,26 @@ func Run(ctx context.Context, p plan.Plan, opts Options) (Result, error) {
return res, nil
}
+// checkTargets refuses a plan that would write two tracks to the same name.
+func checkTargets(p plan.Plan) error {
+ if len(p.Tracks) == 0 {
+ return fmt.Errorf("plan has no tracks")
+ }
+ seen := map[string]string{}
+ for _, t := range p.Tracks {
+ name := filepath.Base(t.Target)
+ if name == "" || name == "." || name == string(filepath.Separator) {
+ return fmt.Errorf("plan has a track with no target name (%s)", filepath.Base(t.Source))
+ }
+ if prev, ok := seen[name]; ok {
+ return fmt.Errorf("refusing to file: %s and %s would both be written to %q, which would overwrite one of them",
+ prev, filepath.Base(t.Source), name)
+ }
+ seen[name] = filepath.Base(t.Source)
+ }
+ return nil
+}
+
func coverName(opts Options) string {
if opts.CoverName != "" {
return opts.CoverName
@@ -217,13 +288,36 @@ func verify(ctx context.Context, p plan.Plan, written []string, mode string, opt
return nil
}
-func promote(target, tmp, collision string, logf func(string, ...any)) error {
+// promote moves a fully built and verified album directory into place and
+// returns the directory it actually landed in.
+//
+// The return value matters: "both" deliberately puts the album somewhere other
+// than the path the plan named, and every later step has to agree about where
+// that is.
+func promote(target, tmp, collision string, logf func(string, ...any)) (string, error) {
if _, err := os.Stat(target); err == nil {
switch collision {
case "replace":
- if err := os.RemoveAll(target); err != nil {
- return err
+ // Move the old album aside and only delete it once the new one is in
+ // place. Removing it first leaves a window -- a failed rename, a
+ // permission problem partway through RemoveAll, a power cut -- in
+ // which the album is simply gone, with nothing to restore.
+ stale := target + ".intake-replaced"
+ if err := os.Rename(target, stale); err != nil {
+ return "", fmt.Errorf("moving the existing album aside: %w", err)
+ }
+ if err := os.Rename(tmp, target); err != nil {
+ // Put the old album back rather than leaving neither.
+ if back := os.Rename(stale, target); back != nil {
+ return "", fmt.Errorf("promote %s: %w (and the previous album is now at %s)", target, err, stale)
+ }
+ return "", fmt.Errorf("promote %s: %w", target, err)
+ }
+ if err := os.RemoveAll(stale); err != nil {
+ logf("note: the replaced album is still at %s: %v", filepath.Base(stale), err)
}
+ logf("placed %s (replacing what was there)", target)
+ return target, nil
case "both":
alt := target + " (2)"
for i := 3; ; i++ {
@@ -236,32 +330,41 @@ func promote(target, tmp, collision string, logf func(string, ...any)) error {
target = alt
case "merge":
if err := mergeInto(tmp, target); err != nil {
- return err
+ return "", err
}
logf("merged into existing %s", filepath.Base(target))
- return nil
+ return target, nil
default:
- return fmt.Errorf("destination already exists: %s", target)
+ return "", fmt.Errorf("destination already exists: %s", target)
}
}
if err := os.Rename(tmp, target); err != nil {
- return fmt.Errorf("promote %s: %w", target, err)
+ return "", fmt.Errorf("promote %s: %w", target, err)
}
logf("placed %s", target)
- return nil
+ return target, nil
}
+// mergeInto moves the contents of src into dst, refusing to overwrite anything.
+//
+// Every possible conflict is found before the first file moves. Checking as it
+// goes left the destination holding half of one album and half of another when
+// the second file clashed, which is worse than either album on its own and
+// cannot be undone by re-running.
func mergeInto(src, dst string) error {
entries, err := os.ReadDir(src)
if err != nil {
return err
}
for _, e := range entries {
- from := filepath.Join(src, e.Name())
to := filepath.Join(dst, e.Name())
if _, err := os.Stat(to); err == nil {
- return fmt.Errorf("merge conflict: %s already exists", to)
+ return fmt.Errorf("merge conflict: %s already exists, so nothing was merged", to)
}
+ }
+ for _, e := range entries {
+ from := filepath.Join(src, e.Name())
+ to := filepath.Join(dst, e.Name())
if err := os.Rename(from, to); err != nil {
return err
}
@@ -286,7 +389,15 @@ func copyFile(src, dst string) error {
return out.Sync()
}
-func cleanStaging(dir string) ([]string, error) {
+// cleanStaging clears a filed album out of staging: its leftover files, its
+// now-empty subdirectories (a CD1 / CD2 rip leaves one empty disc folder behind
+// for every disc but the last), and then the album folder itself.
+//
+// It climbs towards stagingRoot, never past or through it. An empty staging
+// directory is the normal state after the last album is filed, not clutter to be
+// tidied away, and deleting it makes the next scan fail and the next run
+// recreate it. With no stagingRoot it stops at the album folder.
+func cleanStaging(dir, stagingRoot string) ([]string, error) {
entries, err := os.ReadDir(dir)
if err != nil {
if os.IsNotExist(err) {
@@ -305,15 +416,57 @@ func cleanStaging(dir string) ([]string, error) {
}
removed = append(removed, e.Name())
}
- for i := 0; i < 4 && dir != "" && dir != "/" && dir != "."; i++ {
- left, err := os.ReadDir(dir)
+ for cur := dir; withinStaging(cur, stagingRoot); cur = filepath.Dir(cur) {
+ pruneEmptySubdirs(cur)
+ left, err := os.ReadDir(cur)
if err != nil || len(left) > 0 {
break
}
- if err := os.Remove(dir); err != nil {
+ if err := os.Remove(cur); err != nil {
break
}
- dir = filepath.Dir(dir)
}
return removed, nil
}
+
+// withinStaging reports whether dir is inside stagingRoot rather than equal to
+// it or above it. An empty stagingRoot means "do not climb at all".
+func withinStaging(dir, stagingRoot string) bool {
+ if stagingRoot == "" || dir == "" || dir == "/" || dir == "." {
+ return false
+ }
+ root, err := filepath.Abs(stagingRoot)
+ if err != nil {
+ return false
+ }
+ abs, err := filepath.Abs(dir)
+ if err != nil {
+ return false
+ }
+ rel, err := filepath.Rel(root, abs)
+ if err != nil || rel == "." || rel == ".." {
+ return false
+ }
+ return !strings.HasPrefix(rel, ".."+string(filepath.Separator))
+}
+
+// pruneEmptySubdirs removes directories under dir that hold nothing, deepest
+// first, so that a folder like CD2 or Scans does not keep an otherwise finished
+// album sitting in staging forever. A directory holding anything at all is left
+// exactly as it is.
+func pruneEmptySubdirs(dir string) {
+ entries, err := os.ReadDir(dir)
+ if err != nil {
+ return
+ }
+ for _, e := range entries {
+ if !e.IsDir() {
+ continue
+ }
+ sub := filepath.Join(dir, e.Name())
+ pruneEmptySubdirs(sub)
+ if left, err := os.ReadDir(sub); err == nil && len(left) == 0 {
+ os.Remove(sub)
+ }
+ }
+}
diff --git a/internal/execute/execute_test.go b/internal/execute/execute_test.go
@@ -0,0 +1,400 @@
+package execute
+
+import (
+ "context"
+ "os"
+ "os/exec"
+ "path/filepath"
+ "strings"
+ "testing"
+
+ "intake/internal/plan"
+ "intake/internal/scan"
+ "intake/internal/tags"
+)
+
+// Every test here needs a real flac: execute copies it, tags it with metaflac,
+// verifies it with ffprobe and flac --test, and the whole point is that the file
+// that comes out is checked the same way a real one is. The tones are a tenth of
+// a second long, so the cost is milliseconds.
+type fixture struct {
+ root string
+ staging string
+ album string
+}
+
+func newFixture(t *testing.T) *fixture {
+ t.Helper()
+ for _, tool := range []string{"ffmpeg", "ffprobe", "metaflac", "flac"} {
+ if _, err := exec.LookPath(tool); err != nil {
+ t.Skipf("%s is needed to build a fixture", tool)
+ }
+ }
+ root := t.TempDir()
+ f := &fixture{
+ root: root,
+ staging: filepath.Join(root, "staging"),
+ album: filepath.Join(root, "staging", "Artist - Album"),
+ }
+ for _, d := range []string{f.album, filepath.Join(root, "archive"), filepath.Join(root, "library")} {
+ if err := os.MkdirAll(d, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ }
+ return f
+}
+
+// tone writes a flac and gives it just enough tags that a filed copy verifies.
+func (f *fixture) tone(t *testing.T, name, title string, freq int) string {
+ t.Helper()
+ path := filepath.Join(f.album, name)
+ cmd := exec.Command("ffmpeg", "-hide_banner", "-loglevel", "error", "-y",
+ "-f", "lavfi", "-i", sine(freq), path)
+ if out, err := cmd.CombinedOutput(); err != nil {
+ t.Fatalf("building %s: %v %s", name, err, out)
+ }
+ f.tag(t, path, title)
+ return path
+}
+
+func sine(freq int) string {
+ return "sine=frequency=" + itoa(freq) + ":duration=0.1"
+}
+
+func itoa(n int) string {
+ if n == 0 {
+ return "0"
+ }
+ var b []byte
+ for n > 0 {
+ b = append([]byte{byte('0' + n%10)}, b...)
+ n /= 10
+ }
+ return string(b)
+}
+
+func (f *fixture) tag(t *testing.T, path, title string) {
+ t.Helper()
+ lines := "TITLE=" + title + "\nALBUM=Album\nARTIST=Artist\nALBUMARTIST=Artist\nTRACKNUMBER=1\n"
+ cmd := exec.Command("metaflac", "--remove-all-tags", "--import-tags-from=-", path)
+ cmd.Stdin = strings.NewReader(lines)
+ if out, err := cmd.CombinedOutput(); err != nil {
+ t.Fatalf("tagging %s: %v %s", filepath.Base(path), err, out)
+ }
+}
+
+func (f *fixture) plan(tracks ...plan.TrackPlan) plan.Plan {
+ return plan.Plan{
+ Album: scan.Album{Dir: f.album},
+ AlbumName: "Album",
+ AlbumArt: "Artist",
+ ArtistDir: "artist",
+ AlbumDir: "album",
+ ArchiveRoot: filepath.Join(f.root, "archive"),
+ LibraryRoot: filepath.Join(f.root, "library"),
+ ArchiveDir: filepath.Join(f.root, "archive", "artist", "album"),
+ LibraryDir: filepath.Join(f.root, "library", "artist", "album"),
+ Tracks: tracks,
+ }
+}
+
+func trackPlan(src, target string) plan.TrackPlan {
+ return plan.TrackPlan{
+ Source: src,
+ Target: target,
+ Ext: ".flac",
+ Tags: []tags.Note{
+ {Field: tags.FieldTitle, Value: strings.TrimSuffix(target, ".flac")},
+ {Field: tags.FieldAlbumArtist, Value: "Artist"},
+ },
+ }
+}
+
+func (f *fixture) options() Options {
+ return Options{VerifyDecode: "sample", StagingRoot: f.staging}
+}
+
+// Two tracks that share a number and a title produce the same target. copyFile
+// truncates, so without a guard the second write replaces the first, the album
+// is filed a track short, verify passes -- it compares each written file with the
+// plan entry in the same position -- and nothing anywhere reports a problem.
+func TestRunRefusesTwoTracksWithTheSameTarget(t *testing.T) {
+ f := newFixture(t)
+ a := f.tone(t, "01 intro.flac", "Intro", 440)
+ b := f.tone(t, "01 intro (1).flac", "Intro", 660)
+ p := f.plan(trackPlan(a, "1-01_intro.flac"), trackPlan(b, "1-01_intro.flac"))
+
+ res, err := Run(context.Background(), p, f.options())
+ if err == nil {
+ t.Fatal("running a plan with two identical targets should be refused")
+ }
+ if !strings.Contains(err.Error(), "1-01_intro.flac") {
+ t.Errorf("the refusal should name the target: %v", err)
+ }
+ if len(res.Files) != 0 {
+ t.Errorf("nothing should have been written, got %v", res.Files)
+ }
+ if _, err := os.Stat(p.ArchiveDir); !os.IsNotExist(err) {
+ t.Error("the archive directory was created before the plan was refused")
+ }
+ for _, src := range []string{a, b} {
+ if _, err := os.Stat(src); err != nil {
+ t.Errorf("staged file %s was touched: %v", filepath.Base(src), err)
+ }
+ }
+}
+
+func TestRunRefusesAnEmptyPlan(t *testing.T) {
+ f := newFixture(t)
+ if _, err := Run(context.Background(), f.plan(), f.options()); err == nil {
+ t.Fatal("a plan with no tracks should be refused rather than panic")
+ }
+}
+
+// "both" deliberately puts the album somewhere other than the path that was
+// asked for. Everything downstream has to follow it there: the reported archive
+// directory, the file list, the byte count, and the directory a caller goes on to
+// read the masters from.
+func TestBothCollisionReportsWhereTheAlbumActuallyWent(t *testing.T) {
+ f := newFixture(t)
+ existing := f.plan()
+ if err := os.MkdirAll(existing.ArchiveDir, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ old := filepath.Join(existing.ArchiveDir, "1-01_old.flac")
+ if err := os.WriteFile(old, []byte("the album that was already there"), 0o644); err != nil {
+ t.Fatal(err)
+ }
+
+ src := f.tone(t, "01 new.flac", "New", 880)
+ p := f.plan(trackPlan(src, "1-01_new.flac"))
+
+ opts := f.options()
+ opts.Collision = "both"
+ res, err := Run(context.Background(), p, opts)
+ if err != nil {
+ t.Fatal(err)
+ }
+ want := p.ArchiveDir + " (2)"
+ if res.ArchiveDir != want {
+ t.Errorf("reported archive dir = %q, want %q", res.ArchiveDir, want)
+ }
+ if len(res.Files) != 1 || filepath.Dir(res.Files[0]) != want {
+ t.Errorf("reported files = %v, want one file inside %q", res.Files, want)
+ }
+ if fi, err := os.Stat(filepath.Join(want, "1-01_new.flac")); err != nil {
+ t.Fatalf("the new album did not land where it was reported: %v", err)
+ } else if res.Bytes != fi.Size() {
+ t.Errorf("reported %d bytes, the new album is %d", res.Bytes, fi.Size())
+ }
+ if _, err := os.Stat(old); err != nil {
+ t.Errorf("the album that was already there was disturbed: %v", err)
+ }
+ if _, err := os.Stat(filepath.Join(p.ArchiveDir, "1-01_new.flac")); !os.IsNotExist(err) {
+ t.Error("the new album was written over the old one")
+ }
+}
+
+// Replacing an album used to remove the old one first and rename the new one in
+// afterwards. A failure in between -- and a failed RemoveAll leaves the same
+// state -- meant the album was simply gone. The old copy now survives until the
+// new one is in place.
+func TestReplaceKeepsTheOldAlbumUntilTheNewOneIsInPlace(t *testing.T) {
+ dir := t.TempDir()
+ target := filepath.Join(dir, "album")
+ if err := os.MkdirAll(target, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.WriteFile(filepath.Join(target, "keep.flac"), []byte("irreplaceable"), 0o644); err != nil {
+ t.Fatal(err)
+ }
+
+ // A source that cannot be renamed into place.
+ missing := filepath.Join(dir, "gone")
+ if _, err := promote(target, missing, "replace", func(string, ...any) {}); err == nil {
+ t.Fatal("promoting a directory that does not exist should fail")
+ }
+ got, err := os.ReadFile(filepath.Join(target, "keep.flac"))
+ if err != nil {
+ t.Fatalf("the old album was destroyed by a failed replace: %v", err)
+ }
+ if string(got) != "irreplaceable" {
+ t.Errorf("the old album came back as %q", got)
+ }
+}
+
+// A merge is all-or-nothing. Checking for conflicts as the files move left the
+// destination holding part of one album and part of another, which is worse than
+// either album on its own and cannot be put right by running again.
+func TestMergeMovesNothingWhenAnythingClashes(t *testing.T) {
+ dir := t.TempDir()
+ dst := filepath.Join(dir, "dst")
+ src := filepath.Join(dir, "src")
+ for _, d := range []string{dst, src} {
+ if err := os.MkdirAll(d, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ }
+ for _, f := range []string{"01_new.flac", "02_old.flac"} {
+ if err := os.WriteFile(filepath.Join(src, f), []byte("incoming"), 0o644); err != nil {
+ t.Fatal(err)
+ }
+ }
+ if err := os.WriteFile(filepath.Join(dst, "02_old.flac"), []byte("already here"), 0o644); err != nil {
+ t.Fatal(err)
+ }
+
+ if err := mergeInto(src, dst); err == nil {
+ t.Fatal("a clash should stop the merge")
+ }
+ entries, err := os.ReadDir(dst)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(entries) != 1 || entries[0].Name() != "02_old.flac" {
+ var names []string
+ for _, e := range entries {
+ names = append(names, e.Name())
+ }
+ t.Errorf("the destination gained files from a merge that failed: %v", names)
+ }
+ left, err := os.ReadDir(src)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(left) != 2 {
+ t.Errorf("the incoming album was consumed by a merge that failed: %d file(s) left", len(left))
+ }
+}
+
+// Staging cleanup used to walk up to four empty directories, which reached past
+// the album, past staging, and out into the library root. An empty staging
+// directory is the normal state after the last album is filed, and deleting it
+// makes the next scan fail.
+func TestStagingCleanupNeverClimbsPastStaging(t *testing.T) {
+ root := t.TempDir()
+ staging := filepath.Join(root, "staging")
+ album := filepath.Join(staging, "album")
+ if err := os.MkdirAll(album, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ if _, err := cleanStaging(album, staging); err != nil {
+ t.Fatal(err)
+ }
+ for _, keep := range []string{root, staging} {
+ if _, err := os.Stat(keep); err != nil {
+ t.Errorf("%s was removed by staging cleanup: %v", filepath.Base(keep), err)
+ }
+ }
+ if _, err := os.Stat(album); !os.IsNotExist(err) {
+ t.Error("the empty album folder should have been removed")
+ }
+
+ // With no staging root it must not climb at all.
+ album2 := filepath.Join(staging, "album2")
+ if err := os.MkdirAll(album2, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ if _, err := cleanStaging(album2, ""); err != nil {
+ t.Fatal(err)
+ }
+ if _, err := os.Stat(staging); err != nil {
+ t.Errorf("cleanup climbed out of the album with no staging root: %v", err)
+ }
+}
+
+// A CD1 / CD2 rip has the album one level up from the tracks. Clearing only the
+// disc folder left the empty second disc, the album's own sidecars and the album
+// folder itself in staging, so the finished album stayed in the list for ever.
+func TestStagingCleanupClearsWholeMultiDiscAlbum(t *testing.T) {
+ root := t.TempDir()
+ staging := filepath.Join(root, "staging")
+ album := filepath.Join(staging, "Artist - Album")
+ for _, d := range []string{"CD1", "CD2"} {
+ if err := os.MkdirAll(filepath.Join(album, d), 0o755); err != nil {
+ t.Fatal(err)
+ }
+ }
+ if err := os.WriteFile(filepath.Join(album, "album.cue"), []byte("PERFORMER\n"), 0o644); err != nil {
+ t.Fatal(err)
+ }
+ removed, err := cleanStaging(album, staging)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(removed) != 1 || removed[0] != "album.cue" {
+ t.Errorf("leftovers reported as %v, want [album.cue]", removed)
+ }
+ if _, err := os.Stat(album); !os.IsNotExist(err) {
+ t.Error("the album folder was left in staging")
+ }
+ if _, err := os.Stat(staging); err != nil {
+ t.Errorf("staging itself was removed: %v", err)
+ }
+}
+
+// Once the album is promoted it is archived, and failing to delete the staged
+// originals afterwards does not change that. Reporting it as a failure made the
+// caller drop the journal record for an album that is sitting on disk, so verify
+// and audit could no longer see it and a later run would file it again.
+func TestStagedFilesThatCannotBeRemovedDoNotFailTheRun(t *testing.T) {
+ f := newFixture(t)
+ src := f.tone(t, "01 one.flac", "One", 440)
+ p := f.plan(trackPlan(src, "1-01_one.flac"))
+
+ // Read-only album folder: the copy reads from it, the removal from it fails.
+ if err := os.Chmod(f.album, 0o555); err != nil {
+ t.Fatal(err)
+ }
+ t.Cleanup(func() { os.Chmod(f.album, 0o755) })
+
+ res, err := Run(context.Background(), p, f.options())
+ if err != nil {
+ t.Fatalf("a complete archive was reported as a failure: %v", err)
+ }
+ if _, err := os.Stat(filepath.Join(p.ArchiveDir, "1-01_one.flac")); err != nil {
+ t.Fatalf("the album was not archived: %v", err)
+ }
+ if len(res.StagingStuck) != 1 {
+ t.Errorf("StagingStuck = %v, want the one file that could not be removed", res.StagingStuck)
+ }
+ if res.RemovedStaging {
+ t.Error("the run claimed staging was cleared when it was not")
+ }
+}
+
+// An artist folder is created so the album can go inside it. A run that fails --
+// a truncated download, a decode that will not verify -- used to leave the empty
+// folder behind, where it reads as an artist whose albums have gone missing.
+func TestFailedRunLeavesNoEmptyArtistFolder(t *testing.T) {
+ f := newFixture(t)
+ p := f.plan(trackPlan(filepath.Join(f.album, "not-here.flac"), "1-01_one.flac"))
+
+ if _, err := Run(context.Background(), p, f.options()); err == nil {
+ t.Fatal("a plan whose source file is missing should fail")
+ }
+ if _, err := os.Stat(filepath.Dir(p.ArchiveDir)); !os.IsNotExist(err) {
+ t.Errorf("an empty artist folder was left in the archive: %v", err)
+ }
+}
+
+// A plan that is refused before anything is copied must not disturb an artist
+// folder that was already there with albums in it.
+func TestFailedRunKeepsAnArtistFolderThatWasAlreadyThere(t *testing.T) {
+ f := newFixture(t)
+ artistDir := filepath.Dir(f.plan().ArchiveDir)
+ if err := os.MkdirAll(filepath.Join(artistDir, "older-album"), 0o755); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.WriteFile(filepath.Join(artistDir, "older-album", "track.flac"), []byte("x"), 0o644); err != nil {
+ t.Fatal(err)
+ }
+
+ p := f.plan(trackPlan(filepath.Join(f.album, "not-here.flac"), "1-01_one.flac"))
+ if _, err := Run(context.Background(), p, f.options()); err == nil {
+ t.Fatal("a plan whose source file is missing should fail")
+ }
+ if _, err := os.Stat(filepath.Join(artistDir, "older-album", "track.flac")); err != nil {
+ t.Errorf("an existing artist folder was disturbed: %v", err)
+ }
+}
diff --git a/internal/normalize/normalize.go b/internal/normalize/normalize.go
@@ -2,6 +2,7 @@ package normalize
import (
"fmt"
+ "path/filepath"
"strings"
"unicode"
@@ -156,6 +157,33 @@ func DetectCollisions(items [][2]string) []Collision {
return out
}
+// TargetClashes reports tracks that would be written to exactly the same file.
+//
+// DetectCollisions cannot see this one. It groups by the title half of a target
+// and folds repeats of the same title together, so two tracks that share both a
+// title and a track number pass it unnoticed even though their whole target is
+// the same string. That pair is not merely untidy: the second write replaces the
+// first, so the album is filed a track short and every later check still passes.
+func TargetClashes(items [][2]string) []Collision {
+ byName := map[string][]string{}
+ var order []string
+ for _, it := range items {
+ name := filepath.Base(it[1])
+ if _, ok := byName[name]; !ok {
+ order = append(order, name)
+ }
+ byName[name] = append(byName[name], it[0])
+ }
+ var out []Collision
+ for _, name := range order {
+ if len(byName[name]) < 2 {
+ continue
+ }
+ out = append(out, Collision{Target: name, Causes: byName[name]})
+ }
+ return out
+}
+
func Truncate(name string, max int) (string, bool) {
if len(name) <= max {
return name, false
diff --git a/internal/plan/plan.go b/internal/plan/plan.go
@@ -300,15 +300,13 @@ func Build(a scan.Album, archiveRoot, libraryRoot string, artistKey DirKey, disc
p.Warnings = append(p.Warnings, fmt.Sprintf("disc_prefix is %q but this album has %d discs, so tracks on different discs will share numbers", discPrefix, discTotal))
}
- var pairs [][2]string
+ var pairs, bySource [][2]string
for _, it := range items {
target := normalize.TrackFile(it.disc, it.track, width, it.title, filepath.Ext(it.src), withDisc)
pairs = append(pairs, [2]string{it.orig, target})
+ bySource = append(bySource, [2]string{filepath.Base(it.src), target})
}
- for _, c := range normalize.DetectCollisions(pairs) {
- p.Issues = append(p.Issues, Issue{Hard: true, Text: fmt.Sprintf("two different titles normalise to the same name %q (from %s); rename one before continuing",
- c.Target, strings.Join(quoteAll(c.Causes), " and "))})
- }
+ noteCollisions(&p, pairs, bySource)
for _, it := range items {
name := normalize.TrackFile(it.disc, it.track, width, it.title, filepath.Ext(it.src), withDisc)
@@ -439,14 +437,6 @@ func (p Plan) EditSession() names.Session {
}
}
-func (p Plan) nameSession() names.Session {
- s := p.EditSession()
- if s.ArtistDir == "" && p.AlbumArt != "" {
- s.ArtistDir = normalize.DirName(p.AlbumArt)
- }
- return s
-}
-
func (p *Plan) ApplyNames(res names.Result) {
byPath := map[string]names.Item{}
for _, it := range res.Tracks {
@@ -515,14 +505,45 @@ func (p *Plan) ApplyNames(res names.Result) {
}
p.rebuildPaths()
p.reassessIssues(res.AlbumUserEdited, res.ArtistUserEdited)
- var pairs [][2]string
- for _, t := range p.Tracks {
- pairs = append(pairs, [2]string{t.Title, t.Target})
+ noteCollisions(p, titlePairs(p.Tracks), sourcePairs(p.Tracks))
+}
+
+// titlePairs and sourcePairs are the two views noteCollisions needs: what the
+// tracks are called, and which files they came from.
+func titlePairs(tracks []TrackPlan) [][2]string {
+ out := make([][2]string, 0, len(tracks))
+ for _, t := range tracks {
+ out = append(out, [2]string{t.Title, t.Target})
+ }
+ return out
+}
+
+func sourcePairs(tracks []TrackPlan) [][2]string {
+ out := make([][2]string, 0, len(tracks))
+ for _, t := range tracks {
+ out = append(out, [2]string{filepath.Base(t.Source), t.Target})
}
- for _, c := range normalize.DetectCollisions(pairs) {
+ return out
+}
+
+// noteCollisions records the names in a plan that cannot coexist.
+//
+// The first kind is two different titles collapsing to the same normalised text,
+// which is worth stopping for because the title is the only human-readable part
+// of a file name. The second is two tracks whose whole target is the same string,
+// which the first check cannot see when they also share a track number, because
+// it groups by title and folds repeats of a title together. That one is not a
+// naming nuisance but a lost track: the copy truncates and replaces, so the album
+// is filed one track short and every later check still passes.
+func noteCollisions(p *Plan, titles, sources [][2]string) {
+ for _, c := range normalize.DetectCollisions(titles) {
p.Issues = append(p.Issues, Issue{Hard: true, Text: fmt.Sprintf("two different titles normalise to the same name %q (from %s); rename one before continuing",
c.Target, strings.Join(quoteAll(c.Causes), " and "))})
}
+ for _, c := range normalize.TargetClashes(sources) {
+ p.Issues = append(p.Issues, Issue{Hard: true, Text: fmt.Sprintf("%s would be written twice, from %s; give the tracks different numbers or titles, in their tags or in the staged file names",
+ c.Target, strings.Join(quoteAll(c.Causes), " and "))})
+ }
}
func (p *Plan) reassessIssues(albumEdited, artistEdited bool) {
@@ -539,6 +560,7 @@ func (p *Plan) reassessIssues(albumEdited, artistEdited bool) {
p.Issues = removeIssues(p.Issues, `album artist "`)
}
p.Issues = removeIssues(p.Issues, "normalise to the same name")
+ p.Issues = removeIssues(p.Issues, "would be written twice")
}
func BuildWithArtist(a scan.Album, archiveRoot, libraryRoot string, key DirKey, discPrefix string, edited *Plan, session *names.Session) Plan {
@@ -608,14 +630,7 @@ func BuildWithArtist(a scan.Album, archiveRoot, libraryRoot string, key DirKey,
}
p.rebuildPaths()
p.reassessIssues(true, true)
- var pairs [][2]string
- for _, t := range p.Tracks {
- pairs = append(pairs, [2]string{t.Title, t.Target})
- }
- for _, c := range normalize.DetectCollisions(pairs) {
- p.Issues = append(p.Issues, Issue{Hard: true, Text: fmt.Sprintf("two different titles normalise to the same name %q (from %s); rename one before continuing",
- c.Target, strings.Join(quoteAll(c.Causes), " and "))})
- }
+ noteCollisions(&p, titlePairs(p.Tracks), sourcePairs(p.Tracks))
return p
}
diff --git a/internal/plan/plan_test.go b/internal/plan/plan_test.go
@@ -426,3 +426,59 @@ func TestDisputedAlbumValueFallsBackToTheMajority(t *testing.T) {
t.Error("a one-all split has no majority and should be refused")
}
}
+
+// Two tracks that share both a title and a track number produce the same target
+// name, and the copy truncates, so the second silently replaces the first. The
+// check that was supposed to catch this groups by title and folds repeats of the
+// same title together, so it never saw this pair at all.
+func TestDuplicateNumbersWithTheSameTitleAreBlocked(t *testing.T) {
+ p := planFor([]audio.Track{
+ track("01 intro.flac", 1, 1, "Intro"),
+ track("01 intro (1).flac", 1, 1, "Intro"),
+ })
+ if !p.Blocked() {
+ t.Fatalf("both tracks target %q and the plan is not blocked", p.Tracks[0].Target)
+ }
+ if !strings.Contains(firstHard(p), "would be written twice") {
+ t.Errorf("the issue should say the name is written twice: %s", firstHard(p))
+ }
+ if !strings.Contains(firstHard(p), "01 intro.flac") ||
+ !strings.Contains(firstHard(p), "01 intro (1).flac") {
+ t.Errorf("the issue should name both staged files: %s", firstHard(p))
+ }
+}
+
+// The same pair on different discs is fine with the default disc prefix, because
+// the targets differ.
+func TestDuplicateNumbersOnDifferentDiscsAreNotBlocked(t *testing.T) {
+ p := planFor([]audio.Track{
+ track("01 intro.flac", 1, 1, "Intro"),
+ track("01 intro.flac", 1, 2, "Intro"),
+ })
+ if p.Blocked() {
+ t.Fatalf("disc 1 and disc 2 targets differ, but the plan is blocked: %s", firstHard(p))
+ }
+ if p.Tracks[0].Target == p.Tracks[1].Target {
+ t.Fatalf("targets should differ: %q", p.Tracks[0].Target)
+ }
+}
+
+// Without a disc prefix they really do collide, and that has to be caught.
+func TestDuplicateNumbersWithoutDiscPrefixAreBlocked(t *testing.T) {
+ p := planForMode([]audio.Track{
+ track("01 intro.flac", 1, 1, "Intro"),
+ track("01 intro.flac", 1, 2, "Intro"),
+ }, "never")
+ if !p.Blocked() {
+ t.Fatalf("disc_prefix=never makes both tracks %q and the plan is not blocked", p.Tracks[0].Target)
+ }
+}
+
+func firstHard(p Plan) string {
+ for _, i := range p.Issues {
+ if i.Hard {
+ return i.Text
+ }
+ }
+ return ""
+}