From 176a527ec7711c9ed76ae0548ebd2d71047206e1 Mon Sep 17 00:00:00 2001 From: Chris Date: Fri, 25 Sep 2026 13:40:59 -0700 Subject: [PATCH] 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 --- internal/engine/ops.go | 99 ++++++++++++++++++---- internal/engine/relink_test.go | 146 +++++++++++++++++++++++++++++++++ internal/store/models.go | 11 +-- internal/store/store.go | 4 + 4 files changed, 237 insertions(+), 23 deletions(-) create mode 100644 internal/engine/relink_test.go diff --git a/internal/engine/ops.go b/internal/engine/ops.go index d69e628..8fde97b 100644 --- a/internal/engine/ops.go +++ b/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) { - return fmt.Errorf("%s %w", p, fsops.ErrChanged) + if q != "" { + 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 { - return store.Operation{}, fmt.Errorf("slave already has %s: %w", newRel, fsops.ErrExists) + if cur, err := fsops.Lstat(newAbs); err == nil { + 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,20 +295,36 @@ func (e *Engine) Link(ctx context.Context, matchID int64, opt LinkOptions) (stor } return nil } - err := fsops.MkdirAllLike(filepath.Dir(newAbs)) - if err == nil { - err = fsops.LinkNew(s.masterAbs, s.mi, newAbs) + if !preexisting { + err := fsops.MkdirAllLike(filepath.Dir(newAbs)) + if err == nil { + err = fsops.LinkNew(s.masterAbs, s.mi, newAbs) + } + if err != nil { + pruneEmpty(filepath.Dir(newAbs), l.slave.Path) + if q != "" { + dropQuarantine(l, q, s.si) + e.clearQuarantine(ctx, o.ID) + } + return err + } } - if err != nil { - pruneEmpty(filepath.Dir(newAbs), l.slave.Path) - if q != "" { + 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 err - } - if err := removeOriginal(s.slaveAbs, s.si); err != nil { - return err + 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 } diff --git a/internal/engine/relink_test.go b/internal/engine/relink_test.go new file mode 100644 index 0000000..73176fe --- /dev/null +++ b/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") + } +} diff --git a/internal/store/models.go b/internal/store/models.go index 17ca807..94c918c 100644 --- a/internal/store/models.go +++ b/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) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + quarantine_side, tmp_path, bytes, dry_run, trigger, status, error, created_at, match_kind, match_reason, link_preexisted) + 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 } diff --git a/internal/store/store.go b/internal/store/store.go index 5e71565..04b9b9f 100644 --- a/internal/store/store.go +++ b/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.