diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d5300ce..b91fe528 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,17 @@ All notable changes to OpenSave are documented here. This project adheres to [Semantic Versioning](https://semver.org/). +## [Unreleased] + +### Fixed + +- **A touched copy of the last agreed save no longer overwrites the only + real edit.** When one device still holds the confirmed common version and + the other has changed, the changed side wins even if the unchanged files + have a later timestamp. Named extra save locations use that same rule. + A missing or stale agreed base is not given a new meaning. This does not + implement the version vectors in #29 / #30. See issue #41. + ## [2.4.1] — 2026-10-02 A security update: please install it on every device. A paired device could diff --git a/e2e/agreed_base_mtime_test.go b/e2e/agreed_base_mtime_test.go new file mode 100644 index 00000000..45e17096 --- /dev/null +++ b/e2e/agreed_base_mtime_test.go @@ -0,0 +1,87 @@ +package e2e + +import ( + "net/http" + "os" + "path/filepath" + "testing" + "time" + + "github.com/opensave/opensave/testutil" +) + +func touchOlder(t *testing.T, path string) { + t.Helper() + old := time.Now().Add(-48 * time.Hour) + if err := os.Chtimes(path, old, old); err != nil { + t.Fatal(err) + } +} + +func touchNewer(t *testing.T, path string) { + t.Helper() + now := time.Now().Add(2 * time.Hour) + if err := os.Chtimes(path, now, now); err != nil { + t.Fatal(err) + } +} + +// The side that still holds the agreed base must not win just because its +// file was touched more recently. Covers the primary folder and one named +// extra location, through real daemons rather than Compute alone. +func TestAgreedBaseBeatsNewerMtimeOnPrimaryAndExtraRoot(t *testing.T) { + a, b, gameID, aConfig, bConfig := twoLocationPair(t, "BaseMtime", map[string]string{ + "settings.ini": "base-config", + }) + pauseAutoSync(t, a, b, gameID) + + writeIn(t, a.SaveDir, "save.sav", "progressed-primary") + touchOlder(t, filepath.Join(a.SaveDir, "save.sav")) + touchNewer(t, filepath.Join(b.SaveDir, "save.sav")) + + writeIn(t, aConfig, "settings.ini", "progressed-config") + touchOlder(t, filepath.Join(aConfig, "settings.ini")) + touchNewer(t, filepath.Join(bConfig, "settings.ini")) + + if !testutil.WaitFor(45*time.Second, func() bool { + a.API(http.MethodPost, "/api/games/"+gameID+"/sync", nil, nil) + return readIn(b.SaveDir, "save.sav") == "progressed-primary" && + readIn(bConfig, "settings.ini") == "progressed-config" && + readIn(a.SaveDir, "save.sav") == "progressed-primary" && + readIn(aConfig, "settings.ini") == "progressed-config" + }) { + t.Fatalf("agreed base overwrote progress: a primary=%q config=%q b primary=%q config=%q", + readIn(a.SaveDir, "save.sav"), readIn(aConfig, "settings.ini"), + readIn(b.SaveDir, "save.sav"), readIn(bConfig, "settings.ini")) + } +} + +// The other direction: the peer holds the only edit, with an older mtime, +// and this device holds the agreed base with a later timestamp. Sync must +// pull, not push the base over the edit. Primary folder and the named root. +func TestAgreedBasePullsOlderMtimeProgressOnPrimaryAndExtraRoot(t *testing.T) { + a, b, gameID, aConfig, bConfig := twoLocationPair(t, "BaseMtimePull", map[string]string{ + "settings.ini": "base-config", + }) + pauseAutoSync(t, a, b, gameID) + + writeIn(t, b.SaveDir, "save.sav", "progressed-primary") + touchOlder(t, filepath.Join(b.SaveDir, "save.sav")) + touchNewer(t, filepath.Join(a.SaveDir, "save.sav")) + + writeIn(t, bConfig, "settings.ini", "progressed-config") + touchOlder(t, filepath.Join(bConfig, "settings.ini")) + touchNewer(t, filepath.Join(aConfig, "settings.ini")) + + if !testutil.WaitFor(45*time.Second, func() bool { + a.API(http.MethodPost, "/api/games/"+gameID+"/sync", nil, nil) + return readIn(a.SaveDir, "save.sav") == "progressed-primary" && + readIn(aConfig, "settings.ini") == "progressed-config" && + readIn(b.SaveDir, "save.sav") == "progressed-primary" && + readIn(bConfig, "settings.ini") == "progressed-config" + }) { + t.Fatalf("local base overwrote remote progress: a primary=%q config=%q b primary=%q config=%q", + readIn(a.SaveDir, "save.sav"), readIn(aConfig, "settings.ini"), + readIn(b.SaveDir, "save.sav"), readIn(bConfig, "settings.ini")) + } +} diff --git a/internal/p2p/syncengine/base_first_rollback_test.go b/internal/p2p/syncengine/base_first_rollback_test.go new file mode 100644 index 00000000..b8a06fba --- /dev/null +++ b/internal/p2p/syncengine/base_first_rollback_test.go @@ -0,0 +1,133 @@ +package syncengine + +import ( + "testing" + + "github.com/opensave/opensave/internal/delta" +) + +// A save that progressed after the agreed base must not be overwritten by a +// byte-for-byte copy of that base whose file was touched or restored later. +func TestBaseFirstPreventsMtimeRollback(t *testing.T) { + old := delta.Manifest{Files: map[string]delta.FileEntry{ + "profile.sav": {Hash: "same", MtimeMs: 3000}, + "slot.sav": {Hash: "older-progress", MtimeMs: 3000}, + }} + newer := delta.Manifest{Files: map[string]delta.FileEntry{ + "profile.sav": {Hash: "same", MtimeMs: 1000}, + "slot.sav": {Hash: "newer-progress", MtimeMs: 1000}, + }} + base := old.ManifestHash() + lineage := map[string]struct{}{"profile.sav": {}, "slot.sav": {}} + tests := []struct { + name string + local, remote delta.Manifest + wantPush, wantPull bool + }{ + {"local changed, remote base has later mtime", newer, old, true, false}, + {"remote changed, local base has later mtime", old, newer, false, true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if DetectConflict(tc.local, tc.remote, 0, base) { + t.Fatal("one-sided post-base save update must not be considered a conflict") + } + d := ComputeWithBase(tc.local, tc.remote, lineage, nil, base) + gotPush := len(d.FilesToPush) == 1 && d.FilesToPush[0] == "slot.sav" + gotPull := len(d.FilesToPull) == 1 && d.FilesToPull[0] == "slot.sav" + if gotPush != tc.wantPush || gotPull != tc.wantPull || len(d.FilesToDeleteLocally) != 0 || len(d.FilesToDeleteOnPeer) != 0 { + t.Fatalf("decision = %+v, want push=%v pull=%v and no deletions", d, tc.wantPush, tc.wantPull) + } + }) + } +} + +func TestBaseFirstRetainsLegacyMtimeWithoutBase(t *testing.T) { + old := delta.Manifest{Files: map[string]delta.FileEntry{ + "slot.sav": {Hash: "old", MtimeMs: 3000}, + }} + newer := delta.Manifest{Files: map[string]delta.FileEntry{ + "slot.sav": {Hash: "new", MtimeMs: 1000}, + }} + d := ComputeWithBase(newer, old, map[string]struct{}{"slot.sav": {}}, nil, "") + if len(d.FilesToPull) != 1 || d.FilesToPull[0] != "slot.sav" { + t.Fatalf("without an agreed base the newer mtime still wins: %+v", d) + } +} + +func TestBaseFirstNoChangeForIdenticalContents(t *testing.T) { + left := delta.Manifest{Files: map[string]delta.FileEntry{ + "slot.sav": {Hash: "same", MtimeMs: 1}, + }} + right := delta.Manifest{Files: map[string]delta.FileEntry{ + "slot.sav": {Hash: "same", MtimeMs: 9999}, + }} + d := ComputeWithBase(left, right, map[string]struct{}{"slot.sav": {}}, nil, left.ManifestHash()) + if d.HasChanges() { + t.Fatalf("identical contents must not transfer: %+v", d) + } +} + +func TestBaseFirstBothChangedStillConflict(t *testing.T) { + base := delta.Manifest{Files: map[string]delta.FileEntry{ + "slot.sav": {Hash: "old", MtimeMs: 100}, + }} + left := delta.Manifest{Files: map[string]delta.FileEntry{ + "slot.sav": {Hash: "local-new", MtimeMs: 1000}, + }} + right := delta.Manifest{Files: map[string]delta.FileEntry{ + "slot.sav": {Hash: "remote-new", MtimeMs: 2000}, + }} + if !DetectConflict(left, right, 0, base.ManifestHash()) { + t.Fatal("both sides changed relative to base: must require conflict resolution") + } +} + +// A deliberate local deletion of a file that is still in the lineage must +// propagate, not be undone because the peer's copy of the agreed base has a +// later mtime. The base-first file rule does not apply: the file is absent +// here. +func TestBaseFirstDoesNotResurrectADeletedFile(t *testing.T) { + base := delta.Manifest{Files: map[string]delta.FileEntry{ + "slot.sav": {Hash: "old", MtimeMs: 1000}, + }} + remote := delta.Manifest{Files: map[string]delta.FileEntry{ + "slot.sav": {Hash: "old", MtimeMs: 9000}, + }} + local := delta.Manifest{Files: map[string]delta.FileEntry{}} + d := ComputeWithBase(local, remote, map[string]struct{}{"slot.sav": {}}, nil, base.ManifestHash()) + if len(d.FilesToPull) != 0 { + t.Fatalf("deleted file was pulled back: %+v", d) + } + if len(d.FilesToDeleteOnPeer) != 1 || d.FilesToDeleteOnPeer[0] != "slot.sav" { + t.Fatalf("deletion was not propagated: %+v", d) + } +} + +// A peer's tracked deletion must not be undone by pushing the +// unchanged local copy of the agreed file, even with a later timestamp. +func TestBaseFirstDoesNotResurrectPeerDeletedFile(t *testing.T) { + base := delta.Manifest{Files: map[string]delta.FileEntry{ + "slot.sav": {Hash: "old", MtimeMs: 1000}, + }} + local := delta.Manifest{Files: map[string]delta.FileEntry{ + "slot.sav": {Hash: "old", MtimeMs: 9000}, + }} + remote := delta.Manifest{Files: map[string]delta.FileEntry{}} + d := ComputeWithBase(local, remote, + map[string]struct{}{"slot.sav": {}}, nil, base.ManifestHash()) + if len(d.FilesToPush) != 0 || len(d.FilesToDeleteLocally) != 1 || + d.FilesToDeleteLocally[0] != "slot.sav" { + t.Fatalf("remote deletion was undone or ignored: %+v", d) + } +} + +// With no matching agreed state, legacy mtime behaviour remains a limit. +func TestStaleAgreedBaseDoesNotOverrideMtime(t *testing.T) { + local := tieManifest("LOCAL", 2000) + remote := tieManifest("REMOTE", 1000) + d := ComputeWithBase(local, remote, map[string]struct{}{"save.dat": {}}, nil, "stale-base-neither-side-holds") + if len(d.FilesToPush) != 1 || len(d.FilesToPull) != 0 { + t.Fatalf("a stale base changed the mtime decision: %+v", d) + } +} diff --git a/internal/p2p/syncengine/decision.go b/internal/p2p/syncengine/decision.go index c981f8ef..1c4afa2e 100644 --- a/internal/p2p/syncengine/decision.go +++ b/internal/p2p/syncengine/decision.go @@ -148,8 +148,9 @@ func Compute(local, remote delta.Manifest, lastSyncedFiles, lastSyncedDirs map[s } // ComputeWithBase is Compute told which manifest hash both sides last agreed -// on, so an mtime tie can be broken by content rather than by direction. See -// the tie case below for why that matters. +// on. If one whole manifest is still that hash, that side did not change, +// whatever its filesystem mtimes say. An mtime tie still uses the base when +// the stamps are equal. A base that matches neither side is not used. func ComputeWithBase(local, remote delta.Manifest, lastSyncedFiles, lastSyncedDirs map[string]struct{}, baseHash string) Decision { return ComputeWithDeletions(local, remote, lastSyncedFiles, lastSyncedDirs, baseHash, nil) } @@ -234,6 +235,13 @@ func ComputeWithDeletions(local, remote delta.Manifest, lastSyncedFiles, lastSyn case hasLocal && hasRemote && localFile.Hash != remoteFile.Hash: switch { + case baseHash != "" && localHash == baseHash && remoteHash != baseHash: + // This whole manifest is the confirmed common version. The + // peer is the only side that changed. A restored or touched + // copy of the base can have a later mtime; that is not an edit. + d.FilesToPull = append(d.FilesToPull, relPath) + case baseHash != "" && remoteHash == baseHash && localHash != baseHash: + d.FilesToPush = append(d.FilesToPush, relPath) case remoteFile.MtimeMs > localFile.MtimeMs: d.FilesToPull = append(d.FilesToPull, relPath) case localFile.MtimeMs > remoteFile.MtimeMs: diff --git a/internal/p2p/syncengine/mtime_tie_test.go b/internal/p2p/syncengine/mtime_tie_test.go index a22606f5..e6b66d76 100644 --- a/internal/p2p/syncengine/mtime_tie_test.go +++ b/internal/p2p/syncengine/mtime_tie_test.go @@ -89,18 +89,25 @@ func TestAnMtimeTieWithAnUnrelatedBaseStillPulls(t *testing.T) { } } -// Ordinary differing timestamps are untouched by any of this. -func TestANewerSideStillWinsRegardlessOfTheBase(t *testing.T) { - const base = delta.Milli(1_700_000_000_000) +// An agreed common version beats the filesystem clock. This used to expect +// the newer mtime to win even when that side was exactly the agreed base. +// That overwrote the only real edit. The corrected precedence is explicit. +func TestAgreedBaseOverridesNewerMtime(t *testing.T) { + const reference = delta.Milli(1_700_000_000_000) lineage := map[string]struct{}{"save.dat": {}} - local := tieManifest("LOCAL-NEWER", base+50) - remote := tieManifest("REMOTE", base) - // A base saying the LOCAL side is unchanged must not override a clear - // timestamp: the stamps are what decide when they differ. - d := ComputeWithBase(local, remote, lineage, nil, local.ManifestHash()) - if len(d.FilesToPush) != 1 { - t.Errorf("a clearly newer local file was not pushed: push=%v pull=%v", - d.FilesToPush, d.FilesToPull) + local := tieManifest("BASE", reference+50) + remote := tieManifest("REMOTE-EDIT", reference) + base := local.ManifestHash() + if DetectConflict(local, remote, 0, base) { + t.Fatal("one-sided update must reach Compute without a conflict") + } + d := ComputeWithBase(local, remote, lineage, nil, base) + if len(d.FilesToPush) > 0 { + t.Errorf("old agreed-base data must not be pushed: push=%v", d.FilesToPush) + } + if len(d.FilesToPull) != 1 || d.FilesToPull[0] != "save.dat" { + t.Errorf("changed remote must be pulled despite older mtime: pull=%v push=%v", + d.FilesToPull, d.FilesToPush) } } diff --git a/internal/p2p/syncengine/multiroot.go b/internal/p2p/syncengine/multiroot.go index c763a459..6810e402 100644 --- a/internal/p2p/syncengine/multiroot.go +++ b/internal/p2p/syncengine/multiroot.go @@ -132,25 +132,9 @@ func (e *Engine) syncOneRoot(ctx context.Context, gameID string, game store.Game lineageFiles = filterLineage(lineageFiles, rules) lineageDirs = filterLineage(lineageDirs, rules) } - decision := Compute(local, remote, lineageFiles, lineageDirs) - - if !decision.HasChanges() { - e.persistRootLineage(gameID, peer.ID, sr.root.Name, local, remote) - _ = e.Store.SetAgreedHashForRoot(gameID, peer.ID, sr.root.Name, local.RootHash(delta.PrimaryRoot)) - return nil - } - - // Divergence is judged with the SAME detector the primary location uses, - // against this location's own merge base — so one folder disagreeing says - // nothing about the others. - // - // This was originally a hand-rolled check for "a pull and a push in the - // same decision", which is not the same question at all: when both devices - // edit the same file, the classifier can resolve it as a pull alone, the - // check never fires, and this side's version is silently overwritten. The - // point of detecting a conflict is that nobody's work disappears, and a - // second detector that is nearly right is worse than none, because it - // looks like the work has been done. + // The same base the conflict check below uses. Compute used to be called + // with an empty base, so a named location ranked files by mtime even when + // one side was still the confirmed version. base := e.Store.GetAgreedHashForRoot(gameID, peer.ID, sr.root.Name) // A push is proven once the peer is seen holding exactly what was handed // over, whatever became of its report — the repair the main folder makes @@ -180,6 +164,12 @@ func (e *Engine) syncOneRoot(ctx context.Context, gameID string, game store.Game base = remote.RootHash(delta.PrimaryRoot) } } + decision := ComputeWithBase(local, remote, lineageFiles, lineageDirs, base) + if !decision.HasChanges() { + e.persistRootLineage(gameID, peer.ID, sr.root.Name, local, remote) + _ = e.Store.SetAgreedHashForRoot(gameID, peer.ID, sr.root.Name, local.RootHash(delta.PrimaryRoot)) + return nil + } judged := e.unchangedButForPeerDeletions(gameID, sr.root.Name, local, base) if DetectConflict(judged, remote, e.lastSyncTimeMs(peer.ID), base) && !OnlyBehind(judged, remote, lineageFiles) {