Do not fail a sandboxed spawn for missing execution statistics

This was already the case for "local" spawns. Statistics may be missing if the spawn wrapper exits abnormally. Fixes #22151. Closes #22780. PiperOrigin-RevId: 644378541 Change-Id: Ia3d792f380b78945523f21875c593744b60f0c81
bazelbuild · Jun 18, 2024 · ec41dd1 · ec41dd1
1 parent 7b02eaa
commit ec41dd1
Show file tree

Hide file tree

Showing 5 changed files with 34 additions and 48 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java
@@ -20,12 +20,15 @@
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.exec.Protos.Digest;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
+import com.google.devtools.build.lib.shell.ExecutionStatistics;
 import com.google.devtools.build.lib.shell.TerminationStatus;
 import com.google.devtools.build.lib.util.DetailedExitCode;
 import com.google.devtools.build.lib.util.ExitCode;
+import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.protobuf.ByteString;
+import java.io.IOException;
 import java.io.InputStream;
 import java.time.Instant;
 import java.util.Locale;
@@ -670,6 +673,28 @@ public Builder setDigest(Digest digest) {
       this.digest = digest;
       return this;
     }
+
+    /** Adds execution statistics based on a {@code execution_statistics.proto} file. */
+    @CanIgnoreReturnValue
+    public Builder setResourceUsageFromProto(Path statisticsPath) throws IOException {
+      ExecutionStatistics.getResourceUsage(statisticsPath)
+          .ifPresent(
+              resourceUsage -> {
+                setUserTimeInMs((int) resourceUsage.getUserExecutionTime().toMillis());
+                setSystemTimeInMs((int) resourceUsage.getSystemExecutionTime().toMillis());
+                setNumBlockOutputOperations(resourceUsage.getBlockOutputOperations());
+                setNumBlockInputOperations(resourceUsage.getBlockInputOperations());
+                setNumInvoluntaryContextSwitches(resourceUsage.getInvoluntaryContextSwitches());
+                // The memory usage of the largest child process. For Darwin maxrss returns size in
+                // bytes.
+                if (OS.getCurrent() == OS.DARWIN) {
+                  setMemoryInKb(resourceUsage.getMaximumResidentSetSize() / 1000);
+                } else {
+                  setMemoryInKb(resourceUsage.getMaximumResidentSetSize());
+                }
+              });
+      return this;
+    }
   }
 
   /** A {@link Spawn}'s metadata name and {@link Path}. */

diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
@@ -52,12 +52,10 @@
 import com.google.devtools.build.lib.server.FailureDetails;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.server.FailureDetails.Spawn.Code;
-import com.google.devtools.build.lib.shell.ExecutionStatistics;
 import com.google.devtools.build.lib.shell.Subprocess;
 import com.google.devtools.build.lib.shell.SubprocessBuilder;
 import com.google.devtools.build.lib.shell.TerminationStatus;
 import com.google.devtools.build.lib.util.NetUtil;
-import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.errorprone.annotations.FormatMethod;
@@ -470,29 +468,8 @@ private SpawnResult start() throws InterruptedException, ExecException, IOExcept
         if (status != Status.SUCCESS) {
           spawnResultBuilder.setFailureDetail(makeFailureDetail(exitCode, status, actionType));
         }
