From c6e23501181f867cb766d1f88f2dabe9612e6e82 Mon Sep 17 00:00:00 2001 From: mkultraWasHere Date: Tue, 4 Aug 2026 16:57:25 -0400 Subject: [PATCH 1/5] fix(cli): reap Azure Bastion tunnel process tree on exit/interrupt `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 --- cli/cmd/root.go | 12 +++- cli/cmd/validate.go | 12 +++- cli/internal/azure/provision_tunnel.go | 47 ++++++++++--- cli/internal/azure/provision_tunnel_test.go | 74 +++++++++++++++++++++ 4 files changed, 134 insertions(+), 11 deletions(-) create mode 100644 cli/internal/azure/provision_tunnel_test.go diff --git a/cli/cmd/root.go b/cli/cmd/root.go index ac8a3c05..dfd137c1 100644 --- a/cli/cmd/root.go +++ b/cli/cmd/root.go @@ -1,8 +1,11 @@ package cmd import ( + "context" "fmt" "os" + "os/signal" + "syscall" "github.com/dreadnode/dreadgoad/internal/config" "github.com/dreadnode/dreadgoad/internal/logging" @@ -52,8 +55,15 @@ func SetVersionInfo(version, commit, date string) { // Execute runs the root cobra command and returns any error encountered. // It is the entry point called from main. +// +// The command runs under a signal-aware context: Ctrl+C (SIGINT) or SIGTERM +// cancels ctx instead of hard-killing the process, so ctx-aware commands unwind +// and run their deferred cleanup (e.g. tearing down Bastion tunnels) rather than +// leaking child processes. A second signal force-quits. func Execute() error { - if err := rootCmd.Execute(); err != nil { + ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + defer stop() + if err := rootCmd.ExecuteContext(ctx); err != nil { fmt.Fprintln(os.Stderr, err) return err } diff --git a/cli/cmd/validate.go b/cli/cmd/validate.go index 131559f8..f3b467e1 100644 --- a/cli/cmd/validate.go +++ b/cli/cmd/validate.go @@ -117,7 +117,10 @@ func makeRunChecks(v *validate.Validator, p provider.Provider, quick bool) func( } func runValidate(cmd *cobra.Command, args []string) error { - ctx := context.Background() + // Signal-aware context from the root: Ctrl+C/SIGTERM cancels ctx so this + // function unwinds and the deferred Drain below runs (tearing down the + // Bastion tunnel) instead of the process dying with the tunnel orphaned. + ctx := cmd.Context() opts, err := validateOptsFromFlags(cmd) if err != nil { @@ -133,6 +136,13 @@ func runValidate(cmd *cobra.Command, args []string) error { return err } + // Guarantee provider teardown (Bastion tunnel + cached clients) on every + // exit path — normal return, error, or interrupt. Drain is idempotent, so + // the terminal Drain inside makeRunChecks on the happy path is harmless. + if d, ok := infra.Provider.(provider.Drainer); ok { + defer d.Drain() + } + useTUI := !opts.plain && term.IsTerminal(int(os.Stdout.Fd())) if opts.pollInterval > 0 && !useTUI { fmt.Fprintf(os.Stderr, "Warning: --poll is ignored without the live dashboard (TTY/--plain)\n") diff --git a/cli/internal/azure/provision_tunnel.go b/cli/internal/azure/provision_tunnel.go index fae8da86..8b1d607c 100644 --- a/cli/internal/azure/provision_tunnel.go +++ b/cli/internal/azure/provision_tunnel.go @@ -9,6 +9,7 @@ import ( "path/filepath" "strconv" "strings" + "syscall" "time" "github.com/dreadnode/dreadgoad/internal/ludus" @@ -35,15 +36,33 @@ func (t *ProvisionTunnel) SOCKSAddr() string { } // Close terminates the SOCKS5 listener, the underlying SSH connection to the -// controller, and the spawned `az network bastion tunnel` subprocess. +// controller, and the spawned `az network bastion tunnel` subprocess tree. func (t *ProvisionTunnel) Close() { if t.socks != nil { t.socks.Close() } - if t.bastionCmd != nil && t.bastionCmd.Process != nil { - _ = t.bastionCmd.Process.Kill() - _ = t.bastionCmd.Wait() + killBastionTunnel(t.bastionCmd) +} + +// killBastionTunnel reaps the whole `az network bastion tunnel` process tree. +// The `az` entry point is a shell wrapper that *spawns* a `python -m azure.cli` +// child, so killing only cmd.Process (the wrapper) leaves that child running — +// it reparents to init/launchd and the Bastion tunnel leaks. We start the +// command in its own process group (Setpgid) and signal the whole group here. +func killBastionTunnel(cmd *exec.Cmd) { + if cmd == nil || cmd.Process == nil { + return + } + pid := cmd.Process.Pid + if pgid, err := syscall.Getpgid(pid); err == nil { + // Negative pid targets the entire process group (wrapper + python). + _ = syscall.Kill(-pgid, syscall.SIGTERM) + time.Sleep(500 * time.Millisecond) + _ = syscall.Kill(-pgid, syscall.SIGKILL) + } else { + _ = cmd.Process.Kill() } + _ = cmd.Wait() } // StartProvisionTunnel discovers the in-VNet controller, opens a Bastion port @@ -73,7 +92,9 @@ func StartProvisionTunnel(ctx context.Context, c *Client, env string) (*Provisio return nil, fmt.Errorf("controller ephemeral key not found at expected path; was 'infra apply' run?") } - cmd := exec.Command("az", "network", "bastion", "tunnel", + // exec.CommandContext so a cancelled ctx (Ctrl+C / SIGTERM propagated to a + // signal-aware root context) kills the tunnel even if Close() is skipped. + cmd := exec.CommandContext(ctx, "az", "network", "bastion", "tunnel", "--name", bastion.Name, "--resource-group", bastion.ResourceGroup, "--target-resource-id", controller.ID, @@ -81,13 +102,22 @@ func StartProvisionTunnel(ctx context.Context, c *Client, env string) (*Provisio "--port", strconv.Itoa(localPort)) cmd.Stdout = os.Stderr cmd.Stderr = os.Stderr + // Own process group so Close() (and ctx-cancel) can reap the wrapper *and* + // its python child as one unit — see killBastionTunnel. + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + // Kill the whole group on ctx-cancel, not just the wrapper process. + cmd.Cancel = func() error { + if pgid, err := syscall.Getpgid(cmd.Process.Pid); err == nil { + return syscall.Kill(-pgid, syscall.SIGKILL) + } + return cmd.Process.Kill() + } if err := cmd.Start(); err != nil { return nil, fmt.Errorf("start bastion tunnel: %w", err) } if err := waitForLocalPort(ctx, localPort, 60*time.Second); err != nil { - _ = cmd.Process.Kill() - _ = cmd.Wait() + killBastionTunnel(cmd) return nil, fmt.Errorf("bastion tunnel never came up on :%d: %w", localPort, err) } @@ -101,8 +131,7 @@ func StartProvisionTunnel(ctx context.Context, c *Client, env string) (*Provisio } socks, err := ludus.StartSOCKSTunnel(sshCfg) if err != nil { - _ = cmd.Process.Kill() - _ = cmd.Wait() + killBastionTunnel(cmd) return nil, fmt.Errorf("start SOCKS5 over controller: %w", err) } diff --git a/cli/internal/azure/provision_tunnel_test.go b/cli/internal/azure/provision_tunnel_test.go new file mode 100644 index 00000000..3c62cf40 --- /dev/null +++ b/cli/internal/azure/provision_tunnel_test.go @@ -0,0 +1,74 @@ +//go:build !windows + +package azure + +import ( + "os/exec" + "strconv" + "strings" + "syscall" + "testing" + "time" +) + +// processAlive reports whether pid still exists (signal 0 probes without +// delivering). A reaped process yields ESRCH. +func processAlive(pid int) bool { + return syscall.Kill(pid, 0) == nil +} + +// TestKillBastionTunnelReapsChildTree is the regression guard for the tunnel +// leak: the real `az network bastion tunnel` is a shell wrapper that spawns a +// python child, so killing only the wrapper leaves the child (and its tunnel) +// running. We reproduce that topology with `sh` (wrapper) spawning a +// backgrounded `sleep` (child), then assert killBastionTunnel reaps BOTH by +// signalling the whole process group. +func TestKillBastionTunnelReapsChildTree(t *testing.T) { + // sh backgrounds a long sleep (the "python child"), prints its PID, then + // waits — mirroring a wrapper that outlives nothing of its own but holds a + // child that must die with it. + cmd := exec.Command("sh", "-c", "sleep 120 & echo $!; wait") + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + + stdout, err := cmd.StdoutPipe() + if err != nil { + t.Fatalf("stdout pipe: %v", err) + } + if err := cmd.Start(); err != nil { + t.Fatalf("start: %v", err) + } + + // Read the grandchild (sleep) PID the wrapper printed. + buf := make([]byte, 64) + n, err := stdout.Read(buf) + if err != nil { + t.Fatalf("read child pid: %v", err) + } + childPID, err := strconv.Atoi(strings.TrimSpace(string(buf[:n]))) + if err != nil { + t.Fatalf("parse child pid %q: %v", string(buf[:n]), err) + } + + if !processAlive(childPID) { + t.Fatalf("precondition failed: child %d not alive after start", childPID) + } + + killBastionTunnel(cmd) + + // The grandchild reparents to init and is reaped shortly after SIGKILL. + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + if !processAlive(childPID) { + return // reaped — the leak is fixed + } + time.Sleep(50 * time.Millisecond) + } + t.Fatalf("child process %d survived killBastionTunnel — tunnel would leak", childPID) +} + +// TestKillBastionTunnelNilSafe guards the early-error paths that may call +// Close() before the command was started. +func TestKillBastionTunnelNilSafe(t *testing.T) { + killBastionTunnel(nil) + killBastionTunnel(&exec.Cmd{}) // Process == nil +} From 9239f223892eee5c589f02958717cab88140c840 Mon Sep 17 00:00:00 2001 From: mkultraWasHere Date: Tue, 4 Aug 2026 17:30:03 -0400 Subject: [PATCH 2/5] fix(cli): guard group-kill on pgid and honor a second interrupt 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) --- cli/cmd/root.go | 15 ++++++ cli/internal/azure/provision_tunnel.go | 12 +++-- cli/internal/azure/provision_tunnel_test.go | 52 +++++++++++++++++++++ 3 files changed, 76 insertions(+), 3 deletions(-) diff --git a/cli/cmd/root.go b/cli/cmd/root.go index dfd137c1..4dba71a0 100644 --- a/cli/cmd/root.go +++ b/cli/cmd/root.go @@ -63,6 +63,21 @@ func SetVersionInfo(version, commit, date string) { func Execute() error { ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) defer stop() + + // NotifyContext only cancels ctx; it leaves the handler installed and drops + // every later signal on the floor, so a teardown that hangs would trap the + // user with no way out but SIGKILL. Watch the signals separately and hard + // exit on the second one. + forceQuit := make(chan os.Signal, 2) + signal.Notify(forceQuit, os.Interrupt, syscall.SIGTERM) + defer signal.Stop(forceQuit) + go func() { + <-forceQuit // first: ctx cancellation above drives the graceful unwind + <-forceQuit // second: caller is done waiting for cleanup + fmt.Fprintln(os.Stderr, "\ninterrupted again — exiting now; Bastion tunnels may be left running") + os.Exit(130) + }() + if err := rootCmd.ExecuteContext(ctx); err != nil { fmt.Fprintln(os.Stderr, err) return err diff --git a/cli/internal/azure/provision_tunnel.go b/cli/internal/azure/provision_tunnel.go index 8b1d607c..389c8bea 100644 --- a/cli/internal/azure/provision_tunnel.go +++ b/cli/internal/azure/provision_tunnel.go @@ -54,7 +54,10 @@ func killBastionTunnel(cmd *exec.Cmd) { return } pid := cmd.Process.Pid - if pgid, err := syscall.Getpgid(pid); err == nil { + // pgid == pid proves Setpgid took effect. Without that check, a cmd started + // without SysProcAttr.Setpgid reports *our* group, and the negative-pid kill + // below would take down dreadgoad itself along with its foreground group. + if pgid, err := syscall.Getpgid(pid); err == nil && pgid == pid { // Negative pid targets the entire process group (wrapper + python). _ = syscall.Kill(-pgid, syscall.SIGTERM) time.Sleep(500 * time.Millisecond) @@ -105,9 +108,12 @@ func StartProvisionTunnel(ctx context.Context, c *Client, env string) (*Provisio // Own process group so Close() (and ctx-cancel) can reap the wrapper *and* // its python child as one unit — see killBastionTunnel. cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} - // Kill the whole group on ctx-cancel, not just the wrapper process. + // Kill the whole group on ctx-cancel, not just the wrapper process. Same + // pgid == pid guard as killBastionTunnel: never negative-kill a group we + // haven't confirmed belongs to the child. cmd.Cancel = func() error { - if pgid, err := syscall.Getpgid(cmd.Process.Pid); err == nil { + pid := cmd.Process.Pid + if pgid, err := syscall.Getpgid(pid); err == nil && pgid == pid { return syscall.Kill(-pgid, syscall.SIGKILL) } return cmd.Process.Kill() diff --git a/cli/internal/azure/provision_tunnel_test.go b/cli/internal/azure/provision_tunnel_test.go index 3c62cf40..14c603e2 100644 --- a/cli/internal/azure/provision_tunnel_test.go +++ b/cli/internal/azure/provision_tunnel_test.go @@ -3,6 +3,7 @@ package azure import ( + "os" "os/exec" "strconv" "strings" @@ -72,3 +73,54 @@ func TestKillBastionTunnelNilSafe(t *testing.T) { killBastionTunnel(nil) killBastionTunnel(&exec.Cmd{}) // Process == nil } + +// pgidGuardEnv re-enters this test binary as a subprocess for the guard check +// below. A regression there SIGKILLs the caller's whole process group, so the +// dangerous half runs isolated in its own group rather than taking down +// `go test` (and the developer's shell) with it. +const pgidGuardEnv = "DREADGOAD_TEST_PGID_GUARD_CHILD" + +// pgidGuardOK is the exit code the child reports when it survived the kill. +const pgidGuardOK = 7 + +// TestKillBastionTunnelSpareOwnProcessGroup pins the `pgid == pid` guard in +// killBastionTunnel. Given a command started WITHOUT SysProcAttr.Setpgid, +// syscall.Getpgid returns the *caller's* group — so an unguarded +// kill(-pgid, SIGKILL) would take down dreadgoad itself. The guard must detect +// that and fall back to killing only the single process. +func TestKillBastionTunnelSpareOwnProcessGroup(t *testing.T) { + if os.Getenv(pgidGuardEnv) == "1" { + // Detach into our own process group so a regression's group-kill is + // contained to this subprocess. + if err := syscall.Setpgid(0, 0); err != nil { + os.Exit(3) + } + // No Setpgid here: the child inherits OUR pgid, which is exactly the + // condition the guard exists to catch. + victim := exec.Command("sleep", "120") + if err := victim.Start(); err != nil { + os.Exit(4) + } + killBastionTunnel(victim) + // Still executing => the guard held and we did not signal our own group. + os.Exit(pgidGuardOK) + } + + exe, err := os.Executable() + if err != nil { + t.Fatalf("locate test binary: %v", err) + } + cmd := exec.Command(exe, "-test.run=TestKillBastionTunnelSpareOwnProcessGroup") + cmd.Env = append(os.Environ(), pgidGuardEnv+"=1") + + err = cmd.Run() + code := cmd.ProcessState.ExitCode() + if code == pgidGuardOK { + return // guard held + } + if code == -1 { + t.Fatalf("subprocess was killed by a signal (%v) — killBastionTunnel "+ + "signalled its own process group; the pgid == pid guard is missing", cmd.ProcessState) + } + t.Fatalf("subprocess exited %d (err=%v), want %d", code, err, pgidGuardOK) +} From 69c2d5389b9d2118c9018f79467bc3f8709a9f1c Mon Sep 17 00:00:00 2001 From: mkultraWasHere Date: Tue, 4 Aug 2026 17:34:30 -0400 Subject: [PATCH 3/5] perf(cli): poll for tunnel group exit instead of a fixed 500ms sleep 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) --- cli/internal/azure/provision_tunnel.go | 55 ++++++++++++++++++--- cli/internal/azure/provision_tunnel_test.go | 14 +++--- 2 files changed, 56 insertions(+), 13 deletions(-) diff --git a/cli/internal/azure/provision_tunnel.go b/cli/internal/azure/provision_tunnel.go index 389c8bea..0cc8b127 100644 --- a/cli/internal/azure/provision_tunnel.go +++ b/cli/internal/azure/provision_tunnel.go @@ -49,23 +49,64 @@ func (t *ProvisionTunnel) Close() { // child, so killing only cmd.Process (the wrapper) leaves that child running — // it reparents to init/launchd and the Bastion tunnel leaks. We start the // command in its own process group (Setpgid) and signal the whole group here. +// killGracePeriod is how long the tunnel gets to honor SIGTERM before the +// group is SIGKILLed. +const killGracePeriod = 500 * time.Millisecond + func killBastionTunnel(cmd *exec.Cmd) { if cmd == nil || cmd.Process == nil { return } pid := cmd.Process.Pid + + // Reap in the background so the wrapper leaves zombie state the moment it + // dies. This is what makes the polling below work at all: an unreaped + // zombie still answers kill(pid, 0), so waiting on the group without + // concurrently reaping would never observe the exit. + reaped := make(chan struct{}) + go func() { + _ = cmd.Wait() + close(reaped) + }() + // pgid == pid proves Setpgid took effect. Without that check, a cmd started // without SysProcAttr.Setpgid reports *our* group, and the negative-pid kill // below would take down dreadgoad itself along with its foreground group. - if pgid, err := syscall.Getpgid(pid); err == nil && pgid == pid { - // Negative pid targets the entire process group (wrapper + python). - _ = syscall.Kill(-pgid, syscall.SIGTERM) - time.Sleep(500 * time.Millisecond) - _ = syscall.Kill(-pgid, syscall.SIGKILL) - } else { + pgid, err := syscall.Getpgid(pid) + if err != nil || pgid != pid { _ = cmd.Process.Kill() + <-reaped + return + } + + // Negative pid targets the entire process group (wrapper + python). + _ = syscall.Kill(-pgid, syscall.SIGTERM) + if !awaitGroupExit(pgid, reaped, killGracePeriod) { + _ = syscall.Kill(-pgid, syscall.SIGKILL) + } + <-reaped +} + +// awaitGroupExit polls until no member of pgid remains, or timeout elapses. +// Returns true if the group went away on its own, letting the caller skip the +// SIGKILL escalation — a tunnel that honors SIGTERM promptly costs a few +// milliseconds here instead of the full grace period. +func awaitGroupExit(pgid int, reaped <-chan struct{}, timeout time.Duration) bool { + deadline := time.Now().Add(timeout) + for { + select { + case <-reaped: + // Wrapper is reaped, so a surviving group means real stragglers. + if syscall.Kill(-pgid, 0) == syscall.ESRCH { + return true + } + default: + } + if !time.Now().Before(deadline) { + return false + } + time.Sleep(10 * time.Millisecond) } - _ = cmd.Wait() } // StartProvisionTunnel discovers the in-VNet controller, opens a Bastion port diff --git a/cli/internal/azure/provision_tunnel_test.go b/cli/internal/azure/provision_tunnel_test.go index 14c603e2..5c14afe7 100644 --- a/cli/internal/azure/provision_tunnel_test.go +++ b/cli/internal/azure/provision_tunnel_test.go @@ -3,6 +3,7 @@ package azure import ( + "bufio" "os" "os/exec" "strconv" @@ -39,15 +40,16 @@ func TestKillBastionTunnelReapsChildTree(t *testing.T) { t.Fatalf("start: %v", err) } - // Read the grandchild (sleep) PID the wrapper printed. - buf := make([]byte, 64) - n, err := stdout.Read(buf) + // Read the grandchild (sleep) PID the wrapper printed. Read through the + // newline rather than trusting one Read to return the whole line, so a + // split write can't turn into a flaky parse. + line, err := bufio.NewReader(stdout).ReadString('\n') if err != nil { - t.Fatalf("read child pid: %v", err) + t.Fatalf("read child pid (got %q): %v", line, err) } - childPID, err := strconv.Atoi(strings.TrimSpace(string(buf[:n]))) + childPID, err := strconv.Atoi(strings.TrimSpace(line)) if err != nil { - t.Fatalf("parse child pid %q: %v", string(buf[:n]), err) + t.Fatalf("parse child pid %q: %v", line, err) } if !processAlive(childPID) { From 89af74fa940707c8b80c1d63928600243eee363d Mon Sep 17 00:00:00 2001 From: mkultraWasHere Date: Tue, 4 Aug 2026 18:04:48 -0400 Subject: [PATCH 4/5] fix(cli): tear down the Bastion tunnel when provision is interrupted 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) --- cli/cmd/lab_reset.go | 5 ++++- cli/cmd/provision.go | 5 ++++- cli/internal/ansible/retry.go | 7 +++++++ cli/internal/ansible/retry_test.go | 32 ++++++++++++++++++++++++++++++ 4 files changed, 47 insertions(+), 2 deletions(-) diff --git a/cli/cmd/lab_reset.go b/cli/cmd/lab_reset.go index 58662997..cbd16f5e 100644 --- a/cli/cmd/lab_reset.go +++ b/cli/cmd/lab_reset.go @@ -487,7 +487,10 @@ func runLabReset(cmd *cobra.Command, args []string) error { if err != nil { return err } - ctx := context.Background() + // Signal-aware context from the root: `lab reset` shares provisionPlaybooks + // with `provision`, so it opens the same Bastion tunnel and needs the same + // cancellation path to tear it down on interrupt. + ctx := cmd.Context() skipPurge, _ := cmd.Flags().GetBool("skip-purge") skipProvision, _ := cmd.Flags().GetBool("skip-provision") diff --git a/cli/cmd/provision.go b/cli/cmd/provision.go index cb42216a..fe643a99 100644 --- a/cli/cmd/provision.go +++ b/cli/cmd/provision.go @@ -327,7 +327,10 @@ func runProvision(cmd *cobra.Command, args []string) error { if err != nil { return err } - ctx := context.Background() + // Signal-aware context from the root: Ctrl+C/SIGTERM cancels ctx so + // provisionPlaybooks unwinds and its deferred socksTunnel.Close() runs, + // instead of the process dying with the Bastion tunnel orphaned. + ctx := cmd.Context() playsFlag, _ := cmd.Flags().GetString("plays") fromFlag, _ := cmd.Flags().GetString("from") diff --git a/cli/internal/ansible/retry.go b/cli/internal/ansible/retry.go index 472c0126..a15b6f86 100644 --- a/cli/internal/ansible/retry.go +++ b/cli/internal/ansible/retry.go @@ -58,6 +58,13 @@ func RunPlaybookWithRetry(ctx context.Context, opts RetryOptions) error { retryForks := 2 // limit SSM concurrency to avoid session saturation for attempt := range opts.MaxRetries { + // An interrupt reaches ansible-playbook directly (shared foreground + // process group), so the attempt comes back as an ordinary failure and + // the error-strategy path below would log a retry it cannot perform. + // Bail on cancellation before interpreting any result. + if err := ctx.Err(); err != nil { + return err + } if attempt > 0 { log.Info("retry attempt", "attempt", attempt, "playbook", opts.Playbook) log.Info("waiting before retry", "delay", opts.RetryDelay) diff --git a/cli/internal/ansible/retry_test.go b/cli/internal/ansible/retry_test.go index c6481105..596dae5f 100644 --- a/cli/internal/ansible/retry_test.go +++ b/cli/internal/ansible/retry_test.go @@ -1,9 +1,41 @@ package ansible import ( + "bytes" + "context" + "errors" + "log/slog" + "strings" "testing" ) +// TestRunPlaybookWithRetryStopsOnCancelledContext pins the cancellation check +// at the top of the retry loop. Wiring `provision` to the root's signal-aware +// context means an interrupt now surfaces as an ordinary playbook failure, and +// without the check the loop interprets that failure and announces retries it +// cannot perform. The returned error is context.Canceled either way, so this +// asserts on the log: no attempt may be started once ctx is done. +func TestRunPlaybookWithRetryStopsOnCancelledContext(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + var logBuf bytes.Buffer + err := RunPlaybookWithRetry(ctx, RetryOptions{ + Playbook: "noop.yml", + Env: "test", + MaxRetries: 3, + Log: slog.New(slog.NewTextHandler(&logBuf, nil)), + }) + + if !errors.Is(err, context.Canceled) { + t.Fatalf("got err %v, want context.Canceled", err) + } + if got := logBuf.String(); strings.Contains(got, "starting playbook") || + strings.Contains(got, "retrying with") { + t.Fatalf("cancelled context still drove a retry attempt; log was:\n%s", got) + } +} + // TestBuildRetryLimit covers all branches of buildRetryLimit. func TestBuildRetryLimit(t *testing.T) { tests := []struct { From 09515211a246c77d226f1a0c738feca79c34e341 Mon Sep 17 00:00:00 2001 From: mkultraWasHere Date: Tue, 4 Aug 2026 19:04:30 -0400 Subject: [PATCH 5/5] fix(cli): make ProvisionTunnel.Close idempotent under concurrency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- cli/internal/azure/provision_tunnel.go | 19 ++++++++++--- cli/internal/azure/provision_tunnel_test.go | 30 +++++++++++++++++++++ 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/cli/internal/azure/provision_tunnel.go b/cli/internal/azure/provision_tunnel.go index 0cc8b127..7bcd50ef 100644 --- a/cli/internal/azure/provision_tunnel.go +++ b/cli/internal/azure/provision_tunnel.go @@ -9,6 +9,7 @@ import ( "path/filepath" "strconv" "strings" + "sync" "syscall" "time" @@ -23,6 +24,7 @@ type ProvisionTunnel struct { socks *ludus.SOCKSTunnel bastionCmd *exec.Cmd localPort int + closeOnce sync.Once } // ProxyURL returns the SOCKS5 proxy URL Ansible's psrp connection plugin @@ -37,11 +39,20 @@ func (t *ProvisionTunnel) SOCKSAddr() string { // Close terminates the SOCKS5 listener, the underlying SSH connection to the // controller, and the spawned `az network bastion tunnel` subprocess tree. +// +// Teardown runs exactly once even if Close is called concurrently. That is not +// cosmetic: killBastionTunnel reaps via cmd.Wait, and two Wait calls racing on +// one exec.Cmd is a data race the detector flags. Callers reach Close through +// several paths (winrmRunner.close, the deferred Drain in `validate`, the +// deferred socksTunnel.Close in `provision`), so the guarantee lives here +// rather than depending on every caller staying serialized. func (t *ProvisionTunnel) Close() { - if t.socks != nil { - t.socks.Close() - } - killBastionTunnel(t.bastionCmd) + t.closeOnce.Do(func() { + if t.socks != nil { + t.socks.Close() + } + killBastionTunnel(t.bastionCmd) + }) } // killBastionTunnel reaps the whole `az network bastion tunnel` process tree. diff --git a/cli/internal/azure/provision_tunnel_test.go b/cli/internal/azure/provision_tunnel_test.go index 5c14afe7..ab2ef021 100644 --- a/cli/internal/azure/provision_tunnel_test.go +++ b/cli/internal/azure/provision_tunnel_test.go @@ -8,6 +8,7 @@ import ( "os/exec" "strconv" "strings" + "sync" "syscall" "testing" "time" @@ -76,6 +77,35 @@ func TestKillBastionTunnelNilSafe(t *testing.T) { killBastionTunnel(&exec.Cmd{}) // Process == nil } +// TestProvisionTunnelCloseIsRaceFree pins the closeOnce guard. killBastionTunnel +// reaps with cmd.Wait, and two Wait calls on one exec.Cmd is a data race — so +// concurrent Close must collapse to a single teardown. Run under -race; without +// closeOnce the detector reports a write/write race inside os/exec.(*Cmd).Wait. +func TestProvisionTunnelCloseIsRaceFree(t *testing.T) { + cmd := exec.Command("sleep", "120") + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + if err := cmd.Start(); err != nil { + t.Fatalf("start: %v", err) + } + // socks stays nil: this exercises the subprocess half, which is where the + // race lives. + tunnel := &ProvisionTunnel{bastionCmd: cmd} + + var wg sync.WaitGroup + for range 4 { + wg.Add(1) + go func() { + defer wg.Done() + tunnel.Close() + }() + } + wg.Wait() + + if processAlive(cmd.Process.Pid) { + t.Fatalf("process %d survived Close", cmd.Process.Pid) + } +} + // pgidGuardEnv re-enters this test binary as a subprocess for the guard check // below. A regression there SIGKILLs the caller's whole process group, so the // dangerous half runs isolated in its own group rather than taking down