From 57639ee43042cbbec77ff10d5a788c8aebe1eca8 Mon Sep 17 00:00:00 2001 From: ChakraFusion <112588066+ChakraFusion@users.noreply.github.com> Date: Fri, 2 Oct 2026 15:44:43 +0200 Subject: [PATCH 1/2] snapshots: never of a save a sync is writing, never incomplete A pull of a save of many files overflowed the watcher's event queue again and again, and each overflow was taken for the game saving: an automatic snapshot of a half-pulled save every few seconds - 76 on one device for one pull - which pushed the complete ones out of retention. - The watcher asks whether a sync is writing the game (BeingWritten). If so, an overflow is taken for the sync's, and the burst is looked at only once the sync has finished. - A snapshot that could not read every file is discarded, not kept: it would stand in the history as a save that never existed. The watcher leaves the change unrecorded and takes it again at its next pass; a sync that needed the copy before replacing files does not replace them. - OpenSave's own temporary files (*.opensave.tmp) are never archived. Builds on the own-writes change (Liquid-co/OpenSave#23). Co-Authored-By: Claude Opus 5.5 --- internal/daemon/daemon.go | 6 ++++++ internal/p2p/syncengine/engine.go | 2 ++ internal/p2p/syncengine/settle.go | 34 +++++++++++++++++++++++++++++ internal/snapshot/snapshot.go | 22 ++++++++++++++++--- internal/snapshot/zip.go | 5 +++++ internal/snapshot/zip_roots.go | 4 ++++ internal/watcher/owntouch_test.go | 30 ++++++++++++++++++++++++++ internal/watcher/watcher.go | 36 ++++++++++++++++++++++++++++++- 8 files changed, 135 insertions(+), 4 deletions(-) diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go index bb2b4b20..5f757fc4 100644 --- a/internal/daemon/daemon.go +++ b/internal/daemon/daemon.go @@ -243,6 +243,12 @@ func New(opts Options) (*Daemon, error) { _, err := snaps.Create(gameID, "", true) return err }, + 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/snapshot.go b/internal/snapshot/snapshot.go index 5d1f86e5..2c4660ed 100644 --- a/internal/snapshot/snapshot.go +++ b/internal/snapshot/snapshot.go @@ -41,6 +41,18 @@ import ( // before starting one. type UploadHook func(zipPath, remoteFileName string) +// IncompleteError is a snapshot discarded because files of the save could not +// be read — in use by the game, or changing while it was archived. +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 { @@ -253,9 +265,13 @@ 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)) + // A snapshot missing files is not kept: it would stand in the history as + // a save that never existed — and, counted against retention, push out a + // complete one. The caller tries again at the next chance (the watcher + // leaves the change unrecorded, so its next pass takes it again; a sync + // that needed the copy before replacing files does not replace them). + if len(skipped) > 0 { + return store.Snapshot{}, &IncompleteError{Game: game.Name, Skipped: skipped} } if comment == "" { diff --git a/internal/snapshot/zip.go b/internal/snapshot/zip.go index f2d81dde..3bac4cb6 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) 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..a68c068b 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -119,6 +119,25 @@ type Callbacks struct { 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 +790,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 +809,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. From 4dfe3f24e457b30edcb9685e03a5f071b2243786 Mon Sep 17 00:00:00 2001 From: ChakraFusion <112588066+ChakraFusion@users.noreply.github.com> Date: Fri, 16 Oct 2026 17:16:44 +0200 Subject: [PATCH 2/2] snapshots: only automatic ones are discarded when incomplete; said on screen Asked in review: discarding every snapshot that could not read a file also discarded the copy kept before a sync replaces the save, and the sync then did not replace anything, quietly, for as long as a game held a file open (a log, a lock file). - Only the watcher's automatic snapshot (snapshot.CreateAutomatic) is discarded when it misses files; the watcher takes the change again at its next pass. Every other snapshot, the copy before a sync, restore or conflict above all, and one taken by hand, is kept with what could be read and a warning in the log, as before #54. - When a game's automatic snapshots come out incomplete three passes in a row, it is said on screen once, until one succeeds again: the files the game holds open, and that its changes are not backed up or synced until they can be read, usually once the game is closed. The text is in the catalogs (en, zh-CN), with a "common.open" key for the toast's button. Tests: an incomplete automatic snapshot is discarded, while one before replacing and one by hand are kept (a seam on the archive's file open makes a file unreadable on any system); the notice comes once per run of incomplete passes, again after a good snapshot, and never for other failures; the notice's wording, with the files beyond the first three counted. Co-Authored-By: Claude Opus 5.5 --- .../frontend/src/lib/notifications.js | 10 +++ .../frontend/src/lib/notifications.test.js | 21 +++++- cmd/opensave-app/frontend/src/lib/stores.js | 9 ++- cmd/opensave-app/frontend/src/locales/en.json | 6 ++ .../frontend/src/locales/zh-CN.json | 5 +- internal/api/server.go | 3 + internal/daemon/autosnapshot.go | 70 +++++++++++++++++++ internal/daemon/autosnapshot_test.go | 45 ++++++++++++ internal/daemon/daemon.go | 11 ++- internal/snapshot/incomplete_test.go | 69 ++++++++++++++++++ internal/snapshot/snapshot.go | 47 +++++++++---- internal/snapshot/zip.go | 7 +- internal/watcher/watcher.go | 7 ++ 13 files changed, 293 insertions(+), 17 deletions(-) create mode 100644 internal/daemon/autosnapshot.go create mode 100644 internal/daemon/autosnapshot_test.go create mode 100644 internal/snapshot/incomplete_test.go 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 5f757fc4..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,13 @@ 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) }, 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 2c4660ed..ec5047e2 100644 --- a/internal/snapshot/snapshot.go +++ b/internal/snapshot/snapshot.go @@ -41,8 +41,9 @@ import ( // before starting one. type UploadHook func(zipPath, remoteFileName string) -// IncompleteError is a snapshot discarded because files of the save could not -// be read — in use by the game, or changing while it was archived. +// 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 @@ -157,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. // @@ -181,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 @@ -197,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() @@ -265,13 +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 - // A snapshot missing files is not kept: it would stand in the history as - // a save that never existed — and, counted against retention, push out a - // complete one. The caller tries again at the next chance (the watcher - // leaves the change unrecorded, so its next pass takes it again; a sync - // that needed the copy before replacing files does not replace them). + // An automatic snapshot missing files is not kept, and any other is (see + // CreateAutomatic). if len(skipped) > 0 { - return store.Snapshot{}, &IncompleteError{Game: game.Name, Skipped: skipped} + 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 3bac4cb6..81c0db41 100644 --- a/internal/snapshot/zip.go +++ b/internal/snapshot/zip.go @@ -131,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/watcher/watcher.go b/internal/watcher/watcher.go index a68c068b..9431b166 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -114,6 +114,10 @@ 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) @@ -962,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 }