syncengine: count any edit after the last sync as a change

e2e/multiroot_scenarios_test.go

@@ -481,12 +481,12 @@ func TestMultiRootScenario_ALateReportDoesNotHideADivergence(t *testing.T) {
 				t.Fatal(err)
 			}
 			// With no base, the edits are judged against when the two were
-			// last confirmed the same, and only one made more than the 2 s
-			// clock-skew allowance after it counts as new (DetectConflict). A
-			// refresh the setup's own reports started can stamp that time
-			// after SettleSync returns; edits made in the next two seconds
-			// then read as neither side having changed, and the test failed on
-			// its own timing (Windows CI: stamped 1.8 s before the edits).
+			// last confirmed the same. A refresh the setup's own reports
+			// started can stamp that time after SettleSync returns; waiting
+			// for it to settle leaves the report delivered below as the only
+			// thing that moves it. (While edits within 2 s of that time read
+			// as no change, this test failed on its own timing: Windows CI
+			// stamped 1.8 s before the edits.)
 			waitPastLastSynced(t, a, b.NodeID())
 
 			writeIn(t, aConfig, "settings.ini", "A-version")
@@ -522,10 +522,65 @@ func TestMultiRootScenario_ALateReportDoesNotHideADivergence(t *testing.T) {
 	}
 }
 
+// The same divergence with the edits made straight after the two devices were
+// confirmed the same. A location with no merge base is judged against that
+// time, and a 2 s clock-skew allowance was added to it: edits on both devices
+// within 2 s read as neither having changed, and one replaced the other with
+// no conflict raised. Any edit after that time now counts as a change, so two
+// of them are a conflict (#57).
+func TestMultiRootScenario_EditsRightAfterASyncStillConflict(t *testing.T) {
+	a, b, gameID, aConfig, bConfig := twoLocationPair(t, "LocEditsAfterStamp", map[string]string{
+		"settings.ini": "shared v1",
+	})
+	pauseAutoSync(t, a, b, gameID)
+
+	if !testutil.WaitFor(45*time.Second, func() bool {
+		return a.Daemon.Store.GetAgreedHashForRoot(gameID, b.NodeID(), "config") != ""
+	}) {
+		t.Fatal("setup: the two devices never recorded agreeing on the location")
+	}
+	testutil.SettleSync(t, gameID, a, b)
+	if err := a.Daemon.Store.SetAgreedHashForRoot(gameID, b.NodeID(), "config", ""); err != nil {
+		t.Fatal(err)
+	}
+
+	var peer syncengine.Peer
+	for _, p := range a.Daemon.P2P.OnlinePeers() {
+		if p.ID == b.NodeID() {
+			peer = p
+		}
+	}
+	if peer.ID == "" {
+		t.Fatal("B is not online for A")
+	}
+	// Stamps the two as synced now; the edits follow within 2 s.
+	// Finding them the same also records a merge base for the location again,
+	// which would judge the edits by content; cleared, as a lost report leaves
+	// it, so they are judged by the time.
+	a.Daemon.P2P.Sync.RefreshLineage(context.Background(), gameID, peer)
+	if err := a.Daemon.Store.SetAgreedHashForRoot(gameID, b.NodeID(), "config", ""); err != nil {
+		t.Fatal(err)
+	}
+	writeIn(t, aConfig, "settings.ini", "A-version")
+	writeIn(t, bConfig, "settings.ini", "B-version")
+
+	a.API(http.MethodPost, "/api/games/"+gameID+"/sync", nil, nil)
+	if !testutil.WaitFor(45*time.Second, func() bool {
+		return len(a.Daemon.P2P.Sync.ActiveRootConflicts()) > 0
+	}) {
+		t.Fatalf("both devices edited the location and no conflict was raised: A=%q B=%q",
+			readIn(aConfig, "settings.ini"), readIn(bConfig, "settings.ini"))
+	}
+	if got := readIn(aConfig, "settings.ini"); got != "A-version" {
+		t.Errorf("A's edit was replaced with %q", got)
+	}
+	if got := readIn(bConfig, "settings.ini"); got != "B-version" {
+		t.Errorf("B's edit was replaced with %q", got)
+	}
+}
+
 // waitPastLastSynced waits until a's "last synced" time for the peer has