-        if (statisticsPath != null && statisticsPath.exists()) {
-          ExecutionStatistics.getResourceUsage(statisticsPath)
-              .ifPresent(
-                  resourceUsage -> {
-                    spawnResultBuilder.setUserTimeInMs(
-                        (int) resourceUsage.getUserExecutionTime().toMillis());
-                    spawnResultBuilder.setSystemTimeInMs(
-                        (int) resourceUsage.getSystemExecutionTime().toMillis());
-                    spawnResultBuilder.setNumBlockOutputOperations(
-                        resourceUsage.getBlockOutputOperations());
-                    spawnResultBuilder.setNumBlockInputOperations(
-                        resourceUsage.getBlockInputOperations());
-                    spawnResultBuilder.setNumInvoluntaryContextSwitches(
-                        resourceUsage.getInvoluntaryContextSwitches());
-                    // The memory usage of the largest child process. For Darwin maxrss returns size
-                    // in bytes.
-                    if (OS.getCurrent() == OS.DARWIN) {
-                      spawnResultBuilder.setMemoryInKb(
-                          resourceUsage.getMaximumResidentSetSize() / 1000);
-                    } else {
-                      spawnResultBuilder.setMemoryInKb(resourceUsage.getMaximumResidentSetSize());
-                    }
-                  });
+        if (statisticsPath != null) {
+          spawnResultBuilder.setResourceUsageFromProto(statisticsPath);
         }
         spawnMetrics.setTotalTimeInMs((int) totalTimeStopwatch.elapsed().toMillis());
         spawnResultBuilder.setSpawnMetrics(spawnMetrics.build());

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java
@@ -46,7 +46,6 @@
 import com.google.devtools.build.lib.server.FailureDetails;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.server.FailureDetails.Sandbox.Code;
-import com.google.devtools.build.lib.shell.ExecutionStatistics;
 import com.google.devtools.build.lib.shell.Subprocess;
 import com.google.devtools.build.lib.shell.SubprocessBuilder;
 import com.google.devtools.build.lib.shell.TerminationStatus;
@@ -309,28 +308,7 @@ private final SpawnResult run(
 
     Path statisticsPath = sandbox.getStatisticsPath();
     if (statisticsPath != null) {
-      ExecutionStatistics.getResourceUsage(statisticsPath)
-          .ifPresent(
-              resourceUsage -> {
-                spawnResultBuilder.setUserTimeInMs(
-                    (int) resourceUsage.getUserExecutionTime().toMillis());
-                spawnResultBuilder.setSystemTimeInMs(
-                    (int) resourceUsage.getSystemExecutionTime().toMillis());
-                spawnResultBuilder.setNumBlockOutputOperations(
-                    resourceUsage.getBlockOutputOperations());
-                spawnResultBuilder.setNumBlockInputOperations(
-                    resourceUsage.getBlockInputOperations());
-                spawnResultBuilder.setNumInvoluntaryContextSwitches(
-                    resourceUsage.getInvoluntaryContextSwitches());
-                // The memory usage of the largest child process. For Darwin maxrss returns size in
-                // bytes.
-                if (OS.getCurrent() == OS.DARWIN) {
-                  spawnResultBuilder.setMemoryInKb(
-                      resourceUsage.getMaximumResidentSetSize() / 1000);
-                } else {
-                  spawnResultBuilder.setMemoryInKb(resourceUsage.getMaximumResidentSetSize());
-                }
-              });
+      spawnResultBuilder.setResourceUsageFromProto(statisticsPath);
     }
 
     return spawnResultBuilder.build();

diff --git a/src/main/java/com/google/devtools/build/lib/shell/ExecutionStatistics.java b/src/main/java/com/google/devtools/build/lib/shell/ExecutionStatistics.java
@@ -16,6 +16,7 @@
 
 import com.google.devtools.build.lib.vfs.Path;
 import java.io.BufferedInputStream;
+import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InputStream;
 import java.time.Duration;
@@ -40,6 +41,10 @@ public static Optional<ResourceUsage> getResourceUsage(Path executionStatisticsP
       } else {
         return Optional.empty();
       }
+    } catch (FileNotFoundException e) {
+      // Collecting resource usage is best-effort and the file may be missing if the wrapper around
+      // the command terminated abnormally.
+      return Optional.empty();
     }
   }
 

diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java
@@ -728,6 +728,7 @@ protected void chmod(PathFragment path, int mode) throws IOException {
   /**
    * Creates an InputStream accessing the file denoted by the path.
    *
+   * @throws FileNotFoundException if the file does not exist
    * @throws IOException if there was an error opening the file for reading
    */
   protected abstract InputStream getInputStream(PathFragment path) throws IOException;