fix(sync): prefer agreed base over misleading file mtimes

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

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"))
+	}
+}

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)
+	}
+}

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:

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)
 	}
 }

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) {