sync: a lost report no longer brings back a file deleted right after

e2e/deletion_record_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/opensave/opensave/internal/delta"
 	"github.com/opensave/opensave/testutil"
 )
 
@@ -135,3 +136,70 @@ func TestDeletion_NeverRemovesAnEditThePeerMadeAfterwards(t *testing.T) {
 			"that was done after the file was deleted elsewhere", got)
 	}
 }
+
+// A file handed over just before a sync-complete report went missing must
+// still be deletable.
+//
+// The report is what moves the shared base forward and records the files both
+// devices hold. Lost, it leaves the old base and a record without the new file;
+// only the push stays on record. A later sync proves the push from it and
+// banks the base, but the record of shared files stayed behind. Deleted before
+// the watcher noticed, the file then read as one the peer had and this device
+// never did, and was fetched straight back. On Windows CI this was
+// TestWatchAutoSyncChain's "deletion never auto-synced to B". The lost report
+// is set up directly here rather than raced for.
+func TestDeletion_SurvivesALostReport(t *testing.T) {
+	a, b, gameID := pairAndTrack(t, "Lost Report Deletion", map[string]string{"seed.sav": "seed"})
+	testutil.SettleSync(t, gameID, a, b)
+	st := a.Daemon.Store
+	var oldBase string
+	if !testutil.WaitFor(30*time.Second, func() bool {
+		oldBase = st.GetAgreedHash(gameID, b.NodeID())
+		return oldBase != ""
+	}) {
+		t.Fatal("setup: the devices never recorded that they agree")
+	}
+
+	a.WriteSave("auto.sav", "v1")
+	time.Sleep(syncSettleWindow)
+	a.API(http.MethodPost, "/api/games/"+gameID+"/sync", nil, nil)
+	if !testutil.WaitFor(30*time.Second, func() bool { return b.ReadSave("auto.sav") == "v1" }) {
+		t.Fatal("setup: auto.sav never reached B")
+	}
+	testutil.SettleSync(t, gameID, a, b)
+
+	// The report is let through and then undone, rather than dropped: written
+	// over while it is still being recorded, it would leave a mix of before
+	// and after that no lost report produces.
+	handedOver, err := delta.BuildManifest(a.SaveDir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !testutil.WaitFor(30*time.Second, func() bool {
+		return st.GetAgreedHash(gameID, b.NodeID()) == handedOver.ManifestHash()
+	}) {
+		t.Fatal("setup: the report never arrived")
+	}
+	if err := st.SetAgreedHash(gameID, b.NodeID(), oldBase); err != nil {
+		t.Fatal(err)
+	}
+	if err := st.SetPushedHash(gameID, b.NodeID(), handedOver.ManifestHash()); err != nil {
+		t.Fatal(err)
+	}
+	if err := st.SetSyncState(gameID, b.NodeID(), []string{"seed.sav"}, nil); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := os.Remove(filepath.Join(a.SaveDir, "auto.sav")); err != nil {
+		t.Fatal(err)
+	}
+	a.API(http.MethodPost, "/api/games/"+gameID+"/sync", nil, nil) // before the watcher records the deletion
+	time.Sleep(5 * time.Second)
+
+	if got := a.ReadSave("auto.sav"); got != "" {
+		t.Errorf("the deleted file came back: %q", got)
+	}
+	if got := b.ReadSave("auto.sav"); got != "" {
+		t.Errorf("the deletion never reached B: %q", got)
+	}
+}

e2e/emptied_folder_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/opensave/opensave/internal/delta"
 	"github.com/opensave/opensave/testutil"
 )
 
@@ -277,3 +278,47 @@ func TestEmptiedFolder_ASyncStartedDuringAWriteIsStillHeld(t *testing.T) {
 		t.Errorf("the emptied device says %+v; want the game held back over 3 files", e)
 	}
 }
+
+// "Put them back" with no snapshot fetches the files even when a lost report
+// has left the base behind. The push-proof repair records what both devices
+// held; while the game is held as emptied it must leave alone the record put
+// back takes the files out of, or they read as deleted here and the other
+// device loses them too.
+func TestEmptiedFolder_PutBackSurvivesALostReport(t *testing.T) {
+	a, b, gameID := emptiedAndHeld(t, "Emptied Lost Report Game")
+	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: the push still on record, and the base
+	// behind what the peer holds.
+	held, err := delta.BuildManifest(b.SaveDir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := a.Daemon.Store.SetAgreedHash(gameID, b.NodeID(), "a base from before"); err != nil {
+		t.Fatal(err)
+	}
+	if err := a.Daemon.Store.SetPushedHash(gameID, b.NodeID(), held.ManifestHash()); err != nil {
+		t.Fatal(err)
+	}
+	if a.Daemon.Store.GetPushedHash(gameID, b.NodeID()) == "" {
+		t.Fatal("setup: the push record did not stick")
+	}
+	a.APIStatus(http.MethodPost, "/api/games/"+gameID+"/sync", nil, nil)
+	syncBothWays(t, gameID, a, b)
+	stillHasIt(t, b, "the other device")
+	stillHasIt(t, a, "the emptied device, from the other")
+}

internal/p2p/syncengine/engine.go

@@ -575,8 +575,27 @@ func (e *Engine) SyncWithPeer(ctx context.Context, gameID string, peer Peer) (Re
 	// that, a record left over from an earlier push would match again if the
 	// peer ever returned to that state, dragging the base backwards onto it
 	// and skipping a conflict that a later agreement had earned.
+	//
+	// The record of which files both sides hold is repaired with it: the
+	// report is also what records that, so a lost one left a file this device
+	// had just handed over out of it. Deleted here before the watcher noted
+	// the deletion, that file read as one the peer had and this device never
+	// did, and it was fetched straight back — the deletion undone, on Windows
+	// CI as TestWatchAutoSyncChain's "deletion never auto-synced to B". Both
+	// held exactly the pushed state, so its files are what was shared — the
+	// repair extra locations make too (multiroot.go).
+	//
+	// Not while the game is held as emptied (hold.go). That record is then
+	// kept on purpose: "put them back" with no snapshot to restore takes the
+	// files out of it, so that the next sync fetches them from the peer.
+	// Written back here, they read as deleted on this device instead, and the
+	// deletions went to the peer — both devices lost the save
+	// (TestEmptiedFolder_PutBackWithoutASnapshotFetchesThem on Linux CI).
 	if pushed := e.Store.GetPushedHash(gameID, peer.ID); pushed != "" {
 		if remoteHash := remoteData.Manifest.ManifestHash(); agreedHash != remoteHash && remoteHash == pushed {
+			if _, held, err := e.Store.GetDeletionHold(gameID); err == nil && !held {
+				e.persistLineage(gameID, peer.ID, remoteData.Manifest, remoteData.Manifest)
+			}
 			_ = e.Store.SetAgreedHash(gameID, peer.ID, remoteHash)
 			agreedHash = remoteHash
 		}