Repository: tetratelabs/getenvoy
Pull request #278 (2021-06-11 03:57:30)

Writes envoy.pid and verifies Envoy is killed in e2e tests
This creates a foundation for future work by ensure Envoy is killed as
expected. Before, we weren't checking at all or weren't checking
propertly depending on the test.

This also does more hardening around timeouts by sharing a single parent
context during e2e run tests.

codefromthecrypt: 3Q!

Patch:
diff --git a/.github/workflows/commit.yaml b/.github/workflows/commit.yaml
index be0e2bb..ef99a23 100644
--- a/.github/workflows/commit.yaml
+++ b/.github/workflows/commit.yaml
@@ -66,6 +66,7 @@ jobs:
         run: make e2e
 
       - name: "Generate coverage report" # only once (not per OS)
+        if: runner.os == 'Linux'
         run: make coverage
 
       - name: "Upload coverage report" # only on master push and only once (not per OS)
diff --git a/internal/cmd/run.go b/internal/cmd/run.go
index c9ba2aa..706c008 100644
--- a/internal/cmd/run.go
+++ b/internal/cmd/run.go
@@ -48,10 +48,10 @@ The version to use is downloaded and installed, if necessary.
 Envoy interprets the '[arguments...]' and runs in the current working
 directory (aka $CWD) until getenvoy is interrupted (ex Ctrl+C, Ctrl+Break).
 
