fix(install): clean up temp files and child processes on exit#1026
Open
stevenobiajulu wants to merge 4 commits intoNVIDIA:mainfrom
Open
fix(install): clean up temp files and child processes on exit#1026stevenobiajulu wants to merge 4 commits intoNVIDIA:mainfrom
stevenobiajulu wants to merge 4 commits intoNVIDIA:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdded signal-aware cleanup to the installer: track background PIDs and temp files, register traps (EXIT, INT, TERM, HUP) to run a cleanup routine that kills tracked PIDs and removes tracked files, integrate tracking into spinner and nodejs installer, and add a test ensuring sourcing doesn't leave traps. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Installer as install.sh
participant Spinner
participant Worker as BackgroundCmd
participant Cleanup
User->>Installer: run installer
Installer->>Spinner: invoke spin("long command")
Spinner->>Worker: spawn command (background) & create mktemp log
Spinner->>Installer: register PID and file in cleanup lists
User->>Installer: send SIGINT (Ctrl+C)
Installer->>Cleanup: trap triggers cleanup()
Cleanup->>Worker: kill tracked PIDs
Cleanup->>Installer: remove tracked files
Cleanup->>Installer: re-emit signal
Installer->>User: script exits
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
install.sh (2)
397-420: Consider untracking temp file after explicit removal for consistency.The temp file is tracked at line 399 but not untracked after explicit
rm -fat lines 402, 415, and 420. This is harmless (rm -rf on non-existent files is a no-op) but inconsistent withspin()which untracks after removal.♻️ Optional: untrack after explicit removal
rm -f "$nvm_tmp" + remove_tracked_file "$nvm_tmp" error "Failed to download nvm installer"Apply similar changes at lines 415-416 and after line 420.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 397 - 420, The temp file tracker _cleanup_files currently gets the nvm_tmp entry but is not removed when the script explicitly deletes the temp file; update the explicit removal sites that call rm -f "$nvm_tmp" (the failure branch after curl, the integrity-fail branch, and the normal cleanup after spin) to also remove nvm_tmp from _cleanup_files (e.g., by filtering or using the same untrack helper used by spin) so the temporary path is unregistered consistently whenever rm -f "$nvm_tmp" is performed; ensure you modify the codepaths around the curl error handler, the integrity check failure (where actual_hash != NVM_SHA256), and the final cleanup after spin to untrack _cleanup_files alongside deleting the file.
123-134: Signal handling is correct; consider clearing arrays after cleanup to avoid redundant work.The re-raise pattern is correct for propagating proper exit status to parent processes. However, when a signal is caught,
cleanup()runs intrap_signal, and then the EXIT trap also invokescleanup()again after the signal is re-raised. This is safe (idempotent) but wasteful.♻️ Optional: clear arrays after cleanup to avoid double iteration
cleanup() { local rc=$? set +e if (( ${`#_cleanup_pids`[@]} )); then for pid in "${_cleanup_pids[@]}"; do kill "$pid" 2>/dev/null || true done fi if (( ${`#_cleanup_files`[@]} )); then for f in "${_cleanup_files[@]}"; do rm -rf "$f" done fi + _cleanup_pids=() + _cleanup_files=() return "$rc" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 123 - 134, The cleanup function is being run twice when a signal is handled (once in trap_signal and again via the EXIT trap after re-raising), so to avoid redundant array iteration modify trap_signal to clear any tracked arrays (or set a "cleaned" flag) immediately after calling cleanup; specifically, update trap_signal (and/or cleanup) to reset the arrays you iterate (e.g., unset or set to empty) or set a guard variable like CLEANUP_DONE=true and check it at the top of cleanup to skip repeated work—use the function names trap_signal and cleanup and ensure the EXIT trap behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 91-105: The function remove_tracked_file iterates over
_cleanup_files without guarding for an empty/unset array which triggers a Bash
3.2 + set -u failure; update remove_tracked_file to check the length of
_cleanup_files before the for-loop (e.g., if (( ${`#_cleanup_files`[@]} )); then
for value in "${_cleanup_files[@]}"; do ...; done; fi) so the loop is skipped
when the array is empty or unset, leaving the rest of the function (building
kept and reassigning _cleanup_files) unchanged.
- Around line 75-89: The remove_tracked_pid function iterates over
"${_cleanup_pids[@]}" without checking if the array is set, which under set -u
on Bash 3.2 causes an unbound variable error; modify remove_tracked_pid to first
test the array length (e.g., if (( ${`#_cleanup_pids`[@]} )); then ... fi) before
running the for loop that references _cleanup_pids, keeping the existing logic
that rebuilds _cleanup_pids (kept, _cleanup_pids=("${kept[@]}") or
_cleanup_pids=()) and refer to the function name remove_tracked_pid and the
variable _cleanup_pids when making the change.
---
Nitpick comments:
In `@install.sh`:
- Around line 397-420: The temp file tracker _cleanup_files currently gets the
nvm_tmp entry but is not removed when the script explicitly deletes the temp
file; update the explicit removal sites that call rm -f "$nvm_tmp" (the failure
branch after curl, the integrity-fail branch, and the normal cleanup after spin)
to also remove nvm_tmp from _cleanup_files (e.g., by filtering or using the same
untrack helper used by spin) so the temporary path is unregistered consistently
whenever rm -f "$nvm_tmp" is performed; ensure you modify the codepaths around
the curl error handler, the integrity check failure (where actual_hash !=
NVM_SHA256), and the final cleanup after spin to untrack _cleanup_files
alongside deleting the file.
- Around line 123-134: The cleanup function is being run twice when a signal is
handled (once in trap_signal and again via the EXIT trap after re-raising), so
to avoid redundant array iteration modify trap_signal to clear any tracked
arrays (or set a "cleaned" flag) immediately after calling cleanup;
specifically, update trap_signal (and/or cleanup) to reset the arrays you
iterate (e.g., unset or set to empty) or set a guard variable like
CLEANUP_DONE=true and check it at the top of cleanup to skip repeated work—use
the function names trap_signal and cleanup and ensure the EXIT trap behavior
remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install.sh (1)
405-426:⚠️ Potential issue | 🟡 Minor
nvm_tmpis never removed from the_cleanup_filestracking array.Line 405 adds
nvm_tmpto_cleanup_files, but the manual deletions on lines 408, 421, and 426 never callremove_tracked_file. The EXIT trap'scleanup()function will keep a stale pathname and can laterrm -rfa reused path. Thespin()function (lines 250–285) demonstrates the correct pattern: after manualrm -f, it immediately callsremove_tracked_fileto untrack the entry.Add
remove_tracked_file "$nvm_tmp"after each of the threerm -f "$nvm_tmp"calls.Proposed fix
curl -fsSL "https://raw.githubusercontent.com/nvm-sh/nvm/${NVM_VERSION}/install.sh" -o "$nvm_tmp" \ || { rm -f "$nvm_tmp" + remove_tracked_file "$nvm_tmp" error "Failed to download nvm installer" } @@ if [[ "$actual_hash" != "$NVM_SHA256" ]]; then rm -f "$nvm_tmp" + remove_tracked_file "$nvm_tmp" error "nvm installer integrity check failed\n Expected: $NVM_SHA256\n Actual: $actual_hash" fi @@ spin "Installing nvm..." bash "$nvm_tmp" rm -f "$nvm_tmp" + remove_tracked_file "$nvm_tmp"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 405 - 426, The code adds nvm_tmp to the _cleanup_files tracking array but never untracks it when the file is manually removed, leaving cleanup() to later try to remove a reused path; after every manual rm -f "$nvm_tmp" in this block (the curl failure branch, the integrity-failure branch, and after spin finishes) call remove_tracked_file "$nvm_tmp" to remove it from the tracker (follow the pattern used in spin()). Update references around nvm_tmp, spin(), and cleanup() accordingly so the EXIT trap won't operate on a stale path.
♻️ Duplicate comments (1)
install.sh (1)
137-140:⚠️ Potential issue | 🟠 MajorMove trap registration out of top-level scope.
Lines 137-140 run even when
install.shis sourced, while line 728 explicitly skipsmain()in that mode. This means anyone sourcing this file will inherit these EXIT/INT/TERM/HUP handlers in their shell. A subsequent Ctrl+C could then terminate the caller's shell instead of just interrupting a command.🐛 Proposed fix
+install_cleanup_traps() { + trap cleanup EXIT + trap 'trap_signal INT' INT + trap 'trap_signal TERM' TERM + trap 'trap_signal HUP' HUP +} + -trap cleanup EXIT -trap 'trap_signal INT' INT -trap 'trap_signal TERM' TERM -trap 'trap_signal HUP' HUP - main() { + install_cleanup_traps # Parse flags🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 137 - 140, The trap registrations (trap cleanup EXIT and trap 'trap_signal ...') are executed at top-level even when the script is sourced; move those trap setup calls into the execution-only path so they run only when the script is executed (not sourced). Concretely, register traps (using cleanup and trap_signal) inside the main execution guard or at the start of main() and ensure the script uses the standard sourcing check (compare BASH_SOURCE[0] to $0) so sourcing the file does not install EXIT/INT/TERM/HUP handlers into the caller's shell.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 116-119: The cleanup loop currently kills wrapper PIDs stored in
_cleanup_pids but not their descendant process groups; update the logic in the
loop that iterates over _cleanup_pids (used by spin()) to send the signal to the
process group instead of the single PID (use kill -TERM -- -$pid or equivalent)
so that child processes started by bash -c (npm/git/curl) are terminated; keep
the existing stderr redirection and tolerated-failure behavior (2>/dev/null ||
true) and apply this change inside the same loop that references _cleanup_pids
and spin().
- Around line 528-530: pre_extract_openclaw() currently creates tmpdir with
mktemp -d in a child shell but has no child-local trap, so interrupts during npm
pack or tar can leave the temp dir behind; modify the child block (where tmpdir
is created and npm pack / tar run) to register a trap that removes the tmpdir on
EXIT/INT/TERM (e.g. trap 'rm -rf "$tmpdir"' EXIT INT TERM) immediately after
creating tmpdir and before running npm pack/tar so cleanup always runs in the
child, and ensure the trap uses the same tmpdir variable name used in
pre_extract_openclaw.
---
Outside diff comments:
In `@install.sh`:
- Around line 405-426: The code adds nvm_tmp to the _cleanup_files tracking
array but never untracks it when the file is manually removed, leaving cleanup()
to later try to remove a reused path; after every manual rm -f "$nvm_tmp" in
this block (the curl failure branch, the integrity-failure branch, and after
spin finishes) call remove_tracked_file "$nvm_tmp" to remove it from the tracker
(follow the pattern used in spin()). Update references around nvm_tmp, spin(),
and cleanup() accordingly so the EXIT trap won't operate on a stale path.
---
Duplicate comments:
In `@install.sh`:
- Around line 137-140: The trap registrations (trap cleanup EXIT and trap
'trap_signal ...') are executed at top-level even when the script is sourced;
move those trap setup calls into the execution-only path so they run only when
the script is executed (not sourced). Concretely, register traps (using cleanup
and trap_signal) inside the main execution guard or at the start of main() and
ensure the script uses the standard sourcing check (compare BASH_SOURCE[0] to
$0) so sourcing the file does not install EXIT/INT/TERM/HUP handlers into the
caller's shell.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Author
|
Pushed a follow-up commit that:
I did not apply the process-group kill suggestion. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
install.sh (1)
69-143: Cleanup infrastructure is well-designed.The signal handling follows best practices:
trap_signal()runs cleanup, removes the trap, then re-raises the signal so the parent process sees the correct exit status. The guards at lines 79, 98, 116, and 121 correctly prevent Bash 3.2 +set -uunbound variable errors on empty arrays.One minor hygiene suggestion: declare loop variables as local in
cleanup()to avoid polluting the global namespace.♻️ Optional: declare loop variables local
cleanup() { local rc=$? set +e + local pid f if (( ${`#_cleanup_pids`[@]} )); then for pid in "${_cleanup_pids[@]}"; do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 69 - 143, The cleanup() function uses loop variables that are currently global (for pid and f), so declare them as local to avoid polluting the global namespace; update cleanup() to add local declarations like local pid f (or separate local pid and local f) before their respective for-loops so the variables are scoped to cleanup(), leaving register_cleanup_traps(), trap_signal(), remove_tracked_pid(), and remove_tracked_file() unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@install.sh`:
- Around line 69-143: The cleanup() function uses loop variables that are
currently global (for pid and f), so declare them as local to avoid polluting
the global namespace; update cleanup() to add local declarations like local pid
f (or separate local pid and local f) before their respective for-loops so the
variables are scoped to cleanup(), leaving register_cleanup_traps(),
trap_signal(), remove_tracked_pid(), and remove_tracked_file() unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0086a94d-615e-4a42-b8d0-2e0d0a1d88b1
📒 Files selected for processing (2)
install.shtest/install-preflight.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/install-preflight.test.js
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.
Summary
Fixes #1020 by cleaning up tracked background processes and temp files when
install.shexits or is interrupted.cleanup()plus signal handlers that re-raiseINT/TERM/HUPafter cleanupinstall_nodejs()temp installer, then untrack exact matches after normal completionset -ucompatibility via length-guarded array handlingTest plan
install-preflight.test.jspasses — these testssource install.shand call functions in isolation, exercising the cleanup trap on subshell exit with empty arrayscli.test.jspasses (21/21)make checklocally; observed pre-existing local-tooling failures (typescript-eslintmodule resolution and missinghadolint) unrelated to this shell-only patchnode:22-slim); the same pre-existingtest/cli.test.jsgateway failure reproduces onupstream/main, so it is not introduced by this patchset -uduring trap cleanupSummary by CodeRabbit
Bug Fixes
Tests
Documentation