Reap Azure Bastion tunnel process tree on exit/interrupt - #411
Merged
Conversation
`az network bastion tunnel` is a shell wrapper that spawns a python child, so ProvisionTunnel.Close() killing only cmd.Process orphaned the python child and leaked the tunnel. On top of that, `validate` ran under context.Background() and only drained at the end, so Ctrl+C/SIGTERM killed the process before any teardown ran, orphaning the whole tree. - Start the tunnel in its own process group (Setpgid) via CommandContext and kill the whole group in Close() and the early-error paths. - Make the root command context signal-aware (SIGINT/SIGTERM) so ctx-aware commands unwind and run deferred cleanup instead of hard-exiting. - validate uses cmd.Context() and defers the provider Drain on every path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses leaked Azure Bastion tunnel subprocesses by ensuring the az network bastion tunnel process tree is reaped on shutdown, and by wiring CLI execution to a signal-aware context so deferred cleanup runs on interrupt/termination.
Changes:
- Rework Bastion tunnel lifecycle management to kill the entire process group (wrapper + spawned child).
- Run
validateunder the root command’s context and defer provider teardown so tunnels/clients are drained on all exit paths. - Add Unix-only tests covering child-tree reaping behavior and nil-safety.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cli/internal/azure/provision_tunnel.go | Kill Bastion tunnel process groups to avoid orphaned child processes. |
| cli/internal/azure/provision_tunnel_test.go | Add regression tests to ensure process-tree reaping and nil-safety. |
| cli/cmd/validate.go | Use signal-aware command context and defer provider draining for cleanup on interrupt. |
| cli/cmd/root.go | Execute root command under a signal-aware context. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Follow-up hardening on the tunnel teardown added in c6e2350. killBastionTunnel and cmd.Cancel both derived a process group via syscall.Getpgid and then kill(-pgid). If the command was ever started without SysProcAttr.Setpgid, Getpgid reports the *caller's* group, so that negative-pid kill would take down dreadgoad itself along with its foreground process group. Both sites now require pgid == pid before signalling the group and otherwise fall back to a single-process kill. Not reachable from the current call sites, which all set Setpgid, but the helper takes an arbitrary *exec.Cmd and the failure mode is severe. signal.NotifyContext only cancels ctx; it leaves its handler installed and silently drops later signals, so the "a second signal force-quits" behavior the root command documented did not exist and a hung teardown would trap the user. Watch the signals on a separate channel and exit 130 on the second one. Covered by TestKillBastionTunnelSpareOwnProcessGroup, which re-execs the test binary in its own process group so a regression fails the test rather than killing `go test` and the developer's shell. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses review feedback on #411. killBastionTunnel slept the full grace period after SIGTERM on every Close(), so a tunnel that shut down instantly still cost 500ms on the command's exit path. Poll for the group to drain and escalate to SIGKILL only if it outlives the deadline. The reason this needs a concurrent reap: an unreaped child stays a zombie and keeps answering kill(pid, 0), so polling the group while still holding the wait would never observe the exit. cmd.Wait now runs in a goroutine and the poll keys off that. Also read the child PID through the newline in the process-tree test rather than trusting a single Read to return the whole line. TestKillBastionTunnelReapsChildTree drops from ~0.51s to ~0.02s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
provisionPlaybooks already deferred socksTunnel.Close(), but both entry points ran under context.Background(), so Ctrl+C/SIGTERM killed the process before any defer executed and the `az network bastion tunnel` tree survived. Same leak c6e2350 fixed for `validate`, on the command that holds the tunnel longest. runProvision and runLabReset now take cmd.Context(). Both reach the tunnel through the shared provisionPlaybooks. Cancellation changes what a playbook failure means: an interrupt reaches ansible-playbook directly through the shared foreground process group, so the attempt returns as an ordinary failure and the retry loop would classify it and announce a retry it cannot perform. RunPlaybookWithRetry now checks ctx at the top of each attempt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mkultraWasHere
marked this pull request as ready for review
August 4, 2026 22:36
Found during a security pass over this branch. killBastionTunnel reaps the subprocess with cmd.Wait, so two Close calls racing on one exec.Cmd is a genuine data race — reproduced in isolation, the detector reports a write/write inside os/exec.(*Cmd).Wait. Not reachable today: the double Drain in `validate` is serialized because RunTUI waits on runDone before returning. But winrmRunner.close reads and nils r.tunnel outside its mutex while documenting itself as safe to call multiple times, so the only thing preventing the race is caller ordering that nothing enforces. Put the guarantee in Close via sync.Once instead of relying on every caller staying serialized. TestProvisionTunnelCloseIsRaceFree fails under -race without the guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
dreadgoadcommands that reach Azure hosts (validate,provision,health-check, …) leakedaz network bastion tunnelsubprocesses, accumulating orphaned tunnels (and open Bastion sessions) across runs.Fixed
az network bastion tunnelsubprocesses no longer leak on normal exit —ProvisionTunnel.Close()now kills the whole process group, reaping theazshell wrapper and itspythonchild (previously only the wrapper was killed, orphaning the child and its tunnel).validateno longer orphans its tunnel on Ctrl+C / SIGTERM — the root command context is now signal-aware, so the command unwinds and runs deferred cleanup instead of the process dying with the tunnel still up.Changed
Setpgid) viaexec.CommandContext, so a cancelled context also tears down the whole group.validateruns under the signal-aware context (cmd.Context()) anddefers the providerDrain()on every exit path (normal, error, interrupt);Drainis idempotent.Notes
internal/ansible/runner.go.provision_tunnel_test.gocovers child-tree reaping and nil-safety; verified end-to-end against a live range (interrupt mid-validate→ zero orphaned tunnels). Signal handling is Unix-only (test is//go:build !windows).