sneak/sfdupes
1
0
Fork 0
Code Issues 23 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Guarantee the database is closed on every fatal exit path (closes #4) #29

Merged
clawbot merged 1 commits from db-close-on-fatal into main 2026-08-09 04:39:28 +02:00
Conversation 4 Commits 1 Files Changed 6 +566 -57
clawbot commented 2026-08-09 04:27:39 +02:00
Collaborator
Copy Link

fatalf called os.Exit(1), which does not run deferred functions, so
every defer db.Close() was dead on the fatal path: the SQLite WAL was
left uncheckpointed and the -wal/-shm sidecars were left for the
next process to recover. It also made those paths impossible to
exercise in-process.

What changed

  • fatalf is gone. runScan, runReport, runTrees, loadRecords
    and resolveRoots return their errors, so the deferred
    db.Close() — and with it the WAL checkpoint — always runs. Message
    text is unchanged; the fmt.Errorf wrappers use %w.
  • The single exit point is run(args []string, stderr io.Writer) int
    in main.go, called as os.Exit(run(os.Args[1:], os.Stderr)).
  • Error classification, which is the trap here: cobra prints the error
    and the usage text for anything RunE returns, and main
    previously mapped every root.Execute() error to exit 2. A runtime
    failure is not a usage problem, so a runE adapter sets
    SilenceUsage/SilenceErrors on the subcommand and wraps its error
    in fatalError. run reports a fatalError as sfdupes: <err> on
    stderr and exits 1; everything else out of Execute is cobra's own
    argument/flag/unknown-command error, already printed by cobra with
    its usage text, and exits 2. Bare sfdupes still prints usage and
    exits 2.
  • No behaviour change: exit 0 on success even with per-file warnings,
    1 fatal, 2 usage, exactly as README §Error handling specifies, and
    stdout still carries data only (README design goal 4).

Definition of done

  1. runScan/runReport/runTrees return errors; the exit point is
    run in main.go — done.
  2. db.Close() runs on success, per-file-warning and fatal paths —
    done; nothing between the open and the return calls os.Exit any
    more.
  3. Exit codes unchanged — covered by tests and by a manual run of the
    built binary (below).
  4. A test triggers a fatal error after the database is open and
    asserts no -wal is left — TestRunFatalAfterOpenClosesDatabase,
    one subtest each for scan, report and trees.
  5. make check green — plus make docker.

Verification

  • make check: green (tests 0.3s, whole suite well under the 20s
    budget and the 30s timeout; coverage 64% to 86.9%). Local
    golangci-lint is v2.10.1, so this is the weaker gate (#24).
  • make docker: green — that is the authoritative one, with the
    digest-pinned v2.12.2 lint stage and make check run as non-root.
    It caught six goconst findings v2.10.1 did not; the subcommand
    names are now the constants cmdScan/cmdReport/cmdTrees.
  • Manual run of the built binary, checking $? and the stdout/stderr
    split for each case: bare sfdupes 2 (usage on stderr, stdout
    empty), unknown subcommand 2, report x 2, nonexistent PATH
    operand 1 with no usage text, report with no database 1, scan 0,
    report/trees 0 with only TSV on stdout. After the scan the
    database directory holds db.sqlite alone — no -wal, no -shm.

New tests (main_test.go)

  • TestOpenDatabaseKeepsWALWhileOpen establishes the premise of the
    rest: an open database does have a -wal sidecar, so its absence
    afterwards is evidence of a close rather than a vacuous pass.
  • TestRunFatalAfterOpenClosesDatabase: a database that opens cleanly
    and passes the schema-version check but has no files table, so the
    first query fails with the database open. Asserts exit 1, no
    -wal/-shm, sfdupes: on stderr, no usage text, nothing on
    stdout, and that the message is the one only a query on the open
    database can produce.
  • TestRunMissingOperandIsFatalNotUsage, TestRunUsageErrors,
    TestRunHelpAndVersionSucceed, TestRunScanSucceedsDespiteWarnings
    (unreadable file skipped, still exit 0),
    TestRunReportSucceeds/TestRunTreesSucceeds (exit 0, exact TSV on
    stdout).

Out of scope

Deliberately untouched, per the tracker: #5 (signals), #6 (pool leak
on error), #8 (read-only database), #16 (CLI surface tests), #24
(local linter version).

`fatalf` called `os.Exit(1)`, which does not run deferred functions, so every `defer db.Close()` was dead on the fatal path: the SQLite WAL was left uncheckpointed and the `-wal`/`-shm` sidecars were left for the next process to recover. It also made those paths impossible to exercise in-process. ## What changed - `fatalf` is gone. `runScan`, `runReport`, `runTrees`, `loadRecords` and `resolveRoots` return their errors, so the deferred `db.Close()` — and with it the WAL checkpoint — always runs. Message text is unchanged; the `fmt.Errorf` wrappers use `%w`. - The single exit point is `run(args []string, stderr io.Writer) int` in `main.go`, called as `os.Exit(run(os.Args[1:], os.Stderr))`. - Error classification, which is the trap here: cobra prints the error *and* the usage text for anything `RunE` returns, and `main` previously mapped every `root.Execute()` error to exit 2. A runtime failure is not a usage problem, so a `runE` adapter sets `SilenceUsage`/`SilenceErrors` on the subcommand and wraps its error in `fatalError`. `run` reports a `fatalError` as `sfdupes: <err>` on stderr and exits 1; everything else out of `Execute` is cobra's own argument/flag/unknown-command error, already printed by cobra with its usage text, and exits 2. Bare `sfdupes` still prints usage and exits 2. - No behaviour change: exit 0 on success even with per-file warnings, 1 fatal, 2 usage, exactly as README §Error handling specifies, and stdout still carries data only (README design goal 4). ## Definition of done 1. `runScan`/`runReport`/`runTrees` return errors; the exit point is `run` in `main.go` — done. 2. `db.Close()` runs on success, per-file-warning and fatal paths — done; nothing between the open and the return calls `os.Exit` any more. 3. Exit codes unchanged — covered by tests and by a manual run of the built binary (below). 4. A test triggers a fatal error after the database is open and asserts no `-wal` is left — `TestRunFatalAfterOpenClosesDatabase`, one subtest each for `scan`, `report` and `trees`. 5. `make check` green — plus `make docker`. ## Verification - `make check`: green (tests 0.3s, whole suite well under the 20s budget and the 30s timeout; coverage 64% to 86.9%). Local `golangci-lint` is v2.10.1, so this is the weaker gate (#24). - `make docker`: green — that is the authoritative one, with the digest-pinned v2.12.2 lint stage and `make check` run as non-root. It caught six `goconst` findings v2.10.1 did not; the subcommand names are now the constants `cmdScan`/`cmdReport`/`cmdTrees`. - Manual run of the built binary, checking `$?` and the stdout/stderr split for each case: bare `sfdupes` 2 (usage on stderr, stdout empty), unknown subcommand 2, `report x` 2, nonexistent `PATH` operand 1 with no usage text, `report` with no database 1, `scan` 0, `report`/`trees` 0 with only TSV on stdout. After the scan the database directory holds `db.sqlite` alone — no `-wal`, no `-shm`. ## New tests (`main_test.go`) - `TestOpenDatabaseKeepsWALWhileOpen` establishes the premise of the rest: an open database *does* have a `-wal` sidecar, so its absence afterwards is evidence of a close rather than a vacuous pass. - `TestRunFatalAfterOpenClosesDatabase`: a database that opens cleanly and passes the schema-version check but has no `files` table, so the first query fails with the database open. Asserts exit 1, no `-wal`/`-shm`, `sfdupes: ` on stderr, no usage text, nothing on stdout, and that the message is the one only a query on the open database can produce. - `TestRunMissingOperandIsFatalNotUsage`, `TestRunUsageErrors`, `TestRunHelpAndVersionSucceed`, `TestRunScanSucceedsDespiteWarnings` (unreadable file skipped, still exit 0), `TestRunReportSucceeds`/`TestRunTreesSucceeds` (exit 0, exact TSV on stdout). ## Out of scope Deliberately untouched, per the tracker: #5 (signals), #6 (pool leak on error), #8 (read-only database), #16 (CLI surface tests), #24 (local linter version).
clawbot added 1 commit 2026-08-09 04:27:39 +02:00
Guarantee the database is closed on every fatal exit path (closes #4)
All checks were successful
check / check (push) Successful in 57s
Details
73841c9989 
fatalf called os.Exit(1), which does not run deferred functions, so
every defer db.Close() was dead on the fatal path: the SQLite WAL was
left uncheckpointed and the -wal/-shm sidecars were left for the next
process to recover. It also made those paths impossible to exercise
in-process.

fatalf is gone. runScan, runReport, runTrees, loadRecords and
resolveRoots return their errors, so the deferred close always runs,
and the only exit point is run() in main.go.

Mapping errors to exit codes needs care: cobra prints the error and
the command's usage text for anything RunE returns, and main mapped
every Execute() error to exit 2. A runtime failure is not a usage
problem, so the runE adapter silences both for the subcommands and
marks their errors fatalError; run() reports a fatalError as
"sfdupes: ..." on stderr and exits 1, and leaves everything else --
cobra's own argument, flag and unknown-command errors, which cobra has
already reported with its usage text -- on exit 2. The bare
"sfdupes" invocation still prints usage and exits 2.

Exit codes and message text are unchanged: 0 on success even with
per-file warnings, 1 fatal, 2 usage, per README section "Error
handling and exit codes". Everything on stdout is still data only.

main_test.go drives the CLI in-process and covers all three: a fatal
error raised after the database is open (a database with no files
table) closes it and leaves no -wal or -shm behind for scan, report
and trees; a nonexistent PATH operand is fatal, not usage, and prints
no usage text; the usage errors still exit 2; and a scan that skipped
an unreadable file still exits 0.
clawbot self-assigned this 2026-08-09 04:27:43 +02:00
clawbot added the needs-review label 2026-08-09 04:27:43 +02:00
clawbot commented 2026-08-09 04:28:02 +02:00
Author
Collaborator
Copy Link

Built and verified as follows.

Gates

  • make check (local, golangci-lint v2.10.1): green. ok sneak.berlin/go/sfdupes 0.3s coverage: 86.9% of statements (was
    64%); the suite runs in well under a second, so it stays far inside
    the 20-second budget and the 30-second timeout.
  • make docker: green. This is the authoritative gate — the
    digest-pinned v2.12.2 lint stage plus make check as the
    unprivileged builder user. It rejected the first version of the
    branch with six goconst findings that v2.10.1 (issue #24) did not
    report, in main.go and main_test.go; fixed by naming the
    subcommands cmdScan/cmdReport/cmdTrees and the Usage: marker
    in the tests, then re-run to completion.

Behavioural check of the built binary (make build, real process,
real $?, stdout and stderr captured separately):

invocation exit stdout stderr
sfdupes 2 empty usage
sfdupes bogus 2 empty Error: unknown command + usage
sfdupes report x 2 empty Error: unknown command "x" ... + usage
sfdupes scan /nonexistent 1 empty sfdupes: lstat ...: no such file or directory, no usage
sfdupes report (no database) 1 empty sfdupes: ...: no database (run "sfdupes scan" first, ...), no usage
sfdupes scan TREE 0 empty progress + summary
sfdupes report 0 TSV only summary
sfdupes trees 0 TSV only summary

After the scan the database directory contained db.sqlite and
nothing else — the WAL was checkpointed and the sidecars removed on
close.

The subtle part, called out for the reviewer: the exit-1 rows
above are exactly the regression risk in this refactor. Cobra prints
the error and the usage text for anything RunE returns, and the
old main.go mapped every root.Execute() error to exit 2, so a
naive RunE conversion turns every runtime failure into an exit-2
usage error with a usage dump. runE in main.go silences both on
the subcommand and tags the error fatalError; run exits 1 for
those and 2 for everything else cobra produces.
TestRunMissingOperandIsFatalNotUsage and the assertFatalOutput
helper (no Usage: on stderr, nothing on stdout) lock that down, and
TestRunUsageErrors locks down the other direction.

One thing noticed and deliberately left alone (not filed, not
fixed): with stdout closed outright (sfdupes report >&-) the run
still exits 0 rather than failing on the write, because fd 1 gets
handed to the next file the process opens. That behaviour is identical
before and after this branch and is unrelated to the database
lifetime, so it is out of scope here; say the word and I will file it.

Built and verified as follows. **Gates** - `make check` (local, `golangci-lint` v2.10.1): green. `ok sneak.berlin/go/sfdupes 0.3s coverage: 86.9% of statements` (was 64%); the suite runs in well under a second, so it stays far inside the 20-second budget and the 30-second timeout. - `make docker`: green. This is the authoritative gate — the digest-pinned v2.12.2 lint stage plus `make check` as the unprivileged `builder` user. It rejected the first version of the branch with six `goconst` findings that v2.10.1 (issue #24) did not report, in `main.go` and `main_test.go`; fixed by naming the subcommands `cmdScan`/`cmdReport`/`cmdTrees` and the `Usage:` marker in the tests, then re-run to completion. **Behavioural check of the built binary** (`make build`, real process, real `$?`, stdout and stderr captured separately): | invocation | exit | stdout | stderr | | --- | --- | --- | --- | | `sfdupes` | 2 | empty | usage | | `sfdupes bogus` | 2 | empty | `Error: unknown command` + usage | | `sfdupes report x` | 2 | empty | `Error: unknown command "x" ...` + usage | | `sfdupes scan /nonexistent` | 1 | empty | `sfdupes: lstat ...: no such file or directory`, no usage | | `sfdupes report` (no database) | 1 | empty | `sfdupes: ...: no database (run "sfdupes scan" first, ...)`, no usage | | `sfdupes scan TREE` | 0 | empty | progress + summary | | `sfdupes report` | 0 | TSV only | summary | | `sfdupes trees` | 0 | TSV only | summary | After the scan the database directory contained `db.sqlite` and nothing else — the WAL was checkpointed and the sidecars removed on close. **The subtle part**, called out for the reviewer: the exit-1 rows above are exactly the regression risk in this refactor. Cobra prints the error *and* the usage text for anything `RunE` returns, and the old `main.go` mapped every `root.Execute()` error to exit 2, so a naive `RunE` conversion turns every runtime failure into an exit-2 usage error with a usage dump. `runE` in `main.go` silences both on the subcommand and tags the error `fatalError`; `run` exits 1 for those and 2 for everything else cobra produces. `TestRunMissingOperandIsFatalNotUsage` and the `assertFatalOutput` helper (no `Usage:` on stderr, nothing on stdout) lock that down, and `TestRunUsageErrors` locks down the other direction. **One thing noticed and deliberately left alone** (not filed, not fixed): with stdout closed outright (`sfdupes report >&-`) the run still exits 0 rather than failing on the write, because fd 1 gets handed to the next file the process opens. That behaviour is identical before and after this branch and is unrelated to the database lifetime, so it is out of scope here; say the word and I will file it.
clawbot commented 2026-08-09 04:29:19 +02:00
Author
Collaborator
Copy Link

Manager note — independent adversarial review dispatched.

The reviewer did not author this change and is working in its own throwaway worktree. It has been pointed at the specific trap in this refactor: cobra's RunE prints usage alongside the error by default, and the old main mapped every root.Execute() error to exit 2, so the easy way to get this wrong is to silently reclassify a fatal (exit 1) as a usage error (exit 2), or to start dumping usage text on ordinary runtime failures. It has been asked to verify every documented exit path by running the built binary and checking real $?, and to try reverting the production change while keeping the new tests to confirm they actually go red rather than passing vacuously.

Worth recording from the implementation, because it is the clearest evidence yet for #24: make check passed locally, then make docker rejected the first version with six goconst findings. The local linter is v2.10.1; the repo pins v2.12.2. A green local make check in this repo is currently not evidence of anything, and until #24 is fixed make docker is the only gate that counts. The reviewer has been told to treat it that way.

Coverage moved 64% to 86.9% on this branch.

One thing found during implementation and deliberately kept out of this PR, now filed as #30: sfdupes report >&- exits 0 rather than failing the write, which README §Error handling lists as an exit-1 condition. It predates this branch and is unrelated to database lifetime, so excluding it was the right call. The wider version of that bug — unchecked writes meaning report | head and a full disk both silently truncate the report — is the part that actually matters.

Manager note — independent adversarial review dispatched. The reviewer did not author this change and is working in its own throwaway worktree. It has been pointed at the specific trap in this refactor: cobra's `RunE` prints usage alongside the error by default, and the old `main` mapped every `root.Execute()` error to exit 2, so the easy way to get this wrong is to silently reclassify a fatal (exit 1) as a usage error (exit 2), or to start dumping usage text on ordinary runtime failures. It has been asked to verify every documented exit path by running the built binary and checking real `$?`, and to try reverting the production change while keeping the new tests to confirm they actually go red rather than passing vacuously. Worth recording from the implementation, because it is the clearest evidence yet for #24: `make check` passed locally, then `make docker` **rejected** the first version with six `goconst` findings. The local linter is v2.10.1; the repo pins v2.12.2. A green local `make check` in this repo is currently not evidence of anything, and until #24 is fixed `make docker` is the only gate that counts. The reviewer has been told to treat it that way. Coverage moved 64% to 86.9% on this branch. One thing found during implementation and deliberately kept out of this PR, now filed as #30: `sfdupes report >&-` exits 0 rather than failing the write, which README §Error handling lists as an exit-1 condition. It predates this branch and is unrelated to database lifetime, so excluding it was the right call. The wider version of that bug — unchecked writes meaning `report | head` and a full disk both silently truncate the report — is the part that actually matters.
clawbot commented 2026-08-09 04:38:42 +02:00
Author
Collaborator
Copy Link

Review of PR #29 (head 73841c9, base main at ce6d29d)

Verdict: PASS — no blocking findings. Six non-blocking notes below.

Reviewed independently against issue #4, README.md (§Error handling
and exit codes, §Design goal 4, §scan mode, §report mode, §trees
mode), REPO_POLICIES.md and TODO.md. All verification was done in
a throwaway worktree at the PR head; nothing in the shared checkout
was touched.

Definition of done (issue #4)

  1. runScan/runReport/runTrees return errors; single exit point
    is run in main.go — met. grep -rn "os.Exit" *.go finds
    exactly one call site, main.go:55.
  2. db.Close() guaranteed on success, per-file-warning and fatal
    paths — met, verified empirically (below).
  3. Exit codes unchanged — met, verified empirically against a binary
    built from ce6d29d.
  4. A test triggers a fatal error after the database is open and
    asserts no -wal is left — met, and the test is not vacuous
    (mutation-tested, below).
  5. make check green — met, and make docker green too.

Empirical exit-code and stream check

Built binaries from ce6d29d (base) and 73841c9 (head) and ran 15
invocations through each, capturing real $? with stdout and stderr
separated:

invocation exit stdout usage text on stderr
bare sfdupes 2 empty yes
bogus 2 empty yes
scan (no operand) 2 empty yes
report x 2 empty yes
trees x 2 empty yes
scan --nope /tmp 2 empty yes
completion 2 empty yes
scan /nonexistent-xyz 1 empty no
report (no database) 1 empty no
trees (no database) 1 empty no
report with stdout on /dev/full 1 n/a no
trees with stdout on /dev/full 1 n/a no
--help / -h / help / scan --help 0 empty yes (stderr)
--version 0 empty no
scan TREE / report / trees (success) 0 empty / TSV / TSV no

Base and head agree on every exit code, and the stderr bytes hash
identically for every case except two: --version (different
git describe string, expected) and bare sfdupes (see note 2).
No fatal became a usage error, no usage became a fatal, nothing is
swallowed to 0. Usage-text leakage is correct in both directions.
Stream separation holds: stdout is empty for every failing, usage,
help and scan invocation, and carries only TSV for report/trees.

The database actually closes

Built a database with PRAGMA journal_mode=wal; PRAGMA user_version=1
and no files table, then ran each subcommand against it:

  • base ce6d29d: db.sqlite, db.sqlite-wal, db.sqlite-shm left
    behind for scan, report and trees.
  • head 73841c9: db.sqlite alone for all three.

Error text is byte-identical between base and head on all three paths
(sfdupes: database ...: read records: SQL logic error: no such table: files (1), and the update database ... variant for scan). After a
successful scan the database directory holds db.sqlite alone.

The new tests are not vacuous

Mutation-tested: in a scratch copy of the head tree I reintroduced the
defect the PR fixes — dropped the deferred db.Close() in runScan
and loadRecords and closed only on the success return, so the close
is skipped exactly on the fatal path — and kept main_test.go
unchanged. make test goes red:

--- FAIL: TestRunFatalAfterOpenClosesDatabase/scan
    main_test.go:172: ...db.sqlite-wal still present: the database was not closed
    main_test.go:172: ...db.sqlite-shm still present: the database was not closed
--- FAIL: TestRunFatalAfterOpenClosesDatabase/report   (same two)
--- FAIL: TestRunFatalAfterOpenClosesDatabase/trees    (same two)

All three subtests fail on the sidecar assertion, which is the
property under test. TestOpenDatabaseKeepsWALWhileOpen does its job
as the premise: it asserts a -wal exists beside an open database,
so "no -wal afterwards" is evidence of a close and not of a database
that never had a WAL. The fatal-path subtests also assert the message
is no such table: files, which only a query against the already-open
database can produce, so they cannot pass by failing before the open.

Gates

  • make check: green. ok sneak.berlin/go/sfdupes 0.176s coverage: 86.9% of statements, 0 issues, fmt-check clean.
  • make docker: green, exit 0. Lint stage (digest-pinned v2.12.2)
    0 issues after make fmt-check and make lint; builder stage
    make check 0 issues at 86.9% coverage, run as the unprivileged
    builder user; image exported.
  • CI on 73841c9: check / check (push) success.
  • Mergeable: the PR base ce6d29d is current origin/main; the
    branch is a single commit on top of it, git merge-tree reports
    zero conflicts. Fast-forward.

The goconst constants are legitimate, not churn

Verified rather than taken on trust. Restored the string literals
(removed the cmdScan/cmdReport/cmdTrees block, put "scan [--workers N] [-x] PATH...", "report", "trees" back in the Use:
fields, and the literals back in main_test.go) in a scratch copy and
re-ran make docker: it fails with exit 2 and 5 goconst findings at
v2.12.2 —

main.go:127:10: string `report` has 5 occurrences, make it a constant (goconst)
main.go:136:10: string `trees` has 5 occurrences, make it a constant (goconst)
main_test.go:149:3: string `scan` has 7 occurrences, make it a constant (goconst)
main_test.go:150:3: string `report` has 5 occurrences, make it a constant (goconst)
main_test.go:151:3: string `trees` has 5 occurrences, make it a constant (goconst)

Two of the five are in production code, so this is not purely a
test-driven accommodation. The constants are the minimal fix and
preferable to nolint directives.

Scope

Diff is six files: main.go, main_test.go, report.go, scan.go,
trees.go, TODO.md. Nothing from #5 (signals), #6 (pool leak on
error), #8 (read-only database) or #16 (CLI surface tests) is present.
No unrelated refactoring. Dockerfile is untouched: the deliberate
non-root builder user and the comment explaining why (root would
bypass the chmod(0) the permission tests rely on) are intact, and
the permission-dependent assertions in scan_test.go:806-814
(st.skipped != 2 fails under root) still run in the docker gate.

Hygiene

  • No mention of any AI assistant or vendor anywhere in the diff, the
    commit message, or the PR body; no attribution trailers. Author and
    committer are sneak <sneak@sneak.berlin>.
  • Commit title ends with (closes #4); body wraps at 70 columns.
  • TODO.md has an accurate Completed Steps entry at the top, in the
    same commit, in the established style.
  • No non-inclusive terminology introduced.
  • make fmt-check clean.

Non-blocking findings

  1. main_test.go:185 — misleading test name.
    TestRunMissingOperandIsFatalNotUsage tests a nonexistent PATH
    operand, which README §scan mode calls a fatal error (exit 1). But
    "missing operand" is README's phrase for scan with no operand,
    which is a usage error (exit 2) and is covered separately as the
    "scan without paths" case in TestRunUsageErrors. The name
    asserts the opposite of the spec for the case it appears to name.
    Acceptable: TestRunNonexistentPathIsFatalNotUsage.

  2. Bare sfdupes gains one trailing newline on stderr. The old
    code called cmd.Usage(); cobra now prints the usage for the
    returned errNoSubcommand via c.Println(cmd.UsageString()),
    which appends a newline. diff of base vs head stderr for the bare
    invocation is exactly one added blank line at EOF. Exit code, text
    and stream are unchanged and README specifies nothing here, so this
    is cosmetic — but the implementation-plan comment on #4 claims
    "Cobra's own usage output is therefore byte-for-byte what it is
    today", and that claim is not quite true.

  3. run's injected stderr is only half-honoured. run(args []string, stderr io.Writer) routes cobra's error and usage output
    to the writer, but runScan, runReport and runTrees still
    write their warnings, progress and summaries to the package-global
    os.Stderr, and the reports write to os.Stdout. The tests
    therefore have to monkey-patch os.Stdout (captureStdout), and
    scanFixture/TestRunScanSucceedsDespiteWarnings cannot assert
    that the warning actually happened — it only asserts exit 0. Under
    a root test runner the chmod(0) is a no-op and that test degrades
    silently into an ordinary clean scan while still passing. The
    repo-wide root guard survives via scan_test.go:806-814, so this
    is not a hole in the suite, but the new test does not carry its own
    weight; asserting 1 skipped in the summary would fix it. Probably
    best folded into #16, which owns threading the writers through.

  4. runE is opt-in, and misuse fails silently in the wrong
    direction.     A future subcommand wired with a bare RunE (or
    Run) would not be wrapped in fatalError, so run's default
    branch would classify its runtime failure as exit 2 and cobra would
    dump the usage text — the exact regression this PR exists to
    prevent. Worth a note on newRootCommand, or a test that walks
    root.Commands() and asserts every leaf command's RunE is a
    runE product.

  5. errNoSubcommand's message is dead text. The root RunE sets
    cmd.SilenceErrors = true before returning it, so "no subcommand" is never printed anywhere. Harmless, but the sentinel
    exists only to be non-nil; the doc comment could say so outright
    (it half does).

  6. Pre-existing, not caused by this PR: the docker lint stage
    emits The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2. Worth a tracker
    item alongside #24 rather than a change here.

## Review of PR #29 (head `73841c9`, base `main` at `ce6d29d`) **Verdict: PASS** — no blocking findings. Six non-blocking notes below. Reviewed independently against issue #4, `README.md` (§Error handling and exit codes, §Design goal 4, §scan mode, §report mode, §trees mode), `REPO_POLICIES.md` and `TODO.md`. All verification was done in a throwaway worktree at the PR head; nothing in the shared checkout was touched. ### Definition of done (issue #4) 1. `runScan`/`runReport`/`runTrees` return errors; single exit point is `run` in `main.go` — met. `grep -rn "os.Exit" *.go` finds exactly one call site, `main.go:55`. 2. `db.Close()` guaranteed on success, per-file-warning and fatal paths — met, verified empirically (below). 3. Exit codes unchanged — met, verified empirically against a binary built from `ce6d29d`. 4. A test triggers a fatal error after the database is open and asserts no `-wal` is left — met, and the test is not vacuous (mutation-tested, below). 5. `make check` green — met, and `make docker` green too. ### Empirical exit-code and stream check Built binaries from `ce6d29d` (base) and `73841c9` (head) and ran 15 invocations through each, capturing real `$?` with stdout and stderr separated: | invocation | exit | stdout | usage text on stderr | | --- | --- | --- | --- | | bare `sfdupes` | 2 | empty | yes | | `bogus` | 2 | empty | yes | | `scan` (no operand) | 2 | empty | yes | | `report x` | 2 | empty | yes | | `trees x` | 2 | empty | yes | | `scan --nope /tmp` | 2 | empty | yes | | `completion` | 2 | empty | yes | | `scan /nonexistent-xyz` | 1 | empty | **no** | | `report` (no database) | 1 | empty | **no** | | `trees` (no database) | 1 | empty | **no** | | `report` with stdout on `/dev/full` | 1 | n/a | **no** | | `trees` with stdout on `/dev/full` | 1 | n/a | **no** | | `--help` / `-h` / `help` / `scan --help` | 0 | empty | yes (stderr) | | `--version` | 0 | empty | no | | `scan TREE` / `report` / `trees` (success) | 0 | empty / TSV / TSV | no | Base and head agree on every exit code, and the stderr bytes hash identically for every case except two: `--version` (different `git describe` string, expected) and bare `sfdupes` (see note 2). No fatal became a usage error, no usage became a fatal, nothing is swallowed to 0. Usage-text leakage is correct in both directions. Stream separation holds: stdout is empty for every failing, usage, help and `scan` invocation, and carries only TSV for `report`/`trees`. ### The database actually closes Built a database with `PRAGMA journal_mode=wal; PRAGMA user_version=1` and no `files` table, then ran each subcommand against it: - base `ce6d29d`: `db.sqlite`, `db.sqlite-wal`, `db.sqlite-shm` left behind for `scan`, `report` and `trees`. - head `73841c9`: `db.sqlite` alone for all three. Error text is byte-identical between base and head on all three paths (`sfdupes: database ...: read records: SQL logic error: no such table: files (1)`, and the `update database ...` variant for `scan`). After a successful scan the database directory holds `db.sqlite` alone. ### The new tests are not vacuous Mutation-tested: in a scratch copy of the head tree I reintroduced the defect the PR fixes — dropped the deferred `db.Close()` in `runScan` and `loadRecords` and closed only on the success return, so the close is skipped exactly on the fatal path — and kept `main_test.go` unchanged. `make test` goes red: --- FAIL: TestRunFatalAfterOpenClosesDatabase/scan main_test.go:172: ...db.sqlite-wal still present: the database was not closed main_test.go:172: ...db.sqlite-shm still present: the database was not closed --- FAIL: TestRunFatalAfterOpenClosesDatabase/report (same two) --- FAIL: TestRunFatalAfterOpenClosesDatabase/trees (same two) All three subtests fail on the sidecar assertion, which is the property under test. `TestOpenDatabaseKeepsWALWhileOpen` does its job as the premise: it asserts a `-wal` exists beside an *open* database, so "no `-wal` afterwards" is evidence of a close and not of a database that never had a WAL. The fatal-path subtests also assert the message is `no such table: files`, which only a query against the already-open database can produce, so they cannot pass by failing before the open. ### Gates - `make check`: green. `ok sneak.berlin/go/sfdupes 0.176s coverage: 86.9% of statements`, `0 issues`, `fmt-check` clean. - `make docker`: green, exit 0. Lint stage (digest-pinned v2.12.2) `0 issues` after `make fmt-check` and `make lint`; builder stage `make check` `0 issues` at 86.9% coverage, run as the unprivileged `builder` user; image exported. - CI on `73841c9`: `check / check (push)` success. - Mergeable: the PR base `ce6d29d` is current `origin/main`; the branch is a single commit on top of it, `git merge-tree` reports zero conflicts. Fast-forward. ### The `goconst` constants are legitimate, not churn Verified rather than taken on trust. Restored the string literals (removed the `cmdScan`/`cmdReport`/`cmdTrees` block, put `"scan [--workers N] [-x] PATH..."`, `"report"`, `"trees"` back in the `Use:` fields, and the literals back in `main_test.go`) in a scratch copy and re-ran `make docker`: it fails with exit 2 and 5 `goconst` findings at v2.12.2 — main.go:127:10: string `report` has 5 occurrences, make it a constant (goconst) main.go:136:10: string `trees` has 5 occurrences, make it a constant (goconst) main_test.go:149:3: string `scan` has 7 occurrences, make it a constant (goconst) main_test.go:150:3: string `report` has 5 occurrences, make it a constant (goconst) main_test.go:151:3: string `trees` has 5 occurrences, make it a constant (goconst) Two of the five are in production code, so this is not purely a test-driven accommodation. The constants are the minimal fix and preferable to `nolint` directives. ### Scope Diff is six files: `main.go`, `main_test.go`, `report.go`, `scan.go`, `trees.go`, `TODO.md`. Nothing from #5 (signals), #6 (pool leak on error), #8 (read-only database) or #16 (CLI surface tests) is present. No unrelated refactoring. `Dockerfile` is untouched: the deliberate non-root `builder` user and the comment explaining why (root would bypass the `chmod(0)` the permission tests rely on) are intact, and the permission-dependent assertions in `scan_test.go:806-814` (`st.skipped != 2` fails under root) still run in the docker gate. ### Hygiene - No mention of any AI assistant or vendor anywhere in the diff, the commit message, or the PR body; no attribution trailers. Author and committer are `sneak <sneak@sneak.berlin>`. - Commit title ends with ` (closes #4)`; body wraps at 70 columns. - `TODO.md` has an accurate Completed Steps entry at the top, in the same commit, in the established style. - No non-inclusive terminology introduced. - `make fmt-check` clean. --- ## Non-blocking findings 1. **`main_test.go:185` — misleading test name.** `TestRunMissingOperandIsFatalNotUsage` tests a *nonexistent* `PATH` operand, which README §scan mode calls a fatal error (exit 1). But "missing operand" is README's phrase for `scan` with *no* operand, which is a usage error (exit 2) and is covered separately as the `"scan without paths"` case in `TestRunUsageErrors`. The name asserts the opposite of the spec for the case it appears to name. Acceptable: `TestRunNonexistentPathIsFatalNotUsage`. 2. **Bare `sfdupes` gains one trailing newline on stderr.** The old code called `cmd.Usage()`; cobra now prints the usage for the returned `errNoSubcommand` via `c.Println(cmd.UsageString())`, which appends a newline. `diff` of base vs head stderr for the bare invocation is exactly one added blank line at EOF. Exit code, text and stream are unchanged and README specifies nothing here, so this is cosmetic — but the implementation-plan comment on #4 claims "Cobra's own usage output is therefore byte-for-byte what it is today", and that claim is not quite true. 3. **`run`'s injected `stderr` is only half-honoured.** `run(args []string, stderr io.Writer)` routes cobra's error and usage output to the writer, but `runScan`, `runReport` and `runTrees` still write their warnings, progress and summaries to the package-global `os.Stderr`, and the reports write to `os.Stdout`. The tests therefore have to monkey-patch `os.Stdout` (`captureStdout`), and `scanFixture`/`TestRunScanSucceedsDespiteWarnings` cannot assert that the warning actually happened — it only asserts exit 0. Under a root test runner the `chmod(0)` is a no-op and that test degrades silently into an ordinary clean scan while still passing. The repo-wide root guard survives via `scan_test.go:806-814`, so this is not a hole in the suite, but the new test does not carry its own weight; asserting `1 skipped` in the summary would fix it. Probably best folded into #16, which owns threading the writers through. 4. **`runE` is opt-in, and misuse fails silently in the wrong direction.** A future subcommand wired with a bare `RunE` (or `Run`) would not be wrapped in `fatalError`, so `run`'s `default` branch would classify its runtime failure as exit 2 and cobra would dump the usage text — the exact regression this PR exists to prevent. Worth a note on `newRootCommand`, or a test that walks `root.Commands()` and asserts every leaf command's `RunE` is a `runE` product. 5. **`errNoSubcommand`'s message is dead text.** The root `RunE` sets `cmd.SilenceErrors = true` before returning it, so `"no subcommand"` is never printed anywhere. Harmless, but the sentinel exists only to be non-nil; the doc comment could say so outright (it half does). 6. **Pre-existing, not caused by this PR:** the docker lint stage emits `The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2.` Worth a tracker item alongside #24 rather than a change here.
clawbot added merge-ready and removed needs-review labels 2026-08-09 04:39:19 +02:00
clawbot merged commit 2a055c0104 into main 2026-08-09 04:39:28 +02:00
clawbot deleted branch db-close-on-fatal 2026-08-09 04:39:29 +02:00
clawbot commented 2026-08-09 04:40:06 +02:00
Author
Collaborator
Copy Link

Manager note — merged as 2a055c0.

Review verdict PASS, no blocking findings, so this landed via a non-fast-forward merge commit. Branch db-close-on-fatal deleted; origin carries only main. Not assigned to sneak — main is unprotected on this repo, so the manager merges once the gate passes.

Post-merge verification on main:

  • make check green — tests ok in 0.406s, coverage 86.9% (up from 64%), 0 issues. from the linter.
  • Working tree clean, one worktree, no leftover refs.
  • #4 closed automatically by the merge title.

This was a strong review. Rather than reading the diff and reasoning about it, it built binaries from both ce6d29d and 73841c9, ran 15 invocations through each with separated streams, and hashed the stderr output to compare — establishing that exit codes and messages are identical except for two explainable differences. It then mutation-tested the new assertions: it reintroduced the original defect in a scratch copy with main_test.go untouched, and confirmed all three TestRunFatalAfterOpenClosesDatabase subtests go red. That is the difference between "the tests pass" and "the tests would catch a regression", and it is the check most reviews skip.

It also independently confirmed the goconst constants were forced rather than churn — restoring the string literals and re-running make docker fails with five findings at v2.12.2, two of them in main.go itself.

Non-blocking findings and dispositions:

  • Test named TestRunMissingOperandIsFatalNotUsage actually covers a nonexistent operand, while "missing operand" is README's phrase for scan with no operand at all. Misleading name; folded into #16.
  • run threads its writers only into cobra — subcommands still write to the package-global os.Stdout/os.Stderr, so tests must monkey-patch. Folded into #16, where it properly belongs.
  • runE is opt-in, so a future subcommand wired with a bare RunE would reintroduce exactly the misclassification this PR fixes. Folded into #16 as a guard test.
  • Bare sfdupes gains one trailing newline on stderr; cosmetic, but it makes the plan comment's "byte-for-byte identical" claim slightly overstated. No action.
  • errNoSubcommand's message is never printed because the root sets SilenceErrors. Harmless dead string. No action.
  • gomodguard deprecated in favour of gomodguard_v2 — already tracked as #26 and assigned to sneak, since the fix belongs in the canonical config.

Next unit is #6, the hash-phase worker-pool leak. It is the natural follow-on: now that hashPhase errors propagate to a returning runScan instead of dying via os.Exit, the abandoned feeder goroutine and blocked workers are a real leak rather than a theoretical one.

Manager note — merged as `2a055c0`. Review verdict PASS, no blocking findings, so this landed via a non-fast-forward merge commit. Branch `db-close-on-fatal` deleted; `origin` carries only `main`. Not assigned to `sneak` — `main` is unprotected on this repo, so the manager merges once the gate passes. Post-merge verification on `main`: - `make check` green — tests `ok` in 0.406s, coverage 86.9% (up from 64%), `0 issues.` from the linter. - Working tree clean, one worktree, no leftover refs. - #4 closed automatically by the merge title. This was a strong review. Rather than reading the diff and reasoning about it, it built binaries from both `ce6d29d` and `73841c9`, ran 15 invocations through each with separated streams, and hashed the stderr output to compare — establishing that exit codes and messages are identical except for two explainable differences. It then mutation-tested the new assertions: it reintroduced the original defect in a scratch copy with `main_test.go` untouched, and confirmed all three `TestRunFatalAfterOpenClosesDatabase` subtests go red. That is the difference between "the tests pass" and "the tests would catch a regression", and it is the check most reviews skip. It also independently confirmed the `goconst` constants were forced rather than churn — restoring the string literals and re-running `make docker` fails with five findings at v2.12.2, two of them in `main.go` itself. Non-blocking findings and dispositions: - Test named `TestRunMissingOperandIsFatalNotUsage` actually covers a *nonexistent* operand, while "missing operand" is README's phrase for `scan` with no operand at all. Misleading name; folded into #16. - `run` threads its writers only into cobra — subcommands still write to the package-global `os.Stdout`/`os.Stderr`, so tests must monkey-patch. Folded into #16, where it properly belongs. - `runE` is opt-in, so a future subcommand wired with a bare `RunE` would reintroduce exactly the misclassification this PR fixes. Folded into #16 as a guard test. - Bare `sfdupes` gains one trailing newline on stderr; cosmetic, but it makes the plan comment's "byte-for-byte identical" claim slightly overstated. No action. - `errNoSubcommand`'s message is never printed because the root sets `SilenceErrors`. Harmless dead string. No action. - `gomodguard` deprecated in favour of `gomodguard_v2` — already tracked as #26 and assigned to `sneak`, since the fix belongs in the canonical config. Next unit is #6, the hash-phase worker-pool leak. It is the natural follow-on: now that `hashPhase` errors propagate to a returning `runScan` instead of dying via `os.Exit`, the abandoned feeder goroutine and blocked workers are a real leak rather than a theoretical one.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
Review passed; ready to merge
 needs-checks
CI checks failing or not yet green
 needs-rebase
Branch conflicts with main; rebase required
 needs-review
PR awaiting independent adversarial review
 needs-rework
Review found blocking problems; author must rework
No Label merge-ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/sfdupes#29
Delete Branch "db-close-on-fatal"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?