Remote: Check declared outputs when downloading outputs.

An AC entry that misses declared outputs of an action is invalid because, if Bazel accepts this from remote cache, it will detect the missing declared outputs error at a later stage and fail the build.

This PR adds a check for mandatory outputs when downloading outputs from remote cache. If a mandatory output is missing from AC entry, Bazel will ignore the cache entry so build can continue.

Also fixes an issue introduced by bazelbuild#15016 that tests result are not uploaded to remote cache. Test spawns declare all the possible outputs but they usually don't generate them all. This change fixes that by only check for mandatory outputs via `spawn.isMandatoryOutput()`.

Fixes bazelbuild#14543.

Closes bazelbuild#15051.

PiperOrigin-RevId: 435307260

Author: coeuvre

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 action cache 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"),
+            /*runfilesSupplier=*/ 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

@@ -1197,6 +1197,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

@@ -1,4 +1,4 @@
-// Copyright 2017 The Bazel Authors. All rights reserved.
+/// Copyright 2017 The Bazel Authors. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -196,9 +196,12 @@ public final void setUp() throws Exception {
             ImmutableList.of("/bin/echo", "Hi!"),
             ImmutableMap.of("VARIABLE", "value"),
             /*executionInfo=*/ ImmutableMap.<String, String>of(),
+            /*runfilesSupplier=*/ 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");