From 9e950cb63d6bd965a7a71e3b03ede294b8bc83a0 Mon Sep 17 00:00:00 2001 From: Huang-404-Q <129407632+Huang-404-Q@users.noreply.github.com> Date: Sat, 12 Sep 2026 08:50:45 +0800 Subject: [PATCH] fix(cli): fail restore when the backup file does not exist instead of wiping the database (#6085) * fix(db): fail restore when the backup file does not exist instead of wiping the database `navidrome backup restore -b ` passed the flag value straight to the SQLite driver, which opens databases with SQLITE_OPEN_CREATE by default. If the file was not found (for example a file name relative to the working directory instead of the backup directory), the driver silently created an empty database and the backup API copied that emptiness over the live database, reporting 'Restore complete' with an empty instance afterwards. Two changes: - db.Restore now opens the backup file read-only, so a missing file is an error and nothing gets created or overwritten. - A relative --backup-file is resolved against Backup.Path, the same folder 'backup create' writes to; absolute paths keep working as before. Fixes #6083 * fix(db): stat the backup file instead of opening it read-only The read-only DSN added in the previous commit works for the reported case but breaks on other paths: 'file:' + path is parsed as a URI, so a '#' truncates the path and a '%' sequence is percent-decoded, and a read-only open of a WAL database leaves '-shm'/'-wal' sidecars next to the backup. Those sidecars then matched the unanchored prune regex, so 'backup prune -k 3' right after a restore deleted real backups and kept one. Stat the file before opening it and keep passing the plain path to the driver. Paths containing '?' are rejected, since go-sqlite3 splits the DSN there and would otherwise open (and create) a different file. The prune regex is anchored so sidecars are never counted as backups. Also fixes the restore/backup/prune error logs, which printed BasePath (the web URL prefix) instead of the backup location. --------- Co-authored-by: Deluan --- cmd/backup.go | 21 ++++++++--- db/backup.go | 15 +++++++- db/backup_test.go | 89 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 5 deletions(-) diff --git a/cmd/backup.go b/cmd/backup.go index c02f3a19f..eca78a272 100644 --- a/cmd/backup.go +++ b/cmd/backup.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "os" + "path/filepath" "strings" "time" @@ -31,7 +32,7 @@ func init() { pruneCmd.Flags().BoolVarP(&force, "force", "f", false, "bypass warning when backup count is zero") backupRoot.AddCommand(pruneCmd) - restoreCommand.Flags().StringVarP(&restorePath, "backup-file", "b", "", "path of backup database to restore") + restoreCommand.Flags().StringVarP(&restorePath, "backup-file", "b", "", "file name of the backup database to restore (resolved against the backup directory unless it is an absolute path)") restoreCommand.Flags().BoolVarP(&force, "force", "f", false, "bypass restore warning") _ = restoreCommand.MarkFlagRequired("backup-file") backupRoot.AddCommand(restoreCommand) @@ -95,7 +96,7 @@ func runBackup(ctx context.Context) { start := time.Now() path, err := db.Backup(ctx) if err != nil { - log.Fatal("Error backing up database", "backup path", conf.Server.BasePath, err) + log.Fatal("Error backing up database", "backupPath", conf.Server.Backup.Path, err) } elapsed := time.Since(start) @@ -140,7 +141,7 @@ func runPrune(ctx context.Context) { start := time.Now() count, err := db.Prune(ctx) if err != nil { - log.Fatal("Error pruning up database", "backup path", conf.Server.BasePath, err) + log.Fatal("Error pruning database", "backupPath", conf.Server.Backup.Path, err) } elapsed := time.Since(start) @@ -163,6 +164,18 @@ func runRestore(ctx context.Context) { return } + // A relative --backup-file is resolved against Backup.Path, the same folder + // `backup create` writes to. Without this, the value was treated as relative + // to the working directory, where the file does not exist. + if !filepath.IsAbs(restorePath) { + backupPath, err := conf.Server.Backup.Path.Path() + if err != nil { + log.Fatal("Backup directory not available", "backupPath", conf.Server.Backup.Path, err) + return + } + restorePath = filepath.Join(backupPath, restorePath) + } + if !force { fmt.Println("Warning: restoring the Navidrome database should only be done offline, especially if your backup is very old.") fmt.Printf("Please enter YES (all caps) to continue: ") @@ -178,7 +191,7 @@ func runRestore(ctx context.Context) { start := time.Now() err := db.Restore(ctx, restorePath) if err != nil { - log.Fatal("Error restoring database", "backup path", conf.Server.BasePath, err) + log.Fatal("Error restoring database", "backupFile", restorePath, err) } elapsed := time.Since(start) diff --git a/db/backup.go b/db/backup.go index 806bef8e2..74cae553e 100644 --- a/db/backup.go +++ b/db/backup.go @@ -9,6 +9,7 @@ import ( "path/filepath" "regexp" "slices" + "strings" "time" "github.com/mattn/go-sqlite3" @@ -18,7 +19,7 @@ import ( const ( backupPrefix = "navidrome_backup" - backupRegexString = backupPrefix + "_(.+)\\.db" + backupRegexString = "^" + backupPrefix + "_(.+)\\.db$" ) var backupRegex = regexp.MustCompile(backupRegexString) @@ -40,6 +41,18 @@ func backupOrRestore(ctx context.Context, isBackup bool, path string) error { } defer existingConn.Close() + // The driver opens with SQLITE_OPEN_CREATE, so without this check a typo in the + // path would create an empty database and "restore" it over the live one. + if !isBackup { + // The driver splits the DSN at '?', so such a path would open a different file. + if strings.ContainsRune(path, '?') { + return fmt.Errorf("backup path cannot contain '?': %s", path) + } + if _, err := os.Stat(path); err != nil { + return fmt.Errorf("backup file not available: %w", err) + } + } + backupDb, err := sql.Open(Driver, path) if err != nil { return fmt.Errorf("opening backup database in '%s': %w", path, err) diff --git a/db/backup_test.go b/db/backup_test.go index 5d1bfc6e3..609cba3d6 100644 --- a/db/backup_test.go +++ b/db/backup_test.go @@ -5,12 +5,14 @@ import ( "database/sql" "math/rand" "os" + "path/filepath" "time" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" . "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/tests" + "github.com/navidrome/navidrome/utils/singleton" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -103,6 +105,19 @@ var _ = Describe("database backups", func() { Entry("delete all files", 0, 0), Entry("preserve all files when at length", len(timesDecreasingChronologically), len(timesDecreasingChronologically)), Entry("preserve all files when less than count", 10000, len(timesDecreasingChronologically))) + + It("ignores SQLite sidecar files when counting backups", func() { + for _, suffix := range []string{"-shm", "-wal"} { + file, err := os.Create(BackupPath(timesDecreasingChronologically[0]) + suffix) + Expect(err).ToNot(HaveOccurred()) + _ = file.Close() + } + + conf.Server.Backup.Count = len(timesDecreasingChronologically) + pruneCount, err := Prune(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(pruneCount).To(BeZero()) + }) }) Describe("backup and restore", Ordered, func() { @@ -148,4 +163,78 @@ var _ = Describe("database backups", func() { Expect(IsSchemaEmpty(ctx, Db())).To(BeFalse()) }) }) + + Describe("backup and restore with a file-based database", Ordered, func() { + var ctx context.Context + var tempFolder string + var dbFilePath string + + BeforeAll(func() { + ctx = context.Background() + DeferCleanup(configtest.SetupConfig()) + + var err error + tempFolder, err = os.MkdirTemp("", "navidrome_restore") + Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { + Close(ctx) + _ = os.RemoveAll(tempFolder) + }) + + // Mimic the production DSN (consts.DefaultDbPath): a file database in WAL mode. + dbFilePath = filepath.Join(tempFolder, "navidrome.db") + conf.Server.DbPath = dbFilePath + "?_busy_timeout=15000&_journal_mode=WAL&_foreign_keys=on&synchronous=normal" + // The previous container's cleanup closed the shared *sql.DB without + // dropping the singleton; force a fresh connection for this container. + singleton.DeleteInstance[*sql.DB]() + DeferCleanup(Init(ctx)) + }) + + It("restores data into a database whose stale WAL sidecar files were left behind", func() { + By("seeding user data in the current database") + _, err := Db().ExecContext(ctx, `INSERT INTO user (id, user_name, name, email, password, is_admin, created_at, updated_at) + VALUES ('u-restore-1', 'drilladmin', 'drilladmin', 'drilladmin@example.com', 'x', 1, datetime('now'), datetime('now'))`) + Expect(err).ToNot(HaveOccurred()) + + By("creating a backup containing the user row") + path, err := Backup(ctx) + Expect(err).ToNot(HaveOccurred()) + + By("simulating the CLI exiting without closing the pool: sidecar files stay behind") + _, err = Db().ExecContext(ctx, "CREATE TABLE IF NOT EXISTS _restore_probe(x)") + Expect(err).ToNot(HaveOccurred()) + singleton.DeleteInstance[*sql.DB]() + + err = tests.ClearDB() + Expect(err).ToNot(HaveOccurred()) + + By("restoring the backup") + Expect(Restore(ctx, path)).To(Succeed()) + + By("verifying the restored data is readable through a fresh connection") + singleton.DeleteInstance[*sql.DB]() + var userName string + Expect(Db().QueryRowContext(ctx, "SELECT user_name FROM user WHERE id = 'u-restore-1'").Scan(&userName)).To(Succeed()) + Expect(userName).To(Equal("drilladmin")) + }) + + It("fails to restore from a backup file that does not exist, leaving the database intact", func() { + By("seeding user data in the current database") + _, err := Db().ExecContext(ctx, `INSERT INTO user (id, user_name, name, email, password, is_admin, created_at, updated_at) + VALUES ('u-restore-2', 'keepme', 'keepme', 'keepme@example.com', 'x', 1, datetime('now'), datetime('now'))`) + Expect(err).ToNot(HaveOccurred()) + + By("attempting a restore from a nonexistent file") + missingPath := filepath.Join(tempFolder, "does_not_exist.db") + err = Restore(ctx, missingPath) + Expect(err).To(HaveOccurred()) + + By("verifying the database was not wiped") + var userName string + Expect(Db().QueryRowContext(ctx, "SELECT user_name FROM user WHERE id = 'u-restore-2'").Scan(&userName)).To(Succeed()) + Expect(userName).To(Equal("keepme")) + _, statErr := os.Stat(missingPath) + Expect(statErr).To(MatchError(os.ErrNotExist)) + }) + }) })