From b0b26b28e5a2c17045c5ef76e0dd495386ac3f41 Mon Sep 17 00:00:00 2001
From: Son Luong Ngoc <sluongng@gmail.com>
Date: Thu, 7 Jul 2022 08:31:07 -0700
Subject: [PATCH] RemoteExecutionService: fix outputs not being uploaded
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In 4d900ceea12919ad62012830a95e51f9ec1a48bb we introduced validation in
DiskAndRemoteCacheClient.uploadActionResult() where context's step must
be UPLOAD_OUTPUTS to trigger the upload.

However, this value was never set in RemoteExecutionService before hand
thus led to outputs not being uploaded and cause remote cache misses.

Fix #15682

Thanks @adam-singer for doing the investigation 🙏

Closes #15823.

PiperOrigin-RevId: 459519852
Change-Id: Ib004403d7893fe135adcc4b181b607d8cb33f3af
---
 .../lib/remote/RemoteExecutionService.java    |  2 ++
 .../bazel/remote/remote_execution_test.sh     | 27 +++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
index 27132d528c1961..f8c7136388c2b7 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
@@ -1196,6 +1196,8 @@ public void uploadOutputs(RemoteAction action, SpawnResult spawnResult)
         SpawnResult.Status.SUCCESS.equals(spawnResult.status()) && spawnResult.exitCode() == 0,
         "shouldn't upload outputs of failed local action");
 
+    action.getRemoteActionExecutionContext().setStep(Step.UPLOAD_OUTPUTS);
+
     if (remoteOptions.remoteCacheAsync) {
       Single.using(
               remoteCache::retain,
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
index 854d866a75cefe..ba927b296ed784 100755
--- a/src/test/shell/bazel/remote/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -1583,6 +1583,33 @@ EOF
   rm -rf $cache
 }
 
+function test_combined_cache_upload() {
+  mkdir -p a
+  echo 'bar' > a/bar
+  cat > a/BUILD <<'EOF'
+genrule(
+  name = "foo",
+  srcs = [":bar"],
+  outs = ["foo.txt"],
+  cmd = """
+    echo $(location :bar) > "$@"
+  """,
+)
+EOF
+
+  CACHEDIR=$(mktemp -d)
+
+  bazel build \
+    --disk_cache=$CACHEDIR \
+    --remote_cache=grpc://localhost:${worker_port} \
+    //a:foo >& $TEST_log || "Failed to build //a:foo"
+
+  remote_ac_files="$(count_remote_ac_files)"
+  [[ "$remote_ac_files" == 1 ]] || fail "Expected 1 remote action cache entries, not $remote_ac_files"
+  remote_cas_files="$(count_remote_cas_files)"
+  [[ "$remote_cas_files" == 3 ]] || fail "Expected 3 remote cas entries, not $remote_cas_files"
+}
+
 function test_combined_cache_with_no_remote_cache_tag_remote_cache() {
   # Test that actions with no-remote-cache tag can hit disk cache of a combined cache but
   # remote cache is disabled.