Merge pull request #358 from AppSprout-dev/fix/backup-retention

fix: skip pre-migration backup when schema is current, add retention

Author: CalebisGross

cmd/mnemonic/main.go

@@ -1230,18 +1230,30 @@ func serveCommand(configPath string) {
 		die(exitPermission, fmt.Sprintf("creating data directory: %v", err), "check permissions on ~/.mnemonic/")
 	}
 
-	// Pre-migration safety backup (only if DB already exists)
+	// Pre-migration safety backup (only if DB exists AND schema is outdated)
 	if _, statErr := os.Stat(cfg.Store.DBPath); statErr == nil {
-		backupDir, bdErr := backup.EnsureBackupDir()
-		if bdErr != nil {
-			log.Warn("could not create backup directory for pre-migration backup", "error", bdErr)
-		} else {
-			bkPath, bkErr := backup.BackupSQLiteFile(cfg.Store.DBPath, backupDir)
-			if bkErr != nil {
-				log.Warn("pre-migration backup failed", "error", bkErr)
-			} else if bkPath != "" {
-				log.Info("pre-migration backup created", "path", bkPath)
+		currentVer, verErr := backup.ReadSchemaVersion(cfg.Store.DBPath)
+		if verErr != nil {
+			log.Warn("could not read schema version, will back up defensively", "error", verErr)
+			currentVer = -1 // force backup
+		}
+		if currentVer < sqlite.SchemaVersion {
+			backupDir, bdErr := backup.EnsureBackupDir()
+			if bdErr != nil {
+				log.Warn("could not create backup directory for pre-migration backup", "error", bdErr)
+			} else {
+				bkPath, bkErr := backup.BackupSQLiteFile(cfg.Store.DBPath, backupDir)
+				if bkErr != nil {
+					log.Warn("pre-migration backup failed", "error", bkErr)
+				} else if bkPath != "" {
+					log.Info("pre-migration backup created", "path", bkPath)
+				}
+				if pruneErr := backup.PruneOldBackups(backupDir, 3); pruneErr != nil {
+					log.Warn("failed to prune old backups", "error", pruneErr)
+				}
 			}
+		} else {
+			log.Debug("schema is current, skipping pre-migration backup")
 		}
 	}
 

internal/backup/export.go

@@ -2,14 +2,19 @@ package backup
 
 import (
 	"context"
+	"database/sql"
 	"encoding/json"
 	"fmt"
 	"io"
 	"os"
 	"path/filepath"
+	"sort"
+	"strings"
 	"time"
 
 	"github.com/appsprout-dev/mnemonic/internal/store"
+
+	_ "modernc.org/sqlite" // pure-Go SQLite driver
 )
 
 type ExportFormat string
@@ -165,6 +170,52 @@ func BackupSQLiteFile(dbPath string, backupDir string) (string, error) {
 	return backupPath, nil
 }
 
+// ReadSchemaVersion opens the database read-only and returns PRAGMA user_version.
+// Returns 0 for databases that have never had the version set (pre-existing DBs).
+func ReadSchemaVersion(dbPath string) (int, error) {
+	db, err := sql.Open("sqlite", dbPath+"?mode=ro")
+	if err != nil {
+		return 0, fmt.Errorf("opening database for version check: %w", err)
+	}
+	defer func() { _ = db.Close() }()
+
+	var version int
+	if err := db.QueryRow("PRAGMA user_version").Scan(&version); err != nil {
+		return 0, fmt.Errorf("reading user_version: %w", err)
+	}
+	return version, nil
+}
+
+// PruneOldBackups keeps the most recent `keep` pre-migration backup files in dir
+// and removes older ones. Only targets files matching the "pre_migrate_*.db" pattern.
+func PruneOldBackups(dir string, keep int) error {
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		return fmt.Errorf("reading backup directory: %w", err)
+	}
+
+	var backups []string
+	for _, e := range entries {
+		if !e.IsDir() && strings.HasPrefix(e.Name(), "pre_migrate_") && strings.HasSuffix(e.Name(), ".db") {
+			backups = append(backups, e.Name())
+		}
+	}
+
+	// Filenames contain timestamps, so lexicographic sort = chronological order.
+	sort.Strings(backups)
+
+	if len(backups) <= keep {
+		return nil
+	}
+
+	for _, name := range backups[:len(backups)-keep] {
+		if err := os.Remove(filepath.Join(dir, name)); err != nil {
+			return fmt.Errorf("removing old backup %s: %w", name, err)
+		}
+	}
+	return nil
+}
+
 func EnsureBackupDir() (string, error) {
 	homeDir, err := os.UserHomeDir()
 	if err != nil {

internal/backup/export_test.go

@@ -324,6 +324,115 @@ func TestEnsureBackupDir(t *testing.T) {
 	}
 }
 
+func TestReadSchemaVersion_AfterInitSchema(t *testing.T) {
+	_, dbPath, cleanup := setupTestStore(t)
+	defer cleanup()
+
+	ver, err := ReadSchemaVersion(dbPath)
+	if err != nil {
+		t.Fatalf("ReadSchemaVersion failed: %v", err)
+	}
+	if ver != sqlite.SchemaVersion {
+		t.Errorf("expected schema version %d, got %d", sqlite.SchemaVersion, ver)
+	}
+}
+
+func TestReadSchemaVersion_FreshDB(t *testing.T) {
+	// A fresh SQLite DB with no PRAGMA user_version set returns 0.
+	dir := t.TempDir()
+	dbPath := filepath.Join(dir, "fresh.db")
+
+	db, err := sql.Open("sqlite", dbPath)
+	if err != nil {
+		t.Fatalf("failed to open fresh db: %v", err)
+	}
+	// Create at least one table so the file exists on disk.
+	if _, err := db.Exec("CREATE TABLE t(id INTEGER)"); err != nil {
+		t.Fatalf("failed to create table: %v", err)
+	}
+	_ = db.Close()
+
+	ver, err := ReadSchemaVersion(dbPath)
+	if err != nil {
+		t.Fatalf("ReadSchemaVersion failed: %v", err)
+	}
+	if ver != 0 {
+		t.Errorf("expected version 0 for fresh DB, got %d", ver)
+	}
+}
+
+func TestPruneOldBackups(t *testing.T) {
+	dir := t.TempDir()
+
+	// Create 5 fake pre-migration backups with distinct timestamps.
+	names := []string{
+		"pre_migrate_2026-01-01_100000.db",
+		"pre_migrate_2026-01-02_100000.db",
+		"pre_migrate_2026-01-03_100000.db",
+		"pre_migrate_2026-01-04_100000.db",
+		"pre_migrate_2026-01-05_100000.db",
+	}
+	for _, n := range names {
+		if err := os.WriteFile(filepath.Join(dir, n), []byte("fake"), 0644); err != nil {
+			t.Fatalf("failed to create %s: %v", n, err)
+		}
+	}
+
+	// Also create a non-matching file that should NOT be pruned.
+	if err := os.WriteFile(filepath.Join(dir, "backup_2026-01-01.json"), []byte("keep"), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := PruneOldBackups(dir, 3); err != nil {
+		t.Fatalf("PruneOldBackups failed: %v", err)
+	}
+
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	var remaining []string
+	for _, e := range entries {
+		remaining = append(remaining, e.Name())
+	}
+
+	// Should have 3 pre_migrate files + 1 json file = 4 total
+	if len(remaining) != 4 {
+		t.Errorf("expected 4 files remaining, got %d: %v", len(remaining), remaining)
+	}
+
+	// The oldest two should be gone.
+	for _, name := range remaining {
+		if name == names[0] || name == names[1] {
+			t.Errorf("old backup %s should have been pruned", name)
+		}
+	}
+}
+
+func TestPruneOldBackups_FewFiles(t *testing.T) {
+	dir := t.TempDir()
+
+	// Only 2 backups, keep=3 — nothing should be deleted.
+	for _, n := range []string{"pre_migrate_2026-01-01_100000.db", "pre_migrate_2026-01-02_100000.db"} {
+		if err := os.WriteFile(filepath.Join(dir, n), []byte("fake"), 0644); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	if err := PruneOldBackups(dir, 3); err != nil {
+		t.Fatalf("PruneOldBackups failed: %v", err)
+	}
+
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(entries) != 2 {
+		t.Errorf("expected 2 files, got %d", len(entries))
+	}
+}
+
 // jsonContains checks if a JSON string contains a given substring.
 func jsonContains(jsonStr, substr string) bool {
 	return len(jsonStr) > 0 && contains(jsonStr, substr)

internal/store/sqlite/schema.go

@@ -6,6 +6,12 @@ import (
 	"strings"
 )
 
+// SchemaVersion is the current target schema version. Bump this whenever a new
+// migration is added. It is written to PRAGMA user_version after InitSchema
+// completes, and read by the pre-migration backup logic to skip backups when
+// the schema is already current.
+const SchemaVersion = 15
+
 const schema = `
 -- Raw observations before encoding
 CREATE TABLE IF NOT EXISTS raw_memories (
@@ -483,6 +489,11 @@ CREATE INDEX IF NOT EXISTS idx_amendments_memory ON memory_amendments(memory_id)
 		return fmt.Errorf("failed to add tool_usage.suggested_ids column: %w", err)
 	}
 
+	// Record the schema version so pre-migration backups can skip when current.
+	if _, err := db.Exec(fmt.Sprintf("PRAGMA user_version = %d", SchemaVersion)); err != nil {
+		return fmt.Errorf("failed to set user_version: %w", err)
+	}
+
 	return nil
 }