-// stopped moving and lies further back than the clock-skew allowance a
-// location without a merge base is judged with (syncengine's
-// clockSkewToleranceMs, 2 s), so an edit made after it counts as new.
+// stopped moving and lies more than a few seconds back.
 func waitPastLastSynced(t *testing.T, a *testutil.TestDaemon, peerID string) {
 	t.Helper()
 	const past = 3 * time.Second // the 2 s allowance, and a margin

internal/p2p/syncengine/decision.go

@@ -12,10 +12,6 @@ import (
 	"github.com/opensave/opensave/internal/delta"
 )
 
-// clockSkewToleranceMs matches the JS engine's 2-second allowance when
-// comparing manifest mtimes against the last sync time.
-const clockSkewToleranceMs = 2000
-
 // DetectConflict reports whether both sides changed since the last sync —
 // the only situation that needs user intervention.
 //
@@ -24,7 +20,7 @@ const clockSkewToleranceMs = 2000
 //   - Never synced before (lastSyncTimeMs == 0): conflict only if BOTH
 //     sides already have files (two pre-existing, diverged save states).
 //   - Otherwise: conflict when both manifests' latest mtimes are newer
-//     than lastSyncTimeMs + 2s of skew tolerance.
+//     than lastSyncTimeMs.
 func DetectConflict(local, remote delta.Manifest, lastSyncTimeMs int64, agreedHash string) bool {
 	if local.ManifestHash() == remote.ManifestHash() {
 		return false
@@ -50,8 +46,19 @@ func DetectConflict(local, remote delta.Manifest, lastSyncTimeMs int64, agreedHa
 	if lastSyncTimeMs == 0 {
 		return len(local.Files) > 0 && len(remote.Files) > 0
 	}
-	localModified := int64(local.LatestMtime) > lastSyncTimeMs+clockSkewToleranceMs
-	remoteModified := int64(remote.LatestMtime) > lastSyncTimeMs+clockSkewToleranceMs
+	// Any edit after the last sync is a change. A conflict still needs both
+	// sides changed: one side alone wins as it always has. There used to be a
+	// 2 s clock-skew allowance after the sync, and edits made on both devices
+	// within it read as neither having changed, so one replaced the other
+	// with no conflict raised. An extra prompt right after a sync is the
+	// better failure.
+	//
+	// The allowance is not moved before the sync instead: a pulled file keeps
+	// the sender's mtime, which in a quick exchange is just before the stamp,
+	// and counting that copy as a change made the next edit on the other
+	// device a conflict (TestUntrackPropagatesAndRecovers).
+	localModified := int64(local.LatestMtime) > lastSyncTimeMs
+	remoteModified := int64(remote.LatestMtime) > lastSyncTimeMs
 	return localModified && remoteModified
 }
 

internal/p2p/syncengine/decision_test.go

@@ -70,9 +70,26 @@ func TestDetectConflict(t *testing.T) {
 			want:       false,
 		},
 		{
-			name: "clock skew within 2s tolerance does not count as modified",
-			// local mtime is 1.5s past lastSync — inside the tolerance window.
+			name: "both edited within 2s after last sync -> conflict",
+			// Both edits inside the tolerance window. They used to read as
+			// neither side having changed, and one replaced the other.
 			local:      manifest(11500, map[string]delta.FileEntry{"a": fileEntry("h1", 11500)}),
+			remote:     manifest(11800, map[string]delta.FileEntry{"a": fileEntry("h2", 11800)}),
+			lastSyncMs: 10000,
+			want:       true,
+		},
+		{
+			name:       "edited within 2s after last sync on one side only -> no conflict",
+			local:      manifest(11500, map[string]delta.FileEntry{"a": fileEntry("h1", 11500)}),
+			remote:     manifest(5000, map[string]delta.FileEntry{"a": fileEntry("h2", 5000)}),
+			lastSyncMs: 10000,
+			want:       false,
+		},
+		{
+			name: "a copy written just before last sync is no change",
+			// A pulled file keeps the sender's mtime, which in a quick
+			// exchange is just before the sync was stamped.
+			local:      manifest(9500, map[string]delta.FileEntry{"a": fileEntry("h1", 9500)}),
 			remote:     manifest(25000, map[string]delta.FileEntry{"a": fileEntry("h2", 25000)}),
 			lastSyncMs: 10000,
 			want:       false,