fix: scope-intercept signal handling for long-lived workloads#309
Open
rubberduck203 wants to merge 2 commits intomainfrom
Open
fix: scope-intercept signal handling for long-lived workloads#309rubberduck203 wants to merge 2 commits intomainfrom
rubberduck203 wants to merge 2 commits intomainfrom
Conversation
rubberduck203
changed the title
rubberduck203
force-pushed
the
cmac/intercept-forward-signals-on-interactive
branch
from
88de029 to
57b9f02
Compare
… clean up When scope-intercept wraps a long-running process (e.g. as the shebang on a server start script), Ctrl+C in the terminal delivers SIGINT to every member of the foreground process group — both scope-intercept and the wrapped command. Without a custom signal handler, scope-intercept follows the default and aborts immediately, closing the captured stdout/stderr pipes. The child then dies with SIGPIPE before its own SIGINT trap can finish, leaving services like overmind/nginx orphaned. Install Tokio Unix signal listeners for SIGINT, SIGTERM, and SIGHUP at the start of run_command. Their only job is to consume the default- terminate behavior so scope-intercept stays alive while the child runs its trap. The OS already delivered the signal to the child via the shared process group, so no manual forwarding is needed. When an interrupt has fired and the child exits non-zero, skip the known-error analysis, retry, and bug-report flows — the user asked to quit, not to be quizzed. OutputCapture and the scope doctor path are intentionally untouched; this is scoped to the intercept binary, which is the one wrapping interactive long-lived workloads. Includes a new regression test that spawns scope-intercept in its own process group, sends SIGINT via killpg (mimicking a terminal), and asserts the child's bash trap ran and the exit code is 130. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
rubberduck203
force-pushed
the
cmac/intercept-forward-signals-on-interactive
branch
from
57b9f02 to
9c715fe
Compare
rubberduck203
commented
Apr 30, 2026
rubberduck203
commented
Apr 30, 2026
rubberduck203
commented
Apr 30, 2026
Per review on #309: - Replace raw libc::setsid / libc::killpg with the typed `nix` wrappers (`nix::unistd::setsid`, `nix::sys::signal::killpg` with `Pid` and `Signal`). Drops the `as i32` cast and the manual errno handling. - Add a SAFETY comment on the `unsafe { command.pre_exec(...) }` block spelling out the async-signal-safety contract pre_exec requires. - Replace the try_wait + sleep + deadline polling loop with `wait_timeout::ChildExt::wait_timeout`, which is the standard idiom for "wait for a child with a deadline". - Annotate the remaining poll loop (waiting for the bash trap to install a marker file) explaining why a sleep is needed there — no good cross-process file-appeared notification primitive without inotify/kqueue. Cargo.toml: drop `libc`, add `nix = "0.29"` (signal+process features) and `wait-timeout = "0.2"` as dev-deps. Test still passes; full suite remains green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring alerts on:
|
Contributor
Author
|
@SocketSecurity ignore cargo/nix@0.29.0 Test files don't get compiled and distributed into the application. |
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.
Why
scope-interceptis sometimes used as a shebang on long-running scripts to enable self-healing known-error fix-and-retry. We had a report that Ctrl+C stopped working in that configuration: services started under overmind (nginx, etc.) kept running, the overmind socket was orphaned, and the next start failed with an "overmind previously crashed" prompt.The terminal sends SIGINT to every member of the foreground process group — both
scope-interceptand the wrappedenv -S bash. With no signal handler,scope-interceptfollowed the default and aborted immediately. Its captured stdout/stderr pipes closed, and the child died with SIGPIPE before its own SIGINT trap (and overmind's shutdown logic) could complete.scope-interceptwas originally designed for short-lived setup scripts, but using it as a shebang for long-running interactive processes is a strong fit if it cooperates with the child's lifecycle. This PR makes it cooperate.What changed
src/bin/scope-intercept.rs— At the top ofrun_command, install Tokio Unix signal listeners forSIGINT,SIGTERM, andSIGHUP. Their only job is to consume the default-terminate behavior soscope-interceptstays alive while the child runs its cleanup. No manual forwarding is needed — the OS already delivered the signal to the child via the shared process group.When the interrupt flag is set and the child exited non-zero, skip the known-error analysis, retry, and bug-report flows. The user asked to quit, not to be quizzed.
OutputCapture::capture_outputand thescope doctorpath are deliberately not touched — this fix is scoped to the intercept binary.Cargo.toml— Addslibcas a dev-dep so the new test can callsetsid/killpg.Tests
tests/scope_intercept.rs::test_intercept_forwards_sigint_and_waits_for_child_cleanup:trap.shthat traps SIGINT, writescleanup.done, and sleeps.scope-intercept -- bash trap.shin its own session/process group viapre_exec(setsid).readymarker, then sends SIGINT to the group viakillpg— this is what a terminal does on Ctrl+C.cleanup.doneexists with the expected contents),scope-interceptexited within 10s, and the exit code is 130.Verified the test fails on
main(without the fix, exit status isunix_wait_status(2)—scope-interceptkilled mid-flight by SIGINT) and passes with the fix.All 8 existing intercept tests still pass; full suite (177 tests) green.
Test plan
cargo test --test scope_intercept— 8/8 passcargo test— 177/177 passcargo fmt --checkclean