fix: continue walk on per-file errors instead of log.Fatal#56
Conversation
Record errors from unreadable directories, failed stats/opens/reads, writes, and renames; exit non-zero from main when any occurred. Pass goroutine-safe child paths into worker goroutines. Partial fix for dolph#6 (root NewFile/abs failures still fatal). Co-authored-by: Cursor <cursoragent@cursor.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fef7960386
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !fr.hadErrors.Load() { | ||
| t.Fatal("expected hadErrors after unreadable directory") |
There was a problem hiding this comment.
This test hard-codes the expectation that chmod 000 makes private unreadable, but that is false when tests run as uid 0 (a common CI/container setup), where os.ReadDir can still succeed and hadErrors stays false. In those environments the test fails even though traversal logic is fine, so it introduces a deterministic false negative. Gate this assertion behind a non-root check (or otherwise detect whether the directory is actually unreadable before asserting).
Useful? React with 👍 / 👎.
| if err := os.Rename(tempName, f.Path); err != nil { | ||
| log.Fatalf("Unable to atomically move temp file %v to %v: %v", tempName, f.Path, err) | ||
| f.logError("Unable to atomically move temp file %v to %v: %v", tempName, f.Path, err) | ||
| } |
There was a problem hiding this comment.
After this change, write failures are non-fatal and execution continues, but Write leaves tempName behind when os.Rename(tempName, f.Path) fails. In real error cases (e.g., transient rename failures), this now accumulates random temp files across many files instead of aborting once, polluting the target tree and affecting later runs. Add cleanup of tempName on the rename-error path.
Useful? React with 👍 / 👎.
1 participant
Summary
noteError/hadErrorsand exit non-zero frommainwhen any file-level error occurred.NewFilepath resolution failures remain fatal (startup-level).Test plan
go test ./...TestWalkDirContinuesOnUnreadableDirectoryPartial fix for #6
Made with Cursor