From 3d56dd7eb0f84a37dcdbbfcb3ebaed613f31c999 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 21 Sep 2026 19:41:28 +0200 Subject: [PATCH] VACUUM snapshot metadata through the sqlite driver, not a CLI (closes #120) snapshot create compacted the metadata database by running a sqlite3 command-line binary, after every blob had already been uploaded. On a host without that binary, which includes anyone who installed with go install, the backup failed at the last step, and two tests failed the same way. VACUUM now runs through the Go sqlite driver the program already uses, and its error is returned to the caller. The runtime Docker image no longer installs the sqlite package, since nothing in the binary calls it. model: claude-opus-4-8 (implementation, review); claude-fable-5-1 (merge) --- Dockerfile | 8 ++- README.md | 5 +- TODO.md | 11 +++- docs/DATAMODEL.md | 2 +- internal/snapshot/snapshot.go | 26 +++++++-- internal/snapshot/snapshot_test.go | 92 ++++++++++++++++++++++++++++++ script/bootstrap | 3 - 7 files changed, 131 insertions(+), 16 deletions(-) diff --git a/Dockerfile b/Dockerfile index 2ba39e3..51707b4 100644 --- a/Dockerfile +++ b/Dockerfile @@ -22,8 +22,10 @@ FROM golang:1.26.1-alpine@sha256:2389ebfa5b7f43eeafbd6be0c3700cc46690ef842ad962f ARG VERSION=dev -# Install build dependencies for CGO (mattn/go-sqlite3) and sqlite3 CLI (tests) -RUN apk add --no-cache make build-base sqlite +# Build tooling: make, plus a C toolchain because `go test -race` needs cgo. +# The sqlite driver is pure Go (modernc.org/sqlite), so no sqlite library or +# CLI is required. +RUN apk add --no-cache make build-base WORKDIR /src @@ -71,7 +73,7 @@ RUN CGO_ENABLED=0 go build -ldflags "-X 'sneak.berlin/go/vaultik/internal/global # alpine:3.21, 2026-02-25 FROM alpine:3.21@sha256:c3f8e73fdb79deaebaa2037150150191b9dcbfba68b4a46d70103204c53f4709 -RUN apk add --no-cache ca-certificates sqlite +RUN apk add --no-cache ca-certificates # Copy binary from builder COPY --from=builder /vaultik /usr/local/bin/vaultik diff --git a/README.md b/README.md index ccd0220..38218c1 100644 --- a/README.md +++ b/README.md @@ -613,7 +613,6 @@ regardless of color setting (emoji are not color). and the pre-commit hook both run it. A `golangci-lint` installed on `PATH` is not a substitute and is never used on a host, whatever its version. -* `sqlite3` CLI, which the test suite shells out to * S3-compatible object storage (or local filesystem, or rclone remote) ## development workflow @@ -644,8 +643,8 @@ standard: normalized scripts in `script/` are the entrypoints for the development workflow, and the Makefile targets are thin shims that call them. We provide: -* `script/bootstrap` — install all development dependencies (go, sqlite3, - Go module download). It deliberately does not install `golangci-lint`; +* `script/bootstrap` — install all development dependencies (go, Go + module download). It deliberately does not install `golangci-lint`; see `script/lint` below. * `script/setup` — make a fresh clone ready for development: runs `script/bootstrap`, then `script/install-precommit` diff --git a/TODO.md b/TODO.md index 3f2ef0d..7818366 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,15 @@ release" is exactly the contradiction # Completed Steps +- 2026-09-21: Made `snapshot create` VACUUM the per-snapshot metadata + database through the `modernc.org/sqlite` driver instead of shelling + out to the external `sqlite` command-line binary (issue #120). A + backup no longer needs that binary on `PATH`, so `make check` passes + on a stock `go install` host; `script/bootstrap` and the `Dockerfile` + (both the test-build and the shipped runtime stage) no longer install + it, and a new test asserts the uploaded database keeps no pages from + deleted rows. Dropped the now-false note on the 2026-08-07 entry below + that said bootstrap installs it. - 2026-09-21: Made `.gitea/workflows/check.yml` run on pushes to `main` and `next` and on pull requests against either, so unit PRs (whose base is `next`) and `next` itself get a CI run instead of relying on a @@ -529,7 +538,7 @@ release" is exactly the contradiction was green was wrong. - 2026-08-07: Added the standard `.golangci.yml` and `.editorconfig` (issue #59); lint findings under the new config are tracked in issue - #61. `script/bootstrap` now installs sqlite3 (needed by tests). + #61. - 2026-07-07 Adopted scripts-to-rule-them-all: `script/` entrypoints, Makefile shims, README Entrypoints section - 2026-07-02: Consolidated CLI verbs, retired overlapping commands; bound diff --git a/docs/DATAMODEL.md b/docs/DATAMODEL.md index 992ce1b..14f2435 100644 --- a/docs/DATAMODEL.md +++ b/docs/DATAMODEL.md @@ -192,7 +192,7 @@ Tracks blob upload metrics. After a snapshot is completed: 1. Copy database to temporary file 2. Clean temporary database to contain only current snapshot data -3. Export to SQL dump using sqlite3 +3. VACUUM the trimmed database so deleted rows leave no pages behind 4. Compress with zstd and encrypt with age 5. Upload to S3 as `metadata/{remote-key}/db.zst.age` 6. Generate blob manifest and upload as `metadata/{remote-key}/manifest.json.zst` diff --git a/internal/snapshot/snapshot.go b/internal/snapshot/snapshot.go index 36c674b..0c010db 100644 --- a/internal/snapshot/snapshot.go +++ b/internal/snapshot/snapshot.go @@ -44,7 +44,6 @@ import ( "errors" "fmt" "io" - "os/exec" "path/filepath" "strings" "time" @@ -669,14 +668,31 @@ func (sm *SnapshotManager) collectCleanupStats( // vacuumDatabase runs VACUUM on the database to remove deleted data and compact // This is critical for security - ensures no stale/deleted data pages are uploaded +// +// VACUUM runs through the modernc.org/sqlite driver, on a freshly opened +// connection with no transaction in flight (VACUUM cannot run inside one). +// The database opens in WAL mode, so VACUUM's rewrite lands in the WAL; the +// checkpoint on Close flushes it into the main file, which is the file we +// then compress and upload. func (sm *SnapshotManager) vacuumDatabase(ctx context.Context, dbPath string) error { log.Debug("Running VACUUM on database", "path", dbPath) - //nolint:gosec // G204: fixed argv; dbPath is our own temp file path - cmd := exec.CommandContext(ctx, "sqlite3", dbPath, "VACUUM;") - output, err := cmd.CombinedOutput() + db, err := database.New(ctx, dbPath) if err != nil { - return fmt.Errorf("running VACUUM: %w (output: %s)", err, string(output)) + return fmt.Errorf("opening database for VACUUM: %w", err) + } + + defer func() { + cerr := db.Close() + if cerr != nil { + log.Debug("Failed to close database after VACUUM", + "path", dbPath, "error", cerr) + } + }() + + _, err = db.ExecWithLog(ctx, "VACUUM") + if err != nil { + return fmt.Errorf("running VACUUM: %w", err) } return nil diff --git a/internal/snapshot/snapshot_test.go b/internal/snapshot/snapshot_test.go index 878366e..42061a2 100644 --- a/internal/snapshot/snapshot_test.go +++ b/internal/snapshot/snapshot_test.go @@ -2,6 +2,7 @@ package snapshot import ( + "bytes" "context" "database/sql" "io" @@ -96,6 +97,97 @@ func verifyCleanedDB( } } +// TestVacuumDatabaseRemovesDeletedData proves the export path uploads a +// compacted database: after rows carrying a recognizable marker are deleted +// and vacuumDatabase runs, no page holding that marker survives in the file +// on disk (the file compressFile later reads for upload). +func TestVacuumDatabaseRemovesDeletedData(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + fs := afero.NewOsFs() + + tempDir := t.TempDir() + dbPath := filepath.Join(tempDir, "snapshot.db") + + db, err := database.New(ctx, dbPath) + if err != nil { + t.Fatalf("failed to create database: %v", err) + } + + // A marker distinctive enough that its presence in the raw file can only + // come from the rows inserted below. + marker := []byte("VACUUM_PROBE_DEADBEEF_DELETED_ROW") + payload := bytes.Repeat(marker, 128) // ~4 KiB per row + + _, err = db.Conn().ExecContext(ctx, + "CREATE TABLE vacuum_probe (id INTEGER PRIMARY KEY, payload BLOB)") + if err != nil { + t.Fatalf("failed to create probe table: %v", err) + } + + for range 512 { + _, err = db.Conn().ExecContext(ctx, + "INSERT INTO vacuum_probe (payload) VALUES (?)", payload) + if err != nil { + t.Fatalf("failed to insert probe row: %v", err) + } + } + + _, err = db.Conn().ExecContext(ctx, "DELETE FROM vacuum_probe") + if err != nil { + t.Fatalf("failed to delete probe rows: %v", err) + } + + // Close so the deletes reach the main file, mirroring the state + // prepareExportDB hands to vacuumDatabase. + err = db.Close() + if err != nil { + t.Fatalf("failed to close database: %v", err) + } + + beforeInfo, err := fs.Stat(dbPath) + if err != nil { + t.Fatalf("failed to stat database before vacuum: %v", err) + } + + beforeBytes, err := afero.ReadFile(fs, dbPath) + if err != nil { + t.Fatalf("failed to read database before vacuum: %v", err) + } + + if !bytes.Contains(beforeBytes, marker) { + t.Fatalf("expected deleted-row data to linger before vacuum") + } + + sm := &SnapshotManager{fs: fs} + + err = sm.vacuumDatabase(ctx, dbPath) + if err != nil { + t.Fatalf("vacuumDatabase failed: %v", err) + } + + afterBytes, err := afero.ReadFile(fs, dbPath) + if err != nil { + t.Fatalf("failed to read database after vacuum: %v", err) + } + + if bytes.Contains(afterBytes, marker) { + t.Fatalf("deleted-row data survived vacuum in the uploaded file") + } + + afterInfo, err := fs.Stat(dbPath) + if err != nil { + t.Fatalf("failed to stat database after vacuum: %v", err) + } + + if afterInfo.Size() >= beforeInfo.Size() { + t.Fatalf("expected vacuum to shrink the file: before=%d after=%d", + beforeInfo.Size(), afterInfo.Size()) + } +} + func TestCleanSnapshotDBEmptySnapshot(t *testing.T) { // Initialize logger log.Initialize(log.Config{}) diff --git a/script/bootstrap b/script/bootstrap index 01febae..21862fe 100755 --- a/script/bootstrap +++ b/script/bootstrap @@ -114,9 +114,6 @@ main() { # from CI. Nothing on the host is ever used as a linter, at any # version, so installing one here would buy nothing. - # sqlite3 CLI: the test suite shells out to it (VACUUM). - if missing sqlite3; then pkg_install sqlite sqlite3 sqlite sqlite; fi - # goreleaser, at the version pinned by script/install-goreleaser and # verified against a hardcoded sha256. Package managers are not used # for it: they ship whatever version they happen to carry, and the