From dd3cdba6cfffa6330966ffb5c153eea7173bd5df Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 04:47:28 +0000 Subject: [PATCH] sync: a lost report no longer brings back a file deleted right after When a sync-complete report is lost, the push-proof repair banks the pushed state as the shared base, but the record of which files both devices hold stayed as it was. A file handed over in that push was missing from it, so if it was deleted before the watcher noted the deletion, the next sync read it as a file the peer had and this device never did, and fetched it back. The repair now records the pushed state's files as shared too, as extra locations already do. Not while the game is held as emptied: that record is then kept on purpose. "Put them back" with no snapshot takes the files out of it so the next sync fetches them from the peer; written back by the repair, they read as deleted here and the deletions went to the peer, and both devices lost the save (TestEmptiedFolder_PutBackWithoutASnapshotFetchesThem on Linux CI, every run together with the emptied-hold fix). Seen on Windows CI as TestWatchAutoSyncChain's "deletion never auto-synced to B"; TestDeletion_SurvivesALostReport reproduces it deterministically, and TestEmptiedFolder_PutBackSurvivesALostReport the put-back case (fails 3/3 without the hold check). Co-Authored-By: Claude Opus 5.5 --- e2e/deletion_record_test.go | 68 +++++++++++++++++++++++++++++++ e2e/emptied_folder_test.go | 45 ++++++++++++++++++++ internal/p2p/syncengine/engine.go | 19 +++++++++ 3 files changed, 132 insertions(+) diff --git a/e2e/deletion_record_test.go b/e2e/deletion_record_test.go index 5133fe1d..6e93a11e 100644 --- a/e2e/deletion_record_test.go +++ b/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) + } +} diff --git a/e2e/emptied_folder_test.go b/e2e/emptied_folder_test.go index fd23596a..7c703b4b 100644 --- a/e2e/emptied_folder_test.go +++ b/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") +} diff --git a/internal/p2p/syncengine/engine.go b/internal/p2p/syncengine/engine.go index 2e6b7ce6..6c1c965b 100644 --- a/internal/p2p/syncengine/engine.go +++ b/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 }