sync: deletions in extra locations survive a lost report or a late refresh

e2e/multiroot_scenarios_test.go

@@ -966,3 +966,158 @@ 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)
+	}
+}
+
+// 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)
+	}
+}
+
+// "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)
+		}
+	}
+}

internal/p2p/syncengine/multiroot.go

@@ -119,6 +119,38 @@ 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.
+	//
+	// 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 {
+			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)
+		}
+	}
+
 	fileList, dirList, err := e.Store.GetSyncStateForRoot(gameID, peer.ID, sr.root.Name)
 	if err != nil {
 		return err
@@ -132,24 +164,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 +194,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))
@@ -258,6 +288,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))
 	}