Repository: buchgr/bazel-remote
Pull request #270 (2020-05-09 12:55:30)

skip a few more empty CAS blob lookups
Thanks to Ulrik Falklöf <ulrik.falklof@ericsson.com> for catching these.

Background + discussion of followup work:
https://github.com/buchgr/bazel-remote/pull/266


Patch:
diff --git a/cache/disk/disk.go b/cache/disk/disk.go
index 31fe6b4..c30aab6 100644
--- a/cache/disk/disk.go
+++ b/cache/disk/disk.go
@@ -657,7 +657,7 @@ func (c *Cache) GetValidatedActionResult(hash string) (*pb.ActionResult, []byte,
 		}
 
 		for _, f := range tree.Root.GetFiles() {
-			if f.Digest == nil {
+			if f.Digest == nil || f.Digest.SizeBytes == 0 {
 				continue
 			}
 			found, _ := c.Contains(cache.CAS, f.Digest.Hash)
@@ -668,7 +668,7 @@ func (c *Cache) GetValidatedActionResult(hash string) (*pb.ActionResult, []byte,
 
 		for _, child := range tree.GetChildren() {
 			for _, f := range child.GetFiles() {
-				if f.Digest == nil {
+				if f.Digest == nil || f.Digest.SizeBytes == 0 {
 					continue
 				}
 				found, _ := c.Contains(cache.CAS, f.Digest.Hash)


Test patch:
diff --git a/server/grpc_bytestream.go b/server/grpc_bytestream.go
index fe54528..fc3307c 100644
--- a/server/grpc_bytestream.go
+++ b/server/grpc_bytestream.go
@@ -87,6 +87,17 @@ func (s *grpcServer) Read(req *bytestream.ReadRequest,
 		return err
 	}
 
+	if size == 0 {
+		// While not technically an error, clients really shouldn't be
+		// asking for empty data with the bytestream API (IMO).
+		err = resp.Send(&bytestream.ReadResponse{Data: []byte{}})
+		if err != nil {
+			s.accessLogger.Printf("GRPC BYTESTREAM READ FAILED TO SEND RESPONSE: %s", err)
+			return status.Error(codes.Unknown, err.Error())
+		}
+		return nil
+	}
+
 	if req.ReadOffset > size {
 		msg := fmt.Sprintf("ReadOffset %d larger than expected data size %d resource: %s",
 			req.ReadOffset, size, req.ResourceName)
diff --git a/server/grpc_test.go b/server/grpc_test.go
index 0201b44..5768ec0 100644
--- a/server/grpc_test.go
+++ b/server/grpc_test.go
@@ -261,7 +261,7 @@ func TestGrpcAc(t *testing.T) {
 	}
 }
 
-func TestGrpcAcEmptySha256(t *testing.T) {
+func TestGrpcCasEmptySha256(t *testing.T) {
 
 	// Check that we can "download" an empty blob, even if it hasn't
 	// been uploaded.
@@ -316,6 +316,58 @@ func TestGrpcAcRequestInlinedBlobs(t *testing.T) {
 		SizeBytes: int64(len(stderrRaw)),
 	}
 
+	treeWithEmptyFile := pb.Tree{
+		Root: &pb.Directory{
+			Files: []*pb.FileNode{
+				{
+					Name: "emptyfile",
+					Digest: &pb.Digest{
+						Hash: emptySha256,
+					},
+				},
+			},
+		},
+		Children: []*pb.Directory{
+			&pb.Directory{
+				Files: []*pb.FileNode{
+					{
+						Name: "emptyfile",
+						Digest: &pb.Digest{
+							Hash: emptySha256,
+						},
+					},
+				},
+			},
+		},
+	}
+
+	treeWithEmptyFileData, err := proto.Marshal(&treeWithEmptyFile)
+	if err != nil {
+		t.Fatal(err)
+	}
+	treeHash := sha256.Sum256(treeWithEmptyFileData)
+
+	treeWithEmptyFileDigest := pb.Digest{
+		Hash:      hex.EncodeToString(treeHash[:]),
+		SizeBytes: int64(len(treeWithEmptyFileData)),
+	}
+
+	// Note that we're uploading the tree data, but not the empty file blob.
+	treeUpReq := pb.BatchUpdateBlobsRequest{
+		InstanceName: "foo",
+		Requests: []*pb.BatchUpdateBlobsRequest_Request{
+			{
+				Digest: &treeWithEmptyFileDigest,
+				Data:   treeWithEmptyFileData,
+			},
+		},
+	}
+
+	_, err = casClient.BatchUpdateBlobs(ctx, &treeUpReq)
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	ar := pb.ActionResult{
 		OutputFiles: []*pb.OutputFile{
 			{
@@ -332,6 +384,12 @@ func TestGrpcAcRequestInlinedBlobs(t *testing.T) {
 				//Contents: []byte{},
 			},
 		},
+		OutputDirectories: []*pb.OutputDirectory{
+			{
+				Path:       "somedir",
+				TreeDigest: &treeWithEmptyFileDigest,
+			},
+		},
 		StdoutRaw:    stdoutRaw,
 		StdoutDigest: &stdoutDigest,
 		StderrRaw:    stderrRaw,
@@ -416,6 +474,19 @@ func TestGrpcAcRequestInlinedBlobs(t *testing.T) {
 			t.Fatal("missing blob:", r.Digest)
 		}
 	}
+
+	// Triple-check that we can get the inlined results.
+	getAcReq := pb.GetActionResultRequest{
+		ActionDigest:      &arDigest,
+		InlineStdout:      true,
+		InlineStderr:      true,
+		InlineOutputFiles: []string{},
+	}
+
+	_, err = acClient.GetActionResult(ctx, &getAcReq)
+	if err != nil {
+		t.Fatal(err)
+	}
 }
 
 func TestGrpcByteStreamDeadline(t *testing.T) {
@@ -507,6 +578,38 @@ func TestGrpcByteStreamDeadline(t *testing.T) {
 	}
 }
 
+func TestGrpcByteStreamEmptySha256(t *testing.T) {
+	// We should always be able to read the empty blob.
+
+	resource := fmt.Sprintf("emptyRead/blobs/%s/0", emptySha256)
+	bsrReq := bytestream.ReadRequest{ResourceName: resource}
+
+	bsrc, err := bsClient.Read(ctx, &bsrReq)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	var downloadedBlob []byte
+	for {
+		bsrResp, err := bsrc.Recv()
+		if err == io.EOF {
+			break
+		}
+		if err != nil {
+			t.Fatal(err)
+		}
+		if bsrResp == nil {
+			t.Fatalf("Expected non-nil response")
+		}
+
+		downloadedBlob = append(downloadedBlob, bsrResp.Data...)
+
+		if len(downloadedBlob) > 0 {
+			t.Fatalf("Downloaded too much data")
+		}
+	}
+}
+
 func TestGrpcByteStream(t *testing.T) {
 
 	// Must be large enough to test multiple iterations of the
diff --git a/server/http_test.go b/server/http_test.go
index 301b5c4..a1241cd 100644
--- a/server/http_test.go
+++ b/server/http_test.go
@@ -281,6 +281,35 @@ func TestUploadEmptyActionResult(t *testing.T) {
 	}
 }
 
+func TestEmptyBlobAvailable(t *testing.T) {
+	testEmptyBlobAvailable(t, "HEAD")
+	testEmptyBlobAvailable(t, "GET")
+}
+
+func testEmptyBlobAvailable(t *testing.T, method string) {
+	cacheDir := testutils.TempDir(t)
+	defer os.RemoveAll(cacheDir)
+
+	data, hash := testutils.RandomDataAndHash(0)
+	r, err := http.NewRequest(method, "/cas/"+hash, bytes.NewReader(data))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	c := disk.New(cacheDir, 2048, nil)
+	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.StatusOK {
+		t.Fatal("Handler returned wrong status code",
+			"expected ", http.StatusOK,
+			"got ", status)
+	}
+}
+
 func TestStatusPage(t *testing.T) {
 	cacheDir := testutils.TempDir(t)
 	defer os.RemoveAll(cacheDir)