diff --git a/cmd/opensave-app/frontend/src/lib/notifications.js b/cmd/opensave-app/frontend/src/lib/notifications.js index 8bb495a1..d476afbf 100644 --- a/cmd/opensave-app/frontend/src/lib/notifications.js +++ b/cmd/opensave-app/frontend/src/lib/notifications.js @@ -142,6 +142,16 @@ export function arrivalNote(ev, games) { return { title: game.name, body: `Got ${plural(ev.files || 1, 'file')} from ${ev.device}`, game }; } +/** A game whose automatic snapshots keep missing files it holds open + * (daemon/autosnapshot.go), in the words of the catalog: `t` is the + * translate function, `$t` in a component. */ +export function incompleteSnapshotsMessage(data, t) { + const names = data?.files ?? []; + const more = Math.max((data?.count ?? names.length) - names.length, 0); + const files = names.join(', ') + (more ? t('snapshots.incomplete.more', { n: more }) : ''); + return t('snapshots.incomplete', { name: data?.gameName ?? '', files }); +} + /** New games found, as the desktop shows them. */ export function newGamesNote(found) { if (!found?.length) return null; diff --git a/cmd/opensave-app/frontend/src/lib/notifications.test.js b/cmd/opensave-app/frontend/src/lib/notifications.test.js index 27487862..61acda8c 100644 --- a/cmd/opensave-app/frontend/src/lib/notifications.test.js +++ b/cmd/opensave-app/frontend/src/lib/notifications.test.js @@ -1,5 +1,7 @@ import { describe, expect, it } from 'vitest'; -import { arrivalMessage, badgeCount, happened, loadSeenAt, saveSeenAt, waitingOnYou, ARRIVAL_QUIET_MS } from './notifications.js'; +import { arrivalMessage, badgeCount, happened, incompleteSnapshotsMessage, loadSeenAt, saveSeenAt, waitingOnYou, ARRIVAL_QUIET_MS } from './notifications.js'; +import { translate } from './i18n.js'; +import en from '../locales/en.json'; const games = { hades: { id: 'hades', name: 'Hades', branches: { main: { snapshots: [{ id: 's1', problem: 'damaged' }] } } }, @@ -71,3 +73,20 @@ describe('a missing save folder found on another drive', () => { expect(r?.go).toEqual({ view: 'game', params: { gameId: 'ark' } }); }); }); + +describe('incompleteSnapshotsMessage', () => { + const t = (key, params) => translate(en, key, params, new Intl.PluralRules('en')); + + it('names the game and the files it holds open', () => { + expect(incompleteSnapshotsMessage({ gameName: 'Hades', files: ['save.lock'], count: 1 }, t)).toBe( + '"Hades": automatic snapshots keep missing save.lock, which the game holds open. Until those can be read, usually once the game is closed, its changes are not backed up or synced.' + ); + }); + + it('says how many more there are than it names', () => { + const one = incompleteSnapshotsMessage({ gameName: 'Hades', files: ['a', 'b', 'c'], count: 4 }, t); + expect(one).toContain('missing a, b, c and one more file, which'); + const more = incompleteSnapshotsMessage({ gameName: 'Hades', files: ['a', 'b', 'c'], count: 9 }, t); + expect(more).toContain('missing a, b, c and 6 more files, which'); + }); +}); diff --git a/cmd/opensave-app/frontend/src/lib/stores.js b/cmd/opensave-app/frontend/src/lib/stores.js index 5343c4c1..00c319e4 100644 --- a/cmd/opensave-app/frontend/src/lib/stores.js +++ b/cmd/opensave-app/frontend/src/lib/stores.js @@ -1,8 +1,9 @@ // Central app state, fed by the daemon's init dump + live WS updates. import { writable, derived, get } from 'svelte/store'; import { notifyPrefs } from './notifyprefs.js'; -import { arrivalMessage, arrivalNote, newGamesNote } from './notifications.js'; +import { arrivalMessage, arrivalNote, incompleteSnapshotsMessage, newGamesNote } from './notifications.js'; import { toDesktop } from './notify.js'; +import { t } from './i18n.js'; // When each game's arrival was last said (lib/notifications.js). const arrivalShown = {}; @@ -186,6 +187,12 @@ export function applyMessage(msg) { case 'cloud-offers': cloudOffers.set(data ?? []); break; + case 'snapshots-incomplete': + // Sent once per run of discarded automatic snapshots, so no rate limit. + toast(incompleteSnapshotsMessage(data, get(t)), 'error', { + action: { label: get(t)('common.open'), run: () => navigate('game', { gameId: data.gameId }) } + }); + break; case 'cloud-pulled': // Taken without asking, because it carried on from the save this // device had and this device had not changed since. Said out loud all diff --git a/cmd/opensave-app/frontend/src/locales/en.json b/cmd/opensave-app/frontend/src/locales/en.json index e699ecf9..5284c023 100644 --- a/cmd/opensave-app/frontend/src/locales/en.json +++ b/cmd/opensave-app/frontend/src/locales/en.json @@ -2,6 +2,7 @@ "settings.language": "Language", "settings.systemDefault": "System default", "common.cancel": "Cancel", + "common.open": "Open", "common.nowTracking": "Now tracking \"{name}\"", "home.addGame.adding": "Adding…", "home.addGame.changeFolder": "Change folder…", @@ -222,5 +223,10 @@ "restoreProgress.aboutMinutes": { "one": "about a minute", "other": "about {n} minutes" + }, + "snapshots.incomplete": "\"{name}\": automatic snapshots keep missing {files}, which the game holds open. Until those can be read, usually once the game is closed, its changes are not backed up or synced.", + "snapshots.incomplete.more": { + "one": " and one more file", + "other": " and {n} more files" } } diff --git a/cmd/opensave-app/frontend/src/locales/zh-CN.json b/cmd/opensave-app/frontend/src/locales/zh-CN.json index f35d728e..7d1fad2b 100644 --- a/cmd/opensave-app/frontend/src/locales/zh-CN.json +++ b/cmd/opensave-app/frontend/src/locales/zh-CN.json @@ -2,6 +2,7 @@ "settings.language": "语言", "settings.systemDefault": "跟随系统", "common.cancel": "取消", + "common.open": "打开", "common.nowTracking": "已开始跟踪“{name}”", "home.addGame.adding": "添加中…", "home.addGame.changeFolder": "更换文件夹…", @@ -198,5 +199,7 @@ "restoreProgress.line": "{phase} — {percent}%（{done}/{total}）", "restoreProgress.lineWithTimeLeft": "{phase} — {percent}%（{done}/{total}），还剩{left}", "restoreProgress.underAMinute": "不到一分钟", - "restoreProgress.aboutMinutes": "约 {n} 分钟" + "restoreProgress.aboutMinutes": "约 {n} 分钟", + "snapshots.incomplete": "“{name}”：自动快照一直缺少游戏正在占用的 {files}。在这些文件可以读取之前（通常是游戏关闭后），它的更改不会被备份或同步。", + "snapshots.incomplete.more": " 等另外 {n} 个文件" } diff --git a/internal/api/server.go b/internal/api/server.go index 645cc12c..5bf129e1 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -70,6 +70,9 @@ func New(d *daemon.Daemon) *Server { // Newly installed games the background scan found. d.OnNewGames = func(games []daemon.NewGame) { s.Hub.Broadcast("new-games", games) } + // A game whose automatic snapshots keep missing files it holds open. + d.OnSnapshotsIncomplete = func(p daemon.SnapshotsIncomplete) { s.Hub.Broadcast("snapshots-incomplete", p) } + // Syncing paused or resumed — from here, the CLI, the tray, or a timer. d.P2P.Pause.OnChange(func(st syncpause.Status) { s.Hub.Broadcast("sync-pause", st) }) return s diff --git a/internal/daemon/autosnapshot.go b/internal/daemon/autosnapshot.go new file mode 100644 index 00000000..54906551 --- /dev/null +++ b/internal/daemon/autosnapshot.go @@ -0,0 +1,70 @@ +package daemon + +import ( + "errors" + "path/filepath" + "sync" + + "github.com/opensave/opensave/internal/snapshot" +) + +// An automatic snapshot that could not read every file of the save is +// discarded (snapshot.CreateAutomatic), and the watcher takes the change again +// at its next pass. That is right once or twice: a file was being written. +// A game that holds a file open the whole time it runs — a log, a lock file — +// has every one discarded until it closes, and with them the sync each would +// have started. Said only in the log, that looked like a save neither backed +// up nor synced for no reason; so after a few passes in a row it is said on +// screen, once, until a snapshot of the game succeeds again. + +// incompleteAutoSnapshotsToSay is how many passes in a row must end with the +// snapshot discarded before it is said on screen. +const incompleteAutoSnapshotsToSay = 3 + +// SnapshotsIncomplete is what is said: the game, and some of the files that +// could not be read. +type SnapshotsIncomplete struct { + GameID string `json:"gameId"` + GameName string `json:"gameName"` + Files []string `json:"files"` // names only, at most three + Count int `json:"count"` // how many could not be read +} + +type incompleteAutoSnapshots struct { + mu sync.Mutex + inARow map[string]int +} + +// autoSnapshotTaken ends a game's run of discarded automatic snapshots. +func (d *Daemon) autoSnapshotTaken(gameID string) { + d.incomplete.mu.Lock() + delete(d.incomplete.inARow, gameID) + d.incomplete.mu.Unlock() +} + +// autoSnapshotGaveUp is the watcher's pass ending without a snapshot. Only an +// incomplete one counts: any other failure is the log's to tell. +func (d *Daemon) autoSnapshotGaveUp(gameID string, err error) { + var inc *snapshot.IncompleteError + if !errors.As(err, &inc) { + return + } + d.incomplete.mu.Lock() + if d.incomplete.inARow == nil { + d.incomplete.inARow = map[string]int{} + } + d.incomplete.inARow[gameID]++ + n := d.incomplete.inARow[gameID] + d.incomplete.mu.Unlock() + if n != incompleteAutoSnapshotsToSay || d.OnSnapshotsIncomplete == nil { + return + } + said := SnapshotsIncomplete{GameID: gameID, GameName: inc.Game, Count: len(inc.Skipped)} + for _, p := range inc.Skipped { + if len(said.Files) == 3 { + break + } + said.Files = append(said.Files, filepath.Base(p)) + } + d.OnSnapshotsIncomplete(said) +} diff --git a/internal/daemon/autosnapshot_test.go b/internal/daemon/autosnapshot_test.go new file mode 100644 index 00000000..3c554865 --- /dev/null +++ b/internal/daemon/autosnapshot_test.go @@ -0,0 +1,45 @@ +package daemon + +import ( + "errors" + "testing" + + "github.com/opensave/opensave/internal/snapshot" +) + +// A game whose automatic snapshots keep coming out incomplete is said on +// screen once per run of them; a snapshot that succeeds ends the run, and +// failures of any other kind are left to the log. +func TestIncompleteAutoSnapshotsAreSaidOncePerRun(t *testing.T) { + var said []SnapshotsIncomplete + d := &Daemon{OnSnapshotsIncomplete: func(s SnapshotsIncomplete) { said = append(said, s) }} + incomplete := &snapshot.IncompleteError{Game: "Hades", Skipped: []string{"/s/a.log", "/s/b.lock", "/s/c", "/s/d"}} + + for i := 0; i < incompleteAutoSnapshotsToSay-1; i++ { + d.autoSnapshotGaveUp("hades", incomplete) + } + d.autoSnapshotGaveUp("hades", errors.New("disk full")) + if len(said) != 0 { + t.Fatalf("said after %d incomplete passes: %v", incompleteAutoSnapshotsToSay-1, said) + } + d.autoSnapshotGaveUp("hades", incomplete) + if len(said) != 1 { + t.Fatalf("said %d time(s) after %d incomplete passes, want once", len(said), incompleteAutoSnapshotsToSay) + } + got := said[0] + if got.GameID != "hades" || got.GameName != "Hades" || got.Count != 4 || len(got.Files) != 3 || got.Files[0] != "a.log" { + t.Errorf("said %+v", got) + } + d.autoSnapshotGaveUp("hades", incomplete) + if len(said) != 1 { + t.Errorf("said again within the same run") + } + + d.autoSnapshotTaken("hades") + for i := 0; i < incompleteAutoSnapshotsToSay; i++ { + d.autoSnapshotGaveUp("hades", incomplete) + } + if len(said) != 2 { + t.Errorf("a new run after a good snapshot was said %d time(s), want once more", len(said)-1) + } +} diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go index bb2b4b20..c14e4f0f 100644 --- a/internal/daemon/daemon.go +++ b/internal/daemon/daemon.go @@ -90,6 +90,11 @@ type Daemon struct { // newgames.go. OnNewGames func([]NewGame) newGames newGameState + + // OnSnapshotsIncomplete is told when a game's automatic snapshots keep + // coming out incomplete, once per run of them. See autosnapshot.go. + OnSnapshotsIncomplete func(SnapshotsIncomplete) + incomplete incompleteAutoSnapshots // scanMu runs one save scan at a time. See ScanForSaves. scanMu sync.Mutex @@ -240,9 +245,19 @@ func New(opts Options) (*Daemon, error) { }, SetLastManifestHash: s.SetLastManifestHash, CreateSnapshot: func(gameID string) error { - _, err := snaps.Create(gameID, "", true) + _, err := snaps.CreateAutomatic(gameID) + if err == nil { + d.autoSnapshotTaken(gameID) + } return err }, + SnapshotGaveUp: d.autoSnapshotGaveUp, + SyncWriting: func(gameID string) bool { + return d.P2P != nil && d.P2P.Sync != nil && d.P2P.Sync.BeingWritten(gameID) + }, + SyncWritingNow: func(gameID string) bool { + return d.P2P != nil && d.P2P.Sync != nil && d.P2P.Sync.WritingNow(gameID) + }, OnChanged: func(gameID string) { // An emptied save is noticed as it happens, and said on screen, // even with no other device online to hold it back from. diff --git a/internal/p2p/syncengine/engine.go b/internal/p2p/syncengine/engine.go index 5b8d8d10..e5e80bc0 100644 --- a/internal/p2p/syncengine/engine.go +++ b/internal/p2p/syncengine/engine.go @@ -127,6 +127,8 @@ type Engine struct { // on this device right now (settle.go). settleMu sync.Mutex gates map[string]*saveGate + // writtenAt is when a sync last stopped writing each game (BeingWritten). + writtenAt map[string]time.Time // servedMu guards served: the save states this device recently handed // each peer, per game (served.go). servedMu sync.Mutex diff --git a/internal/p2p/syncengine/settle.go b/internal/p2p/syncengine/settle.go index 34942e10..a66c1c8d 100644 --- a/internal/p2p/syncengine/settle.go +++ b/internal/p2p/syncengine/settle.go @@ -131,10 +131,44 @@ func (e *Engine) Writing(gameID string) (done func()) { once = true g := e.gateLocked(gameID) g.writers-- + if e.writtenAt == nil { + e.writtenAt = map[string]time.Time{} + } + e.writtenAt[gameID] = time.Now() e.wakeLocked(gameID, g) } } +// writeEcho is how long after a sync stops writing a game its file events may +// still be arriving. +const writeEcho = 10 * time.Second + +// BeingWritten reports whether a sync on this device is writing a game's save +// now, or was a moment ago — what the watcher asks before taking a burst it +// could not attribute (an overflow of file events) for a change of the game's. +// A pull of a save of many files overflows the event queue again and again, +// and each one read as the game saving: an automatic snapshot of a half-pulled +// save every few seconds, which pushed the real ones out of retention. +func (e *Engine) BeingWritten(gameID string) bool { + if e.WritingNow(gameID) { + return true + } + e.settleMu.Lock() + defer e.settleMu.Unlock() + return time.Since(e.writtenAt[gameID]) < writeEcho +} + +// WritingNow reports whether a sync on this device is writing a game's save at +// this moment — what the watcher waits out before looking at a burst, so it +// never judges a save half-way between two states. Not the moments after: +// a change the game makes then is the game's, and its snapshot is not held up. +func (e *Engine) WritingNow(gameID string) bool { + e.settleMu.Lock() + defer e.settleMu.Unlock() + g := e.gates[gameID] + return g != nil && (g.writers > 0 || g.waitingWriters > 0) +} + // Reading holds the game's save still for a sync to read it: it waits until // nothing is writing it, then keeps writers out until done is called. It // returns ErrSettling if ctx ends first. Hold it only for the reading itself — diff --git a/internal/snapshot/incomplete_test.go b/internal/snapshot/incomplete_test.go new file mode 100644 index 00000000..a9e15053 --- /dev/null +++ b/internal/snapshot/incomplete_test.go @@ -0,0 +1,69 @@ +package snapshot + +import ( + "errors" + "os" + "path/filepath" + "testing" +) + +// holdOpen has the named file of the save fail to open while archiving, as a +// file the game holds open does. +func holdOpen(t *testing.T, name string) { + t.Helper() + open := openForArchive + openForArchive = func(p string) (*os.File, error) { + if filepath.Base(p) == name { + return nil, errors.New("the process cannot access the file because it is being used by another process") + } + return open(p) + } + t.Cleanup(func() { openForArchive = open }) +} + +// An automatic snapshot that could not read every file is discarded: it would +// stand in the history as a save that never existed. +func TestCreateAutomatic_DiscardsAnIncompleteSnapshot(t *testing.T) { + env := setup(t) + writeSave(t, env.saveDir, "slot1.sav", "progress") + writeSave(t, env.saveDir, "game.log", "running") + holdOpen(t, "game.log") + + _, err := env.mgr.CreateAutomatic("game1") + var inc *IncompleteError + if !errors.As(err, &inc) { + t.Fatalf("CreateAutomatic = %v, want an IncompleteError", err) + } + if len(inc.Skipped) != 1 || filepath.Base(inc.Skipped[0]) != "game.log" { + t.Errorf("skipped %v, want game.log", inc.Skipped) + } + if snaps, _ := env.store.ListSnapshots("game1", "main"); len(snaps) != 0 { + t.Errorf("%d snapshot(s) kept, want none", len(snaps)) + } +} + +// Every other snapshot is kept with what could be read, as before: above all +// the copy kept before a sync replaces the save. Refusing it would stop the +// sync replacing files for as long as the game holds one open. +func TestOtherSnapshotsAreKeptWhenAFileCannotBeRead(t *testing.T) { + for name, create := range map[string]func(m *Manager) error{ + "before replacing": func(m *Manager) error { + _, err := m.CreateBeforeReplacing("game1", "before sync replaced local files") + return err + }, + "by hand": func(m *Manager) error { _, err := m.Create("game1", "mine", false); return err }, + } { + t.Run(name, func(t *testing.T) { + env := setup(t) + writeSave(t, env.saveDir, "slot1.sav", "progress") + writeSave(t, env.saveDir, "game.log", "running") + holdOpen(t, "game.log") + if err := create(env.mgr); err != nil { + t.Fatalf("snapshot failed: %v", err) + } + if snaps, _ := env.store.ListSnapshots("game1", "main"); len(snaps) != 1 { + t.Errorf("%d snapshot(s) kept, want 1", len(snaps)) + } + }) + } +} diff --git a/internal/snapshot/snapshot.go b/internal/snapshot/snapshot.go index 5d1f86e5..ec5047e2 100644 --- a/internal/snapshot/snapshot.go +++ b/internal/snapshot/snapshot.go @@ -41,6 +41,19 @@ import ( // before starting one. type UploadHook func(zipPath, remoteFileName string) +// IncompleteError is an automatic snapshot discarded because files of the +// save could not be read — in use by the game, or changing while it was +// archived (CreateAutomatic). +type IncompleteError struct { + Game string + Skipped []string +} + +func (e *IncompleteError) Error() string { + return fmt.Sprintf("snapshot of %q discarded: %d file(s) could not be read (e.g. %s) — taken again at the next chance", + e.Game, len(e.Skipped), e.Skipped[0]) +} + // Manager performs snapshot/branch operations against the store and // filesystem. type Manager struct { @@ -145,6 +158,25 @@ func (m *Manager) Create(gameID, comment string, isSystemAuto bool) (store.Snaps return m.createOnBranch(gameID, game.ActiveBranch, comment, isSystemAuto, true) } +// CreateAutomatic is the snapshot the watcher takes when a save has changed. +// +// It is the one snapshot not kept when it could not read every file of the +// save (IncompleteError): it would stand in the history as a save that never +// existed and, counted against retention, push out a complete one. Nothing is +// lost by not keeping it: the watcher leaves the change unrecorded and takes +// it again at its next pass. Every other snapshot is kept as it was, missing +// files and all, with a warning: one taken by hand, and above all the copy +// kept before a sync, a restore or a conflict replaces the save — refusing +// that would stop the replacing, quietly, for as long as a game holds a file +// open. +func (m *Manager) CreateAutomatic(gameID string) (store.Snapshot, error) { + game, err := m.Store.GetGame(gameID) + if err != nil { + return store.Snapshot{}, err + } + return m.createOnBranchFrom(gameID, game.ActiveBranch, "", "", true, true, true) +} + // CreateBeforeReplacing keeps a copy of a save that is about to be replaced — // by a restore, a sync, the other side of a conflict, a branch switch. // @@ -169,7 +201,7 @@ func (m *Manager) CreateBeforeReplacing(gameID, comment string) (store.Snapshot, // current reports whether the snapshot is of the save as it now stands and // will go on standing, as opposed to a copy kept before replacing it. func (m *Manager) createOnBranch(gameID, branch, comment string, isSystemAuto, current bool) (store.Snapshot, error) { - return m.createOnBranchFrom(gameID, branch, "", comment, isSystemAuto, current) + return m.createOnBranchFrom(gameID, branch, "", comment, isSystemAuto, current, false) } // CreateOnBranchFrom snapshots a folder that is not the game's save onto one @@ -185,12 +217,13 @@ func (m *Manager) CreateOnBranchFrom(gameID, branch, dir, comment string) (store if dir == "" { return store.Snapshot{}, errors.New("no folder to snapshot") } - return m.createOnBranchFrom(gameID, branch, dir, comment, true, false) + return m.createOnBranchFrom(gameID, branch, dir, comment, true, false, false) } // createOnBranchFrom is createOnBranch with the folder to archive given: "" -// for the game's own save locations, as always. -func (m *Manager) createOnBranchFrom(gameID, branch, from, comment string, isSystemAuto, current bool) (store.Snapshot, error) { +// for the game's own save locations, as always. discardIncomplete is for +// CreateAutomatic alone. +func (m *Manager) createOnBranchFrom(gameID, branch, from, comment string, isSystemAuto, current, discardIncomplete bool) (store.Snapshot, error) { m.inFlight.Add() defer m.inFlight.Done() @@ -253,9 +286,15 @@ func (m *Manager) createOnBranchFrom(gameID, branch, from, comment string, isSys return store.Snapshot{}, fmt.Errorf("zip save data: %w", err) } defer os.Remove(stagingPath) // no-op once renamed - if len(skipped) > 0 && m.Log != nil { - sample := skipped[0] - m.Log("warn", fmt.Sprintf("snapshot of %q skipped %d unreadable file(s), e.g. %s", game.Name, len(skipped), sample)) + // An automatic snapshot missing files is not kept, and any other is (see + // CreateAutomatic). + if len(skipped) > 0 { + if discardIncomplete { + return store.Snapshot{}, &IncompleteError{Game: game.Name, Skipped: skipped} + } + if m.Log != nil { + m.Log("warn", fmt.Sprintf("snapshot of %q skipped %d unreadable file(s), e.g. %s", game.Name, len(skipped), skipped[0])) + } } if comment == "" { diff --git a/internal/snapshot/zip.go b/internal/snapshot/zip.go index f2d81dde..81c0db41 100644 --- a/internal/snapshot/zip.go +++ b/internal/snapshot/zip.go @@ -95,6 +95,11 @@ func zipPathCapturing(sourcePath, outPath string, reuse *reuseSource) (skipped [ } return nil } + // OpenSave's own temporary file for a file being pulled: never part + // of the save, and gone by the time it is read. + if strings.HasSuffix(rel, delta.TmpSuffix) { + return nil + } info, infoErr := d.Info() if infoErr != nil { skipped = append(skipped, path) @@ -126,8 +131,13 @@ func zipPathCapturing(sourcePath, outPath string, reuse *reuseSource) (skipped [ // It costs nothing to produce here. The bytes are already streaming through // this function on their way into the archive, so hashing them is a second // writer on the same copy rather than a second pass over the disk. +// +// openForArchive is fsx.OpenShared; a test replaces it to have a file the +// game holds open, on any system. +var openForArchive = fsx.OpenShared + func addFileEntry(w *zip.Writer, filePath, entryName string) (string, error) { - f, err := fsx.OpenShared(filePath) + f, err := openForArchive(filePath) if err != nil { return "", err } diff --git a/internal/snapshot/zip_roots.go b/internal/snapshot/zip_roots.go index a83a8e0a..97de7e42 100644 --- a/internal/snapshot/zip_roots.go +++ b/internal/snapshot/zip_roots.go @@ -144,6 +144,10 @@ func archiveInto(w *zip.Writer, sourcePath, prefix, root string, reuse *reuseSou _, err := w.CreateHeader(&zip.FileHeader{Name: prefix + rel + "/", Method: zip.Store}) return err } + // OpenSave's own temporary file for a file being pulled (zip.go). + if strings.HasSuffix(rel, delta.TmpSuffix) { + return nil + } info, infoErr := d.Info() if infoErr != nil { skipped = append(skipped, path) diff --git a/internal/watcher/owntouch_test.go b/internal/watcher/owntouch_test.go index 6a1e8134..d984bc29 100644 --- a/internal/watcher/owntouch_test.go +++ b/internal/watcher/owntouch_test.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path/filepath" + "sync/atomic" "testing" "time" @@ -103,6 +104,35 @@ func TestOwnNewFolders_AreNotSnapshotted(t *testing.T) { } } +// While a sync is writing the game, the watcher looks at what changed only +// once it has finished — never at a save half-way between two states — and a +// change of the game's is still snapshotted then. +func TestWhileASyncWrites_TheWatcherWaits(t *testing.T) { + saveDir := t.TempDir() + col := newCollector() + var writing atomic.Bool + cb := col.callbacks() + cb.SyncWriting = func(string) bool { return writing.Load() } + cb.SyncWritingNow = cb.SyncWriting + eng := New(cb) + defer eng.Stop() + if err := eng.Watch("game1", saveDir); err != nil { + t.Fatal(err) + } + writing.Store(true) + if err := os.WriteFile(filepath.Join(saveDir, "slot1.sav"), []byte("played"), 0o666); err != nil { + t.Fatal(err) + } + time.Sleep(5 * time.Second) + if n := col.snapshotCount(); n != 0 { + t.Fatalf("%d snapshot(s) while a sync was writing the game", n) + } + writing.Store(false) + if !waitFor(t, 10*time.Second, func() bool { return col.snapshotCount() >= 1 }) { + t.Fatal("the game's change was not snapshotted once the sync had finished") + } +} + // A burst that mixes OpenSave's own changes with one from the game is a real // change and is snapshotted. func TestMixedChanges_AreSnapshotted(t *testing.T) { diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go index 2128ba2f..9431b166 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -114,11 +114,34 @@ type Callbacks struct { SetLastManifestHash func(gameID, hash string) error // CreateSnapshot takes the auto-snapshot. Retried on failure. CreateSnapshot func(gameID string) error + // SnapshotGaveUp is told when a pass gave up on its auto-snapshot after + // every retry, with the last error; the change is taken again at the next + // pass. May be nil. + SnapshotGaveUp func(gameID string, err error) // OnChanged fires after a successful auto-snapshot (the daemon uses it // to kick off P2P sync). Optional-in-behavior: may be nil. OnChanged func(gameID string) // Log receives human-readable watcher activity. May be nil. Log func(level, msg string) + // SyncWriting reports whether a sync on this device is writing the game + // now, or was a moment ago: a burst the watcher cannot attribute — an + // overflow of file events — is then taken for the sync's. May be nil. + SyncWriting func(gameID string) bool + // SyncWritingNow reports whether a sync is writing the game at this + // moment: the watcher looks at the burst once it has finished. May be nil. + SyncWritingNow func(gameID string) bool +} + +// syncWriting reports whether a sync is or was just writing a game +// (Callbacks.SyncWriting). +func (e *Engine) syncWriting(gameID string) bool { + return e.cb.SyncWriting != nil && e.cb.SyncWriting(gameID) +} + +// syncWritingNow reports whether a sync is writing a game at this moment +// (Callbacks.SyncWritingNow). +func (e *Engine) syncWritingNow(gameID string) bool { + return e.cb.SyncWritingNow != nil && e.cb.SyncWritingNow(gameID) } // Engine owns one watch goroutine per tracked game. @@ -771,7 +794,16 @@ func (e *Engine) run(ctx context.Context, gw *gameWatch) { // directory and allocates no second buffer for one it already // has. gw.rewatch.Store(true) - gw.otherEvents = true // the missed events could be anyone's + // The missed events could be anyone's — unless a sync is + // writing this game, which is what overflows the queue: a + // pull of a save of many files does, again and again, and + // each was taken for the game saving, an automatic snapshot + // of a half-pulled save every few seconds. + if e.syncWriting(gw.gameID) { + gw.ownEvents = true + } else { + gw.otherEvents = true + } } else { e.log("warn", fmt.Sprintf("watching %q: %v", gw.gameID, err)) continue @@ -781,6 +813,12 @@ func (e *Engine) run(ctx context.Context, gw *gameWatch) { case <-debounceC: debounce = nil debounceC = nil + // A sync still writing this game: look at the burst once it has + // finished, not at a save half-way between two states. + if e.syncWritingNow(gw.gameID) { + resetDebounce() + continue + } // Re-register, so a folder that went unwatched is watched from // here on, and read the tree below, so it is found now. Doing only // one of those leaves it correct today and silent tomorrow. @@ -928,6 +966,9 @@ func (e *Engine) handleChangeFrom(ctx context.Context, gw *gameWatch, ownOnly bo } if err != nil { e.log("error", fmt.Sprintf("auto-snapshot permanently failed for %q: %v", gw.gameID, err)) + if e.cb.SnapshotGaveUp != nil { + e.cb.SnapshotGaveUp(gw.gameID, err) + } return }