From 427146442a60f4746b994a20979366eb93b9189d Mon Sep 17 00:00:00 2001 From: ChakraFusion <112588066+ChakraFusion@users.noreply.github.com> Date: Thu, 15 Oct 2026 17:59:47 +0200 Subject: [PATCH] syncengine: count any edit after the last sync as a change Without a merge base, DetectConflict compares each side's latest mtime with the last-synced time. A 2 s clock-skew allowance was added to that time, so edits made on both devices within 2 s after a sync read as neither side having changed, and one replaced the other with no conflict raised. Any edit after the last sync now counts as a change. A conflict still needs both sides changed; one side alone wins as before. With a base recorded nothing changes. The allowance is dropped rather than moved before the sync: a pulled file keeps the sender's mtime, just before the stamp in a quick exchange, and counting that copy as a change made the other device's next edit a conflict (TestUntrackPropagatesAndRecovers and TestSwitch_TheSameGameUnderDifferentIDsSyncs fail that way). TestMultiRootScenario_EditsRightAfterASyncStillConflict stamps the two devices as synced, clears the location's base and edits both sides at once; it fails 3/3 without this (A="B-version" B="B-version"). Co-Authored-By: Claude Opus 5.5 --- e2e/multiroot_scenarios_test.go | 73 +++++++++++++++++++++--- internal/p2p/syncengine/decision.go | 21 ++++--- internal/p2p/syncengine/decision_test.go | 21 ++++++- 3 files changed, 97 insertions(+), 18 deletions(-) diff --git a/e2e/multiroot_scenarios_test.go b/e2e/multiroot_scenarios_test.go index 16615064..c6c8d95e 100644 --- a/e2e/multiroot_scenarios_test.go +++ b/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 diff --git a/internal/p2p/syncengine/decision.go b/internal/p2p/syncengine/decision.go index 1c4afa2e..1a1da80b 100644 --- a/internal/p2p/syncengine/decision.go +++ b/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 } diff --git a/internal/p2p/syncengine/decision_test.go b/internal/p2p/syncengine/decision_test.go index d30ac8aa..4ffeef95 100644 --- a/internal/p2p/syncengine/decision_test.go +++ b/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,