Skip to content

Commit 5b54588

Browse files
coeuvrecopybara-github
coeuvre
authored and
copybara-github
committed
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 #14543. Closes #15016. PiperOrigin-RevId: 434448255
1 parent 42c7518 commit 5b54588

4 files changed

Lines changed: 127 additions & 52 deletions

File tree

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

Lines changed: 81 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
import com.google.common.annotations.VisibleForTesting;
5959
import com.google.common.base.Preconditions;
6060
import com.google.common.base.Strings;
61+
import com.google.common.base.Throwables;
6162
import com.google.common.collect.ImmutableList;
6263
import com.google.common.collect.ImmutableMap;
6364
import com.google.common.collect.ImmutableSet;
@@ -1123,24 +1124,52 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
11231124
return null;
11241125
}
11251126

1127+
private Single<UploadManifest> buildUploadManifestAsync(
1128+
RemoteAction action, SpawnResult spawnResult) {
1129+
return Single.fromCallable(
1130+
() -> {
1131+
ImmutableList.Builder<Path> outputFiles = ImmutableList.builder();
1132+
for (ActionInput outputFile : action.spawn.getOutputFiles()) {
1133+
Path outputPath = execRoot.getRelative(outputFile.getExecPath());
1134+
if (!outputPath.exists()) {
1135+
String output;
1136+
if (outputFile instanceof Artifact) {
1137+
output = ((Artifact) outputFile).prettyPrint();
1138+
} else {
1139+
output = outputFile.getExecPathString();
1140+
}
1141+
throw new IOException("Expected output " + output + " was not created locally.");
1142+
}
1143+
outputFiles.add(outputPath);
1144+
}
1145+
1146+
return UploadManifest.create(
1147+
remoteOptions,
1148+
digestUtil,
1149+
remotePathResolver,
1150+
action.actionKey,
1151+
action.action,
1152+
action.command,
1153+
outputFiles.build(),
1154+
action.spawnExecutionContext.getFileOutErr(),
1155+
spawnResult.exitCode());
1156+
});
1157+
}
1158+
11261159
@VisibleForTesting
11271160
UploadManifest buildUploadManifest(RemoteAction action, SpawnResult spawnResult)
1128-
throws ExecException, IOException {
1129-
Collection<Path> outputFiles =
1130-
action.spawn.getOutputFiles().stream()
1131-
.map((inp) -> execRoot.getRelative(inp.getExecPath()))
1132-
.collect(ImmutableList.toImmutableList());
1133-
1134-
return UploadManifest.create(
1135-
remoteOptions,
1136-
digestUtil,
1137-
remotePathResolver,
1138-
action.actionKey,
1139-
action.action,
1140-
action.command,
1141-
outputFiles,
1142-
action.spawnExecutionContext.getFileOutErr(),
1143-
/* exitCode= */ 0);
1161+
throws IOException, ExecException, InterruptedException {
1162+
try {
1163+
return buildUploadManifestAsync(action, spawnResult).blockingGet();
1164+
} catch (RuntimeException e) {
1165+
Throwable cause = e.getCause();
1166+
if (cause != null) {
1167+
Throwables.throwIfInstanceOf(cause, IOException.class);
1168+
Throwables.throwIfInstanceOf(cause, ExecException.class);
1169+
Throwables.throwIfInstanceOf(cause, InterruptedException.class);
1170+
}
1171+
throw e;
1172+
}
11441173
}
11451174

11461175
/** Upload outputs of a remote action which was executed locally to remote cache. */
@@ -1152,42 +1181,43 @@ public void uploadOutputs(RemoteAction action, SpawnResult spawnResult)
11521181
SpawnResult.Status.SUCCESS.equals(spawnResult.status()) && spawnResult.exitCode() == 0,
11531182
"shouldn't upload outputs of failed local action");
11541183

