From 6c57e25baf47143e45e7fb37f6baf7e50678dcf0 Mon Sep 17 00:00:00 2001
From: ChakraFusion <112588066+ChakraFusion@users.noreply.github.com>
Date: Wed, 14 Oct 2026 09:59:13 +0200
Subject: [PATCH 1/3] snapshots: no second copy of a save already kept

A newly tracked game had no recorded baseline (Game.LastManifestHash), a
snapshot taken by hand left the one from before, and changing a game's
exclusions cleared it. Each time, the next file event of any kind - a log
written into a folder just excluded from syncing - or the catch-up a watch
runs when it starts read as the save changing, and an automatic snapshot
was taken of a save already kept. With cloud sync on, that copy was then
the device's newest save, and a newer one another device had put in the
cloud was no longer offered.

TestIgnored_ASaveFromTheCloudLeavesAnIgnoredFileAlone failed that way on
Windows CI whenever the extra snapshot came after the other device's (seen
once in 150 runs with the race detector, with the daemons' logs).

- The initial snapshot and snapshots taken by hand (app, `opensave
  snapshot`, snapshot everything) record the save as it was just before
  them (daemon.SnapshotWithBaseline). A change made while one is being
  written differs from it and is still snapshotted by the watcher. Worst
  case one snapshot too many, as before; never a change taken for the
  baseline.
- An exclusion change restates the recorded hash under the new rules only
  when that claims nothing new: the save is as it was at the last snapshot
  under the old rules, and the new rules bring no file back into view.
  Otherwise it is cleared as before. The app and `opensave exclude` share it
  (daemon.LastSnapshotHashUnder).

Tests: e2e TestBaseline_AFileInAnExcludedFolderTakesNoSnapshot (fails on
dev-public: 2 snapshots), TestBaseline_ASnapshotByHandIsNotTakenAgain (fails
without the change: 3), TestBaseline_TheFirstChangeAfterTrackingIsSnapshotted,
and TestLastSnapshotHashUnder for the four cases.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 e2e/baseline_test.go                     |  78 +++++++++++++++
 internal/api/routes.go                   |  13 +--
 internal/cliapp/cliapp.go                |   2 +-
 internal/cliapp/commands_exclude_game.go |   3 +-
 internal/daemon/autotrack.go             |   2 +-
 internal/daemon/baseline.go              | 121 +++++++++++++++++++++++
 internal/daemon/baseline_test.go         |  76 ++++++++++++++
 internal/daemon/daemon.go                |   2 +-
 internal/daemon/snapshot_all.go          |   2 +-
 9 files changed, 288 insertions(+), 11 deletions(-)
 create mode 100644 e2e/baseline_test.go
 create mode 100644 internal/daemon/baseline.go
 create mode 100644 internal/daemon/baseline_test.go