-Console output is captured as "stdout.log" and "stderr.log" in the run
-directory (` + "`$GETENVOY_HOME/runs/$epochtime`" + `). When interrupted,
-shutdown hooks write files, including admin endpoints, network and process
-state. Upon exit, these zip into ` + "`$GETENVOY_HOME/runs/$epochtime.tar.gz`",
+Envoy's process ID and console output write to "envoy.pid", stdout.log" and
+"stderr.log" in the run directory (` + "`$GETENVOY_HOME/runs/$epochtime`" + `).
+When interrupted, shutdown hooks write files including network and process
+state. On exit, these archive into ` + "`$GETENVOY_HOME/runs/$epochtime.tar.gz`",
 		Before: func(c *cli.Context) error {
 			if err := os.MkdirAll(o.HomeDir, 0750); err != nil {
 				return NewValidationError(err.Error())
diff --git a/internal/envoy/run.go b/internal/envoy/run.go
index ef3b212..bd0589d 100644
--- a/internal/envoy/run.go
+++ b/internal/envoy/run.go
@@ -17,11 +17,15 @@ package envoy
 import (
 	"context"
 	"fmt"
+	"os"
 	"os/exec"
 	"os/signal"
+	"strconv"
 	"strings"
 	"syscall"
 	"time"
+
+	"github.com/tetratelabs/getenvoy/internal/moreos"
 )
 
 // Run execs the binary at the path with the args passed. It is a blocking function that can be shutdown via SIGINT.
@@ -31,7 +35,7 @@ func (r *Runtime) Run(ctx context.Context, args []string) (err error) {
 	cmd := exec.Command(r.opts.EnvoyPath, args...) // #nosec -> users can run whatever binary they like!
 	cmd.Stdout = r.Out
 	cmd.Stderr = r.Err
-	cmd.SysProcAttr = sysProcAttr()
+	cmd.SysProcAttr = moreos.ProcessGroupAttr()
 	r.cmd = cmd
 
 	// Suppress any error and replace it with the envoy exit status when > 1
@@ -54,6 +58,11 @@ func (r *Runtime) Run(ctx context.Context, args []string) (err error) {
 		return fmt.Errorf("unable to start Envoy process: %w", err)
 	}
 
+	// Warn, but don't fail if we can't write the pid file for some reason
+	if err := os.WriteFile(r.pidPath, []byte(strconv.Itoa(cmd.Process.Pid)), 0600); err != nil {
+		fmt.Fprintln(r.Out, "warning:", err) //nolint
+	}
+
 	waitCtx, waitCancel := context.WithCancel(ctx)
 	sigCtx, sigCancel := signal.NotifyContext(waitCtx, syscall.SIGINT, syscall.SIGTERM)
 	defer waitCancel()
diff --git a/internal/envoy/runtime.go b/internal/envoy/runtime.go
index f1e422a..f50e0cc 100644
--- a/internal/envoy/runtime.go
+++ b/internal/envoy/runtime.go
@@ -37,7 +37,7 @@ const (
 // NewRuntime creates a new Runtime that runs envoy in globals.RunOpts RunDir
 // opts allows a user running envoy to control the working directory by ID or path, allowing explicit cleanup.
 func NewRuntime(opts *globals.RunOpts) *Runtime {
-	return &Runtime{opts: opts}
+	return &Runtime{opts: opts, pidPath: filepath.Join(opts.RunDir, "envoy.pid")}
 }
 
 // Runtime manages an Envoy lifecycle
@@ -48,7 +48,7 @@ type Runtime struct {
 	Out io.Writer
 	Err io.Writer
 
-	adminAddress, adminAddressPath string
+	adminAddress, adminAddressPath, pidPath string
 
 	// FakeInterrupt is exposed for unit tests to pretend "getenvoy run" received a Ctrl+C or Ctrl+Break.
 	// End-to-end tests should kill the getenvoy process to achieve the same.
diff --git a/internal/envoy/shutdown.go b/internal/envoy/shutdown.go
index 998d0fb..45d9db7 100644
--- a/internal/envoy/shutdown.go
+++ b/internal/envoy/shutdown.go
@@ -20,9 +20,9 @@ import (
 	"os"
 	"path/filepath"
 	"sync"
-	"syscall"
 	"time"
 
+	"github.com/tetratelabs/getenvoy/internal/moreos"
 	"github.com/tetratelabs/getenvoy/internal/tar"
 )
 
@@ -59,7 +59,9 @@ func (r *Runtime) handleShutdown(ctx context.Context) {
 func (r *Runtime) interruptEnvoy() {
 	p := r.cmd.Process
 	fmt.Fprintf(r.Out, "sending interrupt to envoy (pid=%d)\n", p.Pid) //nolint
-	_ = p.Signal(syscall.SIGINT)
+	if err := moreos.Interrupt(p); err != nil {
+		fmt.Fprintln(r.Out, "warning:", err) //nolint
+	}
 }
 
 func (r *Runtime) archiveRunDir() error {
diff --git a/internal/moreos/moreos.go b/internal/moreos/moreos.go
new file mode 100644
index 0000000..c7e773b
--- /dev/null
+++ b/internal/moreos/moreos.go
@@ -0,0 +1,31 @@
+// Copyright 2021 Tetrate
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package moreos
+
+import (
+	"os"
+	"syscall"
+)
+
+// ProcessGroupAttr sets attributes that ensure exec.Cmd doesn't propagate signals from getenvoy by default.
+// This is used to ensure shutdown hooks can apply
+func ProcessGroupAttr() *syscall.SysProcAttr {
+	return processGroupAttr() // un-exported to prevent godoc drift
+}
+
+// Interrupt attempts to interrupt the process. It doesn't necessarily kill it.
+func Interrupt(p *os.Process) error {
+	return interrupt(p) // un-exported to prevent godoc drift
+}
diff --git a/internal/envoy/procattr.go b/internal/moreos/proc.go
similarity index 68%
rename from internal/envoy/procattr.go
rename to internal/moreos/proc.go
index fef5bea..7308d4a 100644
--- a/internal/envoy/procattr.go
+++ b/internal/moreos/proc.go
@@ -1,4 +1,4 @@
-// Copyright 2019 Tetrate
+// Copyright 2021 Tetrate
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -14,15 +14,17 @@
 
 // +build !linux
 
-package envoy
+package moreos
 
 import (
+	"os"
 	"syscall"
 )
 
-func sysProcAttr() *syscall.SysProcAttr {
-	// TODO: Add a test for "getenvoy run" kill -9, that checks if the child envoy process remains running.
-	return &syscall.SysProcAttr{
-		Setpgid: true, // equivalent to setpgrp() syscall
-	}
+func processGroupAttr() *syscall.SysProcAttr {
+	return &syscall.SysProcAttr{Setpgid: true}
+}
+
+func interrupt(p *os.Process) error {
+	return p.Signal(syscall.SIGINT)
 }
diff --git a/internal/envoy/procattr_linux.go b/internal/moreos/proc_linux.go
similarity index 61%
rename from internal/envoy/procattr_linux.go
rename to internal/moreos/proc_linux.go
index 0cd7b10..1a57ec8 100644
--- a/internal/envoy/procattr_linux.go
+++ b/internal/moreos/proc_linux.go
@@ -1,4 +1,4 @@
-// Copyright 2019 Tetrate
+// Copyright 2021 Tetrate
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -12,13 +12,18 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package envoy
+package moreos
 
-import "syscall"
+import (
+	"os"
+	"syscall"
+)
 
-func sysProcAttr() *syscall.SysProcAttr {
-	return &syscall.SysProcAttr{
-		Setpgid:   true,            // equivalent to setpgrp() syscall
-		Pdeathsig: syscall.SIGTERM, // ensure the child Envoy process is cleaned up even if we die
-	}
+func processGroupAttr() *syscall.SysProcAttr {
+	// Pdeathsig aims to ensure the process group is cleaned up even if this process dies
+	return &syscall.SysProcAttr{Setpgid: true, Pdeathsig: syscall.SIGTERM}
+}
+
+func interrupt(p *os.Process) error {
+	return p.Signal(syscall.SIGINT)
 }


Test patch:
diff --git a/e2e/admin_test.go b/e2e/admin_test.go
index 47ded23..cf550a0 100644
--- a/e2e/admin_test.go
+++ b/e2e/admin_test.go
@@ -15,6 +15,7 @@
 package e2e
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
 	"io"
@@ -35,18 +36,14 @@ type adminClient struct {
 	baseURL string
 }
 
-func (c *adminClient) isReady() (bool, error) {
-	resp, err := http.Get(c.baseURL + "/ready")
-	if err != nil {
-		return false, err
-	}
-	defer resp.Body.Close() //nolint:errcheck
-	return resp.StatusCode == http.StatusOK, nil
+func (c *adminClient) isReady(ctx context.Context) bool {
+	_, err := httpGet(ctx, c.baseURL+"/ready")
+	return err == nil
 }
 
-func (c *adminClient) getMainListenerURL() (string, error) {
+func (c *adminClient) getMainListenerURL(ctx context.Context) (string, error) {
 	var s map[string]interface{}
-	if err := c.getJSON("/listeners", &s); err != nil {
+	if err := c.getJSON(ctx, "/listeners", &s); err != nil {
 		return "", err
 	}
 
@@ -62,16 +59,23 @@ func (c *adminClient) getMainListenerURL() (string, error) {
 	return "", fmt.Errorf("didn't find main port in %+v", s)
 }
 
-func (c *adminClient) getJSON(path string, v interface{}) error {
-	body, err := httpGet(c.baseURL + path + "?format=json")
+func (c *adminClient) getJSON(ctx context.Context, path string, v interface{}) error {
+	body, err := httpGet(ctx, c.baseURL+path+"?format=json")
 	if err != nil {
 		return err
 	}
 	return json.Unmarshal(body, v)
 }
 
-func httpGet(url string) ([]byte, error) {
-	resp, err := http.Get(url)
+func httpGet(ctx context.Context, url string) ([]byte, error) {
+	req, err := http.NewRequestWithContext(ctx, "GET", url, nil)
+	if err != nil {
+		return nil, err
+	}
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		return nil, err
+	}
 	if err != nil {
 		return nil, err
 	}
diff --git a/e2e/getenvoy_run_test.go b/e2e/getenvoy_run_test.go
index 3eed366..9c0bf2c 100644
--- a/e2e/getenvoy_run_test.go
+++ b/e2e/getenvoy_run_test.go
@@ -16,19 +16,22 @@ package e2e
 
 import (
 	"bufio"
+	"context"
 	_ "embed" // We embed the config files to make them easier to copy
 	"fmt"
 	"log"
 	"os"
 	"path/filepath"
 	"regexp"
+	"strconv"
 	"strings"
-	"syscall"
 	"testing"
 	"time"
 
+	"github.com/shirou/gopsutil/v3/process"
 	"github.com/stretchr/testify/require"
 
+	"github.com/tetratelabs/getenvoy/internal/moreos"
 	"github.com/tetratelabs/getenvoy/internal/tar"
 	"github.com/tetratelabs/getenvoy/internal/test/morerequire"
 )
@@ -39,6 +42,8 @@ var (
 	staticFilesystemConfig  []byte
 	adminAddressPathPattern = regexp.MustCompile(`--admin-address-path ([^ ]+)`)
 	envoyStartedLine        = "starting main dispatch loop"
+	// minRunArgs is the minimal config needed to run Envoy 1.18+, non-windows <1.18 need access_log_path: '/dev/stdout'
+	minRunArgs = []string{"run", "--config-yaml", "admin: {address: {socket_address: {address: '127.0.0.1', port_value: 0}}}"}
 )
 
 // TestGetEnvoyRun runs the equivalent of "getenvoy run"
@@ -47,13 +52,10 @@ var (
 func TestGetEnvoyRun(t *testing.T) {
 	t.Parallel() // uses random ports so safe to run parallel
 
-	// Below is the minimal config needed to run Envoy 1.18+, non-windows <1.18 need access_log_path: '/dev/stdout'
-	args := []string{"run", "--config-yaml", "admin: {address: {socket_address: {address: '127.0.0.1', port_value: 0}}}"}
+	cmd, cleanup := envoyRunTest(t, nil, minRunArgs...)
+	defer cleanup()
 
-	c, runArchive := envoyRunTest(t, nil, args...)
-	defer os.Remove(runArchive) //nolint
-
-	verifyRunArchive(t, runArchive, c)
+	verifyRunArchive(t, cmd)
 }
 
 func TestGetEnvoyRun_StaticFilesystem(t *testing.T) {
@@ -66,29 +68,31 @@ func TestGetEnvoyRun_StaticFilesystem(t *testing.T) {
 	responseFromRunDirectory := []byte("foo")
 	require.NoError(t, os.WriteFile("response.txt", responseFromRunDirectory, 0600))
 
-	_, runArchive := envoyRunTest(t, func(c *getEnvoy, a *adminClient) {
-		mainURL, err := a.getMainListenerURL()
-		require.NoError(t, err, `couldn't read mainURL after running [%v]`, c)
+	_, cleanup := envoyRunTest(t, func(ctx context.Context, c *getEnvoy, a *adminClient) {
+		mainURL, err := a.getMainListenerURL(ctx)
+		require.NoError(t, err, "couldn't read mainURL after running [%v]", c)
 
-		body, err := httpGet(mainURL)
-		require.NoError(t, err, `couldn't read %s after running [%v]`, mainURL, c)
+		body, err := httpGet(ctx, mainURL)
+		require.NoError(t, err, "couldn't read %s after running [%v]", mainURL, c)
 
 		// If this passes, we know Envoy is running in the current directory, so can resolve relative configuration.
-		require.Equal(t, responseFromRunDirectory, body, `unexpected content in %s after running [%v]`, mainURL, c)
+		require.Equal(t, responseFromRunDirectory, body, "unexpected content in %s after running [%v]", mainURL, c)
 	}, "run", "-c", "envoy.yaml")
 
-	require.NoError(t, os.Remove(runArchive))
+	cleanup()
 }
 
-// envoyRunTest runs the given args and the test function once envoy is available. This returns the command and the path
-// to the run archive.
+// envoyRunTest runs the given args and the test function once envoy is available. This returns the command and a
+// function to remove the run archive.
 //
 // If the process successfully starts, this blocks until the adminClient is available.  The process is interrupted when
 // the test completes, or a timeout occurs.
-func envoyRunTest(t *testing.T, test func(*getEnvoy, *adminClient), args ...string) (*getEnvoy, string) {
-	c, cancel := newGetEnvoy(args...)
+func envoyRunTest(t *testing.T, test func(context.Context, *getEnvoy, *adminClient), args ...string) (*getEnvoy, func()) {
+	ctx, cancel := context.WithTimeout(context.Background(), runTimeout)
 	defer cancel()
 
+	c := newGetEnvoy(ctx, args...)
+
 	stdout, out := c.cmd.StdoutPipe()
 	outLines := bufio.NewScanner(stdout)
 	require.NoError(t, out)
@@ -100,7 +104,7 @@ func envoyRunTest(t *testing.T, test func(*getEnvoy, *adminClient), args ...stri
 	err = c.cmd.Start()
 	require.NoError(t, err)
 
-	log.Printf(`waiting for getenvoy stdout to match %q after running [%v]`, adminAddressPathPattern, c)
+	log.Printf("waiting for getenvoy stdout to match %q after running [%v]", adminAddressPathPattern, c)
 	var adminAddressPath string
 	go func() {
 		for outLines.Scan() {
@@ -111,7 +115,8 @@ func envoyRunTest(t *testing.T, test func(*getEnvoy, *adminClient), args ...stri
 			// contents until Envoy creates it. This only ensures we know the file name.
 			if adminAddressPathPattern.MatchString(l) {
 				adminAddressPath = adminAddressPathPattern.FindStringSubmatch(l)[1]
-				log.Printf(`waiting for Envoy stdout to match %q after running [%v]`, envoyStartedLine, c)
+				c.runDir = filepath.Dir(adminAddressPath)
+				log.Printf("waiting for Envoy stdout to match %q after running [%v]", envoyStartedLine, c)
 			}
 		}
 	}()
@@ -123,8 +128,9 @@ func envoyRunTest(t *testing.T, test func(*getEnvoy, *adminClient), args ...stri
 
 			// When we get to this line, we can assume Envoy started properly. Run the test
 			if strings.Contains(l, envoyStartedLine) {
-				require.NotEmpty(t, adminAddressPath, "expected adminAddressPath to be set")
-				go runTestAndInterruptEnvoy(t, c, adminAddressPath, test) // don't block stderr when the test is running
+				require.NotEmpty(t, c.runDir, "expected adminAddressPath to be set")
+				requireEnvoyPid(t, c)                        // check here to avoid race condition
+				go runTestAndInterruptEnvoy(ctx, t, c, test) // don't block printing stderr!
 			}
 		}
 	}()
@@ -132,41 +138,59 @@ func envoyRunTest(t *testing.T, test func(*getEnvoy, *adminClient), args ...stri
 	err = c.cmd.Wait() // This won't hang forever because newGetEnvoy started it with a context timeout!
 	require.NoError(t, err)
 
-	// return the run archive location
-	return c, filepath.Dir(adminAddressPath) + ".tar.gz"
+	// Ensure the Envoy process was terminated
+	_, err = process.NewProcessWithContext(ctx, c.envoyPid) // because os.FindProcess is no-op in Linux!
+	require.Error(t, err, "expected GetEnvoy to terminate Envoy after running [%v]", c)
+
+	return c, func() {
+		// this may not be present if the process was kill -9'd so don't error
+		os.Remove(c.runDir + ".tar.gz") //nolint
+	}
 }
 
-func runTestAndInterruptEnvoy(t *testing.T, c *getEnvoy, adminAddressPath string, test func(*getEnvoy, *adminClient)) {
+func runTestAndInterruptEnvoy(ctx context.Context, t *testing.T, c *getEnvoy, test func(context.Context, *getEnvoy, *adminClient)) {
 	defer func() {
-		log.Printf(`shutting down Envoy after running [%v]`, c)
-		_ = c.cmd.Process.Signal(syscall.SIGTERM)
+		log.Printf("shutting down Envoy after running [%v]", c)
+		require.NoError(t, moreos.Interrupt(c.cmd.Process), "error shutting down Envoy after running [%v]", c)
 	}()
-	a := requireEnvoyReady(t, adminAddressPath, c)
+	a := requireEnvoyReady(ctx, t, c)
 	if test != nil {
-		test(c, a)
+		test(ctx, c, a)
 	}
 }
 
-func requireEnvoyReady(t *testing.T, adminAddressPath string, c interface{}) *adminClient {
+func requireEnvoyReady(ctx context.Context, t *testing.T, c *getEnvoy) *adminClient {
+	adminAddressPath := filepath.Join(c.runDir, "admin-address.txt")
 	adminAddress, err := os.ReadFile(adminAddressPath) //nolint:gosec
-	require.NoError(t, err, `error reading admin address file %q after running [%v]`, adminAddressPath, c)
+	require.NoError(t, err, "error reading admin address file %q after running [%v]", adminAddressPath, c)
 
-	log.Printf(`waiting for Envoy adminClient to connect after running [%v]`, c)
+	log.Printf("waiting for Envoy adminClient to connect after running [%v]", c)
 	envoyClient, err := newAdminClient(string(adminAddress))
-	require.NoError(t, err, `error from Envoy adminClient %s after running [%v]`, adminAddress, c)
+	require.NoError(t, err, "error from Envoy adminClient %s after running [%v]", adminAddress, c)
 	require.Eventually(t, func() bool {
-		ready, err := envoyClient.isReady()
-		return err == nil && ready
-	}, 1*time.Minute, 100*time.Millisecond, `Envoy adminClient %s never ready after running [%v]`, adminAddress, c)
+		return envoyClient.isReady(ctx)
+	}, 1*time.Minute, 100*time.Millisecond, "Envoy adminClient %s never ready after running [%v]", adminAddress, c)
 
 	return envoyClient
 }
 
+// requireEnvoyPid ensures $runDir/envoy.pid was written and is valid
+func requireEnvoyPid(t *testing.T, c *getEnvoy) {
+	pidPath := filepath.Join(c.runDir, "envoy.pid")
+	pidTxt, err := os.ReadFile(pidPath)
+	require.NoError(t, err, "couldn't read %s after running [%v]", pidPath, c)
+	pid, err := strconv.Atoi(string(pidTxt))
+	require.NoError(t, err, "invalid Envoy pid in %s after running [%v]", pidPath, c)
+	require.Greater(t, pid, 1, "invalid Envoy pid %s after running [%v]", pid, c)
+	c.envoyPid = int32(pid)
+}
+
 // Run deletes the run directory after making a tar.gz with the same name. This extracts it and tests the contents.
-func verifyRunArchive(t *testing.T, runArchive string, c interface{}) {
+func verifyRunArchive(t *testing.T, c *getEnvoy) {
 	runDir, removeRunDir := morerequire.RequireNewTempDir(t)
 	defer removeRunDir()
 
+	runArchive := c.runDir + ".tar.gz"
 	src, err := os.Open(runArchive)
 	require.NoError(t, err, "error opening %s after shutdown [%v]", runArchive, c)
 
@@ -177,7 +201,7 @@ func verifyRunArchive(t *testing.T, runArchive string, c interface{}) {
 	for _, filename := range []string{"stdout.log", "stderr.log", "config_dump.json", "stats.json"} {
 		path := filepath.Join(runDir, filename)
 		f, err := os.Stat(path)
-		require.NoError(t, err, `run archive %s doesn't contain %s after shutdown [%v]`, runArchive, filename, c)
-		require.NotEmpty(t, f.Size(), `%s was empty after shutdown [%v]`, filename, c)
+		require.NoError(t, err, "run archive %s doesn't contain %s after shutdown [%v]", runArchive, filename, c)
+		require.NotEmpty(t, f.Size(), "%s was empty after shutdown [%v]", filename, c)
 	}
 }
diff --git a/e2e/getenvoy_test.go b/e2e/getenvoy_test.go
index 3c32481..f4271de 100644
--- a/e2e/getenvoy_test.go
+++ b/e2e/getenvoy_test.go
@@ -26,6 +26,6 @@ func TestGetEnvoyVersion(t *testing.T) {
 	stdout, stderr, err := getEnvoyExec("--version")
 
 	require.Regexp(t, `^getenvoy version ([^\s]+)\n$`, stdout)
-	require.Equal(t, ``, stderr)
+	require.Empty(t, stderr)
 	require.NoError(t, err)
 }
diff --git a/e2e/getenvoy_use_test.go b/e2e/getenvoy_use_test.go
index 880acd1..53fd419 100644
--- a/e2e/getenvoy_use_test.go
+++ b/e2e/getenvoy_use_test.go
@@ -58,10 +58,11 @@ func TestGetEnvoyUse(t *testing.T) {
 }
 
 func TestGetEnvoyUse_UnknownVersion(t *testing.T) {
-	stdout, stderr, err := getEnvoyExec("use", "1.1.1")
+	v := "1.1.1"
+	stdout, stderr, err := getEnvoyExec("use", v)
 
 	require.EqualError(t, err, "exit status 1")
 	require.Empty(t, stdout)
-	require.Equal(t, fmt.Sprintf(`error: couldn't find version "1.1.1" for platform "%s/%s"
-`, runtime.GOOS, runtime.GOARCH), stderr)
+	require.Equal(t, fmt.Sprintf(`error: couldn't find version "%s" for platform "%s/%s"
+`, v, runtime.GOOS, runtime.GOARCH), stderr)
 }
diff --git a/e2e/main_test.go b/e2e/main_test.go
index 7396af1..62337af 100644
--- a/e2e/main_test.go
+++ b/e2e/main_test.go
@@ -28,6 +28,8 @@ import (
 	"strings"
 	"testing"
 	"time"
+
+	"github.com/tetratelabs/getenvoy/internal/moreos"
 )
 
 //nolint:golint
@@ -35,6 +37,7 @@ const (
 	getenvoyBinaryEnvKey   = "E2E_GETENVOY_BINARY"
 	envoyVersionsURLEnvKey = "ENVOY_VERSIONS_URL"
 	envoyVersionsJSON      = "../site/envoy-versions.json"
+	runTimeout             = 2 * time.Minute
 )
 
 var (
@@ -59,7 +62,7 @@ func TestMain(m *testing.M) {
 	if _, ok := os.LookupEnv(envoyVersionsURLEnvKey); !ok && strings.Contains(versionLine, "SNAPSHOT") {
 		s, err := mockEnvoyVersionsServer() // no defer s.Close() because os.Exit() subverts it
 		if err != nil {
-			fmt.Fprintf(os.Stderr, `failed to serve %s: %v`, envoyVersionsJSON, err)
+			fmt.Fprintf(os.Stderr, "failed to serve %s: %v", envoyVersionsJSON, err)
 			os.Exit(1)
 		}
 		os.Setenv(envoyVersionsURLEnvKey, s.URL)
@@ -92,7 +95,7 @@ func mockEnvoyVersionsServer() (*httptest.Server, error) {
 			h := r.Header.Get(k)
 			if h != v {
 				w.WriteHeader(500)
-				w.Write([]byte(fmt.Sprintf(`invalid %q: %s != %s\n`, k, h, v))) //nolint
+				w.Write([]byte(fmt.Sprintf("invalid %q: %s != %s\n", k, h, v))) //nolint
 				return
 			}
 		}
@@ -132,12 +135,16 @@ func readGetEnvoyPath() (string, error) {
 	return path, nil
 }
 
-type getEnvoy struct{ cmd *exec.Cmd }
+type getEnvoy struct {
+	cmd      *exec.Cmd
+	runDir   string
+	envoyPid int32
+}
 
-// newGetEnvoy creates an exec.Cmd which will timeout instead of hang forever
-func newGetEnvoy(args ...string) (*getEnvoy, func()) {
-	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
-	return &getEnvoy{exec.CommandContext(ctx, getEnvoyPath, args...)}, cancel //nolint:gosec
+func newGetEnvoy(ctx context.Context, args ...string) *getEnvoy {
+	cmd := exec.CommandContext(ctx, getEnvoyPath, args...)
+	cmd.SysProcAttr = moreos.ProcessGroupAttr()
+	return &getEnvoy{cmd: cmd}
 }
 
 func (b *getEnvoy) String() string {
@@ -145,8 +152,7 @@ func (b *getEnvoy) String() string {
 }
 
 func getEnvoyExec(args ...string) (string, string, error) {
-	g, cancel := newGetEnvoy(args...)
-	defer cancel()
+	g := newGetEnvoy(context.Background(), args...)
 	stdout := new(bytes.Buffer)
 	stderr := new(bytes.Buffer)
 	g.cmd.Stdout = io.MultiWriter(os.Stdout, stdout) // we want to see full `getenvoy` output in the test log
diff --git a/internal/cmd/testdata/getenvoy_run_help.txt b/internal/cmd/testdata/getenvoy_run_help.txt
index b2b9242..91d675f 100644
--- a/internal/cmd/testdata/getenvoy_run_help.txt
+++ b/internal/cmd/testdata/getenvoy_run_help.txt
@@ -16,7 +16,7 @@ DESCRIPTION:
    Envoy interprets the '[arguments...]' and runs in the current working
    directory (aka $CWD) until getenvoy is interrupted (ex Ctrl+C, Ctrl+Break).
    
-   Console output is captured as "stdout.log" and "stderr.log" in the run
-   directory (`$GETENVOY_HOME/runs/$epochtime`). When interrupted,
-   shutdown hooks write files, including admin endpoints, network and process
-   state. Upon exit, these zip into `$GETENVOY_HOME/runs/$epochtime.tar.gz`
+   Envoy's process ID and console output write to "envoy.pid", stdout.log" and
+   "stderr.log" in the run directory (`$GETENVOY_HOME/runs/$epochtime`).
+   When interrupted, shutdown hooks write files including network and process
+   state. On exit, these archive into `$GETENVOY_HOME/runs/$epochtime.tar.gz`
diff --git a/internal/envoy/run_test.go b/internal/envoy/run_test.go
index aa7e8ea..835bc55 100644
--- a/internal/envoy/run_test.go
+++ b/internal/envoy/run_test.go
@@ -21,9 +21,11 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"strconv"
 	"testing"
 	"time"
 
+	"github.com/shirou/gopsutil/v3/process"
 	"github.com/stretchr/testify/require"
 
 	"github.com/tetratelabs/getenvoy/internal/globals"
@@ -85,8 +87,17 @@ func TestRuntime_Run(t *testing.T) {
 			var haveShutdownHook bool
 			r.RegisterShutdownHook(func(_ context.Context) error {
 				pid := requireEnvoyPid(t, r)
-				_, err := os.FindProcess(pid)
+
+				// Validate envoy.pid was written
+				pidText, err := os.ReadFile(r.pidPath)
+				require.NoError(t, err)
+				require.Equal(t, strconv.Itoa(pid), string(pidText))
+				require.Greater(t, pid, 1)
+
+				// Ensure the process can still be looked up (ex it didn't die from accidental signal propagation)
+				_, err = process.NewProcess(int32(pid)) // because os.FindProcess is no-op in Linux!
 				require.NoError(t, err, "shutdownHook called after process shutdown")
+
 				haveShutdownHook = true
 				return nil
 			})
diff --git a/internal/envoy/runtime_test.go b/internal/envoy/runtime_test.go
index 2c7a016..9d30c36 100644
--- a/internal/envoy/runtime_test.go
+++ b/internal/envoy/runtime_test.go
@@ -101,3 +101,8 @@ func TestEnsureAdminAddressPath_ValidateExisting(t *testing.T) {
 		})
 	}
 }
+
+func TestPidFilePath(t *testing.T) {
+	r := NewRuntime(&globals.RunOpts{RunDir: "run"})
+	require.Equal(t, filepath.Join("run", "envoy.pid"), r.pidPath)
+}