Merge pull request #127 from taskcluster/bug1479415

Bug 1479415 - tasks executing a non-executable file should resolve as failed, not exception

helper_all-unix-style_test.go

@@ -143,3 +143,7 @@ func copyTestdataFileTo(src, dest string) [][]string {
 		},
 	}
 }
+
+func singleCommandNoArgs(command string) [][]string {
+	return [][]string{{command}}
+}

helper_windows_test.go

@@ -108,3 +108,7 @@ func copyTestdataFileTo(src, dest string) []string {
 		"copy \"" + sourceFile + "\" \"" + destFile + "\"",
 	}
 }
+
+func singleCommandNoArgs(command string) []string {
+	return []string{command}
+}

main_test.go

@@ -11,6 +11,7 @@ import (
 	"time"
 
 	"github.com/taskcluster/httpbackoff"
+	"github.com/taskcluster/slugid-go/slugid"
 )
 
 // Test failure should resolve as "failed"
@@ -200,3 +201,37 @@ func TestExecutionErrorsText(t *testing.T) {
 		t.FailNow()
 	}
 }
+
+// If a task tries to execute a command that doesn't exist, it should result in
+// a task failure, rather than a task exception, since the task is at fault,
+// not the worker.
+//
+// See https://bugzil.la/1479415
+func TestNonExistentCommandFailsTask(t *testing.T) {
+	defer setup(t)()
+	payload := GenericWorkerPayload{
+		Command:    singleCommandNoArgs(slugid.Nice()),
+		MaxRunTime: 10,
+	}
+	td := testTask(t)
+
+	_ = submitAndAssert(t, td, payload, "failed", "failed")
+}
+
+// If a task tries to execute a file that isn't executable for the current
+// user, it should result in a task failure, rather than a task exception,
+// since the task is at fault, not the worker.
+//
+// See https://bugzil.la/1479415
+func TestNonExecutableBinaryFailsTask(t *testing.T) {
+	defer setup(t)()
+	commands := copyTestdataFile("public-openpgp-key")
+	commands = append(commands, singleCommandNoArgs(filepath.Join(taskContext.TaskDir, "public-openpgp-key"))...)
+	payload := GenericWorkerPayload{
+		Command:    commands,
+		MaxRunTime: 10,
+	}
+	td := testTask(t)
+
+	_ = submitAndAssert(t, td, payload, "failed", "failed")
+}

process/process_all-unix-style.go

@@ -31,23 +31,51 @@ type Result struct {
 }
 
 func (r *Result) Succeeded() bool {
-	return r.SystemError == nil && r.ExitError == nil
+	return r.SystemError == nil && r.ExitError == nil && !r.Aborted
 }
 
+// Unlike on Windows, a system error is grounds for a task failure, rather than
+// a task exception, since it can be caused by e.g. a task trying to execute a
+// command that doesn't exist, or trying to execute a file that isn't
+// executable. Therefore a system error, or an exit error (process ran but
+// returned non-zero exit code) or a task abortion are all task failures.
+//
+// See https://bugzil.la/1479415
 func (r *Result) Failed() bool {
-	return (r.SystemError == nil && r.ExitError != nil) || r.Aborted
+	return r.SystemError != nil || r.ExitError != nil || r.Aborted
 }
 
+// Unlike on Windows, if there is a system error, we don't crash the worker,
+// since this can be caused by task that tries to execute a non-existing
+// command or a file that isn't executable. On Windows all commands are
+// wrapped in a command shell execution, where not being able to execute a
+// shell should cause the worker to panic.
 func (r *Result) CrashCause() error {
-	return r.SystemError
+	return nil
 }
 
-func (r *Result) FailureCause() *exec.ExitError {
-	return r.ExitError
+func (r *Result) FailureCause() error {
+	if r.Aborted {
+		return fmt.Errorf("Task was aborted")
+	}
+	if r.ExitError != nil {
+		return r.ExitError
+	}
+	if r.SystemError != nil {
+		return r.SystemError
+	}
+	return nil
 }
 
+// Unlike on Windows, if there is a system error, we don't crash the worker,
+// since this can be caused by task that tries to execute a non-existing
+// command or a file that isn't executable. On Windows all commands are
+// wrapped in a command shell execution, where not being able to execute a
+// shell should cause the worker to panic.
+//
+// See https://bugzil.la/1479415
 func (r *Result) Crashed() bool {
-	return r.SystemError != nil && !r.Aborted
+	return false
 }
 
 func NewCommand(commandLine []string, workingDirectory string, env []string) (*Command, error) {