Bubble errors up from worker goroutines; exit non-zero on failure#66
Merged
Conversation
Replace every log.Fatal* in find_replace.go and file_handling.go with
returned errors so deferred cleanup can run, then collect errors across
walker goroutines and surface them at main.
Architecture:
- File methods (NewFile, Info, Read, Write, plus a new error-returning
Mode) now return (T, error). The walker calls Info on each child so
Write can rely on cached Mode without re-statting.
- findReplace.{WalkDir,HandleFile,RenameFile,ReplaceContents} return
errors (WalkDir records them on an embedded accumulator so its
goroutines can stay fire-and-forget).
- errAccumulator is a sync.Mutex-guarded []error with an errors.Join
exit point. Each error is also log.Print'd at the point of failure so
the existing operator-visible stderr UX is preserved; the join exists
so main can scrape stderr and so errors.Is unwraps the whole chain.
- main is now a thin os.Exit wrapper around a testable run(args, stderr)
that returns 1 on bad arg count or any traversal error and 0 on a
clean walk.
- File.Write now defer-Removes its temp file immediately after creating
it; on a successful rename the file no longer exists at tempName so
the deferred remove is a no-op, but on a rename failure the temp file
is cleaned up instead of being leaked.
New tests (each was confirmed to fail for the right reason before the
matching production change was finalized):
- TestWalkDir_PermissionDeniedSubdirContinues: a chmod-0 subdirectory
no longer aborts the walk; the sibling subtree is rewritten and the
walker records an fs.ErrPermission referencing the failing path.
Skips under root (where chmod 0 is bypassed) and Windows.
- TestRenameFile_ReturnsErrorOnExistingDestination: an occupied
destination is refused with an error, not log.Fatal.
- TestWalkDir_BadRenameTargetDoesNotAbortSiblings: a sibling whose
post-rename name is already occupied does not abort the rest of the
tree; the walker logs and records the failure and continues.
- TestWriteCleansUpTempFileOnRenameFailure: forcing the rename step to
fail (target is a non-empty directory; deterministic across users)
leaves no stray temp file behind.
- TestRun_{ExitsZeroOnSuccess,ExitsNonZeroOnTraversalError,
BadArgCountPrintsUsage}: run() returns 0 on a clean walk, non-zero
when any error was recorded, and non-zero with a usage line on bad
arg count.
Closes #6
Closes #11
Closes #5
https://claude.ai/code/session_01Tep5t8h97Q9KKbpLMbUEJr
The error-aggregation primitive introduced in this PR uses errors.Join, which was added in Go 1.20. CI was pinned to 1.19 via go.mod and 'go vet' failed with 'Join not declared by package errors'. This is the minimum bump needed for the chosen primitive. The broader 'be on a supported Go release' work (Go 1.19 has been EOL since August 2023) stays as #28.
Since Go 1.20 (the new toolchain floor in this PR), the math/rand global generator is automatically seeded by the runtime; explicit rand.Seed is a deprecated no-op and staticcheck (run by golangci-lint) flags it as SA1019. The line was already vestigial -- left in place by the original commit to keep the diff narrow. The toolchain bump in the previous commit makes that compromise no longer viable. strings.go's RandomString still uses math/rand.Intn; the change to crypto/rand for filesystem paths remains tracked under issue #3.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
log.Fatal*outside ofmaininfind_replace.goandfile_handling.gowith returned errors so deferred cleanup runs on failure.errAccumulator(sync.Mutex+[]error, exposed viaerrors.Join) embedded onfindReplace. Each error is alsolog.Print'd at the point of failure so the existing operator-visible stderr stream (Renaming/Rewritinglines and per-error context) is preserved; the join exists somaincan surface a non-zero exit code, and soerrors.Is/errors.Asunwrap the entire accumulated chain. This matches the "log and continue" semantic the issue calls for and intentionally avoidserrgroup(which stops on the first error). The struct stays small so the upcoming bounded-worker-pool change (Unbounded goroutine fan-out exhausts file descriptors and memory on large trees #7) can drop in without rearchitecting again.mainis now a thinos.Exitwrapper aroundrun(args, stderr) int, making argument parsing and the success/failure exit code testable from inside the package.File.Writedefer os.Remove(tempName)s immediately after creating its temp file. On success the file no longer exists attempName, so the remove is a no-op; on rename failure the temp file is cleaned up instead of being leaked.Architectural shape (for #7 / #8 follow-ups)
NewFilelog.Fatalfon Abs error(*File, error)File.Infolog.Fatalfon stat error(os.FileInfo, error)File.Readlog.Fatalfon open/seek/read(string, error)File.Writelog.Fatalf, no temp cleanuperror, deferred tempos.RemoveWalkDirlog.Fatalfon ReadDirerrAccumulator, continues siblingsHandleFile/RenameFile/ReplaceContentslog.Fatalferror(walker logs + records)mainlog.Fatalfor usage;WalkDir(NewFile("."))run(args, stderr) int; non-zero on any recorded errorTest plan
New tests (each verified to fail for the right reason against the previous behavior before the matching fix was landed):
TestWalkDir_PermissionDeniedSubdirContinues— achmod 0subdirectory no longer aborts the walk. The sibling subtree is still rewritten; the walker records anfs.ErrPermissionreferencing the failing path. Skips under root (chmod is a no-op there) and on Windows.TestRenameFile_ReturnsErrorOnExistingDestination— an occupied destination yields an error rather than alog.Fatal. Source and destination both still exist afterwards.TestWalkDir_BadRenameTargetDoesNotAbortSiblings— a sibling whose post-rename name is already occupied does not abort the rest of the tree; the walker logs and records the failure and continues with the free file.TestWriteCleansUpTempFileOnRenameFailure— forcing the rename to fail (target is a non-empty directory; deterministic under both root and non-root on Linux) leaves no stray temp file behind. (Verified the test fails without the deferredos.Remove.)TestRun_ExitsZeroOnSuccess,TestRun_ExitsNonZeroOnTraversalError,TestRun_BadArgCountPrintsUsage—run()returns 0 on a clean walk, non-zero on any recorded error, and prints the usage line to its stderr writer on bad arg count.Dev loop:
gofmt -l .— no outputgo vet ./...— no outputgo build ./...— no outputgo test -race ./...—PASS(also verified by running the full suite undernobodyso the root-guarded test exercises the real permission path)./build.sh— green, coverage 80.5%Issues closed
Closes #6
Closes #11
Closes #5
The
--strictflag mentioned in #6's body is intentionally not part of this PR (it's an additive CLI surface change requiringrelease:minor+ a README update); the per-error-log-and-continue policy described in the issue is the default. File-able as a separate enhancement if desired.https://claude.ai/code/session_01Tep5t8h97Q9KKbpLMbUEJr
Generated by Claude Code