1155-
try {
1156-
UploadManifest manifest = buildUploadManifest(action, spawnResult);
1157-
if (remoteOptions.remoteCacheAsync) {
1158-
Single.using(
1159-
remoteCache::retain,
1160-
remoteCache ->
1161-
manifest.uploadAsync(
1162-
action.getRemoteActionExecutionContext(), remoteCache, reporter),
1163-
RemoteCache::release)
1164-
.subscribeOn(scheduler)
1165-
.subscribe(
1166-
new SingleObserver<ActionResult>() {
1167-
@Override
1168-
public void onSubscribe(@NonNull Disposable d) {
1169-
backgroundTaskPhaser.register();
1170-
}
1171-
1172-
@Override
1173-
public void onSuccess(@NonNull ActionResult actionResult) {
1174-
backgroundTaskPhaser.arriveAndDeregister();
1175-
}
1176-
1177-
@Override
1178-
public void onError(@NonNull Throwable e) {
1179-
backgroundTaskPhaser.arriveAndDeregister();
1180-
reportUploadError(e);
1181-
}
1182-
});
1183-
} else {
1184-
try (SilentCloseable c =
1185-
Profiler.instance().profile(ProfilerTask.UPLOAD_TIME, "upload outputs")) {
1186-
manifest.upload(action.getRemoteActionExecutionContext(), remoteCache, reporter);
1187-
}
1184+
if (remoteOptions.remoteCacheAsync) {
1185+
Single.using(
1186+
remoteCache::retain,
1187+
remoteCache ->
1188+
buildUploadManifestAsync(action, spawnResult)
1189+
.flatMap(
1190+
manifest ->
1191+
manifest.uploadAsync(
1192+
action.getRemoteActionExecutionContext(), remoteCache, reporter)),
1193+
RemoteCache::release)
1194+
.subscribeOn(scheduler)
1195+
.subscribe(
1196+
new SingleObserver<ActionResult>() {
1197+
@Override
1198+
public void onSubscribe(@NonNull Disposable d) {
1199+
backgroundTaskPhaser.register();
1200+
}
1201+
1202+
@Override
1203+
public void onSuccess(@NonNull ActionResult actionResult) {
1204+
backgroundTaskPhaser.arriveAndDeregister();
1205+
}
1206+
1207+
@Override
1208+
public void onError(@NonNull Throwable e) {
1209+
backgroundTaskPhaser.arriveAndDeregister();
1210+
reportUploadError(e);
1211+
}
1212+
});
1213+
} else {
1214+
try (SilentCloseable c =
1215+
Profiler.instance().profile(ProfilerTask.UPLOAD_TIME, "upload outputs")) {
1216+
UploadManifest manifest = buildUploadManifest(action, spawnResult);
1217+
manifest.upload(action.getRemoteActionExecutionContext(), remoteCache, reporter);
1218+
} catch (IOException e) {
1219+
reportUploadError(e);
11881220
}
1189-
} catch (IOException e) {
1190-
reportUploadError(e);
11911221
}
11921222
}
11931223

‎src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -350,14 +350,15 @@ ActionResult getActionResult() {
350350
/** Uploads outputs and action result (if exit code is 0) to remote cache. */
351351
public ActionResult upload(
352352
RemoteActionExecutionContext context, RemoteCache remoteCache, ExtendedEventHandler reporter)
353-
throws IOException, InterruptedException {
353+
throws IOException, InterruptedException, ExecException {
354354
try {
355355
return uploadAsync(context, remoteCache, reporter).blockingGet();
356356
} catch (RuntimeException e) {
357357
Throwable cause = e.getCause();
358358
if (cause != null) {
359359
throwIfInstanceOf(cause, InterruptedException.class);
360360
throwIfInstanceOf(cause, IOException.class);
361+
throwIfInstanceOf(cause, ExecException.class);
361362
}
362363
throw e;
363364
}

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1367,6 +1367,27 @@ public void uploadOutputs_firesUploadEvents() throws Exception {
13671367
spawn.getResourceOwner(), "ac/" + action.getActionId()));
13681368
}
13691369

1370+
@Test
1371+
public void uploadOutputs_missingDeclaredOutputs_dontUpload() throws Exception {
1372+
Path file = execRoot.getRelative("outputs/file");
1373+
Artifact outputFile = ActionsTestUtil.createArtifact(artifactRoot, file);
1374+
RemoteExecutionService service = newRemoteExecutionService();
1375+
Spawn spawn = newSpawn(ImmutableMap.of(), ImmutableSet.of(outputFile));
1376+
FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
1377+
RemoteAction action = service.buildRemoteAction(spawn, context);
1378+
SpawnResult spawnResult =
1379+
new SpawnResult.Builder()
1380+
.setExitCode(0)
1381+
.setStatus(SpawnResult.Status.SUCCESS)
1382+
.setRunnerName("test")
1383+
.build();
1384+
1385+
service.uploadOutputs(action, spawnResult);
1386+
1387+
// assert
1388+
assertThat(cache.getNumFindMissingDigests()).isEmpty();
1389+
}
1390+
13701391
@Test
13711392
public void uploadInputsIfNotPresent_deduplicateFindMissingBlobCalls() throws Exception {
13721393
int taskCount = 100;

‎src/test/shell/bazel/remote/remote_execution_test.sh‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3535,4 +3535,27 @@ EOF
35353535
[[ "$disk_cas_files" == 3 ]] || fail "Expected 3 disk cas entries, not $disk_cas_files"
35363536
}
35373537

3538+
function test_missing_outputs_dont_upload_action_result() {
3539+
# Test that if declared outputs are not created, even the exit code of action
3540+
# is 0, we treat this as failed and don't upload action result.
3541+
# See https://github.com/bazelbuild/bazel/issues/14543.
3542+
mkdir -p a
3543+
cat > a/BUILD <<EOF
3544+
genrule(
3545+
name = 'foo',
3546+
outs = ["foo.txt"],
3547+
cmd = "echo foo",
3548+
)
3549+
EOF
3550+
3551+
bazel build \
3552+
--remote_cache=grpc://localhost:${worker_port} \
3553+
//a:foo >& $TEST_log && fail "Should failed to build"
3554+
3555+
remote_cas_files="$(count_remote_cas_files)"
3556+
[[ "$remote_cas_files" == 0 ]] || fail "Expected 0 remote cas entries, not $remote_cas_files"
3557+
remote_ac_files="$(count_remote_ac_files)"
3558+
[[ "$remote_ac_files" == 0 ]] || fail "Expected 0 remote action cache entries, not $remote_ac_files"
3559+
}
3560+
35383561
run_suite "Remote execution and remote cache tests"

0 commit comments

Comments
 (0)