Repository: tetratelabs/getenvoy
Pull request #180 (2021-04-16 02:43:33)

Cleans signal handling and reduces complexity of "getenvoy run"
Before, we had complex signal handling in both "getenvoy run" and also
normal command invocation. This solves the first part by reducing the
amount of gears used to run the process, and also moving to
NotifyContext for signal interception.

See #172 for more information on NotifyContext, though this change
doesn't remove the osutil functions, yet. That will be a different PR
as it is a different branch of code.

codefromthecrypt: looks like I'll be doing the log file close refactor now after all as it fails in linux otherwise :D
codefromthecrypt: once this is in, we can move most of the functions that await run completion to e2e, possibly in parallel depending on if #178 is allowed to merge or not (as it is the only thing that requires static ports)

Patch:
diff --git a/pkg/binary/envoy/debug/log.go b/pkg/binary/envoy/debug/log.go
index 59de9f4..d07ec14 100644
--- a/pkg/binary/envoy/debug/log.go
+++ b/pkg/binary/envoy/debug/log.go
@@ -19,7 +19,6 @@ import (
 	"io"
 	"os"
 	"path/filepath"
-	"time"
 
 	"github.com/tetratelabs/log"
 
@@ -29,8 +28,10 @@ import (
 
 // EnableEnvoyLogCollection is a preset option that registers collection of Envoy access logs and stderr
 func EnableEnvoyLogCollection(r *envoy.Runtime) {
-	if err := os.MkdirAll(filepath.Join(r.DebugStore(), "logs"), os.ModePerm); err != nil {
-		log.Errorf("unable to create directory to write logs to, no logs will be captured: %v", err)
+	logsDir := filepath.Join(r.DebugStore(), "logs")
+	err := os.MkdirAll(logsDir, os.ModePerm)
+	if err != nil {
+		log.Errorf("unable to create logs directory %q, so no logs will be captured: %v", logsDir, err)
 		return
 	}
 	r.RegisterPreStart(captureStdout)
@@ -42,13 +43,15 @@ func captureStdout(r binary.Runner) error {
 	if err != nil {
 		return err
 	}
+	r.RegisterPostTermination(func(runner binary.Runner) error {
+		return f.Close()
+	})
 	r.SetStdout(func(w io.Writer) io.Writer {
 		if w == nil {
 			return f
 		}
 		return io.MultiWriter(w, f)
 	})
-	go capture(r, f)
 	return nil
 }
 
@@ -57,13 +60,15 @@ func captureStderr(r binary.Runner) error {
 	if err != nil {
 		return err
 	}
+	r.RegisterPostTermination(func(runner binary.Runner) error {
+		return f.Close()
+	})
 	r.SetStderr(func(w io.Writer) io.Writer {
 		if w == nil {
 			return f
 		}
 		return io.MultiWriter(w, f)
 	})
-	go capture(r, f)
 	return nil
 }
 
@@ -75,22 +80,3 @@ func createLogFile(path string) (*os.File, error) {
 	}
 	return f, nil
 }
-
-func capture(r binary.Runner, file io.Closer) {
-	r.RegisterWait(1)
-	wait(r, binary.StatusTerminated)
-	if err := file.Close(); err != nil {
-		log.Errorf("error closing access log file: %v", err)
-	}
-	r.RegisterDone()
-}
-
-// wait blocks until the child process reaches the state passed
-// Note: It does not guarantee that it is in the specified state just that it has reached it
-func wait(r binary.Runner, state int) {
-	for r.Status() < state {
-		// This is a call to a function to allow the goroutine to be preempted for garbage collection
-		// The sleep duration is somewhat arbitrary
-		func() { time.Sleep(time.Millisecond * 100) }()
-	}
-}
diff --git a/pkg/binary/envoy/run.go b/pkg/binary/envoy/run.go
index 36ec192..8ddfad6 100644
--- a/pkg/binary/envoy/run.go
+++ b/pkg/binary/envoy/run.go
@@ -26,7 +26,6 @@ import (
 	"syscall"
 	"time"
 
-	"github.com/mholt/archiver/v3"
 	"github.com/tetratelabs/log"
 
 	"github.com/tetratelabs/getenvoy/pkg/manifest"
@@ -46,32 +45,50 @@ func (r *Runtime) RunPath(path string, args []string) error {
 		return fmt.Errorf("unable to stat %q: %v", path, err)
 	}
 
-	ctx, cancel := context.WithCancel(context.Background())
-	r.ctx = ctx
-
-	// #nosec -> users can run whatever binary they like!
-	r.cmd = exec.Command(path, args...)
-	r.cmd.Dir = r.WorkingDir
-	r.cmd.Stdout = r.IO.Out
-	r.cmd.Stderr = r.IO.Err
-	r.cmd.SysProcAttr = sysProcAttr()
+	// We can't use CommandContext even if that seems correct here. The reason is that we need to invoke preTerminate
+	// handlers, and they expect the process to still be running. For example, this allows admin API hooks.
+	cmd := exec.Command(path, args...) // #nosec -> users can run whatever binary they like!
+	cmd.Dir = r.WorkingDir
+	cmd.Stdout = r.IO.Out
+	cmd.Stderr = r.IO.Err
+	cmd.SysProcAttr = sysProcAttr()
+	r.cmd = cmd
 
 	r.handlePreStart()
 
-	go r.runEnvoy(cancel)
+	if cmd.Stdout == nil {
+		cmd.Stdout = os.Stdout
+	}
+	if cmd.Stderr == nil {
+		cmd.Stderr = os.Stderr
+	}
 
-	r.waitForTerminationSignals()
-	r.handleTermination()
-	cancel() // this should never actually do anything but lets avoid any future context leaks
+	log.Infof("Envoy command: %v", cmd.Args)
+	if err := cmd.Start(); err != nil {
+		return fmt.Errorf("unable to start Envoy process: %w", err)
+	}
+
+	waitCtx, waitCancel := context.WithCancel(context.Background())
+	sigCtx, sigCancel := signal.NotifyContext(waitCtx, syscall.SIGINT, syscall.SIGTERM)
+	defer waitCancel()
+	r.fakeInterrupt = sigCancel
+
+	go waitForExit(cmd, waitCancel) // waits in a goroutine. We may need to kill the process if a signal occurs first.
 
-	// Block until the Envoy process and termination handler are finished cleaning up
-	r.wg.Wait()
+	// Block until we receive SIGINT or are canceled because Envoy has died
+	<-sigCtx.Done()
 
-	// Tar up the debug data and clean up
-	if err := archiver.Archive([]string{r.DebugStore()}, r.DebugStore()+".tar.gz"); err != nil {
-		return fmt.Errorf("unable to archive debug store directory %v: %v", r.DebugStore(), err)
+	if cmd.ProcessState != nil {
+		log.Infof("Envoy process (PID=%d) terminated prematurely", cmd.Process.Pid)
+		return r.handlePostTermination()
 	}
-	return os.RemoveAll(r.DebugStore())
+
+	r.handleTermination()
+
+	// Block until the process is complete. This ensures file descriptors are closed.
+	<-waitCtx.Done()
+
+	return r.handlePostTermination()
 }
 
 // DebugStore returns the location at which the runtime instance persists debug data for this given instance
@@ -90,60 +107,18 @@ func (r *Runtime) SetStderr(fn func(io.Writer) io.Writer) {
 	r.cmd.Stderr = fn(r.cmd.Stderr)
 }
 
-// RegisterWait informs the runtime it needs to wait for you to complete
-// It is a wrapper around sync.WaitGroup.Add()
-func (r *Runtime) RegisterWait(delta int) {
-	r.wg.Add(delta)
-}
-
-// RegisterDone informs the runtime that you have completed
-// It is a wrapper around sync.WaitGroup.Done()
-func (r *Runtime) RegisterDone() {
-	r.wg.Done()
-}
-
 // AppendArgs appends the passed args to the child process' args
 func (r *Runtime) AppendArgs(args []string) {
 	r.cmd.Args = append(r.cmd.Args, args...)
 }
 
-func (r *Runtime) waitForTerminationSignals() {
-	signal.Notify(r.signals, syscall.SIGINT, syscall.SIGTERM)
-
-	// Block until we receive SIGINT or are canceled because Envoy has died
-	select {
-	case <-r.ctx.Done():
-		log.Infof("No Envoy processes remaining, terminating GetEnvoy process (PID=%d)", os.Getpid())
-		return
-	case <-r.signals:
-		log.Infof("GetEnvoy process (PID=%d) received SIGINT", os.Getpid())
-		return
-	}
-}
-
-func (r *Runtime) runEnvoy(cancel context.CancelFunc) {
-	if r.cmd.Stdout == nil {
-		r.cmd.Stdout = os.Stdout
-	}
-	if r.cmd.Stderr == nil {
-		r.cmd.Stderr = os.Stderr
-	}
-
-	r.wg.Add(1)
-	defer r.wg.Done()
+func waitForExit(cmd *exec.Cmd, cancel context.CancelFunc) {
 	defer cancel()
-
-	log.Infof("Envoy command: %v", r.cmd.Args)
-	if err := r.cmd.Start(); err != nil {
-		log.Errorf("Unable to start Envoy process: %v", err)
-		return
-	}
-
-	if err := r.cmd.Wait(); err != nil {
-		if r.cmd.ProcessState.ExitCode() == -1 {
-			log.Infof("Envoy process (PID=%d) terminated via %v", r.cmd.Process.Pid, err)
+	if err := cmd.Wait(); err != nil {
+		if cmd.ProcessState.ExitCode() == -1 {
+			log.Infof("Envoy process (PID=%d) terminated via %v", cmd.Process.Pid, err)
 		} else {
-			log.Infof("Envoy process (PID=%d) terminated with an error: %v", r.cmd.Process.Pid, err)
+			log.Infof("Envoy process (PID=%d) terminated with an error: %v", cmd.Process.Pid, err)
 		}
 	}
 }
diff --git a/pkg/binary/envoy/runtime.go b/pkg/binary/envoy/runtime.go
index 9a9d7b7..f577d22 100644
--- a/pkg/binary/envoy/runtime.go
+++ b/pkg/binary/envoy/runtime.go
@@ -18,10 +18,8 @@ import (
 	"context"
 	"fmt"
 	"net/http"
-	"os"
 	"os/exec"
 	"path/filepath"
-	"sync"
 
 	"github.com/tetratelabs/getenvoy/pkg/binary"
 	"github.com/tetratelabs/getenvoy/pkg/common"
@@ -58,14 +56,13 @@ func (o RuntimeOptions) AndAll(opts RuntimeOptions) RuntimeOptions {
 func NewRuntime(options ...RuntimeOption) (binary.FetchRunner, error) {
 	local := common.HomeDir
 	runtime := &Runtime{
-		Config:         NewConfig(),
-		RootDir:        local,
-		fetcher:        fetcher{local},
-		TmplDir:        filepath.Join(local, "templates"),
-		wg:             &sync.WaitGroup{},
-		signals:        make(chan os.Signal),
-		preStart:       make([]func(binary.Runner) error, 0),
-		preTermination: make([]func(binary.Runner) error, 0),
+		Config:          NewConfig(),
+		RootDir:         local,
+		fetcher:         fetcher{local},
+		TmplDir:         filepath.Join(local, "templates"),
+		preStart:        make([]func(binary.Runner) error, 0),
+		preTermination:  make([]func(binary.Runner) error, 0),
+		postTermination: make([]func(binary.Runner) error, 0),
 	}
 
 	if debugErr := runtime.initializeDebugStore(); debugErr != nil {
@@ -95,14 +92,12 @@ type Runtime struct {
 	WorkingDir string
 	IO         ioutil.StdStreams
 
-	cmd *exec.Cmd
-	ctx context.Context
-	wg  *sync.WaitGroup
+	cmd           *exec.Cmd
+	fakeInterrupt context.CancelFunc
 
-	signals chan os.Signal
-
-	preStart       []func(binary.Runner) error
-	preTermination []func(binary.Runner) error
+	preStart        []func(binary.Runner) error
+	preTermination  []func(binary.Runner) error
+	postTermination []func(binary.Runner) error
 
 	getAdminAddress func(r *Runtime) string
 	isReady         bool
@@ -171,7 +166,11 @@ func (r *Runtime) envoyReady() (int, error) {
 	}
 }
 
-// SendSignal sends a signal to the parent process
-func (r *Runtime) SendSignal(s os.Signal) {
-	r.signals <- s
+// FakeInterrupt is exposed for unit tests to pretend "getenvoy run" received an interrupt or a ctrl-c. End-to-end
+// tests should kill the getenvoy process to achieve the same.
+func (r *Runtime) FakeInterrupt() {
+	fakeInterrupt := r.fakeInterrupt
+	if fakeInterrupt != nil {
+		fakeInterrupt()
+	}
 }
diff --git a/pkg/binary/envoy/termination.go b/pkg/binary/envoy/termination.go
index 815d134..e43eee5 100644
--- a/pkg/binary/envoy/termination.go
+++ b/pkg/binary/envoy/termination.go
@@ -15,26 +15,22 @@
 package envoy
 
 import (
+	"fmt"
+	"os"
 	"syscall"
 
+	"github.com/mholt/archiver/v3"
 	"github.com/tetratelabs/log"
 
 	"github.com/tetratelabs/getenvoy/pkg/binary"
 )
 
 func (r *Runtime) handleTermination() {
-	if r.cmd.Process == nil {
-		return // Envoy hasn't started at all
-	}
-	if r.cmd.ProcessState != nil {
-		if r.cmd.ProcessState.Success() {
-			log.Infof("Envoy process (PID=%d) exited successfully", r.cmd.Process.Pid)
-			return
-		}
-		log.Infof("Envoy process (PID=%d) terminated prematurely", r.cmd.Process.Pid)
-		return
-	}
+	cmd := r.cmd
+
+	defer interrupt(cmd.Process) // Ensure the SIGINT forwards to Envoy even if a pre-termination hook panics
 
+	log.Infof("GetEnvoy process (PID=%d) received SIGINT", os.Getpid())
 	// Execute all registered preTermination functions
 	for _, f := range r.preTermination {
 		if err := f(r); err != nil {
@@ -42,9 +38,26 @@ func (r *Runtime) handleTermination() {
 		}
 	}
 
-	// Forward on the SIGINT to Envoy
-	log.Infof("Sending Envoy process (PID=%d) SIGINT", r.cmd.Process.Pid)
-	r.cmd.Process.Signal(syscall.SIGINT) //nolint
+	interrupt(cmd.Process)
+}
+
+func interrupt(p *os.Process) {
+	log.Infof("Sending Envoy process (PID=%d) SIGINT", p.Pid)
+	_ = p.Signal(syscall.SIGINT)
+}
+
+func (r *Runtime) handlePostTermination() error {
+	for _, f := range r.postTermination {
+		if err := f(r); err != nil {
+			log.Errorf("failed to handle post termination: %v", err)
+		}
+	}
+
+	// Tar up the debug data and clean up
+	if err := archiver.Archive([]string{r.DebugStore()}, r.DebugStore()+".tar.gz"); err != nil {
+		return fmt.Errorf("unable to archive debug store directory %v: %v", r.DebugStore(), err)
+	}
+	return os.RemoveAll(r.DebugStore())
 }
 
 // RegisterPreTermination registers the passed functions to be run after Envoy has started
@@ -52,3 +65,9 @@ func (r *Runtime) handleTermination() {
 func (r *Runtime) RegisterPreTermination(f ...func(binary.Runner) error) {
 	r.preTermination = append(r.preTermination, f...)
 }
+
+// RegisterPostTermination registers the passed functions to be run after Envoy has terminated
+// and just before GetEnvoy archives the debug directory.
+func (r *Runtime) RegisterPostTermination(f ...func(binary.Runner) error) {
+	r.postTermination = append(r.postTermination, f...)
+}
diff --git a/pkg/binary/interface.go b/pkg/binary/interface.go
index 0fb2a7d..e9f7e41 100644
--- a/pkg/binary/interface.go
+++ b/pkg/binary/interface.go
@@ -16,7 +16,6 @@ package binary
 
 import (
 	"io"
-	"os"
 
 	"github.com/tetratelabs/getenvoy/pkg/manifest"
 )
@@ -27,9 +26,8 @@ type Runner interface {
 	RunPath(path string, args []string) error
 	RegisterPreStart(f ...func(Runner) error)
 	RegisterPreTermination(f ...func(Runner) error)
-	RegisterWait(int)
-	RegisterDone()
-	SendSignal(signal os.Signal)
+	RegisterPostTermination(f ...func(Runner) error)
+	FakeInterrupt()
 	Status() int
 	GetPid() (int, error)
 	AppendArgs([]string)


Test patch:
diff --git a/pkg/binary/envoy/controlplane/istio_test.go b/pkg/binary/envoy/controlplane/istio_test.go
index 0008990..5bd08ff 100644
--- a/pkg/binary/envoy/controlplane/istio_test.go
+++ b/pkg/binary/envoy/controlplane/istio_test.go
@@ -60,7 +60,7 @@ func TestEnableIstioBootstrap(t *testing.T) {
 		func(r *envoy.Runtime) {
 			r.Config.Mode = envoy.ParseMode("loadbalancer")
 			// Choosing a random port doesn't help because envoy_bootstrap.json hard-codes 15020 15021 and 15090
-			// See https://github.com/istio/istio/issues/32184 for follow-up
+			// See https://github.com/istio/istio/issues/31138
 			r.Config.AdminPort = 15000
 			r.Config.XDSAddress = pilotGrpc
 			r.Config.IPAddresses = []string{"127.0.0.1"} // prevent calling controlplane.retrieveIPs() on CI hosts
@@ -72,8 +72,7 @@ func TestEnableIstioBootstrap(t *testing.T) {
 	defer os.RemoveAll(runtime.DebugStore())
 
 	envoytest.RequireRunTerminate(t, runtime, envoytest.RunKillOptions{
-		DisableAutoAdminPort: true,
-		RetainDebugStore:     true, // Assertions below inspect files in the debug store
+		DisableAutoAdminPort: true, // Istio uses fixed ports https://github.com/istio/istio/issues/31138
 	})
 
 	serverInfo, err := os.ReadFile(filepath.Join(runtime.DebugStore(), "server_info.json"))
diff --git a/pkg/binary/envoy/debug/admin_test.go b/pkg/binary/envoy/debug/admin_test.go
index d88d0dd..65a4e2e 100644
--- a/pkg/binary/envoy/debug/admin_test.go
+++ b/pkg/binary/envoy/debug/admin_test.go
@@ -35,8 +35,7 @@ func TestEnableEnvoyAdminDataCollection(t *testing.T) {
 	defer os.RemoveAll(r.DebugStore())
 
 	envoytest.RequireRunTerminate(t, r, envoytest.RunKillOptions{
-		Bootstrap:        "testdata/admin.yaml", // we need services defined in order to check adminAPIPaths
-		RetainDebugStore: true,                  // Assertions below inspect files in the debug store
+		Bootstrap: "testdata/admin.yaml", // we need services defined in order to check adminAPIPaths
 	})
 
 	for _, filename := range adminAPIPaths {
diff --git a/pkg/binary/envoy/debug/log_test.go b/pkg/binary/envoy/debug/log_test.go
index a5bd071..4708035 100644
--- a/pkg/binary/envoy/debug/log_test.go
+++ b/pkg/binary/envoy/debug/log_test.go
@@ -30,9 +30,7 @@ func TestEnableEnvoyLogCollection(t *testing.T) {
 	require.NoError(t, err, "error getting envoy runtime")
 	defer os.RemoveAll(r.DebugStore())
 
-	envoytest.RequireRunTerminate(t, r, envoytest.RunKillOptions{
-		RetainDebugStore: false, // We don't need to unarchive the tar.gz because access logs hold the DebugStore open.
-	})
+	envoytest.RequireRunTerminate(t, r, envoytest.RunKillOptions{})
 
 	// We expect to see logs because when envoy starts up, the status checker will make HTTP requests to /ready
 	for _, filename := range []string{"logs/access.log", "logs/error.log"} {
diff --git a/pkg/binary/envoy/debug/lsof_test.go b/pkg/binary/envoy/debug/lsof_test.go
index fb02a10..46d077e 100644
--- a/pkg/binary/envoy/debug/lsof_test.go
+++ b/pkg/binary/envoy/debug/lsof_test.go
@@ -32,9 +32,7 @@ func TestEnableOpenFilesDataCollection(t *testing.T) {
 	require.NoError(t, err, "error getting envoy runtime")
 	defer os.RemoveAll(r.DebugStore())
 
-	envoytest.RequireRunTerminate(t, r, envoytest.RunKillOptions{
-		RetainDebugStore: true, // Assertions below inspect files in the debug store
-	})
+	envoytest.RequireRunTerminate(t, r, envoytest.RunKillOptions{})
 
 	file := "lsof/lsof.json"
 	path := filepath.Join(r.DebugStore(), file)
diff --git a/pkg/binary/envoy/debug/node_test.go b/pkg/binary/envoy/debug/node_test.go
index 3b83912..4bd7800 100644
--- a/pkg/binary/envoy/debug/node_test.go
+++ b/pkg/binary/envoy/debug/node_test.go
@@ -32,9 +32,7 @@ func TestEnableNodeCollection(t *testing.T) {
 	require.NoError(t, err, "error creating envoy runtime")
 	defer os.RemoveAll(r.DebugStore())
 
-	envoytest.RequireRunTerminate(t, r, envoytest.RunKillOptions{
-		RetainDebugStore: true, // Assertions below inspect files in the debug store
-	})
+	envoytest.RequireRunTerminate(t, r, envoytest.RunKillOptions{})
 
 	files := [...]string{"node/ps.txt", "node/network_interface.json", "node/connections.json"}
 	for _, file := range files {
diff --git a/pkg/binary/envoy/run_test.go b/pkg/binary/envoy/run_test.go
index 9670e90..5d8b8ab 100644
--- a/pkg/binary/envoy/run_test.go
+++ b/pkg/binary/envoy/run_test.go
@@ -15,7 +15,6 @@
 package envoy
 
 import (
-	"fmt"
 	"os"
 	"path/filepath"
 	"sync"
@@ -31,26 +30,25 @@ import (
 
 func TestRuntime_RunPath(t *testing.T) {
 	tests := []struct {
-		name        string
-		args        []string
-		killerFunc  func(*Runtime)
-		wantPreTerm bool
+		name                      string
+		args                      []string
+		killerFunc                func(*Runtime)
+		wantPreTerm, wantPostTerm bool
 	}{
 		{
-			name:        "GetEnvoy shot first",
-			killerFunc:  func(r *Runtime) { r.SendSignal(syscall.SIGINT) },
-			wantPreTerm: true,
+			name:         "GetEnvoy shot first",
+			killerFunc:   func(r *Runtime) { r.FakeInterrupt() },
+			wantPreTerm:  true,
+			wantPostTerm: true,
 		},
 		{
-			name:        "Envoy shot first",
-			killerFunc:  func(r *Runtime) { r.cmd.Process.Signal(syscall.SIGINT) },
-			wantPreTerm: false,
+			name:       "Envoy shot first",
+			killerFunc: func(r *Runtime) { r.cmd.Process.Signal(syscall.SIGINT) },
 		},
 		{
-			name:        "Envoy simulate error",
-			killerFunc:  func(r *Runtime) { time.Sleep(time.Millisecond * 100) },
-			args:        []string{"error"},
-			wantPreTerm: false,
+			name:       "Envoy simulate error",
+			killerFunc: func(r *Runtime) { time.Sleep(time.Millisecond * 100) },
+			args:       []string{"error"},
 		},
 	}
 
@@ -58,7 +56,7 @@ func TestRuntime_RunPath(t *testing.T) {
 		tc := tt
 		t.Run(tc.name, func(t *testing.T) {
 			// This ensures functions are called in the correct order
-			r, preStartCalled, preTerminationCalled := newRuntimeWithMockFunctions(t)
+			r, preStartCalled, preTerminationCalled, postTerminationCalled := newRuntimeWithMockFunctions(t)
 			tempDir, revertTempDir := morerequire.RequireNewTempDir(t)
 			defer revertTempDir()
 			r.store = tempDir
@@ -84,13 +82,14 @@ func TestRuntime_RunPath(t *testing.T) {
 
 			// Assert appropriate functions are called
 			require.True(t, *preStartCalled, "preStart was not called")
-			require.Equal(t, tc.wantPreTerm, *preTerminationCalled, fmt.Sprintf("expected preTermination execution to be %v", tc.wantPreTerm))
+			require.Equal(t, tc.wantPreTerm, *preTerminationCalled, "expected preTermination execution to be %v", tc.wantPreTerm)
+			require.Equal(t, tc.wantPostTerm, *postTerminationCalled, "expected postTermination execution to be %v", tc.wantPostTerm)
 		})
 	}
 }
 
 // This ensures functions are called in the correct order
-func newRuntimeWithMockFunctions(t *testing.T) (*Runtime, *bool, *bool) {
+func newRuntimeWithMockFunctions(t *testing.T) (*Runtime, *bool, *bool, *bool) {
 	preStartCalled := false
 	preStart := func(r *Runtime) {
 		r.RegisterPreStart(func(r binary.Runner) error {
@@ -117,7 +116,14 @@ func newRuntimeWithMockFunctions(t *testing.T) (*Runtime, *bool, *bool) {
 			return nil
 		})
 	}
-	runtime, _ := NewRuntime(preStart, preTermination)
+	postTerminationCalled := false
+	postTermination := func(r *Runtime) {
+		r.RegisterPreTermination(func(r binary.Runner) error {
+			postTerminationCalled = true
+			return nil
+		})
+	}
+	runtime, _ := NewRuntime(preStart, preTermination, postTermination)
 
-	return runtime.(*Runtime), &preStartCalled, &preTerminationCalled
+	return runtime.(*Runtime), &preStartCalled, &preTerminationCalled, &postTerminationCalled
 }
diff --git a/pkg/binary/envoytest/util.go b/pkg/binary/envoytest/util.go
index 0bf7045..64ff8f7 100644
--- a/pkg/binary/envoytest/util.go
+++ b/pkg/binary/envoytest/util.go
@@ -15,11 +15,11 @@
 package envoytest
 
 import (
+	"context"
 	"fmt"
 	"os"
 	"path/filepath"
 	"sync"
-	"syscall"
 	"testing"
 	"time"
 
@@ -88,11 +88,10 @@ func fetchEnvoy() error {
 type RunKillOptions struct {
 	Bootstrap            string
 	DisableAutoAdminPort bool
-	RetainDebugStore     bool
 }
 
-// RequireRunTerminate executes envoy, waits for ready, sends sigint, waits for termination, then unarchives the debug directory.
-// It should be used when you just want to cycle through an Envoy lifecycle.
+// RequireRunTerminate executes envoy, waits for ready, sends sigint, waits for termination, then unarchives the debug
+// directory. It should be used when you just want to cycle through an Envoy lifecycle.
 func RequireRunTerminate(t *testing.T, r binary.Runner, options RunKillOptions) {
 	key, err := manifest.NewKey(reference)
 	require.NoError(t, err)
@@ -115,24 +114,25 @@ func RequireRunTerminate(t *testing.T, r binary.Runner, options RunKillOptions)
 	}
 
 	// This ensures on any panic the envoy process is terminated, which can prevent test hangs.
-	deferredTerminate := func() {
-		// envoy.waitForTerminationSignals() registers SIGINT and SIGTERM
-		r.SendSignal(syscall.SIGTERM)
+	deferredInterrupt := func() {
+		r.FakeInterrupt()
 	}
 
 	defer func() {
-		if deferredTerminate != nil {
-			deferredTerminate()
+		if deferredInterrupt != nil {
+			deferredInterrupt()
 		}
 	}()
 
 	// Ensure we don't leave tar.gz files around after the test completes
 	defer os.RemoveAll(r.DebugStore() + ".tar.gz") // nolint
 
+	ctx, cancel := context.WithCancel(context.Background())
 	go func() {
 		if err := r.Run(key, args); err != nil {
 			log.Errorf("unable to run key %s: %v", key, err)
 		}
+		cancel()
 	}()
 
 	// Look for terminated or ready, so that we fail faster than polling for status ready
@@ -143,17 +143,17 @@ func RequireRunTerminate(t *testing.T, r binary.Runner, options RunKillOptions)
 	require.Equal(t, expectedStatus, r.Status(), "never achieved status(%d)", expectedStatus)
 
 	// Now, terminate the server.
-	r.SendSignal(syscall.SIGTERM)
-	require.Eventually(t, func() bool {
-		return r.Status() == binary.StatusTerminated
-	}, 10*time.Second, 50*time.Millisecond, "never achieved StatusTerminated")
+	r.FakeInterrupt()
+	deferredInterrupt = nil
 
-	deferredTerminate = nil // We succeeded, so no longer need to kill the envoy process
+	select { // Await run completion
+	case <-time.After(10 * time.Second):
+		t.Fatal("Run never completed")
+	case <-ctx.Done():
+	}
 
 	// RunPath deletes the debug store directory after making a tar.gz with the same name.
 	// Restore it so assertions can read the contents later.
-	if options.RetainDebugStore {
-		e := archiver.Unarchive(r.DebugStore()+".tar.gz", filepath.Dir(r.DebugStore()))
-		require.NoError(t, e, "error extracting DebugStore")
-	}
+	e := archiver.Unarchive(r.DebugStore()+".tar.gz", filepath.Dir(r.DebugStore()))
+	require.NoError(t, e, "error extracting DebugStore")
 }