fix: end the session on stop signals in run and proxy, and exit the way the child did - #15
Merged
Conversation
cvl
commented
Sep 9, 2026
cvl
commented
Contributor
- style: reword ignored-signal comment so gofmt keeps it literal
- style: gofmt run_signal.go
- test: live signal test expects shellroute to die by SIGTERM like its child
- fix(proxy): a stop signal during session startup ends the session
- fix(run): leave stop signals that are ignored on entry alone
- fix(run): a stop signal during session startup ends the session
- fix(run): exit the way the child did — re-raise its signal, keep its code
- style: fix gofmt alignment in run_signal.go
- fix: normal test wait 20s to match API client timeout
- fix: cleanup timeout 25s (5s escalation + 15s API timeout + margin)
- fix: guard SIGKILL against PID reuse, 10s cleanup timeout, specific CONTRIBUTING entry
- fix: live test uses process group check, 7s graceful cleanup, guarded mktemp
- fix: live signal test — child exits 0, exact PGID check, strict args
- test: add live signal-handling test for shellroute run
- refactor: extract RunSignalHandler, fix second-signal swallow
- test: real subprocess signal tests for shellroute run
- fix: handle SIGINT/SIGTERM/SIGHUP in shellroute run
Forward signal to child process group, escalate to SIGKILL after 5 seconds if child doesn't exit, then always call sess.Stop() for clean API session teardown. Ensures systemd stop, Ctrl+C, and terminal close all end the session cleanly instead of relying on the orphan reaper. Signed-off-by: contra <vytautas@civilis.lt>
8 tests using real bash subprocesses: - SIGTERM forwarded, child exits via trap - SIGHUP forwarded, child exits via trap - SIGKILL escalation when child ignores SIGTERM - No escalation when child exits promptly - Signal to dead process doesn't panic - Second signal not swallowed during escalation wait - All three signals registered in run.go - sess.Stop() always called after Wait() Signed-off-by: contra <vytautas@civilis.lt>
Production signal handler extracted to run_signal.go as RunSignalHandler. Fixes: - Second signal during escalation wait triggers immediate SIGKILL (was swallowed by single-read channel) - Buffer channel size 2 so second signal isn't lost - EscalateAfter injectable for tests run.go calls RunSignalHandler with process group PID. Tests call production RunSignalHandler directly: - SIGTERM forwarding via syscall.Kill(Getpid()) - SIGHUP forwarding - Timeout escalation to SIGKILL - Second signal immediate SIGKILL - No escalation when child exits promptly - No panic after child exit Signed-off-by: contra <vytautas@civilis.lt>
scripts/test-run-signal-live.sh --live [COUNTRY] Requires --live opt-in (creates one paid session). Builds from current checkout, launches child with SIGTERM trap, sends SIGTERM to shellroute, verifies: child received signal, session ended cleanly, no orphan processes. Not in CI — manual verification for release gating. Signed-off-by: contra <vytautas@civilis.lt>
Fixes all blockers: - Child trap exits 0 (not 42), assert shellroute exits 0 - Orphan check uses exact child PGID from temp file - All temp files in single WORK_DIR initialized before trap - Build from REPO_ROOT with ./cmd/shellroute - Strict arg parsing rejects unknown flags and extra args - Process group cleanup in EXIT trap - Exact 'shellroute session ended' marker - Output shown on every failure - CONTRIBUTING.md documents optional live tests Signed-off-by: contra <vytautas@civilis.lt>
… mktemp - Orphan verification: kill -0 -- -PGID (process group, not just PID) - Cleanup: SIGTERM → wait up to 7s for shellroute graceful path → SIGKILL - Child group cleanup: numeric PGID validation, kill -9 -- -PGID - mktemp failure guarded before any work - Exact marker: 'shellroute session ended.' with period Signed-off-by: contra <vytautas@civilis.lt>
…ONTRIBUTING entry Cleanup recheck kill -0 before SIGKILL to avoid hitting a reused PID. Timeout increased to 10s (5s child escalation + API teardown). CONTRIBUTING describes the specific test, not generic 'some tests'. Signed-off-by: contra <vytautas@civilis.lt>
Signed-off-by: contra <vytautas@civilis.lt>
Signed-off-by: contra <vytautas@civilis.lt>
Signed-off-by: contra <vytautas@civilis.lt>
…code A child killed by a signal made shellroute exit 255 (Go reports -1 for signal deaths) and print "Command failed". Under systemd stop with the default KillMode, most programs die from SIGTERM, so a clean stop left the unit in "failed (status=255)"; Ctrl+C gave 255 instead of 130. Now shellroute mirrors the child: exit codes pass through, and SIGINT/ SIGTERM/SIGHUP/SIGKILL deaths are re-raised on shellroute itself so systemd sees a clean stop and shells get the 128+n convention. Other signals use 128+n directly because the Go runtime would turn them into a crash dump. "Command failed" is only printed for non-zero exit codes. Signed-off-by: contra <vytautas@civilis.lt>
Signal handling started only after the child was running. A SIGTERM or Ctrl+C while the session was being created (up to ~60s: API call plus exit-IP detection) killed shellroute with the default action and left the API session open until the server reaper closed it. RunSignalHandler becomes SignalHandler, registered before session.Start: before Attach a signal cancels the session context and is remembered; Attach switches to forwarding, delivering a signal that arrived earlier to the child. Exit-IP detection now observes the context so the abort is prompt, and the interrupted startup ends the session, prints the summary, and exits by re-raising the signal. Signed-off-by: contra <vytautas@civilis.lt>
signal.Notify re-enables a signal that was ignored when the process started. Under nohup that defeated the hangup immunity: shellroute caught the SIGHUP, forwarded it to a child that had inherited the ignore, and escalated to SIGKILL five seconds later. On v0.1.3 both survived. The handler now skips any requested signal that signal.Ignored reports, so nohup keeps working and other stop signals stay handled. Signed-off-by: contra <vytautas@civilis.lt>
Same gap as in run mode: signals were only handled once the proxy was up, so a SIGTERM or Ctrl+C during session creation killed the process and left the API session open. The handler is now registered before session.Start; an interrupted startup ends the session and prints the summary like a normal disconnect. Signed-off-by: contra <vytautas@civilis.lt>
…child The child now dies from the forwarded SIGTERM instead of exiting 0, which is what real programs under systemd do, and shellroute must exit by SIGTERM (status 143 in bash) rather than 0 or 255. Signed-off-by: contra <vytautas@civilis.lt>
Signed-off-by: contra <vytautas@civilis.lt>
Signed-off-by: contra <vytautas@civilis.lt>
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.