diff --git a/agent/proxyprocess/daemon_test.go b/agent/proxyprocess/daemon_test.go index 0fbfb2f127..f37cf1194b 100644 --- a/agent/proxyprocess/daemon_test.go +++ b/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, } diff --git a/agent/proxyprocess/manager_test.go b/agent/proxyprocess/manager_test.go index 2737b85fb7..b277a245d6 100644 --- a/agent/proxyprocess/manager_test.go +++ b/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() diff --git a/agent/proxyprocess/proxy_test.go b/agent/proxyprocess/proxy_test.go index 9db343680b..e0d04792ed 100644 --- a/agent/proxyprocess/proxy_test.go +++ b/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) + }) +}