Repository: buchgr/bazel-remote (go)

Issue:
Prune empty actions
We have a case where some empty actions managed to sneak into our `ac` cache. 

```
$ find ac -size 0 | head
ac/0a/0a372b8f6f57e408155b2d77291599fa69c3ad44c32a370579bc3fd582f462e6
ac/1d/1d16b14f8365aa77a51617086c6a6675fd216480223ffa02a5cbf33322570f8d
ac/87/875ac9c5592f5f8c86dc05d3c61d18a9d7aac85c0930b7b36290ba9f46fa3367
```

These are problematic because Bazel will fail the build when the outputs are not created. 

Would it make sense for `bazel-remote` to refuse to serve empty actions?

Pull request:
when validating uploads, reject empty ActionResults
If ActionCache validation is enabled, we refuse to serve empty
ActionResult blobs. We should go one step further and reject them
from being uploaded in the first place.

Fixes #103.

Patch:
diff --git a/cache/http/http.go b/cache/http/http.go
index 197762b..33f3ed5 100644
--- a/cache/http/http.go
+++ b/cache/http/http.go
@@ -54,6 +54,9 @@ func uploadFile(remote *http.Client, baseURL *url.URL, local cache.Cache, access
 	if err != nil {
 		return
 	}
+	io.Copy(ioutil.Discard, rsp.Body)
+	rsp.Body.Close()
+
 	logResponse(accessLogger, "PUT", rsp.StatusCode, url)
 	return
 }
diff --git a/server/grpc_ac.go b/server/grpc_ac.go
index 8ae1893..3dfd737 100644
--- a/server/grpc_ac.go
+++ b/server/grpc_ac.go
@@ -13,6 +13,11 @@ import (
 	"github.com/buchgr/bazel-remote/cache"
 )
 
+var (
+	errEmptyActionResult = status.Error(codes.InvalidArgument,
+		"rejecting empty ActionResult")
+)
+
 // ActionCache interface:
 
 func (s *grpcServer) GetActionResult(ctx context.Context,
@@ -57,6 +62,12 @@ func (s *grpcServer) UpdateActionResult(ctx context.Context,
 		return nil, status.Error(codes.Unknown, err.Error())
 	}
 
+	if len(data) == 0 {
+		s.accessLogger.Printf("%s %s %s", errorPrefix, req.ActionDigest.Hash,
+			errEmptyActionResult.Error())
+		return nil, errEmptyActionResult
+	}
+
 	err = s.cache.Put(cache.AC, req.ActionDigest.Hash,
 		int64(len(data)), bytes.NewReader(data))
 	if err != nil {
diff --git a/server/http.go b/server/http.go
index 2f70d03..00dac8a 100644
--- a/server/http.go
+++ b/server/http.go
@@ -196,7 +196,7 @@ func (h *httpCache) CacheHandler(w http.ResponseWriter, r *http.Request) {
 			// We need the content-length header to make sure we have enough disk space.
 			msg := fmt.Sprintf("PUT without Content-Length (key = %s)", path(kind, hash))
 			http.Error(w, msg, http.StatusBadRequest)
-			h.errorLogger.Printf("%s", msg)
+			h.errorLogger.Printf("PUT %s: %s", path(kind, hash), msg)
 			return
 		}
 
@@ -206,17 +206,25 @@ func (h *httpCache) CacheHandler(w http.ResponseWriter, r *http.Request) {
 
 			data, err := ioutil.ReadAll(rc)
 			if err != nil {
-				msg := "Failed to read request body"
-				http.Error(w, msg, http.StatusBadRequest) // find a better status
-				h.errorLogger.Printf("%s", msg)
+				msg := "failed to read request body"
+				http.Error(w, msg, http.StatusInternalServerError)
+				h.errorLogger.Printf("PUT %s: %s", path(kind, hash), msg)
+				return
+			}
+
+			if len(data) == 0 {
+				msg := "rejecting empty ActionResult"
+				http.Error(w, msg, http.StatusBadRequest)
+				h.errorLogger.Printf("PUT %s: %s", path(kind, hash), msg)
+				return
 			}
 
 			ar := &pb.ActionResult{}
 			err = proto.Unmarshal(data, ar)
 			if err != nil {
-				msg := "Put received an invalid ActionResult"
+				msg := "received an invalid ActionResult"
 				http.Error(w, msg, http.StatusBadRequest)
-				h.errorLogger.Printf("%s", msg)
+				h.errorLogger.Printf("PUT %s: %s", path(kind, hash), msg)
 				return
 			}
 


Test patch:
diff --git a/server/grpc_test.go b/server/grpc_test.go
index a65cf43..9cb00e0 100644
--- a/server/grpc_test.go
+++ b/server/grpc_test.go
@@ -185,6 +185,30 @@ func TestGrpcAc(t *testing.T) {
 		checkBadDigestErr(t, err, tc)
 	}
 
+	zeroActionResult := pb.ActionResult{}
+	zeroData, err := proto.Marshal(&zeroActionResult)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(zeroData) != 0 {
+		t.Fatal("expected a zero-sized test blob")
+	}
+	_, zeroHash := testutils.RandomDataAndHash(0)
+	zeroDigest := pb.Digest{
+		Hash:      zeroHash,
+		SizeBytes: 0,
+	}
+	zeroReq := pb.UpdateActionResultRequest{
+		ActionDigest: &zeroDigest,
+		ActionResult: &zeroActionResult,
+	}
+	_, err = acClient.UpdateActionResult(ctx, &zeroReq)
+	s, ok = status.FromError(err)
+	if !ok || s.Code() != codes.InvalidArgument ||
+		err.Error() != errEmptyActionResult.Error() {
+		t.Fatal("expected empty ActionResult to be rejected", err)
+	}
+
 	// GetActionResultRequest again, expect cache hit.
 
 	gacrResp, err = acClient.GetActionResult(ctx, &getReq)
diff --git a/server/http_test.go b/server/http_test.go
index ec99258..250c067 100644
--- a/server/http_test.go
+++ b/server/http_test.go
@@ -212,6 +212,47 @@ func TestUploadCorruptedFile(t *testing.T) {
 	}
 }
 
+func TestUploadEmptyActionResult(t *testing.T) {
+	cacheDir := testutils.CreateTmpCacheDirs(t)
+	defer os.RemoveAll(cacheDir)
+
+	data, hash := testutils.RandomDataAndHash(0)
+
+	r, err := http.NewRequest("PUT", "/ac/"+hash, bytes.NewReader(data))
+	if err != nil {
+		t.Error(err)
+	}
+
+	c := disk.New(cacheDir, 2048)
+	validate := true
+	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), validate)
+	rr := httptest.NewRecorder()
+	handler := http.HandlerFunc(h.CacheHandler)
+	handler.ServeHTTP(rr, r)
+
+	if status := rr.Code; status != http.StatusBadRequest {
+		t.Error("Handler returned wrong status code",
+			"expected ", http.StatusBadRequest,
+			"got ", status)
+	}
+
+	// Check that no file was saved in the cache
+	f, err := os.Open(cacheDir)
+	defer f.Close()
+	if err != nil {
+		t.Fatal(err)
+	}
+	entries, err := f.Readdir(-1)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, fileEntry := range entries {
+		if !fileEntry.IsDir() {
+			t.Error("Unexpected file in the cache ", fileEntry.Name())
+		}
+	}
+}
+
 func TestStatusPage(t *testing.T) {
 	cacheDir := testutils.CreateTmpCacheDirs(t)
 	defer os.RemoveAll(cacheDir)