tests: avoid leaking child processes from agent/proxyprocess package

agent/proxyprocess/daemon_test.go

@@ -30,8 +30,11 @@ func TestDaemonStartStop(t *testing.T) {
 	uuid, err := uuid.GenerateUUID()
 	require.NoError(err)
 
+	cmd, destroy := helperProcess("start-stop", path)
+	defer destroy()
+
 	d := &Daemon{
-		Command:    helperProcess("start-stop", path),
+		Command:    cmd,
 		ProxyID:    "tubes",
 		ProxyToken: uuid,
 		Logger:     testLogger,
@@ -78,8 +81,11 @@ func TestDaemonRestart(t *testing.T) {
 	defer closer()
 	path := filepath.Join(td, "file")
 
+	cmd, destroy := helperProcess("restart", path)
+	defer destroy()
+
 	d := &Daemon{
-		Command: helperProcess("restart", path),
+		Command: cmd,
 		Logger:  testLogger,
 	}
 	require.NoError(d.Start())
@@ -117,7 +123,8 @@ func TestDaemonLaunchesNewProcessGroup(t *testing.T) {
 	// Start the parent process wrapping a start-stop test. The parent is acting
 	// as our "agent". We need an extra indirection to be able to kill the "agent"
 	// and still be running the test process.
-	parentCmd := helperProcess("parent", pidPath, "start-stop", path)
+	parentCmd, destroy := helperProcess("parent", pidPath, "start-stop", path)
+	defer destroy()
 
 	// We MUST run this as a separate process group otherwise the Kill below will
 	// kill this test process (and possibly your shell/editor that launched it!)
@@ -186,7 +193,9 @@ func TestDaemonLaunchesNewProcessGroup(t *testing.T) {
 
 	// Start a new parent that will "adopt" the existing child even though it will
 	// not be an actual child process.
-	fosterCmd := helperProcess("parent", pidPath, "start-stop", path)
+	fosterCmd, destroy := helperProcess("parent", pidPath, "start-stop", path)
+	defer destroy()
+
 	// Don't care about it being same process group this time as we will just kill
 	// it normally.
 	require.NoError(fosterCmd.Start())
@@ -246,6 +255,21 @@ func TestDaemonLaunchesNewProcessGroup(t *testing.T) {
 	// even harder!
 
 	// Let defer clean up the child process(es)
+
+	// Get the NEW child PID
+	bs, err = ioutil.ReadFile(pidPath)
+	require.NoError(err)
+	pid, err = strconv.Atoi(string(bs))
+	require.NoError(err)
+	proc2, err := os.FindProcess(pid)
+	require.NoError(err)
+
+	// Always cleanup child process after
+	defer func() {
+		if proc2 != nil {
+			proc2.Kill()
+		}
+	}()
 }
 
 func TestDaemonStop_kill(t *testing.T) {
@@ -257,8 +281,11 @@ func TestDaemonStop_kill(t *testing.T) {
 
 	path := filepath.Join(td, "file")
 
+	cmd, destroy := helperProcess("stop-kill", path)
+	defer destroy()
+
 	d := &Daemon{
-		Command:      helperProcess("stop-kill", path),
+		Command:      cmd,
 		ProxyToken:   "hello",
 		Logger:       testLogger,
 		gracefulWait: 200 * time.Millisecond,
@@ -316,14 +343,19 @@ func TestDaemonStop_killAdopted(t *testing.T) {
 	// ensure we are exercising that code path.
 
 	// Start the "child" process
-	childCmd := helperProcess("stop-kill", path)
+	childCmd, destroy := helperProcess("stop-kill", path)
+	defer destroy()
+
 	require.NoError(childCmd.Start())
 	go func() { childCmd.Wait() }() // Prevent it becoming a zombie when killed
 	defer func() { childCmd.Process.Kill() }()
 
 	// Create the Daemon
+	cmd, destroy := helperProcess("stop-kill", path)
+	defer destroy()
+
 	d := &Daemon{
-		Command:      helperProcess("stop-kill", path),
+		Command:      cmd,
 		ProxyToken:   "hello",
 		Logger:       testLogger,
 		gracefulWait: 200 * time.Millisecond,
@@ -380,8 +412,11 @@ func TestDaemonStart_pidFile(t *testing.T) {
 	uuid, err := uuid.GenerateUUID()
 	require.NoError(err)
 
+	cmd, destroy := helperProcess("start-once", path)
+	defer destroy()
+
 	d := &Daemon{
-		Command:    helperProcess("start-once", path),
+		Command:    cmd,
 		ProxyToken: uuid,
 		Logger:     testLogger,
 		PidPath:    pidPath,
@@ -422,8 +457,11 @@ func TestDaemonRestart_pidFile(t *testing.T) {
 	path := filepath.Join(td, "file")
 	pidPath := filepath.Join(td, "pid")
 
+	cmd, destroy := helperProcess("restart", path)
+	defer destroy()
+
 	d := &Daemon{
-		Command: helperProcess("restart", path),
+		Command: cmd,
 		Logger:  testLogger,
 		PidPath: pidPath,
 	}
@@ -618,8 +656,11 @@ func TestDaemonUnmarshalSnapshot(t *testing.T) {
 	uuid, err := uuid.GenerateUUID()
 	require.NoError(err)
 
+	cmd, destroy := helperProcess("start-stop", path)
+	defer destroy()
+
 	d := &Daemon{
-		Command:    helperProcess("start-stop", path),
+		Command:    cmd,
 		ProxyToken: uuid,
 		Logger:     testLogger,
 	}
@@ -676,8 +717,11 @@ func TestDaemonUnmarshalSnapshot_notRunning(t *testing.T) {
 	uuid, err := uuid.GenerateUUID()
 	require.NoError(err)
 
+	cmd, destroy := helperProcess("start-stop", path)
+	defer destroy()
+
 	d := &Daemon{
-		Command:    helperProcess("start-stop", path),
+		Command:    cmd,
 		ProxyToken: uuid,
 		Logger:     testLogger,
 	}

agent/proxyprocess/manager_test.go

@@ -43,7 +43,11 @@ func TestManagerRun_initialSync(t *testing.T) {
 	td, closer := testTempDir(t)
 	defer closer()
 	path := filepath.Join(td, "file")
-	testStateProxy(t, state, "web", helperProcess("restart", path))
+
+	cmd, destroy := helperProcess("restart", path)
+	defer destroy()
+
+	testStateProxy(t, state, "web", cmd)
 
 	// Start the manager
 	go m.Run()
@@ -78,7 +82,11 @@ func TestManagerRun_syncNew(t *testing.T) {
 	td, closer := testTempDir(t)
 	defer closer()
 	path := filepath.Join(td, "file")
-	testStateProxy(t, state, "web", helperProcess("restart", path))
+
+	cmd, destroy := helperProcess("restart", path)
+	defer destroy()
+
+	testStateProxy(t, state, "web", cmd)
 
 	// We should see the path appear shortly
 	retry.Run(t, func(r *retry.R) {
@@ -91,7 +99,11 @@ func TestManagerRun_syncNew(t *testing.T) {
 
 	// Add another proxy
 	path = path + "2"
-	testStateProxy(t, state, "db", helperProcess("restart", path))
+
+	cmd, destroy = helperProcess("restart", path)
+	defer destroy()
+
+	testStateProxy(t, state, "db", cmd)
 	retry.Run(t, func(r *retry.R) {
 		_, err := os.Stat(path)
 		if err == nil {
@@ -117,7 +129,11 @@ func TestManagerRun_syncDelete(t *testing.T) {
 	td, closer := testTempDir(t)
 	defer closer()
 	path := filepath.Join(td, "file")
-	id := testStateProxy(t, state, "web", helperProcess("restart", path))
+
+	cmd, destroy := helperProcess("restart", path)
+	defer destroy()
+
+	id := testStateProxy(t, state, "web", cmd)
 
 	// We should see the path appear shortly
 	retry.Run(t, func(r *retry.R) {
@@ -157,7 +173,11 @@ func TestManagerRun_syncUpdate(t *testing.T) {
 	td, closer := testTempDir(t)
 	defer closer()
 	path := filepath.Join(td, "file")
-	testStateProxy(t, state, "web", helperProcess("restart", path))
+
+	cmd, destroy := helperProcess("restart", path)
+	defer destroy()
+
+	testStateProxy(t, state, "web", cmd)
 
 	// We should see the path appear shortly
 	retry.Run(t, func(r *retry.R) {
@@ -171,7 +191,12 @@ func TestManagerRun_syncUpdate(t *testing.T) {
 	// Update the proxy with a new path
 	oldPath := path
 	path = path + "2"
-	testStateProxy(t, state, "web", helperProcess("restart", path))
+
+	cmd, destroy = helperProcess("restart", path)
+	defer destroy()
+
+	testStateProxy(t, state, "web", cmd)
+
 	retry.Run(t, func(r *retry.R) {
 		_, err := os.Stat(path)
 		if err == nil {
@@ -204,7 +229,11 @@ func TestManagerRun_daemonLogs(t *testing.T) {
 
 	// Create the service and calculate the log paths
 	path := filepath.Join(m.DataDir, "notify")
-	id := testStateProxy(t, state, "web", helperProcess("output", path))
+
+	cmd, destroy := helperProcess("output", path)
+	defer destroy()
+
+	id := testStateProxy(t, state, "web", cmd)
 	stdoutPath := logPath(logDir, id, "stdout")
 	stderrPath := logPath(logDir, id, "stderr")
 
@@ -244,7 +273,11 @@ func TestManagerRun_daemonPid(t *testing.T) {
 
 	// Create the service and calculate the log paths
 	path := filepath.Join(m.DataDir, "notify")
-	id := testStateProxy(t, state, "web", helperProcess("output", path))
+
+	cmd, destroy := helperProcess("output", path)
+	defer destroy()
+
+	id := testStateProxy(t, state, "web", cmd)
 	pidPath := pidPath(pidDir, id)
 
 	// Start the manager
@@ -280,7 +313,11 @@ func TestManagerPassesEnvironment(t *testing.T) {
 	td, closer := testTempDir(t)
 	defer closer()
 	path := filepath.Join(td, "env-variables")
-	testStateProxy(t, state, "environTest", helperProcess("environ", path))
+
+	cmd, destroy := helperProcess("environ", path)
+	defer destroy()
+
+	testStateProxy(t, state, "environTest", cmd)
 
 	//Run the manager
 	go m.Run()
@@ -331,7 +368,11 @@ func TestManagerPassesProxyEnv(t *testing.T) {
 	td, closer := testTempDir(t)
 	defer closer()
 	path := filepath.Join(td, "env-variables")
-	testStateProxy(t, state, "environTest", helperProcess("environ", path))
+
+	cmd, destroy := helperProcess("environ", path)
+	defer destroy()
+
+	testStateProxy(t, state, "environTest", cmd)
 
 	//Run the manager
 	go m.Run()
@@ -378,7 +419,11 @@ func TestManagerRun_snapshotRestore(t *testing.T) {
 	td, closer := testTempDir(t)
 	defer closer()
 	path := filepath.Join(td, "file")
-	testStateProxy(t, state, "web", helperProcess("start-stop", path))
+
+	cmd, destroy := helperProcess("start-stop", path)
+	defer destroy()
+
+	testStateProxy(t, state, "web", cmd)
 
 	// Set a low snapshot period so we get a snapshot
 	m.SnapshotPeriod = 10 * time.Millisecond
@@ -427,7 +472,12 @@ func TestManagerRun_snapshotRestore(t *testing.T) {
 	// Add a second proxy so that we can determine when we're up
 	// and running.
 	path2 := filepath.Join(td, "file2")
-	testStateProxy(t, state, "db", helperProcess("start-stop", path2))
+
+	cmd, destroy = helperProcess("start-stop", path2)
+	defer destroy()
+
+	testStateProxy(t, state, "db", cmd)
+
 	retry.Run(t, func(r *retry.R) {
 		_, err := os.Stat(path2)
 		if err == nil {
@@ -465,7 +515,11 @@ func TestManagerRun_rootDisallow(t *testing.T) {
 	td, closer := testTempDir(t)
 	defer closer()
 	path := filepath.Join(td, "file")
-	testStateProxy(t, state, "web", helperProcess("restart", path))
+
+	cmd, destroy := helperProcess("restart", path)
+	defer destroy()
+
+	testStateProxy(t, state, "web", cmd)
 
 	// Start the manager
 	go m.Run()

agent/proxyprocess/proxy_test.go

@@ -43,14 +43,19 @@ const helperProcessSentinel = "WANT_HELPER_PROCESS"
 // helperProcess returns an *exec.Cmd that can be used to execute the
 // TestHelperProcess function below. This can be used to test multi-process
 // interactions.
-func helperProcess(s ...string) *exec.Cmd {
+func helperProcess(s ...string) (*exec.Cmd, func()) {
 	cs := []string{"-test.run=TestHelperProcess", "--", helperProcessSentinel}
 	cs = append(cs, s...)
 
 	cmd := exec.Command(os.Args[0], cs...)
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
-	return cmd
+	destroy := func() {
+		if p := cmd.Process; p != nil {
+			p.Kill()
+		}
+	}
+	return cmd, destroy
 }
 
 // This is not a real test. This is just a helper process kicked off by tests
@@ -77,6 +82,8 @@ func TestHelperProcess(t *testing.T) {
 	// While running, this creates a file in the given directory (args[0])
 	// and deletes it only when it is stopped.
 	case "start-stop":
+		limitProcessLifetime(2 * time.Minute)
+
 		ch := make(chan os.Signal, 1)
 		signal.Notify(ch, os.Interrupt, syscall.SIGTERM)
 		defer signal.Stop(ch)
@@ -98,6 +105,8 @@ func TestHelperProcess(t *testing.T) {
 	// exists. When that file is removed, this process exits. This can be
 	// used to test restarting.
 	case "restart":
+		limitProcessLifetime(2 * time.Minute)
+
 		ch := make(chan os.Signal, 1)
 		signal.Notify(ch, os.Interrupt)
 		defer signal.Stop(ch)
@@ -127,6 +136,8 @@ func TestHelperProcess(t *testing.T) {
 			}
 		}
 	case "stop-kill":
+		limitProcessLifetime(2 * time.Minute)
+
 		// Setup listeners so it is ignored
 		ch := make(chan os.Signal, 1)
 		signal.Notify(ch, os.Interrupt)
@@ -142,6 +153,8 @@ func TestHelperProcess(t *testing.T) {
 		}
 		// Check if the external process can access the enivironmental variables
 	case "environ":
+		limitProcessLifetime(2 * time.Minute)
+
 		stop := make(chan os.Signal, 1)
 		signal.Notify(stop, os.Interrupt)
 		defer signal.Stop(stop)
@@ -172,6 +185,8 @@ func TestHelperProcess(t *testing.T) {
 		<-stop
 
 	case "output":
+		limitProcessLifetime(2 * time.Minute)
+
 		fmt.Fprintf(os.Stdout, "hello stdout\n")
 		fmt.Fprintf(os.Stderr, "hello stderr\n")
 
@@ -199,12 +214,17 @@ func TestHelperProcess(t *testing.T) {
 	// If the PID file already exists, it will "adopt" the child rather than
 	// launch a new one.
 	case "parent":
+		limitProcessLifetime(2 * time.Minute)
+
 		// We will write the PID for the child to the file in the first argument
 		// then pass rest of args through to command.
 		pidFile := args[0]
 
+		cmd, destroyChild := helperProcess(args[1:]...)
+		defer destroyChild()
+
 		d := &Daemon{
-			Command: helperProcess(args[1:]...),
+			Command: cmd,
 			Logger:  testLogger,
 			PidPath: pidFile,
 		}
@@ -251,3 +271,12 @@ func TestHelperProcess(t *testing.T) {
 		os.Exit(2)
 	}
 }
+
+// limitProcessLifetime installs a background goroutine that self-exits after
+// the specified duration elapses to prevent leaking processes from tests that
+// may spawn them.
+func limitProcessLifetime(dur time.Duration) {
+	go time.AfterFunc(dur, func() {
+		os.Exit(99)
+	})
+}