Skip to content

Commit 5299b8d

Browse files
authored
Recover from an ActionResult json print error (buildfarm#2390)
Clients will upload results that contain execution_metadata that may contain unknown protobuf message types in the auxiliary_metadata repeated Any field. Purge unparseable objects and print warnings indicating what happened.
1 parent a3024fd commit 5299b8d

File tree

2 files changed

+74
-1
lines changed

2 files changed

+74
-1
lines changed

src/main/java/build/buildfarm/instance/shard/RedisShardBackplane.java

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222

2323
import build.bazel.remote.execution.v2.ActionResult;
2424
import build.bazel.remote.execution.v2.ExecuteOperationMetadata;
25+
import build.bazel.remote.execution.v2.ExecutedActionMetadata;
2526
import build.bazel.remote.execution.v2.ExecutionStage;
2627
import build.bazel.remote.execution.v2.Platform;
2728
import build.bazel.remote.execution.v2.RequestMetadata;
@@ -72,6 +73,7 @@
7273
import com.google.common.collect.Sets;
7374
import com.google.common.util.concurrent.ListenableFuture;
7475
import com.google.longrunning.Operation;
76+
import com.google.protobuf.Any;
7577
import com.google.protobuf.InvalidProtocolBufferException;
7678
import com.google.protobuf.Timestamp;
7779
import com.google.protobuf.util.JsonFormat;
@@ -940,10 +942,45 @@ public void blacklistAction(String actionId) throws IOException {
940942
jedis, actionId, "", configs.getBackplane().getActionBlacklistExpire()));
941943
}
942944

945+
private String printActionResult(ActionResult actionResult)
946+
throws InvalidProtocolBufferException {
947+
InvalidProtocolBufferException cause;
948+
try {
949+
return actionResultPrinter.print(actionResult);
950+
} catch (InvalidProtocolBufferException e) {
951+
// can happen with unknown types in auxiliary_metadata
952+
// this is extremely brittle and will have issues with any novel introduction of Any in remote
953+
// apis releases
954+
cause = e;
955+
}
956+
957+
ActionResult.Builder builder = actionResult.toBuilder();
958+
ExecutedActionMetadata.Builder metadata =
959+
builder.getExecutionMetadataBuilder().clearAuxiliaryMetadata();
960+
for (Any auxiliaryMetadata : actionResult.getExecutionMetadata().getAuxiliaryMetadataList()) {
961+
try {
962+
// test the serialization capacity of this any
963+
actionResultPrinter.print(auxiliaryMetadata);
964+
// serialization passed, re-add it
965+
metadata.addAuxiliaryMetadata(auxiliaryMetadata);
966+
} catch (InvalidProtocolBufferException e) {
967+
// ignore
968+
}
969+
}
970+
971+
String json = actionResultPrinter.print(builder.build());
972+
// purge must have succeeded, indicate as much to the server log
973+
log.log(
974+
Level.WARNING,
975+
"error printing auxiliary_metadata for key %s, unrecognized content purged",
976+
cause);
977+
return json;
978+
}
979+
943980
@SuppressWarnings("ConstantConditions")
944981
@Override
945982
public void putActionResult(ActionKey actionKey, ActionResult actionResult) throws IOException {
946-
String json = actionResultPrinter.print(actionResult);
983+
String json = printActionResult(actionResult);
947984
client.run(
948985
jedis ->
949986
state.actionCache.insert(

src/test/java/build/buildfarm/instance/shard/RedisShardBackplaneTest.java

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,12 @@
2525
import static org.mockito.Mockito.verifyNoMoreInteractions;
2626
import static org.mockito.Mockito.when;
2727

28+
import build.bazel.remote.execution.v2.Action;
29+
import build.bazel.remote.execution.v2.ActionResult;
30+
import build.bazel.remote.execution.v2.ExecuteOperationMetadata;
2831
import build.bazel.remote.execution.v2.RequestMetadata;
2932
import build.buildfarm.common.DigestUtil;
33+
import build.buildfarm.common.DigestUtil.ActionKey;
3034
import build.buildfarm.common.DigestUtil.HashFunction;
3135
import build.buildfarm.common.config.BuildfarmConfigs;
3236
import build.buildfarm.common.config.Queue;
@@ -44,11 +48,13 @@
4448
import build.buildfarm.v1test.QueueEntry;
4549
import build.buildfarm.v1test.ShardWorker;
4650
import build.buildfarm.v1test.WorkerChange;
51+
import build.buildfarm.v1test.WorkerExecutedMetadata;
4752
import build.buildfarm.worker.resources.LocalResourceSet;
4853
import com.google.common.collect.ImmutableList;
4954
import com.google.common.collect.ImmutableMap;
5055
import com.google.common.collect.ImmutableSet;
5156
import com.google.longrunning.Operation;
57+
import com.google.protobuf.Any;
5258
import com.google.protobuf.ByteString;
5359
import com.google.protobuf.util.JsonFormat;
5460
import java.io.IOException;
@@ -502,4 +508,34 @@ public void testAddWorker() throws IOException {
502508
JsonFormat.printer().print(shardWorker));
503509
verify(jedis, times(1)).publish(anyString(), anyString());
504510
}
511+
512+
@Test
513+
public void putActionResultPurgesUnknownAuxiliaryMetadatas() throws Exception {
514+
UnifiedJedis jedis = mock(UnifiedJedis.class);
515+
RedisClient client = new RedisClient(jedis);
516+
DistributedState state = new DistributedState();
517+
state.actionCache = mock(RedisMap.class);
518+
RedisShardBackplane backplane = createBackplane("put-action-result-purges-test");
519+
backplane.start(client, state, "putActionResultPurges/test:0000", name -> {});
520+
DigestUtil digestUtil = new DigestUtil(HashFunction.SHA256);
521+
ActionKey actionKey = digestUtil.computeActionKey(Action.getDefaultInstance());
522+
523+
ActionResult.Builder actionResult = ActionResult.newBuilder();
524+
// action results cannot currently parse ExecuteOperationMetadata, ensure this continues to be
525+
// true.
526+
actionResult
527+
.getExecutionMetadataBuilder()
528+
.addAuxiliaryMetadata(Any.pack(ExecuteOperationMetadata.getDefaultInstance()))
529+
.addAuxiliaryMetadata(Any.pack(WorkerExecutedMetadata.getDefaultInstance()));
530+
531+
backplane.putActionResult(actionKey, actionResult.build());
532+
533+
ArgumentCaptor<String> resultCaptor = ArgumentCaptor.forClass(String.class);
534+
verify(state.actionCache, times(1))
535+
.insert(eq(jedis), eq(actionKey.toString()), resultCaptor.capture(), any(Integer.class));
536+
verifyNoMoreInteractions(state.actionCache);
537+
String json = resultCaptor.getValue();
538+
assertThat(
539+
backplane.parseActionResult(json).getExecutionMetadata().getAuxiliaryMetadataCount() == 1);
540+
}
505541
}

0 commit comments

Comments
 (0)