Remote: Check declared outputs when downloading outputs.

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

@@ -1011,6 +1011,23 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
       metadata = parseActionResultMetadata(action, result);
     }
 
+    // Check that all mandatory outputs are created.
+    for (ActionInput output : action.spawn.getOutputFiles()) {
+      if (action.spawn.isMandatoryOutput(output)) {
+        Path localPath = execRoot.getRelative(output.getExecPath());
+        if (!metadata.files.containsKey(localPath)
+            && !metadata.directories.containsKey(localPath)
+            && !metadata.symlinks.containsKey(localPath)) {
+          throw new IOException(
+              "Invalid AC entry "
+                  + action.actionKey.getDigest().getHash()
+                  + ": expected output "
+                  + prettyPrint(output)
+                  + " does not exist.");
+        }
+      }
+    }
+
     FileOutErr outErr = action.spawnExecutionContext.getFileOutErr();
 
     ImmutableList.Builder<ListenableFuture<FileMetadata>> downloadsBuilder =
@@ -1124,23 +1141,27 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
     return null;
   }
 
+  private static String prettyPrint(ActionInput actionInput) {
+    if (actionInput instanceof Artifact) {
+      return ((Artifact) actionInput).prettyPrint();
+    } else {
+      return actionInput.getExecPathString();
+    }
+  }
+
   private Single<UploadManifest> buildUploadManifestAsync(
       RemoteAction action, SpawnResult spawnResult) {
     return Single.fromCallable(
         () -> {
           ImmutableList.Builder<Path> outputFiles = ImmutableList.builder();
+          // Check that all mandatory outputs are created.
           for (ActionInput outputFile : action.spawn.getOutputFiles()) {
-            Path outputPath = execRoot.getRelative(outputFile.getExecPath());
-            if (!outputPath.exists()) {
-              String output;
-              if (outputFile instanceof Artifact) {
-                output = ((Artifact) outputFile).prettyPrint();
-              } else {
-                output = outputFile.getExecPathString();
-              }
-              throw new IOException("Expected output " + output + " was not created locally.");
+            Path localPath = execRoot.getRelative(outputFile.getExecPath());
+            if (action.spawn.isMandatoryOutput(outputFile) && !localPath.exists()) {
+              throw new IOException(
+                  "Expected output " + prettyPrint(outputFile) + " was not created locally.");
             }
-            outputFiles.add(outputPath);
+            outputFiles.add(localPath);
           }
 
           return UploadManifest.create(

src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java

@@ -1101,9 +1101,21 @@ public void downloadOutputs_missingInMemoryOutputWithMinimal_returnsNull() throw
     ActionResult r = ActionResult.newBuilder().setExitCode(0).build();
     RemoteActionResult result = RemoteActionResult.createFromCache(CachedActionResult.remote(r));
     Artifact a1 = ActionsTestUtil.createArtifact(artifactRoot, "file1");
+    // set file1 as declared output but not mandatory output
     Spawn spawn =
-        newSpawn(
-            ImmutableMap.of(REMOTE_EXECUTION_INLINE_OUTPUTS, "outputs/file1"), ImmutableSet.of(a1));
+        new SimpleSpawn(
+            new FakeOwner("foo", "bar", "//dummy:label"),
+            /*arguments=*/ ImmutableList.of(),
+            /*environment=*/ ImmutableMap.of(),
+            /*executionInfo=*/ ImmutableMap.of(REMOTE_EXECUTION_INLINE_OUTPUTS, "outputs/file1"),
+            /*runfileSupplier=*/ null,
+            /*filesetMappings=*/ ImmutableMap.of(),
+            /*inputs=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
+            /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
+            /*outputs=*/ ImmutableSet.of(a1),
+            /*mandatoryOutputs=*/ ImmutableSet.of(),
+            ResourceSet.ZERO);
+
     MetadataInjector injector = mock(MetadataInjector.class);
     FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn, injector);
     RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class);
@@ -1120,6 +1132,32 @@ public void downloadOutputs_missingInMemoryOutputWithMinimal_returnsNull() throw
     verify(injector, never()).injectFile(eq(a1), remoteFileMatchingDigest(d1));
   }
 
+  @Test
+  public void downloadOutputs_missingMandatoryOutputs_reportError() throws Exception {
+    // Test that an AC which misses mandatory outputs is correctly ignored.
+    Digest fooDigest = cache.addContents(remoteActionExecutionContext, "foo-contents");
+    ActionResult.Builder builder = ActionResult.newBuilder();
+    builder.addOutputFilesBuilder().setPath("outputs/foo").setDigest(fooDigest);
+    RemoteActionResult result =
+        RemoteActionResult.createFromCache(CachedActionResult.remote(builder.build()));
+    ImmutableSet.Builder<Artifact> outputs = ImmutableSet.builder();
+    ImmutableList<String> expectedOutputFiles = ImmutableList.of("outputs/foo", "outputs/bar");
+    for (String outputFile : expectedOutputFiles) {
+      Path path = remotePathResolver.outputPathToLocalPath(outputFile);
+      Artifact output = ActionsTestUtil.createArtifact(artifactRoot, path);
+      outputs.add(output);
+    }
+    Spawn spawn = newSpawn(ImmutableMap.of(), outputs.build());
+    FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
+    RemoteExecutionService service = newRemoteExecutionService();
+    RemoteAction action = service.buildRemoteAction(spawn, context);
+
+    IOException error =
+        assertThrows(IOException.class, () -> service.downloadOutputs(action, result));
+
+    assertThat(error).hasMessageThat().containsMatch("expected output .+ does not exist.");
+  }
+
   @Test
   public void uploadOutputs_uploadDirectory_works() throws Exception {
     // Test that uploading a directory works.
@@ -1368,7 +1406,7 @@ public void uploadOutputs_firesUploadEvents() throws Exception {
   }
 
   @Test
-  public void uploadOutputs_missingDeclaredOutputs_dontUpload() throws Exception {
+  public void uploadOutputs_missingMandatoryOutputs_dontUpload() throws Exception {
     Path file = execRoot.getRelative("outputs/file");
     Artifact outputFile = ActionsTestUtil.createArtifact(artifactRoot, file);
     RemoteExecutionService service = newRemoteExecutionService();

src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java

@@ -20,6 +20,7 @@
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.spy;
@@ -602,6 +603,7 @@ public void testDownloadMinimal() throws Exception {
     when(remoteCache.downloadActionResult(
             any(RemoteActionExecutionContext.class), any(), /* inlineOutErr= */ eq(false)))
         .thenReturn(CachedActionResult.remote(success));
+    doReturn(null).when(cache.getRemoteExecutionService()).downloadOutputs(any(), any());
 
     // act
     CacheHandle cacheHandle = cache.lookup(simpleSpawn, simplePolicy);

src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java

@@ -40,13 +40,15 @@
 import build.bazel.remote.execution.v2.ExecutedActionMetadata;
 import build.bazel.remote.execution.v2.ExecutionStage.Value;
 import build.bazel.remote.execution.v2.LogFile;
+import build.bazel.remote.execution.v2.OutputFile;
 import com.google.common.collect.ClassToInstanceMap;
 import com.google.common.collect.ImmutableClassToInstanceMap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.eventbus.EventBus;
 import com.google.common.io.ByteStreams;
+import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListeningScheduledExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.common.util.concurrent.SettableFuture;
@@ -1197,6 +1199,7 @@ public void testDownloadTopLevel() throws Exception {
     RemoteSpawnRunner runner = newSpawnRunner(ImmutableSet.of(topLevelOutput));
     RemoteExecutionService service = runner.getRemoteExecutionService();
     doReturn(cachedActionResult).when(service).lookupCache(any());
+    doReturn(null).when(service).downloadOutputs(any(), any());
 
     Spawn spawn = newSimpleSpawn(topLevelOutput);
     FakeSpawnExecutionContext policy = getSpawnContext(spawn);

src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java

@@ -196,9 +196,12 @@ public final void setUp() throws Exception {
             ImmutableList.of("/bin/echo", "Hi!"),
             ImmutableMap.of("VARIABLE", "value"),
             /*executionInfo=*/ ImmutableMap.<String, String>of(),
+            /*runfileSupplier=*/ null,
+            /*filesetMappings=*/ ImmutableMap.of(),
             /*inputs=*/ NestedSetBuilder.create(
                 Order.STABLE_ORDER, ActionInputHelper.fromPath("input")),
-            /*outputs=*/ ImmutableSet.<ActionInput>of(
+            /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
+            /*outputs=*/ ImmutableSet.of(
                 new ActionInput() {
                   @Override
                   public String getExecPathString() {
@@ -231,6 +234,7 @@ public PathFragment getExecPath() {
                     return PathFragment.create("bar");
                   }
                 }),
+            /*mandatoryOutputs=*/ ImmutableSet.of(),
             ResourceSet.ZERO);
 
     Path stdout = fs.getPath("/tmp/stdout");