From 89a861a98934897468367df978d0870c4bedd2b7 Mon Sep 17 00:00:00 2001 From: clawbot Date: Sat, 3 Oct 2026 12:22:29 +0000 Subject: [PATCH] Open the database read-only for report and trees (closes #8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit report and trees now connect read-only (mode=ro, query_only, the same busy timeout) and no longer set the journal mode, which is a write. A read-only connection to a WAL database still needs its -wal and -shm files, or write access to the directory to create them, so scan now switches the database back to rollback-journal mode whenever it closes it: between scans the file alone holds the database. If a report has the database open at that moment the switch is refused; scan warns and the database stays in WAL mode, with its -wal and -shm files, until the next scan. README §Database states what readers need. Model: opus-5-5 --- README.md | 32 +++++++++++++------ TODO.md | 4 +++ db.go | 48 +++++++++++++++++++++------- db_test.go | 44 +++++++++++++++++++++++++ main_test.go | 90 ++++++++++++++++++++++++++++++++++++++++++++++++---- report.go | 6 ++-- scan.go | 7 ++-- 7 files changed, 197 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index d9c042a..d48f695 100644 --- a/README.md +++ b/README.md @@ -152,16 +152,28 @@ All three subcommands operate on a single SQLite database file: use. `report` and `trees` require an existing database; a missing database file is a fatal error (exit 1) telling the user to run `scan` first. -- The database uses WAL journal mode and a busy timeout, so running a - report while a cron `scan` is in progress is safe. The filesystem - is authoritative; the database is an eventually-consistent - reflection of it. Hashed records are committed in batched - transactions while the scan is still running (keeping the WAL - small and letting concurrent reports observe progress), so a - report may see a scan's changes partially applied, and a scan - that dies partway leaves a valid database holding everything - hashed so far; the next scan skips those records and converges - toward the filesystem. +- While `scan` runs, the database is in WAL journal mode with a busy + timeout, so running a report while a cron `scan` is in progress is + safe. The filesystem is authoritative; the database is an + eventually-consistent reflection of it. Hashed records are + committed in batched transactions while the scan is still running + (keeping the WAL small and letting concurrent reports observe + progress), so a report may see a scan's changes partially applied, + and a scan that dies partway leaves a valid database holding + everything hashed so far; the next scan skips those records and + converges toward the filesystem. +- `scan` switches the database back to rollback-journal mode when it + closes it, so between scans the database file alone holds the whole + database. Each switch needs the database to itself: a `scan` that + starts while a report is still reading waits for it up to the + 10-second busy timeout, then fails; a `scan` that ends while a + report has the database open warns and leaves the database in WAL + mode until the next scan. +- `report` and `trees` open the database read-only and need only read + access to the database file, and no write access to its directory. + While the database is in WAL mode they also read the `-wal` and + `-shm` files beside it, which SQLite creates with the database + file's permissions. - Schema (`PRAGMA user_version` is the schema version, currently 1; a database with any other version is a fatal error): diff --git a/TODO.md b/TODO.md index 7724ba8..c6de6e9 100644 --- a/TODO.md +++ b/TODO.md @@ -29,6 +29,10 @@ # Completed Steps +- `report` and `trees` open the database read-only, and `scan` leaves it + out of WAL mode, so reading needs only read access (2026-10-03, closes + https://git.eeqj.de/sneak/sfdupes/issues/8) + - escape tabs, newlines, carriage returns and backslashes in report, trees and warning paths; the root directory's path is `/` (2026-10-03, https://git.eeqj.de/sneak/sfdupes/issues/7) diff --git a/db.go b/db.go index f732608..2060dc4 100644 --- a/db.go +++ b/db.go @@ -74,16 +74,24 @@ func databasePath() string { return defaultDatabasePath } -// openDB opens the SQLite database at path with WAL journaling and a -// busy timeout, so a report can run while a cron scan is in progress. -// It does not create or verify the schema. -func openDB(path string) (*sql.DB, error) { - dsn := "file:" + path + - "?_pragma=busy_timeout(10000)" + - "&_pragma=journal_mode(WAL)" + - "&_pragma=synchronous(NORMAL)" +// scanParams are the connection parameters for scan: read-write, with +// WAL journaling and a busy timeout, so a report can run while a cron +// scan is in progress. closeScanDatabase leaves WAL mode again. +const scanParams = "_pragma=busy_timeout(10000)" + + "&_pragma=journal_mode(WAL)" + + "&_pragma=synchronous(NORMAL)" - db, err := sql.Open("sqlite", dsn) +// reportParams are the connection parameters for report and trees: +// read-only, with the same busy timeout. They set no journal mode, +// because setting one is a write. +const reportParams = "mode=ro" + + "&_pragma=busy_timeout(10000)" + + "&_pragma=query_only(1)" + +// openDB opens the SQLite database at path with the connection +// parameters params. It does not create or verify the schema. +func openDB(path, params string) (*sql.DB, error) { + db, err := sql.Open("sqlite", "file:"+path+"?"+params) if err != nil { return nil, fmt.Errorf("open database %s: %w", path, err) } @@ -104,7 +112,7 @@ func openScanDatabase(ctx context.Context, path string) (*sql.DB, error) { return nil, fmt.Errorf("create database directory: %w", err) } - db, err := openDB(path) + db, err := openDB(path, scanParams) if err != nil { return nil, err } @@ -119,6 +127,24 @@ func openScanDatabase(ctx context.Context, path string) (*sql.DB, error) { return db, nil } +// closeScanDatabase switches the database at path from WAL back to +// rollback-journal mode and closes it. Out of WAL mode the database +// file alone holds the whole database, so a reader needs no -wal or +// -shm file beside it, nor write access to create them. The switch +// fails while a report has the database open; the database then stays +// in WAL mode, still readable, until a later scan closes it. +func closeScanDatabase(ctx context.Context, db *sql.DB, path string) { + // Runs on the way out of a cancelled scan too. + _, err := db.ExecContext(context.WithoutCancel(ctx), + "PRAGMA journal_mode = DELETE") + if err != nil { + fmt.Fprintf(os.Stderr, "scan: database %s left in WAL mode: %v\n", + path, err) + } + + _ = db.Close() +} + // openReportDatabase opens an existing database for the report and // trees subcommands. A missing database file is an error directing the // user to run scan first; the schema version must match exactly. @@ -134,7 +160,7 @@ func openReportDatabase(ctx context.Context, return nil, fmt.Errorf("database: %w", err) } - db, err := openDB(path) + db, err := openDB(path, reportParams) if err != nil { return nil, err } diff --git a/db_test.go b/db_test.go index b68bddd..bc66c9c 100644 --- a/db_test.go +++ b/db_test.go @@ -5,6 +5,7 @@ import ( "database/sql" "errors" "fmt" + "os" "path/filepath" "slices" "strings" @@ -130,6 +131,49 @@ func TestOpenReportDatabaseOK(t *testing.T) { _ = db.Close() } +func TestCloseScanDatabaseWhileReportOpen(t *testing.T) { + t.Parallel() + + // A report holding the database open stops scan from taking it out + // of WAL mode. The -wal and -shm files must then stay beside it, so + // that a later report still needs only read access. + path := testDBPath(t) + + scanDB, err := openScanDatabase(t.Context(), path) + if err != nil { + t.Fatal(err) + } + + reportDB, err := openReportDatabase(t.Context(), path) + if err != nil { + t.Fatal(err) + } + + closeScanDatabase(t.Context(), scanDB, path) + + _ = reportDB.Close() + + _, err = os.Stat(path + "-wal") + if err != nil { + t.Fatalf("no -wal left: the switch out of WAL mode was not "+ + "stopped: %v", err) + } + + makeReadOnly(t, path) + + reportDB, err = openReportDatabase(t.Context(), path) + if err != nil { + t.Fatalf("openReportDatabase: %v", err) + } + + defer func() { _ = reportDB.Close() }() + + _, err = loadFileRows(t.Context(), reportDB) + if err != nil { + t.Fatalf("loadFileRows: %v", err) + } +} + func TestApplyChangesRoundTrip(t *testing.T) { t.Parallel() diff --git a/main_test.go b/main_test.go index b307a82..432e9e4 100644 --- a/main_test.go +++ b/main_test.go @@ -44,6 +44,44 @@ func assertNoSidecars(t *testing.T, path string) { } } +// makeReadOnly takes write permission away from the database at path, +// from any WAL sidecar beside it, and from their directory, as for a +// user reading a database that a root cron scan keeps. Root ignores +// file permissions, so it skips the test when run as root. +func makeReadOnly(t *testing.T, path string) { + t.Helper() + + if os.Geteuid() == 0 { + t.Skip("root ignores file permissions") + } + + err := os.Chmod(path, 0o400) + if err != nil { + t.Fatal(err) + } + + for _, suffix := range walSuffixes { + err = os.Chmod(path+suffix, 0o400) + if err != nil && !errors.Is(err, fs.ErrNotExist) { + t.Fatal(err) + } + } + + dir := filepath.Dir(path) + + //nolint:gosec // reaching the database needs the search bit + err = os.Chmod(dir, 0o500) + if err != nil { + t.Fatal(err) + } + + // Runs before t.TempDir's own cleanup, which must delete the files. + t.Cleanup(func() { + //nolint:gosec // removing the directory needs its search bit back + _ = os.Chmod(dir, 0o700) + }) +} + // captureStdout redirects os.Stdout to a file for the rest of the test // and returns a function reading back everything written to it. Only // machine-readable data belongs on stdout (README design goal 4), so @@ -91,13 +129,13 @@ func captureStdout(t *testing.T) func() string { // brokenDatabase writes a database that opens cleanly and passes the // schema-version check but has no files table, so the first query // fails with the database already open: a fatal error on a path that -// owns an open database. +// owns an open database. It closes the database the way scan does. func brokenDatabase(t *testing.T) string { t.Helper() path := testDBPath(t) - db, err := openDB(path) + db, err := openDB(path, scanParams) if err != nil { t.Fatal(err) } @@ -108,10 +146,7 @@ func brokenDatabase(t *testing.T) string { t.Fatal(err) } - err = db.Close() - if err != nil { - t.Fatal(err) - } + closeScanDatabase(t.Context(), db, path) return path } @@ -144,7 +179,10 @@ func TestOpenDatabaseKeepsWALWhileOpen(t *testing.T) { func TestRunFatalAfterOpenClosesDatabase(t *testing.T) { // Every subcommand that owns an open database must close it when - // it fails: no os.Exit between the open and the return. + // it fails: no os.Exit between the open and the return. The + // sidecar check is evidence of the close only for scan: report and + // trees only read a database that is out of WAL mode, which leaves + // nothing on disk whether they close it or not. cases := map[string][]string{ cmdScan: {cmdScan}, cmdReport: {cmdReport}, @@ -395,3 +433,41 @@ func TestRunTreesSucceeds(t *testing.T) { assertNoSidecars(t, path) } + +func TestRunReportsNeedOnlyReadAccess(t *testing.T) { + // README §Database: report and trees need only read access to the + // database file. With its directory read-only as well, SQLite + // cannot create any file beside it. + path := testDBPath(t) + t.Setenv(databaseEnv, path) + + dupes := scanFixture(t) + assertNoSidecars(t, path) + makeReadOnly(t, path) + + cases := map[string]string{ + cmdReport: "first\tdupe\tsize\n" + + dupes[0] + "\t" + dupes[1] + "\t300\n", + cmdTrees: "first\tdupe\tfiles\tsize\n" + + filepath.Dir(dupes[0]) + "\t" + filepath.Dir(dupes[1]) + + "\t1\t300\n", + } + + for name, want := range cases { + var stderr bytes.Buffer + + stdout := captureStdout(t) + + code := run([]string{name}, &stderr) + if code != exitOK { + t.Errorf("run(%s) = %d, want %d; stderr: %s", + name, code, exitOK, stderr.String()) + + continue + } + + if got := stdout(); got != want { + t.Errorf("%s stdout = %q, want %q", name, got, want) + } + } +} diff --git a/report.go b/report.go index f02ce2b..00b7b01 100644 --- a/report.go +++ b/report.go @@ -31,9 +31,9 @@ type scanRec struct { // loadRecords opens the database and reads every file record for the // report and trees subcommands. Any database problem — including a // missing database — is fatal. The error is returned rather than -// exiting, so that the deferred close — which checkpoints the SQLite -// WAL — always runs; the database is closed before the caller formats -// its output, so it stays closed even if that output fails. +// exiting, so that the deferred close always runs; the database is +// closed before the caller formats its output, so it stays closed even +// if that output fails. func loadRecords(ctx context.Context) ([]scanRec, error) { dbPath := databasePath() diff --git a/scan.go b/scan.go index 6d7aaf4..a215c5a 100644 --- a/scan.go +++ b/scan.go @@ -87,8 +87,9 @@ type fileMeta struct { // hash only when its size, head, and tail match another file's. Flag // parsing and the at-least-one-operand check are done by cobra. Errors // are returned rather than exiting, so that the deferred close — which -// checkpoints the SQLite WAL — always runs. Cancelling ctx unwinds the -// worker pools and aborts the scan with the context's error. +// takes the database out of WAL mode — always runs. Cancelling ctx +// unwinds the worker pools and aborts the scan with the context's +// error. func runScan(ctx context.Context, roots []string, workers int, oneFS bool, ) error { @@ -108,7 +109,7 @@ func runScan(ctx context.Context, roots []string, workers int, return err } - defer func() { _ = db.Close() }() + defer closeScanDatabase(ctx, db, dbPath) st, err := syncScan(ctx, db, roots, workers, oneFS) if err != nil { -- 2.54.0