amadan

carlos / platform Public

hostbox: ColdRestore removes the temp database's -wal and -shm

A restored database is in WAL mode, so lease.CheckDBContext's read-only
open attached a .restore-<random>.db-wal and .db-shm it could not delete.
Install renamed only the main file and its cleanup only knew that name, so
every restore left a pair behind. Remove exactly the two names derived from
this restore's own temp path after the check and before the rename (as
moves.BuildGeneration does), and in the failure-path cleanup. No glob:
another restore's .restore-* files are left alone.

Test: a WAL-mode fixture restored on the success path and on a failed-check
path leaves no .restore-* file of its own and keeps a concurrent restore's.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
claude 6e2171850c3918ea406121a0b54a74cdad54ec73 parent d905a67
3 files changed, +146 −1
  • internal/hostbox/restore.go +25 −1
  • internal/hostbox/restore_test.go +7 −0
  • internal/hostbox/restore_wal_test.go +114 −0
diff --git a/internal/hostbox/restore.go b/internal/hostbox/restore.go
index 903e0ee4a..13a921f5e 100644
--- a/internal/hostbox/restore.go
+++ b/internal/hostbox/restore.go
@@ -93,7 +93,12 @@ func (r ColdRestore) Install(ctx context.Context, dbPath string, plan *genselect
return err
}
_ = os.Remove(tmpPath) // Litestream requires an absent output.
- defer os.Remove(tmpPath)
+ // The check below opens the temp database read-only; a restored database
+ // is in WAL mode, so that open attaches a -wal and a -shm it cannot
+ // delete on close. Remove exactly the names derived from this restore's
+ // own temp path, never a glob: another restore's .restore-* files may be
+ // in flight beside it.
+ defer removeRestoreTemp(tmpPath)
client := &pinnedClient{files: files}
replica := litestream.NewReplicaWithClient(nil, client)
opt := litestream.NewRestoreOptions()
@@ -126,6 +131,9 @@ func (r ColdRestore) Install(ctx context.Context, dbPath string, plan *genselect
if err := lease.CheckDBContext(ctx, tmpPath); err != nil {
return fmt.Errorf("check restored database: %w", err)
}
+ // The check may attach a WAL and a shared-memory file; neither is part
+ // of the restore's result and neither may follow the rename.
+ removeRestoreSidecars(tmpPath)
if err := syncFile(tmpPath); err != nil {
return err
}
@@ -187,6 +195,22 @@ func safeGeneration(s string) bool {
return s != "" && s != "." && s != ".." && !strings.ContainsAny(s, "/\\\x00\r\n")
}
+// removeRestoreSidecars removes the -wal and -shm files a check of the temp
+// database at tmpPath may have attached. Best effort, as in
+// moves.BuildGeneration: a name that is already absent is the normal case.
+func removeRestoreSidecars(tmpPath string) {
+ for _, p := range []string{tmpPath + "-wal", tmpPath + "-shm"} {
+ _ = os.Remove(p)
+ }
+}
+
+// removeRestoreTemp is the failure-path cleanup: the temp database and its
+// sidecars, by the names this restore derived and no others.
+func removeRestoreTemp(tmpPath string) {
+ _ = os.Remove(tmpPath)
+ removeRestoreSidecars(tmpPath)
+}
+
func purgeDB(path string) error {
for _, p := range []string{path, path + "-wal", path + "-shm", path + ".gen"} {
if err := os.Remove(p); err != nil && !errors.Is(err, os.ErrNotExist) {
diff --git a/internal/hostbox/restore_test.go b/internal/hostbox/restore_test.go
index 14e664f3e..0f9b4d943 100644
--- a/internal/hostbox/restore_test.go
+++ b/internal/hostbox/restore_test.go
@@ -281,6 +281,13 @@ func restoreLTXFixture(t *testing.T, value string, level int, min, max ltx.TXID)
if err = db.Close(); err != nil {
t.Fatal(err)
}
+ return ltxFromDBFile(t, path, level, min, max)
+}
+
+// ltxFromDBFile cuts the database file at path into one LTX object and
+// returns its key, bytes and file checksum.
+func ltxFromDBFile(t *testing.T, path string, level int, min, max ltx.TXID) (string, []byte, uint64) {
+ t.Helper()
raw := mustRead(t, path)
pageSize := int(binary.BigEndian.Uint16(raw[16:18]))
if pageSize == 1 {
diff --git a/internal/hostbox/restore_wal_test.go b/internal/hostbox/restore_wal_test.go
new file mode 100644
index 000000000..4c8413a0f
--- /dev/null
+++ b/internal/hostbox/restore_wal_test.go
@@ -0,0 +1,114 @@
+package hostbox
+
+import (
+ "database/sql"
+ "os"
+ "path/filepath"
+ "strings"
+ "testing"
+
+ "github.com/carlosframework/platform/internal/genselect"
+ "github.com/carlosframework/platform/internal/lease"
+)
+
+// walLTXFixture builds a snapshot whose database is in WAL mode, as every
+// database restored from a real litestream replica is (litestream requires
+// it). With tables false the schema is created and dropped again, so the
+// file is a valid WAL-mode database that the restore check refuses ("no
+// tables") after it has opened it.
+func walLTXFixture(t *testing.T, value string, tables bool) (string, []byte, uint64) {
+ t.Helper()
+ path := filepath.Join(t.TempDir(), "fixture.db")
+ db, err := sql.Open("sqlite", path)
+ if err != nil {
+ t.Fatal(err)
+ }
+ db.SetMaxOpenConns(1)
+ stmts := []string{`PRAGMA journal_mode=WAL`, `CREATE TABLE fixture(value TEXT)`}
+ if tables {
+ stmts = append(stmts, `INSERT INTO fixture VALUES ('`+value+`')`)
+ } else {
+ stmts = append(stmts, `DROP TABLE fixture`)
+ }
+ for _, s := range stmts {
+ if _, err := db.Exec(s); err != nil {
+ t.Fatalf("%s: %v", s, err)
+ }
+ }
+ if err := db.Close(); err != nil {
+ t.Fatal(err)
+ }
+ raw := mustRead(t, path)
+ if len(raw) < 20 || raw[18] != 2 || raw[19] != 2 {
+ t.Fatalf("fixture header says journal mode bytes %d,%d; want 2,2 (WAL)", raw[18], raw[19])
+ }
+ return ltxFromDBFile(t, path, 9, 1, 1)
+}
+
+func restoreLeftovers(t *testing.T, dir string) []string {
+ t.Helper()
+ es, err := os.ReadDir(dir)
+ if err != nil {
+ t.Fatal(err)
+ }
+ var out []string
+ for _, e := range es {
+ if strings.HasPrefix(e.Name(), ".restore-") {
+ out = append(out, e.Name())
+ }
+ }
+ return out
+}
+
+// A restored database is in WAL mode, so the read-only check attaches a -wal
+// and a -shm it cannot delete. Install must leave none of them behind, on the
+// way to success and on the way to a failed check, and must never touch the
+// .restore-* files of a concurrent restore.
+func TestColdRestoreLeavesNoRestoreTempFilesBehindWALDatabase(t *testing.T) {
+ cases := []struct {
+ name string
+ tables bool
+ wantErr string
+ }{
+ {name: "success", tables: true},
+ {name: "failed check", tables: false, wantErr: "check restored database"},
+ }
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ dir := t.TempDir()
+ db := filepath.Join(dir, "shop.db")
+ key, snapshot, crc := walLTXFixture(t, "selected", tc.tables)
+ rd := &restoreReader{data: map[string][]byte{key + "@v1": snapshot}}
+ // Another restore's files, in flight beside ours.
+ others := []string{".restore-other.db", ".restore-other.db-shm", ".restore-other.db-wal"}
+ for _, o := range others {
+ if err := os.WriteFile(filepath.Join(dir, o), []byte("not ours"), 0o600); err != nil {
+ t.Fatal(err)
+ }
+ }
+ err := (ColdRestore{Reader: rd, Bucket: "bucket", Generation: "one", Work: &restoreWork{}}).Install(t.Context(), db,
+ &genselect.Plan{Generation: "one", MaxTXID: 1, Pin: &genselect.Pin{SnapshotCRC64: crc}, Files: []genselect.File{{Key: key, VersionID: "v1", Level: 9, MinTXID: 1, MaxTXID: 1, Size: int64(len(snapshot))}}})
+ if tc.wantErr == "" {
+ if err != nil {
+ t.Fatalf("Install: %v", err)
+ }
+ if err := lease.CheckDB(db); err != nil {
+ t.Fatalf("installed database: %v", err)
+ }
+ // The query above may attach its own sidecars to the installed
+ // database; they are named for it, not for the restore temp.
+ } else {
+ if err == nil || !strings.Contains(err.Error(), tc.wantErr) {
+ t.Fatalf("Install error = %v, want %q", err, tc.wantErr)
+ }
+ if _, err := os.Stat(db); !os.IsNotExist(err) {
+ t.Fatalf("a refused restore left a database: %v", err)
+ }
+ }
+ got := restoreLeftovers(t, dir)
+ if strings.Join(got, ",") != strings.Join(others, ",") {
+ t.Fatalf(".restore-* files = %v, want only the concurrent restore's %v", got, others)
+ }
+ })
+ }
+}