Skip to content

Commit

Permalink
Remote: Don't upload action result if declared outputs are not created.
Browse files Browse the repository at this point in the history 
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 bazelbuild#14543.

Closes bazelbuild#15016.

PiperOrigin-RevId: 434448255
  • Loading branch information
@coeuvre @copybara-github
coeuvre authored and copybara-github committed Mar 14, 2022
1 parent 42c7518 commit 5b54588
Show file tree
Hide file tree
Showing 4 changed files with 127 additions and 52 deletions.
132 changes: 81 additions & 51 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1123,24 +1124,52 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
return null;
}

private Single<UploadManifest> buildUploadManifestAsync(
RemoteAction action, SpawnResult spawnResult) {
return Single.fromCallable(
() -> {
ImmutableList.Builder<Path> 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<Path> 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. */
Expand All @@ -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<ActionResult>() {
@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<ActionResult>() {
@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);
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,14 +350,15 @@ 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) {
Throwable cause = e.getCause();
if (cause != null) {
throwIfInstanceOf(cause, InterruptedException.class);
throwIfInstanceOf(cause, IOException.class);
throwIfInstanceOf(cause, ExecException.class);
}
throw e;
}
Expand Down
21 changes: 21 additions & 0 deletions src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
23 changes: 23 additions & 0 deletions src/test/shell/bazel/remote/remote_execution_test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 <<EOF
genrule(
name = 'foo',
outs = ["foo.txt"],
cmd = "echo foo",
)
EOF

bazel build \
--remote_cache=grpc://localhost:${worker_port} \
//a:foo >& $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"

0 comments on commit 5b54588

Please sign in to comment.