fix(install): add trap handler to spin() for clean Ctrl+C teardown#1070
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughGlobal cleanup and signal handling were added to Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User (Ctrl+C)"
participant OS as "OS"
participant Script as "install.sh"
participant Job as "Background Job\n(e.g., npm, git, docker)"
rect rgba(100,149,237,0.5)
User->>OS: SIGINT / SIGTERM
OS->>Script: deliver signal (trap)
end
rect rgba(60,179,113,0.5)
Script->>Script: _global_cleanup (trap handler)
Script->>Job: kill background PID(s)
Script->>Script: rm -f temp log files
Script->>OS: exit
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
install.sh (1)
197-238: Consider a global EXIT trap for more robust cleanup (optional).The current function-scoped trap handles the primary use case well. For additional robustness, the linked issue (
#1020) suggested maintaining global arrays (_cleanup_pids,_cleanup_files) with an EXIT trap to catch all exit paths. This would protect against edge cases likeset -etriggering between trap clearance and explicit cleanup.Given the sequential nature of spin() calls in this script and the small risk window, the current approach is reasonable. The global pattern could be considered if future changes introduce more complex cleanup needs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 197 - 238, The spin() function currently installs a temporary INT/TERM trap and removes the temp file on completion, but to make cleanup robust for all exit paths (e.g., set -e or unexpected exits) add global cleanup arrays and a single EXIT trap: create global arrays _cleanup_pids and _cleanup_files and an initialization that registers a one-time trap on EXIT which iterates those arrays to kill remaining PIDs and remove files; inside spin() push the background pid onto _cleanup_pids and the mktemp path onto _cleanup_files, and on normal completion remove those entries from the arrays before clearing the per-function trap; ensure symbols referenced are spin, _cleanup_pids, _cleanup_files and the global EXIT trap so temp logs are always removed and background processes killed even if the script exits unexpectedly.
🤖 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 197-238: The spin() function currently installs a temporary
INT/TERM trap and removes the temp file on completion, but to make cleanup
robust for all exit paths (e.g., set -e or unexpected exits) add global cleanup
arrays and a single EXIT trap: create global arrays _cleanup_pids and
_cleanup_files and an initialization that registers a one-time trap on EXIT
which iterates those arrays to kill remaining PIDs and remove files; inside
spin() push the background pid onto _cleanup_pids and the mktemp path onto
_cleanup_files, and on normal completion remove those entries from the arrays
before clearing the per-function trap; ensure symbols referenced are spin,
_cleanup_pids, _cleanup_files and the global EXIT trap so temp logs are always
removed and background processes killed even if the script exits unexpectedly.
|
@latenighthackathon all your commits need to have verified signatures. Please fix on this PR and any other PRs you put up against NemoClaw |
|
@kjw3 - fixed TY! |
spin() runs commands in the background with a spinner animation but had no signal handler. Pressing Ctrl+C during installation would kill the parent shell while leaving the background process running and the temp log file on disk. Add INT/TERM trap inside spin() that kills the background process and removes the temp file, then restore default signal handling after the background process exits normally. Closes NVIDIA#1020
The function-scoped INT/TERM trap in spin() handles Ctrl+C well, but set -e triggering between trap clearance and explicit rm -f could leak temp files or orphan background processes. Add global _cleanup_pids and _cleanup_files arrays with an EXIT trap that catches all exit paths. spin() registers its PID and temp file on entry and deregisters on normal completion. This defense-in-depth ensures cleanup even on unexpected exits. Addresses CodeRabbit review suggestion.
latenighthackathon
force-pushed
the
fix/install-spin-trap-handler
branch
from
f715321 to
587cb94
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 248-250: The removals of $pid and $log from the global cleanup
arrays (_cleanup_pids and _cleanup_files) are happening before the actual
cleanup (rm -f and any pid termination) and risk leaking resources if a signal
arrives; move the two lines that dereference
_cleanup_pids=("${_cleanup_pids[@]/$pid/}") and
_cleanup_files=("${_cleanup_files[@]/$log/}") so they execute only after the
corresponding cleanup actions (e.g., after rm -f "$log" and after the pid
termination/cleanup code) have completed; ensure the deregistration happens
immediately after the actual removal/kill so the EXIT trap still knows about the
resource until it is gone.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Move _cleanup_pids and _cleanup_files deregistration to after the actual cleanup actions (rm -f, pid termination) so the global EXIT trap still covers these resources if a signal arrives mid-cleanup. Closes CodeRabbit review comment on NVIDIA#1070. Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
|
Thanks to @stevenobiajulu who also tackled this in #1026 — appreciate the parallel effort on the install cleanup. Great to see multiple contributors independently identifying the same reliability gap. |
2 participants
Summary
spin() runs background commands with a spinner but had no signal handler. Ctrl+C during installation orphaned the background process and leaked temp files. Added INT/TERM trap and global EXIT trap for defense-in-depth.
Related Issue
Closes #1020
Changes
Type of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Checklist
General
Code Changes
Summary by CodeRabbit