From a91114d6d1a1e71b15c111fda0dca5a398e19a09 Mon Sep 17 00:00:00 2001 From: ChakraFusion <112588066+ChakraFusion@users.noreply.github.com> Date: Tue, 13 Oct 2026 18:13:46 +0200 Subject: [PATCH 1/3] sync: a deletion in an extra location survives a lost sync-complete report The report that a peer took a location is also what records which files both sides hold there. When it was lost - it times out on a loaded machine ("could not tell ... that ... is sync-complete: context deadline exceeded") - a file deleted here afterwards read as one the peer had and this device never did, and the next sync fetched it back: the deletion undone. Seen on CI as TestMultiRootScenario_DeletionInAnExtraLocationPropagates failing once on Windows (Liquid-co/OpenSave#23); reproduced deterministically by TestMultiRootScenario_ADeletionSurvivesALostReport. - The existing repair - a push is proven once the peer holds exactly what was handed over - now runs before anything is classified, and restores the lineage too: both held exactly the pushed state, so its files are what was shared. - Extra locations use the deletions this device recorded, as the main folder does (ComputeWithDeletions): a recorded deletion does not depend on the lineage, and is acted on only while the peer's copy is still exactly what was deleted. Co-Authored-By: Claude Opus 5.5 --- e2e/multiroot_scenarios_test.go | 49 ++++++++++++++++++++++++++ internal/p2p/syncengine/multiroot.go | 51 ++++++++++++++++++++-------- 2 files changed, 85 insertions(+), 15 deletions(-) diff --git a/e2e/multiroot_scenarios_test.go b/e2e/multiroot_scenarios_test.go index 16615064..6ef31b7c 100644 --- a/e2e/multiroot_scenarios_test.go +++ b/e2e/multiroot_scenarios_test.go @@ -966,3 +966,52 @@ func TestIgnore_ExcludedFilesInExtraLocationsAreStillSnapshotted(t *testing.T) { "rollback empties the folder first, so anything missing from the snapshot is destroyed", got) } } + +// The same deletion when the peer's report that it took the location never +// arrived: on a loaded machine it can time out ("could not tell ... that ... +// is sync-complete: context deadline exceeded"), and then this device has no +// record that the peer holds those files. It must still not take the deleted +// file back from the peer as one it never had. +func TestMultiRootScenario_ADeletionSurvivesALostReport(t *testing.T) { + a, b, gameID, aConfig, bConfig := twoLocationPair(t, "LocLostReport", map[string]string{ + "settings.ini": "fullscreen=1", + "keys.cfg": "jump=space", + }) + pauseAutoSync(t, a, b, gameID) + + // Where A stands when the report is lost: it handed the location over and + // recorded that, and nothing since says the peer took it. + pushed, err := delta.BuildManifest(aConfig) + if err != nil { + t.Fatal(err) + } + store := a.Daemon.Store + if err := store.SetSyncStateForRoot(gameID, b.NodeID(), "config", nil, nil); err != nil { + t.Fatal(err) + } + if err := store.SetAgreedHashForRoot(gameID, b.NodeID(), "config", ""); err != nil { + t.Fatal(err) + } + if err := store.SetPushedHashForRoot(gameID, b.NodeID(), "config", pushed.RootHash(delta.PrimaryRoot)); err != nil { + t.Fatal(err) + } + + if err := os.Remove(filepath.Join(aConfig, "keys.cfg")); err != nil { + t.Fatal(err) + } + time.Sleep(syncSettleWindow) + a.API(http.MethodPost, "/api/games/"+gameID+"/sync", nil, nil) + + if !testutil.WaitFor(45*time.Second, func() bool { + return readIn(bConfig, "keys.cfg") == "" + }) { + t.Fatalf("the deletion never reached the peer: keys.cfg there=%q, here=%q", + readIn(bConfig, "keys.cfg"), readIn(aConfig, "keys.cfg")) + } + if got := readIn(aConfig, "keys.cfg"); got != "" { + t.Errorf("the deleted file came back here: %q", got) + } + if got := readIn(bConfig, "settings.ini"); got != "fullscreen=1" { + t.Errorf("an untouched file on the peer changed: %q", got) + } +} diff --git a/internal/p2p/syncengine/multiroot.go b/internal/p2p/syncengine/multiroot.go index 6810e402..04edf595 100644 --- a/internal/p2p/syncengine/multiroot.go +++ b/internal/p2p/syncengine/multiroot.go @@ -119,6 +119,29 @@ func (e *Engine) syncOneRoot(ctx context.Context, gameID string, game store.Game remote = filterManifest(remote, rules) } + // 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 + // (see SyncWithPeer), and for the same reason. The push was recorded below + // and nothing ever read it, so a lost report left this location's base + // behind both devices, and the next edit on one of them read as both + // having moved: a conflict over a change only one device made. + // + // The lineage is repaired with it, before anything is classified: the + // report is also what records which files both sides hold. Without that, + // a file deleted here after a lost report read as one the peer had and + // this device never did, and it was fetched back — the deletion undone + // (TestMultiRootScenario_ADeletionSurvivesALostReport). Both held exactly + // the pushed state, so its files are the lineage. + // + // Both hashes are of the filtered view, which is what the push recorded. + if pushed := e.Store.GetPushedHashForRoot(gameID, peer.ID, sr.root.Name); pushed != "" { + if remoteHash := remote.RootHash(delta.PrimaryRoot); remoteHash == pushed && + e.Store.GetAgreedHashForRoot(gameID, peer.ID, sr.root.Name) != remoteHash { + e.persistRootLineage(gameID, peer.ID, sr.root.Name, remote, remote) + _ = e.Store.SetAgreedHashForRoot(gameID, peer.ID, sr.root.Name, remoteHash) + } + } + fileList, dirList, err := e.Store.GetSyncStateForRoot(gameID, peer.ID, sr.root.Name) if err != nil { return err @@ -132,24 +155,22 @@ func (e *Engine) syncOneRoot(ctx context.Context, gameID string, game store.Game lineageFiles = filterLineage(lineageFiles, rules) lineageDirs = filterLineage(lineageDirs, rules) } + // And the deletions this device recorded in this location, as the main + // folder uses them (see SyncWithPeer): a recorded deletion does not depend + // on the lineage surviving, and is acted on only while the peer's copy is + // still exactly what was deleted. + deleted := e.recordedDeletions(gameID, sr.root.Name) + if !rules.Empty() { + for path := range deleted { + if rules.Match(path) { + delete(deleted, path) + } + } + } // 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 - // (see SyncWithPeer), and for the same reason. The push was recorded below - // and nothing ever read it, so a lost report left this location's base - // behind both devices, and the next edit on one of them read as both - // having moved: a conflict over a change only one device made. - // - // Both hashes are of the filtered view, which is what the push recorded. - if pushed := e.Store.GetPushedHashForRoot(gameID, peer.ID, sr.root.Name); pushed != "" { - if remoteHash := remote.RootHash(delta.PrimaryRoot); base != remoteHash && remoteHash == pushed { - _ = e.Store.SetAgreedHashForRoot(gameID, peer.ID, sr.root.Name, remoteHash) - base = remoteHash - } - } // A base recorded before the rules existed was hashed over everything, so // it can equal neither filtered side — and a base matching neither reads // as both having moved, which is a conflict on the first sync after anyone @@ -164,7 +185,7 @@ func (e *Engine) syncOneRoot(ctx context.Context, gameID string, game store.Game base = remote.RootHash(delta.PrimaryRoot) } } - decision := ComputeWithBase(local, remote, lineageFiles, lineageDirs, base) + decision := ComputeWithDeletions(local, remote, lineageFiles, lineageDirs, base, deleted) 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)) From aeafe4814988225c89ca2cdc26629a6c5fa5f2e8 Mon Sep 17 00:00:00 2001 From: ChakraFusion <112588066+ChakraFusion@users.noreply.github.com> Date: Tue, 13 Oct 2026 19:04:25 +0200 Subject: [PATCH 2/3] sync: an extra location's record keeps deletions in flight, as the main folder's does The refresh a peer's sync-complete report starts runs in the background, and rebuilt the record of which files both sides hold in an extra location from this device's files as they were when it ran. A file deleted here in the moment before - a loaded machine is enough - dropped out of the record, read as one the peer had and this device never did, and the next sync fetched it back. The second way TestMultiRootScenario_DeletionInAnExtraLocationPropagates failed on Windows (Liquid-co/OpenSave#23, and again with the previous commit, without a lost report); reproduced deterministically by TestMultiRootScenario_ADeletionSurvivesALateRefresh. persistRootLineage now does what persistLineage does for the main folder: keeps previously recorded paths that one side still holds (deletions in flight, keepPendingDeletions), and keeps excluded paths out. Co-Authored-By: Claude Opus 5.5 --- e2e/multiroot_scenarios_test.go | 50 ++++++++++++++++++++++++++++ internal/p2p/syncengine/multiroot.go | 16 +++++++++ 2 files changed, 66 insertions(+) diff --git a/e2e/multiroot_scenarios_test.go b/e2e/multiroot_scenarios_test.go index 6ef31b7c..edf82e55 100644 --- a/e2e/multiroot_scenarios_test.go +++ b/e2e/multiroot_scenarios_test.go @@ -1015,3 +1015,53 @@ func TestMultiRootScenario_ADeletionSurvivesALostReport(t *testing.T) { t.Errorf("an untouched file on the peer changed: %q", got) } } + +// The report that the peer took the location arrives, and the refresh it +// starts in the background runs only after a file has been deleted here — a +// moment's delay on a loaded machine. The refresh must not drop the deleted +// file from the record of what both hold, or the next sync fetches it back. +func TestMultiRootScenario_ADeletionSurvivesALateRefresh(t *testing.T) { + a, b, gameID, aConfig, bConfig := twoLocationPair(t, "LocLateRefresh", map[string]string{ + "settings.ini": "fullscreen=1", + "keys.cfg": "jump=space", + }) + pauseAutoSync(t, a, b, gameID) + if !testutil.WaitFor(45*time.Second, func() bool { + files, _, _ := a.Daemon.Store.GetSyncStateForRoot(gameID, b.NodeID(), "config") + for _, f := range files { + if f == "keys.cfg" { + return true + } + } + return false + }) { + t.Fatal("setup: A never recorded that B holds keys.cfg") + } + + if err := os.Remove(filepath.Join(aConfig, "keys.cfg")); 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") + } + // The refresh the report started, running late. + a.Daemon.P2P.Sync.RefreshLineage(context.Background(), gameID, peer) + + time.Sleep(syncSettleWindow) + a.API(http.MethodPost, "/api/games/"+gameID+"/sync", nil, nil) + if !testutil.WaitFor(45*time.Second, func() bool { + return readIn(bConfig, "keys.cfg") == "" + }) { + t.Fatalf("the deletion never reached the peer: keys.cfg there=%q, here=%q", + readIn(bConfig, "keys.cfg"), readIn(aConfig, "keys.cfg")) + } + if got := readIn(aConfig, "keys.cfg"); got != "" { + t.Errorf("the deleted file came back here: %q", got) + } +} diff --git a/internal/p2p/syncengine/multiroot.go b/internal/p2p/syncengine/multiroot.go index 04edf595..8cbde5a7 100644 --- a/internal/p2p/syncengine/multiroot.go +++ b/internal/p2p/syncengine/multiroot.go @@ -279,6 +279,22 @@ func (e *Engine) syncOneRoot(ctx context.Context, gameID string, game store.Game func (e *Engine) persistRootLineage(gameID, peerID, root string, local, remote delta.Manifest) { files, dirs := IntersectLineage(local, remote) + // As persistLineage does for the main folder: a path both held before + // that one side holds no longer is a deletion in flight, and its record is + // what lets the next sync send it on rather than fetch the file back. The + // refresh after a peer's report runs in the background, so a file deleted + // here before it ran dropped out of the record — and came back + // (TestMultiRootScenario_ADeletionSurvivesALateRefresh). + if oldFiles, oldDirs, err := e.Store.GetSyncStateForRoot(gameID, peerID, root); err == nil { + files = keepPendingDeletions(files, oldFiles, local.Files, remote.Files) + dirs = keepPendingDirDeletions(dirs, oldDirs, local.Dirs, remote.Dirs) + } + // And excluded paths stay out of it, as in the main folder: the refresh + // rebuilds this from unfiltered manifests. + if rules := e.rulesFor(gameID); !rules.Empty() { + files = filterPathList(files, rules) + dirs = filterPathList(dirs, rules) + } if err := e.Store.SetSyncStateForRoot(gameID, peerID, root, files, dirs); err != nil { e.Log("warn", fmt.Sprintf("persist lineage for the %q location failed: %v", root, err)) } From 6df6bcd962598f91a9a59aee429994eb68a0aba1 Mon Sep 17 00:00:00 2001 From: ChakraFusion <112588066+ChakraFusion@users.noreply.github.com> Date: Thu, 15 Oct 2026 13:33:30 +0200 Subject: [PATCH 3/3] sync: an emptied location's put-back survives the push-proof repair The push-proof repair for extra locations (first commit here) records the pushed state's files as shared. While the game is held as emptied that record is kept on purpose: "put them back" with no snapshot takes the location's files out of it so the next sync fetches them from the peer. Written back by the repair, they read as deleted on this device instead, the deletions went to the peer, and both devices lost the location's files. The repair now leaves the lineage alone while the game is held as emptied. Found while checking the same repair for the main folder, which had the same flaw. Test: TestMultiRootScenario_PutBackOfAnEmptiedLocationSurvivesALostReport (fails 3/3 without this: the location empty on both devices). Co-Authored-By: Claude Opus 5.5 --- e2e/multiroot_scenarios_test.go | 56 ++++++++++++++++++++++++++++ internal/p2p/syncengine/multiroot.go | 11 +++++- 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/e2e/multiroot_scenarios_test.go b/e2e/multiroot_scenarios_test.go index edf82e55..e6a4e54d 100644 --- a/e2e/multiroot_scenarios_test.go +++ b/e2e/multiroot_scenarios_test.go @@ -1065,3 +1065,59 @@ func TestMultiRootScenario_ADeletionSurvivesALateRefresh(t *testing.T) { t.Errorf("the deleted file came back here: %q", got) } } + +// "Put them back" for an emptied extra location, with no snapshot to restore, +// fetches its files even when a lost report has left the location's base +// behind. The push-proof repair records what both devices held; while the +// game is held as emptied it must leave alone the lineage put back takes the +// files out of, or they read as deleted here and the peer loses them too. +func TestMultiRootScenario_PutBackOfAnEmptiedLocationSurvivesALostReport(t *testing.T) { + files := map[string]string{"settings.ini": "shared v1", "keys.cfg": "jump=space"} + a, b, gameID, aConfig, bConfig := twoLocationPair(t, "LocPutBack", files) + testutil.SettleSync(t, gameID, a, b) + + emptyFolder(t, aConfig) + syncBothWays(t, gameID, a, b) + if e := emptiedState(t, a, gameID); e == nil { + t.Fatal("setup: emptying the location did not hold the game back") + } + var snaps map[string]struct { + Branches map[string]struct { + Snapshots []struct { + ID string `json:"id"` + } `json:"snapshots"` + } `json:"branches"` + } + a.API(http.MethodGet, "/api/games", nil, &snaps) + for _, br := range snaps[gameID].Branches { + for _, s := range br.Snapshots { + if err := a.Daemon.Store.SetSnapshotCheck(s.ID, time.Now().UnixMilli(), "damaged for this test"); err != nil { + t.Fatal(err) + } + } + } + a.API(http.MethodPost, "/api/games/"+gameID+"/emptied", map[string]string{"answer": "restore"}, nil) + + // What a lost report leaves for the location: the base behind what the + // peer holds, and the push still on record. + m, err := delta.BuildManifest(bConfig) + if err != nil { + t.Fatal(err) + } + if err := a.Daemon.Store.SetAgreedHashForRoot(gameID, b.NodeID(), "config", "a base from before"); err != nil { + t.Fatal(err) + } + if err := a.Daemon.Store.SetPushedHashForRoot(gameID, b.NodeID(), "config", m.RootHash(delta.PrimaryRoot)); err != nil { + t.Fatal(err) + } + + syncBothWays(t, gameID, a, b) + for rel, want := range files { + if got := readIn(bConfig, rel); got != want { + t.Errorf("the other device: config/%s is %q, want %q", rel, got, want) + } + if got := readIn(aConfig, rel); got != want { + t.Errorf("the emptied device, from the other: config/%s is %q, want %q", rel, got, want) + } + } +} diff --git a/internal/p2p/syncengine/multiroot.go b/internal/p2p/syncengine/multiroot.go index 8cbde5a7..81a64fea 100644 --- a/internal/p2p/syncengine/multiroot.go +++ b/internal/p2p/syncengine/multiroot.go @@ -133,11 +133,20 @@ func (e *Engine) syncOneRoot(ctx context.Context, gameID string, game store.Game // (TestMultiRootScenario_ADeletionSurvivesALostReport). Both held exactly // the pushed state, so its files are the lineage. // + // Not while the game is held as emptied (hold.go): the lineage is then + // kept on purpose. "Put them back" with no snapshot takes the files out of + // it so that the next sync fetches them; written back here, they read as + // deleted on this device, and the deletions went to the peer — both + // devices lost the location's files + // (TestMultiRootScenario_PutBackOfAnEmptiedLocationSurvivesALostReport). + // // Both hashes are of the filtered view, which is what the push recorded. if pushed := e.Store.GetPushedHashForRoot(gameID, peer.ID, sr.root.Name); pushed != "" { if remoteHash := remote.RootHash(delta.PrimaryRoot); remoteHash == pushed && e.Store.GetAgreedHashForRoot(gameID, peer.ID, sr.root.Name) != remoteHash { - e.persistRootLineage(gameID, peer.ID, sr.root.Name, remote, remote) + if _, held, err := e.Store.GetDeletionHold(gameID); err == nil && !held { + e.persistRootLineage(gameID, peer.ID, sr.root.Name, remote, remote) + } _ = e.Store.SetAgreedHashForRoot(gameID, peer.ID, sr.root.Name, remoteHash) } }