Don't link failed builds to 'latest' link

NOTE: We use the terms 'link' and 'symlink' interchangeably.

BACKGROUND:

1. Builds that exit with a non-zero status code are (and should be)
   considered failed by mistry's semantics. Those builds are not linked to
   the 'latest' build, since we only want successful builds to serve as a
   cache for incremental building.

2. When a new build is submitted, we first check if there's an identical
   (i.e. same params) previous build already finished, so that we can
   short-circuit and use it's results (aka. "build result cache") instead of
   building again the same things. However, if that previous build was a
   failure, we remove it completely from the filesystem and proceed with
   the new build (essentially re-building).

BUG:

The code that determined if we should link a finished build to
'latest' (i.e. by checking if the build was successful), did NOT check
the exit code of the build's container. Therefore we would (erroneously)
link failed builds to 'latest', which violates (1).

Then if a subsequent identical build was submitted, we'd see
that the cached result is a failure and we'd remove the build (see (2)),
despite it being a link to 'latest'. This is obviously bad, since we
should never delete a build pointed to by the 'latest' link.

This would cause builds to fallback to non-incremental mode (i.e. start
with cold caches), even though they should be incremental.

FIX:

From now on, take into account the container command exit code to
determine if the build should be linked from the 'latest' link. If the
build exited with a non-zero status code, we do not link it.

cmd/mistry/main.go

@@ -287,7 +287,7 @@ EXAMPLES:
 					fmt.Printf("%s\n", body)
 				}
 
-				if bi.ExitCode != 0 {
+				if bi.ExitCode != types.ExitSuccess {
 					if bi.ContainerStderr != "" {
 						fmt.Fprintln(os.Stderr, "Container error logs:\n", bi.ContainerStderr)
 					} else {

cmd/mistryd/testdata/projects/failed-build-link/Dockerfile

@@ -0,0 +1,8 @@
+FROM debian:stretch
+
+COPY docker-entrypoint.sh /usr/local/bin/docker-entrypoint.sh
+RUN chmod +x /usr/local/bin/docker-entrypoint.sh
+
+WORKDIR /data
+
+ENTRYPOINT ["/usr/local/bin/docker-entrypoint.sh"]

cmd/mistryd/testdata/projects/failed-build-link/docker-entrypoint.sh

@@ -0,0 +1,2 @@
+#!/bin/bash
+exit `cat params/_exitcode`

cmd/mistryd/worker.go

@@ -131,14 +131,12 @@ func (s *Server) Work(ctx context.Context, j *Job) (buildInfo *types.BuildInfo,
 			}
 		}
 
-		// if the build was successful, symlink it to the 'latest'
-		// path
-		if err == nil {
+		// if build was successful, point 'latest' link to it
+		if err == nil && j.BuildInfo.ExitCode == types.ExitSuccess {
 			// eliminate concurrent filesystem operations since
 			// they could result in a corrupted state (eg. if
 			// jobs of the same project simultaneously finish
 			// successfully)
-
 			s.pq.Lock(j.Project)
 			defer s.pq.Unlock(j.Project)
 

cmd/mistryd/worker_test.go

@@ -68,3 +68,41 @@ func TestFailedPendingBuildCleanup(t *testing.T) {
 		}
 	}
 }
+
+// regression test for incremental building bug
+func TestBuildCacheWhenFailed(t *testing.T) {
+	group := "ppp"
+
+	// a successful build - it'll be symlinked
+	_, err := postJob(
+		types.JobRequest{Project: "failed-build-link",
+			Params: types.Params{"_exitcode": "0"},
+			Group:  group})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// a failed build - it should NOT be symlinked
+	_, err = postJob(
+		types.JobRequest{Project: "failed-build-link",
+			Params: types.Params{"_exitcode": "1", "foo": "bar"},
+			Group:  group})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// repeat the previous failed build - it
+	// SHOULD be incremental
+	buildInfo, err := postJob(
+		types.JobRequest{Project: "failed-build-link",
+			Params: types.Params{"_exitcode": "1", "foo": "bar"},
+			Group:  group})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if !buildInfo.Incremental {
+		t.Fatal("build should be incremental, but it isn't")
+	}
+
+}

pkg/types/build_info.go

@@ -8,6 +8,9 @@ import (
 // before even running the container
 const ContainerFailureExitCode = -999
 
+// ExitSuccess indicates that the build was successful.
+const ExitSuccess = 0
+
 // BuildInfo contains various information regarding the outcome of a
 // particular build.
 type BuildInfo struct {