From 5b545886fcd5bd95678567b0ca45d2a5e524beab Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Mon, 14 Mar 2022 06:34:32 -0700 Subject: [PATCH] Remote: Don't upload action result if declared outputs are not created. Even if the exit code is 0. Missing declared outputs will be detected by Bazel later and fail the build, so avoid uploading this false positive cache entry. Wrap buildUploadManifest inside a `Single.fromCallable` since there are many IOs (both the check we add in this PR and stats already there). If `--experimental_remote_cache_async` is set, these IOs will now be executed in a background thread. Fixes https://github.com/bazelbuild/bazel/issues/14543. Closes #15016. PiperOrigin-RevId: 434448255 --- .../lib/remote/RemoteExecutionService.java | 132 +++++++++++------- .../build/lib/remote/UploadManifest.java | 3 +- .../remote/RemoteExecutionServiceTest.java | 21 +++ .../bazel/remote/remote_execution_test.sh | 23 +++ 4 files changed, 127 insertions(+), 52 deletions(-) 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..ebe3ec05f57943 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("Expected output " + output + " was not created locally."); + } + outputFiles.add(outputPath); + } + + return UploadManifest.create( + remoteOptions, + digestUtil, + remotePathResolver, + action.actionKey, + action.action, + action.command, + outputFiles.build(), + action.spawnExecutionContext.getFileOutErr(), + spawnResult.exitCode()); + }); + } + @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..e8088b1552fb2c 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"