Guarantee the database is closed on every fatal exit path #4
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sneak/sfdupes#4
Reference in New Issue
Block a user
fatalf(main.go:105-108) callsos.Exit(1), which does not run deferred functions. Everydefer db.Close()in the codebase is therefore dead on the fatal path:scan.go:66registersdefer func() { _ = db.Close() }(), andscan.go:70callsfatalf.report.go:40registers the close, andreport.go:44callsfatalf.Consequence: on a fatal error the SQLite connection is never closed, so no WAL checkpoint runs and the
-wal/-shmsidecar 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,runReportandrunTreesreturn errors instead of callingfatalffrom inside a function that owns an open database; the single exit point lives inmain(or in a thin wrapper it calls).db.Close()is guaranteed to run on every path — success, per-file-error, and fatal.-walfile left behind.make checkgreen.Implementation plan (branch
db-close-on-fatal, frommainatce6d29d):1. Single exit point in
main.gomainbecomesos.Exit(run(os.Args[1:], os.Stderr)).run(args []string, stderr io.Writer) intbuilds the command tree(
newRootCommand(stderr)), executes it, and maps the outcome to anexit code. It is the only place that knows about exit codes, and it
is callable from tests in-process.
fatalfand itsos.Exit(1)are deleted.2. Not turning fatal errors into usage errors
Cobra prints the error and the usage text for anything
RunEreturns, and today
main.gomaps everyExecute()error to exit 2.So the error class has to be carried explicitly:
fatalErrorwrapper type marks "the command ran and failed" —runtime failure, exit 1;
runEadapter wraps each subcommand implementation: it setscmd.SilenceUsage/cmd.SilenceErrorsbefore calling it (so aruntime failure never dumps usage text) and wraps a non-nil error in
fatalError;runreports afatalErrorassfdupes: <err>on stderr andreturns 1; anything else coming out of
Executeis a cobraargument/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.
sfdupes(no subcommand) case keeps printing usage andexiting 2: its
RunEsilences only the error message and returns apackage-level sentinel, so cobra prints the usage text and
runfalls through to the exit-2 branch.
3. Subcommands return errors
runScan(...) error,runReport() error,runTrees() error,loadRecords() ([]scanRec, error),resolveRoots(...) ([]string, error); everyfatalfcall site becomes a wrappedreturn, withthe same message text as today (
update database %s: %vetc.).defer func() { _ = db.Close() }()inrunScanandloadRecordsthen actually runs on the fatal path; nothing between the open and
the return calls
os.Exitany more.4. Tests (new
main_test.go)TestOpenDatabaseCreatesWAL: an open database has a-walsidecarand a
Closeremoves it. This gives the assertions below teeth —without it, "no
-walafterwards" could pass vacuously.scan,reportandtrees:a database file that opens cleanly and passes the schema-version
check but has no
filestable, so the first query fails with thedatabase open. Asserts exit code 1, no
-wal/-shmleft beside thedatabase,
sfdupes:on stderr, and no usage text.PATHoperand is fatal (exit 1, no usage text), perREADME §Error handling.
scanwithno operand,
report/treeswith a positional argument, unknownsubcommand.
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.mdgets its Completed Steps entry in the same commit;make checkplusmake docker(the digest-pinned v2.12.2 lint stage) beforethe PR.