Repository: buchgr/bazel-remote
Pull request #126 (2019-11-25 06:55:14)

use sync.Mutex instead of sync.RWMutex
Apart from CurrentSize and NumItems (which are used by the http status
page), we only lock this mutex for rw, never for reading only. So let's
use the simpler sync.Mutex type.

Also, use the de-facto standard variable name for mutexes in Go code, "mu".

buchgr: Good catch. I think we should probably use a different lock for reading and writing though?
mostynb: > Good catch. I think we should probably use a different lock for reading and writing though?

CurrentSize and NumItems are the only methods that would be able to use a read-only lock, and they're only called from the http status page. I assume the status page is loaded relatively infrequently so it might be better to just use a simple Mutex, which I assume is slightly faster than RWMutex for write-heavy workloads like we have.

MaxSize acquires the lock but only reads from a field that is never modified, it may as well just read
the field without locking. I will push a new commit to remove it.
mostynb: I tried to benchmark sync.Mutex vs sync.RWMutex's write lock to confirm my assumption (RWMutex should be slower in write-heavy scenarios). I used BenchmarkMutexWrite from this stack overflow question:
https://stackoverflow.com/questions/57562606/why-does-sync-mutex-largely-drop-performance-when-goroutine-contention-is-more-t

Then I changed it to use sync.RWMutex instead and compared the results with benchstat. The "old" column here uses sync.Mutex and "new" uses sync.RWMutex:

name                          old time/op  new time/op  delta
MutexWrite/goroutines-2400-8   163ns ± 3%   261ns ± 2%   +59.68%  (p=0.008 n=5+5)
MutexWrite/goroutines-2480-8   165ns ± 3%   273ns ± 3%   +65.45%  (p=0.008 n=5+5)
MutexWrite/goroutines-2560-8   170ns ± 4%   292ns ± 2%   +71.88%  (p=0.008 n=5+5)
MutexWrite/goroutines-2640-8   181ns ± 7%   366ns ± 1%  +102.10%  (p=0.008 n=5+5)
MutexWrite/goroutines-2720-8   317ns ± 2%   372ns ± 1%   +17.34%  (p=0.008 n=5+5)
MutexWrite/goroutines-2800-8   322ns ± 2%   376ns ± 6%   +16.75%  (p=0.008 n=5+5)
MutexWrite/goroutines-2880-8   318ns ± 2%   366ns ± 6%   +15.03%  (p=0.008 n=5+5)
MutexWrite/goroutines-2960-8   318ns ± 2%   370ns ± 3%   +16.50%  (p=0.008 n=5+5)
MutexWrite/goroutines-3040-8   326ns ± 3%   362ns ± 2%   +11.17%  (p=0.008 n=5+5)
MutexWrite/goroutines-3120-8   332ns ± 1%   367ns ± 1%   +10.61%  (p=0.008 n=5+5)
MutexWrite/goroutines-3200-8   339ns ± 1%   373ns ± 1%   +10.15%  (p=0.008 n=5+5)
MutexWrite/goroutines-3280-8   354ns ± 2%   383ns ± 1%    +8.43%  (p=0.008 n=5+5)
MutexWrite/goroutines-3360-8   356ns ± 1%   394ns ± 3%   +10.72%  (p=0.008 n=5+5)
MutexWrite/goroutines-3440-8   369ns ± 2%   411ns ± 2%   +11.33%  (p=0.008 n=5+5)
MutexWrite/goroutines-3520-8   375ns ± 3%   409ns ± 2%    +9.00%  (p=0.008 n=5+5)
MutexWrite/goroutines-3600-8   376ns ± 4%   431ns ± 1%   +14.69%  (p=0.008 n=5+5)
MutexWrite/goroutines-3680-8   383ns ± 6%   421ns ± 1%    +9.86%  (p=0.008 n=5+5)
MutexWrite/goroutines-3760-8   364ns ± 7%   431ns ± 1%   +18.40%  (p=0.008 n=5+5)
MutexWrite/goroutines-3840-8   356ns ± 6%   437ns ± 2%   +22.88%  (p=0.008 n=5+5)
MutexWrite/goroutines-3920-8   357ns ± 2%   428ns ± 1%   +19.76%  (p=0.008 n=5+5)
MutexWrite/goroutines-4000-8   362ns ± 4%   396ns ± 2%    +9.44%  (p=0.008 n=5+5)
MutexWrite/goroutines-4080-8   366ns ± 4%   413ns ± 3%   +13.02%  (p=0.008 n=5+5)
MutexWrite/goroutines-4160-8   371ns ± 4%   411ns ± 3%   +10.61%  (p=0.008 n=5+5)
MutexWrite/goroutines-4240-8   370ns ± 3%   413ns ± 3%   +11.39%  (p=0.008 n=5+5)
MutexWrite/goroutines-4320-8   380ns ± 1%   411ns ± 3%    +8.27%  (p=0.008 n=5+5)
MutexWrite/goroutines-4400-8   392ns ± 2%   421ns ± 6%    +7.29%  (p=0.008 n=5+5)
MutexWrite/goroutines-4480-8   388ns ± 2%   420ns ± 4%    +8.19%  (p=0.008 n=5+5)
MutexWrite/goroutines-4560-8   392ns ± 2%   427ns ± 1%    +8.87%  (p=0.008 n=5+5)
MutexWrite/goroutines-4640-8   398ns ± 1%   431ns ± 2%    +8.24%  (p=0.008 n=5+5)
MutexWrite/goroutines-4720-8   411ns ± 3%   443ns ± 3%    +7.84%  (p=0.008 n=5+5)

