From e7bbeace0e9eb44b7b016fcbc7317f7988e37ec5 Mon Sep 17 00:00:00 2001 From: Omar Date: Fri, 23 Oct 2026 19:28:37 +0530 Subject: [PATCH] A backup folder that is not connected is not written to A network share that is not connected leaves its mount point behind: an empty folder, or none. Backups to a local-folder destination on such a share were written into it, onto this machine's own disk, and counted as sent, while the share never got them (#46). A backup folder that was gone was created again by the next backup, with the same result. A local backup folder is now marked when it is chosen in Cloud Backup: created if need be, with a small file (.opensave-backup-folder) that says it is OpenSave's backup folder. A backup, and a look at what the folder holds, goes ahead only when the folder is there and marked. Otherwise it fails saying the folder is not available, check that the drive or share is connected, and the backup is sent again at a later cloud check, as any failed backup is. A folder chosen before the marker existed that already holds OpenSave's backups is marked the first time it is used, so nobody has to choose their folder again. --- internal/api/routes.go | 9 ++ internal/cloud/cloud_test.go | 15 ++-- internal/cloud/provider_local.go | 85 +++++++++++++++++++ internal/cloud/provider_local_marker_test.go | 89 ++++++++++++++++++++ internal/daemon/cloud_retry_test.go | 8 +- internal/daemon/cloudsync_test.go | 7 ++ 6 files changed, 206 insertions(+), 7 deletions(-) create mode 100644 internal/cloud/provider_local_marker_test.go diff --git a/internal/api/routes.go b/internal/api/routes.go index 3e81071e..903646e5 100644 --- a/internal/api/routes.go +++ b/internal/api/routes.go @@ -10,6 +10,7 @@ import ( "time" "github.com/go-chi/chi/v5" + "github.com/opensave/opensave/internal/cloud" "github.com/opensave/opensave/internal/daemon" "github.com/opensave/opensave/internal/p2p/syncengine" "github.com/opensave/opensave/internal/snapshot" @@ -293,6 +294,14 @@ func (s *Server) applyCloudPatch(patch *cloudSyncPatch) error { cfg.CustomClientSecrets[k] = v } } + // A local folder chosen now is the one meant: marked as the backup folder, + // so that later, a share that is not connected is told apart from it + // (cloud.MarkLocalFolder). + if cfg.Provider == "local" && (patch.URL != nil || patch.Provider != nil) { + if err := cloud.MarkLocalFolder(cfg.URL); err != nil { + return err + } + } return s.Daemon.Store.UpdateCloudConfig(cfg) } diff --git a/internal/cloud/cloud_test.go b/internal/cloud/cloud_test.go index 1a33c1cb..64adc9b0 100644 --- a/internal/cloud/cloud_test.go +++ b/internal/cloud/cloud_test.go @@ -60,7 +60,7 @@ func writeTempZip(t *testing.T, content string) string { func TestLocalFolderRoundTrip(t *testing.T) { svc, s := newTestService(t) - destDir := t.TempDir() + destDir := markedDir(t) setCloudConfig(t, s, func(c *store.CloudConfig) { c.Enabled = true c.Provider = "local" @@ -274,7 +274,7 @@ func TestGoogleDriveProviderWithTokenRefresh(t *testing.T) { // stay, older ones are deleted, other games/branches are untouched. func TestPruneGameBranch(t *testing.T) { svc, s := newTestService(t) - destDir := t.TempDir() + destDir := markedDir(t) setCloudConfig(t, s, func(c *store.CloudConfig) { c.Enabled = true c.Provider = "local" @@ -789,7 +789,7 @@ func TestUploadIsDoneWithTheArchiveWhenItSaysSo(t *testing.T) { setCloudConfig(t, s, func(c *store.CloudConfig) { c.Enabled = true c.Provider = "local" - c.URL = t.TempDir() + c.URL = markedDir(t) }) src := writeTempZip(t, "zip bytes") var removeErr error @@ -861,13 +861,16 @@ func folderNames(t *testing.T, dir string) []string { } var names []string for _, e := range entries { + if e.Name() == folderMarker { + continue // what makes it the backup folder, not a copy's leftover + } names = append(names, e.Name()) } return names } func TestALocalCopyInProgressIsNotListed(t *testing.T) { - dir := t.TempDir() + dir := markedDir(t) p := localFolder{cfg: store.CloudConfig{URL: dir}} const name = "game__main__snap_1.zip" body := []byte(strings.Repeat("archive bytes ", 4096)) @@ -908,7 +911,7 @@ func TestALocalCopyInProgressIsNotListed(t *testing.T) { } func TestALocalCopyThatFailsLeavesNothing(t *testing.T) { - dir := t.TempDir() + dir := markedDir(t) p := localFolder{cfg: store.CloudConfig{URL: dir}} if err := p.put(&failingReader{}, "game__main__snap_1.zip"); err == nil { t.Fatal("a copy whose source failed reported success") @@ -963,7 +966,7 @@ func TestALocalCopyIsAsReadableAsBefore(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("permission bits are not how Windows shares a folder") } - dir := t.TempDir() + dir := markedDir(t) plain := filepath.Join(dir, "plain") f, err := os.Create(plain) if err != nil { diff --git a/internal/cloud/provider_local.go b/internal/cloud/provider_local.go index 966cb651..e57072e0 100644 --- a/internal/cloud/provider_local.go +++ b/internal/cloud/provider_local.go @@ -17,6 +17,85 @@ type localFolder struct { cfg store.CloudConfig } +// folderMarker is the file that says a folder is OpenSave's backup folder. +// It is written when the folder is chosen (MarkLocalFolder), and a folder +// without it is not written to or read as a backup. +// +// A network share that is not connected leaves its mount point behind: an +// empty folder, or none. Backups were written into it, onto this machine's +// own disk, and counted as sent, while the share never got them; and its +// listing read as a backup with nothing in it. The marker is on the share, +// so it is missing from the mount point. +const folderMarker = ".opensave-backup-folder" + +// MarkLocalFolder makes dir OpenSave's backup folder: created if need be, +// and marked. It runs when the folder is chosen, which is when it is known to +// be the one meant. +func MarkLocalFolder(dir string) error { + dir = strings.TrimSpace(dir) + if dir == "" { + return nil + } + if err := os.MkdirAll(dir, 0o777); err != nil { + return fmt.Errorf("could not use %q as the backup folder: %w", dir, err) + } + return writeMarker(dir) +} + +func writeMarker(dir string) error { + path := filepath.Join(dir, folderMarker) + if _, err := os.Stat(path); err == nil { + return nil + } + body := "This folder holds OpenSave's backups. OpenSave looks for this file " + + "to know the folder is there: please leave it in place." + if err := os.WriteFile(path, []byte(body), 0o666); err != nil { + return fmt.Errorf("could not use %q as the backup folder: %w", dir, err) + } + return nil +} + +// ErrFolderUnavailable is a backup folder that is not there to be used: not +// connected, moved, or never chosen as one. +type ErrFolderUnavailable struct{ Dir string } + +func (e ErrFolderUnavailable) Error() string { + return fmt.Sprintf("the backup folder %q is not available — if it is on a network share or a removable drive, "+ + "check that it is connected; if it was moved, choose it again in Cloud Backup", e.Dir) +} + +// ready says whether the configured folder is the backup folder now: there, +// and marked. A folder already holding OpenSave's backups, chosen before the +// marker existed, is marked now rather than refused. +func (p localFolder) ready() error { + dir := p.cfg.URL + if info, err := os.Stat(dir); err != nil || !info.IsDir() { + return ErrFolderUnavailable{Dir: dir} + } + if _, err := os.Stat(filepath.Join(dir, folderMarker)); err == nil { + return nil + } + if holdsBackups(dir) { + return writeMarker(dir) + } + return ErrFolderUnavailable{Dir: dir} +} + +// holdsBackups says whether dir holds a file named as OpenSave names what it +// stores: ____.zip, __head--.zip. +func holdsBackups(dir string) bool { + entries, err := os.ReadDir(dir) + if err != nil { + return false + } + for _, e := range entries { + if e.Type().IsRegular() && strings.HasSuffix(e.Name(), ".zip") && strings.Contains(e.Name(), "__") { + return true + } + } + return false +} + func (p localFolder) upload(f *os.File, size int64, fileName string) error { return p.put(f, fileName) } @@ -29,6 +108,9 @@ func (p localFolder) put(r io.Reader, fileName string) error { if p.cfg.URL == "" { return fmt.Errorf("no local folder destination configured") } + if err := p.ready(); err != nil { + return err + } return writeFileWhole(filepath.Join(p.cfg.URL, fileName), func(w io.Writer) error { _, err := io.Copy(w, r) return err @@ -39,6 +121,9 @@ func (p localFolder) list() ([]CloudFile, error) { if p.cfg.URL == "" { return []CloudFile{}, nil } + if err := p.ready(); err != nil { + return nil, err + } entries, err := os.ReadDir(p.cfg.URL) if err != nil { return nil, fmt.Errorf("read local folder destination %q: %w", p.cfg.URL, err) diff --git a/internal/cloud/provider_local_marker_test.go b/internal/cloud/provider_local_marker_test.go new file mode 100644 index 00000000..99d3ebc5 --- /dev/null +++ b/internal/cloud/provider_local_marker_test.go @@ -0,0 +1,89 @@ +package cloud + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/opensave/opensave/internal/store" +) + +// markedDir is a folder chosen as the backup folder, as Cloud Backup's +// settings make one (MarkLocalFolder). +func markedDir(t *testing.T) string { + t.Helper() + dir := t.TempDir() + if err := MarkLocalFolder(dir); err != nil { + t.Fatal(err) + } + return dir +} + +// A network share that is not connected leaves an empty mount point behind. +// Backups were written into it, onto this machine's own disk, and counted as +// sent; its listing read as a backup with nothing in it (GitHub #46). Neither +// happens to a folder that was never chosen as the backup folder. +func TestBackupFolder_AnUnconnectedShareIsNotWrittenTo(t *testing.T) { + mountPoint := t.TempDir() // the share's folder, with the share not there + p := localFolder{cfg: store.CloudConfig{URL: mountPoint}} + + err := p.put(strings.NewReader("archive"), "game__main__snap_1.zip") + var unavailable ErrFolderUnavailable + if !errors.As(err, &unavailable) { + t.Fatalf("put into an unconnected share = %v, want the folder reported unavailable", err) + } + if entries, _ := os.ReadDir(mountPoint); len(entries) != 0 { + t.Errorf("something was written into the mount point: %v", entries) + } + if files, err := p.list(); err == nil { + t.Errorf("an unconnected share listed as a backup holding %v", files) + } +} + +// A backup folder that is gone is not made again by a backup: that is a +// folder on this machine standing in for one on a drive that is not there. +func TestBackupFolder_AMissingFolderIsNotCreated(t *testing.T) { + gone := filepath.Join(t.TempDir(), "NAS", "backups") + p := localFolder{cfg: store.CloudConfig{URL: gone}} + if err := p.put(strings.NewReader("archive"), "game__main__snap_1.zip"); err == nil { + t.Fatal("a backup into a missing folder succeeded") + } + if _, err := os.Stat(gone); err == nil { + t.Error("the missing backup folder was created by a backup") + } +} + +// Choosing a folder makes it the backup folder, creating it if need be; a +// share connected again is found by its marker. +func TestBackupFolder_ChosenFolderTakesBackups(t *testing.T) { + dir := filepath.Join(t.TempDir(), "new backups") + if err := MarkLocalFolder(dir); err != nil { + t.Fatal(err) + } + p := localFolder{cfg: store.CloudConfig{URL: dir}} + if err := p.put(strings.NewReader("archive"), "game__main__snap_1.zip"); err != nil { + t.Fatalf("put into the chosen folder: %v", err) + } + files, err := p.list() + if err != nil || len(files) != 1 || files[0].Name != "game__main__snap_1.zip" { + t.Fatalf("list = %+v, %v; want the one backup, and not the marker", files, err) + } +} + +// A folder chosen before the marker existed, already holding OpenSave's +// backups, keeps working: it is marked the first time it is used. +func TestBackupFolder_AFolderWithBackupsFromBeforeIsAdopted(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "game__main__snap_1.zip"), []byte("old backup"), 0o666); err != nil { + t.Fatal(err) + } + p := localFolder{cfg: store.CloudConfig{URL: dir}} + if err := p.put(strings.NewReader("archive"), "game__main__snap_2.zip"); err != nil { + t.Fatalf("put into a folder of earlier backups: %v", err) + } + if _, err := os.Stat(filepath.Join(dir, folderMarker)); err != nil { + t.Errorf("the folder was not marked: %v", err) + } +} diff --git a/internal/daemon/cloud_retry_test.go b/internal/daemon/cloud_retry_test.go index 17b3f76b..8a5b7a64 100644 --- a/internal/daemon/cloud_retry_test.go +++ b/internal/daemon/cloud_retry_test.go @@ -48,6 +48,9 @@ func TestAFailedCloudUploadIsSentAgainWhenTheCloudIsBack(t *testing.T) { if err := os.MkdirAll(cloudDir, 0o777); err != nil { t.Fatal(err) } + if err := cloud.MarkLocalFolder(cloudDir); err != nil { // the share, back with its marker + t.Fatal(err) + } dev.check() if _, err := os.Stat(filepath.Join(cloudDir, name)); err != nil { @@ -76,11 +79,14 @@ func TestAFailedUploadOfASnapshotSinceRemovedIsForgotten(t *testing.T) { if err := os.MkdirAll(cloudDir, 0o777); err != nil { t.Fatal(err) } + if err := cloud.MarkLocalFolder(cloudDir); err != nil { // the share, back with its marker + t.Fatal(err) + } dev.check() if pending, _ := dev.d.Store.CloudRetries(); len(pending) != 0 { t.Errorf("a snapshot that is gone is still waiting to be sent: %+v", pending) } - if entries, _ := os.ReadDir(cloudDir); len(entries) != 0 { + if entries, _ := os.ReadDir(cloudDir); len(entries) != 1 { // the backup folder's marker, and nothing sent t.Errorf("something was sent for a snapshot that is gone: %v", entries) } } diff --git a/internal/daemon/cloudsync_test.go b/internal/daemon/cloudsync_test.go index 877b083f..947d8f72 100644 --- a/internal/daemon/cloudsync_test.go +++ b/internal/daemon/cloudsync_test.go @@ -44,6 +44,13 @@ func newCloudDevice(t *testing.T, name, cloudDir string) *cloudDevice { cfg.Enabled = true cfg.Provider = "local" cfg.URL = cloudDir + // Chosen as the backup folder, as Cloud Backup's settings do — unless the + // test starts with the cloud out of reach (a file where the folder goes). + if info, err := os.Stat(cloudDir); err != nil || info.IsDir() { + if err := cloud.MarkLocalFolder(cloudDir); err != nil { + t.Fatal(err) + } + } if err := d.Store.UpdateCloudConfig(cfg); err != nil { t.Fatal(err) }