VACUUM snapshot metadata through the sqlite driver, not a CLI (closes #120)
check / check (pull_request) Successful in 2m40s
check / check (pull_request) Successful in 2m40s
snapshot create vacuumed the exported metadata database by shelling out to a sqlite3 binary, so a backup failed at the very end on any host without that CLI. vacuumDatabase now opens the database with the modernc.org/sqlite driver and runs VACUUM through it, outside any transaction; internal/snapshot no longer imports os/exec. The database opens in WAL mode, so the close after VACUUM checkpoints the rewrite into the main file that is compressed and uploaded. A new test deletes marked rows, vacuums, and asserts the file shrank and no longer holds the deleted bytes. script/bootstrap and the Dockerfile test image stop installing the CLI. Disclosure: the Dockerfile test-image change was not exercised by local make check, which builds only the lint image. Model: opus-4-8
This commit is contained in:
+4
-2
@@ -22,8 +22,10 @@ FROM golang:1.26.1-alpine@sha256:2389ebfa5b7f43eeafbd6be0c3700cc46690ef842ad962f
|
|||||||
|
|
||||||
ARG VERSION=dev
|
ARG VERSION=dev
|
||||||
|
|
||||||
# Install build dependencies for CGO (mattn/go-sqlite3) and sqlite3 CLI (tests)
|
# Build tooling: make, plus a C toolchain because `go test -race` needs cgo.
|
||||||
RUN apk add --no-cache make build-base sqlite
|
# 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
|
WORKDIR /src
|
||||||
|
|
||||||
|
|||||||
@@ -603,7 +603,6 @@ regardless of color setting (emoji are not color).
|
|||||||
and the pre-commit hook both run it. A `golangci-lint` installed on
|
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
|
`PATH` is not a substitute and is never used on a host, whatever its
|
||||||
version.
|
version.
|
||||||
* `sqlite3` CLI, which the test suite shells out to
|
|
||||||
* S3-compatible object storage (or local filesystem, or rclone remote)
|
* S3-compatible object storage (or local filesystem, or rclone remote)
|
||||||
|
|
||||||
## development workflow
|
## development workflow
|
||||||
@@ -634,8 +633,8 @@ standard: normalized scripts in `script/` are the entrypoints for the
|
|||||||
development workflow, and the Makefile targets are thin shims that call
|
development workflow, and the Makefile targets are thin shims that call
|
||||||
them. We provide:
|
them. We provide:
|
||||||
|
|
||||||
* `script/bootstrap` — install all development dependencies (go, sqlite3,
|
* `script/bootstrap` — install all development dependencies (go, Go
|
||||||
Go module download). It deliberately does not install `golangci-lint`;
|
module download). It deliberately does not install `golangci-lint`;
|
||||||
see `script/lint` below.
|
see `script/lint` below.
|
||||||
* `script/setup` — make a fresh clone ready for development: runs
|
* `script/setup` — make a fresh clone ready for development: runs
|
||||||
`script/bootstrap`, then `script/install-precommit`
|
`script/bootstrap`, then `script/install-precommit`
|
||||||
|
|||||||
@@ -25,6 +25,14 @@ release" is exactly the contradiction
|
|||||||
|
|
||||||
# Completed Steps
|
# 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`
|
||||||
|
test image 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`
|
- 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
|
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
|
base is `next`) and `next` itself get a CI run instead of relying on a
|
||||||
@@ -529,7 +537,7 @@ release" is exactly the contradiction
|
|||||||
was green was wrong.
|
was green was wrong.
|
||||||
- 2026-08-07: Added the standard `.golangci.yml` and `.editorconfig`
|
- 2026-08-07: Added the standard `.golangci.yml` and `.editorconfig`
|
||||||
(issue #59); lint findings under the new config are tracked in issue
|
(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,
|
- 2026-07-07 Adopted scripts-to-rule-them-all: `script/` entrypoints,
|
||||||
Makefile shims, README Entrypoints section
|
Makefile shims, README Entrypoints section
|
||||||
- 2026-07-02: Consolidated CLI verbs, retired overlapping commands; bound
|
- 2026-07-02: Consolidated CLI verbs, retired overlapping commands; bound
|
||||||
|
|||||||
+1
-1
@@ -192,7 +192,7 @@ Tracks blob upload metrics.
|
|||||||
After a snapshot is completed:
|
After a snapshot is completed:
|
||||||
1. Copy database to temporary file
|
1. Copy database to temporary file
|
||||||
2. Clean temporary database to contain only current snapshot data
|
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
|
4. Compress with zstd and encrypt with age
|
||||||
5. Upload to S3 as `metadata/{snapshot-id}/db.zst.age`
|
5. Upload to S3 as `metadata/{snapshot-id}/db.zst.age`
|
||||||
6. Generate blob manifest and upload as `metadata/{snapshot-id}/manifest.json.zst`
|
6. Generate blob manifest and upload as `metadata/{snapshot-id}/manifest.json.zst`
|
||||||
|
|||||||
@@ -44,7 +44,6 @@ import (
|
|||||||
"errors"
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
"io"
|
"io"
|
||||||
"os/exec"
|
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
"strings"
|
"strings"
|
||||||
"time"
|
"time"
|
||||||
@@ -669,14 +668,31 @@ func (sm *SnapshotManager) collectCleanupStats(
|
|||||||
|
|
||||||
// vacuumDatabase runs VACUUM on the database to remove deleted data and compact
|
// 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
|
// 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 {
|
func (sm *SnapshotManager) vacuumDatabase(ctx context.Context, dbPath string) error {
|
||||||
log.Debug("Running VACUUM on database", "path", dbPath)
|
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 {
|
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
|
return nil
|
||||||
|
|||||||
@@ -2,6 +2,7 @@
|
|||||||
package snapshot
|
package snapshot
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"bytes"
|
||||||
"context"
|
"context"
|
||||||
"database/sql"
|
"database/sql"
|
||||||
"io"
|
"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) {
|
func TestCleanSnapshotDBEmptySnapshot(t *testing.T) {
|
||||||
// Initialize logger
|
// Initialize logger
|
||||||
log.Initialize(log.Config{})
|
log.Initialize(log.Config{})
|
||||||
|
|||||||
@@ -114,9 +114,6 @@ main() {
|
|||||||
# from CI. Nothing on the host is ever used as a linter, at any
|
# from CI. Nothing on the host is ever used as a linter, at any
|
||||||
# version, so installing one here would buy nothing.
|
# 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
|
# goreleaser, at the version pinned by script/install-goreleaser and
|
||||||
# verified against a hardcoded sha256. Package managers are not used
|
# verified against a hardcoded sha256. Package managers are not used
|
||||||
# for it: they ship whatever version they happen to carry, and the
|
# for it: they ship whatever version they happen to carry, and the
|
||||||
|
|||||||
Reference in New Issue
Block a user