From 705c8729cac4a46f751cc4353472328131f59059 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sun, 4 Oct 2026 02:30:21 +0200 Subject: [PATCH] Hold a lock so a second scan fails at once (closes #53) scan takes an exclusive flock(2) on a lock file beside the database (its path with .lock appended) before it walks anything or opens the database, and holds it until it returns. A second scan against the same database fails at once with a one-line error naming the lock file and exits 1. report and trees never take the lock. The lock ends with the process, so a fatal error or an interrupt releases it; the file is never deleted. golang.org/x/sys becomes a direct dependency. The README smoke test now keeps the database outside the scanned tree, where its empty lock file would have joined the empty-file group. Model: opus-5-5 --- README.md | 34 ++++++++++++++------ TODO.md | 4 +++ db.go | 47 +++++++++++++++++++++++++++ go.mod | 2 +- main_test.go | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++ scan.go | 19 ++++++++--- 6 files changed, 180 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 527b3fd..36a99ce 100644 --- a/README.md +++ b/README.md @@ -113,8 +113,9 @@ Goals, in order: `sfdupes`. - Dependencies: standard library, `github.com/spf13/cobra` for the CLI, **one progress-bar library** - (`github.com/schollz/progressbar/v3`), and **one SQLite driver** - (`modernc.org/sqlite`, pure Go, so builds keep cgo disabled). + (`github.com/schollz/progressbar/v3`), **one SQLite driver** + (`modernc.org/sqlite`, pure Go, so builds keep cgo disabled), and + `golang.org/x/sys` for `flock(2)` (the scan lock, see "Database"). `github.com/spf13/viper` is permitted if configuration-file support is ever needed, but is not currently used. No other third-party deps. @@ -152,6 +153,20 @@ 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. +- Only one `scan` runs against a database at a time. For its whole + run, `scan` holds an exclusive `flock(2)` lock on a lock file + beside the database, named by appending `.lock` to the database + path (`/var/lib/sfdupes/db.sqlite.lock` by default), taken before + it walks the filesystem or opens the database. A second `scan` + against the same database does not wait: it fails at once with a + one-line error naming the lock file and exits 1, without walking + anything or opening the database, and the running scan carries on. + The lock file is created on first use, open to its owner only, and + left in place: a leftover file blocks nothing, because the lock + ends with the process holding it however it ends, a fatal error or + an interrupt included, and deleting the file while a scan runs + would let a second scan start. `report` and `trees` never take the + lock, so they run during a scan. - 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 @@ -567,9 +582,10 @@ Additional requirements: ### Error handling and exit codes - `0`: success, even if individual files were skipped with warnings. -- `1`: fatal error (e.g., a `PATH` operand does not exist, the - database cannot be created/opened/read/written, a missing database - for `report`/`trees`, stdout write failure). +- `1`: fatal error (e.g., a `PATH` operand does not exist, another + `scan` is already running against the same database, the database + cannot be created/opened/read/written, a missing database for + `report`/`trees`, stdout write failure). - `2`: usage error (including `scan` with no `PATH` operand and `report`/`trees` with any positional argument). @@ -722,7 +738,7 @@ All of the following, run in this directory, must pass: ```sh d=$(mktemp -d) - export SFDUPES_DATABASE="$d/db.sqlite" + export SFDUPES_DATABASE="$(mktemp -d)/db.sqlite" mkdir -p "$d/a" "$d/b" head -c 2000 /dev/urandom > "$d/a/one.bin" cp "$d/a/one.bin" "$d/b/copy.bin" @@ -750,9 +766,9 @@ All of the following, run in this directory, must pass: ./sfdupes report ``` - (The scan database lives inside `$d` here purely for test hygiene; - scanning `$d` therefore also records the SQLite file itself, which - is harmless.) + (The database lives in a temp directory of its own: inside `$d`, + the scan would record it, and its empty lock file would join the + `empty1`/`empty2` group.) Expected from the first `report`: `one.bin`/`copy.bin`/`copy2.bin` form one group (two dupe rows, `first` is the lexicographically diff --git a/TODO.md b/TODO.md index 5928fe5..57b026f 100644 --- a/TODO.md +++ b/TODO.md @@ -29,6 +29,10 @@ # Completed Steps +- `scan` holds a lock on a lock file beside the database for its whole run, + so a second `scan` fails at once with exit 1 (2026-10-03, + https://git.eeqj.de/sneak/sfdupes/issues/53) + - test stdout write failures in `report` and `trees`; README states that `| head` ends sfdupes by `SIGPIPE` and `>&-` writes to `/dev/null` (2026-10-03, https://git.eeqj.de/sneak/sfdupes/issues/30) diff --git a/db.go b/db.go index 2060dc4..1c1ffac 100644 --- a/db.go +++ b/db.go @@ -11,6 +11,7 @@ import ( "slices" "strconv" + "golang.org/x/sys/unix" // The pure-Go SQLite driver, registered as "sqlite"; keeps cgo // disabled. _ "modernc.org/sqlite" @@ -32,6 +33,11 @@ const schemaVersion = 1 // scan. const dbDirPerm = 0o755 +// lockFilePerm is the mode for the scan lock file. Anyone who can open +// the file can hold the lock and keep every scan from running, so it +// is open to its owner only. +const lockFilePerm = 0o600 + // createTableSQL is the schema applied to a fresh database. Paths are // BLOBs because Unix paths are raw bytes, not guaranteed UTF-8. const createTableSQL = ` @@ -64,6 +70,10 @@ var errNoDatabase = errors.New( // does not understand. var errSchemaVersion = errors.New("unsupported database schema version") +// errScanRunning reports that another scan holds the lock on the +// database. +var errScanRunning = errors.New("another scan is running") + // databasePath resolves the database location: SFDUPES_DATABASE when // set and non-empty, the compiled-in default otherwise. func databasePath() string { @@ -104,6 +114,43 @@ func openDB(path, params string) (*sql.DB, error) { return db, nil } +// lockScanDatabase takes the lock that keeps a second scan off the +// database at path: an exclusive flock(2) on the file beside it named +// path with ".lock" appended, created along with the database's parent +// directory if missing. A lock held by another scan fails at once +// instead of waiting. The lock lasts until the returned file is closed +// or the process ends. The file is never deleted: a scan that deleted +// it would let the next scan lock a new file while another still holds +// the old one. +func lockScanDatabase(path string) (*os.File, error) { + err := os.MkdirAll(filepath.Dir(path), dbDirPerm) + if err != nil { + return nil, fmt.Errorf("create database directory: %w", err) + } + + lockPath := path + ".lock" + + //nolint:gosec // the operator chooses the database path + f, err := os.OpenFile(lockPath, os.O_RDWR|os.O_CREATE, lockFilePerm) + if err != nil { + return nil, err + } + + err = unix.Flock(int(f.Fd()), unix.LOCK_EX|unix.LOCK_NB) + if err != nil { + _ = f.Close() + + if errors.Is(err, unix.EWOULDBLOCK) { + return nil, fmt.Errorf("%w (lock held on %s)", + errScanRunning, lockPath) + } + + return nil, fmt.Errorf("lock %s: %w", lockPath, err) + } + + return f, nil +} + // openScanDatabase opens the database for the scan subcommand, creating // the file, its parent directory, and the schema as needed. func openScanDatabase(ctx context.Context, path string) (*sql.DB, error) { diff --git a/go.mod b/go.mod index 9b0faaa..5a13062 100644 --- a/go.mod +++ b/go.mod @@ -5,6 +5,7 @@ go 1.25.7 require ( github.com/schollz/progressbar/v3 v3.19.1 github.com/spf13/cobra v1.10.2 + golang.org/x/sys v0.46.0 modernc.org/sqlite v1.54.0 ) @@ -18,7 +19,6 @@ require ( github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec // indirect github.com/rivo/uniseg v0.4.7 // indirect github.com/spf13/pflag v1.0.9 // indirect - golang.org/x/sys v0.46.0 // indirect golang.org/x/term v0.44.0 // indirect modernc.org/libc v1.74.1 // indirect modernc.org/mathutil v1.7.1 // indirect diff --git a/main_test.go b/main_test.go index f04801b..ac53a0e 100644 --- a/main_test.go +++ b/main_test.go @@ -412,6 +412,95 @@ func TestRunReportsNeedOnlyReadAccess(t *testing.T) { } } +// holdScanLock takes the lock on the database at path, as a running +// scan does, and holds it until the test ends. It fails the test when +// the lock is already held. +func holdScanLock(t *testing.T, path string) { + t.Helper() + + lock, err := lockScanDatabase(path) + if err != nil { + t.Fatalf("lock %s: %v", path, err) + } + + t.Cleanup(func() { _ = lock.Close() }) +} + +func TestRunSecondScanFails(t *testing.T) { + // README §Database: while one scan holds the lock, a second scan + // fails at once, naming the lock file, without creating the + // database. + path := testDBPath(t) + t.Setenv(databaseEnv, path) + + holdScanLock(t, path) + + var stdout, stderr bytes.Buffer + + code := run([]string{cmdScan, t.TempDir()}, &stdout, &stderr) + if code != exitFatal { + t.Errorf("run(scan) = %d, want %d", code, exitFatal) + } + + want := "sfdupes: another scan is running (lock held on " + + path + ".lock)\n" + if got := stderr.String(); got != want { + t.Errorf("stderr = %q, want %q", got, want) + } + + if got := stdout.String(); got != "" { + t.Errorf("stdout = %q, want nothing (data only)", got) + } + + _, err := os.Stat(path) + if !errors.Is(err, fs.ErrNotExist) { + t.Errorf("stat %s = %v, want the database not created", path, err) + } +} + +func TestRunScanReleasesLock(t *testing.T) { + // README §Database: a scan releases the lock however it ends. + t.Run("success", func(t *testing.T) { + path := testDBPath(t) + t.Setenv(databaseEnv, path) + + scanFixture(t) + holdScanLock(t, path) + }) + + t.Run("fatal error", func(t *testing.T) { + path := brokenDatabase(t) + t.Setenv(databaseEnv, path) + + code := run([]string{cmdScan, t.TempDir()}, io.Discard, io.Discard) + if code != exitFatal { + t.Fatalf("run(scan) = %d, want %d", code, exitFatal) + } + + holdScanLock(t, path) + }) +} + +func TestRunReportsDuringScan(t *testing.T) { + // README §Database: report and trees never take the lock, so they + // run while a scan holds it. + path := testDBPath(t) + t.Setenv(databaseEnv, path) + + scanFixture(t) + holdScanLock(t, path) + + for _, name := range []string{cmdReport, cmdTrees} { + var stderr bytes.Buffer + + code := run([]string{name}, io.Discard, &stderr) + if code != exitOK { + t.Errorf("run(%s) = %d, want %d; stderr: %s", + name, code, exitOK, stderr.String()) + } + } +} + func TestRunStdoutClosedIsFatal(t *testing.T) { // README §Error handling: a stdout write failure exits 1, reported // in one line on stderr. diff --git a/scan.go b/scan.go index a215c5a..c46c42a 100644 --- a/scan.go +++ b/scan.go @@ -85,11 +85,13 @@ type fileMeta struct { // least one other file shares are ever hashed: a size-unique file // cannot be a duplicate. A file of headTailMin or more gets its content // 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 -// takes the database out of WAL mode — always runs. Cancelling ctx -// unwinds the worker pools and aborts the scan with the context's -// error. +// parsing and the at-least-one-operand check are done by cobra. The +// scan holds the lock on the database for its whole run, so a second +// scan fails before it walks the filesystem or opens the database. +// Errors are returned rather than exiting, so that the deferred close — +// which takes the database out of WAL mode — always runs, and the lock +// is released after it. 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 { @@ -104,6 +106,13 @@ func runScan(ctx context.Context, roots []string, workers int, dbPath := databasePath() + lock, err := lockScanDatabase(dbPath) + if err != nil { + return err + } + + defer func() { _ = lock.Close() }() + db, err := openScanDatabase(ctx, dbPath) if err != nil { return err