From 4b50182c3a04beb6e98a197adff9c05da9e6fadc Mon Sep 17 00:00:00 2001 From: Seagulls Date: Thu, 8 Oct 2026 23:13:04 +0100 Subject: [PATCH 1/7] store: persist a per-game provisioning hold AutoSync off only stops this device from starting a sync. A paired device can still ask for the manifest and move files, and an in-memory pause dies on restart. Record the hold in the same transaction as the game, in a table older builds ignore, so a crash cannot leave a game the next launch will sync. Existing games have no row and stay as they are. --- internal/store/games.go | 15 +++ .../0038_game_provisioning_holds.sql | 16 +++ internal/store/provisioning.go | 116 ++++++++++++++++++ internal/store/provisioning_test.go | 101 +++++++++++++++ 4 files changed, 248 insertions(+) create mode 100644 internal/store/migrations/0038_game_provisioning_holds.sql create mode 100644 internal/store/provisioning.go create mode 100644 internal/store/provisioning_test.go diff --git a/internal/store/games.go b/internal/store/games.go index 52d9c928..89dcb3bf 100644 --- a/internal/store/games.go +++ b/internal/store/games.go @@ -23,6 +23,17 @@ func SlugifyGameID(name string) string { return strings.Trim(id, "-") } +// ValidExplicitGameID reports whether id is a stable game id a caller may +// pin while provisioning. It has to be the same slug OpenSave would derive, +// so a paired device can be given the source's id without accepting a path +// or an arbitrary string. +func ValidExplicitGameID(id string) bool { + if id == "" || len(id) > 128 { + return false + } + return id == SlugifyGameID(id) +} + // ErrNotFound is returned by single-row lookups when no matching row exists. var ErrNotFound = errors.New("not found") @@ -73,6 +84,10 @@ type Game struct { // the thing that deletes an excluded config. SyncIgnore string `db:"sync_ignore" json:"syncIgnore"` CreatedAt string `db:"created_at" json:"createdAt"` + // ProvisioningHold asks TrackGame to record a provisioning hold with the + // game. It is not a column: the durable flag is game_provisioning_holds, + // so SELECT * into this struct stays valid for older and newer builds. + ProvisioningHold bool `db:"-" json:"-"` } // CreateGame inserts a new game and its default "main" branch in one diff --git a/internal/store/migrations/0038_game_provisioning_holds.sql b/internal/store/migrations/0038_game_provisioning_holds.sql new file mode 100644 index 00000000..8bd63c5f --- /dev/null +++ b/internal/store/migrations/0038_game_provisioning_holds.sql @@ -0,0 +1,16 @@ +-- A game being configured must not sync, even after a crash or a daemon +-- restart, and even if the other device has already started syncing it. +-- +-- This is not auto_sync = 0. That flag only stops this device from starting +-- a sync. A paired device that already has the game still asks for the +-- manifest and can pull or push files. Pause is in memory and dies with the +-- process. Neither is a place to leave a half-configured game. +-- +-- A table, not a column on games. Older builds read games with SELECT * and +-- a new column breaks them; a table they do not know is ignored. No row +-- means the game is not held, which is every game that existed before this +-- migration. +CREATE TABLE game_provisioning_holds ( + game_id TEXT PRIMARY KEY REFERENCES games(id) ON DELETE CASCADE, + held_at TEXT NOT NULL +); diff --git a/internal/store/provisioning.go b/internal/store/provisioning.go new file mode 100644 index 00000000..4e357f53 --- /dev/null +++ b/internal/store/provisioning.go @@ -0,0 +1,116 @@ +package store + +import ( + "database/sql" + "errors" + "fmt" + "time" +) + +// Provisioning hold: a game that has been recorded but is not ready to sync. +// +// The row is written in the same transaction as the game, so a crash cannot +// leave a tracked game that the next launch will sync. Existing games have +// no row and are left alone. See migrations/0038_game_provisioning_holds.sql. + +// ProvisioningHeld reports whether gameID is still being configured. +// An unknown id is not held. +func (s *Store) ProvisioningHeld(gameID string) (bool, error) { + if gameID == "" { + return false, nil + } + var one int + err := s.db.Get(&one, `SELECT 1 FROM game_provisioning_holds WHERE game_id = ?`, gameID) + if errors.Is(err, sql.ErrNoRows) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("provisioning hold %s: %w", gameID, err) + } + return true, nil +} + +// ProvisioningHeldSet is every game currently held, for callers that walk +// the library and must not query once per game. +func (s *Store) ProvisioningHeldSet() (map[string]struct{}, error) { + var ids []string + if err := s.db.Select(&ids, `SELECT game_id FROM game_provisioning_holds`); err != nil { + return nil, fmt.Errorf("list provisioning holds: %w", err) + } + out := make(map[string]struct{}, len(ids)) + for _, id := range ids { + out[id] = struct{}{} + } + return out, nil +} + +// CreateHeldGame inserts a game and its provisioning hold in one transaction. +// AutoSync is forced off. The local untrack tombstone is cleared in the same +// transaction: re-tracking is explicit, but it must not become visible to a +// peer before the hold exists. +func (s *Store) CreateHeldGame(g Game) error { + if g.ID == "" { + return fmt.Errorf("held game requires an id") + } + if g.ActiveBranch == "" { + g.ActiveBranch = "main" + } + if g.MaxSnapshots == 0 { + g.MaxSnapshots = 20 + } + g.AutoSync = false + + tx, err := s.db.Beginx() + if err != nil { + return err + } + defer tx.Rollback() + + if _, err := tx.NamedExec(` + INSERT INTO games (id, name, save_path, active_branch, auto_sync, max_snapshots, max_manual_snapshots, app_id, exe_path, cover_url) + VALUES (:id, :name, :save_path, :active_branch, :auto_sync, :max_snapshots, :max_manual_snapshots, :app_id, :exe_path, :cover_url)`, + g); err != nil { + return fmt.Errorf("insert game: %w", err) + } + if _, err := tx.Exec(`INSERT INTO branches (game_id, name) VALUES (?, ?)`, g.ID, g.ActiveBranch); err != nil { + return fmt.Errorf("insert default branch: %w", err) + } + if _, err := tx.Exec( + `INSERT INTO game_provisioning_holds (game_id, held_at) VALUES (?, ?)`, + g.ID, time.Now().UTC().Format(time.RFC3339Nano), + ); err != nil { + return fmt.Errorf("insert provisioning hold: %w", err) + } + if _, err := tx.Exec(`DELETE FROM untracked_games WHERE game_id = ?`, g.ID); err != nil { + return fmt.Errorf("clear tombstone: %w", err) + } + return tx.Commit() +} + +// ReleaseProvisioning clears the hold and turns AutoSync on, in one +// transaction. A game that is not held is left unchanged, including a game +// whose AutoSync the user turned off on purpose. The bool is whether this +// call released a hold. +func (s *Store) ReleaseProvisioning(gameID string) (bool, error) { + tx, err := s.db.Beginx() + if err != nil { + return false, err + } + defer tx.Rollback() + + res, err := tx.Exec(`DELETE FROM game_provisioning_holds WHERE game_id = ?`, gameID) + if err != nil { + return false, fmt.Errorf("release provisioning hold %s: %w", gameID, err) + } + n, err := res.RowsAffected() + if err != nil { + return false, err + } + if n == 0 { + return false, tx.Commit() + } + if _, err := tx.Exec(`UPDATE games SET auto_sync = 1 WHERE id = ?`, gameID); err != nil { + return false, fmt.Errorf("enable auto-sync %s: %w", gameID, err) + } + return true, tx.Commit() +} diff --git a/internal/store/provisioning_test.go b/internal/store/provisioning_test.go new file mode 100644 index 00000000..668265cc --- /dev/null +++ b/internal/store/provisioning_test.go @@ -0,0 +1,101 @@ +package store + +import ( + "testing" +) + +func TestCreateHeldGameIsAtomicAndDefaultsExistingGamesClear(t *testing.T) { + s := openTestStore(t) + + plain := Game{ID: "already-live", Name: "Already Live", SavePath: "/tmp/live", ActiveBranch: "main", AutoSync: true, MaxSnapshots: 20} + if err := s.CreateGame(plain); err != nil { + t.Fatal(err) + } + held, err := s.ProvisioningHeld("already-live") + if err != nil { + t.Fatal(err) + } + if held { + t.Fatal("a game created without the hold was held; existing libraries must not change") + } + + if err := s.CreateHeldGame(Game{ + ID: "configuring", Name: "Configuring", SavePath: "/tmp/cfg", MaxSnapshots: 20, + AutoSync: true, // caller cannot override the forced off + }); err != nil { + t.Fatal(err) + } + got, err := s.GetGame("configuring") + if err != nil { + t.Fatal(err) + } + if got.AutoSync { + t.Fatal("held game was stored with AutoSync on") + } + held, err = s.ProvisioningHeld("configuring") + if err != nil || !held { + t.Fatalf("hold missing after commit: held=%v err=%v", held, err) + } + + set, err := s.ProvisioningHeldSet() + if err != nil { + t.Fatal(err) + } + if _, ok := set["configuring"]; !ok { + t.Fatal("held set omitted the new game") + } + if _, ok := set["already-live"]; ok { + t.Fatal("held set included a game that was not held") + } + + released, err := s.ReleaseProvisioning("already-live") + if err != nil { + t.Fatal(err) + } + if released { + t.Fatal("releasing a game that was not held reported a release") + } + plain, err = s.GetGame("already-live") + if err != nil { + t.Fatal(err) + } + if !plain.AutoSync { + t.Fatal("a no-op release changed AutoSync on a game that was not held") + } + + released, err = s.ReleaseProvisioning("configuring") + if err != nil || !released { + t.Fatalf("release = %v, %v", released, err) + } + got, err = s.GetGame("configuring") + if err != nil { + t.Fatal(err) + } + if !got.AutoSync { + t.Fatal("release left AutoSync off") + } + held, err = s.ProvisioningHeld("configuring") + if err != nil || held { + t.Fatalf("hold survived release: held=%v err=%v", held, err) + } + + released, err = s.ReleaseProvisioning("configuring") + if err != nil || released { + t.Fatalf("second release = %v, %v; want a no-op", released, err) + } +} + +func TestValidExplicitGameID(t *testing.T) { + ok := []string{"hades", "crash-bandicoot-4", "a1"} + bad := []string{"", "Hades", "../x", "a/b", "a b", "a--b", "-a", "a-"} + for _, id := range ok { + if !ValidExplicitGameID(id) { + t.Errorf("rejected %q", id) + } + } + for _, id := range bad { + if ValidExplicitGameID(id) { + t.Errorf("accepted %q", id) + } + } +} From e840f37a06873c21768747bec5424e1ad9be3570 Mon Sep 17 00:00:00 2001 From: Seagulls Date: Thu, 8 Oct 2026 23:13:04 +0100 Subject: [PATCH 2/7] sync: keep a held game out of every transfer path Creating or placing with provisioningHold, or with autoSync false, records the game and refuses manifest, block, delete, snapshot, watch, retry, reconcile and direct sync until that one game is released. Releasing does not sync the rest of the library. An explicit id is accepted only while held, so the other device can be registered without an offer and without sending save contents. --- internal/api/routes.go | 34 ++++- internal/api/routes_offered_games.go | 34 ++++- internal/api/routes_peers.go | 6 + internal/api/routes_provisioning.go | 32 +++++ internal/api/routes_roots.go | 8 ++ internal/api/server.go | 1 + internal/cliapp/cliapp.go | 34 ++++- internal/cliapp/client.go | 36 ++++++ internal/cliapp/commands_games.go | 48 ++++++- internal/cliapp/commands_offers.go | 15 ++- internal/daemon/cloud_retry.go | 4 + internal/daemon/cloudsync.go | 2 +- internal/daemon/daemon.go | 186 ++++++++++++++++++++++++++- internal/p2p/engine.go | 29 ++++- internal/p2p/routes.go | 24 +++- internal/p2p/syncengine/engine.go | 32 ++++- internal/p2p/syncengine/hold.go | 15 +++ internal/p2p/wanclient_handlers.go | 14 +- testutil/harness.go | 51 +++++++- 19 files changed, 571 insertions(+), 34 deletions(-) create mode 100644 internal/api/routes_provisioning.go diff --git a/internal/api/routes.go b/internal/api/routes.go index 65a28d0c..77bb2fe1 100644 --- a/internal/api/routes.go +++ b/internal/api/routes.go @@ -29,6 +29,7 @@ func (s *Server) routes(r chi.Router) { r.Get("/api/games", s.handleListGames) r.Post("/api/games", s.handleTrackGame) + r.Post("/api/games/{gameId}/release-provisioning", s.handleReleaseProvisioning) r.Get("/api/suggest-name", s.handleSuggestName) r.Post("/api/games/untrack-bulk", s.handleBulkUntrack) @@ -339,9 +340,16 @@ func (s *Server) handlePruneSnapshots(w http.ResponseWriter, r *http.Request) { func (s *Server) handleTrackGame(w http.ResponseWriter, r *http.Request) { var body struct { - Name string `json:"name"` - SavePath string `json:"savePath"` - AppID string `json:"appId"` + ID string `json:"id"` + Name string `json:"name"` + SavePath string `json:"savePath"` + AppID string `json:"appId"` + ProvisioningHold bool `json:"provisioningHold"` + // AutoSync is only consulted when the caller sets it. Omitted keeps + // today's behaviour: track, then sync. false is a request to hold the + // game, because storing AutoSync off after the fact does not stop a + // peer that already knows the id from pulling the save. + AutoSync *bool `json:"autoSync"` } if err := readJSON(r, &body); err != nil { writeError(w, http.StatusBadRequest, err.Error()) @@ -351,8 +359,20 @@ func (s *Server) handleTrackGame(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, "name and savePath are required") return } + hold := body.ProvisioningHold || (body.AutoSync != nil && !*body.AutoSync) + if body.ID != "" && !hold { + writeError(w, http.StatusBadRequest, "an explicit game id is only accepted when the game is created held") + return + } + if body.ID != "" && !store.ValidExplicitGameID(body.ID) { + writeError(w, http.StatusBadRequest, "game id must be a lowercase slug") + return + } - game, err := s.Daemon.TrackGame(store.Game{Name: body.Name, SavePath: body.SavePath, AppID: body.AppID}) + game, err := s.Daemon.TrackGame(store.Game{ + ID: body.ID, Name: body.Name, SavePath: body.SavePath, AppID: body.AppID, + ProvisioningHold: hold, + }) if err != nil { // Duplicates (id or path) are conflicts; anything else the daemon // rejects is bad input. @@ -455,10 +475,12 @@ func (s *Server) handleUpdateGame(w http.ResponseWriter, r *http.Request) { return } - // Re-watch if the save location or autoSync flag changed. + // Re-watch if the save location or autoSync flag changed. A hold still + // wins: turning autoSync on in the row must not start a watch that + // would sync a game that has not been released. if game.SavePath != oldSavePath || game.AutoSync != oldAutoSync { s.Daemon.Watcher.Unwatch(gameID) - if game.AutoSync { + if game.AutoSync && !s.Daemon.StoreProvisioningHeld(gameID) { if err := s.Daemon.Watcher.Watch(gameID, game.SavePath); err != nil { s.Daemon.Log.Log("warn", "re-watch failed: "+err.Error()) } diff --git a/internal/api/routes_offered_games.go b/internal/api/routes_offered_games.go index a950d486..090c03a5 100644 --- a/internal/api/routes_offered_games.go +++ b/internal/api/routes_offered_games.go @@ -42,7 +42,9 @@ func (s *Server) handlePlaceOfferedGame(w http.ResponseWriter, r *http.Request) gameID := chi.URLParam(r, "gameId") var body struct { - Path string `json:"path"` + Path string `json:"path"` + ProvisioningHold bool `json:"provisioningHold"` + AutoSync *bool `json:"autoSync"` } if err := readJSON(r, &body); err != nil { writeError(w, http.StatusBadRequest, err.Error()) @@ -58,6 +60,22 @@ func (s *Server) handlePlaceOfferedGame(w http.ResponseWriter, r *http.Request) writeError(w, http.StatusInternalServerError, err.Error()) return } + hold := body.ProvisioningHold || (body.AutoSync != nil && !*body.AutoSync) + if len(offers) == 0 && hold { + // The first place committed and the response was lost. Repeating it + // must not 404 into a second attempt that invents another game. + if _, err := s.Daemon.Store.GetGame(gameID); err == nil { + game, err := s.Daemon.TrackGame(store.Game{ + ID: gameID, Name: gameID, SavePath: body.Path, ProvisioningHold: true, + }) + if err != nil { + writeError(w, http.StatusConflict, err.Error()) + return + } + writeJSON(w, http.StatusOK, s.gamePayload(game)) + return + } + } if len(offers) == 0 { // Either it was placed or declined already, or the peer that offered // it has been unpaired. Not an error worth alarming anyone about. @@ -66,12 +84,16 @@ func (s *Server) handlePlaceOfferedGame(w http.ResponseWriter, r *http.Request) } offer := offers[0] + // autoSync: false is the request from issue #40. It is stored as a + // provisioning hold, not as the ordinary AutoSync flag: that flag does + // not stop a peer from pulling the save. Omitted keeps today's placement. game, err := s.Daemon.TrackGame(store.Game{ - ID: offer.GameID, - Name: offer.Name, - SavePath: body.Path, - AppID: offer.AppID, - CoverURL: offer.CoverURL, + ID: offer.GameID, + Name: offer.Name, + SavePath: body.Path, + AppID: offer.AppID, + CoverURL: offer.CoverURL, + ProvisioningHold: hold, }) if err != nil { // Path validation lives in TrackGame and its messages are written for diff --git a/internal/api/routes_peers.go b/internal/api/routes_peers.go index 4eb9b594..53a065a6 100644 --- a/internal/api/routes_peers.go +++ b/internal/api/routes_peers.go @@ -252,6 +252,8 @@ func syncReason(err error) string { return "paused" case errors.Is(err, syncengine.ErrHeld): return "held" + case errors.Is(err, syncengine.ErrProvisioning): + return "provisioning" case errors.Is(err, p2p.ErrNoPeersOnline): return "offline" default: @@ -269,6 +271,10 @@ func (s *Server) handleSyncAll(w http.ResponseWriter, r *http.Request) { } results := map[string]any{} for _, g := range games { + if s.Daemon.StoreProvisioningHeld(g.ID) { + results[g.ID] = map[string]string{"status": "skipped", "reason": "provisioning"} + continue + } if !g.AutoSync { results[g.ID] = map[string]string{"status": "skipped", "reason": "autoSync disabled"} continue diff --git a/internal/api/routes_provisioning.go b/internal/api/routes_provisioning.go new file mode 100644 index 00000000..5f16bde3 --- /dev/null +++ b/internal/api/routes_provisioning.go @@ -0,0 +1,32 @@ +package api + +import ( + "errors" + "net/http" + + "github.com/go-chi/chi/v5" + + "github.com/opensave/opensave/internal/store" +) + +// handleReleaseProvisioning ends the hold on one game and syncs that game +// only. It does not pause or sync the rest of the library. A repeat after +// the hold is gone is a success and does not sync again. +func (s *Server) handleReleaseProvisioning(w http.ResponseWriter, r *http.Request) { + gameID := chi.URLParam(r, "gameId") + released, err := s.Daemon.ReleaseProvisioning(gameID) + if err != nil { + status := http.StatusBadRequest + if errors.Is(err, store.ErrNotFound) { + status = http.StatusNotFound + } + writeError(w, status, err.Error()) + return + } + s.BroadcastGamesUpdate() + writeJSON(w, http.StatusOK, map[string]any{ + "id": gameID, + "released": released, + "alreadyReleased": !released, + }) +} diff --git a/internal/api/routes_roots.go b/internal/api/routes_roots.go index 77915bf3..2cfd44a8 100644 --- a/internal/api/routes_roots.go +++ b/internal/api/routes_roots.go @@ -6,6 +6,8 @@ import ( "strings" "github.com/go-chi/chi/v5" + + "github.com/opensave/opensave/internal/daemon" ) // A game's extra save locations: the folders beyond its main one that belong @@ -68,6 +70,12 @@ func (s *Server) handleAddGameRoot(w http.ResponseWriter, r *http.Request) { return } path = abs + if s.Daemon.StoreProvisioningHeld(gameID) { + if err := daemon.RefuseSymlinkSave(path); err != nil { + writeError(w, http.StatusBadRequest, err.Error()) + return + } + } } // Rejections here are the overlap and naming rules, which are the user's // to hear about rather than a server fault: "that folder is already diff --git a/internal/api/server.go b/internal/api/server.go index 66a32562..0879a4ec 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -551,6 +551,7 @@ func (s *Server) gamePayload(g store.Game) map[string]any { "savePath": g.SavePath, "activeBranch": g.ActiveBranch, "autoSync": g.AutoSync, + "provisioningHold": s.Daemon != nil && s.Daemon.StoreProvisioningHeld(g.ID), "maxSnapshots": g.MaxSnapshots, "maxManualSnapshots": g.MaxManualSnapshots, "appId": g.AppID, diff --git a/internal/cliapp/cliapp.go b/internal/cliapp/cliapp.go index dacaa2ac..9e800243 100644 --- a/internal/cliapp/cliapp.go +++ b/internal/cliapp/cliapp.go @@ -571,6 +571,20 @@ func loadScanResults(homeDir string) []scanChoice { } func cmdAdd(d *daemon.Daemon, args []string) int { + hold, args := holdFlag(args) + explicitID, args, err := idFlag(args) + if err != nil { + fmt.Fprintf(os.Stderr, "error: %v\n", err) + return 1 + } + if explicitID != "" && !hold { + fmt.Fprintln(os.Stderr, "error: --id is only accepted with --hold") + return 1 + } + if explicitID != "" && !store.ValidExplicitGameID(explicitID) { + fmt.Fprintln(os.Stderr, "error: --id must be a lowercase slug") + return 1 + } // A single numeric argument means "the nth thing the last scan showed". // Unambiguous against the two-argument form, so a game genuinely called // "3" is still trackable by naming its path. @@ -587,7 +601,7 @@ func cmdAdd(d *daemon.Daemon, args []string) int { fmt.Fprintln(os.Stderr, "error: nothing in that path says which game it is — give a name: opensave add ") return 1 } - return trackGame(d, name, args[0]) + return trackGame(d, name, args[0], hold, explicitID) } choices := loadScanResults(d.Paths.HomeDir) if len(choices) == 0 { @@ -599,31 +613,37 @@ func cmdAdd(d *daemon.Daemon, args []string) int { return 1 } pick := choices[n-1] - return trackGame(d, pick.Name, pick.SavePath) + return trackGame(d, pick.Name, pick.SavePath, hold, explicitID) } if len(args) < 2 { - fmt.Fprintln(os.Stderr, "usage: opensave add \n opensave add (named from the path)\n opensave add (from the last `opensave scan`)") + fmt.Fprintln(os.Stderr, "usage: opensave add \n opensave add --hold [--id ] \n opensave add (named from the path)\n opensave add (from the last `opensave scan`)") return 1 } - return trackGame(d, args[0], args[1]) + return trackGame(d, args[0], args[1], hold, explicitID) } // trackGame is the shared tail of both `add` forms, so picking a scan result // and typing a path by hand cannot drift apart in what they report. -func trackGame(d *daemon.Daemon, name, savePath string) int { +func trackGame(d *daemon.Daemon, name, savePath string, hold bool, explicitID string) int { abs, err := filepath.Abs(savePath) if err != nil { fmt.Fprintf(os.Stderr, "error: %v\n", err) return 1 } - game, err := d.TrackGame(store.Game{Name: name, SavePath: abs}) + game, err := d.TrackGame(store.Game{ + ID: explicitID, Name: name, SavePath: abs, ProvisioningHold: hold, + }) if err != nil { fmt.Fprintf(os.Stderr, "error: %v\n", err) return 1 } - success("Now tracking %s", bold(game.Name)) + if hold { + success("Now tracking %s without syncing — release it when it is configured", bold(game.Name)) + } else { + success("Now tracking %s", bold(game.Name)) + } note("id: " + game.ID) note("path: " + game.SavePath) return 0 diff --git a/internal/cliapp/client.go b/internal/cliapp/client.go index 70193426..90af34d6 100644 --- a/internal/cliapp/client.go +++ b/internal/cliapp/client.go @@ -138,6 +138,42 @@ func daemonRunning() bool { // Every command can emit JSON so the CLI is scriptable — the point of a // headless client is being driven by something other than a human. +// holdFlag reports whether --hold was passed, and returns the remaining args. +func holdFlag(args []string) (bool, []string) { + return stripFlag(args, "--hold") +} + +// idFlag reports a --id and returns the remaining args. +func idFlag(args []string) (string, []string, error) { + out := make([]string, 0, len(args)) + id := "" + for i := 0; i < len(args); i++ { + if args[i] != "--id" { + out = append(out, args[i]) + continue + } + if i+1 >= len(args) { + return "", nil, fmt.Errorf("--id requires a game id") + } + id = args[i+1] + i++ + } + return id, out, nil +} + +func stripFlag(args []string, flag string) (bool, []string) { + out := make([]string, 0, len(args)) + found := false + for _, a := range args { + if a == flag { + found = true + continue + } + out = append(out, a) + } + return found, out +} + // jsonFlag reports whether --json was passed, and returns the remaining args. func jsonFlag(args []string) (bool, []string) { out := args[:0:0] diff --git a/internal/cliapp/commands_games.go b/internal/cliapp/commands_games.go index 4e3f53c4..fb1012da 100644 --- a/internal/cliapp/commands_games.go +++ b/internal/cliapp/commands_games.go @@ -20,6 +20,9 @@ import ( // cmdGame edits one game's settings. func cmdGame(d *daemon.Daemon, args []string) int { asJSON, args := jsonFlag(args) + if len(args) >= 2 && args[1] == "release" { + return releaseProvisioning(d, gameIDFrom(args), asJSON) + } if len(args) < 2 || args[1] != "set" { fmt.Fprintln(os.Stderr, gameUsage) return 1 @@ -80,7 +83,7 @@ func cmdGame(d *daemon.Daemon, args []string) int { // Re-watch so a path or auto-sync change takes effect immediately rather // than at the next restart. d.Watcher.Unwatch(game.ID) - if game.AutoSync { + if game.AutoSync && !d.StoreProvisioningHeld(game.ID) { if err := d.Watcher.Watch(game.ID, game.SavePath); err != nil { d.Log.Log("warn", "re-watch after config change failed: "+err.Error()) } @@ -93,7 +96,50 @@ func cmdGame(d *daemon.Daemon, args []string) int { return 0 } +func gameIDFrom(args []string) string { + if len(args) == 0 { + return "" + } + return args[0] +} + +func releaseProvisioning(d *daemon.Daemon, gameID string, asJSON bool) int { + if gameID == "" { + fmt.Fprintln(os.Stderr, gameUsage) + return 1 + } + // Prefer the running daemon so the release starts a watch and a sync of + // this game there. The short-lived process below is the fallback when + // nothing is listening, and the next launch of the real daemon picks the + // cleared hold up from the database. + if daemonRunning() { + raw, err := daemonRequest("POST", "/api/games/"+gameID+"/release-provisioning", map[string]any{}) + if err != nil { + return fail(asJSON, err) + } + if asJSON { + return emitRawJSON(raw) + } + success("Released %s. It can sync from here on.", bold(gameID)) + return 0 + } + released, err := d.ReleaseProvisioning(gameID) + if err != nil { + return fail(asJSON, err) + } + if asJSON { + return emitJSON(map[string]any{"id": gameID, "released": released, "alreadyReleased": !released}) + } + if released { + success("Released %s. It can sync from here on.", bold(gameID)) + } else { + success("%s was already released.", bold(gameID)) + } + return 0 +} + const gameUsage = `usage: opensave game set + opensave game release let a game that was being configured start syncing name Display name (also how peers match this game) path Move tracking to a different save location diff --git a/internal/cliapp/commands_offers.go b/internal/cliapp/commands_offers.go index baa4805a..aa459198 100644 --- a/internal/cliapp/commands_offers.go +++ b/internal/cliapp/commands_offers.go @@ -24,10 +24,13 @@ type offeredGame struct { const offersUsage = `usage: opensave offers games another device syncs, waiting for a folder here opensave offers place track it here, in that folder + opensave offers place --hold + track it held, so it does not sync until released opensave offers decline don't — it is not offered again` func cmdOffers(args []string) int { asJSON, args := jsonFlag(args) + hold, args := holdFlag(args) if len(args) == 0 || args[0] == "list" { return listOffers(asJSON) } @@ -41,7 +44,11 @@ func cmdOffers(args []string) int { if err != nil { return fail(asJSON, err) } - raw, err := daemonRequest("POST", "/api/offered-games/"+url.PathEscape(args[1])+"/place", map[string]string{"path": folder}) + body := map[string]any{"path": folder} + if hold { + body["provisioningHold"] = true + } + raw, err := daemonRequest("POST", "/api/offered-games/"+url.PathEscape(args[1])+"/place", body) if err != nil { return fail(asJSON, err) } @@ -52,7 +59,11 @@ func cmdOffers(args []string) int { Name string `json:"name"` } _ = json.Unmarshal(raw, &game) - success("Now tracking %s at %s — it syncs from here on.", bold(orNone(game.Name)), folder) + if hold { + success("Now tracking %s at %s without syncing — release it when it is configured.", bold(orNone(game.Name)), folder) + } else { + success("Now tracking %s at %s — it syncs from here on.", bold(orNone(game.Name)), folder) + } return 0 case "decline": if len(args) < 2 { diff --git a/internal/daemon/cloud_retry.go b/internal/daemon/cloud_retry.go index ea8d06e8..8b3f83f5 100644 --- a/internal/daemon/cloud_retry.go +++ b/internal/daemon/cloud_retry.go @@ -2,6 +2,7 @@ package daemon import ( "fmt" + "strings" "time" "github.com/opensave/opensave/internal/cloud" @@ -60,6 +61,9 @@ func (d *Daemon) retryFailedUploads(inCloud []cloud.CloudFile) { } d.Log.Log("info", "cloud: sending again "+what+" that did not go up earlier") for _, p := range send { + if id, _, ok := strings.Cut(p.RemoteName, "__"); ok && d.provisioningHeld(id) { + continue + } d.uploads.Add() d.runCloudUpload(p.ZipPath, p.RemoteName, d.Log) } diff --git a/internal/daemon/cloudsync.go b/internal/daemon/cloudsync.go index 6b4dbbf5..cfe697ea 100644 --- a/internal/daemon/cloudsync.go +++ b/internal/daemon/cloudsync.go @@ -534,7 +534,7 @@ func (d *Daemon) CheckCloud() { if !follows && d.neverHeldFiles(game) { follows, state = true, saveUnchanged } - if follows && state == saveUnchanged && settings.CloudAutoPull && game.AutoSync { + if follows && state == saveUnchanged && settings.CloudAutoPull && game.AutoSync && !d.provisioningHeld(game.ID) { err := d.pullFromCloud(game, cand.file, cand.head.DeviceName) if err == nil { if d.OnCloudPulled != nil { diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go index 306ce6ff..efa24c7f 100644 --- a/internal/daemon/daemon.go +++ b/internal/daemon/daemon.go @@ -197,6 +197,12 @@ func New(opts Options) (*Daemon, error) { // Counted here, on the caller's goroutine, and only then moved to the // background, so that Stop, once it has seen the snapshot finish, // also sees its upload. + // A game still being configured must not publish its snapshot. The + // name is gameID__branch__snapID.zip (cloudSnapshotName). Checked + // before the pause queue: a resume must not send it either. + if id, _, ok := strings.Cut(remoteFileName, "__"); ok && d.provisioningHeld(id) { + return + } if d.P2P.Pause.Paused() { d.holdUpload(zipPath, remoteFileName) return @@ -285,7 +291,7 @@ func (d *Daemon) Start() error { _ = d.Store.UpdateGame(game) } } - if !game.AutoSync { + if !game.AutoSync || d.provisioningHeld(game.ID) { continue } if err := d.watchGame(game.ID, game.SavePath); err != nil { @@ -617,7 +623,7 @@ func (d *Daemon) ResyncWatchers() (started, stopped int) { names := make(map[string]string, len(games)) for _, game := range games { names[game.ID] = game.Name - if game.AutoSync { + if game.AutoSync && !d.provisioningHeld(game.ID) { want[game.ID] = game.SavePath } } @@ -682,6 +688,16 @@ func isDuplicateGameID(err error) bool { func (d *Daemon) TrackGame(game store.Game) (store.Game, error) { abs, err := d.ValidateSavePath(game.SavePath) if err != nil { + // A retry of a held create names a folder this game already tracks. + // That is the same admission, not a second game. Checked here because + // the duplicate-path refusal otherwise wins before the hold path runs. + if game.ProvisioningHold && game.ID != "" { + if existing, findErr := d.Store.GetGame(game.ID); findErr == nil { + if reused, reuseErr := d.reuseHeldGame(existing, game.SavePath); reuseErr == nil { + return reused, nil + } + } + } return store.Game{}, err } game.SavePath = abs @@ -716,6 +732,13 @@ func (d *Daemon) TrackGame(game store.Game) (store.Game, error) { game.ID = id } + // A game still being configured is recorded held, and nothing is said to + // a peer until that hold is released. NotifyRetrack and the sync below + // are how a new game becomes visible; both have to stay off here. + if game.ProvisioningHold { + return d.trackProvisioningHold(game) + } + // Explicit (re)tracking overrides any earlier untrack: clear the local // tombstone, and tell peers to clear theirs too so a peer that mirrored // an earlier untrack will accept this game again (otherwise its @@ -823,6 +846,147 @@ func (d *Daemon) TrackGame(game store.Game) (store.Game, error) { return created, nil } +// trackProvisioningHold records a game that must not sync until released. +// The hold is committed with the row. No retrack, no watch, no peer sync. +func (d *Daemon) trackProvisioningHold(game store.Game) (store.Game, error) { + if err := refuseSymlinkSave(game.SavePath); err != nil { + return store.Game{}, err + } + if existing, err := d.Store.FindGameBySavePath(game.SavePath); err == nil { + if game.ID != "" && existing.ID != game.ID { + return store.Game{}, fmt.Errorf("%q already tracks this folder", existing.Name) + } + return d.reuseHeldGame(existing, game.SavePath) + } + if game.ID != "" { + if existing, err := d.Store.GetGame(game.ID); err == nil { + return d.reuseHeldGame(existing, game.SavePath) + } + } + game.AutoSync = false + if game.ActiveBranch == "" { + game.ActiveBranch = "main" + } + if err := d.Store.CreateHeldGame(game); err != nil { + if isDuplicateGameID(err) { + if existing, getErr := d.Store.GetGame(game.ID); getErr == nil { + return d.reuseHeldGame(existing, game.SavePath) + } + } + return store.Game{}, err + } + d.setUpRegistryCapture(game) + d.initialSnapshots.Add() + go func() { + defer d.initialSnapshots.Done() + if _, err := d.Snapshots.Create(game.ID, "Initial snapshot", true); err != nil { + d.Log.Log("warn", fmt.Sprintf("initial snapshot for %q failed: %v", game.Name, err)) + } + // Deliberately no watch and no SyncGame. Watching would push the + // first change, and syncing would publish the game to paired peers. + d.Log.Log("info", fmt.Sprintf("tracking %q at %s without syncing; it is still being configured", game.Name, logging.Quote(game.SavePath))) + }() + created, err := d.Store.GetGame(game.ID) + if err != nil { + return store.Game{}, err + } + return created, nil +} + +// reuseHeldGame is a retry of a create that already committed. Same id and +// same folder returns the existing row. Anything else is a conflict: a live +// game must not be turned into a hold, and a held game must not be moved. +func (d *Daemon) reuseHeldGame(existing store.Game, abs string) (store.Game, error) { + held, err := d.Store.ProvisioningHeld(existing.ID) + if err != nil { + return store.Game{}, err + } + if !held { + return store.Game{}, fmt.Errorf("%q already exists", existing.Name) + } + if !sameSaveLocation(existing.SavePath, abs) { + return store.Game{}, fmt.Errorf("%q is already being configured at %q", existing.Name, existing.SavePath) + } + return existing, nil +} + +func sameSaveLocation(a, b string) bool { + if strings.EqualFold(filepath.Clean(a), filepath.Clean(b)) { + return true + } + ai, aerr := os.Stat(a) + bi, berr := os.Stat(b) + return aerr == nil && berr == nil && os.SameFile(ai, bi) +} + +// refuseSymlinkSave rejects a save location that is itself a symlink. +// Tracking the link would follow it and sync whatever it points at, including +// a folder outside the directory the caller named. +func refuseSymlinkSave(path string) error { + info, err := os.Lstat(path) + if err != nil { + return fmt.Errorf("save path does not exist: %s", path) + } + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("refusing a symlink as a save location: %s", path) + } + return nil +} + +// StoreProvisioningHeld reports whether gameID, or an alias of it, is still +// being configured. The API uses it to keep a hold from being watched. +func (d *Daemon) StoreProvisioningHeld(gameID string) bool { + return d.provisioningHeld(gameID) +} + +// RefuseSymlinkSave rejects a save location that is itself a symlink. +func RefuseSymlinkSave(path string) error { + return refuseSymlinkSave(path) +} + +func (d *Daemon) provisioningHeld(gameID string) bool { + if d == nil || d.Store == nil || gameID == "" { + return false + } + held, err := d.Store.ProvisioningHeld(gameID) + if err == nil && held { + return true + } + if canonical, ok := d.Store.ResolveGameAlias(gameID); ok && canonical != gameID { + held, err = d.Store.ProvisioningHeld(canonical) + return err == nil && held + } + return false +} + +// ReleaseProvisioning clears the hold and lets this game sync. It syncs only +// this game. Other games are not paused and are not part of the release. +// A second call is a no-op and does not sync again. +func (d *Daemon) ReleaseProvisioning(gameID string) (bool, error) { + game, err := d.Store.GetGame(gameID) + if err != nil { + return false, err + } + released, err := d.Store.ReleaseProvisioning(gameID) + if err != nil || !released { + return released, err + } + if err := d.watchGame(game.ID, game.SavePath); err != nil && + !errors.Is(err, watcher.ErrStopped) && !errors.Is(err, watcher.ErrSaveFolderMissing) { + d.Log.Log("warn", fmt.Sprintf("could not watch %q after release: %v", game.Name, err)) + } + d.P2P.GoSync(func(ctx context.Context) { + syncCtx, cancel := context.WithTimeout(ctx, 10*time.Minute) + defer cancel() + if _, err := d.P2P.SyncGame(syncCtx, gameID); err != nil && + !errors.Is(err, syncengine.ErrPaused) && !errors.Is(err, syncengine.ErrHeld) && + !errors.Is(err, syncengine.ErrProvisioning) { + d.Log.Log("info", fmt.Sprintf("sync after releasing %q: %v", game.Name, err)) + } + }) + return true, nil +} + // validateSavePath rejects save locations that can never be right: paths // that don't exist, whole-profile/system directories, drive roots, // OpenSave's own data directory, and paths another game already tracks. @@ -1045,6 +1209,8 @@ func (d *Daemon) UntrackGame(gameID string) error { if game, err := d.Store.GetGame(gameID); err == nil { name, savePath = game.Name, game.SavePath } + // Read before the delete: the hold row cascades away with the game. + held := d.provisioningHeld(gameID) if err := d.Store.DeleteGame(gameID); err != nil { return err } @@ -1064,7 +1230,12 @@ func (d *Daemon) UntrackGame(gameID string) error { // that went missing while untracked as a deletion to propagate. _ = d.Store.ForgetGameSyncState(gameID) d.P2P.ClearPendingResync(gameID) - d.P2P.NotifyUntrack(gameID) + // A held game was never published. Telling peers to untrack it would + // remove a copy that the other device is still configuring, or one it + // has already released, because of an abort on this side. + if !held { + d.P2P.NotifyUntrack(gameID) + } return nil } @@ -1079,6 +1250,9 @@ func (d *Daemon) LinkGames(canonicalID, aliasID string) error { if canonicalID == "" || aliasID == "" || canonicalID == aliasID { return fmt.Errorf("invalid link %q -> %q", aliasID, canonicalID) } + if d.provisioningHeld(canonicalID) || d.provisioningHeld(aliasID) { + return fmt.Errorf("a game still being configured cannot be linked") + } if _, err := d.Store.GetGame(canonicalID); err != nil { return fmt.Errorf("canonical game %q: %w", canonicalID, err) } @@ -1197,6 +1371,10 @@ func (d *Daemon) UnlinkGame(aliasID string) error { // untrackFromPeer mirrors a peer's untrack: remove the game + tombstone it, // WITHOUT re-notifying (no loop). func (d *Daemon) untrackFromPeer(gameID string) { + if d.provisioningHeld(gameID) { + d.Log.Log("info", fmt.Sprintf("ignored a peer untrack of %q while it is still being configured", gameID)) + return + } d.Watcher.Unwatch(gameID) // Remember where THIS device kept the game before the record goes. A // re-track on the peer used to bring the game back by auto-tracking from @@ -1289,7 +1467,7 @@ func (d *Daemon) watchGame(gameID, savePath string) error { // nobody is covering any more. func (d *Daemon) RewatchGame(gameID string) { game, err := d.Store.GetGame(gameID) - if err != nil || !game.AutoSync { + if err != nil || !game.AutoSync || d.provisioningHeld(gameID) { return } if err := d.watchGame(game.ID, game.SavePath); err != nil { diff --git a/internal/p2p/engine.go b/internal/p2p/engine.go index b1777e2e..b33ad87c 100644 --- a/internal/p2p/engine.go +++ b/internal/p2p/engine.go @@ -444,6 +444,11 @@ func (e *Engine) SyncGame(ctx context.Context, gameID string) (map[string]syncen if e.Pause.Paused() { return nil, syncengine.ErrPaused } + // Before pinging anyone. A held game must not be the reason a peer is + // contacted, and the refusal must not depend on a peer being online. + if e.provisioningHeld(gameID) || e.provisioningHeld(e.localGameID(gameID)) { + return nil, syncengine.ErrProvisioning + } gameID = e.localGameID(gameID) e.PingPairedPeers(ctx) online := e.OnlinePeers() @@ -602,6 +607,12 @@ func (e *Engine) retryPendingResyncs(ctx context.Context) { if ctx.Err() != nil { return // shutting down; the failsafe picks these up next start } + if e.provisioningHeld(id) { + e.pendingMu.Lock() + delete(e.pendingResync, id) + e.pendingMu.Unlock() + continue + } e.Log("info", fmt.Sprintf("retrying interrupted sync for %s", id)) results, err := e.Sync.SyncGame(ctx, id, online) if err == nil { @@ -616,6 +627,21 @@ func (e *Engine) retryPendingResyncs(ctx context.Context) { } } +func (e *Engine) provisioningHeld(gameID string) bool { + if e == nil || e.Store == nil || gameID == "" { + return false + } + held, err := e.Store.ProvisioningHeld(gameID) + if err == nil && held { + return true + } + if canonical, ok := e.Store.ResolveGameAlias(gameID); ok && canonical != gameID { + held, err = e.Store.ProvisioningHeld(canonical) + return err == nil && held + } + return false +} + // SyncAllGames syncs every tracked game (used when a peer comes online). func (e *Engine) SyncAllGames(ctx context.Context) { if e.Pause.Paused() { @@ -629,8 +655,9 @@ func (e *Engine) SyncAllGames(ctx context.Context) { if len(online) == 0 { return } + held, _ := e.Store.ProvisioningHeldSet() for _, g := range games { - if !g.AutoSync { + if _, skip := held[g.ID]; !g.AutoSync || skip { continue } results, err := e.Sync.SyncGame(ctx, g.ID, online) diff --git a/internal/p2p/routes.go b/internal/p2p/routes.go index 67d11a5c..9ae74ce6 100644 --- a/internal/p2p/routes.go +++ b/internal/p2p/routes.go @@ -359,8 +359,14 @@ func (e *Engine) PeerGameList() []PeerGame { if err != nil { return []PeerGame{} } + held, _ := e.Store.ProvisioningHeldSet() out := make([]PeerGame, 0, len(games)) for _, g := range games { + // A game still being configured is not offered for linking. Linking + // would publish its id, and the peer would then ask for its saves. + if _, skip := held[g.ID]; skip { + continue + } out = append(out, PeerGame{ID: g.ID, Name: g.Name, SavePath: g.SavePath, AppID: g.AppID}) } return out @@ -678,6 +684,10 @@ func (e *Engine) handleManifest(w http.ResponseWriter, r *http.Request) { jsonError(w, http.StatusNotFound, syncengine.HeldMessage) return } + if e.provisioningHeld(game.ID) { + jsonError(w, http.StatusConflict, syncengine.ProvisioningMessage) + return + } // Never describe a save a sync here is writing: part-way through, it is a // mixture no device holds, and the asker would judge it as a save that had // moved (syncengine/settle.go). Held still while it is read; a write in @@ -759,6 +769,10 @@ func (e *Engine) handleBlocks(w http.ResponseWriter, r *http.Request) { jsonError(w, http.StatusNotFound, "Game not found.") return } + if e.provisioningHeld(game.ID) { + jsonError(w, http.StatusConflict, syncengine.ProvisioningMessage) + return + } base, ok := e.resolveServeRoot(gameID, game, body.Root) if !ok { jsonError(w, http.StatusNotFound, "This device has no save location named "+strconv.Quote(body.Root)+" for that game.") @@ -803,6 +817,10 @@ func (e *Engine) handleDeleteFile(w http.ResponseWriter, r *http.Request) { jsonError(w, http.StatusNotFound, "Game not found.") return } + if e.provisioningHeld(game.ID) { + jsonError(w, http.StatusConflict, syncengine.ProvisioningMessage) + return + } base, ok := e.resolveServeRoot(gameID, game, body.Root) if !ok { jsonError(w, http.StatusNotFound, "This device has no save location named "+strconv.Quote(body.Root)+" for that game.") @@ -962,10 +980,14 @@ func (e *Engine) peerByAddress(ip string) (syncengine.Peer, bool) { // newer content for us to pull. func (e *Engine) handleSyncTrigger(w http.ResponseWriter, r *http.Request) { gameID := chi.URLParam(r, "gameId") + if e.provisioningHeld(gameID) { + jsonError(w, http.StatusConflict, syncengine.ProvisioningMessage) + return + } e.GoSync(func(ctx context.Context) { ctx, cancel := context.WithTimeout(ctx, 10*time.Minute) defer cancel() - if _, err := e.SyncGame(ctx, gameID); err != nil && !errors.Is(err, syncengine.ErrHeld) { + if _, err := e.SyncGame(ctx, gameID); err != nil && !errors.Is(err, syncengine.ErrHeld) && !errors.Is(err, syncengine.ErrProvisioning) { e.Log("warn", fmt.Sprintf("triggered sync for %s: %v", gameID, err)) } }) diff --git a/internal/p2p/syncengine/engine.go b/internal/p2p/syncengine/engine.go index e3b02b00..77133a5d 100644 --- a/internal/p2p/syncengine/engine.go +++ b/internal/p2p/syncengine/engine.go @@ -70,7 +70,9 @@ type Result struct { // peer_missing: the peer does not track this game. peer_awaiting_folder: // the peer knows about it but is waiting for someone to choose a folder. // peer_holding: the peer's save was emptied and it is holding the game - // back until told whether that was meant (hold.go). peer_busy: the peer was + // back until told whether that was meant (hold.go). peer_provisioning: + // the peer is still configuring the game and must not be read as empty. + // peer_busy: the peer was // still writing the game for a sync of its own; this one runs again when it // has finished (settle.go). Status string `json:"status"` // in_sync | updated | updated_bidirectional | deletions_synced | triggered_peer_pull | conflict | peer_missing | peer_awaiting_folder | peer_holding | peer_busy @@ -228,6 +230,21 @@ func (e *Engine) SyncBusy(gameID string) bool { return e.activeSyncs[gameID] || e.pendingSyncs[gameID] || e.followUps[gameID] > 0 } +func (e *Engine) provisioningHeld(gameID string) bool { + if e == nil || e.Store == nil || gameID == "" { + return false + } + held, err := e.Store.ProvisioningHeld(gameID) + if err == nil && held { + return true + } + if canonical, ok := e.Store.ResolveGameAlias(gameID); ok && canonical != gameID { + held, err = e.Store.ProvisioningHeld(canonical) + return err == nil && held + } + return false +} + func (e *Engine) SyncGame(ctx context.Context, gameID string, onlinePeers []Peer) (map[string]Result, error) { // Every sync this device starts comes through here — a watched change, a // peer coming online, the periodic reconcile, a sync asked for by hand or @@ -240,6 +257,11 @@ func (e *Engine) SyncGame(ctx context.Context, gameID string, onlinePeers []Peer if held, err := e.CheckHold(gameID, false); err == nil && held { return nil, ErrHeld } + // A game still being configured does not sync, whichever caller got + // here: a watch, a retry, a peer trigger, or a sync asked for by hand. + if e.provisioningHeld(gameID) { + return nil, ErrProvisioning + } e.mu.Lock() if e.activeSyncs[gameID] { e.pendingSyncs[gameID] = true @@ -335,7 +357,7 @@ func (e *Engine) SyncGame(ctx context.Context, gameID string, onlinePeers []Peer // its own changes instead of detecting the conflict and asking. switch res.Status { case "conflict", "error": - case "peer_missing", "peer_awaiting_folder", "peer_holding": + case "peer_missing", "peer_awaiting_folder", "peer_holding", "peer_provisioning": // The two devices talked and finished, which is what the // per-device stamp has always recorded. But nothing of THIS game // moved — the peer does not track it, or is still waiting to be @@ -430,6 +452,12 @@ func (e *Engine) SyncWithPeer(ctx context.Context, gameID string, peer Peer) (Re if isHeld(err) { return Result{Status: "peer_holding", PeerID: peer.ID, PeerName: peer.Name}, nil } + // The peer has the game and is still configuring it. Nothing is + // taken from it and nothing is sent: an empty answer would be read + // as a deleted save. + if isProvisioning(err) { + return Result{Status: "peer_provisioning", PeerID: peer.ID, PeerName: peer.Name}, nil + } if isGameNotFound(err) { return Result{Status: "peer_missing", PeerID: peer.ID, PeerName: peer.Name}, nil } diff --git a/internal/p2p/syncengine/hold.go b/internal/p2p/syncengine/hold.go index 7060c241..4c768f8d 100644 --- a/internal/p2p/syncengine/hold.go +++ b/internal/p2p/syncengine/hold.go @@ -64,6 +64,21 @@ const HeldMessage = "This device is holding this game back: its save files were // ErrHeld is SyncGame declining to sync a game that is held back. var ErrHeld = errors.New("its save files were all deleted here; it is not synced until you say whether that was meant") +// ProvisioningMessage is what this device says when a peer asks for a game +// that is still being configured. It must not contain "not found": that +// phrase is how the asker decides the peer does not track the game, and a +// provisioning hold is not an untrack. It is also distinct from HeldMessage, +// which means the save was emptied. +const ProvisioningMessage = "This device is still configuring this game and is not sending or receiving its saves yet" + +// ErrProvisioning is SyncGame declining to sync a game that is still being +// configured. It is not ErrHeld: that one is the emptied-save question. +var ErrProvisioning = errors.New("this game is still being configured and is not synced until that finishes") + +func isProvisioning(err error) bool { + return err != nil && strings.Contains(err.Error(), ProvisioningMessage) +} + // ErrNoHold is an answer to a question nobody asked: the game is not held. var ErrNoHold = errors.New("that game is not held back") diff --git a/internal/p2p/wanclient_handlers.go b/internal/p2p/wanclient_handlers.go index 95203dd3..f6f98dbe 100644 --- a/internal/p2p/wanclient_handlers.go +++ b/internal/p2p/wanclient_handlers.go @@ -555,7 +555,7 @@ func (w *WanClient) routeRequest(ctx context.Context, msg RelayMessage) (int, an w.engine.GoSync(func(ctx context.Context) { syncCtx, cancel := context.WithTimeout(ctx, 10*time.Minute) defer cancel() - if _, err := w.engine.SyncGame(syncCtx, gameID); err != nil && !errors.Is(err, syncengine.ErrPaused) && !errors.Is(err, syncengine.ErrHeld) { + if _, err := w.engine.SyncGame(syncCtx, gameID); err != nil && !errors.Is(err, syncengine.ErrPaused) && !errors.Is(err, syncengine.ErrHeld) && !errors.Is(err, syncengine.ErrProvisioning) { w.engine.Log("warn", fmt.Sprintf("WAN-triggered sync %s: %v", gameID, err)) } }) @@ -603,6 +603,9 @@ func (w *WanClient) serveManifest(route string, body json.RawMessage, peerID str if w.engine.holdingBack(game) { return 404, map[string]string{"error": syncengine.HeldMessage} } + if w.engine.provisioningHeld(game.ID) { + return 409, map[string]string{"error": syncengine.ProvisioningMessage} + } // As on the LAN: a save a sync here is writing is not described // (syncengine/settle.go). Each request runs on its own goroutine, so the // wait does not hold up the relay traffic that finishes the write. @@ -645,6 +648,9 @@ func (w *WanClient) serveBlocks(route string, rawBody json.RawMessage) (int, any if err != nil { return 404, map[string]string{"error": "Game not found."} } + if w.engine.provisioningHeld(game.ID) { + return 409, map[string]string{"error": syncengine.ProvisioningMessage} + } if !delta.IsSafePath(game.SavePath, body.RelPath) { return 403, map[string]string{"error": "Access denied: path traversal attempt detected."} } @@ -686,6 +692,9 @@ func (w *WanClient) serveDeleteFile(route string, rawBody json.RawMessage, fromP if err != nil { return 404, map[string]string{"error": "Game not found."} } + if w.engine.provisioningHeld(game.ID) { + return 409, map[string]string{"error": syncengine.ProvisioningMessage} + } if !delta.IsSafePath(game.SavePath, body.RelPath) { return 403, map[string]string{"error": "invalid path"} } @@ -732,6 +741,9 @@ func (w *WanClient) serveSnapshotDownload(route string) (int, any) { if err != nil { return 404, map[string]string{"error": "Snapshot ZIP file not found."} } + if w.engine.provisioningHeld(snap.GameID) { + return 409, map[string]string{"error": syncengine.ProvisioningMessage} + } archive, done, err := snapshot.OpenArchive(snap.ZipPath) if err != nil { return 404, map[string]string{"error": "Snapshot ZIP file not found."} diff --git a/testutil/harness.go b/testutil/harness.go index 9323e7b4..0427a3ab 100644 --- a/testutil/harness.go +++ b/testutil/harness.go @@ -176,12 +176,59 @@ func NewTestDaemon(t *testing.T, name string) *TestDaemon { td := &TestDaemon{T: t, Daemon: d, Server: srv, Addr: addr, Port: port, SaveDir: saveDir} t.Cleanup(func() { - srv.Stop() - d.Stop() + td.Stop() }) return td } +// Stop shuts down whatever daemon this handle currently points at. +func (td *TestDaemon) Stop() { + if td.Server != nil { + td.Server.Stop() + td.Server = nil + } + if td.Daemon != nil { + td.Daemon.Stop() + td.Daemon = nil + } +} + +// Restart replaces the process the way an unmanaged `opensave daemon start` +// does: daemon.New + Start on the same data directory and the same port. +// In-memory pause does not survive. The persisted store does. +func (td *TestDaemon) Restart() { + td.T.Helper() + home := td.Daemon.Paths.HomeDir + port := td.Port + saveDir := td.SaveDir + td.Stop() + + d, err := daemon.New(daemon.Options{HomeOverride: home, DisableDiscovery: true}) + if err != nil { + td.T.Fatalf("restart daemon.New: %v", err) + } + d.Scanner.ManifestURL = "" + d.Scanner.SteamUserdataPaths = []string{} + d.Scanner.SteamRoots = []string{} + d.Scanner.LocalLowDir = filepath.Join(home, "no-locallow") + d.Scanner.ResolveAppName = nil + if err := d.Start(); err != nil { + d.Stop() + td.T.Fatalf("restart daemon.Start: %v", err) + } + srv := api.New(d) + addr, err := srv.Start(port) + if err != nil { + d.Stop() + td.T.Fatalf("restart api.Start: %v", err) + } + td.Daemon = d + td.Server = srv + td.Addr = addr + td.Port = port + td.SaveDir = saveDir +} + // API performs a JSON request against this daemon's API and decodes the // response into out (out may be nil). Non-2xx responses fail the test // unless allowError is set. From 3b760e2fdfc6fd51ff0c51c5b902d3582280352e Mon Sep 17 00:00:00 2001 From: Seagulls Date: Thu, 8 Oct 2026 23:13:04 +0100 Subject: [PATCH 3/7] test: prove a held game survives restart and one-sided release Cover paired creation without an offer, unmanaged daemon.New restart, inbound refusal, offer placement, symlink and duplicate paths, named roots, and a 100-game library that still syncs an unrelated title. --- CHANGELOG.md | 14 ++ e2e/provisioning_hold_test.go | 433 ++++++++++++++++++++++++++++++++++ 2 files changed, 447 insertions(+) create mode 100644 e2e/provisioning_hold_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d5300ce..98a4105d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,20 @@ All notable changes to OpenSave are documented here. This project adheres to [Semantic Versioning](https://semver.org/). +## [Unreleased] + +### Added + +- **A game can be tracked or placed without syncing until it is released.** + `provisioningHold: true` (or `autoSync: false`) on `POST /api/games` and + `POST /api/offered-games/{id}/place` records the game and a hold in one + step. The hold survives a daemon restart and is not the ordinary AutoSync + flag: peers cannot pull or push that game's saves, and releasing one game + does not sync the rest of the library. `POST /api/games/{id}/release-provisioning` + clears it. An explicit game id is accepted only while creating a hold, so + two devices can be given the same id without waiting for an offer. + See issue #40. + ## [2.4.1] — 2026-10-02 A security update: please install it on every device. A paired device could diff --git a/e2e/provisioning_hold_test.go b/e2e/provisioning_hold_test.go new file mode 100644 index 00000000..d727704b --- /dev/null +++ b/e2e/provisioning_hold_test.go @@ -0,0 +1,433 @@ +package e2e + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "net/http" + "os" + "path/filepath" + "testing" + "time" + + "github.com/opensave/opensave/internal/p2p/syncengine" + "github.com/opensave/opensave/internal/store" + "github.com/opensave/opensave/testutil" +) + +func sideDir(t *testing.T, td *testutil.TestDaemon, name string) string { + t.Helper() + dir := filepath.Join(filepath.Dir(td.SaveDir), name) + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + return dir +} + +func writeHoldFile(t *testing.T, dir, rel, content string) { + t.Helper() + full := filepath.Join(dir, filepath.FromSlash(rel)) + if err := os.MkdirAll(filepath.Dir(full), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte(content), 0o600); err != nil { + t.Fatal(err) + } +} + +func readHoldFile(dir, rel string) string { + raw, err := os.ReadFile(filepath.Join(dir, filepath.FromSlash(rel))) + if err != nil { + return "" + } + return string(raw) +} + +func fileHash(dir, rel string) string { + raw, err := os.ReadFile(filepath.Join(dir, filepath.FromSlash(rel))) + if err != nil { + return "" + } + sum := sha256.Sum256(raw) + return hex.EncodeToString(sum[:]) +} + +type holdGame struct { + ID string `json:"id"` + AutoSync bool `json:"autoSync"` + ProvisioningHold bool `json:"provisioningHold"` + SavePath string `json:"savePath"` +} + +func createHeld(t *testing.T, td *testutil.TestDaemon, id, name, path string) holdGame { + t.Helper() + var got holdGame + td.API(http.MethodPost, "/api/games", map[string]any{ + "id": id, "name": name, "savePath": path, "provisioningHold": true, + }, &got) + if got.ID != id || got.AutoSync || !got.ProvisioningHold { + t.Fatalf("held create = %+v", got) + } + held, err := td.Daemon.Store.ProvisioningHeld(id) + if err != nil || !held { + t.Fatalf("hold not committed for %s: %v", id, err) + } + return got +} + +func assertHeldQuiet(t *testing.T, source, bridge *testutil.TestDaemon, id, sourceDir, bridgeDir, marker string) { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + source.Daemon.P2P.PingPairedPeers(ctx) + source.Daemon.P2P.SyncAllGames(ctx) + bridge.Daemon.P2P.PingPairedPeers(ctx) + bridge.Daemon.P2P.SyncAllGames(ctx) + if _, err := source.Daemon.P2P.Sync.SyncGame(ctx, id, source.Daemon.P2P.OnlinePeers()); err != nil && err != syncengine.ErrProvisioning { + // A missing peer is not a transfer. A provisioning refusal is the + // gate. Any other error is reported, but a successful sync is not. + if err.Error() != syncengine.ErrProvisioning.Error() { + t.Logf("direct syncengine sync: %v", err) + } + } + var syncResp struct { + Error string `json:"error"` + Reason string `json:"reason"` + } + status := source.APIStatus(http.MethodPost, "/api/games/"+id+"/sync", nil, &syncResp) + if status < 400 || syncResp.Reason != "provisioning" { + t.Fatalf("direct sync of a held game returned %d %+v", status, syncResp) + } + if readHoldFile(bridgeDir, marker) != "" || readHoldFile(sourceDir, "from-bridge.sav") != "" { + t.Fatalf("held game transferred: bridge=%q source=%q", readHoldFile(bridgeDir, marker), readHoldFile(sourceDir, "from-bridge.sav")) + } + if _, err := bridge.Daemon.Store.GetGame(id); err != nil { + t.Fatal("bridge lost the held game") + } +} + +// A paired source can record a new game without the bridge learning it, and +// the bridge can record the same id itself. Restart, reconcile, a direct +// sync and the retry path must not move bytes. An unrelated game still syncs. +func TestProvisioningHold_PairedCreateRestartAndIsolation(t *testing.T) { + source := testutil.NewTestDaemon(t, "Hold-Source") + bridge := testutil.NewTestDaemon(t, "Hold-Bridge") + + unrelatedSrc := sideDir(t, source, "unrelated") + unrelatedBr := sideDir(t, bridge, "unrelated") + writeHoldFile(t, unrelatedSrc, "keep/unrelated.sav", "ONLY-UNRELATED") + writeHoldFile(t, unrelatedBr, "keep/unrelated.sav", "ONLY-UNRELATED") + source.API(http.MethodPost, "/api/games", map[string]string{"name": "Hold Unrelated", "savePath": unrelatedSrc}, nil) + bridge.API(http.MethodPost, "/api/games", map[string]string{"name": "Hold Unrelated", "savePath": unrelatedBr}, nil) + source.PairWith(bridge) + + newSrc := sideDir(t, source, "new-game") + newBr := sideDir(t, bridge, "new-game") + writeHoldFile(t, newSrc, "profile/source.sav", "ONLY-SOURCE") + const id = "hold-new-game" + createHeld(t, source, id, "Hold New Game", newSrc) + // No offer is required. The bridge is given the source's id directly. + offers, _ := bridge.Daemon.Store.ListOfferedGames() + if len(offers) != 0 { + t.Fatalf("held source published an offer: %+v", offers) + } + createHeld(t, bridge, id, "Hold New Game", newBr) + + writeHoldFile(t, unrelatedSrc, "keep/fresh.sav", "UNRELATED-FRESH") + if !testutil.WaitFor(30*time.Second, func() bool { + source.API(http.MethodPost, "/api/games/"+store.SlugifyGameID("Hold Unrelated")+"/sync", nil, nil) + return readHoldFile(unrelatedBr, "keep/fresh.sav") == "UNRELATED-FRESH" + }) { + t.Fatal("unrelated game stopped syncing while another game was held") + } + if readHoldFile(newBr, "profile/source.sav") != "" { + t.Fatal("unrelated sync also copied the held game") + } + + // Crash after the row is committed: the next process is a fresh daemon.New. + source.Restart() + bridge.Restart() + for _, node := range []*testutil.TestDaemon{source, bridge} { + if node.Daemon.SyncPauseStatus().Paused { + t.Fatal("restart kept an in-memory pause; the hold must not depend on one") + } + held, err := node.Daemon.Store.ProvisioningHeld(id) + if err != nil || !held { + t.Fatalf("%s lost the hold across restart: %v", node.Name(), err) + } + } + assertHeldQuiet(t, source, bridge, id, newSrc, newBr, "profile/source.sav") + + var manifest struct { + Error string `json:"error"` + } + if status := bridge.APIStatus(http.MethodGet, "/api/p2p/manifest/"+id, nil, &manifest); status < 400 || manifest.Error != syncengine.ProvisioningMessage { + t.Fatalf("manifest of a held game = %d %q", status, manifest.Error) + } + if manifest.Error != "" && (len(manifest.Error) > 0 && containsNotFound(manifest.Error)) { + t.Fatalf("provisioning refusal looks like not-found: %q", manifest.Error) + } + writeHoldFile(t, newBr, "profile/bridge.sav", "ONLY-BRIDGE") + var deleted struct { + Error string `json:"error"` + } + if status := bridge.APIStatus(http.MethodPost, "/api/p2p/delete-file/"+id, map[string]string{"relPath": "profile/bridge.sav"}, &deleted); status < 400 { + t.Fatalf("delete-file on a held game returned %d", status) + } + if readHoldFile(newBr, "profile/bridge.sav") != "ONLY-BRIDGE" { + t.Fatal("inbound delete changed a held save") + } + + // One side released, then both processes replaced. The other hold remains. + var rel struct { + Released bool `json:"released"` + } + source.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", nil, &rel) + if !rel.Released { + t.Fatal("source release did not clear the hold") + } + source.Restart() + bridge.Restart() + held, err := bridge.Daemon.Store.ProvisioningHeld(id) + if err != nil || !held { + t.Fatal("bridge hold did not survive a restart after only the source was released") + } + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + source.Daemon.P2P.PingPairedPeers(ctx) + source.Daemon.P2P.SyncAllGames(ctx) + if readHoldFile(newBr, "profile/source.sav") != "" || readHoldFile(newSrc, "profile/bridge.sav") != "" { + t.Fatal("one-sided release transferred saves") + } + + bridge.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", nil, &rel) + if !testutil.WaitFor(45*time.Second, func() bool { + source.API(http.MethodPost, "/api/games/"+id+"/sync", nil, nil) + return readHoldFile(newBr, "profile/source.sav") == "ONLY-SOURCE" && + fileHash(newSrc, "profile/source.sav") == fileHash(newBr, "profile/source.sav") + }) { + t.Fatalf("release did not sync: bridge=%q", readHoldFile(newBr, "profile/source.sav")) + } + if readHoldFile(unrelatedBr, "keep/fresh.sav") != "UNRELATED-FRESH" { + t.Fatal("release disturbed the unrelated game") + } +} + +func containsNotFound(s string) bool { + return len(s) >= 9 && (s == "Game not found." || (len(s) > 0 && (containsFold(s, "not found")))) +} + +func containsFold(s, sub string) bool { + return len(s) >= len(sub) && (s == sub || len(sub) == 0 || (func() bool { + for i := 0; i+len(sub) <= len(s); i++ { + if equalFoldASCII(s[i:i+len(sub)], sub) { + return true + } + } + return false + })()) +} + +func equalFoldASCII(a, b string) bool { + if len(a) != len(b) { + return false + } + for i := 0; i < len(a); i++ { + ca, cb := a[i], b[i] + if ca >= 'A' && ca <= 'Z' { + ca += 'a' - 'A' + } + if cb >= 'A' && cb <= 'Z' { + cb += 'a' - 'A' + } + if ca != cb { + return false + } + } + return true +} + +func TestProvisioningHold_PlaceOfferWithoutSyncing(t *testing.T) { + source := testutil.NewTestDaemon(t, "Place-Source") + bridge := testutil.NewTestDaemon(t, "Place-Bridge") + source.PairWith(bridge) + setUnknownGamePolicy(t, bridge, store.UnknownGameAsk) + + source.WriteSave("slot.sav", "FROM-SOURCE") + gameID := source.TrackGame("Place Held Game") + syncTo(source, gameID, bridge.NodeID()) + if !testutil.WaitFor(20*time.Second, func() bool { + offers, _ := bridge.Daemon.Store.ListOfferedGames() + return len(offers) == 1 && offers[0].GameID == gameID + }) { + t.Fatal("bridge did not record an offer to place") + } + + dest := sideDir(t, bridge, "placed") + var placed holdGame + bridge.API(http.MethodPost, "/api/offered-games/"+gameID+"/place", map[string]any{ + "path": dest, "autoSync": false, + }, &placed) + if placed.ID != gameID || !placed.ProvisioningHold || placed.AutoSync { + t.Fatalf("held place = %+v", placed) + } + bridge.API(http.MethodPost, "/api/offered-games/"+gameID+"/place", map[string]any{ + "path": dest, "provisioningHold": true, + }, &placed) + games, _ := bridge.Daemon.Store.ListGames() + n := 0 + for _, g := range games { + if g.ID == gameID { + n++ + } + } + if n != 1 { + t.Fatalf("repeated place created %d games", n) + } + source.API(http.MethodPost, "/api/games/"+gameID+"/sync", nil, nil) + if readHoldFile(dest, "slot.sav") != "" { + t.Fatal("placing with autoSync false synced the save") + } + + bridge.API(http.MethodPost, "/api/games/"+gameID+"/release-provisioning", nil, nil) + if !testutil.WaitFor(45*time.Second, func() bool { + source.API(http.MethodPost, "/api/games/"+gameID+"/sync", nil, nil) + return readHoldFile(dest, "slot.sav") == "FROM-SOURCE" + }) { + t.Fatalf("release did not deliver the save, dest=%q", readHoldFile(dest, "slot.sav")) + } +} + +func TestProvisioningHold_RejectsUnsafePathsAndDuplicates(t *testing.T) { + node := testutil.NewTestDaemon(t, "Hold-Paths") + live := sideDir(t, node, "live") + node.API(http.MethodPost, "/api/games", map[string]string{"name": "Live Occupant", "savePath": live}, nil) + + var errBody struct { + Error string `json:"error"` + } + if status := node.APIStatus(http.MethodPost, "/api/games", map[string]any{ + "id": "other-id", "name": "Other", "savePath": live, "provisioningHold": true, + }, &errBody); status < 400 { + t.Fatal("held create accepted an occupied folder") + } + if g, err := node.Daemon.Store.GetGame("other-id"); err == nil { + t.Fatalf("occupied create still inserted %+v", g) + } + + outside := sideDir(t, node, "outside") + link := filepath.Join(filepath.Dir(node.SaveDir), "escape-link") + if err := os.Symlink(outside, link); err != nil { + t.Fatal(err) + } + if status := node.APIStatus(http.MethodPost, "/api/games", map[string]any{ + "id": "symlink-game", "name": "Symlink", "savePath": link, "provisioningHold": true, + }, &errBody); status < 400 { + t.Fatal("held create accepted a symlink") + } + + primary := sideDir(t, node, "primary") + createHeld(t, node, "dup-game", "Dup", primary) + other := sideDir(t, node, "other") + if status := node.APIStatus(http.MethodPost, "/api/games", map[string]any{ + "id": "dup-game", "name": "Dup", "savePath": other, "provisioningHold": true, + }, &errBody); status < 400 { + t.Fatal("a second path for a held id was accepted") + } + again := createHeld(t, node, "dup-game", "Dup", primary) + if again.SavePath == "" { + t.Fatal("retry of the same held create failed") + } + if status := node.APIStatus(http.MethodPost, "/api/games", map[string]any{ + "id": "pinned", "name": "Pinned", "savePath": primary, + }, &errBody); status != http.StatusBadRequest { + t.Fatalf("explicit id without a hold returned %d", status) + } +} + +func TestProvisioningHold_NamedRootsBeforeRelease(t *testing.T) { + source := testutil.NewTestDaemon(t, "Roots-Source") + bridge := testutil.NewTestDaemon(t, "Roots-Bridge") + source.PairWith(bridge) + const id = "roots-game" + src := sideDir(t, source, "main") + br := sideDir(t, bridge, "main") + srcA, srcB := sideDir(t, source, "root-a"), sideDir(t, source, "root-b") + brA, brB := sideDir(t, bridge, "root-a"), sideDir(t, bridge, "root-b") + writeHoldFile(t, src, "main.sav", "MAIN") + writeHoldFile(t, srcA, "a.sav", "ROOT-A") + writeHoldFile(t, srcB, "b.sav", "ROOT-B") + createHeld(t, source, id, "Roots Game", src) + createHeld(t, bridge, id, "Roots Game", br) + for _, spec := range []struct { + node *testutil.TestDaemon + name, path string + }{ + {source, "config", srcA}, + {source, "mods", srcB}, + {bridge, "config", brA}, + {bridge, "mods", brB}, + } { + spec.node.API(http.MethodPost, "/api/games/"+id+"/roots", map[string]string{"name": spec.name, "path": spec.path}, nil) + } + if status := source.APIStatus(http.MethodPost, "/api/games/"+id+"/sync", nil, nil); status < 400 { + t.Fatalf("sync of a held game returned %d", status) + } + if readHoldFile(br, "main.sav") != "" || readHoldFile(brA, "a.sav") != "" || readHoldFile(brB, "b.sav") != "" { + t.Fatal("named roots synced before release") + } + source.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", nil, nil) + bridge.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", nil, nil) + if !testutil.WaitFor(45*time.Second, func() bool { + source.API(http.MethodPost, "/api/games/"+id+"/sync", nil, nil) + return readHoldFile(br, "main.sav") == "MAIN" && readHoldFile(brA, "a.sav") == "ROOT-A" && readHoldFile(brB, "b.sav") == "ROOT-B" + }) { + t.Fatalf("roots after release: main=%q a=%q b=%q", readHoldFile(br, "main.sav"), readHoldFile(brA, "a.sav"), readHoldFile(brB, "b.sav")) + } +} + +func TestProvisioningHold_HundredGamesStayOneDaemon(t *testing.T) { + source := testutil.NewTestDaemon(t, "Bulk-Source") + bridge := testutil.NewTestDaemon(t, "Bulk-Bridge") + liveSrc := sideDir(t, source, "live") + liveBr := sideDir(t, bridge, "live") + writeHoldFile(t, liveSrc, "live.sav", "LIVE") + writeHoldFile(t, liveBr, "live.sav", "LIVE") + source.API(http.MethodPost, "/api/games", map[string]string{"name": "Bulk Live", "savePath": liveSrc}, nil) + bridge.API(http.MethodPost, "/api/games", map[string]string{"name": "Bulk Live", "savePath": liveBr}, nil) + source.PairWith(bridge) + + start := time.Now() + for i := 0; i < 100; i++ { + id := "bulk-" + hex.EncodeToString([]byte{byte(i)}) + dir := sideDir(t, source, id) + createHeld(t, source, id, "Bulk "+id, dir) + } + if elapsed := time.Since(start); elapsed > 45*time.Second { + t.Fatalf("100 held creates took %s; this must not be a daemon per game", elapsed) + } + writeHoldFile(t, liveSrc, "live.sav", "LIVE-UPDATED") + if !testutil.WaitFor(30*time.Second, func() bool { + source.API(http.MethodPost, "/api/games/"+store.SlugifyGameID("Bulk Live")+"/sync", nil, nil) + return readHoldFile(liveBr, "live.sav") == "LIVE-UPDATED" + }) { + t.Fatal("the live game did not sync beside 100 held games") + } + source.Restart() + set, err := source.Daemon.Store.ProvisioningHeldSet() + if err != nil { + t.Fatal(err) + } + if len(set) != 100 { + t.Fatalf("restart kept %d holds, want 100", len(set)) + } +} + +func TestProvisioningHold_OrdinaryTrackUnchanged(t *testing.T) { + node := testutil.NewTestDaemon(t, "Ordinary") + var got holdGame + node.API(http.MethodPost, "/api/games", map[string]string{"name": "Ordinary Game", "savePath": node.SaveDir}, &got) + if !got.AutoSync || got.ProvisioningHold { + t.Fatalf("ordinary track = %+v; omitting the hold must keep today's behaviour", got) + } +} From 1508845f93bf1f9affe3259c1b35a786fe183fb2 Mon Sep 17 00:00:00 2001 From: Seagulls Date: Fri, 9 Oct 2026 10:30:26 +0100 Subject: [PATCH 4/7] sync: fail closed when a provisioning hold cannot be read A failed SQLite read used to look like "not held", so a protected game could be watched, served, or synced. Hold lookups now return the error, and every transfer path refuses rather than continuing. Release no longer calls SyncGame, which would contact every online peer before the caller has said which pair should converge. --- CHANGELOG.md | 8 +- e2e/provisioning_hold_test.go | 81 +++++++++++++++++++ internal/api/routes.go | 5 +- internal/api/routes_peers.go | 7 +- internal/api/routes_provisioning.go | 7 +- internal/api/routes_roots.go | 7 +- internal/api/server.go | 15 +++- internal/cliapp/commands_games.go | 5 +- internal/daemon/cloud_retry.go | 2 +- internal/daemon/cloudsync.go | 2 +- internal/daemon/daemon.go | 69 +++++++++------- internal/p2p/engine.go | 64 +++++++++++---- internal/p2p/provisioning_fault_test.go | 40 +++++++++ internal/p2p/routes.go | 34 +++++--- internal/p2p/syncengine/engine.go | 25 +++--- internal/p2p/syncengine/hold.go | 9 +++ .../p2p/syncengine/provisioning_fault_test.go | 55 +++++++++++++ internal/p2p/wanclient_handlers.go | 22 ++--- internal/store/db.go | 6 ++ internal/store/provisioning.go | 66 ++++++++++++++- internal/store/provisioning_test.go | 33 ++++++++ 21 files changed, 468 insertions(+), 94 deletions(-) create mode 100644 internal/p2p/provisioning_fault_test.go create mode 100644 internal/p2p/syncengine/provisioning_fault_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 98a4105d..7f09e886 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,9 +13,11 @@ All notable changes to OpenSave are documented here. This project adheres to step. The hold survives a daemon restart and is not the ordinary AutoSync flag: peers cannot pull or push that game's saves, and releasing one game does not sync the rest of the library. `POST /api/games/{id}/release-provisioning` - clears it. An explicit game id is accepted only while creating a hold, so - two devices can be given the same id without waiting for an offer. - See issue #40. + clears the hold and does not sync by itself. The caller syncs afterwards, + so release does not contact every online peer. A third device that is still + held cannot send or receive that game's saves. An explicit game id is + accepted only while creating a hold. A failed hold lookup is not treated as + "not held". See issue #40. ## [2.4.1] — 2026-10-02 diff --git a/e2e/provisioning_hold_test.go b/e2e/provisioning_hold_test.go index d727704b..13f5095f 100644 --- a/e2e/provisioning_hold_test.go +++ b/e2e/provisioning_hold_test.go @@ -423,6 +423,87 @@ func TestProvisioningHold_HundredGamesStayOneDaemon(t *testing.T) { } } +func TestProvisioningHold_DoesNotConvertAnExistingGame(t *testing.T) { + node := testutil.NewTestDaemon(t, "Hold-Existing") + var live holdGame + node.API(http.MethodPost, "/api/games", map[string]string{ + "name": "Already Live", "savePath": node.SaveDir, + }, &live) + writeHoldFile(t, node.SaveDir, "keep.sav", "LIVE-BYTES") + var errBody struct { + Error string `json:"error"` + } + if status := node.APIStatus(http.MethodPost, "/api/games", map[string]any{ + "id": live.ID, "name": "Already Live", "savePath": node.SaveDir, "provisioningHold": true, + }, &errBody); status < 400 { + t.Fatal("a hold was accepted for a game that was already tracking") + } + got, err := node.Daemon.Store.GetGame(live.ID) + if err != nil { + t.Fatal(err) + } + if !got.AutoSync { + t.Fatal("the failed hold turned AutoSync off") + } + held, err := node.Daemon.Store.ProvisioningHeld(live.ID) + if err != nil || held { + t.Fatalf("existing game held=%v err=%v", held, err) + } + if readHoldFile(node.SaveDir, "keep.sav") != "LIVE-BYTES" { + t.Fatal("the failed hold changed save bytes") + } +} + +// Three peers in a star: ends talk only to the middle. Releasing the two ends +// that should converge must not move the third while it is still held, and +// release itself must not start that transfer. +func TestProvisioningHold_StarLeavesHeldEndpointAlone(t *testing.T) { + bazzite := testutil.NewTestDaemon(t, "Star-Bazzite") + bridge := testutil.NewTestDaemon(t, "Star-Bridge") + deck := testutil.NewTestDaemon(t, "Star-Deck") + bazzite.PairWith(bridge) + deck.PairWith(bridge) + if _, err := bazzite.Daemon.Store.GetPeer(deck.NodeID()); err == nil { + t.Fatal("bazzite is paired with deck; the fixture is not a star") + } + + const id = "star-game" + bz := sideDir(t, bazzite, "star") + br := sideDir(t, bridge, "star") + dk := sideDir(t, deck, "star") + writeHoldFile(t, bz, "progress.sav", "BAZZITE-ONLY") + writeHoldFile(t, dk, "progress.sav", "DECK-ONLY") + createHeld(t, bazzite, id, "Star Game", bz) + createHeld(t, bridge, id, "Star Game", br) + createHeld(t, deck, id, "Star Game", dk) + + var rel struct { + Released bool `json:"released"` + } + bazzite.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", nil, &rel) + bridge.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", nil, &rel) + // Release must not itself copy. The deck is still held. + if readHoldFile(br, "progress.sav") != "" || readHoldFile(dk, "progress.sav") != "DECK-ONLY" { + t.Fatal("release transferred before an explicit sync") + } + if !testutil.WaitFor(45*time.Second, func() bool { + bazzite.API(http.MethodPost, "/api/games/"+id+"/sync", nil, nil) + return readHoldFile(br, "progress.sav") == "BAZZITE-ONLY" && + readHoldFile(dk, "progress.sav") == "DECK-ONLY" && + readHoldFile(bz, "progress.sav") == "BAZZITE-ONLY" + }) { + t.Fatalf("star sync moved a held endpoint or failed to converge the released pair: bridge=%q deck=%q bazzite=%q", + readHoldFile(br, "progress.sav"), readHoldFile(dk, "progress.sav"), readHoldFile(bz, "progress.sav")) + } + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + bridge.Daemon.P2P.PingPairedPeers(ctx) + bridge.Daemon.P2P.SyncAllGames(ctx) + if readHoldFile(dk, "progress.sav") != "DECK-ONLY" || readHoldFile(bz, "progress.sav") != "BAZZITE-ONLY" { + t.Fatal("reconcile on the middle peer moved a held endpoint or overwrote the released source") + } +} + func TestProvisioningHold_OrdinaryTrackUnchanged(t *testing.T) { node := testutil.NewTestDaemon(t, "Ordinary") var got holdGame diff --git a/internal/api/routes.go b/internal/api/routes.go index 77bb2fe1..16e4b420 100644 --- a/internal/api/routes.go +++ b/internal/api/routes.go @@ -480,7 +480,10 @@ func (s *Server) handleUpdateGame(w http.ResponseWriter, r *http.Request) { // would sync a game that has not been released. if game.SavePath != oldSavePath || game.AutoSync != oldAutoSync { s.Daemon.Watcher.Unwatch(gameID) - if game.AutoSync && !s.Daemon.StoreProvisioningHeld(gameID) { + held, holdErr := s.Daemon.StoreProvisioningHeld(gameID) + if holdErr != nil { + s.Daemon.Log.Log("warn", "not watching "+gameID+": provisioning hold could not be read: "+holdErr.Error()) + } else if game.AutoSync && !held { if err := s.Daemon.Watcher.Watch(gameID, game.SavePath); err != nil { s.Daemon.Log.Log("warn", "re-watch failed: "+err.Error()) } diff --git a/internal/api/routes_peers.go b/internal/api/routes_peers.go index 53a065a6..0b443e6f 100644 --- a/internal/api/routes_peers.go +++ b/internal/api/routes_peers.go @@ -271,7 +271,12 @@ func (s *Server) handleSyncAll(w http.ResponseWriter, r *http.Request) { } results := map[string]any{} for _, g := range games { - if s.Daemon.StoreProvisioningHeld(g.ID) { + held, holdErr := s.Daemon.StoreProvisioningHeld(g.ID) + if holdErr != nil { + results[g.ID] = map[string]string{"status": "error", "reason": "provisioning-unreadable", "error": holdErr.Error()} + continue + } + if held { results[g.ID] = map[string]string{"status": "skipped", "reason": "provisioning"} continue } diff --git a/internal/api/routes_provisioning.go b/internal/api/routes_provisioning.go index 5f16bde3..12d100d7 100644 --- a/internal/api/routes_provisioning.go +++ b/internal/api/routes_provisioning.go @@ -9,9 +9,10 @@ import ( "github.com/opensave/opensave/internal/store" ) -// handleReleaseProvisioning ends the hold on one game and syncs that game -// only. It does not pause or sync the rest of the library. A repeat after -// the hold is gone is a success and does not sync again. +// handleReleaseProvisioning ends the hold on one game. It does not sync. +// SyncGame would contact every online peer. The caller syncs after releasing +// only the peers that should converge. A repeat after the hold is gone is a +// success and does not sync. func (s *Server) handleReleaseProvisioning(w http.ResponseWriter, r *http.Request) { gameID := chi.URLParam(r, "gameId") released, err := s.Daemon.ReleaseProvisioning(gameID) diff --git a/internal/api/routes_roots.go b/internal/api/routes_roots.go index 2cfd44a8..e3afa21d 100644 --- a/internal/api/routes_roots.go +++ b/internal/api/routes_roots.go @@ -70,7 +70,12 @@ func (s *Server) handleAddGameRoot(w http.ResponseWriter, r *http.Request) { return } path = abs - if s.Daemon.StoreProvisioningHeld(gameID) { + held, holdErr := s.Daemon.StoreProvisioningHeld(gameID) + if holdErr != nil { + writeError(w, http.StatusServiceUnavailable, "could not read whether this game is still being configured") + return + } + if held { if err := daemon.RefuseSymlinkSave(path); err != nil { writeError(w, http.StatusBadRequest, err.Error()) return diff --git a/internal/api/server.go b/internal/api/server.go index 0879a4ec..090ee940 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -514,6 +514,19 @@ func (s *Server) gamesPayload() map[string]any { return out } +// provisioningHoldField is true when the game is held or the hold cannot be +// read. A failed read must not be reported as false. +func (s *Server) provisioningHoldField(id string) bool { + if s == nil || s.Daemon == nil { + return true + } + held, err := s.Daemon.StoreProvisioningHeld(id) + if err != nil { + return true + } + return held +} + func (s *Server) gamePayload(g store.Game) map[string]any { branchNames, _ := s.Daemon.Store.ListBranches(g.ID) branches := map[string]any{} @@ -551,7 +564,7 @@ func (s *Server) gamePayload(g store.Game) map[string]any { "savePath": g.SavePath, "activeBranch": g.ActiveBranch, "autoSync": g.AutoSync, - "provisioningHold": s.Daemon != nil && s.Daemon.StoreProvisioningHeld(g.ID), + "provisioningHold": s.provisioningHoldField(g.ID), "maxSnapshots": g.MaxSnapshots, "maxManualSnapshots": g.MaxManualSnapshots, "appId": g.AppID, diff --git a/internal/cliapp/commands_games.go b/internal/cliapp/commands_games.go index fb1012da..38fd1d2e 100644 --- a/internal/cliapp/commands_games.go +++ b/internal/cliapp/commands_games.go @@ -83,7 +83,10 @@ func cmdGame(d *daemon.Daemon, args []string) int { // Re-watch so a path or auto-sync change takes effect immediately rather // than at the next restart. d.Watcher.Unwatch(game.ID) - if game.AutoSync && !d.StoreProvisioningHeld(game.ID) { + held, holdErr := d.StoreProvisioningHeld(game.ID) + if holdErr != nil { + d.Log.Log("warn", "not watching "+game.ID+": provisioning hold could not be read: "+holdErr.Error()) + } else if game.AutoSync && !held { if err := d.Watcher.Watch(game.ID, game.SavePath); err != nil { d.Log.Log("warn", "re-watch after config change failed: "+err.Error()) } diff --git a/internal/daemon/cloud_retry.go b/internal/daemon/cloud_retry.go index 8b3f83f5..b22e0f0e 100644 --- a/internal/daemon/cloud_retry.go +++ b/internal/daemon/cloud_retry.go @@ -61,7 +61,7 @@ func (d *Daemon) retryFailedUploads(inCloud []cloud.CloudFile) { } d.Log.Log("info", "cloud: sending again "+what+" that did not go up earlier") for _, p := range send { - if id, _, ok := strings.Cut(p.RemoteName, "__"); ok && d.provisioningHeld(id) { + if id, _, ok := strings.Cut(p.RemoteName, "__"); ok && d.provisioningBlocks(id) { continue } d.uploads.Add() diff --git a/internal/daemon/cloudsync.go b/internal/daemon/cloudsync.go index cfe697ea..d41e6357 100644 --- a/internal/daemon/cloudsync.go +++ b/internal/daemon/cloudsync.go @@ -534,7 +534,7 @@ func (d *Daemon) CheckCloud() { if !follows && d.neverHeldFiles(game) { follows, state = true, saveUnchanged } - if follows && state == saveUnchanged && settings.CloudAutoPull && game.AutoSync && !d.provisioningHeld(game.ID) { + if follows && state == saveUnchanged && settings.CloudAutoPull && game.AutoSync && !d.provisioningBlocks(game.ID) { err := d.pullFromCloud(game, cand.file, cand.head.DeviceName) if err == nil { if d.OnCloudPulled != nil { diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go index efa24c7f..1bd7320a 100644 --- a/internal/daemon/daemon.go +++ b/internal/daemon/daemon.go @@ -200,7 +200,7 @@ func New(opts Options) (*Daemon, error) { // A game still being configured must not publish its snapshot. The // name is gameID__branch__snapID.zip (cloudSnapshotName). Checked // before the pause queue: a resume must not send it either. - if id, _, ok := strings.Cut(remoteFileName, "__"); ok && d.provisioningHeld(id) { + if id, _, ok := strings.Cut(remoteFileName, "__"); ok && d.provisioningBlocks(id) { return } if d.P2P.Pause.Paused() { @@ -291,7 +291,7 @@ func (d *Daemon) Start() error { _ = d.Store.UpdateGame(game) } } - if !game.AutoSync || d.provisioningHeld(game.ID) { + if !game.AutoSync || d.provisioningBlocks(game.ID) { continue } if err := d.watchGame(game.ID, game.SavePath); err != nil { @@ -623,7 +623,7 @@ func (d *Daemon) ResyncWatchers() (started, stopped int) { names := make(map[string]string, len(games)) for _, game := range games { names[game.ID] = game.Name - if game.AutoSync && !d.provisioningHeld(game.ID) { + if game.AutoSync && !d.provisioningBlocks(game.ID) { want[game.ID] = game.SavePath } } @@ -934,8 +934,9 @@ func refuseSymlinkSave(path string) error { } // StoreProvisioningHeld reports whether gameID, or an alias of it, is still -// being configured. The API uses it to keep a hold from being watched. -func (d *Daemon) StoreProvisioningHeld(gameID string) bool { +// being configured. A non-nil error means the hold could not be read; that +// is not the same as false. +func (d *Daemon) StoreProvisioningHeld(gameID string) (bool, error) { return d.provisioningHeld(gameID) } @@ -944,24 +945,33 @@ func RefuseSymlinkSave(path string) error { return refuseSymlinkSave(path) } -func (d *Daemon) provisioningHeld(gameID string) bool { - if d == nil || d.Store == nil || gameID == "" { - return false +func (d *Daemon) provisioningHeld(gameID string) (bool, error) { + if d == nil || d.Store == nil { + if gameID == "" { + return false, nil + } + return false, fmt.Errorf("provisioning hold could not be read") } - held, err := d.Store.ProvisioningHeld(gameID) - if err == nil && held { + return d.Store.ProvisioningBlocks(gameID) +} + +// provisioningBlocks is true when the game is held or the hold cannot be +// read. Callers that would otherwise sync, watch, or upload must use this +// and not a bare false. +func (d *Daemon) provisioningBlocks(gameID string) bool { + held, err := d.provisioningHeld(gameID) + if err != nil { + if d.Log != nil { + d.Log.Log("warn", fmt.Sprintf("treating %s as not ready to sync: provisioning hold could not be read: %v", gameID, err)) + } return true } - if canonical, ok := d.Store.ResolveGameAlias(gameID); ok && canonical != gameID { - held, err = d.Store.ProvisioningHeld(canonical) - return err == nil && held - } - return false + return held } -// ReleaseProvisioning clears the hold and lets this game sync. It syncs only -// this game. Other games are not paused and are not part of the release. -// A second call is a no-op and does not sync again. +// ReleaseProvisioning clears the hold and starts watching. It does not sync. +// SyncGame would contact every online peer. The caller syncs after it has +// released only the peers that should converge. A second call is a no-op. func (d *Daemon) ReleaseProvisioning(gameID string) (bool, error) { game, err := d.Store.GetGame(gameID) if err != nil { @@ -975,15 +985,12 @@ func (d *Daemon) ReleaseProvisioning(gameID string) (bool, error) { !errors.Is(err, watcher.ErrStopped) && !errors.Is(err, watcher.ErrSaveFolderMissing) { d.Log.Log("warn", fmt.Sprintf("could not watch %q after release: %v", game.Name, err)) } - d.P2P.GoSync(func(ctx context.Context) { - syncCtx, cancel := context.WithTimeout(ctx, 10*time.Minute) - defer cancel() - if _, err := d.P2P.SyncGame(syncCtx, gameID); err != nil && - !errors.Is(err, syncengine.ErrPaused) && !errors.Is(err, syncengine.ErrHeld) && - !errors.Is(err, syncengine.ErrProvisioning) { - d.Log.Log("info", fmt.Sprintf("sync after releasing %q: %v", game.Name, err)) - } - }) + // Do not SyncGame here. That call contacts every online peer and can + // spread one endpoint's save to another before the caller has released + // only the pair it means to converge. The caller syncs explicitly. + // A peer that is still held still refuses, so a later reconcile cannot + // move that peer's files. + d.Log.Log("info", fmt.Sprintf("released %q; it can sync when asked, and only with peers that are not still being configured", game.Name)) return true, nil } @@ -1210,7 +1217,7 @@ func (d *Daemon) UntrackGame(gameID string) error { name, savePath = game.Name, game.SavePath } // Read before the delete: the hold row cascades away with the game. - held := d.provisioningHeld(gameID) + held := d.provisioningBlocks(gameID) if err := d.Store.DeleteGame(gameID); err != nil { return err } @@ -1250,7 +1257,7 @@ func (d *Daemon) LinkGames(canonicalID, aliasID string) error { if canonicalID == "" || aliasID == "" || canonicalID == aliasID { return fmt.Errorf("invalid link %q -> %q", aliasID, canonicalID) } - if d.provisioningHeld(canonicalID) || d.provisioningHeld(aliasID) { + if d.provisioningBlocks(canonicalID) || d.provisioningBlocks(aliasID) { return fmt.Errorf("a game still being configured cannot be linked") } if _, err := d.Store.GetGame(canonicalID); err != nil { @@ -1371,7 +1378,7 @@ func (d *Daemon) UnlinkGame(aliasID string) error { // untrackFromPeer mirrors a peer's untrack: remove the game + tombstone it, // WITHOUT re-notifying (no loop). func (d *Daemon) untrackFromPeer(gameID string) { - if d.provisioningHeld(gameID) { + if d.provisioningBlocks(gameID) { d.Log.Log("info", fmt.Sprintf("ignored a peer untrack of %q while it is still being configured", gameID)) return } @@ -1467,7 +1474,7 @@ func (d *Daemon) watchGame(gameID, savePath string) error { // nobody is covering any more. func (d *Daemon) RewatchGame(gameID string) { game, err := d.Store.GetGame(gameID) - if err != nil || !game.AutoSync || d.provisioningHeld(gameID) { + if err != nil || !game.AutoSync || d.provisioningBlocks(gameID) { return } if err := d.watchGame(game.ID, game.SavePath); err != nil { diff --git a/internal/p2p/engine.go b/internal/p2p/engine.go index b33ad87c..afa9d370 100644 --- a/internal/p2p/engine.go +++ b/internal/p2p/engine.go @@ -446,8 +446,13 @@ func (e *Engine) SyncGame(ctx context.Context, gameID string) (map[string]syncen } // Before pinging anyone. A held game must not be the reason a peer is // contacted, and the refusal must not depend on a peer being online. - if e.provisioningHeld(gameID) || e.provisioningHeld(e.localGameID(gameID)) { - return nil, syncengine.ErrProvisioning + if err := e.refuseIfProvisioning(gameID); err != nil { + return nil, err + } + if resolved := e.localGameID(gameID); resolved != gameID { + if err := e.refuseIfProvisioning(resolved); err != nil { + return nil, err + } } gameID = e.localGameID(gameID) e.PingPairedPeers(ctx) @@ -607,7 +612,12 @@ func (e *Engine) retryPendingResyncs(ctx context.Context) { if ctx.Err() != nil { return // shutting down; the failsafe picks these up next start } - if e.provisioningHeld(id) { + held, holdErr := e.provisioningHeld(id) + if holdErr != nil { + e.Log("warn", fmt.Sprintf("not retrying %s: provisioning hold could not be read: %v", id, holdErr)) + continue + } + if held { e.pendingMu.Lock() delete(e.pendingResync, id) e.pendingMu.Unlock() @@ -627,19 +637,41 @@ func (e *Engine) retryPendingResyncs(ctx context.Context) { } } -func (e *Engine) provisioningHeld(gameID string) bool { - if e == nil || e.Store == nil || gameID == "" { - return false +func (e *Engine) provisioningHeld(gameID string) (bool, error) { + if e == nil || e.Store == nil { + if gameID == "" { + return false, nil + } + return false, fmt.Errorf("provisioning hold could not be read") } - held, err := e.Store.ProvisioningHeld(gameID) - if err == nil && held { - return true + return e.Store.ProvisioningBlocks(gameID) +} + +// provisioningServeRefusal is the HTTP answer for a peer asking for a game +// that is held, or whose hold cannot be read. refuse is false only when the +// game is confirmed not held. +func (e *Engine) provisioningServeRefusal(gameID string) (refuse bool, status int, msg string) { + held, err := e.provisioningHeld(gameID) + if err != nil { + return true, http.StatusServiceUnavailable, syncengine.ProvisioningUnreadableMessage } - if canonical, ok := e.Store.ResolveGameAlias(gameID); ok && canonical != gameID { - held, err = e.Store.ProvisioningHeld(canonical) - return err == nil && held + if held { + return true, http.StatusConflict, syncengine.ProvisioningMessage } - return false + return false, 0, "" +} + +// refuseIfProvisioning stops a sync when the game is held or the hold cannot +// be read. A lookup failure is not permission to sync. +func (e *Engine) refuseIfProvisioning(gameID string) error { + held, err := e.provisioningHeld(gameID) + if err != nil { + return fmt.Errorf("%w: %v", syncengine.ErrProvisioningUnreadable, err) + } + if held { + return syncengine.ErrProvisioning + } + return nil } // SyncAllGames syncs every tracked game (used when a peer comes online). @@ -655,7 +687,11 @@ func (e *Engine) SyncAllGames(ctx context.Context) { if len(online) == 0 { return } - held, _ := e.Store.ProvisioningHeldSet() + held, err := e.Store.ProvisioningHeldSet() + if err != nil { + e.Log("warn", "not syncing: provisioning holds could not be read: "+err.Error()) + return + } for _, g := range games { if _, skip := held[g.ID]; !g.AutoSync || skip { continue diff --git a/internal/p2p/provisioning_fault_test.go b/internal/p2p/provisioning_fault_test.go new file mode 100644 index 00000000..b9454a3c --- /dev/null +++ b/internal/p2p/provisioning_fault_test.go @@ -0,0 +1,40 @@ +package p2p + +import ( + "context" + "errors" + "path/filepath" + "testing" + + "github.com/opensave/opensave/internal/store" + "github.com/opensave/opensave/internal/syncpause" +) + +// SyncAllGames used to ignore a failed hold-set read and sync every AutoSync +// game. With a peer online and the game row still readable, that is a transfer. +// Sync is nil here: reaching it panics, which is the failure this test wants +// if the early return is removed. +func TestSyncAllGamesStopsWhenHoldSetUnreadable(t *testing.T) { + s, err := store.Open(filepath.Join(t.TempDir(), "opensave.db")) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { s.Close() }) + if err := s.EnsureDefaultSettings(t.TempDir(), t.TempDir()); err != nil { + t.Fatal(err) + } + if err := s.CreateGame(store.Game{ + ID: "live", Name: "Live", SavePath: t.TempDir(), ActiveBranch: "main", + AutoSync: true, MaxSnapshots: 5, + }); err != nil { + t.Fatal(err) + } + if err := s.UpsertPeer(store.Peer{ + ID: "peer", Name: "Peer", Address: "127.0.0.1", Port: 9, Status: "online", + }); err != nil { + t.Fatal(err) + } + s.SetProvisioningReadFault(errors.New("injected hold read failure")) + e := &Engine{Store: s, Pause: syncpause.New(), Log: func(string, string) {}} + e.SyncAllGames(context.Background()) +} diff --git a/internal/p2p/routes.go b/internal/p2p/routes.go index 9ae74ce6..6378efd9 100644 --- a/internal/p2p/routes.go +++ b/internal/p2p/routes.go @@ -354,12 +354,15 @@ type PeerGame struct { // would put a device's whole library on the wire continuously to say // nothing that changes. This is asked for once, when a human opens the // picker. -func (e *Engine) PeerGameList() []PeerGame { +func (e *Engine) PeerGameList() ([]PeerGame, error) { games, err := e.Store.ListGames() if err != nil { - return []PeerGame{} + return nil, err + } + held, err := e.Store.ProvisioningHeldSet() + if err != nil { + return nil, err } - held, _ := e.Store.ProvisioningHeldSet() out := make([]PeerGame, 0, len(games)) for _, g := range games { // A game still being configured is not offered for linking. Linking @@ -369,11 +372,16 @@ func (e *Engine) PeerGameList() []PeerGame { } out = append(out, PeerGame{ID: g.ID, Name: g.Name, SavePath: g.SavePath, AppID: g.AppID}) } - return out + return out, nil } func (e *Engine) handlePeerGameList(w http.ResponseWriter, r *http.Request) { - jsonOK(w, e.PeerGameList()) + list, err := e.PeerGameList() + if err != nil { + jsonError(w, http.StatusServiceUnavailable, syncengine.ProvisioningUnreadableMessage) + return + } + jsonOK(w, list) } // FetchPeerGames asks a paired peer what it is tracking, over whichever @@ -684,8 +692,8 @@ func (e *Engine) handleManifest(w http.ResponseWriter, r *http.Request) { jsonError(w, http.StatusNotFound, syncengine.HeldMessage) return } - if e.provisioningHeld(game.ID) { - jsonError(w, http.StatusConflict, syncengine.ProvisioningMessage) + if refuse, status, msg := e.provisioningServeRefusal(game.ID); refuse { + jsonError(w, status, msg) return } // Never describe a save a sync here is writing: part-way through, it is a @@ -769,8 +777,8 @@ func (e *Engine) handleBlocks(w http.ResponseWriter, r *http.Request) { jsonError(w, http.StatusNotFound, "Game not found.") return } - if e.provisioningHeld(game.ID) { - jsonError(w, http.StatusConflict, syncengine.ProvisioningMessage) + if refuse, status, msg := e.provisioningServeRefusal(game.ID); refuse { + jsonError(w, status, msg) return } base, ok := e.resolveServeRoot(gameID, game, body.Root) @@ -817,8 +825,8 @@ func (e *Engine) handleDeleteFile(w http.ResponseWriter, r *http.Request) { jsonError(w, http.StatusNotFound, "Game not found.") return } - if e.provisioningHeld(game.ID) { - jsonError(w, http.StatusConflict, syncengine.ProvisioningMessage) + if refuse, status, msg := e.provisioningServeRefusal(game.ID); refuse { + jsonError(w, status, msg) return } base, ok := e.resolveServeRoot(gameID, game, body.Root) @@ -980,8 +988,8 @@ func (e *Engine) peerByAddress(ip string) (syncengine.Peer, bool) { // newer content for us to pull. func (e *Engine) handleSyncTrigger(w http.ResponseWriter, r *http.Request) { gameID := chi.URLParam(r, "gameId") - if e.provisioningHeld(gameID) { - jsonError(w, http.StatusConflict, syncengine.ProvisioningMessage) + if refuse, status, msg := e.provisioningServeRefusal(gameID); refuse { + jsonError(w, status, msg) return } e.GoSync(func(ctx context.Context) { diff --git a/internal/p2p/syncengine/engine.go b/internal/p2p/syncengine/engine.go index 77133a5d..3f519129 100644 --- a/internal/p2p/syncengine/engine.go +++ b/internal/p2p/syncengine/engine.go @@ -230,19 +230,14 @@ func (e *Engine) SyncBusy(gameID string) bool { return e.activeSyncs[gameID] || e.pendingSyncs[gameID] || e.followUps[gameID] > 0 } -func (e *Engine) provisioningHeld(gameID string) bool { - if e == nil || e.Store == nil || gameID == "" { - return false - } - held, err := e.Store.ProvisioningHeld(gameID) - if err == nil && held { - return true - } - if canonical, ok := e.Store.ResolveGameAlias(gameID); ok && canonical != gameID { - held, err = e.Store.ProvisioningHeld(canonical) - return err == nil && held +func (e *Engine) provisioningHeld(gameID string) (bool, error) { + if e == nil || e.Store == nil { + if gameID == "" { + return false, nil + } + return false, fmt.Errorf("provisioning hold could not be read") } - return false + return e.Store.ProvisioningBlocks(gameID) } func (e *Engine) SyncGame(ctx context.Context, gameID string, onlinePeers []Peer) (map[string]Result, error) { @@ -259,7 +254,11 @@ func (e *Engine) SyncGame(ctx context.Context, gameID string, onlinePeers []Peer } // A game still being configured does not sync, whichever caller got // here: a watch, a retry, a peer trigger, or a sync asked for by hand. - if e.provisioningHeld(gameID) { + held, holdErr := e.provisioningHeld(gameID) + if holdErr != nil { + return nil, fmt.Errorf("%w: %v", ErrProvisioningUnreadable, holdErr) + } + if held { return nil, ErrProvisioning } e.mu.Lock() diff --git a/internal/p2p/syncengine/hold.go b/internal/p2p/syncengine/hold.go index 4c768f8d..c318d7f4 100644 --- a/internal/p2p/syncengine/hold.go +++ b/internal/p2p/syncengine/hold.go @@ -75,6 +75,15 @@ const ProvisioningMessage = "This device is still configuring this game and is n // configured. It is not ErrHeld: that one is the emptied-save question. var ErrProvisioning = errors.New("this game is still being configured and is not synced until that finishes") +// ProvisioningUnreadableMessage is what this device says when it cannot tell +// whether a game is still being configured. It is not "not found" and it is +// not permission to send or receive the save. +const ProvisioningUnreadableMessage = "This device could not read whether this game is still being configured, so it is not sending or receiving its saves" + +// ErrProvisioningUnreadable is a failed hold lookup. It must not be treated +// as "not held". +var ErrProvisioningUnreadable = errors.New("could not read whether this game is still being configured") + func isProvisioning(err error) bool { return err != nil && strings.Contains(err.Error(), ProvisioningMessage) } diff --git a/internal/p2p/syncengine/provisioning_fault_test.go b/internal/p2p/syncengine/provisioning_fault_test.go new file mode 100644 index 00000000..cb80f184 --- /dev/null +++ b/internal/p2p/syncengine/provisioning_fault_test.go @@ -0,0 +1,55 @@ +package syncengine + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/opensave/opensave/internal/store" +) + +// A hold lookup that fails must not be treated as "not held". The game row +// is still readable, which is the shape that used to let SyncGame continue +// into FetchManifest. +func TestUnreadableProvisioningHoldDoesNotFetchManifest(t *testing.T) { + env := setupEngine(t) + if err := env.store.CreateHeldGame(store.Game{ + ID: "held1", Name: "Held", SavePath: env.localDir, MaxSnapshots: 10, + }); err != nil { + t.Fatal(err) + } + if _, err := env.store.GetGame("held1"); err != nil { + t.Fatal(err) + } + env.store.SetProvisioningReadFault(errors.New("injected hold read failure")) + if _, err := env.store.GetGame("held1"); err != nil { + t.Fatalf("fault leaked into game reads: %v", err) + } + + _, err := env.engine.SyncGame(context.Background(), "held1", []Peer{env.peer}) + if err == nil || !errors.Is(err, ErrProvisioningUnreadable) { + t.Fatalf("sync err = %v, want ErrProvisioningUnreadable", err) + } + if env.transport.manifestCalls != 0 { + t.Fatalf("FetchManifest was called %d times after a failed hold read", env.transport.manifestCalls) + } + if len(env.transport.deletedOnPeer) != 0 { + t.Fatalf("a failed hold read deleted %v", env.transport.deletedOnPeer) + } +} + +// An unpatched 2.4.1 client classifies a manifest error with these helpers. +// A provisioning refusal must not collapse into "not found", which is how a +// peer decides the save is gone. +func TestStockClientDoesNotTreatProvisioningRefusalAsMissing(t *testing.T) { + body := errors.New(`peer returned 409: {"error":"` + ProvisioningMessage + `"}`) + if isGameNotFound(body) || isHeld(body) || isAwaitingFolder(body) { + t.Fatalf("stock classifiers swallowed a provisioning refusal: notFound=%v held=%v awaiting=%v", + isGameNotFound(body), isHeld(body), isAwaitingFolder(body)) + } + unread := errors.New(`peer returned 503: {"error":"` + ProvisioningUnreadableMessage + `"}`) + if isGameNotFound(unread) || strings.Contains(strings.ToLower(unread.Error()), "not found") { + t.Fatal("an unreadable hold looks like the game is missing") + } +} diff --git a/internal/p2p/wanclient_handlers.go b/internal/p2p/wanclient_handlers.go index f6f98dbe..77204852 100644 --- a/internal/p2p/wanclient_handlers.go +++ b/internal/p2p/wanclient_handlers.go @@ -530,7 +530,11 @@ func (w *WanClient) routeRequest(ctx context.Context, msg RelayMessage) (int, an return 200, map[string]any{"success": true} case route == "/games": - return 200, w.engine.PeerGameList() + list, err := w.engine.PeerGameList() + if err != nil { + return 503, map[string]string{"error": syncengine.ProvisioningUnreadableMessage} + } + return 200, list case strings.HasPrefix(route, "/manifest/"): return w.serveManifest(route, msg.Body, msg.From) @@ -603,8 +607,8 @@ func (w *WanClient) serveManifest(route string, body json.RawMessage, peerID str if w.engine.holdingBack(game) { return 404, map[string]string{"error": syncengine.HeldMessage} } - if w.engine.provisioningHeld(game.ID) { - return 409, map[string]string{"error": syncengine.ProvisioningMessage} + if refuse, status, msg := w.engine.provisioningServeRefusal(game.ID); refuse { + return status, map[string]string{"error": msg} } // As on the LAN: a save a sync here is writing is not described // (syncengine/settle.go). Each request runs on its own goroutine, so the @@ -648,8 +652,8 @@ func (w *WanClient) serveBlocks(route string, rawBody json.RawMessage) (int, any if err != nil { return 404, map[string]string{"error": "Game not found."} } - if w.engine.provisioningHeld(game.ID) { - return 409, map[string]string{"error": syncengine.ProvisioningMessage} + if refuse, status, msg := w.engine.provisioningServeRefusal(game.ID); refuse { + return status, map[string]string{"error": msg} } if !delta.IsSafePath(game.SavePath, body.RelPath) { return 403, map[string]string{"error": "Access denied: path traversal attempt detected."} @@ -692,8 +696,8 @@ func (w *WanClient) serveDeleteFile(route string, rawBody json.RawMessage, fromP if err != nil { return 404, map[string]string{"error": "Game not found."} } - if w.engine.provisioningHeld(game.ID) { - return 409, map[string]string{"error": syncengine.ProvisioningMessage} + if refuse, status, msg := w.engine.provisioningServeRefusal(game.ID); refuse { + return status, map[string]string{"error": msg} } if !delta.IsSafePath(game.SavePath, body.RelPath) { return 403, map[string]string{"error": "invalid path"} @@ -741,8 +745,8 @@ func (w *WanClient) serveSnapshotDownload(route string) (int, any) { if err != nil { return 404, map[string]string{"error": "Snapshot ZIP file not found."} } - if w.engine.provisioningHeld(snap.GameID) { - return 409, map[string]string{"error": syncengine.ProvisioningMessage} + if refuse, status, msg := w.engine.provisioningServeRefusal(snap.GameID); refuse { + return status, map[string]string{"error": msg} } archive, done, err := snapshot.OpenArchive(snap.ZipPath) if err != nil { diff --git a/internal/store/db.go b/internal/store/db.go index 9e69aeac..053bd83c 100644 --- a/internal/store/db.go +++ b/internal/store/db.go @@ -11,6 +11,7 @@ import ( "runtime" "sort" "strings" + "sync" "github.com/jmoiron/sqlx" _ "modernc.org/sqlite" @@ -24,6 +25,11 @@ var migrationsFS embed.FS // cloudtokens.go). type Store struct { db *sqlx.DB + // provisioningReadFault is nil in production. Tests set it so a hold + // lookup fails while the rest of the database still answers. A failed + // hold read must not be treated as "not held". + faultMu sync.Mutex + provisioningReadFault error } // Open creates (if needed) and opens the SQLite database at path, applying diff --git a/internal/store/provisioning.go b/internal/store/provisioning.go index 4e357f53..29f12a06 100644 --- a/internal/store/provisioning.go +++ b/internal/store/provisioning.go @@ -13,12 +13,70 @@ import ( // leave a tracked game that the next launch will sync. Existing games have // no row and are left alone. See migrations/0038_game_provisioning_holds.sql. +// SetProvisioningReadFault makes later hold lookups fail with err while +// every other table still answers. Pass nil to clear it. Production never +// sets this; it exists so a test can prove a failed read is not "not held". +func (s *Store) SetProvisioningReadFault(err error) { + if s == nil { + return + } + s.faultMu.Lock() + s.provisioningReadFault = err + s.faultMu.Unlock() +} + +func (s *Store) provisioningFault() error { + if s == nil { + return fmt.Errorf("provisioning hold could not be read") + } + s.faultMu.Lock() + defer s.faultMu.Unlock() + return s.provisioningReadFault +} + +// ProvisioningBlocks reports whether gameID must not sync. An unknown id is +// not held. An alias of a held game is held. A failed read is an error, never +// a quiet false: callers must not serve, watch, or sync when this fails. +func (s *Store) ProvisioningBlocks(gameID string) (bool, error) { + if s == nil { + return false, fmt.Errorf("provisioning hold could not be read") + } + if gameID == "" { + return false, nil + } + if err := s.provisioningFault(); err != nil { + return false, err + } + held, err := s.ProvisioningHeld(gameID) + if err != nil || held { + return held, err + } + var canonical string + err = s.db.Get(&canonical, `SELECT game_id FROM game_aliases WHERE alias_id = ?`, gameID) + if errors.Is(err, sql.ErrNoRows) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("provisioning hold alias %s: %w", gameID, err) + } + if canonical == "" || canonical == gameID { + return false, nil + } + return s.ProvisioningHeld(canonical) +} + // ProvisioningHeld reports whether gameID is still being configured. -// An unknown id is not held. +// An unknown id is not held. A database error is returned, not treated as false. func (s *Store) ProvisioningHeld(gameID string) (bool, error) { + if s == nil { + return false, fmt.Errorf("provisioning hold could not be read") + } if gameID == "" { return false, nil } + if err := s.provisioningFault(); err != nil { + return false, err + } var one int err := s.db.Get(&one, `SELECT 1 FROM game_provisioning_holds WHERE game_id = ?`, gameID) if errors.Is(err, sql.ErrNoRows) { @@ -33,6 +91,12 @@ func (s *Store) ProvisioningHeld(gameID string) (bool, error) { // ProvisioningHeldSet is every game currently held, for callers that walk // the library and must not query once per game. func (s *Store) ProvisioningHeldSet() (map[string]struct{}, error) { + if s == nil { + return nil, fmt.Errorf("provisioning hold could not be read") + } + if err := s.provisioningFault(); err != nil { + return nil, err + } var ids []string if err := s.db.Select(&ids, `SELECT game_id FROM game_provisioning_holds`); err != nil { return nil, fmt.Errorf("list provisioning holds: %w", err) diff --git a/internal/store/provisioning_test.go b/internal/store/provisioning_test.go index 668265cc..567f7a0e 100644 --- a/internal/store/provisioning_test.go +++ b/internal/store/provisioning_test.go @@ -1,6 +1,7 @@ package store import ( + "fmt" "testing" ) @@ -85,6 +86,38 @@ func TestCreateHeldGameIsAtomicAndDefaultsExistingGamesClear(t *testing.T) { } } +func TestProvisioningReadFaultIsNotNotHeld(t *testing.T) { + s := openTestStore(t) + if err := s.CreateHeldGame(Game{ID: "held-game", Name: "Held", SavePath: "/tmp/held", MaxSnapshots: 20}); err != nil { + t.Fatal(err) + } + if err := s.AddGameAlias("alias-of-held", "held-game"); err != nil { + t.Fatal(err) + } + blocks, err := s.ProvisioningBlocks("alias-of-held") + if err != nil || !blocks { + t.Fatalf("alias of a held game blocks=%v err=%v", blocks, err) + } + blocks, err = s.ProvisioningBlocks("no-such-game") + if err != nil || blocks { + t.Fatalf("unknown id blocks=%v err=%v; want not held", blocks, err) + } + + s.SetProvisioningReadFault(fmt.Errorf("injected hold read failure")) + blocks, err = s.ProvisioningBlocks("held-game") + if err == nil || blocks { + t.Fatalf("faulty read blocks=%v err=%v; a failed read must be an error, not held=false", blocks, err) + } + if _, err := s.ProvisioningHeldSet(); err == nil { + t.Fatal("held set succeeded while reads were faulted") + } + // The game row itself is still readable. That is the fail-open shape: + // the hold query fails and a caller that ignores the error would sync. + if _, err := s.GetGame("held-game"); err != nil { + t.Fatalf("fault leaked into game reads: %v", err) + } +} + func TestValidExplicitGameID(t *testing.T) { ok := []string{"hades", "crash-bandicoot-4", "a1"} bad := []string{"", "Hades", "../x", "a/b", "a b", "a--b", "-a", "a-"} From af05e652f050404f8dcdb99f93a2e49ab9bb9891 Mon Sep 17 00:00:00 2001 From: Seagulls Date: Fri, 9 Oct 2026 10:52:57 +0100 Subject: [PATCH 5/7] api: stage a hold release without turning AutoSync on Default release still enables AutoSync, and reconcile then syncs that game with every online peer that is not held. That is now tested. {"autoSync": false} clears the hold and leaves AutoSync off, so reconcile, reconnect and the watcher do not sync. A PATCH cannot clear the hold. A failed hold read is not reported as an ordinary false. --- CHANGELOG.md | 11 +-- e2e/provisioning_hold_test.go | 129 ++++++++++++++++++++++++++++ internal/api/routes.go | 11 +++ internal/api/routes_provisioning.go | 31 ++++++- internal/api/server.go | 37 ++++---- internal/cliapp/commands_games.go | 50 ++++++++--- internal/daemon/daemon.go | 31 ++++--- internal/store/provisioning.go | 16 +++- internal/store/provisioning_test.go | 15 ++++ 9 files changed, 277 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f09e886..2347dc41 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,11 +13,12 @@ All notable changes to OpenSave are documented here. This project adheres to step. The hold survives a daemon restart and is not the ordinary AutoSync flag: peers cannot pull or push that game's saves, and releasing one game does not sync the rest of the library. `POST /api/games/{id}/release-provisioning` - clears the hold and does not sync by itself. The caller syncs afterwards, - so release does not contact every online peer. A third device that is still - held cannot send or receive that game's saves. An explicit game id is - accepted only while creating a hold. A failed hold lookup is not treated as - "not held". See issue #40. + clears the hold. An empty body turns AutoSync on, so reconcile can sync + that game with every online peer that is not still held. `{"autoSync": false}` + clears the hold and leaves AutoSync off, so reconcile, reconnect and the + watcher do not sync it; the caller syncs explicitly. A PATCH cannot clear + the hold. `provisioningHoldUnknown` is true when the hold cannot be read, + and `provisioningHold` is not reported false in that case. See issue #40. ## [2.4.1] — 2026-10-02 diff --git a/e2e/provisioning_hold_test.go b/e2e/provisioning_hold_test.go index 13f5095f..2049ae84 100644 --- a/e2e/provisioning_hold_test.go +++ b/e2e/provisioning_hold_test.go @@ -504,6 +504,135 @@ func TestProvisioningHold_StarLeavesHeldEndpointAlone(t *testing.T) { } } +// Default release turns AutoSync on. The periodic reconcile and a peer +// coming online call SyncAllGames, not the explicit sync route. That is +// enough to move bytes. This is the witness that "release does not call +// SyncGame" is not "nothing syncs until asked". +func TestProvisioningHold_DefaultReleaseLetsReconcileSync(t *testing.T) { + source := testutil.NewTestDaemon(t, "Reconcile-Source") + bridge := testutil.NewTestDaemon(t, "Reconcile-Bridge") + source.PairWith(bridge) + const id = "reconcile-game" + src := sideDir(t, source, "recon") + br := sideDir(t, bridge, "recon") + writeHoldFile(t, src, "slot.sav", "RECONCILE-BYTES") + createHeld(t, source, id, "Reconcile Game", src) + createHeld(t, bridge, id, "Reconcile Game", br) + source.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", nil, nil) + bridge.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", nil, nil) + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + source.Daemon.P2P.PingPairedPeers(ctx) + source.Daemon.P2P.SyncAllGames(ctx) + if !testutil.WaitFor(20*time.Second, func() bool { + return readHoldFile(br, "slot.sav") == "RECONCILE-BYTES" + }) { + t.Fatal("default release plus reconcile did not sync; the witness is wrong or the path changed") + } +} + +// Staged release clears the hold and leaves AutoSync off. Reconcile, a +// restart, and a file write must not move bytes. An explicit sync may, and +// a peer that is still held must refuse. Both ends of the star are tried as +// the writer. +func TestProvisioningHold_StagedReleaseSkipsReconcile(t *testing.T) { + for _, writer := range []string{"bazzite", "deck"} { + t.Run(writer, func(t *testing.T) { + bazzite := testutil.NewTestDaemon(t, "Staged-Bazzite") + bridge := testutil.NewTestDaemon(t, "Staged-Bridge") + deck := testutil.NewTestDaemon(t, "Staged-Deck") + bazzite.PairWith(bridge) + deck.PairWith(bridge) + const id = "staged-game" + bz := sideDir(t, bazzite, "staged") + br := sideDir(t, bridge, "staged") + dk := sideDir(t, deck, "staged") + writeHoldFile(t, bz, "progress.sav", "BAZZITE-ONLY") + writeHoldFile(t, dk, "progress.sav", "DECK-ONLY") + createHeld(t, bazzite, id, "Staged Game", bz) + createHeld(t, bridge, id, "Staged Game", br) + createHeld(t, deck, id, "Staged Game", dk) + + progressed := bazzite + progressedDir, quietDir, quietBytes := bz, dk, "DECK-ONLY" + if writer == "deck" { + progressed = deck + progressedDir, quietDir, quietBytes = dk, bz, "BAZZITE-ONLY" + } + var rel struct { + Released bool `json:"released"` + AutoSync bool `json:"autoSync"` + } + progressed.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", map[string]any{"autoSync": false}, &rel) + bridge.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", map[string]any{"autoSync": false}, &rel) + if rel.AutoSync { + t.Fatal("staged release turned AutoSync on") + } + writeHoldFile(t, progressedDir, "later.sav", "AFTER-RELEASE") + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + for _, node := range []*testutil.TestDaemon{bazzite, bridge, deck} { + node.Daemon.P2P.PingPairedPeers(ctx) + node.Daemon.P2P.SyncAllGames(ctx) + } + if readHoldFile(br, "progress.sav") != "" || readHoldFile(quietDir, "progress.sav") != quietBytes { + t.Fatalf("staged reconcile moved bytes: bridge=%q quiet=%q", readHoldFile(br, "progress.sav"), readHoldFile(quietDir, "progress.sav")) + } + progressed.Restart() + bridge.Restart() + progressed.Daemon.P2P.PingPairedPeers(ctx) + progressed.Daemon.P2P.SyncAllGames(ctx) + if readHoldFile(br, "progress.sav") != "" { + t.Fatal("restart after staged release let reconcile copy the save") + } + if !testutil.WaitFor(45*time.Second, func() bool { + progressed.API(http.MethodPost, "/api/games/"+id+"/sync", nil, nil) + return readHoldFile(br, "progress.sav") == map[string]string{"bazzite": "BAZZITE-ONLY", "deck": "DECK-ONLY"}[writer] + }) { + t.Fatalf("explicit sync after staged release did not converge the released pair: bridge=%q", readHoldFile(br, "progress.sav")) + } + if readHoldFile(quietDir, "progress.sav") != quietBytes || readHoldFile(quietDir, "later.sav") != "" { + t.Fatal("explicit sync moved the peer that is still held") + } + }) + } +} + +func TestProvisioningHold_PatchDoesNotUnhold(t *testing.T) { + source := testutil.NewTestDaemon(t, "Patch-Source") + bridge := testutil.NewTestDaemon(t, "Patch-Bridge") + source.PairWith(bridge) + const id = "patch-game" + src := sideDir(t, source, "patch") + br := sideDir(t, bridge, "patch") + writeHoldFile(t, src, "slot.sav", "PATCH-BYTES") + createHeld(t, source, id, "Patch Game", src) + createHeld(t, bridge, id, "Patch Game", br) + var patched struct { + Name string `json:"name"` + AutoSync bool `json:"autoSync"` + ProvisioningHold bool `json:"provisioningHold"` + ProvisioningHoldUnknown bool `json:"provisioningHoldUnknown"` + } + source.API(http.MethodPatch, "/api/games/"+id, map[string]any{ + "name": "Renamed", "savePath": src, "autoSync": true, + }, &patched) + if patched.Name != "Renamed" || patched.AutoSync || !patched.ProvisioningHold || patched.ProvisioningHoldUnknown { + t.Fatalf("patch changed the hold contract: %+v", patched) + } + held, err := source.Daemon.Store.ProvisioningHeld(id) + if err != nil || !held { + t.Fatal("PATCH cleared the hold") + } + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + source.Daemon.P2P.PingPairedPeers(ctx) + source.Daemon.P2P.SyncAllGames(ctx) + if readHoldFile(br, "slot.sav") != "" { + t.Fatal("PATCH autoSync true let reconcile copy a held save") + } +} + func TestProvisioningHold_OrdinaryTrackUnchanged(t *testing.T) { node := testutil.NewTestDaemon(t, "Ordinary") var got holdGame diff --git a/internal/api/routes.go b/internal/api/routes.go index 16e4b420..4309fe74 100644 --- a/internal/api/routes.go +++ b/internal/api/routes.go @@ -399,11 +399,22 @@ func (s *Server) handleUpdateGame(w http.ResponseWriter, r *http.Request) { oldAutoSync := game.AutoSync oldIgnore := game.SyncIgnore oldAppID := game.AppID + heldBefore, holdErr := s.Daemon.StoreProvisioningHeld(gameID) + if holdErr != nil { + writeError(w, http.StatusServiceUnavailable, "could not read whether this game is still being configured") + return + } if err := readJSON(r, &game); err != nil { writeError(w, http.StatusBadRequest, err.Error()) return } game.ID = gameID // id is not client-mutable + // A hold is not a column on the game. A PATCH that sends autoSync, including + // a form that round-trips the whole object, must not clear it and must not + // turn the column on while the hold remains. Release is the only unblock. + if heldBefore { + game.AutoSync = oldAutoSync + } // A changed save path is validated exactly as a fresh track is. This // decoded straight into the stored game and wrote it back, so a path that diff --git a/internal/api/routes_provisioning.go b/internal/api/routes_provisioning.go index 12d100d7..b2f76920 100644 --- a/internal/api/routes_provisioning.go +++ b/internal/api/routes_provisioning.go @@ -10,12 +10,29 @@ import ( ) // handleReleaseProvisioning ends the hold on one game. It does not sync. -// SyncGame would contact every online peer. The caller syncs after releasing -// only the peers that should converge. A repeat after the hold is gone is a -// success and does not sync. +// +// An empty body turns AutoSync on and starts watching. Reconcile and a peer +// coming online will then sync this game with every online peer that is not +// still held. {"autoSync": false} clears the hold and leaves AutoSync off, so +// those paths do not sync. The caller syncs explicitly, then turns AutoSync +// on when that result is the one it wants kept. A repeat after the hold is +// gone does not change AutoSync. func (s *Server) handleReleaseProvisioning(w http.ResponseWriter, r *http.Request) { gameID := chi.URLParam(r, "gameId") - released, err := s.Daemon.ReleaseProvisioning(gameID) + var body struct { + AutoSync *bool `json:"autoSync"` + } + if r.ContentLength != 0 { + if err := readJSON(r, &body); err != nil { + writeError(w, http.StatusBadRequest, err.Error()) + return + } + } + enableAutoSync := true + if body.AutoSync != nil { + enableAutoSync = *body.AutoSync + } + released, err := s.Daemon.ReleaseProvisioningMode(gameID, enableAutoSync) if err != nil { status := http.StatusBadRequest if errors.Is(err, store.ErrNotFound) { @@ -24,10 +41,16 @@ func (s *Server) handleReleaseProvisioning(w http.ResponseWriter, r *http.Reques writeError(w, status, err.Error()) return } + game, gerr := s.Daemon.Store.GetGame(gameID) + auto := enableAutoSync + if gerr == nil { + auto = game.AutoSync + } s.BroadcastGamesUpdate() writeJSON(w, http.StatusOK, map[string]any{ "id": gameID, "released": released, "alreadyReleased": !released, + "autoSync": auto, }) } diff --git a/internal/api/server.go b/internal/api/server.go index 090ee940..d83b6945 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -514,17 +514,18 @@ func (s *Server) gamesPayload() map[string]any { return out } -// provisioningHoldField is true when the game is held or the hold cannot be -// read. A failed read must not be reported as false. -func (s *Server) provisioningHoldField(id string) bool { +// provisioningHoldState reports the hold and whether that read failed. +// A failed read is not an ordinary false: held is true and unknown is true, +// so a client cannot treat the game as ready. +func (s *Server) provisioningHoldState(id string) (held, unknown bool) { if s == nil || s.Daemon == nil { - return true + return true, true } held, err := s.Daemon.StoreProvisioningHeld(id) if err != nil { - return true + return true, true } - return held + return held, false } func (s *Server) gamePayload(g store.Game) map[string]any { @@ -558,18 +559,20 @@ func (s *Server) gamePayload(g store.Game) map[string]any { if since := s.Daemon.PlayingSince(g.ID); !since.IsZero() { playingSince = since.UTC().Format(time.RFC3339) } + hold, holdUnknown := s.provisioningHoldState(g.ID) return map[string]any{ - "id": g.ID, - "name": g.Name, - "savePath": g.SavePath, - "activeBranch": g.ActiveBranch, - "autoSync": g.AutoSync, - "provisioningHold": s.provisioningHoldField(g.ID), - "maxSnapshots": g.MaxSnapshots, - "maxManualSnapshots": g.MaxManualSnapshots, - "appId": g.AppID, - "exePath": g.ExePath, - "coverUrl": g.CoverURL, + "id": g.ID, + "name": g.Name, + "savePath": g.SavePath, + "activeBranch": g.ActiveBranch, + "autoSync": g.AutoSync, + "provisioningHold": hold, + "provisioningHoldUnknown": holdUnknown, + "maxSnapshots": g.MaxSnapshots, + "maxManualSnapshots": g.MaxManualSnapshots, + "appId": g.AppID, + "exePath": g.ExePath, + "coverUrl": g.CoverURL, // Whether the art cached for this game is explicit, so the client can // blur it until someone asks to see it. An cannot read a // response header, so it travels with the game rather than the image. diff --git a/internal/cliapp/commands_games.go b/internal/cliapp/commands_games.go index 38fd1d2e..71d15da0 100644 --- a/internal/cliapp/commands_games.go +++ b/internal/cliapp/commands_games.go @@ -21,7 +21,7 @@ import ( func cmdGame(d *daemon.Daemon, args []string) int { asJSON, args := jsonFlag(args) if len(args) >= 2 && args[1] == "release" { - return releaseProvisioning(d, gameIDFrom(args), asJSON) + return releaseProvisioning(d, args, asJSON) } if len(args) < 2 || args[1] != "set" { fmt.Fprintln(os.Stderr, gameUsage) @@ -52,6 +52,13 @@ func cmdGame(d *daemon.Daemon, args []string) int { case "cover-url": game.CoverURL = value case "auto-sync": + held, holdErr := d.StoreProvisioningHeld(game.ID) + if holdErr != nil { + return fail(asJSON, fmt.Errorf("could not read whether this game is still being configured: %w", holdErr)) + } + if held { + return fail(asJSON, fmt.Errorf("%s is still being configured; release it before changing auto-sync", game.ID)) + } game.AutoSync = isTruthy(value) case "max-snapshots": n, err := strconv.Atoi(value) @@ -106,35 +113,49 @@ func gameIDFrom(args []string) string { return args[0] } -func releaseProvisioning(d *daemon.Daemon, gameID string, asJSON bool) int { +func releaseProvisioning(d *daemon.Daemon, args []string, asJSON bool) int { + noAuto, rest := stripFlag(args[2:], "--no-autosync") + if len(args) < 2 || len(rest) != 0 { + fmt.Fprintln(os.Stderr, gameUsage) + return 1 + } + gameID := gameIDFrom(args) if gameID == "" { fmt.Fprintln(os.Stderr, gameUsage) return 1 } - // Prefer the running daemon so the release starts a watch and a sync of - // this game there. The short-lived process below is the fallback when - // nothing is listening, and the next launch of the real daemon picks the - // cleared hold up from the database. + body := map[string]any{} + if noAuto { + body["autoSync"] = false + } + // Prefer the running daemon so a default release starts the watch there. + // --no-autosync must not: reconcile would otherwise sync the game. if daemonRunning() { - raw, err := daemonRequest("POST", "/api/games/"+gameID+"/release-provisioning", map[string]any{}) + raw, err := daemonRequest("POST", "/api/games/"+gameID+"/release-provisioning", body) if err != nil { return fail(asJSON, err) } if asJSON { return emitRawJSON(raw) } - success("Released %s. It can sync from here on.", bold(gameID)) + if noAuto { + success("Released %s without auto-sync. Sync it explicitly, then turn auto-sync on.", bold(gameID)) + } else { + success("Released %s. Reconcile can sync it from here on.", bold(gameID)) + } return 0 } - released, err := d.ReleaseProvisioning(gameID) + released, err := d.ReleaseProvisioningMode(gameID, !noAuto) if err != nil { return fail(asJSON, err) } if asJSON { - return emitJSON(map[string]any{"id": gameID, "released": released, "alreadyReleased": !released}) + return emitJSON(map[string]any{"id": gameID, "released": released, "alreadyReleased": !released, "autoSync": !noAuto}) } - if released { - success("Released %s. It can sync from here on.", bold(gameID)) + if released && noAuto { + success("Released %s without auto-sync. Sync it explicitly, then turn auto-sync on.", bold(gameID)) + } else if released { + success("Released %s. Reconcile can sync it from here on.", bold(gameID)) } else { success("%s was already released.", bold(gameID)) } @@ -142,7 +163,10 @@ func releaseProvisioning(d *daemon.Daemon, gameID string, asJSON bool) int { } const gameUsage = `usage: opensave game set - opensave game release let a game that was being configured start syncing + opensave game release [--no-autosync] + clear a provisioning hold. Without --no-autosync, + reconcile may sync it. With --no-autosync, it stays + quiet until an explicit sync. name Display name (also how peers match this game) path Move tracking to a different save location diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go index 1bd7320a..fc79f5f1 100644 --- a/internal/daemon/daemon.go +++ b/internal/daemon/daemon.go @@ -969,28 +969,33 @@ func (d *Daemon) provisioningBlocks(gameID string) bool { return held } -// ReleaseProvisioning clears the hold and starts watching. It does not sync. -// SyncGame would contact every online peer. The caller syncs after it has -// released only the peers that should converge. A second call is a no-op. +// ReleaseProvisioning clears the hold, turns AutoSync on, and starts watching. +// It does not call SyncGame. Reconcile and a peer coming online will, because +// AutoSync is on. Use ReleaseProvisioningMode when that must not happen yet. func (d *Daemon) ReleaseProvisioning(gameID string) (bool, error) { + return d.ReleaseProvisioningMode(gameID, true) +} + +// ReleaseProvisioningMode clears the hold. enableAutoSync false does not +// watch and does not turn AutoSync on, so reconcile, reconnect and a file +// change do not sync. An explicit sync still can, and a peer that is still +// held still refuses. A second call is a no-op and does not change AutoSync. +func (d *Daemon) ReleaseProvisioningMode(gameID string, enableAutoSync bool) (bool, error) { game, err := d.Store.GetGame(gameID) if err != nil { return false, err } - released, err := d.Store.ReleaseProvisioning(gameID) + released, err := d.Store.ReleaseProvisioningMode(gameID, enableAutoSync) if err != nil || !released { return released, err } - if err := d.watchGame(game.ID, game.SavePath); err != nil && - !errors.Is(err, watcher.ErrStopped) && !errors.Is(err, watcher.ErrSaveFolderMissing) { - d.Log.Log("warn", fmt.Sprintf("could not watch %q after release: %v", game.Name, err)) + if enableAutoSync { + if err := d.watchGame(game.ID, game.SavePath); err != nil && + !errors.Is(err, watcher.ErrStopped) && !errors.Is(err, watcher.ErrSaveFolderMissing) { + d.Log.Log("warn", fmt.Sprintf("could not watch %q after release: %v", game.Name, err)) + } } - // Do not SyncGame here. That call contacts every online peer and can - // spread one endpoint's save to another before the caller has released - // only the pair it means to converge. The caller syncs explicitly. - // A peer that is still held still refuses, so a later reconcile cannot - // move that peer's files. - d.Log.Log("info", fmt.Sprintf("released %q; it can sync when asked, and only with peers that are not still being configured", game.Name)) + d.Log.Log("info", fmt.Sprintf("released %q (autoSync=%t); it is not synced by this call", game.Name, enableAutoSync)) return true, nil } diff --git a/internal/store/provisioning.go b/internal/store/provisioning.go index 29f12a06..3d4cd27d 100644 --- a/internal/store/provisioning.go +++ b/internal/store/provisioning.go @@ -156,6 +156,14 @@ func (s *Store) CreateHeldGame(g Game) error { // whose AutoSync the user turned off on purpose. The bool is whether this // call released a hold. func (s *Store) ReleaseProvisioning(gameID string) (bool, error) { + return s.ReleaseProvisioningMode(gameID, true) +} + +// ReleaseProvisioningMode clears the hold. enableAutoSync false leaves +// AutoSync off, so reconcile, reconnect and the watcher do not sync the game. +// The caller then syncs it explicitly. A game that is not held is left +// unchanged either way. +func (s *Store) ReleaseProvisioningMode(gameID string, enableAutoSync bool) (bool, error) { tx, err := s.db.Beginx() if err != nil { return false, err @@ -173,8 +181,12 @@ func (s *Store) ReleaseProvisioning(gameID string) (bool, error) { if n == 0 { return false, tx.Commit() } - if _, err := tx.Exec(`UPDATE games SET auto_sync = 1 WHERE id = ?`, gameID); err != nil { - return false, fmt.Errorf("enable auto-sync %s: %w", gameID, err) + auto := 0 + if enableAutoSync { + auto = 1 + } + if _, err := tx.Exec(`UPDATE games SET auto_sync = ? WHERE id = ?`, auto, gameID); err != nil { + return false, fmt.Errorf("set auto-sync %s: %w", gameID, err) } return true, tx.Commit() } diff --git a/internal/store/provisioning_test.go b/internal/store/provisioning_test.go index 567f7a0e..51fc34c1 100644 --- a/internal/store/provisioning_test.go +++ b/internal/store/provisioning_test.go @@ -64,6 +64,21 @@ func TestCreateHeldGameIsAtomicAndDefaultsExistingGamesClear(t *testing.T) { t.Fatal("a no-op release changed AutoSync on a game that was not held") } + if err := s.CreateHeldGame(Game{ID: "staged", Name: "Staged", SavePath: "/tmp/staged", MaxSnapshots: 20}); err != nil { + t.Fatal(err) + } + released, err = s.ReleaseProvisioningMode("staged", false) + if err != nil || !released { + t.Fatalf("staged release = %v, %v", released, err) + } + staged, err := s.GetGame("staged") + if err != nil { + t.Fatal(err) + } + if staged.AutoSync { + t.Fatal("staged release turned AutoSync on") + } + released, err = s.ReleaseProvisioning("configuring") if err != nil || !released { t.Fatalf("release = %v, %v", released, err) From eb60926b03ee1450358034a3efc2ff84ddfdb842 Mon Sep 17 00:00:00 2001 From: Seagulls Date: Fri, 9 Oct 2026 12:49:29 +0100 Subject: [PATCH 6/7] test: make provisioning safety contracts clear and upstream-neutral --- e2e/provisioning_hold_test.go | 246 ++++++++++++++++++---------- internal/api/routes.go | 6 + internal/api/routes_provisioning.go | 3 +- internal/cliapp/commands_games.go | 8 +- 4 files changed, 174 insertions(+), 89 deletions(-) diff --git a/e2e/provisioning_hold_test.go b/e2e/provisioning_hold_test.go index 2049ae84..5f7836f4 100644 --- a/e2e/provisioning_hold_test.go +++ b/e2e/provisioning_hold_test.go @@ -4,6 +4,7 @@ import ( "context" "crypto/sha256" "encoding/hex" + "errors" "net/http" "os" "path/filepath" @@ -35,11 +36,16 @@ func writeHoldFile(t *testing.T, dir, rel, content string) { } } -func readHoldFile(dir, rel string) string { - raw, err := os.ReadFile(filepath.Join(dir, filepath.FromSlash(rel))) - if err != nil { +func readHoldFile(t *testing.T, dir, rel string) string { + t.Helper() + path := filepath.Join(dir, filepath.FromSlash(rel)) + raw, err := os.ReadFile(path) + if os.IsNotExist(err) { return "" } + if err != nil { + t.Fatalf("reading save fixture %q: %v", path, err) + } return string(raw) } @@ -83,23 +89,19 @@ func assertHeldQuiet(t *testing.T, source, bridge *testutil.TestDaemon, id, sour source.Daemon.P2P.SyncAllGames(ctx) bridge.Daemon.P2P.PingPairedPeers(ctx) bridge.Daemon.P2P.SyncAllGames(ctx) - if _, err := source.Daemon.P2P.Sync.SyncGame(ctx, id, source.Daemon.P2P.OnlinePeers()); err != nil && err != syncengine.ErrProvisioning { - // A missing peer is not a transfer. A provisioning refusal is the - // gate. Any other error is reported, but a successful sync is not. - if err.Error() != syncengine.ErrProvisioning.Error() { - t.Logf("direct syncengine sync: %v", err) - } + if _, err := source.Daemon.P2P.Sync.SyncGame(ctx, id, source.Daemon.P2P.OnlinePeers()); !errors.Is(err, syncengine.ErrProvisioning) { + t.Fatalf("held game must refuse direct sync, got %v", err) } var syncResp struct { Error string `json:"error"` Reason string `json:"reason"` } status := source.APIStatus(http.MethodPost, "/api/games/"+id+"/sync", nil, &syncResp) - if status < 400 || syncResp.Reason != "provisioning" { + if status != http.StatusConflict || syncResp.Reason != "provisioning" { t.Fatalf("direct sync of a held game returned %d %+v", status, syncResp) } - if readHoldFile(bridgeDir, marker) != "" || readHoldFile(sourceDir, "from-bridge.sav") != "" { - t.Fatalf("held game transferred: bridge=%q source=%q", readHoldFile(bridgeDir, marker), readHoldFile(sourceDir, "from-bridge.sav")) + if readHoldFile(t, bridgeDir, marker) != "" || readHoldFile(t, sourceDir, "from-bridge.sav") != "" { + t.Fatalf("held game transferred: bridge=%q source=%q", readHoldFile(t, bridgeDir, marker), readHoldFile(t, sourceDir, "from-bridge.sav")) } if _, err := bridge.Daemon.Store.GetGame(id); err != nil { t.Fatal("bridge lost the held game") @@ -136,11 +138,11 @@ func TestProvisioningHold_PairedCreateRestartAndIsolation(t *testing.T) { writeHoldFile(t, unrelatedSrc, "keep/fresh.sav", "UNRELATED-FRESH") if !testutil.WaitFor(30*time.Second, func() bool { source.API(http.MethodPost, "/api/games/"+store.SlugifyGameID("Hold Unrelated")+"/sync", nil, nil) - return readHoldFile(unrelatedBr, "keep/fresh.sav") == "UNRELATED-FRESH" + return readHoldFile(t, unrelatedBr, "keep/fresh.sav") == "UNRELATED-FRESH" }) { t.Fatal("unrelated game stopped syncing while another game was held") } - if readHoldFile(newBr, "profile/source.sav") != "" { + if readHoldFile(t, newBr, "profile/source.sav") != "" { t.Fatal("unrelated sync also copied the held game") } @@ -174,7 +176,7 @@ func TestProvisioningHold_PairedCreateRestartAndIsolation(t *testing.T) { if status := bridge.APIStatus(http.MethodPost, "/api/p2p/delete-file/"+id, map[string]string{"relPath": "profile/bridge.sav"}, &deleted); status < 400 { t.Fatalf("delete-file on a held game returned %d", status) } - if readHoldFile(newBr, "profile/bridge.sav") != "ONLY-BRIDGE" { + if readHoldFile(t, newBr, "profile/bridge.sav") != "ONLY-BRIDGE" { t.Fatal("inbound delete changed a held save") } @@ -196,19 +198,19 @@ func TestProvisioningHold_PairedCreateRestartAndIsolation(t *testing.T) { defer cancel() source.Daemon.P2P.PingPairedPeers(ctx) source.Daemon.P2P.SyncAllGames(ctx) - if readHoldFile(newBr, "profile/source.sav") != "" || readHoldFile(newSrc, "profile/bridge.sav") != "" { + if readHoldFile(t, newBr, "profile/source.sav") != "" || readHoldFile(t, newSrc, "profile/bridge.sav") != "" { t.Fatal("one-sided release transferred saves") } bridge.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", nil, &rel) if !testutil.WaitFor(45*time.Second, func() bool { source.API(http.MethodPost, "/api/games/"+id+"/sync", nil, nil) - return readHoldFile(newBr, "profile/source.sav") == "ONLY-SOURCE" && + return readHoldFile(t, newBr, "profile/source.sav") == "ONLY-SOURCE" && fileHash(newSrc, "profile/source.sav") == fileHash(newBr, "profile/source.sav") }) { - t.Fatalf("release did not sync: bridge=%q", readHoldFile(newBr, "profile/source.sav")) + t.Fatalf("release did not sync: bridge=%q", readHoldFile(t, newBr, "profile/source.sav")) } - if readHoldFile(unrelatedBr, "keep/fresh.sav") != "UNRELATED-FRESH" { + if readHoldFile(t, unrelatedBr, "keep/fresh.sav") != "UNRELATED-FRESH" { t.Fatal("release disturbed the unrelated game") } } @@ -285,16 +287,16 @@ func TestProvisioningHold_PlaceOfferWithoutSyncing(t *testing.T) { t.Fatalf("repeated place created %d games", n) } source.API(http.MethodPost, "/api/games/"+gameID+"/sync", nil, nil) - if readHoldFile(dest, "slot.sav") != "" { + if readHoldFile(t, dest, "slot.sav") != "" { t.Fatal("placing with autoSync false synced the save") } bridge.API(http.MethodPost, "/api/games/"+gameID+"/release-provisioning", nil, nil) if !testutil.WaitFor(45*time.Second, func() bool { source.API(http.MethodPost, "/api/games/"+gameID+"/sync", nil, nil) - return readHoldFile(dest, "slot.sav") == "FROM-SOURCE" + return readHoldFile(t, dest, "slot.sav") == "FROM-SOURCE" }) { - t.Fatalf("release did not deliver the save, dest=%q", readHoldFile(dest, "slot.sav")) + t.Fatalf("release did not deliver the save, dest=%q", readHoldFile(t, dest, "slot.sav")) } } @@ -373,16 +375,16 @@ func TestProvisioningHold_NamedRootsBeforeRelease(t *testing.T) { if status := source.APIStatus(http.MethodPost, "/api/games/"+id+"/sync", nil, nil); status < 400 { t.Fatalf("sync of a held game returned %d", status) } - if readHoldFile(br, "main.sav") != "" || readHoldFile(brA, "a.sav") != "" || readHoldFile(brB, "b.sav") != "" { + if readHoldFile(t, br, "main.sav") != "" || readHoldFile(t, brA, "a.sav") != "" || readHoldFile(t, brB, "b.sav") != "" { t.Fatal("named roots synced before release") } source.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", nil, nil) bridge.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", nil, nil) if !testutil.WaitFor(45*time.Second, func() bool { source.API(http.MethodPost, "/api/games/"+id+"/sync", nil, nil) - return readHoldFile(br, "main.sav") == "MAIN" && readHoldFile(brA, "a.sav") == "ROOT-A" && readHoldFile(brB, "b.sav") == "ROOT-B" + return readHoldFile(t, br, "main.sav") == "MAIN" && readHoldFile(t, brA, "a.sav") == "ROOT-A" && readHoldFile(t, brB, "b.sav") == "ROOT-B" }) { - t.Fatalf("roots after release: main=%q a=%q b=%q", readHoldFile(br, "main.sav"), readHoldFile(brA, "a.sav"), readHoldFile(brB, "b.sav")) + t.Fatalf("roots after release: main=%q a=%q b=%q", readHoldFile(t, br, "main.sav"), readHoldFile(t, brA, "a.sav"), readHoldFile(t, brB, "b.sav")) } } @@ -409,7 +411,7 @@ func TestProvisioningHold_HundredGamesStayOneDaemon(t *testing.T) { writeHoldFile(t, liveSrc, "live.sav", "LIVE-UPDATED") if !testutil.WaitFor(30*time.Second, func() bool { source.API(http.MethodPost, "/api/games/"+store.SlugifyGameID("Bulk Live")+"/sync", nil, nil) - return readHoldFile(liveBr, "live.sav") == "LIVE-UPDATED" + return readHoldFile(t, liveBr, "live.sav") == "LIVE-UPDATED" }) { t.Fatal("the live game did not sync beside 100 held games") } @@ -449,7 +451,7 @@ func TestProvisioningHold_DoesNotConvertAnExistingGame(t *testing.T) { if err != nil || held { t.Fatalf("existing game held=%v err=%v", held, err) } - if readHoldFile(node.SaveDir, "keep.sav") != "LIVE-BYTES" { + if readHoldFile(t, node.SaveDir, "keep.sav") != "LIVE-BYTES" { t.Fatal("the failed hold changed save bytes") } } @@ -458,48 +460,49 @@ func TestProvisioningHold_DoesNotConvertAnExistingGame(t *testing.T) { // that should converge must not move the third while it is still held, and // release itself must not start that transfer. func TestProvisioningHold_StarLeavesHeldEndpointAlone(t *testing.T) { - bazzite := testutil.NewTestDaemon(t, "Star-Bazzite") - bridge := testutil.NewTestDaemon(t, "Star-Bridge") - deck := testutil.NewTestDaemon(t, "Star-Deck") - bazzite.PairWith(bridge) - deck.PairWith(bridge) - if _, err := bazzite.Daemon.Store.GetPeer(deck.NodeID()); err == nil { - t.Fatal("bazzite is paired with deck; the fixture is not a star") + leafA := testutil.NewTestDaemon(t, "Star-LeafA") + hub := testutil.NewTestDaemon(t, "Star-Hub") + leafB := testutil.NewTestDaemon(t, "Star-LeafB") + leafA.PairWith(hub) + leafB.PairWith(hub) + if _, err := leafA.Daemon.Store.GetPeer(leafB.NodeID()); err == nil { + t.Fatal("leafA is paired with leafB; the fixture is not a star") } const id = "star-game" - bz := sideDir(t, bazzite, "star") - br := sideDir(t, bridge, "star") - dk := sideDir(t, deck, "star") - writeHoldFile(t, bz, "progress.sav", "BAZZITE-ONLY") - writeHoldFile(t, dk, "progress.sav", "DECK-ONLY") - createHeld(t, bazzite, id, "Star Game", bz) - createHeld(t, bridge, id, "Star Game", br) - createHeld(t, deck, id, "Star Game", dk) + bz := sideDir(t, leafA, "star") + br := sideDir(t, hub, "star") + dk := sideDir(t, leafB, "star") + writeHoldFile(t, bz, "progress.sav", "LEAF-A-ONLY") + writeHoldFile(t, dk, "progress.sav", "LEAF-B-ONLY") + createHeld(t, leafA, id, "Star Game", bz) + createHeld(t, hub, id, "Star Game", br) + createHeld(t, leafB, id, "Star Game", dk) var rel struct { Released bool `json:"released"` } - bazzite.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", nil, &rel) - bridge.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", nil, &rel) - // Release must not itself copy. The deck is still held. - if readHoldFile(br, "progress.sav") != "" || readHoldFile(dk, "progress.sav") != "DECK-ONLY" { - t.Fatal("release transferred before an explicit sync") + leafA.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", nil, &rel) + hub.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", nil, &rel) + // Default release enables AutoSync; the released pair may converge + // before this assertion. The held third peer must remain untouched. + if readHoldFile(t, dk, "progress.sav") != "LEAF-B-ONLY" { + t.Fatal("default release touched the held third peer") } if !testutil.WaitFor(45*time.Second, func() bool { - bazzite.API(http.MethodPost, "/api/games/"+id+"/sync", nil, nil) - return readHoldFile(br, "progress.sav") == "BAZZITE-ONLY" && - readHoldFile(dk, "progress.sav") == "DECK-ONLY" && - readHoldFile(bz, "progress.sav") == "BAZZITE-ONLY" + leafA.API(http.MethodPost, "/api/games/"+id+"/sync", nil, nil) + return readHoldFile(t, br, "progress.sav") == "LEAF-A-ONLY" && + readHoldFile(t, dk, "progress.sav") == "LEAF-B-ONLY" && + readHoldFile(t, bz, "progress.sav") == "LEAF-A-ONLY" }) { - t.Fatalf("star sync moved a held endpoint or failed to converge the released pair: bridge=%q deck=%q bazzite=%q", - readHoldFile(br, "progress.sav"), readHoldFile(dk, "progress.sav"), readHoldFile(bz, "progress.sav")) + t.Fatalf("star sync moved a held endpoint or failed to converge the released pair: hub=%q leafB=%q leafA=%q", + readHoldFile(t, br, "progress.sav"), readHoldFile(t, dk, "progress.sav"), readHoldFile(t, bz, "progress.sav")) } ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) defer cancel() - bridge.Daemon.P2P.PingPairedPeers(ctx) - bridge.Daemon.P2P.SyncAllGames(ctx) - if readHoldFile(dk, "progress.sav") != "DECK-ONLY" || readHoldFile(bz, "progress.sav") != "BAZZITE-ONLY" { + hub.Daemon.P2P.PingPairedPeers(ctx) + hub.Daemon.P2P.SyncAllGames(ctx) + if readHoldFile(t, dk, "progress.sav") != "LEAF-B-ONLY" || readHoldFile(t, bz, "progress.sav") != "LEAF-A-ONLY" { t.Fatal("reconcile on the middle peer moved a held endpoint or overwrote the released source") } } @@ -525,7 +528,7 @@ func TestProvisioningHold_DefaultReleaseLetsReconcileSync(t *testing.T) { source.Daemon.P2P.PingPairedPeers(ctx) source.Daemon.P2P.SyncAllGames(ctx) if !testutil.WaitFor(20*time.Second, func() bool { - return readHoldFile(br, "slot.sav") == "RECONCILE-BYTES" + return readHoldFile(t, br, "slot.sav") == "RECONCILE-BYTES" }) { t.Fatal("default release plus reconcile did not sync; the witness is wrong or the path changed") } @@ -536,68 +539,108 @@ func TestProvisioningHold_DefaultReleaseLetsReconcileSync(t *testing.T) { // a peer that is still held must refuse. Both ends of the star are tried as // the writer. func TestProvisioningHold_StagedReleaseSkipsReconcile(t *testing.T) { - for _, writer := range []string{"bazzite", "deck"} { + for _, writer := range []string{"leafA", "leafB"} { t.Run(writer, func(t *testing.T) { - bazzite := testutil.NewTestDaemon(t, "Staged-Bazzite") - bridge := testutil.NewTestDaemon(t, "Staged-Bridge") - deck := testutil.NewTestDaemon(t, "Staged-Deck") - bazzite.PairWith(bridge) - deck.PairWith(bridge) + leafA := testutil.NewTestDaemon(t, "Staged-LeafA") + hub := testutil.NewTestDaemon(t, "Staged-Hub") + leafB := testutil.NewTestDaemon(t, "Staged-LeafB") + leafA.PairWith(hub) + leafB.PairWith(hub) const id = "staged-game" - bz := sideDir(t, bazzite, "staged") - br := sideDir(t, bridge, "staged") - dk := sideDir(t, deck, "staged") - writeHoldFile(t, bz, "progress.sav", "BAZZITE-ONLY") - writeHoldFile(t, dk, "progress.sav", "DECK-ONLY") - createHeld(t, bazzite, id, "Staged Game", bz) - createHeld(t, bridge, id, "Staged Game", br) - createHeld(t, deck, id, "Staged Game", dk) - - progressed := bazzite - progressedDir, quietDir, quietBytes := bz, dk, "DECK-ONLY" - if writer == "deck" { - progressed = deck - progressedDir, quietDir, quietBytes = dk, bz, "BAZZITE-ONLY" + bz := sideDir(t, leafA, "staged") + br := sideDir(t, hub, "staged") + dk := sideDir(t, leafB, "staged") + writeHoldFile(t, bz, "progress.sav", "LEAF-A-ONLY") + writeHoldFile(t, dk, "progress.sav", "LEAF-B-ONLY") + createHeld(t, leafA, id, "Staged Game", bz) + createHeld(t, hub, id, "Staged Game", br) + createHeld(t, leafB, id, "Staged Game", dk) + + progressed := leafA + progressedDir, quietDir, quietBytes := bz, dk, "LEAF-B-ONLY" + if writer == "leafB" { + progressed = leafB + progressedDir, quietDir, quietBytes = dk, bz, "LEAF-A-ONLY" } var rel struct { Released bool `json:"released"` AutoSync bool `json:"autoSync"` } progressed.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", map[string]any{"autoSync": false}, &rel) - bridge.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", map[string]any{"autoSync": false}, &rel) + hub.API(http.MethodPost, "/api/games/"+id+"/release-provisioning", map[string]any{"autoSync": false}, &rel) if rel.AutoSync { t.Fatal("staged release turned AutoSync on") } writeHoldFile(t, progressedDir, "later.sav", "AFTER-RELEASE") ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) defer cancel() - for _, node := range []*testutil.TestDaemon{bazzite, bridge, deck} { + for _, node := range []*testutil.TestDaemon{leafA, hub, leafB} { node.Daemon.P2P.PingPairedPeers(ctx) node.Daemon.P2P.SyncAllGames(ctx) } - if readHoldFile(br, "progress.sav") != "" || readHoldFile(quietDir, "progress.sav") != quietBytes { - t.Fatalf("staged reconcile moved bytes: bridge=%q quiet=%q", readHoldFile(br, "progress.sav"), readHoldFile(quietDir, "progress.sav")) + if readHoldFile(t, br, "progress.sav") != "" || readHoldFile(t, quietDir, "progress.sav") != quietBytes { + t.Fatalf("staged reconcile moved bytes: hub=%q quiet=%q", readHoldFile(t, br, "progress.sav"), readHoldFile(t, quietDir, "progress.sav")) } progressed.Restart() - bridge.Restart() + hub.Restart() progressed.Daemon.P2P.PingPairedPeers(ctx) progressed.Daemon.P2P.SyncAllGames(ctx) - if readHoldFile(br, "progress.sav") != "" { + if readHoldFile(t, br, "progress.sav") != "" { t.Fatal("restart after staged release let reconcile copy the save") } if !testutil.WaitFor(45*time.Second, func() bool { progressed.API(http.MethodPost, "/api/games/"+id+"/sync", nil, nil) - return readHoldFile(br, "progress.sav") == map[string]string{"bazzite": "BAZZITE-ONLY", "deck": "DECK-ONLY"}[writer] + return readHoldFile(t, br, "progress.sav") == map[string]string{"leafA": "LEAF-A-ONLY", "leafB": "LEAF-B-ONLY"}[writer] }) { - t.Fatalf("explicit sync after staged release did not converge the released pair: bridge=%q", readHoldFile(br, "progress.sav")) + t.Fatalf("explicit sync after staged release did not converge the released pair: hub=%q", readHoldFile(t, br, "progress.sav")) + } + if readHoldFile(t, br, "later.sav") != "AFTER-RELEASE" { + t.Fatal("explicit sync omitted later save progress") } - if readHoldFile(quietDir, "progress.sav") != quietBytes || readHoldFile(quietDir, "later.sav") != "" { + for _, rel := range []string{"progress.sav", "later.sav"} { + want, got := fileHash(progressedDir, rel), fileHash(br, rel) + if want == "" || want != got { + t.Fatalf("sync hash differs for %q: source=%q hub=%q", rel, want, got) + } + } + if readHoldFile(t, quietDir, "progress.sav") != quietBytes || readHoldFile(t, quietDir, "later.sav") != "" { t.Fatal("explicit sync moved the peer that is still held") } }) } } +// A staged release blocks local auto-sync attempts but NOT an unheld +// peer's incoming request. This endpoint is not peer-specific authorization. +func TestProvisioningHold_StagedReleaseAllowsPeerInitiatedSync(t *testing.T) { + source := testutil.NewTestDaemon(t, "SyncRequester") + receiver := testutil.NewTestDaemon(t, "StagedReceiver") + source.WriteSave("slot.sav", "PEER-INITIATED") + gameID := source.TrackGame("Peer Initiated Staged Game") + recvDir := sideDir(t, receiver, "incoming") + createHeld(t, receiver, gameID, "Peer Initiated Staged Game", recvDir) + source.PairWith(receiver) + var state struct { + Released bool `json:"released"` + AutoSync bool `json:"autoSync"` + } + receiver.API(http.MethodPost, "/api/games/"+gameID+"/release-provisioning", + map[string]any{"autoSync": false}, &state) + if !state.Released || state.AutoSync { + t.Fatalf("staged release state = %+v", state) + } + if !testutil.WaitFor(45*time.Second, func() bool { + source.API(http.MethodPost, "/api/games/"+gameID+"/sync", nil, nil) + return readHoldFile(t, recvDir, "slot.sav") == "PEER-INITIATED" + }) { + t.Fatal("released receiver did not accept peer-initiated sync") + } + game, err := receiver.Daemon.Store.GetGame(gameID) + if err != nil || game.AutoSync { + t.Fatalf("peer request changed local AutoSync: %+v, %v", game, err) + } +} + func TestProvisioningHold_PatchDoesNotUnhold(t *testing.T) { source := testutil.NewTestDaemon(t, "Patch-Source") bridge := testutil.NewTestDaemon(t, "Patch-Bridge") @@ -628,11 +671,40 @@ func TestProvisioningHold_PatchDoesNotUnhold(t *testing.T) { defer cancel() source.Daemon.P2P.PingPairedPeers(ctx) source.Daemon.P2P.SyncAllGames(ctx) - if readHoldFile(br, "slot.sav") != "" { + if readHoldFile(t, br, "slot.sav") != "" { t.Fatal("PATCH autoSync true let reconcile copy a held save") } } +// Path updates to a still-held game must reject symlink roots, as +// initial held creation and named-root admission already do. +func TestProvisioningHold_HeldPathUpdateRejectsSymlink(t *testing.T) { + node := testutil.NewTestDaemon(t, "HeldPathUpdate") + original := sideDir(t, node, "primary") + createHeld(t, node, "held-path-update", "Held Path Update", original) + target := sideDir(t, node, "target") + link := filepath.Join(filepath.Dir(node.SaveDir), "held-path-link") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks unsupported: %v", err) + } + var resp struct { + Error string `json:"error"` + } + status := node.APIStatus(http.MethodPatch, "/api/games/held-path-update", + map[string]any{"savePath": link}, &resp) + if status < 400 || status >= 500 { + t.Fatalf("held game accepted symlink update: HTTP %d %+v", status, resp) + } + game, err := node.Daemon.Store.GetGame("held-path-update") + if err != nil || game.SavePath != original || game.AutoSync { + t.Fatalf("rejected path update changed game: %+v, %v", game, err) + } + held, err := node.Daemon.Store.ProvisioningHeld(game.ID) + if err != nil || !held { + t.Fatalf("rejected path update removed hold: %v, %v", held, err) + } +} + func TestProvisioningHold_OrdinaryTrackUnchanged(t *testing.T) { node := testutil.NewTestDaemon(t, "Ordinary") var got holdGame diff --git a/internal/api/routes.go b/internal/api/routes.go index 4309fe74..b3e9e1e2 100644 --- a/internal/api/routes.go +++ b/internal/api/routes.go @@ -432,6 +432,12 @@ func (s *Server) handleUpdateGame(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, err.Error()) return } + if heldBefore { + if err := daemon.RefuseSymlinkSave(abs); err != nil { + writeError(w, http.StatusBadRequest, err.Error()) + return + } + } game.SavePath = abs } // Cover art: a user-set custom URL is always kept. An empty cover, or diff --git a/internal/api/routes_provisioning.go b/internal/api/routes_provisioning.go index b2f76920..f71868e9 100644 --- a/internal/api/routes_provisioning.go +++ b/internal/api/routes_provisioning.go @@ -16,7 +16,8 @@ import ( // still held. {"autoSync": false} clears the hold and leaves AutoSync off, so // those paths do not sync. The caller syncs explicitly, then turns AutoSync // on when that result is the one it wants kept. A repeat after the hold is -// gone does not change AutoSync. +// gone does not change AutoSync. Staged release blocks this device +// from initiating automatic sync, not unheld peers from requesting files. func (s *Server) handleReleaseProvisioning(w http.ResponseWriter, r *http.Request) { gameID := chi.URLParam(r, "gameId") var body struct { diff --git a/internal/cliapp/commands_games.go b/internal/cliapp/commands_games.go index 71d15da0..7e571729 100644 --- a/internal/cliapp/commands_games.go +++ b/internal/cliapp/commands_games.go @@ -150,7 +150,13 @@ func releaseProvisioning(d *daemon.Daemon, args []string, asJSON bool) int { return fail(asJSON, err) } if asJSON { - return emitJSON(map[string]any{"id": gameID, "released": released, "alreadyReleased": !released, "autoSync": !noAuto}) + // An idempotent repeat changes nothing. Return stored AutoSync, + // not the mode requested by this invocation. + game, err := d.Store.GetGame(gameID) + if err != nil { + return fail(asJSON, err) + } + return emitJSON(map[string]any{"id": gameID, "released": released, "alreadyReleased": !released, "autoSync": game.AutoSync}) } if released && noAuto { success("Released %s without auto-sync. Sync it explicitly, then turn auto-sync on.", bold(gameID)) From 2fee76ffe91e9387d6ee717727a6b85779ddf0fd Mon Sep 17 00:00:00 2001 From: Seagulls Date: Fri, 9 Oct 2026 16:09:38 +0100 Subject: [PATCH 7/7] test: give the 100-game fixture a real agreed base The unrelated live game was tracked on both sides with identical bytes and no recorded convergence. A later edit then correctly conflicted instead of syncing. Sync once first, require both agreed hashes, and only then create the held games. --- e2e/provisioning_hold_test.go | 36 ++++++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/e2e/provisioning_hold_test.go b/e2e/provisioning_hold_test.go index 5f7836f4..8c3af641 100644 --- a/e2e/provisioning_hold_test.go +++ b/e2e/provisioning_hold_test.go @@ -390,31 +390,53 @@ func TestProvisioningHold_NamedRootsBeforeRelease(t *testing.T) { func TestProvisioningHold_HundredGamesStayOneDaemon(t *testing.T) { source := testutil.NewTestDaemon(t, "Bulk-Source") - bridge := testutil.NewTestDaemon(t, "Bulk-Bridge") + receiver := testutil.NewTestDaemon(t, "Bulk-Receiver") liveSrc := sideDir(t, source, "live") - liveBr := sideDir(t, bridge, "live") + liveRecv := sideDir(t, receiver, "live") writeHoldFile(t, liveSrc, "live.sav", "LIVE") - writeHoldFile(t, liveBr, "live.sav", "LIVE") source.API(http.MethodPost, "/api/games", map[string]string{"name": "Bulk Live", "savePath": liveSrc}, nil) - bridge.API(http.MethodPost, "/api/games", map[string]string{"name": "Bulk Live", "savePath": liveBr}, nil) - source.PairWith(bridge) + receiver.API(http.MethodPost, "/api/games", map[string]string{"name": "Bulk Live", "savePath": liveRecv}, nil) + source.PairWith(receiver) + liveID := store.SlugifyGameID("Bulk Live") + if !testutil.WaitFor(30*time.Second, func() bool { + source.API(http.MethodPost, "/api/games/"+liveID+"/sync", nil, nil) + return readHoldFile(t, liveSrc, "live.sav") == "LIVE" && readHoldFile(t, liveRecv, "live.sav") == "LIVE" + }) { + t.Fatal("the unrelated live game did not establish an initial sync") + } + srcBase := source.Daemon.Store.GetAgreedHash(liveID, receiver.NodeID()) + recvBase := receiver.Daemon.Store.GetAgreedHash(liveID, source.NodeID()) + if srcBase == "" || recvBase == "" || srcBase != recvBase { + t.Fatalf("initial sync left no bilateral agreed base: source=%q receiver=%q", srcBase, recvBase) + } start := time.Now() for i := 0; i < 100; i++ { id := "bulk-" + hex.EncodeToString([]byte{byte(i)}) dir := sideDir(t, source, id) + writeHoldFile(t, dir, "held.sav", "HELD-"+id) createHeld(t, source, id, "Bulk "+id, dir) } if elapsed := time.Since(start); elapsed > 45*time.Second { t.Fatalf("100 held creates took %s; this must not be a daemon per game", elapsed) } + offers, err := receiver.Daemon.Store.ListOfferedGames() + if err != nil { + t.Fatal(err) + } + if len(offers) != 0 { + t.Fatalf("held creates published %d offers", len(offers)) + } writeHoldFile(t, liveSrc, "live.sav", "LIVE-UPDATED") if !testutil.WaitFor(30*time.Second, func() bool { - source.API(http.MethodPost, "/api/games/"+store.SlugifyGameID("Bulk Live")+"/sync", nil, nil) - return readHoldFile(t, liveBr, "live.sav") == "LIVE-UPDATED" + source.API(http.MethodPost, "/api/games/"+liveID+"/sync", nil, nil) + return readHoldFile(t, liveRecv, "live.sav") == "LIVE-UPDATED" && readHoldFile(t, liveSrc, "live.sav") == "LIVE-UPDATED" }) { t.Fatal("the live game did not sync beside 100 held games") } + if _, err := os.Stat(filepath.Join(liveRecv, "held.sav")); !os.IsNotExist(err) { + t.Fatalf("a held save appeared on the receiver: %v", err) + } source.Restart() set, err := source.Daemon.Store.ProvisioningHeldSet() if err != nil {