diff --git a/e2e/baseline_test.go b/e2e/baseline_test.go
new file mode 100644
index 00000000..9da72d65
--- /dev/null
+++ b/e2e/baseline_test.go
@@ -0,0 +1,78 @@
+package e2e
+
+import (
+	"net/http"
+	"testing"
+	"time"
+
+	"github.com/opensave/opensave/testutil"
+)
+
+// A newly tracked game's save is recorded as its baseline, so a file event
+// that changes nothing synced takes no second snapshot of it.
+//
+// It had none: the first event of any kind after tracking snapshotted the
+// unchanged save again, and so did the first after the game's exclusions
+// changed. A game writing a log into a folder excluded from syncing did that
+// straight away. With cloud sync on, the copy was then the device's newest
+// save, and a newer one another device had put in the cloud was no longer
+// offered — TestIgnored_ASaveFromTheCloudLeavesAnIgnoredFileAlone failed that
+// way whenever the extra snapshot came after the other device's.
+
+func TestBaseline_AFileInAnExcludedFolderTakesNoSnapshot(t *testing.T) {
+	d := testutil.NewTestDaemon(t, "BaselineExcluded")
+	d.WriteSave("settings.ini", "graphics")
+	d.WriteSave("slot1.sav", "act 1")
+	game := d.TrackGame("Baseline Game")
+	waitForWatch(t, d, game)
+	if !testutil.WaitFor(30*time.Second, func() bool { return countSnapshots(d, game) == 1 }) {
+		t.Fatalf("the initial snapshot was not taken: %d snapshots", countSnapshots(d, game))
+	}
+	d.API(http.MethodPatch, "/api/games/"+game, map[string]any{"syncIgnore": "settings.ini\nlogs/"}, nil)
+
+	d.WriteSave("logs/session.log", "started")
+	d.WriteSave("settings.ini", "other graphics")
+	time.Sleep(8 * time.Second) // well past the watcher's debounce
+	if n := countSnapshots(d, game); n != 1 {
+		t.Errorf("%d snapshots after only excluded files changed, want the initial one alone", n)
+	}
+
+	// A real change is still taken.
+	d.WriteSave("slot1.sav", "act 2")
+	if !testutil.WaitFor(30*time.Second, func() bool { return countSnapshots(d, game) == 2 }) {
+		t.Errorf("a change to the save was not snapshotted: %d snapshots", countSnapshots(d, game))
+	}
+}
+
+// A snapshot taken by hand is the baseline from then on: the change it kept is
+// not snapshotted a second time when the watcher gets to it.
+func TestBaseline_ASnapshotByHandIsNotTakenAgain(t *testing.T) {
+	d := testutil.NewTestDaemon(t, "BaselineManual")
+	d.WriteSave("slot1.sav", "act 1")
+	game := d.TrackGame("Baseline Manual Game")
+	waitForWatch(t, d, game)
+	if !testutil.WaitFor(30*time.Second, func() bool { return countSnapshots(d, game) == 1 }) {
+		t.Fatalf("the initial snapshot was not taken: %d snapshots", countSnapshots(d, game))
+	}
+	d.WriteSave("slot1.sav", "act 2")
+	d.API(http.MethodPost, "/api/games/"+game+"/snapshot", map[string]string{"comment": "before the boss"}, nil)
+	time.Sleep(8 * time.Second) // well past the watcher's debounce
+	if n := countSnapshots(d, game); n != 2 {
+		t.Errorf("%d snapshots, want the initial one and the one taken by hand", n)
+	}
+}
+
+// The first real change after tracking is snapshotted as before.
+func TestBaseline_TheFirstChangeAfterTrackingIsSnapshotted(t *testing.T) {
+	d := testutil.NewTestDaemon(t, "BaselineFirst")
+	d.WriteSave("slot1.sav", "act 1")
+	game := d.TrackGame("Baseline First Game")
+	waitForWatch(t, d, game)
+	if !testutil.WaitFor(30*time.Second, func() bool { return countSnapshots(d, game) == 1 }) {
+		t.Fatalf("the initial snapshot was not taken: %d snapshots", countSnapshots(d, game))
+	}
+	d.WriteSave("slot1.sav", "act 2")
+	if !testutil.WaitFor(30*time.Second, func() bool { return countSnapshots(d, game) == 2 }) {
+		t.Errorf("the first change after tracking was not snapshotted: %d snapshots", countSnapshots(d, game))
+	}
+}
diff --git a/internal/api/routes.go b/internal/api/routes.go
index 920b1a33..b464526b 100644
--- a/internal/api/routes.go
+++ b/internal/api/routes.go
@@ -442,14 +442,15 @@ func (s *Server) handleUpdateGame(w http.ResponseWriter, r *http.Request) {
 			s.Daemon.Log.Log("warn", fmt.Sprintf(
 				"could not reset sync agreement for %q after its exclusions changed: %v", game.Name, err))
 		}
-		// The last-snapshot hash goes too, for exactly the same reason: it was
+		// The last-snapshot hash too, for exactly the same reason: it was
 		// recorded over the old view, and the sync engine compares against it
 		// to decide whether the save holds changes a pull would overwrite. A
 		// value that can never match again makes that check fire on every
