fix(signal): spawn lifecycle scripts detached to stop a duplicate SIGINT - #60
itsybitsybootsy wants to merge 1 commit into
Conversation
A terminal's Ctrl-C delivers SIGINT to the whole foreground process
group at once, so a lifecycle script (spawned in our process group)
already receives it directly. procInterrupt() then forwards another
SIGINT on top of that. A script whose own handler only runs once (the
common process.once('SIGINT', cleanup) pattern) gets killed by the
redundant second signal before it can finish a graceful shutdown.
Spawn the script detached instead. On POSIX this makes it a new
process group and session leader, so the terminal's broadcast no
longer reaches it directly and procInterrupt()'s forward becomes the
only way it's delivered.
Once the script is its own process group leader, forward the signal
to that whole group instead of just its pid. A compound command like
`sleep 5 && echo done` runs sleep as its own process, and forwarding
to only the `sh` pid never reaches it, so the chain ran to completion
instead of stopping. This wasn't a problem before, because the
terminal's own broadcast used to reach sleep directly too.
This has two costs. A script killed by an external process-group-wide
signal (rather than through pnpm) no longer dies incidentally with
it, since it is no longer in that group. And a script with no
controlling terminal can't open /dev/tty, which some tools use for a
secure prompt (sudo -S, ssh, gpg, git's askpass fallback) even with
stdin piped.
Fixes: pnpm/pnpm#7374
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📜 Recent review details🧰 Additional context used🪛 ast-grep (0.44.1)lib/spawn.js[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec. (detect-child-process) test/signal-group.js[warning] 63-63: Avoid require with non-literal values (detect-non-literal-require) [warning] 89-89: Avoid require with non-literal values (detect-non-literal-require) [warning] 3-3: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec. (detect-child-process) 🔇 Additional comments (8)
📝 WalkthroughWalkthroughLifecycle scripts now use detached process groups on non-Windows platforms. Signal handling targets the group when possible, while spawned-process PIDs are exposed and new fixtures test graceful cleanup and compound-command interruption. ChangesLifecycle signal handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Lifecycle
participant SpawnedShell
participant ProcessGroup
participant ScriptChild
Lifecycle->>SpawnedShell: Start detached lifecycle script
SpawnedShell->>ProcessGroup: Create process group
Lifecycle->>ProcessGroup: Send SIGINT
ProcessGroup->>ScriptChild: Deliver signal
ScriptChild-->>Lifecycle: Complete cleanup and exit
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Fixes: pnpm/pnpm#7374
What's the problem
pnpm run <script>can kill the script before it finishes handling Ctrl-C. A terminal's SIGINT is delivered to the whole foreground process group at once, so the script (spawned in our process group) already gets it directly.procInterrupt()then forwards another SIGINT on top of that. A script usingprocess.once('SIGINT', cleanup)(a very common pattern) has its handler consumed by the first signal and starts async cleanup; the second, redundant signal arrives with no handler left and Node kills it immediately, before cleanup finishes. This is what people are reporting as Kubernetes pods and CI jobs dying without running their shutdown code.The fix
Spawn the script
detached(POSIX only,process.platform !== 'win32'). This makes it a new process group and session leader, so a terminal's broadcast no longer reaches it directly, andprocInterrupt()'s forward becomes the only way it ever learns about SIGINT. No more guessing whether to forward.I tried a smaller fix first (skip the forward when
process.stdin/stdout/stderrlooked like a TTY) but it's wrong in both directions: an interactive session with a targetedkill -INT <pid>(an IDE's stop button does this) skips the forward and the script hangs forever, and a non-interactive wrapper that broadcasts to its own process group still gets the original bug. There's no way to tell from inside a bare SIGINT handler how a given signal was actually delivered, so the detached approach (removing the ambiguity instead of guessing at it) is the one I ended up with.Once the script is its own process group leader,
procInterrupt()/procKill()forward to that whole group (process.kill(-proc.pid, sig)) instead of just its pid. Caught this in my own testing: a compound command likesleep 5 && echo donerunssleepas its own process still insh's group, and a pid-only forward never reaches it, so the chain ran to completion instead of stopping. This wasn't a problem before this PR, because the terminal's own broadcast used to reachsleepdirectly too.lib/spawn.jsdidn't expose the child's pid at all, so I addedcooked.pid = raw.pidthere.Trade-offs, both real
open('/dev/tty'). Some tools do that for a secure prompt even with stdin piped (sudo -S, ansshpassphrase prompt,gpg's pinentry-curses, git's askpass fallback). That will now fail with ENXIO where it worked before.How I verified it
Added
test/signal-group.js, three tests: a signal delivered to our own process group doesn't also reach the script directly anymore (a driver spawneddetached: trueso a realprocess.kill(0, 'SIGINT')doesn't touch the test runner's own process tree), a signal targeted only at our pid still gets forwarded, and a compound&&command is actually interrupted rather than running to completion. All three fail on the current code and pass with the fix.I was worried the detach would break scripts that read stdin interactively (SIGTTIN stopping a background process group reading the controlling terminal), so I checked it directly rather than assuming: a
pty.fork()-based harness on both macOS and a realubuntu-latestGitHub Actions run shows a detached child reading from an inherited pty completes normally, no stop. Node'sdetachedalso callssetsid(), and SIGTTIN only applies to a process's own controlling-terminal relationship, which a fresh session doesn't have, so reads on the inherited fd just work like a normal file descriptor.node --test test/*.js: 9/9 pass, no new failures.npx standard: clean.Summary by CodeRabbit
Bug Fixes
New Features