diff --git a/e2e/baseline_test.go b/e2e/baseline_test.go new file mode 100644 index 00000000..f7482c58 --- /dev/null +++ b/e2e/baseline_test.go @@ -0,0 +1,118 @@ +package e2e + +import ( + "net/http" + "os" + "path/filepath" + "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)) + } +} + +// 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.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/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/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/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/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..5b20e945 --- /dev/null +++ b/internal/daemon/baseline.go @@ -0,0 +1,172 @@ +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.) +// +// 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, was, hashErr := "", "", error(nil) + if game, err := d.Store.GetGame(gameID); err == nil { + was = game.LastManifestHash + 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.SetLastManifestHashIf(snap.GameID, was, 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) +} + +// 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. +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"` 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") + } +} 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