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 b808898034dd65..d24082f7ec6a2f 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 @@ -58,6 +58,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Strings; +import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -1123,24 +1124,52 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re return null; } + private Single buildUploadManifestAsync( + RemoteAction action, SpawnResult spawnResult) { + return Single.fromCallable( + () -> { + ImmutableList.Builder outputFiles = ImmutableList.builder(); + 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("Output " + output + " was not created."); + } + outputFiles.add(outputPath); + } + + return UploadManifest.create( + remoteOptions, + digestUtil, + remotePathResolver, + action.actionKey, + action.action, + action.command, + outputFiles.build(), + action.spawnExecutionContext.getFileOutErr(), + /* exitCode= */ 0); + }); + } + @VisibleForTesting UploadManifest buildUploadManifest(RemoteAction action, SpawnResult spawnResult) - throws ExecException, IOException { - Collection outputFiles = - action.spawn.getOutputFiles().stream() - .map((inp) -> execRoot.getRelative(inp.getExecPath())) - .collect(ImmutableList.toImmutableList()); - - return UploadManifest.create( - remoteOptions, - digestUtil, - remotePathResolver, - action.actionKey, - action.action, - action.command, - outputFiles, - action.spawnExecutionContext.getFileOutErr(), - /* exitCode= */ 0); + throws IOException, ExecException, InterruptedException { + try { + return buildUploadManifestAsync(action, spawnResult).blockingGet(); + } catch (RuntimeException e) { + Throwable cause = e.getCause(); + if (cause != null) { + Throwables.throwIfInstanceOf(cause, IOException.class); + Throwables.throwIfInstanceOf(cause, ExecException.class); + Throwables.throwIfInstanceOf(cause, InterruptedException.class); + } + throw e; + } } /** Upload outputs of a remote action which was executed locally to remote cache. */ @@ -1152,42 +1181,43 @@ public void uploadOutputs(RemoteAction action, SpawnResult spawnResult) SpawnResult.Status.SUCCESS.equals(spawnResult.status()) && spawnResult.exitCode() == 0, "shouldn't upload outputs of failed local action"); - try { - UploadManifest manifest = buildUploadManifest(action, spawnResult); - if (remoteOptions.remoteCacheAsync) { - Single.using( - remoteCache::retain, - remoteCache -> - manifest.uploadAsync( - action.getRemoteActionExecutionContext(), remoteCache, reporter), - RemoteCache::release) - .subscribeOn(scheduler) - .subscribe( - new SingleObserver() { - @Override - public void onSubscribe(@NonNull Disposable d) { - backgroundTaskPhaser.register(); - } - - @Override - public void onSuccess(@NonNull ActionResult actionResult) { - backgroundTaskPhaser.arriveAndDeregister(); - } - - @Override - public void onError(@NonNull Throwable e) { - backgroundTaskPhaser.arriveAndDeregister(); - reportUploadError(e); - } - }); - } else { - try (SilentCloseable c = - Profiler.instance().profile(ProfilerTask.UPLOAD_TIME, "upload outputs")) { - manifest.upload(action.getRemoteActionExecutionContext(), remoteCache, reporter); - } + if (remoteOptions.remoteCacheAsync) { + Single.using( + remoteCache::retain, + remoteCache -> + buildUploadManifestAsync(action, spawnResult) + .flatMap( + manifest -> + manifest.uploadAsync( + action.getRemoteActionExecutionContext(), remoteCache, reporter)), + RemoteCache::release) + .subscribeOn(scheduler) + .subscribe( + new SingleObserver() { + @Override + public void onSubscribe(@NonNull Disposable d) { + backgroundTaskPhaser.register(); + } + + @Override + public void onSuccess(@NonNull ActionResult actionResult) { + backgroundTaskPhaser.arriveAndDeregister(); + } + + @Override + public void onError(@NonNull Throwable e) { + backgroundTaskPhaser.arriveAndDeregister(); + reportUploadError(e); + } + }); + } else { + try (SilentCloseable c = + Profiler.instance().profile(ProfilerTask.UPLOAD_TIME, "upload outputs")) { + UploadManifest manifest = buildUploadManifest(action, spawnResult); + manifest.upload(action.getRemoteActionExecutionContext(), remoteCache, reporter); + } catch (IOException e) { + reportUploadError(e); } - } catch (IOException e) { - reportUploadError(e); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java b/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java index b9b391227306d4..ae7755ea3bccd0 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java +++ b/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java @@ -350,7 +350,7 @@ ActionResult getActionResult() { /** Uploads outputs and action result (if exit code is 0) to remote cache. */ public ActionResult upload( RemoteActionExecutionContext context, RemoteCache remoteCache, ExtendedEventHandler reporter) - throws IOException, InterruptedException { + throws IOException, InterruptedException, ExecException { try { return uploadAsync(context, remoteCache, reporter).blockingGet(); } catch (RuntimeException e) { @@ -358,6 +358,7 @@ public ActionResult upload( if (cause != null) { throwIfInstanceOf(cause, InterruptedException.class); throwIfInstanceOf(cause, IOException.class); + throwIfInstanceOf(cause, ExecException.class); } throw e; } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index 1f9f6bd8065d35..9011e20c98149b 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -1367,6 +1367,27 @@ public void uploadOutputs_firesUploadEvents() throws Exception { spawn.getResourceOwner(), "ac/" + action.getActionId())); } + @Test + public void uploadOutputs_missingDeclaredOutputs_dontUpload() throws Exception { + Path file = execRoot.getRelative("outputs/file"); + Artifact outputFile = ActionsTestUtil.createArtifact(artifactRoot, file); + RemoteExecutionService service = newRemoteExecutionService(); + Spawn spawn = newSpawn(ImmutableMap.of(), ImmutableSet.of(outputFile)); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteAction action = service.buildRemoteAction(spawn, context); + SpawnResult spawnResult = + new SpawnResult.Builder() + .setExitCode(0) + .setStatus(SpawnResult.Status.SUCCESS) + .setRunnerName("test") + .build(); + + service.uploadOutputs(action, spawnResult); + + // assert + assertThat(cache.getNumFindMissingDigests()).isEmpty(); + } + @Test public void uploadInputsIfNotPresent_deduplicateFindMissingBlobCalls() throws Exception { int taskCount = 100; diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 64a9cec6f74365..f6756df1390235 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3535,4 +3535,27 @@ EOF [[ "$disk_cas_files" == 3 ]] || fail "Expected 3 disk cas entries, not $disk_cas_files" } +function test_missing_outputs_dont_upload_action_result() { + # Test that if declared outputs are not created, even the exit code of action + # is 0, we treat this as failed and don't upload action result. + # See https://github.com/bazelbuild/bazel/issues/14543. + mkdir -p a + cat > a/BUILD <& $TEST_log && fail "Should failed to build" + + remote_cas_files="$(count_remote_cas_files)" + [[ "$remote_cas_files" == 0 ]] || fail "Expected 0 remote cas entries, not $remote_cas_files" + remote_ac_files="$(count_remote_ac_files)" + [[ "$remote_ac_files" == 0 ]] || fail "Expected 0 remote action cache entries, not $remote_ac_files" +} + run_suite "Remote execution and remote cache tests"