-		// pull. Clearing it disables the check until the next snapshot
-		// re-records it, which is the safe direction — it can cost one
-		// unnecessary prompt, never a silent overwrite.
-		game.LastManifestHash = ""
+		// pull. Restated in the new view when the save provably has not
+		// changed since that snapshot, and cleared otherwise, which disables
+		// the check until the next snapshot re-records it — the safe
+		// direction (daemon.LastSnapshotHashUnder).
+		game.LastManifestHash = s.Daemon.LastSnapshotHashUnder(game, oldIgnore)
 	}
 
 	if err := s.Daemon.Store.UpdateGame(game); err != nil {
@@ -609,7 +610,7 @@ func (s *Server) handleCreateSnapshot(w http.ResponseWriter, r *http.Request) {
 	}
 	_ = readJSON(r, &body) // empty body is fine
 
-	snap, err := s.Daemon.Snapshots.Create(gameID, body.Comment, false)
+	snap, err := s.Daemon.SnapshotWithBaseline(gameID, body.Comment, false)
 	if err != nil {
 		writeError(w, notFoundToStatus(err), err.Error())
 		return
diff --git a/internal/cliapp/cliapp.go b/internal/cliapp/cliapp.go
index 6bdd7183..4e2f6714 100644
--- a/internal/cliapp/cliapp.go
+++ b/internal/cliapp/cliapp.go
@@ -847,7 +847,7 @@ func cmdSnapshot(d *daemon.Daemon, args []string) int {
 	}
 	comment := strings.Join(words, " ")
 
-	snap, err := d.Snapshots.Create(args[0], comment, false)
+	snap, err := d.SnapshotWithBaseline(args[0], comment, false)
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "error: %v\n", err)
 		return 1
diff --git a/internal/cliapp/commands_exclude_game.go b/internal/cliapp/commands_exclude_game.go
index 69d24a8b..bc371455 100644
--- a/internal/cliapp/commands_exclude_game.go
+++ b/internal/cliapp/commands_exclude_game.go
@@ -119,6 +119,7 @@ func saveIgnore(d *daemon.Daemon, asJSON bool, gameID, gameName string, lines []
 	if err != nil {
 		return fail(asJSON, err)
 	}
+	oldIgnore := game.SyncIgnore
 	game.SyncIgnore = strings.Join(lines, "\n")
 
 	// Same reset the API performs: the recorded agreement was measured over a
@@ -129,7 +130,7 @@ func saveIgnore(d *daemon.Daemon, asJSON bool, gameID, gameName string, lines []
 		d.P2P.Sync.FilteredContentHash(gameID, game)); err != nil {
 		d.Log.Log("warn", "could not reset sync agreement after an exclusion change: "+err.Error())
 	}
-	game.LastManifestHash = ""
+	game.LastManifestHash = d.LastSnapshotHashUnder(game, oldIgnore)
 
 	if err := d.Store.UpdateGame(game); err != nil {
 		return fail(asJSON, err)
diff --git a/internal/daemon/autotrack.go b/internal/daemon/autotrack.go
index 039e1326..dc92aa01 100644
--- a/internal/daemon/autotrack.go
+++ b/internal/daemon/autotrack.go
@@ -41,7 +41,7 @@ func (d *Daemon) adoptAutoTracked(game store.Game) {
 			readDone, err := d.P2P.Sync.Reading(ctx, game.ID)
 			cancel()
 			if err == nil {
-				_, err = d.Snapshots.Create(game.ID, "Initial snapshot", true)
+				_, err = d.SnapshotWithBaseline(game.ID, "Initial snapshot", true)
 				readDone()
 			}
 			if err != nil {
diff --git a/internal/daemon/baseline.go b/internal/daemon/baseline.go
new file mode 100644
index 00000000..e21f0485
--- /dev/null
+++ b/internal/daemon/baseline.go
@@ -0,0 +1,121 @@
+package daemon
+
+import (
+	"github.com/opensave/opensave/internal/delta"
+	"github.com/opensave/opensave/internal/ignore"
+	"github.com/opensave/opensave/internal/store"
+	"github.com/opensave/opensave/internal/watcher"
+)
+
+// SnapshotWithBaseline takes a snapshot of a game's save and records, as the
+// hash the watcher compares against, the save as it was just before it — for
+// the snapshots taken outside the watcher: a newly tracked game's first one,
+// and those taken by hand.
+//
+// Without it, a newly tracked game had no baseline, and one snapshotted by
+// hand kept the one from before. Either way the next file event of any kind —
+// a log written into an excluded folder — or the catch-up a watch runs when it
+// starts read as the save changing, and an automatic snapshot was taken of a
+// save already kept. With cloud sync on, that copy was then this device's
+// newest save, and a newer one another device had put in the cloud meanwhile
+// was no longer offered.
+//
+// Taken before the snapshot, not after: a change made while it is being
+// written then differs from the baseline, and the watcher snapshots it. The
+// worst case is one snapshot too many, as before; never a change taken for the
+// baseline. (The watcher itself never writes a baseline for that reason — see
+// catchUpOne.)
+func (d *Daemon) SnapshotWithBaseline(gameID, comment string, isSystemAuto bool) (store.Snapshot, error) {
+	before, hashErr := "", error(nil)
+	if game, err := d.Store.GetGame(gameID); err == nil {
+		before, hashErr = d.currentContentHash(game)
+	} else {
+		hashErr = err
+	}
+	snap, err := d.Snapshots.Create(gameID, comment, isSystemAuto)
+	if err != nil {
+		return snap, err
+	}
+	if hashErr == nil {
+		_ = d.Store.SetLastManifestHash(snap.GameID, before)
+	}
+	return snap, nil
+}
+
+// LastSnapshotHashUnder restates a game's last-snapshot hash under the
+// exclusion rules it has just been given (game.SyncIgnore), or returns "" when
+// that cannot be done safely. oldIgnore is the rules the hash was recorded
+// under.
+//
+// The recorded value was taken over the old rules, so it can never match
+// again. Cleared, the next file event of any kind — a log written into a
+// folder that was just excluded — read as the save changing, and an automatic
+// snapshot was taken of a save nobody had changed. With cloud sync on, that
+// copy was then this device's newest save, and a newer one from another
+// device was no longer offered.
+//
+// Restated only when that claims nothing new: the save is exactly as it was at
+// the last snapshot under the old rules, and the new rules bring no file back
+// into view. Otherwise "", as before: the safe direction, which can cost one
+// unnecessary snapshot but never leaves a change unsnapshotted.
+func (d *Daemon) LastSnapshotHashUnder(game store.Game, oldIgnore string) string {
+	if game.LastManifestHash == "" {
+		return ""
+	}
+	extra, err := d.Store.GameRootPaths(game.ID)
+	if err != nil {
+		return ""
+	}
+	// As the save is now, not as a cache last saw it: a change it has not
+	// noticed yet must count as one.
+	delta.InvalidateRoot(game.SavePath)
+	for _, p := range extra {
+		delta.InvalidateRoot(p)
+	}
+	m, failures, err := delta.BuildMultiManifest(game.SavePath, extra)
+	if err != nil || len(failures) > 0 {
+		return ""
+	}
+	if watcher.ContentHash(m, oldIgnore) != game.LastManifestHash {
+		return "" // changed since the last snapshot: the next one takes it
+	}
+	if bringsBack(m, ignore.Parse(oldIgnore), ignore.Parse(game.SyncIgnore)) {
+		return ""
+	}
+	return watcher.ContentHash(m, game.SyncIgnore)
+}
+
+// bringsBack reports whether any path in m was excluded by the old rules and
+// is not by the new ones. Such a file was never part of the hash recorded, so
+// nothing says it is unchanged since the last snapshot.
+func bringsBack(m delta.Manifest, oldRules, newRules ignore.Rules) bool {
+	if oldRules.Empty() {
+		return false
+	}
+	back := func(p string) bool {
+		return oldRules.Match(p) && (newRules.Empty() || !newRules.Match(p))
+	}
+	for p := range m.Files {
+		if back(p) {
+			return true
+		}
+	}
+	for _, dir := range m.Dirs {
+		if back(dir) {
+			return true
+		}
+	}
+	for _, root := range m.Extra {
+		for p := range root.Files {
+			if back(p) {
+				return true
+			}
+		}
+		for _, dir := range root.Dirs {
+			if back(dir) {
+				return true
+			}
+		}
+	}
+	return false
+}
diff --git a/internal/daemon/baseline_test.go b/internal/daemon/baseline_test.go
new file mode 100644
index 00000000..5b476ac7
--- /dev/null
+++ b/internal/daemon/baseline_test.go
@@ -0,0 +1,76 @@
+package daemon
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/opensave/opensave/internal/delta"
+	"github.com/opensave/opensave/internal/store"
+	"github.com/opensave/opensave/internal/watcher"
+)
+
+// A game's last-snapshot hash restated under new exclusion rules: only when
+// the save is provably as it was at that snapshot, and the new rules bring no
+// file back; "" otherwise, which takes the next snapshot as before.
+func TestLastSnapshotHashUnder(t *testing.T) {
+	d := newTestDaemon(t)
+	write := func(dir, rel, content string) {
+		t.Helper()
+		p := filepath.Join(dir, filepath.FromSlash(rel))
+		if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.WriteFile(p, []byte(content), 0o644); err != nil {
+			t.Fatal(err)
+		}
+		delta.InvalidateRoot(dir)
+	}
+	hashOf := func(dir, rules string) string {
+		t.Helper()
+		m, _, err := delta.BuildMultiManifest(dir, nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+		return watcher.ContentHash(m, rules)
+	}
+	setUp := func(oldRules string) (store.Game, string) {
+		dir := t.TempDir()
+		write(dir, "slot1.sav", "act 1")
+		write(dir, "settings.ini", "graphics")
+		write(dir, "logs/a.log", "log")
+		return store.Game{ID: "g-" + filepath.Base(dir), SavePath: dir, SyncIgnore: oldRules,
+			LastManifestHash: hashOf(dir, oldRules)}, dir
+	}
+
+	t.Run("unchanged save, narrower view: restated", func(t *testing.T) {
+		game, dir := setUp("")
+		game.SyncIgnore = "logs/\nsettings.ini"
+		if got, want := d.LastSnapshotHashUnder(game, ""), hashOf(dir, game.SyncIgnore); got != want {
+			t.Errorf("got %q, want the save's hash under the new rules %q", got, want)
+		}
+	})
+	t.Run("changed since the snapshot: cleared", func(t *testing.T) {
+		game, dir := setUp("")
+		write(dir, "slot1.sav", "act 2")
+		game.SyncIgnore = "logs/"
+		if got := d.LastSnapshotHashUnder(game, ""); got != "" {
+			t.Errorf("got %q for a save changed since its snapshot; that change would go unsnapshotted", got)
+		}
+	})
+	t.Run("a file brought back into view: cleared", func(t *testing.T) {
+		game, _ := setUp("logs/\nsettings.ini")
+		game.SyncIgnore = "logs/"
+		if got := d.LastSnapshotHashUnder(game, "logs/\nsettings.ini"); got != "" {
+			t.Errorf("got %q, though settings.ini was never part of the recorded hash", got)
+		}
+	})
+	t.Run("nothing recorded: nothing to restate", func(t *testing.T) {
+		game, _ := setUp("")
+		game.LastManifestHash = ""
+		game.SyncIgnore = "logs/"
+		if got := d.LastSnapshotHashUnder(game, ""); got != "" {
+			t.Errorf("got %q with no recorded hash", got)
+		}
+	})
+}
diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go
index 3d732f86..d655e226 100644
--- a/internal/daemon/daemon.go
+++ b/internal/daemon/daemon.go
@@ -801,7 +801,7 @@ func (d *Daemon) TrackGame(game store.Game) (store.Game, error) {
 	d.initialSnapshots.Add()
 	go func() {
 		defer d.initialSnapshots.Done()
-		if _, err := d.Snapshots.Create(game.ID, "Initial snapshot", true); err != nil {
+		if _, err := d.SnapshotWithBaseline(game.ID, "Initial snapshot", true); err != nil {
 			d.Log.Log("warn", fmt.Sprintf("initial snapshot for %q failed: %v", game.Name, err))
 		}
 		if _, err := d.Store.GetGame(game.ID); err != nil {
diff --git a/internal/daemon/snapshot_all.go b/internal/daemon/snapshot_all.go
index 051700cc..6c984052 100644
--- a/internal/daemon/snapshot_all.go
+++ b/internal/daemon/snapshot_all.go
@@ -28,7 +28,7 @@ func (d *Daemon) SnapshotAll(comment string) SnapshotAllResult {
 		return res
 	}
 	for _, g := range games {
-		if _, err := d.Snapshots.Create(g.ID, comment, false); err != nil {
+		if _, err := d.SnapshotWithBaseline(g.ID, comment, false); err != nil {
 			res.Failed = append(res.Failed, struct {
 				GameID string `json:"gameId"`
 				Name   string `json:"name"`

From 1a186c83ce66cb8feaf897e33ea863772501423c Mon Sep 17 00:00:00 2001
From: ChakraFusion <112588066+ChakraFusion@users.noreply.github.com>
Date: Wed, 14 Oct 2026 17:23:00 +0200
Subject: [PATCH 2/3] snapshots: adding a save location takes no snapshot
 unless it brings files; Stop waits for a catch-up

Follow-up to the first commit, found from this PR's own CI failure
(TestAddingAnOrdinarySaveLocationStillWorks, Linux: "TempDir RemoveAll
cleanup: ... 002: directory not empty").

- The hash the watcher compares against covers every save location, so
  adding or removing one changed it while no file changed. With a baseline
  now recorded at tracking, the catch-up the re-watch runs then snapshotted
  a save already kept. A location change now restates the hash when the
  save is as it was at the last snapshot and an added location holds no
  file yet (daemon.LocationsChanged, from the app and `opensave locations`).
  A location that brings files is still snapshotted: the backup should
  hold them.
- Stop cancelled the catch-up worker without waiting for it, so a catch-up
  already writing a snapshot went on after Stop returned - into a daemon
  home the test was removing ("002" is the test server's home), or past a
  closed database as the app quits. Stop now waits for it, bounded like
  stopping a watch.

Tests: e2e TestBaseline_AddingASaveLocationTakesNoSnapshotUnlessItBringsFiles
(fails without the change: 2 snapshots) and TestStopWaitsForACatchUpUnderWay
(fails without the change: Stop returns mid-snapshot).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 e2e/baseline_test.go                  | 40 +++++++++++++++++++
 internal/api/routes_roots.go          |  4 ++
 internal/cliapp/commands_locations.go |  4 ++
 internal/daemon/baseline.go           | 45 ++++++++++++++++++++++
 internal/watcher/stop_catchup_test.go | 55 +++++++++++++++++++++++++++
 internal/watcher/watcher.go           | 28 ++++++++++++--
 6 files changed, 172 insertions(+), 4 deletions(-)
 create mode 100644 internal/watcher/stop_catchup_test.go

diff --git a/e2e/baseline_test.go b/e2e/baseline_test.go
index 9da72d65..f7482c58 100644
--- a/e2e/baseline_test.go
+++ b/e2e/baseline_test.go
@@ -2,6 +2,8 @@ package e2e
 
 import (
 	"net/http"
+	"os"
+	"path/filepath"
 	"testing"
 	"time"
 
@@ -76,3 +78,41 @@ func TestBaseline_TheFirstChangeAfterTrackingIsSnapshotted(t *testing.T) {
 		t.Errorf("the first change after tracking was not snapshotted: %d snapshots", countSnapshots(d, game))
 	}
 }
+
+// Adding a save location changes no file, so it takes no snapshot; one that
+// brings files with it is snapshotted, as the backup should hold them.
+//
+// The hash the watcher compares against covers every location, so adding one
+// changed it, and the catch-up a watch runs when it starts snapshotted a save
+// already kept.
+func TestBaseline_AddingASaveLocationTakesNoSnapshotUnlessItBringsFiles(t *testing.T) {
+	d := testutil.NewTestDaemon(t, "BaselineLocation")
+	d.WriteSave("slot1.sav", "act 1")
+	game := d.TrackGame("Baseline Location Game")
+	waitForWatch(t, d, game)
+	if !testutil.WaitFor(30*time.Second, func() bool { return countSnapshots(d, game) == 1 }) {
+		t.Fatalf("the initial snapshot was not taken: %d snapshots", countSnapshots(d, game))
+	}
+
+	empty := filepath.Join(testutil.TempDir(t), "Config")
+	if err := os.MkdirAll(empty, 0o777); err != nil {
+		t.Fatal(err)
+	}
+	d.API(http.MethodPost, "/api/games/"+game+"/roots", map[string]string{"name": "config", "path": empty}, nil)
+	time.Sleep(8 * time.Second) // past the debounce and the catch-up
+	if n := countSnapshots(d, game); n != 1 {
+		t.Errorf("%d snapshots after adding an empty location, want the initial one alone", n)
+	}
+
+	full := filepath.Join(testutil.TempDir(t), "Mods")
+	if err := os.MkdirAll(full, 0o777); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(full, "mod.cfg"), []byte("on"), 0o666); err != nil {
+		t.Fatal(err)
+	}
+	d.API(http.MethodPost, "/api/games/"+game+"/roots", map[string]string{"name": "mods", "path": full}, nil)
+	if !testutil.WaitFor(30*time.Second, func() bool { return countSnapshots(d, game) == 2 }) {
+		t.Errorf("a location that brought files was not snapshotted: %d snapshots", countSnapshots(d, game))
+	}
+}
diff --git a/internal/api/routes_roots.go b/internal/api/routes_roots.go
index 77915bf3..7718b5de 100644
--- a/internal/api/routes_roots.go
+++ b/internal/api/routes_roots.go
@@ -72,10 +72,12 @@ func (s *Server) handleAddGameRoot(w http.ResponseWriter, r *http.Request) {
 	// Rejections here are the overlap and naming rules, which are the user's
 	// to hear about rather than a server fault: "that folder is already
 	// covered by the main save" is a sentence, not a 500.
+	oldRoots, _ := s.Daemon.Store.GameRootPaths(gameID)
 	if err := s.Daemon.Store.AddGameRoot(gameID, body.Name, path); err != nil {
 		writeError(w, http.StatusBadRequest, err.Error())
 		return
 	}
+	s.Daemon.LocationsChanged(gameID, oldRoots)
 	// Start watching it now, not at the next restart.
 	s.Daemon.RewatchGame(gameID)
 	s.BroadcastGamesUpdate()
@@ -90,10 +92,12 @@ func (s *Server) handleRemoveGameRoot(w http.ResponseWriter, r *http.Request) {
 		writeError(w, http.StatusBadRequest, "invalid location name")
 		return
 	}
+	oldRoots, _ := s.Daemon.Store.GameRootPaths(gameID)
 	if err := s.Daemon.Store.RemoveGameRoot(gameID, name); err != nil {
 		writeError(w, http.StatusInternalServerError, err.Error())
 		return
 	}
+	s.Daemon.LocationsChanged(gameID, oldRoots)
 	// Stop watching it now, or it keeps waking the game up over a folder
 	// nobody is covering any more.
 	s.Daemon.RewatchGame(gameID)
diff --git a/internal/cliapp/commands_locations.go b/internal/cliapp/commands_locations.go
index e052ab6d..671cae3a 100644
--- a/internal/cliapp/commands_locations.go
+++ b/internal/cliapp/commands_locations.go
@@ -29,9 +29,11 @@ func cmdLocations(d *daemon.Daemon, args []string) int {
 		if err != nil {
 			return fail(asJSON, err)
 		}
+		oldRoots, _ := d.Store.GameRootPaths(gameID)
 		if err := d.Store.AddGameRoot(gameID, name, abs); err != nil {
 			return fail(asJSON, err)
 		}
+		d.LocationsChanged(gameID, oldRoots)
 		if asJSON {
 			return emitJSON(map[string]any{"game": gameID, "location": name, "path": abs})
 		}
@@ -44,9 +46,11 @@ func cmdLocations(d *daemon.Daemon, args []string) int {
 			fmt.Fprintln(os.Stderr, locationsUsage)
 			return 1
 		}
+		oldRoots, _ := d.Store.GameRootPaths(args[1])
 		if err := d.Store.RemoveGameRoot(args[1], args[2]); err != nil {
 			return fail(asJSON, err)
 		}
+		d.LocationsChanged(args[1], oldRoots)
 		if asJSON {
 			return emitJSON(map[string]any{"removed": args[2]})
 		}
diff --git a/internal/daemon/baseline.go b/internal/daemon/baseline.go
index e21f0485..586f5dc0 100644
--- a/internal/daemon/baseline.go
+++ b/internal/daemon/baseline.go
@@ -85,6 +85,51 @@ func (d *Daemon) LastSnapshotHashUnder(game store.Game, oldIgnore string) string
 	return watcher.ContentHash(m, game.SyncIgnore)
 }
 
+// LocationsChanged restates a game's last-snapshot hash after one of its save
+// locations was added or removed (oldRoots is GameRootPaths from before), when
+// that claims nothing new. Called before the game is watched again.
+//
+// The hash covers every location, so adding or removing one changes it while
+// no file changed: the catch-up a watch runs when it starts then took a
+// snapshot of a save already kept. The test that adds a location ended while
+// that snapshot was still being written, which failed its cleanup on Linux CI.
+//
+// Restated only when the save is as it was at the last snapshot over the old
+// locations, and a location added holds no file yet. A location that brings
+// files with it is left to be snapshotted: the backup should hold it.
+func (d *Daemon) LocationsChanged(gameID string, oldRoots map[string]string) {
+	game, err := d.Store.GetGame(gameID)
+	if err != nil || game.LastManifestHash == "" {
+		return
+	}
+	newRoots, err := d.Store.GameRootPaths(gameID)
+	if err != nil {
+		return
+	}
+	build := func(roots map[string]string) (delta.Manifest, bool) {
+		delta.InvalidateRoot(game.SavePath)
+		for _, p := range roots {
+			delta.InvalidateRoot(p)
+		}
+		m, failures, err := delta.BuildMultiManifest(game.SavePath, roots)
+		return m, err == nil && len(failures) == 0
+	}
+	before, ok := build(oldRoots)
+	if !ok || watcher.ContentHash(before, game.SyncIgnore) != game.LastManifestHash {
+		return // changed since the last snapshot: the next one takes it
+	}
+	after, ok := build(newRoots)
+	if !ok {
+		return
+	}
+	for name := range newRoots {
+		if _, had := oldRoots[name]; !had && len(after.Extra[name].Files) > 0 {
+			return // a location that brings files: snapshotted, as it should be
+		}
+	}
+	_ = d.Store.SetLastManifestHash(gameID, watcher.ContentHash(after, game.SyncIgnore))
+}
+
 // bringsBack reports whether any path in m was excluded by the old rules and
 // is not by the new ones. Such a file was never part of the hash recorded, so
 // nothing says it is unchanged since the last snapshot.
diff --git a/internal/watcher/stop_catchup_test.go b/internal/watcher/stop_catchup_test.go
new file mode 100644
index 00000000..16a25c78
--- /dev/null
+++ b/internal/watcher/stop_catchup_test.go
@@ -0,0 +1,55 @@
+package watcher
+
+import (
+	"testing"
+	"time"
+)
+
+// Stop waits for a catch-up already taking a snapshot, so whatever stopping
+// is for — closing the database, quitting, removing the folder — does not
+// happen under it. It returned at once, and the snapshot went on writing into
+// a folder being removed (TestAddingAnOrdinarySaveLocationStillWorks on Linux
+// CI: "directory not empty").
+func TestStopWaitsForACatchUpUnderWay(t *testing.T) {
+	saveDir := t.TempDir()
+	col := newCollector()
+	col.manifestHashes["game1"] = "a save from before" // so the catch-up snapshots
+	cb := col.callbacks()
+	inSnapshot := make(chan struct{}, 1)
+	release := make(chan struct{})
+	take := cb.CreateSnapshot
+	cb.CreateSnapshot = func(gameID string) error {
+		select {
+		case inSnapshot <- struct{}{}:
+		default:
+		}
+		<-release
+		return take(gameID)
+	}
+	eng := New(cb)
+	if err := eng.Watch("game1", saveDir); err != nil {
+		t.Fatal(err)
+	}
+	select {
+	case <-inSnapshot:
+	case <-time.After(15 * time.Second):
+		t.Fatal("the catch-up never took its snapshot")
+	}
+
+	stopped := make(chan struct{})
+	go func() {
+		eng.Stop()
+		close(stopped)
+	}()
+	select {
+	case <-stopped:
+		t.Fatal("Stop returned while the catch-up was still taking its snapshot")
+	case <-time.After(500 * time.Millisecond):
+	}
+	close(release)
+	select {
+	case <-stopped:
+	case <-time.After(10 * time.Second):
+		t.Fatal("Stop did not return once the catch-up had finished")
+	}
+}
diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go
index 787c8a6b..357b422c 100644
--- a/internal/watcher/watcher.go
+++ b/internal/watcher/watcher.go
@@ -134,6 +134,10 @@ type Engine struct {
 	catchUp    chan catchUpJob
 	catchUpCtx context.Context
 	stopCatch  context.CancelFunc
+	// catchUpDone is closed when the catch-up worker has returned; Stop
+	// waits for it (bounded), so a check already taking a snapshot finishes
+	// before whatever Stop is for goes on.
+	catchUpDone chan struct{}
 
 	// reconcileEvery is this engine's copy of reconcileInterval, read once
 	// when it is built.
@@ -226,13 +230,17 @@ func New(cb Callbacks) *Engine {
 		// Dropping a catch-up when the queue is full is safe: the next change
 		// to that game snapshots it anyway, and the periodic watch reconcile
 		// re-queues it.
-		catchUp:    make(chan catchUpJob, 256),
-		catchUpCtx: ctx,
-		stopCatch:  cancel,
+		catchUp:     make(chan catchUpJob, 256),
+		catchUpCtx:  ctx,
+		stopCatch:   cancel,
+		catchUpDone: make(chan struct{}),
 		// Read here, on the caller's goroutine, not inside the worker.
 		reconcileEvery: reconcileInterval,
 	}
-	go e.catchUpWorker()
+	go func() {
+		defer close(e.catchUpDone)
+		e.catchUpWorker()
+	}()
 	go e.reconcileWorker()
 	return e
 }
@@ -540,6 +548,18 @@ func (e *Engine) Stop() {
 	for _, gw := range stopping {
 		gw.stop()
 	}
+	// A catch-up already under way may be writing a snapshot. Stop returned
+	// while it did, and the daemon went on to close its database and the
+	// app to quit, or a test to remove the folder it was writing into
+	// (TestAddingAnOrdinarySaveLocationStillWorks on Linux CI: "directory not
+	// empty"). Bounded, as stopping a watch is.
+	if e.catchUpDone != nil {
+		select {
+		case <-e.catchUpDone:
+		case <-time.After(watchStopTimeout):
+			e.log("warn", fmt.Sprintf("a catch-up check did not finish within %s of stopping; left running", watchStopTimeout))
+		}
+	}
 }
 
 // ErrSaveFolderMissing is a watch refused because the save folder is not

From b162fe230b714d6ce5992081b1d7e642e982a3b1 Mon Sep 17 00:00:00 2001
From: ChakraFusion <112588066+ChakraFusion@users.noreply.github.com>
Date: Wed, 14 Oct 2026 18:20:21 +0200
Subject: [PATCH 3/3] snapshots: a hash taken before a snapshot never replaces
 a newer one

A newly tracked game's first snapshot runs in the background. When a
snapshot was taken by hand while it ran, the hand-taken one could finish
first and record its value, and the first snapshot's older value then
replaced it - the watcher read the save as changed and took a snapshot of
one already kept. Seen in a daemon log on the fork: "auto-snapshot created"
right after a snapshot taken by hand of the same files.

SnapshotWithBaseline now records its value only while the recorded one is
still what it was when the snapshot started (Store.SetLastManifestHashIf).
When it is not, another snapshot recorded something newer, and that stands.

Test: TestSetLastManifestHashIf_DoesNotReplaceANewerValue.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 internal/daemon/baseline.go               | 10 +++++--
 internal/store/games.go                   | 13 +++++++++
 internal/store/last_manifest_hash_test.go | 32 +++++++++++++++++++++++
 3 files changed, 53 insertions(+), 2 deletions(-)
 create mode 100644 internal/store/last_manifest_hash_test.go

diff --git a/internal/daemon/baseline.go b/internal/daemon/baseline.go
index 586f5dc0..5b20e945 100644
--- a/internal/daemon/baseline.go
+++ b/internal/daemon/baseline.go
@@ -25,9 +25,15 @@ import (
 // worst case is one snapshot too many, as before; never a change taken for the
 // baseline. (The watcher itself never writes a baseline for that reason — see
 // catchUpOne.)
+//
+// Recorded only if no other snapshot recorded one in the meantime. A newly
+// tracked game's first snapshot runs in the background; a snapshot taken by
+// hand meanwhile finished first, and the first one's older value then replaced
+// its newer one, and the watcher snapshotted the save again.
 func (d *Daemon) SnapshotWithBaseline(gameID, comment string, isSystemAuto bool) (store.Snapshot, error) {
-	before, hashErr := "", error(nil)
+	before, was, hashErr := "", "", error(nil)
 	if game, err := d.Store.GetGame(gameID); err == nil {
+		was = game.LastManifestHash
 		before, hashErr = d.currentContentHash(game)
 	} else {
 		hashErr = err
@@ -37,7 +43,7 @@ func (d *Daemon) SnapshotWithBaseline(gameID, comment string, isSystemAuto bool)
 		return snap, err
 	}
 	if hashErr == nil {
-		_ = d.Store.SetLastManifestHash(snap.GameID, before)
+		_, _ = d.Store.SetLastManifestHashIf(snap.GameID, was, before)
 	}
 	return snap, nil
 }
diff --git a/internal/store/games.go b/internal/store/games.go
index 52d9c928..d6acc6de 100644
--- a/internal/store/games.go
+++ b/internal/store/games.go
@@ -177,6 +177,19 @@ func (s *Store) SetLastManifestHash(gameID, hash string) error {
 	return checkRowAffected(res)
 }
 
+// SetLastManifestHashIf records hash only while the recorded value is still
+// was: a hash taken before a snapshot must not replace one a later snapshot
+// recorded in the meantime. Reports whether it was recorded.
+func (s *Store) SetLastManifestHashIf(gameID, was, hash string) (bool, error) {
+	res, err := s.db.Exec(`UPDATE games SET last_manifest_hash = ? WHERE id = ? AND COALESCE(last_manifest_hash, '') = ?`,
+		hash, gameID, was)
+	if err != nil {
+		return false, fmt.Errorf("set last manifest hash %s: %w", gameID, err)
+	}
+	n, err := res.RowsAffected()
+	return err == nil && n > 0, err
+}
+
 // RenameGameIf renames a game only while it still has the name from: a name
 // given automatically must never replace one the person typed in between.
 // Reports whether it was renamed.
diff --git a/internal/store/last_manifest_hash_test.go b/internal/store/last_manifest_hash_test.go
new file mode 100644
index 00000000..39aabb5f
--- /dev/null
+++ b/internal/store/last_manifest_hash_test.go
@@ -0,0 +1,32 @@
+package store
+
+import "testing"
+
+// A hash taken before a snapshot is recorded only while nothing newer was
+// recorded meanwhile: a newly tracked game's first snapshot, running in the
+// background, finished after one taken by hand and replaced its newer value.
+func TestSetLastManifestHashIf_DoesNotReplaceANewerValue(t *testing.T) {
+	s := openTestStore(t)
+	if err := s.CreateGame(Game{ID: "g", Name: "G", SavePath: t.TempDir()}); err != nil {
+		t.Fatal(err)
+	}
+
+	if ok, err := s.SetLastManifestHashIf("g", "", "first"); err != nil || !ok {
+		t.Fatalf("recording over nothing: ok=%v err=%v", ok, err)
+	}
+	// Another snapshot records a newer value…
+	if err := s.SetLastManifestHash("g", "newer"); err != nil {
+		t.Fatal(err)
+	}
+	// …and the one that started before it must not replace it.
+	if ok, err := s.SetLastManifestHashIf("g", "first", "older"); err != nil || ok {
+		t.Fatalf("an older value replaced a newer one: ok=%v err=%v", ok, err)
+	}
+	g, err := s.GetGame("g")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if g.LastManifestHash != "newer" {
+		t.Errorf("recorded %q, want %q", g.LastManifestHash, "newer")
+	}
+}