Fix cross-folder links leaving the original next to the new link

When a match lives in another folder than the master (e.g. Game Rips/ vs
Video Games/), linking creates the new link at the master's path and then
removes the original. That removal re-checked the original's modification
time, which FUSE filesystems such as Unraid's /mnt/user can report
inconsistently; the check then failed after the link was already made,
leaving both files and a failed operation whose quarantine copy was hidden.

- Once the original is quarantined, verify it by identity (same inode as
  the quarantine copy) instead of by timestamp. Identical files without a
  quarantine copy keep the strict check, now with a detailed error.
- If the original still cannot be removed, roll back: remove the new link
  and the unneeded quarantine copy so nothing changes. If the original was
  swapped for another file mid-way, its quarantine copy is kept.
- Linking again when the master's path already holds a hardlink of the
  master only retires the leftover original, which cleans up files left
  by the old behaviour; undo then keeps that pre-existing link
  (migration 5 records this).
- Promote gets the same identity check and rollback.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

internal/engine/ops.go

@@ -105,13 +105,34 @@ func dropQuarantine(l *libs, q string, keep fsops.Info) {
 
 // removeOriginal deletes an original file after it was safely superseded. The
 // inode must still be the one we verified.
-func removeOriginal(p string, expect fsops.Info) error {
+//
+// With a quarantine copy q, identity is proven by p and q being the same
+// file right now. Timestamps are deliberately not compared: FUSE filesystems
+// such as Unraid's /mnt/user can report them inconsistently between calls,
+// and any change to the file is preserved in quarantine anyway because both
+// names point at the same data. Without a quarantine copy (a byte-identical
+// file), the size and modification time must also be unchanged.
+func removeOriginal(p string, expect fsops.Info, q string) error {
 	now, err := fsops.RegularFile(p)
 	if err != nil {
 		return err
 	}
-	if !now.SameInode(expect) || !now.Unchanged(expect.Size, expect.MtimeNs) {
+	if q != "" {
-		return fmt.Errorf("%s %w", p, fsops.ErrChanged)
+		qi, err := fsops.RegularFile(q)
+		if err != nil {
+			return fmt.Errorf("quarantine copy: %w", err)
+		}
+		if !now.SameInode(qi) {
+			return fmt.Errorf("%s is no longer the file that was quarantined: %w", p, fsops.ErrChanged)
+		}
+	} else {
+		if !now.SameInode(expect) {
+			return fmt.Errorf("%s was replaced by another file: %w", p, fsops.ErrChanged)
+		}
+		if !now.Unchanged(expect.Size, expect.MtimeNs) {
+			return fmt.Errorf("%s changed after it was compared (size %d -> %d, mtime %d -> %d): %w",
+				p, expect.Size, now.Size, expect.MtimeNs, now.MtimeNs, fsops.ErrChanged)
+		}
 	}
 	if err := os.Remove(p); err != nil {
 		return err
@@ -119,6 +140,18 @@ func removeOriginal(p string, expect fsops.Info) error {
 	return fsops.SyncDir(filepath.Dir(p))
 }
 
+// testHookBeforeRemove lets tests change the filesystem between creating the
+// new link and removing the original. Always nil outside tests.
+var testHookBeforeRemove func(slaveAbs string)
+
+// stillQuarantined reports whether the original at p is intact and still the
+// quarantined file, i.e. the quarantine copy is not needed after a rollback.
+func stillQuarantined(p, q string) bool {
+	pi, err1 := fsops.RegularFile(p)
+	qi, err2 := fsops.RegularFile(q)
+	return err1 == nil && err2 == nil && pi.SameInode(qi)
+}
+
 // pruneEmpty removes empty directories from dir upward, stopping at stop.
 func pruneEmpty(dir, stop string) {
 	for dir != stop && fsops.Within(stop, dir) {
@@ -211,14 +244,20 @@ func (e *Engine) Link(ctx context.Context, matchID int64, opt LinkOptions) (stor
 		return store.Operation{}, err
 	}
 	rename := newRel != m.RelSlave
+	// preexisting: the target already is a hardlink of the master (e.g. left
+	// behind by an earlier failed link). Then only the original is retired.
+	preexisting := false
 	if rename {
-		if _, err := os.Lstat(newAbs); err == nil {
+		if cur, err := fsops.Lstat(newAbs); err == nil {
-			return store.Operation{}, fmt.Errorf("slave already has %s: %w", newRel, fsops.ErrExists)
+			if !cur.Mode.IsRegular() || !cur.SameInode(s.mi) {
+				return store.Operation{}, fmt.Errorf("slave already has a different file at %s: %w", newRel, fsops.ErrExists)
+			}
+			preexisting = true
 		}
 	}
 
 	o := store.Operation{PairID: l.pair.ID, Action: store.ActLink, RelMaster: m.RelMaster, RelSlave: m.RelSlave,
-		RelNew: newRel, Trigger: opt.Trigger, MatchKind: m.Kind, MatchReason: m.Reason}
+		RelNew: newRel, Trigger: opt.Trigger, MatchKind: m.Kind, MatchReason: m.Reason, LinkPreexisted: preexisting}
 	if identical {
 		o.Bytes = s.si.Size // space freed
 	} else {
@@ -256,6 +295,7 @@ func (e *Engine) Link(ctx context.Context, matchID int64, opt LinkOptions) (stor
 			}
 			return nil
 		}
+		if !preexisting {
 			err := fsops.MkdirAllLike(filepath.Dir(newAbs))
 			if err == nil {
 				err = fsops.LinkNew(s.masterAbs, s.mi, newAbs)
@@ -268,8 +308,23 @@ func (e *Engine) Link(ctx context.Context, matchID int64, opt LinkOptions) (stor
 				}
 				return err
 			}
-		if err := removeOriginal(s.slaveAbs, s.si); err != nil {
+		}
-			return err
+		if testHookBeforeRemove != nil {
+			testHookBeforeRemove(s.slaveAbs)
+		}
+		if err := removeOriginal(s.slaveAbs, s.si, q); err != nil {
+			// Never leave both copies behind: undo the new link (it is only
+			// an extra name for the master file) and the quarantine copy.
+			if !preexisting {
+				if rbErr := fsops.RemoveLink(newAbs, s.mi); rbErr == nil {
+					pruneEmpty(filepath.Dir(newAbs), l.slave.Path)
+				}
+			}
+			if q != "" && stillQuarantined(s.slaveAbs, q) {
+				dropQuarantine(l, q, s.si)
+				e.clearQuarantine(ctx, o.ID)
+			}
+			return fmt.Errorf("could not remove the original slave file, so nothing was changed: %w", err)
 		}
 		pruneEmpty(filepath.Dir(s.slaveAbs), l.slave.Path) // e.g. the single's now-empty folder
 		return nil
@@ -359,7 +414,15 @@ func (e *Engine) promote(ctx context.Context, matchID int64, trigger string) (st
 			e.clearQuarantine(ctx, o.ID)
 			return err
 		}
-		return removeOriginal(s.masterAbs, s.mi)
+		if err := removeOriginal(s.masterAbs, s.mi, q); err != nil {
+			_ = fsops.RemoveLink(newMasterAbs, s.si)
+			if stillQuarantined(s.masterAbs, q) {
+				dropQuarantine(l, q, s.mi)
+				e.clearQuarantine(ctx, o.ID)
+			}
+			return fmt.Errorf("could not remove the original master file, so nothing was changed: %w", err)
+		}
+		return nil
 	}()
 	err = e.finish(ctx, &o, err)
 	promoted := m
@@ -645,7 +708,7 @@ func (e *Engine) Undo(ctx context.Context, opID int64) error {
 			}
 			dropQuarantine(l, o.QuarantinePath, qi)
 		}
-		if renamed {
+		if renamed && !o.LinkPreexisted {
 			if err := fsops.RemoveLink(newAbs, mi); err != nil {
 				return err
 			}

internal/engine/relink_test.go

@@ -0,0 +1,146 @@
+package engine
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/cdnutter/playlink/internal/fsops"
+	"github.com/cdnutter/playlink/internal/store"
+)
+
+// A cross-folder upgrade: the slave file sits in another folder than the
+// master's, so linking moves it into the master's folder structure.
+func crossFolder(t *testing.T) (ev *env, m store.Match, masterAbs, oldAbs, newAbs string) {
+	t.Helper()
+	ev = setup(t, false)
+	enableApprox(t, ev)
+	masterAbs = ev.write(ev.master, "Composer/Game1/01 Song.flac", "lossless")
+	oldAbs = ev.write(ev.slave, "Composer/Game Rips/01 Song.mp3", "lossy")
+	newAbs = filepath.Join(ev.slave, "Composer/Game1/01 Song.flac")
+	ev.scan()
+	m = ev.match("Composer/Game Rips/01 Song.mp3")
+	if m.Kind != store.KindApprox || LinkTarget(m.RelSlave, m.RelMaster, false) != "Composer/Game1/01 Song.flac" {
+		t.Fatalf("setup: unexpected match %s -> %s", m.Kind, m.RelMaster)
+	}
+	t.Cleanup(func() { testHookBeforeRemove = nil })
+	return
+}
+
+func exists(p string) bool { _, err := os.Lstat(p); return err == nil }
+
+// The reported bug: an unreliable modification time must not leave both the
+// original and the new link behind.
+func TestCrossFolderLinkIgnoresMtimeNoise(t *testing.T) {
+	ev, m, masterAbs, oldAbs, newAbs := crossFolder(t)
+	testHookBeforeRemove = func(p string) {
+		// What a FUSE filesystem may report after the new links were made.
+		_ = os.Chtimes(p, time.Now(), time.Now().Add(-time.Hour))
+	}
+	op, err := ev.e.Link(ctx, m.ID, LinkOptions{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if exists(oldAbs) {
+		t.Fatal("the original must not remain next to the new link")
+	}
+	if !sameInode(t, masterAbs, newAbs) || ev.read(op.QuarantinePath) != "lossy" {
+		t.Fatal("expected the link at the master's path and the original in quarantine")
+	}
+}
+
+// If the original really cannot be removed, the new link is rolled back and
+// the library is left exactly as it was.
+func TestCrossFolderLinkRollsBack(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("root ignores directory permissions")
+	}
+	ev, m, _, oldAbs, newAbs := crossFolder(t)
+	dir := filepath.Dir(oldAbs)
+	testHookBeforeRemove = func(string) { _ = os.Chmod(dir, 0o555) } // deletion now fails
+	t.Cleanup(func() { _ = os.Chmod(dir, 0o755) })
+	op, err := ev.e.Link(ctx, m.ID, LinkOptions{})
+	if err == nil || !strings.Contains(err.Error(), "nothing was changed") {
+		t.Fatalf("want a rollback error, got %v", err)
+	}
+	_ = os.Chmod(dir, 0o755)
+	if exists(newAbs) || exists(filepath.Dir(newAbs)) {
+		t.Fatal("the new link (and its folder) should be rolled back")
+	}
+	if ev.read(oldAbs) != "lossy" {
+		t.Fatal("the original must be untouched")
+	}
+	stored, _ := ev.st.Op(ctx, op.ID)
+	if stored.Status != store.OpFailed || stored.QuarantinePath != "" {
+		t.Fatalf("op should be failed with no quarantine left, got %s %q", stored.Status, stored.QuarantinePath)
+	}
+	if entries, _ := os.ReadDir(filepath.Join(ev.slave, ".playlink-trash")); len(entries) != 0 {
+		t.Fatalf("quarantine should be empty after rollback, has %d entries", len(entries))
+	}
+}
+
+// If the original was swapped for another file mid-way, the quarantine copy
+// is the only copy of the original's data, so it is kept.
+func TestCrossFolderLinkKeepsQuarantineWhenOriginalWasSwapped(t *testing.T) {
+	ev, m, _, oldAbs, newAbs := crossFolder(t)
+	testHookBeforeRemove = func(p string) {
+		_ = os.Remove(p)
+		_ = os.WriteFile(p, []byte("someone else's new file"), 0o644)
+	}
+	op, err := ev.e.Link(ctx, m.ID, LinkOptions{})
+	if err == nil || !errors.Is(err, fsops.ErrChanged) {
+		t.Fatalf("want a changed-file error, got %v", err)
+	}
+	if exists(newAbs) {
+		t.Fatal("the new link should be rolled back")
+	}
+	if ev.read(oldAbs) != "someone else's new file" {
+		t.Fatal("the new file must not be touched")
+	}
+	stored, _ := ev.st.Op(ctx, op.ID)
+	if stored.QuarantinePath == "" || ev.read(stored.QuarantinePath) != "lossy" {
+		t.Fatal("the original's data must stay in quarantine")
+	}
+}
+
+// Leftovers from the old bug: the master's path already holds the link and
+// the original is still there. Linking again only retires the original.
+func TestRelinkCleansUpLeftoverOriginal(t *testing.T) {
+	ev, _, masterAbs, oldAbs, newAbs := crossFolder(t)
+	if err := os.MkdirAll(filepath.Dir(newAbs), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Link(masterAbs, newAbs); err != nil {
+		t.Fatal(err)
+	}
+	ev.scan()
+	m := ev.match("Composer/Game Rips/01 Song.mp3")
+	op, err := ev.e.Link(ctx, m.ID, LinkOptions{})
+	if err != nil {
+		t.Fatalf("relinking over an existing link of the master should work: %v", err)
+	}
+	if exists(oldAbs) || !sameInode(t, masterAbs, newAbs) {
+		t.Fatal("the leftover original should be retired and the link kept")
+	}
+	if err := ev.e.Undo(ctx, op.ID); err != nil {
+		t.Fatal(err)
+	}
+	if ev.read(oldAbs) != "lossy" || !sameInode(t, masterAbs, newAbs) {
+		t.Fatal("undo should restore the original and keep the link that existed before")
+	}
+}
+
+// A different file at the target is still refused.
+func TestRelinkRefusesForeignFileAtTarget(t *testing.T) {
+	ev, m, _, oldAbs, newAbs := crossFolder(t)
+	ev.write(ev.slave, "Composer/Game1/01 Song.flac", "unrelated")
+	if _, err := ev.e.Link(ctx, m.ID, LinkOptions{}); !errors.Is(err, fsops.ErrExists) {
+		t.Fatalf("want ErrExists, got %v", err)
+	}
+	if ev.read(newAbs) != "unrelated" || ev.read(oldAbs) != "lossy" {
+		t.Fatal("nothing may change")
+	}
+}

internal/store/models.go

@@ -624,6 +624,7 @@ type Operation struct {
 	RelNew         string `json:"rel_new"`
 	MatchKind      string `json:"match_kind"`
 	MatchReason    string `json:"match_reason"`
+	LinkPreexisted bool   `json:"link_preexisted"`
 	QuarantinePath string `json:"quarantine_path"`
 	QuarantineSide string `json:"quarantine_side"`
 	TmpPath        string `json:"-"`
@@ -656,13 +657,13 @@ const (
 )
 
 const opCols = `id, pair_id, action, rel_master, rel_slave, rel_new, quarantine_path, quarantine_side, tmp_path, bytes,
-	dry_run, trigger, status, error, created_at, finished_at, undone_at, purged_at, match_kind, match_reason`
+	dry_run, trigger, status, error, created_at, finished_at, undone_at, purged_at, match_kind, match_reason, link_preexisted`
 
 func scanOp(r scanner) (Operation, error) {
 	var o Operation
 	var fin, und, pur sql.NullInt64
 	err := r.Scan(&o.ID, &o.PairID, &o.Action, &o.RelMaster, &o.RelSlave, &o.RelNew, &o.QuarantinePath, &o.QuarantineSide,
-		&o.TmpPath, &o.Bytes, &o.DryRun, &o.Trigger, &o.Status, &o.Error, &o.CreatedAt, &fin, &und, &pur, &o.MatchKind, &o.MatchReason)
+		&o.TmpPath, &o.Bytes, &o.DryRun, &o.Trigger, &o.Status, &o.Error, &o.CreatedAt, &fin, &und, &pur, &o.MatchKind, &o.MatchReason, &o.LinkPreexisted)
 	if fin.Valid {
 		o.FinishedAt = &fin.Int64
 	}
@@ -678,10 +679,10 @@ func scanOp(r scanner) (Operation, error) {
 func (s *Store) CreateOp(ctx context.Context, o *Operation) error {
 	o.CreatedAt = now()
 	res, err := s.DB.ExecContext(ctx, `INSERT INTO operations (pair_id, action, rel_master, rel_slave, rel_new, quarantine_path,
-		quarantine_side, tmp_path, bytes, dry_run, trigger, status, error, created_at, match_kind, match_reason)
+		quarantine_side, tmp_path, bytes, dry_run, trigger, status, error, created_at, match_kind, match_reason, link_preexisted)
-		VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`,
+		VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`,
 		o.PairID, o.Action, o.RelMaster, o.RelSlave, o.RelNew, o.QuarantinePath, o.QuarantineSide, o.TmpPath, o.Bytes,
-		o.DryRun, o.Trigger, o.Status, o.Error, o.CreatedAt, o.MatchKind, o.MatchReason)
+		o.DryRun, o.Trigger, o.Status, o.Error, o.CreatedAt, o.MatchKind, o.MatchReason, o.LinkPreexisted)
 	if err != nil {
 		return err
 	}

internal/store/store.go

@@ -114,6 +114,10 @@ var migrations = []string{
 	// matches that are not at the same path (other format, approximate, ...).
 	`ALTER TABLE operations ADD COLUMN match_kind TEXT NOT NULL DEFAULT '';
 	ALTER TABLE operations ADD COLUMN match_reason TEXT NOT NULL DEFAULT '';`,
+
+	// 5: a link whose target already existed (only the original was retired);
+	// undo must then leave that target alone.
+	`ALTER TABLE operations ADD COLUMN link_preexisted INTEGER NOT NULL DEFAULT 0;`,
 }
 
 // Open opens (and migrates) the database in dataDir.