diff --git a/README.md b/README.md index 255bb36..27f10b4 100644 --- a/README.md +++ b/README.md @@ -271,16 +271,21 @@ All three subcommands operate on a single SQLite database file: 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. + by appending `.lock` to the database's path with symlinks resolved + (`/var/lib/sfdupes/db.sqlite.lock` by default), taken before it walks the + filesystem or opens the database. Every path to one database, through a + symlink to the file or to a directory above it, names the same lock file. Just + before it takes the lock, `scan` creates the database file, empty, if it is + missing (through a symlink at the database path, it creates the symlink's + target), so the lock file is named the same way before the first scan sets up + the database as after. 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 eventually-consistent reflection of it. diff --git a/TODO.md b/TODO.md index 2e0aeda..ef00742 100644 --- a/TODO.md +++ b/TODO.md @@ -29,6 +29,11 @@ # Completed Steps +- the scan lock file is named from the database's path with symlinks resolved, + after `scan` creates the database file if it is missing, so a scan through a + symlink to the database fails while another scan holds the lock (2026-10-14, + https://git.eeqj.de/sneak/sfdupes/issues/101) + - on a terminal, a warning for the last file of a phase no longer erases the phase's finished bar: the bar is drawn again below the warning (2026-10-14, https://git.eeqj.de/sneak/sfdupes/issues/102) diff --git a/db.go b/db.go index 07b39cf..e0dff42 100644 --- a/db.go +++ b/db.go @@ -35,6 +35,10 @@ const schemaVersion = 1 // scan. const dbDirPerm = 0o755 +// dbFilePerm is the mode for a database file created by scan, the mode +// SQLite gives a database file it creates. +const dbFilePerm = 0o644 + // 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. @@ -140,19 +144,37 @@ func openDB(path, params string) (*sql.DB, error) { // 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. +// by the database's path with symlinks resolved and ".lock" appended, +// so every path to one database names the same lock file. If missing, +// the database's parent directory and an empty database file are +// created first, so the lock file is named the same way before the +// database is set up as after. 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 lock 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" + // Like SQLite, this follows a symlink at path and creates its + // target. SQLite opens an empty file as a new database. + //nolint:gosec // the operator chooses the database path + dbFile, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, dbFilePerm) + if err != nil { + return nil, fmt.Errorf("create database: %w", err) + } + + _ = dbFile.Close() + + resolved, err := filepath.EvalSymlinks(path) + if err != nil { + return nil, fmt.Errorf("resolve database path: %w", err) + } + + lockPath := resolved + ".lock" //nolint:gosec // the operator chooses the database path f, err := os.OpenFile(lockPath, os.O_RDWR|os.O_CREATE, lockFilePerm) diff --git a/db_test.go b/db_test.go index 6920d6e..e4b8600 100644 --- a/db_test.go +++ b/db_test.go @@ -13,11 +13,19 @@ import ( "time" ) -// testDBPath returns a database path inside a fresh temp dir. +// testDBPath returns a database path inside a fresh temp dir, with +// symlinks resolved as the scan lock resolves them, so the lock file +// is this path plus ".lock" even where the temp dir is reached through +// a symlink. func testDBPath(t *testing.T) string { t.Helper() - return filepath.Join(t.TempDir(), "db.sqlite") + dir, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + + return filepath.Join(dir, "db.sqlite") } // openTestDB creates a fresh scan database in a temp dir. diff --git a/main_test.go b/main_test.go index 7742365..ca6faf7 100644 --- a/main_test.go +++ b/main_test.go @@ -822,7 +822,7 @@ func holdScanLock(t *testing.T, path string) { 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 + // fails at once, naming the lock file, without opening the // database. path := testDBPath(t) t.Setenv(databaseEnv, path) @@ -848,10 +848,99 @@ func TestRunSecondScanFails(t *testing.T) { 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) + // Taking the lock created the database file, empty; opening it as + // a database would have written to it. + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) } + + if info.Size() != 0 { + t.Errorf("database size = %d, want 0 (not opened)", info.Size()) + } +} + +// assertSecondScanFails runs a scan against the database +// SFDUPES_DATABASE names while the test holds the lock on it. It fails +// the test unless that scan fails at once, naming the lock file beside +// path, the database's real path. +func assertSecondScanFails(t *testing.T, path string) { + t.Helper() + + var stderr bytes.Buffer + + code := run([]string{cmdScan, t.TempDir()}, io.Discard, &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) + } +} + +func TestRunSecondScanThroughSymlinkFails(t *testing.T) { + // README §Database: every path to one database names the same lock + // file, so while a scan holds it through one path, a scan through + // another fails, naming the lock file beside the database's real + // path. + t.Run("symlink to the database", func(t *testing.T) { + path := testDBPath(t) + t.Setenv(databaseEnv, path) + + scanFixture(t) + + link := filepath.Join(t.TempDir(), "link.sqlite") + + err := os.Symlink(path, link) + if err != nil { + t.Fatal(err) + } + + holdScanLock(t, path) + t.Setenv(databaseEnv, link) + assertSecondScanFails(t, path) + }) + + t.Run("symlink to its directory, no database yet", func(t *testing.T) { + path := testDBPath(t) + link := filepath.Join(t.TempDir(), "link") + + err := os.Symlink(filepath.Dir(path), link) + if err != nil { + t.Fatal(err) + } + + holdScanLock(t, path) + t.Setenv(databaseEnv, filepath.Join(link, filepath.Base(path))) + assertSecondScanFails(t, path) + }) + + t.Run("symlink whose target does not exist yet", func(t *testing.T) { + path := testDBPath(t) + link := filepath.Join(t.TempDir(), "link.sqlite") + + err := os.Symlink(path, link) + if err != nil { + t.Fatal(err) + } + + // The first scan takes the lock through the link before the + // database exists, then creates the database through it. + holdScanLock(t, link) + + db, err := openScanDatabase(t.Context(), link) + if err != nil { + t.Fatal(err) + } + + closeScanDatabase(t.Context(), db, link) + + t.Setenv(databaseEnv, link) + assertSecondScanFails(t, path) + }) } func TestRunScanReleasesLock(t *testing.T) {