Add bin/shakapacker-watch binstub for clean Ctrl-C shutdown#1026
Conversation
The new wrapper script traps INT/TERM signals, forwards TERM to the underlying bin/shakapacker process, and waits quietly. This prevents Ruby interrupt backtraces when stopping Procfile-based dev workflows (e.g., bin/dev with overmind/foreman). Use bin/shakapacker-watch --watch in Procfiles instead of bin/shakapacker --watch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughA new shell wrapper binstub Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
ReviewThe approach is sound - a POSIX shell wrapper that traps signals and forwards TERM to the child is the right way to suppress Ruby interrupt backtraces in Procfile-based dev workflows. The binstub wiring via lib/install/bin/ and the rake task is consistent with the existing pattern for bin/shakapacker and bin/shakapacker-dev-server. Three issues in the shell script worth addressing (inline comments added):
Minor nit: spec/dummy/bin/shakapacker-watch is a verbatim copy of lib/install/bin/shakapacker-watch. If the two ever drift this will cause confusion. It might be worth adding a comment noting it is generated by rake shakapacker:binstubs, or generating it as part of CI setup rather than committing it. |
Greptile SummaryThis PR introduces a Key points:
Confidence Score: 4/5Safe to merge after fixing the trap ordering and the #NNN changelog placeholder. The overall design is sound and the primary use case (Procfile + Ctrl-C) is handled correctly because the trap IS in place before the long-running lib/install/bin/shakapacker-watch and spec/dummy/bin/shakapacker-watch (identical fix needed in both for trap ordering) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[bin/shakapacker-watch invoked] --> B["bin/shakapacker & (background)"]
B --> C["child_pid=$!"]
C --> D["trap cleanup INT TERM ⚠️ set AFTER fork"]
D --> E["wait $child_pid"]
E -->|"child exits 0"| F["status=0"]
E -->|"child exits non-zero"| G["status=$?"]
E -->|"INT or TERM received"| H["cleanup()"]
F --> I["child_pid=''"]
G --> I
I --> J["exit $status"]
H --> K{"child_pid set?"}
K -->|yes| L["kill -TERM $child_pid"]
K -->|no| N["exit 0"]
L --> M["wait $child_pid"]
M --> N
style D fill:#ffcccc,stroke:#cc0000
Reviews (1): Last reviewed commit: "Update CHANGELOG with actual PR number" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Review of shakapacker-watch binstub. The approach is sound - wrapping the watcher in a shell script that traps signals is a well-established pattern for clean Procfile shutdown. A few issues worth addressing before merge. 1. Relative path: the child is invoked with a bare relative path, so the script only works when run from the app root. Using dirname-based resolution makes it portable (see inline comment). 2. Trap set after child launch: there is a narrow race window between launching the child and installing the trap. Moving the trap call before the child launch closes it (see inline comment). 3. Cleanup exits 0 on TERM: the cleanup handler always exits 0, masking signal-killed termination from supervisors. The POSIX fix is to reset the trap and re-raise the signal (see inline comment). Minor: check_binstubs.rake does not check for bin/shakapacker-watch (optional but worth adding). The spec/dummy copy is consistent with other dummy bin scripts. No automated signal-handling tests, but sh -n is reasonable for a shell script. Overall the PR solves a real UX pain point. Main ask is fixing path resolution and trap ordering. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Signal trap set after child process starts
- Moved INT/TERM trap installation before child launch in both shakapacker-watch scripts to eliminate the startup signal race window.
Or push these changes by commenting:
@cursor push 1f46d05a85
Preview (1f46d05a85)
diff --git a/lib/install/bin/shakapacker-watch b/lib/install/bin/shakapacker-watch
--- a/lib/install/bin/shakapacker-watch
+++ b/lib/install/bin/shakapacker-watch
@@ -12,9 +12,9 @@
exit 0
}
+trap cleanup INT TERM
bin/shakapacker "$@" &
child_pid=$!
-trap cleanup INT TERM
if wait "$child_pid"; then
status=0
diff --git a/spec/dummy/bin/shakapacker-watch b/spec/dummy/bin/shakapacker-watch
--- a/spec/dummy/bin/shakapacker-watch
+++ b/spec/dummy/bin/shakapacker-watch
@@ -12,9 +12,9 @@
exit 0
}
+trap cleanup INT TERM
bin/shakapacker "$@" &
child_pid=$!
-trap cleanup INT TERM
if wait "$child_pid"; then
status=0You can send follow-ups to this agent here.
…tdown Use script_dir for reliable sibling script resolution instead of bare bin/shakapacker. Move trap before spawning the background process so signals are never missed. Add a 5-second graceful wait loop with SIGKILL fallback so orphan webpack processes cannot survive Ctrl-C. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Review: Add bin/shakapacker-watch binstubThe approach is sound — absorbing INT/TERM in a shell wrapper and forwarding TERM to the child is the right pattern for Procfile setups. The signal-handling logic is correct: Issues1.
Users who upgrade and forget to re-run 2.
Nits
|
|
Re: adding |
Prevents the two identical files from silently drifting apart. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 `@lib/install/bin/shakapacker-watch`:
- Around line 7-24: cleanup() can run before child_pid is assigned causing the
newly spawned watcher to survive; add a shutdown coordination flag (e.g.,
shutdown_requested) and modify cleanup() to set this flag and return immediately
when child_pid is empty, and then after the watcher is started and child_pid is
assigned check shutdown_requested and, if set, perform the same termination
sequence (kill -TERM/KILL and wait) to ensure the child is always stopped;
reference the cleanup() function and the child_pid and shutdown_requested
variables when implementing the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 402aab34-8049-4576-a7eb-2479fef862cf
⛔ Files ignored due to path filters (2)
spec/dummy/package-lock.jsonis excluded by!**/package-lock.jsonspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
CHANGELOG.mdlib/install/bin/shakapacker-watchspec/dummy/Procfile.devspec/dummy/Procfile.dev-staticspec/dummy/Procfile.dev-static-assetsspec/dummy/bin/shakapacker-watchspec/dummy/package.json
Review: Add bin/shakapacker-watch binstubThe overall approach is sound — a POSIX sh wrapper that traps INT/TERM and forwards TERM to the child is the right pattern for clean Ctrl-C shutdown in Procfile-based workflows. A few issues to address: Signal exit code (inline comment on line 22)The
|
Document the new shakapacker-watch binstub in the README Development section (update binstub count, add to Common Development Commands) and add --help/-h flag handling to the shell script with usage info, examples, and a Procfile snippet. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review: Add bin/shakapacker-watch binstubOverall: The approach is sound — a shell wrapper that traps INT/TERM and forwards TERM to the child is the right way to absorb Ctrl-C noise in Procfile workflows. The binstub slots cleanly into the existing Must fixchild_pid assignment race (lines 57–58): The Low severity / informational
Not an issue
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/install/bin/shakapacker-watch (1)
42-42: Minor portability note:sleep 0.1is not strictly POSIX.Fractional seconds work with GNU coreutils and macOS, but may fail on minimal/older shells. If broad portability is a goal, consider a fallback or use integer sleeps (e.g., 1-second intervals with fewer iterations).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/install/bin/shakapacker-watch` at line 42, In shakapacker-watch, the fractional call "sleep 0.1" is not POSIX-portable; change it to try a fractional sleep and fall back to an integer sleep when the shell's sleep doesn't support fractions (e.g. attempt "sleep 0.1" and if it fails, call "sleep 1"), keeping the same behavior loop structure — locate the "sleep 0.1" token and replace it with a small fallback wrapper (try fractional sleep and on error use integer sleep).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/install/bin/shakapacker-watch`:
- Line 42: In shakapacker-watch, the fractional call "sleep 0.1" is not
POSIX-portable; change it to try a fractional sleep and fall back to an integer
sleep when the shell's sleep doesn't support fractions (e.g. attempt "sleep 0.1"
and if it fails, call "sleep 1"), keeping the same behavior loop structure —
locate the "sleep 0.1" token and replace it with a small fallback wrapper (try
fractional sleep and on error use integer sleep).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f370fbaf-d309-4f36-ac7e-f28c7b2b6797
📒 Files selected for processing (2)
README.mdlib/install/bin/shakapacker-watch
|
Added |
- Add shutdown_requested flag to close race window between child launch and PID assignment in shakapacker-watch - Add bin/shakapacker-watch to doctor.rb print_binstub_status (informational only, not check_binstub since the watch binstub is optional) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/install/bin/shakapacker-watch (1)
46-50: Minor portability consideration: fractionalsleepargument.
sleep 0.1requires GNU coreutils or similar; POSIX only guarantees integer seconds. This works on Linux and macOS but may fail on minimal BusyBox or older systems. Given this is a development-time script (Procfile workflows), the impact is low.💡 Alternative using a portable fallback
If broader portability is desired:
while [ "$i" -lt 50 ] && kill -0 "$child_pid" 2>/dev/null; do - sleep 0.1 + sleep 1 2>/dev/null || sleep 0.1 i=$((i + 1)) doneOr simply increase the poll count with integer sleeps. However, keeping
sleep 0.1is reasonable for typical developer environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/install/bin/shakapacker-watch` around lines 46 - 50, The loop uses a fractional sleep (sleep 0.1) which is not guaranteed by POSIX; change it to an integer-second sleep and adjust the loop count to preserve the same total wait time. For example, in the while loop that checks kill -0 "$child_pid" (the block using variables i and child_pid), replace sleep 0.1 and i<50 with sleep 1 and i<5 (or another integer pair that totals ~5 seconds) so the script remains portable on BusyBox/older systems while keeping the same timeout behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/install/bin/shakapacker-watch`:
- Around line 46-50: The loop uses a fractional sleep (sleep 0.1) which is not
guaranteed by POSIX; change it to an integer-second sleep and adjust the loop
count to preserve the same total wait time. For example, in the while loop that
checks kill -0 "$child_pid" (the block using variables i and child_pid), replace
sleep 0.1 and i<50 with sleep 1 and i<5 (or another integer pair that totals ~5
seconds) so the script remains portable on BusyBox/older systems while keeping
the same timeout behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c875638e-6b1f-421f-b060-80b937f7f10a
📒 Files selected for processing (2)
lib/install/bin/shakapacker-watchlib/shakapacker/doctor.rb
|
Overall: looks solid. The signal-handling logic is correct and the use case is well-motivated. Signal handling details: trap is set before the background process spawns (no missed signals), the shutdown_requested flag closes the race window between the background launch and child_pid assignment, and the SIGKILL fallback after the grace period prevents orphaned webpack processes. The portable CDPATH+dirname idiom for script_dir is correct. One portability note: sleep 0.1 (fractional seconds) is not POSIX-compliant. It works on GNU coreutils and macOS but errors on Alpine Linux / BusyBox ash, which is common in Docker-based Rails setups. sleep 1 with a loop limit of 5 would be fully portable and still give a 5-second grace window. The cleanup function always exits with code 0 on signal — right for Ctrl-C but means SIGTERM also masks any non-zero webpack exit. Fine for a dev tool, just worth being aware of. The decision to add shakapacker-watch to print_binstub_status but not check_binstub is correct since the binstub is optional. The script name implies watch mode but --watch must still be passed explicitly; the docs and examples are clear about this. |
| kill -TERM "$child_pid" 2>/dev/null || true | ||
| i=0 | ||
| while [ "$i" -lt 50 ] && kill -0 "$child_pid" 2>/dev/null; do | ||
| sleep 0.1 |
There was a problem hiding this comment.
sleep 0.1 uses fractional seconds, which is not POSIX-compliant. It works on GNU coreutils (Linux) and macOS but errors on Alpine Linux / BusyBox ash — common in Docker-based Rails setups. A portable alternative that keeps the same 5-second total grace window:
| sleep 0.1 | |
| sleep 1 | |
| i=$((i + 1)) | |
| done | |
| if kill -0 "$child_pid" 2>/dev/null; then |
Change the loop bound from 50 to 5 alongside this to match the original timeout.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Optional binstub prevents "All binstubs found" doctor message
- Updated doctor binstub status reporting to require only the three required binstubs for the consolidated success message while reporting shakapacker-watch separately as optional.
Or push these changes by commenting:
@cursor push ba29c57533
Preview (ba29c57533)
diff --git a/lib/shakapacker/doctor.rb b/lib/shakapacker/doctor.rb
--- a/lib/shakapacker/doctor.rb
+++ b/lib/shakapacker/doctor.rb
@@ -1010,22 +1010,27 @@
end
def print_binstub_status
- binstubs = [
+ required_binstubs = [
"bin/shakapacker",
"bin/shakapacker-dev-server",
- "bin/shakapacker-config",
- "bin/shakapacker-watch"
+ "bin/shakapacker-config"
]
+ optional_binstubs = ["bin/shakapacker-watch"]
- existing_binstubs = binstubs.select { |b| doctor.root_path.join(b).exist? }
+ existing_required_binstubs = required_binstubs.select { |b| doctor.root_path.join(b).exist? }
+ existing_optional_binstubs = optional_binstubs.select { |b| doctor.root_path.join(b).exist? }
- if existing_binstubs.length == binstubs.length
- puts "✓ All Shakapacker binstubs found (#{existing_binstubs.join(', ')})"
- elsif existing_binstubs.any?
- existing_binstubs.each do |binstub|
+ if existing_required_binstubs.length == required_binstubs.length
+ puts "✓ All Shakapacker binstubs found (#{required_binstubs.join(', ')})"
+ elsif existing_required_binstubs.any?
+ existing_required_binstubs.each do |binstub|
puts "✓ #{binstub} found"
end
end
+
+ existing_optional_binstubs.each do |binstub|
+ puts "✓ #{binstub} found (optional)"
+ end
end
def print_info_messagesYou can send follow-ups to this agent here.
| "bin/shakapacker-dev-server", | ||
| "bin/shakapacker-config" | ||
| "bin/shakapacker-config", | ||
| "bin/shakapacker-watch" |
There was a problem hiding this comment.
Optional binstub prevents "All binstubs found" doctor message
Low Severity
Adding bin/shakapacker-watch to the binstubs array in print_binstub_status means the "✓ All Shakapacker binstubs found" message now requires this optional binstub to be present. Every existing installation that upgrades without running rake shakapacker:binstubs will regress from seeing the consolidated "All found" message to seeing individual per-binstub ✓ lines, even though the installation is perfectly functional. This contradicts the stated intent that shakapacker-watch is optional — the doctor now effectively treats it as required for a clean bill of health.
1 participant
Summary
bin/shakapacker-watchshell script binstub that traps INT/TERM signals and forwards TERM to the underlyingbin/shakapackerprocess for clean shutdownbin/shakapacker-watch --watchinstead ofbin/shakapacker --watchbin/shakapackerandbin/shakapacker-dev-serverviarake shakapacker:binstubsWhy
When using
bin/devwith overmind or foreman, pressing Ctrl-C causes Ruby interrupt backtraces from watcher processes. This is a bad first-run experience. The wrapper script absorbs the INT signal and sends a clean TERM to the child process.Related: shakacode/react_on_rails#2652 — this PR moves the
bin/shakapacker-watchwrapper from react_on_rails into shakapacker where it belongs, since shakapacker already generates the other bin scripts.Test plan
sh -n lib/install/bin/shakapacker-watchpasses syntax checkbundle exec rubocoppassesyarn lintpassesbundle exec rspec spec/shakapacker/tests pass (979/981, 2 pre-existing failures unrelated to this change)rake shakapacker:binstubscopiesbin/shakapacker-watchto the appbin/devwith Ctrl-C exits cleanly without Ruby backtraces🤖 Generated with Claude Code
Note
Low Risk
Low risk: adds a new optional binstub and updates docs/examples; core compilation/runtime logic is unchanged aside from doctor reporting an extra expected binstub.
Overview
Adds a new
bin/shakapacker-watchshell wrapper binstub that runsbin/shakapackerand trapsINT/TERMto terminate the child process cleanly (avoiding interrupt backtraces in Procfile runners likebin/dev).Updates docs and diagnostics to recognize/recommend the new binstub (
README,CHANGELOG, andShakapacker::Doctorbinstub reporting), and switches the dummy app Procfiles (plus dummy binstub) to useshakapacker-watchfor--watchworkflows.Written by Cursor Bugbot for commit 210eac5. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Documentation
Chores