Repository: buchgr/bazel-remote (go)

Issue:
incorrect ActionResult validation
I recently added some ActionResult validation, including one check that seems to be technically incorrect but still useful in practice: zero-length serialized ActionResult messages are currently considered invalid.

This is technically incorrect, because an ActionResult might legitimately have no outputs apart from the exit code (and if that exit code is 0 then the serialized value could be zero-length). One possible use case for this is a sanity check.  But this is bad, because a zero-length file is perhaps the most common form of data corruption, and we have no way to recognise when this is corruption and when it is legitimate.

However in practice, there are many cases where this type of ActionResult can be reasonably be expected never to occur (eg when REAPI is only used for compilation jobs, like recc or goma jobs). And in that case, maybe it's worth keeping this validation, off by default and hidden by a command line flag. Would such a flag be welcome, or should I just remove this validation?

Pull request description:
ensure ExecutionMetadata.Worker is set on all uploaded ActionResults
proto3 does not encode default values, so if all fields in an ActionResult
are default then the serialized value is zero-length. We would like to
avoid using zero-length files, because this is the most common symptom of
filesystem corruption. Unfortunately, a default-valued ActionResult is
actually valid, so we can't reject it automatically.

We can modify ActionResult messages without changing their meaning, by
ensuring that one of the ExecutionMetadata's fields to a non-default
value. ExecutionMetadata.Worker seems like a good choice- the server can
set it to a meaningful value even if the client (or worker) left it empty.

This allows us to keep the zero-sized file corruption check when serving
ActionCache results.

Fixes #121.

Patch:
diff --git a/README.md b/README.md
index 587570e..33a980d 100644
--- a/README.md
+++ b/README.md
@@ -8,6 +8,9 @@ size and bazel-remote will automatically enforce this limit and clean the cache
 last access time. The cache supports HTTP basic authentication with usernames and passwords being specified by a
 `.htpasswd` file.
 
+**Project status**: bazel-remote has been serving TBs of cache artifacts per day since April 2018, both on
+commodity hardware and AWS servers. Outgoing bandwidth can exceed 15 Gbit/s on the right AWS instance type.
+
 ## HTTP/1.1 REST API
 
 Cache entries are set and retrieved by key, and there are two types of keys that can be used:
@@ -26,9 +29,6 @@ bazel-remote also has experimental support for the ActionCache, ContentAddressab
 [Bazel Remote Execution API v2](https://github.com/bazelbuild/remote-apis/blob/master/build/bazel/remote/execution/v2/remote_execution.proto),
 and the corresponding parts of the [Byte Stream API](https://github.com/googleapis/googleapis/blob/master/google/bytestream/bytestream.proto).
 
-**Project status**: bazel-remote has been serving TBs of cache artifacts per day since April 2018, both on
-commodity hardware and AWS servers. Outgoing bandwidth can exceed 15 Gbit/s on the right AWS instance type.
-
 ## Usage
 
 The cache can be configured via command line flags, environment variables or a YAML configuration
diff --git a/WORKSPACE b/WORKSPACE
index 1dbbda8..f4edc86 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -202,7 +202,7 @@ http_archive(
 
 go_repository(
     name = "com_github_bazelbuild_remote_apis",
-    commit = "cd42f25a6c5d7bd97859ab946ddb9a7d8e48b23a",
+    commit = "b5123b1bb2853393c7b9aa43236db924d7e32d61",
     importpath = "github.com/bazelbuild/remote-apis",
 )
 
diff --git a/cache/disk/disk.go b/cache/disk/disk.go
index f714444..d9d1a9c 100644
--- a/cache/disk/disk.go
+++ b/cache/disk/disk.go
@@ -43,15 +43,15 @@ func New(dir string, maxSizeBytes int64) cache.Cache {
 	for _, c1 := range hexLetters {
 		for _, c2 := range hexLetters {
 			subDir := string(c1) + string(c2)
-			err := os.MkdirAll(filepath.Join(dir, cache.CAS.String(), subDir), os.FileMode(0744))
+			err := os.MkdirAll(filepath.Join(dir, cache.CAS.String(), subDir), os.ModePerm)
 			if err != nil {
 				log.Fatal(err)
 			}
-			err = os.MkdirAll(filepath.Join(dir, cache.AC.String(), subDir), os.FileMode(0744))
+			err = os.MkdirAll(filepath.Join(dir, cache.AC.String(), subDir), os.ModePerm)
 			if err != nil {
 				log.Fatal(err)
 			}
-			err = os.MkdirAll(filepath.Join(dir, cache.RAW.String(), subDir), os.FileMode(0744))
+			err = os.MkdirAll(filepath.Join(dir, cache.RAW.String(), subDir), os.ModePerm)
 			if err != nil {
 				log.Fatal(err)
 			}
@@ -204,14 +204,16 @@ func (c *diskCache) Put(kind cache.EntryKind, hash string, expectedSize int64, r
 	}()
 
 	// Download to a temporary file
-	f, err := ioutil.TempFile(c.dir, "upload")
+	filePath := cacheFilePath(kind, c.dir, hash)
+	tmpFilePath := filePath + ".tmp"
+	f, err := os.Create(tmpFilePath)
 	if err != nil {
 		return err
 	}
 	defer func() {
 		if !shouldCommit {
 			// Only delete the temp file if moving it didn't succeed.
-			os.Remove(f.Name())
+			os.Remove(tmpFilePath)
 		}
 		// Just in case we didn't already close it.  No need to check errors.
 		f.Close()
@@ -248,8 +250,7 @@ func (c *diskCache) Put(kind cache.EntryKind, hash string, expectedSize int64, r
 	}
 
 	// Rename to the final path
-	filePath := cacheFilePath(kind, c.dir, hash)
-	err = os.Rename(f.Name(), filePath)
+	err = os.Rename(tmpFilePath, filePath)
 	// Only commit if renaming succeeded
 	if err == nil {
 		// This flag is used by the defer() block above.
@@ -309,7 +310,7 @@ func (c *diskCache) CurrentSize() int64 {
 
 func ensureDirExists(path string) {
 	if _, err := os.Stat(path); os.IsNotExist(err) {
-		err = os.MkdirAll(path, os.FileMode(0744))
+		err = os.MkdirAll(path, os.ModePerm)
 		if err != nil {
 			log.Fatal(err)
 		}
diff --git a/go.mod b/go.mod
index d8c25b2..dde4f12 100644
--- a/go.mod
+++ b/go.mod
@@ -2,7 +2,7 @@ module github.com/buchgr/bazel-remote
 
 require (
 	github.com/abbot/go-http-auth v0.4.0
-	github.com/bazelbuild/remote-apis v0.0.0-20190709012138-e7282cf0f0e1
+	github.com/bazelbuild/remote-apis v0.0.0-20191119143007-b5123b1bb285
 	github.com/djherbis/atime v1.0.0
 	github.com/go-ini/ini v1.49.0 // indirect
 	github.com/golang/protobuf v1.3.2
@@ -10,10 +10,12 @@ require (
 	github.com/google/uuid v1.1.1
 	github.com/minio/minio-go v6.0.14+incompatible
 	github.com/mitchellh/go-homedir v1.1.0 // indirect
+	github.com/smartystreets/goconvey v1.6.4 // indirect
 	github.com/urfave/cli v1.20.0
 	golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be
 	google.golang.org/genproto v0.0.0-20190801165951-fa694d86fc64
 	google.golang.org/grpc v1.23.0
+	gopkg.in/ini.v1 v1.51.0 // indirect
 	gopkg.in/yaml.v2 v2.2.1
 )
 
diff --git a/go.sum b/go.sum
index 67c000b..43c3acb 100644
--- a/go.sum
+++ b/go.sum
@@ -3,8 +3,8 @@ cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMT
 github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
 github.com/abbot/go-http-auth v0.4.0 h1:QjmvZ5gSC7jm3Zg54DqWE/T5m1t2AfDu6QlXJT0EVT0=
 github.com/abbot/go-http-auth v0.4.0/go.mod h1:Cz6ARTIzApMJDzh5bRMSUou6UMSp0IEXg9km/ci7TJM=
-github.com/bazelbuild/remote-apis v0.0.0-20190709012138-e7282cf0f0e1 h1:faRBoPUfI4lBAhz/6t8jDWOUGhFES9wC7TFi5dqaLUM=
-github.com/bazelbuild/remote-apis v0.0.0-20190709012138-e7282cf0f0e1/go.mod h1:9Y+1FnaNUGVV6wKE0Jdh+mguqDUsyd9uUqokalrC7DQ=
+github.com/bazelbuild/remote-apis v0.0.0-20191119143007-b5123b1bb285 h1:OPH+hf+ICw8WEp2CV2ncfdyWPC30Cmw8b5NKun0n5IQ=
+github.com/bazelbuild/remote-apis v0.0.0-20191119143007-b5123b1bb285/go.mod h1:9Y+1FnaNUGVV6wKE0Jdh+mguqDUsyd9uUqokalrC7DQ=
 github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=
 github.com/djherbis/atime v1.0.0 h1:ySLvBAM0EvOGaX7TI4dAM5lWj+RdJUCKtGSEHN8SGBg=
 github.com/djherbis/atime v1.0.0/go.mod h1:5W+KBIuTwVGcqjIfaTwt+KSYX1o6uep8dtevevQP/f8=
@@ -21,10 +21,18 @@ github.com/google/go-cmp v0.2.0 h1:+dTQ8DZQJz0Mb/HjFlkptS1FeQ4cWSnN941F8aEG4SQ=
 github.com/google/go-cmp v0.2.0/go.mod h1:oXzfMopK8JAjlY9xF4vHSVASa0yLyX7SntLO5aqRK0M=
 github.com/google/uuid v1.1.1 h1:Gkbcsh/GbpXz7lPftLA3P6TYMwjCLYm83jiFQZF/3gY=
 github.com/google/uuid v1.1.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
+github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1 h1:EGx4pi6eqNxGaHF6qqu48+N2wcFQ5qg5FXgOdqsJ5d8=
+github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY=
+github.com/jtolds/gls v4.20.0+incompatible h1:xdiiI2gbIgH/gLH7ADydsJ1uDOEzR8yvV7C0MuV77Wo=
+github.com/jtolds/gls v4.20.0+incompatible/go.mod h1:QJZ7F/aHp+rZTRtaJ1ow/lLfFfVYBRgL+9YlvaHOwJU=
 github.com/minio/minio-go v6.0.14+incompatible h1:fnV+GD28LeqdN6vT2XdGKW8Qe/IfjJDswNVuni6km9o=
 github.com/minio/minio-go v6.0.14+incompatible/go.mod h1:7guKYtitv8dktvNUGrhzmNlA5wrAABTQXCoesZdFQO8=
 github.com/mitchellh/go-homedir v1.1.0 h1:lukF9ziXFxDFPkA1vsr5zpc1XuPDn/wFntq5mG+4E0Y=
 github.com/mitchellh/go-homedir v1.1.0/go.mod h1:SfyaCUpYCn1Vlf4IUYiD9fPX4A5wJrkLzIz1N1q0pr0=
+github.com/smartystreets/assertions v0.0.0-20180927180507-b2de0cb4f26d h1:zE9ykElWQ6/NYmHa3jpm/yHnI4xSofP+UP6SpjHcSeM=
+github.com/smartystreets/assertions v0.0.0-20180927180507-b2de0cb4f26d/go.mod h1:OnSkiWE9lh6wB0YB77sQom3nweQdgAjqCqsofrRNTgc=
+github.com/smartystreets/goconvey v1.6.4 h1:fv0U8FUIMPNf1L9lnHLvLhgicrIVChEkdzIKYqbNC9s=
+github.com/smartystreets/goconvey v1.6.4/go.mod h1:syvi0/a8iFYH4r/RixwvyeAJjdLS9QV7WQ/tjFTllLA=
 github.com/urfave/cli v1.20.0 h1:fDqGv3UG/4jbVl/QkFwEdddtEDjh/5Ov6X+0B/3bPaw=
 github.com/urfave/cli v1.20.0/go.mod h1:70zkFmudgCuE/ngEzBv17Jvp/497gISqfk5gWijbERA=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2 h1:VklqNMn3ovrHsnt90PveolxSbWFaJdECFbxSq0Mqo2M=
@@ -53,6 +61,7 @@ golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 golang.org/x/tools v0.0.0-20190114222345-bf090417da8b/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
 golang.org/x/tools v0.0.0-20190226205152-f727befe758c/go.mod h1:9Yl7xja0Znq3iFh3HoIrodX9oNMXvdceNzlUR8zjMvY=
 golang.org/x/tools v0.0.0-20190311212946-11955173bddd/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs=
+golang.org/x/tools v0.0.0-20190328211700-ab21143f2384/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs=
 golang.org/x/tools v0.0.0-20190524140312-2c0ae7006135/go.mod h1:RgjU9mgBXZiqYHBnxXauZ1Gv1EHHAz9KjViQ78xBX0Q=
 google.golang.org/appengine v1.1.0/go.mod h1:EbEs0AVv82hx2wNQdGPgUI5lhzA/G0D9YwlJXL52JkM=
 google.golang.org/appengine v1.4.0 h1:/wp5JvzpHIxhs/dumFmF7BXTf3Z+dd4uXta4kVyO508=
@@ -67,6 +76,8 @@ google.golang.org/grpc v1.23.0 h1:AzbTB6ux+okLTzP8Ru1Xs41C303zdcfEht7MQnYJt5A=
 google.golang.org/grpc v1.23.0/go.mod h1:Y5yQAOtifL1yxbo5wqy6BxZv8vAUGQwXBOALyacEbxg=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
+gopkg.in/ini.v1 v1.51.0 h1:AQvPpx3LzTDM0AjnIRlVFwFFGC+npRopjZxLJj6gdno=
+gopkg.in/ini.v1 v1.51.0/go.mod h1:pNLf8WUiyNEtQjuu5G5vTm06TEv9tsIgeAvK8hOrP4k=
 gopkg.in/yaml.v2 v2.2.1 h1:mUhvW9EsL+naU5Q3cakzfE91YhliOondGd6ZrsDBHQE=
 gopkg.in/yaml.v2 v2.2.1/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
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.go b/server/grpc.go
index b49fda5..d15a6d9 100644
--- a/server/grpc.go
+++ b/server/grpc.go
@@ -81,7 +81,7 @@ func (s *grpcServer) GetCapabilities(ctx context.Context,
 			SymlinkAbsolutePathStrategy: pb.SymlinkAbsolutePathStrategy_ALLOWED,
 		},
 		LowApiVersion:  &semver.SemVer{Major: int32(2)},
-		HighApiVersion: &semver.SemVer{Major: int32(2)},
+		HighApiVersion: &semver.SemVer{Major: int32(2), Minor: int32(1)},
 	}
 
 	s.accessLogger.Printf("GRPC GETCAPABILITIES")
diff --git a/server/grpc_ac.go b/server/grpc_ac.go
index 3dfd737..4dcb9ea 100644
--- a/server/grpc_ac.go
+++ b/server/grpc_ac.go
@@ -4,17 +4,22 @@ import (
 	"bytes"
 	"context"
 	"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")
 )
 
@@ -56,6 +61,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 +90,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")
 	}
 }
 
diff --git a/utils/testutils.go b/utils/testutils.go
index d42a822..70ad301 100644
--- a/utils/testutils.go
+++ b/utils/testutils.go
@@ -20,19 +20,19 @@ func TempDir(t *testing.T) string {
 
 func CreateRandomFile(dir string, size int64) (string, error) {
 	data, hash := RandomDataAndHash(size)
-	os.MkdirAll(dir, os.FileMode(0744))
+	os.MkdirAll(dir, os.ModePerm)
 	filepath := dir + "/" + hash
 
-	return hash, ioutil.WriteFile(filepath, data, 0744)
+	return hash, ioutil.WriteFile(filepath, data, os.ModePerm)
 }
 
 func CreateCacheFile(dir string, size int64) (string, error) {
 	data, hash := RandomDataAndHash(size)
 	subdir := dir + "/" + hash[0:2]
-	os.MkdirAll(subdir, os.FileMode(0744))
+	os.MkdirAll(subdir, os.ModePerm)
 	filepath := subdir + "/" + hash
 
-	return hash, ioutil.WriteFile(filepath, data, 0744)
+	return hash, ioutil.WriteFile(filepath, data, os.ModePerm)
 }
 
 func RandomDataAndHash(size int64) ([]byte, string) {
@@ -48,7 +48,7 @@ func CreateTmpCacheDirs(t *testing.T) string {
 	if err != nil {
 		t.Error("Couldn't create tmp dir", err)
 	}
-	os.MkdirAll(path, os.FileMode(0744))
+	os.MkdirAll(path, os.ModePerm)
 	return path
 }