Patch:
diff --git a/cache/disk/disk.go b/cache/disk/disk.go
index d9d1a9c..5abc9a9 100644
--- a/cache/disk/disk.go
+++ b/cache/disk/disk.go
@@ -31,7 +31,7 @@ func (i *lruItem) Size() int64 {
 // diskCache is an implementation of the cache backed by files on a filesystem.
 type diskCache struct {
 	dir string
-	mux *sync.RWMutex
+	mu  *sync.Mutex
 	lru SizedLRU
 }
 
@@ -71,7 +71,7 @@ func New(dir string, maxSizeBytes int64) cache.Cache {
 
 	cache := &diskCache{
 		dir: filepath.Clean(dir),
-		mux: &sync.RWMutex{},
+		mu:  &sync.Mutex{},
 		lru: NewSizedLRU(maxSizeBytes, onEvict),
 	}
 
@@ -158,7 +158,7 @@ func (c *diskCache) loadExistingFiles() error {
 }
 
 func (c *diskCache) Put(kind cache.EntryKind, hash string, expectedSize int64, r io.Reader) error {
-	c.mux.Lock()
+	c.mu.Lock()
 
 	key := cacheKey(kind, hash)
 
@@ -168,7 +168,7 @@ func (c *diskCache) Put(kind cache.EntryKind, hash string, expectedSize int64, r
 	// condition on the bazel side) but it's useful to overwrite poisoned items.
 	if existingItem, found := c.lru.Get(key); found {
 		if !existingItem.(*lruItem).committed {
-			c.mux.Unlock()
+			c.mu.Unlock()
 			io.Copy(ioutil.Discard, r)
 			return nil
 		}
@@ -180,7 +180,7 @@ func (c *diskCache) Put(kind cache.EntryKind, hash string, expectedSize int64, r
 		committed: false,
 	}
 	ok := c.lru.Add(key, newItem)
-	c.mux.Unlock()
+	c.mu.Unlock()
 	if !ok {
 		return &cache.Error{
 			Code: http.StatusInsufficientStorage,
@@ -193,8 +193,8 @@ func (c *diskCache) Put(kind cache.EntryKind, hash string, expectedSize int64, r
 	// but this stuff is really easy to get wrong without defer().
 	shouldCommit := false
 	defer func() {
-		c.mux.Lock()
-		defer c.mux.Unlock()
+		c.mu.Lock()
+		defer c.mu.Unlock()
 
 		if shouldCommit {
 			newItem.committed = true
@@ -282,8 +282,8 @@ func (c *diskCache) Get(kind cache.EntryKind, hash string) (rdr io.ReadCloser, s
 }
 
 func (c *diskCache) Contains(kind cache.EntryKind, hash string) (ok bool) {
-	c.mux.Lock()
-	defer c.mux.Unlock()
+	c.mu.Lock()
+	defer c.mu.Unlock()
 
 	val, found := c.lru.Get(cacheKey(kind, hash))
 	// Uncommitted (i.e. uploading items) should be reported as not ok
@@ -291,20 +291,19 @@ func (c *diskCache) Contains(kind cache.EntryKind, hash string) (ok bool) {
 }
 
 func (c *diskCache) NumItems() int {
-	c.mux.Lock()
-	defer c.mux.Unlock()
+	c.mu.Lock()
+	defer c.mu.Unlock()
 	return c.lru.Len()
 }
 
 func (c *diskCache) MaxSize() int64 {
-	c.mux.Lock()
-	defer c.mux.Unlock()
+	// The underlying value is never modified, no need to lock.
 	return c.lru.MaxSize()
 }
 
 func (c *diskCache) CurrentSize() int64 {
-	c.mux.Lock()
-	defer c.mux.Unlock()
+	c.mu.Lock()
+	defer c.mu.Unlock()
 	return c.lru.CurrentSize()
 }
 
diff --git a/server/BUILD.bazel b/server/BUILD.bazel
index d8e9119..8149ba7 100644
--- a/server/BUILD.bazel
+++ b/server/BUILD.bazel
@@ -22,6 +22,7 @@ go_library(
         "@org_golang_google_grpc//:go_default_library",
         "@org_golang_google_grpc//codes:go_default_library",
         "@org_golang_google_grpc//encoding/gzip:go_default_library",
+        "@org_golang_google_grpc//peer:go_default_library",
         "@org_golang_google_grpc//status:go_default_library",
     ],
 )
diff --git a/server/grpc_ac.go b/server/grpc_ac.go
index 3dfd737..4ed802f 100644
--- a/server/grpc_ac.go
+++ b/server/grpc_ac.go
@@ -3,21 +3,35 @@ package server
 import (
 	"bytes"
 	"context"
+	"crypto/sha256"
+	"encoding/hex"
 	"fmt"
+	"net"
+	"strings"
 
 	pb "github.com/bazelbuild/remote-apis/build/bazel/remote/execution/v2"
 	"github.com/golang/protobuf/proto"
 	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/peer"
 	"google.golang.org/grpc/status"
 
 	"github.com/buchgr/bazel-remote/cache"
 )
 
 var (
-	errEmptyActionResult = status.Error(codes.InvalidArgument,
+	// This is an Internal error rather than InvalidArgument because
+	// we modify incoming ActionResults to make them non-zero.
+	errEmptyActionResult = status.Error(codes.Internal,
 		"rejecting empty ActionResult")
 )
 
+const (
+	// gRPC by default rejects messages larger than 4M.
+	// Inline a little less than this, enough so we don't
+	// need to worry about serialization overhead.
+	maxInlineSize = 3 * 1024 * 1024 // 3M
+)
+
 // ActionCache interface:
 
 func (s *grpcServer) GetActionResult(ctx context.Context,
@@ -42,11 +56,98 @@ func (s *grpcServer) GetActionResult(ctx context.Context,
 			fmt.Sprintf("%s not found in AC", req.ActionDigest.Hash))
 	}
 
+	// Don't inline stdout/stderr/output files unless they were requested.
+
+	var inlinedSoFar int64
+
+	err = s.maybeInline(req.InlineStdout,
+		&result.StdoutRaw, &result.StdoutDigest, &inlinedSoFar)
+	if err != nil {
+		s.accessLogger.Printf("%s %s %s", errorPrefix, req.ActionDigest.Hash, err)
+		return nil, status.Error(codes.Unknown, err.Error())
+	}
+
+	err = s.maybeInline(req.InlineStderr,
+		&result.StderrRaw, &result.StderrDigest, &inlinedSoFar)
+	if err != nil {
+		s.accessLogger.Printf("%s %s %s", errorPrefix, req.ActionDigest.Hash, err)
+		return nil, status.Error(codes.Unknown, err.Error())
+	}
+
+	inlinableFiles := make(map[string]struct{}, len(req.InlineOutputFiles))
+	for _, p := range req.InlineOutputFiles {
+		inlinableFiles[p] = struct{}{}
+	}
+	for _, of := range result.GetOutputFiles() {
+		_, ok := inlinableFiles[of.Path]
+		err = s.maybeInline(ok, &of.Contents, &of.Digest, &inlinedSoFar)
+		if err != nil {
+			s.accessLogger.Printf("%s %s %s", errorPrefix, req.ActionDigest.Hash, err)
+			return nil, status.Error(codes.Unknown, err.Error())
+		}
+	}
+
 	s.accessLogger.Printf("GRPC AC GET %s OK", req.ActionDigest.Hash)
 
 	return result, nil
 }
 
+func (s *grpcServer) maybeInline(inline bool, slice *[]byte, digest **pb.Digest, inlinedSoFar *int64) error {
+
+	if (*inlinedSoFar + int64(len(*slice))) > maxInlineSize {
+		inline = false
+	} else if digest != nil && *digest != nil &&
+		(*inlinedSoFar+(*digest).SizeBytes) > maxInlineSize {
+		inline = false
+	}
+
+	if !inline {
+		if len(*slice) == 0 {
+			return nil // Not inlined, nothing to do.
+		}
+
+		if *digest == nil {
+			hash := sha256.Sum256(*slice)
+			*digest = &pb.Digest{
+				Hash:      hex.EncodeToString(hash[:]),
+				SizeBytes: int64(len(*slice)),
+			}
+		}
+
+		if !s.cache.Contains(cache.CAS, (*digest).Hash) {
+			err := s.cache.Put(cache.CAS, (*digest).Hash, (*digest).SizeBytes,
+				bytes.NewReader(*slice))
+			if err != nil {
+				return err
+			}
+		}
+
+		*slice = []byte{}
+		return nil
+	}
+
+	if len(*slice) > 0 {
+		*inlinedSoFar += int64(len(*slice))
+		return nil // Already inlined.
+	}
+
+	if digest == nil || *digest == nil {
+		return nil // Nothing to inline?
+	}
+
+	// Otherwise, attempt to inline.
+	if (*digest).SizeBytes > 0 {
+		data, err := s.getBlobData((*digest).Hash, (*digest).SizeBytes)
+		if err != nil {
+			return err
+		}
+		*slice = data
+		*inlinedSoFar += (*digest).SizeBytes
+	}
+
+	return nil
+}
+
 func (s *grpcServer) UpdateActionResult(ctx context.Context,
 	req *pb.UpdateActionResultRequest) (*pb.ActionResult, error) {
 
@@ -56,6 +157,9 @@ func (s *grpcServer) UpdateActionResult(ctx context.Context,
 		return nil, err
 	}
 
+	// Ensure that the serialized ActionResult has non-zero length.
+	addWorkerMetadataGRPC(ctx, req.ActionResult)
+
 	data, err := proto.Marshal(req.ActionResult)
 	if err != nil {
 		s.accessLogger.Printf("%s %s %s", errorPrefix, req.ActionDigest.Hash, err)
@@ -82,3 +186,38 @@ func (s *grpcServer) UpdateActionResult(ctx context.Context,
 	// https://cloud.google.com/apis/design/standard_methods
 	return req.ActionResult, nil
 }
+
+func addWorkerMetadataGRPC(ctx context.Context, ar *pb.ActionResult) {
+	if ar.ExecutionMetadata == nil {
+		ar.ExecutionMetadata = &pb.ExecutedActionMetadata{}
+	} else if ar.ExecutionMetadata.Worker != "" {
+		return
+	}
+
+	p, ok := peer.FromContext(ctx)
+	if !ok {
+		ar.ExecutionMetadata.Worker = "unknown"
+		return
+	}
+
+	addr := p.Addr.String()
+
+	if addr == "" {
+		ar.ExecutionMetadata.Worker = "unknown"
+		return
+	}
+
+	if !strings.ContainsAny(addr, ":") {
+		// The addr in our unit tests is "bufconn".
+		ar.ExecutionMetadata.Worker = addr
+		return
+	}
+
+	worker, _, err := net.SplitHostPort(addr)
+	if err != nil {
+		ar.ExecutionMetadata.Worker = addr
+		return
+	}
+
+	ar.ExecutionMetadata.Worker = worker
+}
diff --git a/server/http.go b/server/http.go
index f9a15f5..5e61dc4 100644
--- a/server/http.go
+++ b/server/http.go
@@ -199,6 +199,7 @@ func (h *httpCache) CacheHandler(w http.ResponseWriter, r *http.Request) {
 			h.errorLogger.Printf("PUT %s: %s", path(kind, hash), msg)
 			return
 		}
+		contentLength := r.ContentLength
 
 		rc := r.Body
 		if h.validateAC && kind == cache.AC {
@@ -212,21 +213,22 @@ func (h *httpCache) CacheHandler(w http.ResponseWriter, r *http.Request) {
 				return
 			}
 
-			if len(data) == 0 {
-				msg := "rejecting empty ActionResult"
+			if int64(len(data)) != contentLength {
+				msg := fmt.Sprintf("sizes don't match. Expected %d, found %d",
+					contentLength, len(data))
 				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)
+			// Ensure that the serialized ActionResult has non-zero length.
+			data, code, err := addWorkerMetadataHTTP(r.RemoteAddr, data)
 			if err != nil {
-				msg := "received an invalid ActionResult"
-				http.Error(w, msg, http.StatusBadRequest)
-				h.errorLogger.Printf("PUT %s: %s", path(kind, hash), msg)
+				http.Error(w, err.Error(), code)
+				h.errorLogger.Printf("PUT %s: %s", path(kind, hash), err.Error())
 				return
 			}
+			contentLength = int64(len(data))
 
 			// Note: we do not currently verify that the blobs exist
 			// in the CAS.
@@ -234,7 +236,7 @@ func (h *httpCache) CacheHandler(w http.ResponseWriter, r *http.Request) {
 			rc = ioutil.NopCloser(bytes.NewReader(data))
 		}
 
-		err := h.cache.Put(kind, hash, r.ContentLength, rc)
+		err := h.cache.Put(kind, hash, contentLength, rc)
 		if err != nil {
 			if cerr, ok := err.(*cache.Error); ok {
 				http.Error(w, err.Error(), cerr.Code)
@@ -271,6 +273,37 @@ func (h *httpCache) CacheHandler(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
+func addWorkerMetadataHTTP(addr string, orig []byte) (data []byte, code int, err error) {
+	ar := &pb.ActionResult{}
+	err = proto.Unmarshal(orig, ar)
+	if err != nil {
+		return orig, http.StatusBadRequest, err
+	}
+
+	if ar.ExecutionMetadata == nil {
+		ar.ExecutionMetadata = &pb.ExecutedActionMetadata{}
+	} else if ar.ExecutionMetadata.Worker != "" {
+		return orig, http.StatusOK, nil
+	}
+
+	worker := addr
+	if worker == "" {
+		worker, _, err = net.SplitHostPort(addr)
+		if err != nil || worker == "" {
+			worker = "unknown"
+		}
+	}
+
+	ar.ExecutionMetadata.Worker = worker
+
+	data, err = proto.Marshal(ar)
+	if err != nil {
+		return orig, http.StatusInternalServerError, err
+	}
+
+	return data, http.StatusOK, nil
+}
+
 // Produce a debugging page with some stats about the cache.
 func (h *httpCache) StatusPageHandler(w http.ResponseWriter, r *http.Request) {
 	defer r.Body.Close()


Test patch:
diff --git a/server/grpc_test.go b/server/grpc_test.go
index 9cb00e0..af96d92 100644
--- a/server/grpc_test.go
+++ b/server/grpc_test.go
@@ -173,6 +173,16 @@ func TestGrpcAc(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	// We expect the returned metadata to have changed.
+	if upACResp.ExecutionMetadata == nil {
+		t.Fatal("Error: expected ExecutionMetadata to be non-nil")
+	}
+	if upACResp.ExecutionMetadata.Worker != "bufconn" {
+		t.Fatal("Error: expected ExecutionMetadata.Worker to be set")
+	}
+	// Remove the metadata so we can compare with the request.
+	upACResp.ExecutionMetadata = nil
+
 	if !proto.Equal(&ar, upACResp) {
 		t.Fatal("Error: uploaded and returned ActionResult differ")
 	}
@@ -202,11 +212,23 @@ func TestGrpcAc(t *testing.T) {
 		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)
+	zeroResp, err := acClient.UpdateActionResult(ctx, &zeroReq)
+	if proto.Equal(&zeroReq, zeroResp) {
+		t.Fatal("expected non-zero ActionResult to be returned")
+	}
+
+	// We expect the returned metadata to have changed.
+	if zeroResp.ExecutionMetadata == nil {
+		t.Fatal("Error: expected ExecutionMetadata to be non-nil")
+	}
+	if zeroResp.ExecutionMetadata.Worker != "bufconn" {
+		t.Fatal("Error: expected ExecutionMetadata.Worker to be set")
+	}
+	// Remove the metadata so we can compare with the request.
+	zeroResp.ExecutionMetadata = nil
+
+	if !proto.Equal(zeroReq.ActionResult, zeroResp) {
+		t.Fatal("expected returned ActionResult to only differ by ExecutionMetadata")
 	}
 
 	// GetActionResultRequest again, expect cache hit.
@@ -216,6 +238,16 @@ func TestGrpcAc(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	// We expect the returned metadata to have changed.
+	if gacrResp.ExecutionMetadata == nil {
+		t.Fatal("Error: expected ExecutionMetadata to be non-nil")
+	}
+	if gacrResp.ExecutionMetadata.Worker != "bufconn" {
+		t.Fatal("Error: expected ExecutionMetadata.Worker to be set")
+	}
+	// Remove the metadata so we can compare with the request.
+	gacrResp.ExecutionMetadata = nil
+
 	if !proto.Equal(&ar, gacrResp) {
 		t.Fatal("Error: uploaded and returned ActionResult differ")
 	}
diff --git a/server/http_test.go b/server/http_test.go
index 3811c61..9d72e80 100644
--- a/server/http_test.go
+++ b/server/http_test.go
@@ -17,9 +17,11 @@ import (
 	"testing"
 
 	"github.com/buchgr/bazel-remote/cache"
-
 	"github.com/buchgr/bazel-remote/cache/disk"
 	"github.com/buchgr/bazel-remote/utils"
+
+	pb "github.com/bazelbuild/remote-apis/build/bazel/remote/execution/v2"
+	"github.com/golang/protobuf/proto"
 )
 
 func TestDownloadFile(t *testing.T) {
@@ -220,7 +222,7 @@ func TestUploadEmptyActionResult(t *testing.T) {
 
 	r, err := http.NewRequest("PUT", "/ac/"+hash, bytes.NewReader(data))
 	if err != nil {
-		t.Error(err)
+		t.Fatal(err)
 	}
 
 	c := disk.New(cacheDir, 2048)
@@ -230,26 +232,37 @@ func TestUploadEmptyActionResult(t *testing.T) {
 	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,
+	if status := rr.Code; status != http.StatusOK {
+		t.Fatal("Handler returned wrong status code",
+			"expected ", http.StatusOK,
 			"got ", status)
 	}
 
-	// Check that no file was saved in the cache
-	f, err := os.Open(cacheDir)
-	defer f.Close()
+	cacheFile := filepath.Join(cacheDir, "ac", hash[:2], hash)
+	cachedData, err := ioutil.ReadFile(cacheFile)
 	if err != nil {
 		t.Fatal(err)
 	}
-	entries, err := f.Readdir(-1)
+	if len(cachedData) == 0 {
+		t.Fatal("expected non-zero length ActionResult to be cached")
+	}
+
+	ar := pb.ActionResult{}
+	err = proto.Unmarshal(cachedData, &ar)
 	if err != nil {
 		t.Fatal(err)
 	}
-	for _, fileEntry := range entries {
-		if !fileEntry.IsDir() {
-			t.Error("Unexpected file in the cache ", fileEntry.Name())
-		}
+	if ar.ExecutionMetadata == nil {
+		t.Fatal("expected non-nil ExecutionMetadata")
+	}
+	ar.ExecutionMetadata = nil
+
+	remarshaled, err := proto.Marshal(&ar)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(remarshaled) != 0 {
+		t.Fatal("expected zero-length blob once the metadata is stripped")
 	}
 }