diff --git a/TODO.md b/TODO.md index bd9f750..1d91eeb 100644 --- a/TODO.md +++ b/TODO.md @@ -29,6 +29,20 @@ # Completed Steps +- guarantee the database is closed on every fatal exit path + (2026-08-09, branch `db-close-on-fatal`, closes #4): `fatalf` and + its `os.Exit(1)` are gone, so the deferred `db.Close()` — and with + it the SQLite WAL checkpoint — now actually runs when a subcommand + fails; `runScan`, `runReport`, `runTrees`, `loadRecords` and + `resolveRoots` return errors instead. The single exit point is `run` + in `main.go`: it maps a `fatalError` (anything a subcommand + returned) to exit 1 and cobra's own argument and flag errors to exit + 2, which keeps a runtime failure from being reported as a usage + error or printing the usage text. New `main_test.go` drives the CLI + in-process and asserts the exit codes from README §Error handling + plus the stdout/stderr split, including that a fatal error raised + after the database is open leaves no `-wal`/`-shm` sidecar behind + for `scan`, `report` or `trees` - update golangci-lint to v2.12.2 with the canonical config (2026-08-09, branch `golangci-v2.12.2`, merged as `38a01bd`, closes #3): bumped the pinned linter in the `Dockerfile` lint diff --git a/main.go b/main.go index 777d25a..873bf7d 100644 --- a/main.go +++ b/main.go @@ -16,20 +16,35 @@ package main import ( + "errors" "fmt" + "io" "os" "runtime" "github.com/spf13/cobra" ) -// Exit codes: 0 is success (even with per-file warnings), exitFatal is -// a fatal error, exitUsage is a usage error. +// Exit codes: exitOK is success (even with per-file warnings), +// exitFatal is a fatal error, exitUsage is a usage error. const ( + exitOK = 0 exitFatal = 1 exitUsage = 2 ) +// The subcommand names, as typed on the command line. +const ( + cmdScan = "scan" + cmdReport = "report" + cmdTrees = "trees" +) + +// errNoSubcommand is returned by the root command when it is invoked +// without a subcommand. That is a usage error, and the usage text +// cobra prints for it is the whole message. +var errNoSubcommand = errors.New("no subcommand") + // Version is the build version, injected at link time via -ldflags // (see the Makefile); "dev" for a plain go build. // @@ -37,22 +52,64 @@ const ( var Version = "dev" func main() { + os.Exit(run(os.Args[1:], os.Stderr)) +} + +// run executes args against the command tree and returns the process +// exit code. It is the program's single exit point: the subcommands +// return their errors instead of exiting, so every deferred cleanup — +// above all closing the database, which checkpoints the SQLite WAL — +// runs before the process ends. +func run(args []string, stderr io.Writer) int { + // A nil slice makes cobra fall back to os.Args, which would let a + // test binary's own flags reach the command tree. + if args == nil { + args = []string{} + } + + root := newRootCommand(stderr) + root.SetArgs(args) + + err := root.Execute() + + var fatal fatalError + + switch { + case err == nil: + return exitOK + case errors.As(err, &fatal): + // The command ran and failed: a runtime error, reported + // without the usage text that a usage error gets. + _, _ = fmt.Fprintf(stderr, "sfdupes: %v\n", err) + + return exitFatal + default: + // A usage error: cobra has already printed the message and + // the usage text. + return exitUsage + } +} + +// newRootCommand builds the command tree. Everything on stdout is +// machine-readable data; all human-facing output (help, usage, errors) +// goes to stderr. +func newRootCommand(stderr io.Writer) *cobra.Command { root := &cobra.Command{ Use: "sfdupes", Short: "Find candidate duplicate files by size and head/tail SHA-256", Version: Version, Args: cobra.NoArgs, - Run: func(cmd *cobra.Command, _ []string) { - // A missing subcommand prints usage and exits 2. - _ = cmd.Usage() + RunE: func(cmd *cobra.Command, _ []string) error { + // A missing subcommand prints usage and exits 2: cobra + // prints the usage text for the returned error, and run + // maps everything that is not a fatal error to exit 2. + cmd.SilenceErrors = true - os.Exit(exitUsage) + return errNoSubcommand }, } - // Everything on stdout is machine-readable data; all human-facing - // output (help, usage, errors) goes to stderr. - root.SetOut(os.Stderr) - root.SetErr(os.Stderr) + root.SetOut(stderr) + root.SetErr(stderr) root.CompletionOptions.DisableDefaultCmd = true var ( @@ -61,12 +118,12 @@ func main() { ) scanCmd := &cobra.Command{ - Use: "scan [--workers N] [-x] PATH...", + Use: cmdScan + " [--workers N] [-x] PATH...", Short: "Walk trees and synchronize the scan database", Args: cobra.MinimumNArgs(1), - Run: func(_ *cobra.Command, args []string) { - runScan(args, scanWorkers, scanOneFS) - }, + RunE: runE(func(args []string) error { + return runScan(args, scanWorkers, scanOneFS) + }), } scanCmd.Flags().IntVar(&scanWorkers, "workers", runtime.NumCPU(), "concurrent workers for the walk and hash phases") @@ -74,35 +131,55 @@ func main() { "do not cross filesystem boundaries") reportCmd := &cobra.Command{ - Use: "report", + Use: cmdReport, Short: "Read the scan database and print the file-level duplicates report", Args: cobra.NoArgs, - Run: func(_ *cobra.Command, _ []string) { - runReport() - }, + RunE: runE(func(_ []string) error { + return runReport() + }), } treesCmd := &cobra.Command{ - Use: "trees", + Use: cmdTrees, Short: "Read the scan database and print the duplicate-tree report", Args: cobra.NoArgs, - Run: func(_ *cobra.Command, _ []string) { - runTrees() - }, + RunE: runE(func(_ []string) error { + return runTrees() + }), } root.AddCommand(scanCmd, reportCmd, treesCmd) - err := root.Execute() - if err != nil { - // Cobra has already printed the error and usage to stderr; - // an invalid subcommand or bad arguments is a usage error. - os.Exit(exitUsage) + return root +} + +// runE adapts a subcommand implementation to cobra's RunE. Cobra +// prints the error and the command's usage text for every error RunE +// returns, but a subcommand that ran and failed has no usage problem +// to report: both are silenced here, and the error is marked fatal so +// that run reports it on stderr and exits 1 rather than 2. +func runE(fn func(args []string) error) func(*cobra.Command, []string) error { + return func(cmd *cobra.Command, args []string) error { + cmd.SilenceUsage = true + cmd.SilenceErrors = true + + err := fn(args) + if err != nil { + return fatalError{err: err} + } + + return nil } } -// fatalf reports a fatal error and exits 1. -func fatalf(format string, args ...any) { - fmt.Fprintf(os.Stderr, "sfdupes: "+format+"\n", args...) - os.Exit(exitFatal) +// fatalError marks a runtime failure, as opposed to the usage errors +// cobra itself produces while parsing arguments and flags. Both come +// out of Execute as plain errors, so the wrapper is what tells run to +// report this one as "sfdupes: ..." and exit 1. +type fatalError struct { + err error } + +func (e fatalError) Error() string { return e.err.Error() } + +func (e fatalError) Unwrap() error { return e.err } diff --git a/main_test.go b/main_test.go new file mode 100644 index 0000000..9dbff77 --- /dev/null +++ b/main_test.go @@ -0,0 +1,397 @@ +package main + +import ( + "bytes" + "context" + "errors" + "io" + "io/fs" + "os" + "path/filepath" + "strconv" + "strings" + "testing" +) + +// usageMarker is the first line of cobra's usage text, which a usage +// error prints and a runtime failure must not. +const usageMarker = "Usage:" + +// walSuffixes are the SQLite sidecar files a WAL-mode database keeps +// while it is open. A clean close checkpoints the WAL and removes +// both; finding either afterwards means the database was never closed. +// +//nolint:gochecknoglobals // a constant list, immutable by convention +var walSuffixes = []string{"-wal", "-shm"} + +// assertNoSidecars fails when a WAL sidecar is still present beside the +// database at path. +func assertNoSidecars(t *testing.T, path string) { + t.Helper() + + for _, suffix := range walSuffixes { + _, err := os.Stat(path + suffix) + if err == nil { + t.Errorf("%s%s still present: the database was not closed", + path, suffix) + + continue + } + + if !errors.Is(err, fs.ErrNotExist) { + t.Fatal(err) + } + } +} + +// 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 +// the tests assert on it directly. +func captureStdout(t *testing.T) func() string { + t.Helper() + + f, err := os.Create(filepath.Join(t.TempDir(), "stdout")) + if err != nil { + t.Fatal(err) + } + + saved := os.Stdout + os.Stdout = f + + t.Cleanup(func() { + os.Stdout = saved + + _ = f.Close() + }) + + return func() string { + // Read what has been written without disturbing the write + // offset, so the capture can be inspected more than once. + size, err := f.Seek(0, io.SeekCurrent) + if err != nil { + t.Fatal(err) + } + + if size == 0 { + return "" + } + + b := make([]byte, size) + + _, err = f.ReadAt(b, 0) + if err != nil { + t.Fatal(err) + } + + return string(b) + } +} + +// 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. +func brokenDatabase(t *testing.T) string { + t.Helper() + + path := testDBPath(t) + + db, err := openDB(path) + if err != nil { + t.Fatal(err) + } + + _, err = db.ExecContext(context.Background(), + "PRAGMA user_version = "+strconv.Itoa(schemaVersion)) + if err != nil { + t.Fatal(err) + } + + err = db.Close() + if err != nil { + t.Fatal(err) + } + + return path +} + +func TestOpenDatabaseKeepsWALWhileOpen(t *testing.T) { + t.Parallel() + + // The premise of the fatal-path tests below: an open database has + // a -wal sidecar, so its absence afterwards is evidence that the + // database was closed and its WAL checkpointed. + path := testDBPath(t) + + db, err := openScanDatabase(path) + if err != nil { + t.Fatal(err) + } + + _, err = os.Stat(path + "-wal") + if err != nil { + t.Fatalf("no -wal beside an open database: %v", err) + } + + err = db.Close() + if err != nil { + t.Fatal(err) + } + + assertNoSidecars(t, path) +} + +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. + cases := map[string][]string{ + cmdScan: {cmdScan}, + cmdReport: {cmdReport}, + cmdTrees: {cmdTrees}, + } + + for name, args := range cases { + t.Run(name, func(t *testing.T) { + path := brokenDatabase(t) + t.Setenv(databaseEnv, path) + + if name == cmdScan { + args = append(args, t.TempDir()) + } + + var stderr bytes.Buffer + + stdout := captureStdout(t) + + code := run(args, &stderr) + if code != exitFatal { + t.Errorf("run(%v) = %d, want %d", args, code, exitFatal) + } + + assertNoSidecars(t, path) + assertFatalOutput(t, stderr.String(), stdout()) + + // Proof that the failure happened after the open: only a + // query against the opened database can report this. + if !strings.Contains(stderr.String(), "no such table: files") { + t.Errorf("stderr = %q, want the failure to come from a "+ + "query on the open database", stderr.String()) + } + }) + } +} + +func TestRunMissingOperandIsFatalNotUsage(t *testing.T) { + // README §Error handling: a PATH operand that does not exist is a + // fatal error (1), not a usage error (2) — and a runtime failure + // must not dump the usage text. + t.Setenv(databaseEnv, testDBPath(t)) + + var stderr bytes.Buffer + + stdout := captureStdout(t) + + missing := filepath.Join(t.TempDir(), "nope") + + code := run([]string{cmdScan, missing}, &stderr) + if code != exitFatal { + t.Errorf("run(scan %s) = %d, want %d", missing, code, exitFatal) + } + + assertFatalOutput(t, stderr.String(), stdout()) +} + +// assertFatalOutput checks that a fatal error was reported the way +// README §Error handling and design goal 4 require: the message on +// stderr, prefixed with the program name, no usage text, and nothing +// at all on stdout. +func assertFatalOutput(t *testing.T, stderr, stdout string) { + t.Helper() + + if !strings.Contains(stderr, "sfdupes: ") { + t.Errorf("stderr = %q, want a \"sfdupes: \" error report", stderr) + } + + if strings.Contains(stderr, usageMarker) { + t.Errorf("stderr = %q, want no usage text for a runtime failure", + stderr) + } + + if stdout != "" { + t.Errorf("stdout = %q, want nothing (data only)", stdout) + } +} + +func TestRunUsageErrors(t *testing.T) { + // Usage errors keep exiting 2 with cobra's own report on stderr. + cases := map[string]struct { + args []string + want string + }{ + "no subcommand": {[]string{}, usageMarker}, + "scan without paths": {[]string{cmdScan}, usageMarker}, + "report with args": {[]string{cmdReport, "x"}, usageMarker}, + "trees with args": {[]string{cmdTrees, "x"}, usageMarker}, + "unknown flag": {[]string{cmdScan, "--nope", "/"}, usageMarker}, + "unknown subcommand": {[]string{"nope"}, "unknown command"}, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + // No usage error may reach the database, so point it at a + // path that does not exist. + t.Setenv(databaseEnv, testDBPath(t)) + + var stderr bytes.Buffer + + stdout := captureStdout(t) + + code := run(tc.args, &stderr) + if code != exitUsage { + t.Errorf("run(%v) = %d, want %d", tc.args, code, exitUsage) + } + + if !strings.Contains(stderr.String(), tc.want) { + t.Errorf("stderr = %q, want %q", stderr.String(), tc.want) + } + + if got := stdout(); got != "" { + t.Errorf("stdout = %q, want nothing (data only)", got) + } + }) + } +} + +// TestRunHelpAndVersionSucceed checks that the two informational flags +// exit 0 and keep their human-facing output on stderr. +// +//nolint:paralleltest // captureStdout replaces the process-wide os.Stdout +func TestRunHelpAndVersionSucceed(t *testing.T) { + assertHumanOutput(t, "--help") + assertHumanOutput(t, "--version") +} + +// assertHumanOutput runs sfdupes with one informational flag and checks +// that it succeeds with its output on stderr and stdout untouched +// (README design goal 4). +func assertHumanOutput(t *testing.T, arg string) { + t.Helper() + + var stderr bytes.Buffer + + stdout := captureStdout(t) + + code := run([]string{arg}, &stderr) + if code != exitOK { + t.Errorf("run(%s) = %d, want %d", arg, code, exitOK) + } + + if stderr.Len() == 0 { + t.Errorf("run(%s) wrote nothing to stderr", arg) + } + + if got := stdout(); got != "" { + t.Errorf("stdout = %q, want nothing (data only)", got) + } +} + +// scanFixture builds a small tree holding one duplicate pair and one +// unreadable file and scans it into the database the caller has +// pointed SFDUPES_DATABASE at. The unreadable file makes the scan warn +// and skip, which README §Error handling still calls a successful run. +// It returns the duplicate pair's paths. +func scanFixture(t *testing.T) []string { + t.Helper() + + dir := t.TempDir() + + dupes := []string{ + writeFile(t, dir, "one/a.bin", pattern(1, 300)), + writeFile(t, dir, "two/a.bin", pattern(1, 300)), + } + + // Same size as the pair, so the scan queues it for hashing and the + // read fails. + unreadable := writeFile(t, dir, "unreadable.bin", pattern(2, 300)) + + err := os.Chmod(unreadable, 0) + if err != nil { + t.Fatal(err) + } + + var stderr bytes.Buffer + + stdout := captureStdout(t) + + code := run([]string{cmdScan, dir}, &stderr) + if code != exitOK { + t.Fatalf("run(scan) = %d, want %d; stderr: %s", + code, exitOK, stderr.String()) + } + + if got := stdout(); got != "" { + t.Errorf("scan stdout = %q, want nothing (data only)", got) + } + + return dupes +} + +func TestRunScanSucceedsDespiteWarnings(t *testing.T) { + path := testDBPath(t) + t.Setenv(databaseEnv, path) + + scanFixture(t) + assertNoSidecars(t, path) +} + +func TestRunReportSucceeds(t *testing.T) { + path := testDBPath(t) + t.Setenv(databaseEnv, path) + + dupes := scanFixture(t) + + var stderr bytes.Buffer + + stdout := captureStdout(t) + + code := run([]string{cmdReport}, &stderr) + if code != exitOK { + t.Fatalf("run(report) = %d, want %d; stderr: %s", + code, exitOK, stderr.String()) + } + + want := "first\tdupe\tsize\n" + dupes[0] + "\t" + dupes[1] + "\t300\n" + if got := stdout(); got != want { + t.Errorf("stdout = %q, want %q", got, want) + } + + assertNoSidecars(t, path) +} + +func TestRunTreesSucceeds(t *testing.T) { + path := testDBPath(t) + t.Setenv(databaseEnv, path) + + dupes := scanFixture(t) + + var stderr bytes.Buffer + + stdout := captureStdout(t) + + code := run([]string{cmdTrees}, &stderr) + if code != exitOK { + t.Fatalf("run(trees) = %d, want %d; stderr: %s", + code, exitOK, stderr.String()) + } + + // The two directories holding the duplicate pair are duplicate + // trees of each other. + want := "first\tdupe\tfiles\tsize\n" + + filepath.Dir(dupes[0]) + "\t" + filepath.Dir(dupes[1]) + "\t1\t300\n" + if got := stdout(); got != want { + t.Errorf("stdout = %q, want %q", got, want) + } + + assertNoSidecars(t, path) +} diff --git a/report.go b/report.go index 4324155..f326fb6 100644 --- a/report.go +++ b/report.go @@ -28,23 +28,26 @@ 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. -func loadRecords() []scanRec { +// 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. +func loadRecords() ([]scanRec, error) { dbPath := databasePath() db, err := openReportDatabase(dbPath) if err != nil { - fatalf("%v", err) + return nil, err } defer func() { _ = db.Close() }() recs, err := loadFileRows(db) if err != nil { - fatalf("database %s: %v", dbPath, err) + return nil, fmt.Errorf("database %s: %w", dbPath, err) } - return recs + return recs, nil } // dupeGroup is one set of candidate-duplicate files: identical size, @@ -59,15 +62,19 @@ type dupeGroup struct { // from the database and prints the file-level duplicates report as TSV // on stdout. It never touches the scanned filesystem; its only I/O is // the database, stdout, and stderr. -func runReport() { - recs := loadRecords() +func runReport() error { + recs, err := loadRecords() + if err != nil { + return err + } + dupes := collectDupeGroups(recs) out := bufio.NewWriterSize(os.Stdout, ioBufSize) - _, err := fmt.Fprintln(out, "first\tdupe\tsize") + _, err = fmt.Fprintln(out, "first\tdupe\tsize") if err != nil { - fatalf("write stdout: %v", err) + return fmt.Errorf("write stdout: %w", err) } dupeFiles := 0 @@ -79,7 +86,7 @@ func runReport() { _, err = fmt.Fprintf(out, "%s\t%s\t%d\n", g.paths[0], p, g.size) if err != nil { - fatalf("write stdout: %v", err) + return fmt.Errorf("write stdout: %w", err) } dupeFiles++ @@ -89,13 +96,15 @@ func runReport() { err = out.Flush() if err != nil { - fatalf("write stdout: %v", err) + return fmt.Errorf("write stdout: %w", err) } fmt.Fprintf(os.Stderr, "report: %d records read, %d duplicate groups, %d dupe files, "+ "%s reclaimable\n", len(recs), len(dupes), dupeFiles, humanBytes(reclaimable)) + + return nil } // collectDupeGroups groups records by signature and returns every group diff --git a/scan.go b/scan.go index ecc6b37..e33318b 100644 --- a/scan.go +++ b/scan.go @@ -49,25 +49,30 @@ type fileMeta struct { // under the PATH operands. Only files whose size at least one other // file shares are ever hashed: a size-unique file cannot be a // duplicate. Flag parsing and the at-least-one-operand check are done -// by cobra. -func runScan(roots []string, workers int, oneFS bool) { +// by cobra. Errors are returned rather than exiting, so that the +// deferred close — which checkpoints the SQLite WAL — always runs. +func runScan(roots []string, workers int, oneFS bool) error { if workers < 1 { workers = 1 } - roots = resolveRoots(roots) + roots, err := resolveRoots(roots) + if err != nil { + return err + } + dbPath := databasePath() db, err := openScanDatabase(dbPath) if err != nil { - fatalf("%v", err) + return err } defer func() { _ = db.Close() }() st, err := syncScan(db, roots, workers, oneFS) if err != nil { - fatalf("update database %s: %v", dbPath, err) + return fmt.Errorf("update database %s: %w", dbPath, err) } fmt.Fprintf(os.Stderr, @@ -75,31 +80,33 @@ func runScan(roots []string, workers int, oneFS bool) { "%d unchanged), %d skipped\n", st.added+st.updated+st.unchanged, st.added, st.updated, st.removed, st.unchanged, st.skipped) + + return nil } // resolveRoots converts each PATH operand to an absolute, lexically // cleaned path (symlinks are not resolved) and verifies that it // exists. Database records are keyed by absolute path, so scan results // must not depend on the working directory. -func resolveRoots(roots []string) []string { +func resolveRoots(roots []string) ([]string, error) { abs := make([]string, 0, len(roots)) for _, root := range roots { a, err := filepath.Abs(root) if err != nil { - fatalf("resolve %s: %v", root, err) + return nil, fmt.Errorf("resolve %s: %w", root, err) } // A nonexistent operand is a fatal error before any scanning. _, err = os.Lstat(a) if err != nil { - fatalf("%v", err) + return nil, err } abs = append(abs, a) } - return abs + return abs, nil } // pruneRoots drops operands already covered by another operand: diff --git a/trees.go b/trees.go index f408831..7dba881 100644 --- a/trees.go +++ b/trees.go @@ -34,8 +34,11 @@ type treeNode struct { // maximal duplicate-tree groups as TSV on stdout. It never touches the // scanned filesystem; its only I/O is the database, stdout, and // stderr. -func runTrees() { - recs := loadRecords() +func runTrees() error { + recs, err := loadRecords() + if err != nil { + return err + } super, allDirs := buildHierarchy(recs) super.compute() @@ -44,9 +47,9 @@ func runTrees() { out := bufio.NewWriterSize(os.Stdout, ioBufSize) - _, err := fmt.Fprintln(out, "first\tdupe\tfiles\tsize") + _, err = fmt.Fprintln(out, "first\tdupe\tfiles\tsize") if err != nil { - fatalf("write stdout: %v", err) + return fmt.Errorf("write stdout: %w", err) } dupeTrees := 0 @@ -59,7 +62,7 @@ func runTrees() { _, err = fmt.Fprintf(out, "%s\t%s\t%d\t%d\n", first.path, n.path, first.fileCount, first.totalSize) if err != nil { - fatalf("write stdout: %v", err) + return fmt.Errorf("write stdout: %w", err) } dupeTrees++ @@ -69,13 +72,15 @@ func runTrees() { err = out.Flush() if err != nil { - fatalf("write stdout: %v", err) + return fmt.Errorf("write stdout: %w", err) } fmt.Fprintf(os.Stderr, "trees: %d records read, %d duplicate tree groups, %d dupe trees, "+ "%s reclaimable\n", len(recs), len(dupes), dupeTrees, humanBytes(reclaimable)) + + return nil } // buildHierarchy reconstructs the directory hierarchy from the record