fix(main): exit with code 130 on SIGINT (#615)#863
Open
gustcol wants to merge 2 commits intopeak:masterfrom
Open
fix(main): exit with code 130 on SIGINT (#615)#863gustcol wants to merge 2 commits intopeak:masterfrom
gustcol wants to merge 2 commits intopeak:masterfrom
Conversation
Fixes peak#615 When Ctrl-C was pressed, different s5cmd commands returned different exit codes: 'ls' returned 0 (the context-cancel error was swallowed), 'cp' returned 1 (its context-cancel path returned an error). Neither matched the POSIX convention of 130 (128 + SIGINT=2), making it hard for scripts to distinguish Ctrl-C from actual failures. Fix: register a second os.Interrupt notification channel (intCh) alongside the signal.NotifyContext cancel mechanism. After command.Main returns, a non-blocking receive on intCh tells us whether SIGINT was the cause; if so we call os.Exit(130) before evaluating the command error. Reproducer: s5cmd ls s3://bucket/ &; sleep 0.1; kill -INT $!; echo $? # Before: 0 (ls) or 1 (cp) After this change: echo $? # 130 for any command interrupted by SIGINT
The original test could be flaky when run alongside other packages because credential/region lookup sometimes finished before the 500 ms sleep elapsed, causing the binary to exit before SIGINT was sent. Use --no-sign-request + AWS_DEFAULT_REGION=us-east-1 to skip both credential and region-discovery requests so the binary blocks immediately on the black-hole TCP dial. Increase the pre-signal sleep to 3 s to absorb build-time variation under parallel test load.
Author
Test resultsUnit tests ( Changes included beyond the original fix:
Specific tests (run 3 times to confirm stable): Wasabi smoke test: SIGINT exits with code 130. SIGTERM or error exit remains 1. |
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.
Context
Fixes #615
Root cause
Different commands returned different exit codes on Ctrl-C:
lsreturned 0 — the context-cancel path swallowed the errorcpreturned 1 — its context-cancel path propagated an errorThis made it impossible to reliably detect Ctrl-C in scripts vs real failures.
Fix
Register a dedicated
os.Interruptnotification channel (intCh) inmain(), alongside the existingsignal.NotifyContext. Aftercommand.Mainreturns, a non-blocking receive onintChtells uswhether SIGINT was the cause. If yes, we call
os.Exit(130)beforeevaluating the command's own error return.
Files changed:
main.goTests
TestSIGINTExitCodeinmain_test.go: builds the binary, runsa command against an unreachable endpoint so it blocks, sends SIGINT,
and asserts exit code 130.
Manual verification
Notes
exit code 0 or 1 specifically from Ctrl-C interrupts.