Guarantee the database is closed on every fatal exit path #4

New Issue

clawbot · 2026-08-09T03:43:31+02:00

clawbot commented

2026-08-09 03:43:31 +02:00

fatalf (main.go:105-108) calls os.Exit(1), which does not run deferred functions. Every defer db.Close() in the codebase is therefore dead on the fatal path:

scan.go:66 registers defer func() { _ = db.Close() }(), and scan.go:70 calls fatalf.
report.go:40 registers the close, and report.go:44 calls fatalf.

Consequence: on a fatal error the SQLite connection is never closed, so no WAL checkpoint runs and the -wal / -shm sidecar files are left for the next process to recover. It also makes these paths untestable in-process, which blocks the CLI-surface tests (#16) and the interrupt work (#5).

Definition of done

runScan, runReport and runTrees return errors instead of calling fatalf from inside a function that owns an open database; the single exit point lives in main (or in a thin wrapper it calls).
db.Close() is guaranteed to run on every path — success, per-file-error, and fatal.
Exit codes are unchanged and still match README §Error handling: 0 success, 1 fatal, 2 usage.
A test triggers a fatal error after the database is open and asserts the database closes cleanly, with no -wal file left behind.
make check green.

`fatalf` (`main.go:105-108`) calls `os.Exit(1)`, which does not run deferred functions. Every `defer db.Close()` in the codebase is therefore dead on the fatal path: - `scan.go:66` registers `defer func() { _ = db.Close() }()`, and `scan.go:70` calls `fatalf`. - `report.go:40` registers the close, and `report.go:44` calls `fatalf`. Consequence: on a fatal error the SQLite connection is never closed, so no WAL checkpoint runs and the `-wal` / `-shm` sidecar files are left for the next process to recover. It also makes these paths untestable in-process, which blocks the CLI-surface tests (#16) and the interrupt work (#5). ## Definition of done 1. `runScan`, `runReport` and `runTrees` return errors instead of calling `fatalf` from inside a function that owns an open database; the single exit point lives in `main` (or in a thin wrapper it calls). 2. `db.Close()` is guaranteed to run on every path — success, per-file-error, and fatal. 3. Exit codes are unchanged and still match README §Error handling: 0 success, 1 fatal, 2 usage. 4. A test triggers a fatal error after the database is open and asserts the database closes cleanly, with no `-wal` file left behind. 5. `make check` green.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:43:31 +02:00

clawbot referenced this issue

2026-08-09 03:43:40 +02:00

Handle SIGINT/SIGTERM gracefully during scan #5

clawbot referenced this issue

2026-08-09 03:46:13 +02:00

No test covers the CLI surface: exit codes, stream separation, or the TSV writers #16

clawbot referenced this issue

2026-08-09 03:47:54 +02:00

Land golangci-lint v2.12.2 and the canonical .golangci.yml on main #3

clawbot referenced this issue

2026-08-09 04:05:32 +02:00

Point TODO.md at the tracker and record landed work (closes #27) #28

clawbot commented

2026-08-09 04:11:01 +02:00

Implementation plan (branch db-close-on-fatal, from main at ce6d29d):

1. Single exit point in main.go

main becomes os.Exit(run(os.Args[1:], os.Stderr)).
run(args []string, stderr io.Writer) int builds the command tree
(newRootCommand(stderr)), executes it, and maps the outcome to an
exit code. It is the only place that knows about exit codes, and it
is callable from tests in-process.
fatalf and its os.Exit(1) are deleted.

2. Not turning fatal errors into usage errors

Cobra prints the error and the usage text for anything RunE
returns, and today main.go maps every Execute() error to exit 2.
So the error class has to be carried explicitly:

a fatalError wrapper type marks "the command ran and failed" —
runtime failure, exit 1;
a small runE adapter wraps each subcommand implementation: it sets
cmd.SilenceUsage/cmd.SilenceErrors before calling it (so a
runtime failure never dumps usage text) and wraps a non-nil error in
fatalError;
run reports a fatalError as sfdupes: <err> on stderr and
returns 1; anything else coming out of Execute is a cobra
argument/flag/unknown-command error, which cobra has already printed
with its usage text, and returns 2. Cobra's own usage output is
therefore byte-for-byte what it is today.
the bare sfdupes (no subcommand) case keeps printing usage and
exiting 2: its RunE silences only the error message and returns a
package-level sentinel, so cobra prints the usage text and run
falls through to the exit-2 branch.

3. Subcommands return errors

runScan(...) error, runReport() error, runTrees() error,
loadRecords() ([]scanRec, error), resolveRoots(...) ([]string, error); every fatalf call site becomes a wrapped return, with
the same message text as today (update database %s: %v etc.).
defer func() { _ = db.Close() }() in runScan and loadRecords
then actually runs on the fatal path; nothing between the open and
the return calls os.Exit any more.

4. Tests (new main_test.go)

TestOpenDatabaseCreatesWAL: an open database has a -wal sidecar
and a Close removes it. This gives the assertions below teeth —
without it, "no -wal afterwards" could pass vacuously.
fatal-after-open, one subtest each for scan, report and trees:
a database file 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 code 1, no -wal/-shm left beside the
database, sfdupes: on stderr, and no usage text.
a nonexistent PATH operand is fatal (exit 1, no usage text), per
README §Error handling.
usage errors are exit 2 with usage text: no subcommand, scan with
no operand, report/trees with a positional argument, unknown
subcommand.
success is exit 0, including a scan whose per-file warnings skipped
a file (unreadable file, as the existing hard-link test does), and
stdout carries only the TSV data (README design goal 4).

5. Out of scope (tracked separately, deliberately untouched):
#5 signals, #6 pool leak on error, #8 read-only database, #16 CLI
surface tests. Exit-code semantics and all message text stay exactly
as README §Error handling specifies.

TODO.md gets its Completed Steps entry in the same commit; make check plus make docker (the digest-pinned v2.12.2 lint stage) before
the PR.

Implementation plan (branch `db-close-on-fatal`, from `main` at `ce6d29d`): **1. Single exit point in `main.go`** - `main` becomes `os.Exit(run(os.Args[1:], os.Stderr))`. - `run(args []string, stderr io.Writer) int` builds the command tree (`newRootCommand(stderr)`), executes it, and maps the outcome to an exit code. It is the only place that knows about exit codes, and it is callable from tests in-process. - `fatalf` and its `os.Exit(1)` are deleted. **2. Not turning fatal errors into usage errors** Cobra prints the error *and* the usage text for anything `RunE` returns, and today `main.go` maps every `Execute()` error to exit 2. So the error class has to be carried explicitly: - a `fatalError` wrapper type marks "the command ran and failed" — runtime failure, exit 1; - a small `runE` adapter wraps each subcommand implementation: it sets `cmd.SilenceUsage`/`cmd.SilenceErrors` before calling it (so a runtime failure never dumps usage text) and wraps a non-nil error in `fatalError`; - `run` reports a `fatalError` as `sfdupes: <err>` on stderr and returns 1; anything else coming out of `Execute` is a cobra argument/flag/unknown-command error, which cobra has already printed with its usage text, and returns 2. Cobra's own usage output is therefore byte-for-byte what it is today. - the bare `sfdupes` (no subcommand) case keeps printing usage and exiting 2: its `RunE` silences only the error message and returns a package-level sentinel, so cobra prints the usage text and `run` falls through to the exit-2 branch. **3. Subcommands return errors** - `runScan(...) error`, `runReport() error`, `runTrees() error`, `loadRecords() ([]scanRec, error)`, `resolveRoots(...) ([]string, error)`; every `fatalf` call site becomes a wrapped `return`, with the same message text as today (`update database %s: %v` etc.). - `defer func() { _ = db.Close() }()` in `runScan` and `loadRecords` then actually runs on the fatal path; nothing between the open and the return calls `os.Exit` any more. **4. Tests (new `main_test.go`)** - `TestOpenDatabaseCreatesWAL`: an open database has a `-wal` sidecar and a `Close` removes it. This gives the assertions below teeth — without it, "no `-wal` afterwards" could pass vacuously. - fatal-after-open, one subtest each for `scan`, `report` and `trees`: a database file 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 code 1, no `-wal`/`-shm` left beside the database, `sfdupes: ` on stderr, and no usage text. - a nonexistent `PATH` operand is fatal (exit 1, no usage text), per README §Error handling. - usage errors are exit 2 with usage text: no subcommand, `scan` with no operand, `report`/`trees` with a positional argument, unknown subcommand. - success is exit 0, including a scan whose per-file warnings skipped a file (unreadable file, as the existing hard-link test does), and stdout carries only the TSV data (README design goal 4). **5. Out of scope** (tracked separately, deliberately untouched): #5 signals, #6 pool leak on error, #8 read-only database, #16 CLI surface tests. Exit-code semantics and all message text stay exactly as README §Error handling specifies. `TODO.md` gets its Completed Steps entry in the same commit; `make check` plus `make docker` (the digest-pinned v2.12.2 lint stage) before the PR.

clawbot referenced this issue from a commit

2026-08-09 04:27:13 +02:00

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

clawbot referenced a pull request that will close this issue

2026-08-09 04:27:39 +02:00

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

clawbot referenced this issue

2026-08-09 04:29:07 +02:00

report exits 0 when stdout is closed, instead of failing the write #30

clawbot referenced this issue

2026-08-09 04:38:42 +02:00

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