fix(installer): abort on signal instead of resuming and exiting 0 - #64
Conversation
install.sh registered one handler for six events:
trap cleanup EXIT HUP INT QUIT TERM
A trap on a non-EXIT signal runs its handler and then RESUMES the
script. It does not terminate. So Ctrl-C during a long step ran cleanup,
fell through to the next step, printed "Ready!" and exited 0.
set -e does not rescue it. Every long step here sits in a condition, and
a command in a condition context is exempt from set -e by definition, so
the interrupted step was merely observed and logged as a soft failure.
Verified in bash and in busybox ash, before and after:
old under bash -> exit=0 reached "Ready!"
old under sh -> exit=0 reached "Ready!"
new under bash -> exit=130 did not
new under sh -> exit=130 did not
The worst case is not Ctrl-C. It is SIGTERM in the documented unattended
mode: CI kills the run on timeout, the installer continues through the
remaining steps and exits 0, and the wrapper records a successful
provision of a machine that was never provisioned.
Second-order: cleanup also clears the temp-path list, so a signal
arriving just after make_tmpdir removed the temp directory and forgot it
while the script carried on. That degrades safely — the next write into
the deleted path fails and the step is skipped with a warning — but it
is another way the run looked fine and was not.
Fix is two lines. cleanup is idempotent, so the EXIT trap firing after
the signal handler is a no-op.
POSIX only, no bashisms, so the curl-pipe path is unaffected. Verified:
sh -n passes, shellcheck at warning severity in sh dialect is clean, and
stamp --check is still in sync.
scripts/install-hooks.sh has the same pattern and is deliberately left
alone here — it is a separate pinned artifact whose digest is generated,
so it belongs in its own change.
Found by the reference-corpus audit, nvm defensive-shell lens.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 3 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
get-resq-software | 4c730d1 | Aug 16 2026, 09:49 AM |
|
In The To reproduce, run: sh install.sh --This will output: I suggest adding a case for --- a/install.sh
+++ b/install.sh
@@ -936,6 +936,7 @@
for _arg in "$@"; do
case "$_arg" in
-h|--help) usage; exit 0 ;;
+ --) break ;;
-V|--version) printf
"$SCRIPT_VERSION"; exit 0 ;;
*) fail "Unknown option: $_arg (try --help)" ;;
esac
|
Highest-severity finding from the reference-corpus audit (nvm defensive-shell lens), verified before and after in two shells.
The bug
trap cleanup EXIT HUP INT QUIT TERMOne handler for six events. A trap on a non-EXIT signal runs its handler and then resumes — it does not terminate. So Ctrl-C during a long step ran
cleanup, fell through to the next step, printedReady!and exited 0.set -edoesn't rescue it either: every long step sits in a condition —if ! sh "$_nix_installer" install,if nix develop … --command true,if ! gh release download— and a command in a condition context is exempt fromset -eby definition. The interrupted step was simply logged as a soft failure and the installer carried on.Verified, both directions
(
shhere is busybox ash — a genuine POSIX shell, not bash wearing a different name.)Why it matters more than Ctrl-C
The worst case is SIGTERM in the documented unattended mode (
REPO=<name> YES=1). CI kills the run on timeout, the installer continues through its remaining steps, exits 0, and the wrapper records a successful provision of a machine that was never provisioned.Second-order:
cleanupalso clears_TMP_PATHS, so a signal arriving just aftermake_tmpdirremoved the temp directory and forgot it while the script continued. That one degrades safely — the next write into the deleted path fails and the step is skipped with a warning — but it's another way the run looked fine and wasn't.The fix
Two lines.
cleanupis idempotent (it clears_TMP_PATHSin the current shell — only the read loop is a subshell), so theEXITtrap firing afterwards is a no-op.POSIX only, no bashisms, so the curl-pipe path is unaffected. Verified:
sh -npasses,shellcheck -S warning -s sh -xis clean,stamp --checkstill in sync.Scope
scripts/install-hooks.sh:131has the identical pattern and is deliberately not touched here — it's a separate pinned artifact with a generated digest, so it belongs in its own change.Note this reaches users only after a release;
mainand the servedv0.4.3will differ until then.Related
The audit produced 20 confirmed findings against 10 refuted. This is the one worth landing on its own. #63 carries the CI-gate half — including that
-S errorprovably passes a file containing a bash array and[[ ]], so a bashism could have entered this very script with a green board.