Repository navigation
Tear down session tongs on SIGHUP and SIGTERM - #95
Merged
Merged
Conversation
Closing the terminal or tmux pane a session runs in sends the launcher SIGHUP, and nothing handled it: Python's default action killed the process without running run_with_tongs' finally, so the session tongs -- including socket-holding ones with host-level power -- kept running with nothing left to remove them. A plain `kill` (SIGTERM) leaked them the same way, and so did either signal arriving during a teardown that had already started. Only Ctrl-C, via KeyboardInterrupt, was handled. While main orchestrates tongs, SIGHUP and SIGTERM now raise TerminationSignal, which unwinds through the same finally and exits 128 + signum (129, 143), as SIGINT exits 130. The no-tongs passthrough still execs untouched, a signal already ignored on entry (nohup) stays ignored, and the previous handlers are restored once the run returns. - Teardown runs inside a guard main hands run_with_tongs, which blocks HUP/TERM until it finishes, however teardown was entered. Closing a pane hangs up the whole process group, so the block has to reach teardown's own `docker rm` children too, which inherit it; a signal held meanwhile is only recorded, and main turns it into the exit status. A Ctrl-C keeps 130, and only the first HUP/TERM is taken. - The foreground anvil client is forwarded the signal -- including one arriving while a Ctrl-C it survived is waited out -- given 3s, then killed, and always reaped before teardown starts. Under a TTY docker's sig-proxy is off, so the signal just ends the client. - Nothing is written once a signal has been received, since after a hangup the terminal may be gone: a launch error is not reported, and no traceback escapes.
run_with_tongs recorded a session tong, the session network, a joined org network, and a shared tong's session connection only after the docker call that made it returned. A Ctrl-C, SIGHUP, or SIGTERM landing during that call -- or between its return and the record -- skipped the record, so teardown missed a container or network the daemon may already have created. Each is now recorded first; teardown's docker calls are best-effort and do nothing for one that never came to exist. Recording first also covers a session tong whose `docker run -d` is cut short, without removing anything inside _start_one_tong: doing it there would also remove a concurrent launch's live shared tong when `docker run` fails on its name. A shared tong cut short is left as the daemon had it: the next launch reuses it if it is running with a matching config hash, and recreates it if it is stopped or missing.
The teardown guard blocked SIGHUP and SIGTERM so teardown's `docker rm` children would inherit the block, but the docker CLI is a Go program and the Go runtime clears an inherited signal mask at startup. A hangup that reached the process group mid-teardown could still kill the docker client and leave its tong behind; the tests passed only because their stand-in child was Python. The guard now sets both signals to SIG_IGN for the teardown instead. Go keeps an inherited SIGHUP ignore, so a later hangup can no longer cut a `docker rm` short. It does not keep SIGTERM ignored, so a group SIGTERM can still reach one; that gap is narrower, since a hangup is what closing a pane sends. An ignored signal is not recorded, so one arriving mid-teardown no longer changes the exit status or suppresses a launch error report: the run's own outcome stands. A signal at the guard's edges is still only recorded, never raised.
The hangup story was told in four places -- the cli module docstring, the package docstring, run_with_tongs, and the docs -- and the docstrings this branch added restated what the code beside them already says. Each is cut to the contract it owns: the module docstrings name the behavior, `_wait_foreground` keeps only what forwarding and reaping guarantee, and errors.py says in one sentence why its two classes live there. The docs now also name Ctrl-\ (SIGQUIT), which is not trapped, among the ways to skip teardown.
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.
Closing the terminal or tmux pane a session runs in sends the launcher SIGHUP. Nothing handled it, so the process died without running
run_with_tongs' teardown and the session tongs kept running, including socket-holding ones with host-level power. A plainkill(SIGTERM) leaked them the same way; only Ctrl-C was handled.nohup) stays ignored.docker rmchildren.Tests include real-signal subprocess tests: SIGHUP and SIGTERM to the launcher and to its process group mid-teardown, plus Ctrl-C/TERM interleavings against a real child.