Fix exit codes not being respected by RunWithTimeout

We're assuming that `Process.Wait()` returns an error if the process exits
with an error code. `Cmd.Wait()` returns with an error value on a non-zero
exit code but `Process.Wait()` returns an error on OS-specific details (like
if the process isn't a child process) and instead we should be checking the
value of the `ProcessState.Success()` method.

Added two tests that ensure we don't miss this again.

commands/commands.go

@@ -2,6 +2,7 @@ package commands
 
 import (
 	"errors"
+	"fmt"
 	"io"
 	"os"
 	"os/exec"
@@ -195,14 +196,19 @@ func (c *Command) waitForTimeout() error {
 		}()
 	}
 	log.Debugf("%s.run waiting for PID %d: ", c.Name, cmd.Process.Pid)
-	if _, err := cmd.Process.Wait(); err != nil {
+	state, err := cmd.Process.Wait()
+	if err != nil {
 		if err.Error() == errNoChild {
 			log.Debugf(err.Error())
 			return nil // process exited cleanly before we hit wait4
 		}
 		log.Errorf("%s exited with error: %v", c.Name, err)
 		return err
 	}
+	if state != nil && !state.Success() {
+		return fmt.Errorf("%s exited with error", c.Name)
+	}
+
 	if doTimeout {
 		// if we send on this when we haven't set up the receiver
 		// we'll deadlock

commands/commands_test.go

@@ -77,6 +77,14 @@ func TestRunWithTimeout(t *testing.T) {
 	}
 }
 
+func TestRunWithTimeoutFailed(t *testing.T) {
+	cmd, _ := NewCommand("./testdata/test.sh failStuff --debug", "0")
+	fields := log.Fields{"process": "test"}
+	if err := RunWithTimeout(cmd, fields); err == nil {
+		t.Errorf("Expected error but got nil")
+	}
+}
+
 func TestEmptyCommand(t *testing.T) {
 	if cmd, err := NewCommand("", "0"); cmd != nil || err == nil {
 		t.Errorf("Expected exit (nil, err) but got %s, %s", cmd, err)

services/services_test.go

@@ -22,6 +22,16 @@ func TestHealthCheck(t *testing.T) {
 	}
 }
 
+func TestHealthCheckBad(t *testing.T) {
+	cmd1, _ := commands.NewCommand("./testdata/test.sh failStuff", "")
+	service := &Service{
+		healthCheckCmd: cmd1,
+	}
+	if err := service.CheckHealth(); err == nil {
+		t.Errorf("Expected error from CheckHealth but got nil")
+	}
+}
+
 type TestFragmentServices struct {
 	Services []Service
 }