From 7f863386f773371807d4d3640aac07027f6f7598 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sat, 3 Oct 2026 13:37:06 +0000 Subject: [PATCH] Test report and trees stdout write failures (closes #30) report and trees already checked every stdout write and the final flush. run now takes the stdout it hands to them, so tests pass a closed file or a failing writer instead of swapping os.Stdout: a closed stdout exits 1 with a one-line diagnostic, and the writer's error reaches the caller. README "Error handling" now states the two cases that never reach sfdupes as a failed write: a pipe reader that exits early ends the process with SIGPIPE, as with cat; and stdout closed with >&- is replaced by /dev/null by the Go runtime before main runs, so the run succeeds. Model: opus-5-5 --- README.md | 12 ++++ TODO.md | 4 ++ main.go | 19 +++--- main_test.go | 167 +++++++++++++++++++++++++------------------------ report.go | 5 +- report_test.go | 8 +-- scan_test.go | 3 +- trees.go | 5 +- trees_test.go | 8 +-- 9 files changed, 128 insertions(+), 103 deletions(-) diff --git a/README.md b/README.md index d9c042a..e2a1871 100644 --- a/README.md +++ b/README.md @@ -561,6 +561,18 @@ Additional requirements: - `2`: usage error (including `scan` with no `PATH` operand and `report`/`trees` with any positional argument). +A stdout write failure, such as a full disk, is reported in one line on +stderr and exits 1. Two cases never reach sfdupes as a failed write: + +- When the reader of a stdout pipe exits early, as in + `sfdupes report | head`, the next write ends sfdupes with `SIGPIPE`, + quietly and without a summary, the way `cat` or `sort` end. The + shell reports the signal (status 141 in most shells), not exit 1. +- When stdout is closed outright (`sfdupes report >&-`), the Go + runtime opens `/dev/null` in its place before sfdupes starts, so + the output is discarded and the run succeeds, as with + `> /dev/null`. + ## Entrypoints This repository adheres to the diff --git a/TODO.md b/TODO.md index 7724ba8..fb16773 100644 --- a/TODO.md +++ b/TODO.md @@ -29,6 +29,10 @@ # Completed Steps +- 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) + - 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/main.go b/main.go index d2160c1..3a8ce57 100644 --- a/main.go +++ b/main.go @@ -57,22 +57,27 @@ var errNoSubcommand = errors.New("no subcommand") var Version = "dev" func main() { - os.Exit(run(os.Args[1:], os.Stderr)) + // Once the reader of a stdout pipe has gone, as in "sfdupes report | + // head", the Go runtime ends the process with SIGPIPE on the next + // write instead of returning an error (README "Error handling"). + // Registering for SIGPIPE with os/signal would change that. + os.Exit(run(os.Args[1:], os.Stdout, 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 { +// runs before the process ends. The report and trees subcommands write +// their data to stdout. +func run(args []string, stdout, 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 := newRootCommand(stdout, stderr) root.SetArgs(args) err := root.Execute() @@ -98,7 +103,7 @@ func run(args []string, stderr io.Writer) int { // 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 { +func newRootCommand(stdout, stderr io.Writer) *cobra.Command { root := &cobra.Command{ Use: "sfdupes", Short: "Find candidate duplicate files by size and head/tail/content SHA-256", @@ -140,7 +145,7 @@ func newRootCommand(stderr io.Writer) *cobra.Command { Short: "Read the scan database and print the file-level duplicates report", Args: cobra.NoArgs, RunE: runE(func(ctx context.Context, _ []string) error { - return runReport(ctx) + return runReport(ctx, stdout) }), } @@ -149,7 +154,7 @@ func newRootCommand(stderr io.Writer) *cobra.Command { Short: "Read the scan database and print the duplicate-tree report", Args: cobra.NoArgs, RunE: runE(func(ctx context.Context, _ []string) error { - return runTrees(ctx) + return runTrees(ctx, stdout) }), } diff --git a/main_test.go b/main_test.go index b307a82..4ef14b1 100644 --- a/main_test.go +++ b/main_test.go @@ -44,50 +44,6 @@ func assertNoSidecars(t *testing.T, path string) { } } -// 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 @@ -160,17 +116,15 @@ func TestRunFatalAfterOpenClosesDatabase(t *testing.T) { args = append(args, t.TempDir()) } - var stderr bytes.Buffer + var stdout, stderr bytes.Buffer - stdout := captureStdout(t) - - code := run(args, &stderr) + code := run(args, &stdout, &stderr) if code != exitFatal { t.Errorf("run(%v) = %d, want %d", args, code, exitFatal) } assertNoSidecars(t, path) - assertFatalOutput(t, stderr.String(), stdout()) + assertFatalOutput(t, stderr.String(), stdout.String()) // Proof that the failure happened after the open: only a // query against the opened database can report this. @@ -188,18 +142,16 @@ func TestRunMissingOperandIsFatalNotUsage(t *testing.T) { // must not dump the usage text. t.Setenv(databaseEnv, testDBPath(t)) - var stderr bytes.Buffer - - stdout := captureStdout(t) + var stdout, stderr bytes.Buffer missing := filepath.Join(t.TempDir(), "nope") - code := run([]string{cmdScan, missing}, &stderr) + code := run([]string{cmdScan, missing}, &stdout, &stderr) if code != exitFatal { t.Errorf("run(scan %s) = %d, want %d", missing, code, exitFatal) } - assertFatalOutput(t, stderr.String(), stdout()) + assertFatalOutput(t, stderr.String(), stdout.String()) } // assertFatalOutput checks that a fatal error was reported the way @@ -243,11 +195,9 @@ func TestRunUsageErrors(t *testing.T) { // path that does not exist. t.Setenv(databaseEnv, testDBPath(t)) - var stderr bytes.Buffer + var stdout, stderr bytes.Buffer - stdout := captureStdout(t) - - code := run(tc.args, &stderr) + code := run(tc.args, &stdout, &stderr) if code != exitUsage { t.Errorf("run(%v) = %d, want %d", tc.args, code, exitUsage) } @@ -256,7 +206,7 @@ func TestRunUsageErrors(t *testing.T) { t.Errorf("stderr = %q, want %q", stderr.String(), tc.want) } - if got := stdout(); got != "" { + if got := stdout.String(); got != "" { t.Errorf("stdout = %q, want nothing (data only)", got) } }) @@ -265,9 +215,9 @@ func TestRunUsageErrors(t *testing.T) { // 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) { + t.Parallel() + assertHumanOutput(t, "--help") assertHumanOutput(t, "--version") } @@ -278,11 +228,9 @@ func TestRunHelpAndVersionSucceed(t *testing.T) { func assertHumanOutput(t *testing.T, arg string) { t.Helper() - var stderr bytes.Buffer + var stdout, stderr bytes.Buffer - stdout := captureStdout(t) - - code := run([]string{arg}, &stderr) + code := run([]string{arg}, &stdout, &stderr) if code != exitOK { t.Errorf("run(%s) = %d, want %d", arg, code, exitOK) } @@ -291,7 +239,7 @@ func assertHumanOutput(t *testing.T, arg string) { t.Errorf("run(%s) wrote nothing to stderr", arg) } - if got := stdout(); got != "" { + if got := stdout.String(); got != "" { t.Errorf("stdout = %q, want nothing (data only)", got) } } @@ -320,17 +268,15 @@ func scanFixture(t *testing.T) []string { t.Fatal(err) } - var stderr bytes.Buffer + var stdout, stderr bytes.Buffer - stdout := captureStdout(t) - - code := run([]string{cmdScan, dir}, &stderr) + code := run([]string{cmdScan, dir}, &stdout, &stderr) if code != exitOK { t.Fatalf("run(scan) = %d, want %d; stderr: %s", code, exitOK, stderr.String()) } - if got := stdout(); got != "" { + if got := stdout.String(); got != "" { t.Errorf("scan stdout = %q, want nothing (data only)", got) } @@ -351,18 +297,16 @@ func TestRunReportSucceeds(t *testing.T) { dupes := scanFixture(t) - var stderr bytes.Buffer + var stdout, stderr bytes.Buffer - stdout := captureStdout(t) - - code := run([]string{cmdReport}, &stderr) + code := run([]string{cmdReport}, &stdout, &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 { + if got := stdout.String(); got != want { t.Errorf("stdout = %q, want %q", got, want) } @@ -375,11 +319,9 @@ func TestRunTreesSucceeds(t *testing.T) { dupes := scanFixture(t) - var stderr bytes.Buffer + var stdout, stderr bytes.Buffer - stdout := captureStdout(t) - - code := run([]string{cmdTrees}, &stderr) + code := run([]string{cmdTrees}, &stdout, &stderr) if code != exitOK { t.Fatalf("run(trees) = %d, want %d; stderr: %s", code, exitOK, stderr.String()) @@ -389,9 +331,72 @@ func TestRunTreesSucceeds(t *testing.T) { // 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 { + if got := stdout.String(); got != want { t.Errorf("stdout = %q, want %q", got, want) } assertNoSidecars(t, path) } + +func TestRunStdoutClosedIsFatal(t *testing.T) { + // README §Error handling: a stdout write failure exits 1, reported + // in one line on stderr. + for _, name := range []string{cmdReport, cmdTrees} { + t.Run(name, func(t *testing.T) { + t.Setenv(databaseEnv, testDBPath(t)) + + scanFixture(t) + + stdout, err := os.Create(filepath.Join(t.TempDir(), "stdout")) + if err != nil { + t.Fatal(err) + } + + err = stdout.Close() + if err != nil { + t.Fatal(err) + } + + var stderr bytes.Buffer + + code := run([]string{name}, stdout, &stderr) + if code != exitFatal { + t.Errorf("run(%s) = %d, want %d", name, code, exitFatal) + } + + got := stderr.String() + if !strings.HasPrefix(got, "sfdupes: write stdout: ") || + !strings.Contains(got, os.ErrClosed.Error()) || + strings.Count(got, "\n") != 1 { + t.Errorf("stderr = %q, want one line reporting the "+ + "failed stdout write", got) + } + }) + } +} + +// errWriteFailed is the error failingWriter returns. +var errWriteFailed = errors.New("write failed") + +// failingWriter is a stdout that fails every write. +type failingWriter struct{} + +func (failingWriter) Write([]byte) (int, error) { return 0, errWriteFailed } + +func TestStdoutWriteErrorPropagates(t *testing.T) { + t.Setenv(databaseEnv, testDBPath(t)) + + scanFixture(t) + + cases := map[string]func(context.Context, io.Writer) error{ + cmdReport: runReport, + cmdTrees: runTrees, + } + + for name, fn := range cases { + err := fn(t.Context(), failingWriter{}) + if !errors.Is(err, errWriteFailed) { + t.Errorf("%s: error = %v, want %v", name, err, errWriteFailed) + } + } +} diff --git a/report.go b/report.go index f02ce2b..0ec045b 100644 --- a/report.go +++ b/report.go @@ -4,6 +4,7 @@ import ( "bufio" "context" "fmt" + "io" "os" "slices" "strings" @@ -65,7 +66,7 @@ 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(ctx context.Context) error { +func runReport(ctx context.Context, stdout io.Writer) error { recs, err := loadRecords(ctx) if err != nil { return err @@ -73,7 +74,7 @@ func runReport(ctx context.Context) error { dupes := collectDupeGroups(recs) - out := bufio.NewWriterSize(os.Stdout, ioBufSize) + out := bufio.NewWriterSize(stdout, ioBufSize) _, err = fmt.Fprintln(out, "first\tdupe\tsize") if err != nil { diff --git a/report_test.go b/report_test.go index d1aa290..9aa092d 100644 --- a/report_test.go +++ b/report_test.go @@ -50,11 +50,9 @@ func seedDatabase(t *testing.T, recs []scanRec) string { func TestRunReportEscapesPaths(t *testing.T) { t.Setenv(databaseEnv, seedDatabase(t, awkwardPairRecs())) - var stderr bytes.Buffer + var stdout, stderr bytes.Buffer - stdout := captureStdout(t) - - code := run([]string{cmdReport}, &stderr) + code := run([]string{cmdReport}, &stdout, &stderr) if code != exitOK { t.Fatalf("run(report) = %d, want %d; stderr: %s", code, exitOK, stderr.String()) @@ -62,7 +60,7 @@ func TestRunReportEscapesPaths(t *testing.T) { want := "first\tdupe\tsize\n" + `/d/\tone\ntwo\rthree\\four/f` + "\t/d/A/f\t5\n" - if got := stdout(); got != want { + if got := stdout.String(); got != want { t.Errorf("stdout = %q, want %q", got, want) } } diff --git a/scan_test.go b/scan_test.go index f47005d..5e2a259 100644 --- a/scan_test.go +++ b/scan_test.go @@ -7,6 +7,7 @@ import ( "database/sql" "encoding/hex" "fmt" + "io" "os" "path/filepath" "runtime" @@ -1548,7 +1549,7 @@ func TestScanHashWriteFailureUnwindsPool(t *testing.T) { code := run([]string{ cmdScan, "--workers", strconv.Itoa(hashLeakWorkers), dir, - }, &stderr) + }, io.Discard, &stderr) if code != exitFatal { t.Fatalf("run(scan) = %d, want %d; stderr: %s", code, exitFatal, stderr.String()) diff --git a/trees.go b/trees.go index f43374c..735c5b5 100644 --- a/trees.go +++ b/trees.go @@ -5,6 +5,7 @@ import ( "context" "crypto/sha256" "fmt" + "io" "os" "slices" "strconv" @@ -36,7 +37,7 @@ 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(ctx context.Context) error { +func runTrees(ctx context.Context, stdout io.Writer) error { recs, err := loadRecords(ctx) if err != nil { return err @@ -47,7 +48,7 @@ func runTrees(ctx context.Context) error { dupes := collectTreeGroups(allDirs, super) - out := bufio.NewWriterSize(os.Stdout, ioBufSize) + out := bufio.NewWriterSize(stdout, ioBufSize) _, err = fmt.Fprintln(out, "first\tdupe\tfiles\tsize") if err != nil { diff --git a/trees_test.go b/trees_test.go index abf61f1..5cc8817 100644 --- a/trees_test.go +++ b/trees_test.go @@ -108,11 +108,9 @@ func TestBuildHierarchyRootPath(t *testing.T) { func TestRunTreesEscapesPaths(t *testing.T) { t.Setenv(databaseEnv, seedDatabase(t, awkwardPairRecs())) - var stderr bytes.Buffer + var stdout, stderr bytes.Buffer - stdout := captureStdout(t) - - code := run([]string{cmdTrees}, &stderr) + code := run([]string{cmdTrees}, &stdout, &stderr) if code != exitOK { t.Fatalf("run(trees) = %d, want %d; stderr: %s", code, exitOK, stderr.String()) @@ -120,7 +118,7 @@ func TestRunTreesEscapesPaths(t *testing.T) { want := "first\tdupe\tfiles\tsize\n" + `/d/\tone\ntwo\rthree\\four` + "\t/d/A\t1\t5\n" - if got := stdout(); got != want { + if got := stdout.String(); got != want { t.Errorf("stdout = %q, want %q", got, want) } }