Repository navigation
fix: replace os.Exit and panic in cp and sync with error handling - #882
Open
terryrankine wants to merge 1 commit into
Open
terryrankine wants to merge 1 commit into
terryrankine wants to merge 1 commit into
Conversation
When a worker hit "too many open files", cp and sync printed the fd limit hint and called os.Exit(1). That skipped every deferred cleanup: the progress bar was never finished, the log writer was never flushed, and the --stat summary was never printed. cp also panicked on an "unexpected src-dst pair", which the command validator already rejects, so a normal error is enough. sync additionally created its cancel func without ever calling it, and its planRun goroutines ranged over the object channels with no way to stop on cancellation. cp: on "too many open files", print the hint, record the error, cancel the context so in-flight and queued objects stop, and let Run return through the normal path. Cancellation errors from the remaining tasks are already filtered out by printError, and the process still exits non-zero because the error is returned. The unexpected src-dst pair is now reported like any other per-object error. sync: add the missing defer cancel(). Drop the waiter goroutine that held the other os.Exit; no task was ever submitted to that waiter, so the branch was unreachable (the generated cp commands handle the fd limit themselves through Copy.Run). planRun goroutines now select on ctx.Done() so they return promptly on cancel, and Run closes its side of the command pipe on return so a goroutine blocked writing a command is unblocked and can exit too. Extsort error channels are drained in the listing goroutines instead of in separate goroutines; extsort closes them together with the output channel, so this cannot block. extsort sends a nil error when the context is cancelled mid-merge, which previously made printError dereference nil, so nil is skipped. shouldStopSync now also stops on RequestError and SerializationError, since continuing to list after a transport failure only produces a misleading partial sync. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 of 7 tasks
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
1 participant
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.
Context
Related to #698.
cpandsynccalledos.Exit(1)from inside a goroutine on "too many open files", andcppanicked on an "unexpected src-dst pair".Root cause
os.Exitskips every deferred cleanup: the progress bar is never finished, the log writer never flushed, the--statsummary never printed. Thesynccopy of that handler was on a waiter that nothing ever submitted to, so it was unreachable.syncalso created a cancel func it never called, andplanRun's goroutines ranged over channels with no way to stop on cancellation. The extsort error channels were drained in fire-and-forget goroutines, and extsort v1.0.2 sends a nil error on context cancel, whichprintErrorwould dereference.Fix
cp: on "too many open files", print the hint, record the error, cancel the context so queued work stops, and return through the normal path (still exits non-zero). The unexpected-pairpanicbecomes a per-object error.sync:defer cancel(); remove the dead waiter block;planRungoroutines select onctx.Done();Runcloses its side of the command pipe on return so a goroutine blocked on write can exit; extsort errors drained inline with a nil guard;shouldStopSyncalso stops onRequestError/SerializationError(continuing after a transport failure only yields a misleading partial sync).Tests
TestShouldStopSync(8 cases),TestPlanRunReturnsOnCancelledContext(delete on/off; asserts the writer is closed),TestPlanRunGeneratesCommands.Linux, Go 1.24,
go test -count=1 -race ./...: all packagesok.gofmt,go vet,staticcheck,unparam,semgrep: clean.🤖 Generated with Claude Code