From 8b825e2377ef2d803e003dd02fc1b3f21c568317 Mon Sep 17 00:00:00 2001
From: Tim Gross <tim@0x74696d.com>
Date: Fri, 30 Sep 2016 08:55:29 -0400
Subject: [PATCH] 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      |  8 +++++++-
 commands/commands_test.go |  8 ++++++++
 services/services_test.go | 10 ++++++++++
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/commands/commands.go b/commands/commands.go
index cea0b032..9a2f0877 100644
--- a/commands/commands.go
+++ b/commands/commands.go
@@ -2,6 +2,7 @@ package commands
 
 import (
 	"errors"
+	"fmt"
 	"io"
 	"os"
 	"os/exec"
@@ -195,7 +196,8 @@ 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
@@ -203,6 +205,10 @@ func (c *Command) waitForTimeout() error {
 		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
diff --git a/commands/commands_test.go b/commands/commands_test.go
index 0795b19d..7e230729 100644
--- a/commands/commands_test.go
+++ b/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)
diff --git a/services/services_test.go b/services/services_test.go
index ae9cea78..a8326569 100644
--- a/services/services_test.go
+++ b/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
 }