Delete the --experimental_delay_virtual_input_materialization flag.

This flag was added in 8fd4fe8 as an attempt to roll out a fix for a sandboxing bug, but it turned out not to be the right fix and it was never flipped.

PiperOrigin-RevId: 468168717
Change-Id: I3a58aaef9a26564e67da9558ff0d2f56e733b3e2

Author: tjgq

src/main/java/com/google/devtools/build/lib/sandbox/AbstractContainerizingSandboxedSpawn.java

@@ -115,7 +115,6 @@ public void createFileSystem() throws IOException {
     // Finally create what needs creating.
     SandboxHelpers.createDirectories(dirsToCreate, sandboxExecRoot, /* strict=*/ true);
     createInputs(inputsToCreate, inputs);
-    inputs.materializeVirtualInputs(sandboxExecRoot);
   }
 
   protected void filterInputsAndDirsToCreate(

src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java

@@ -15,7 +15,6 @@
 package com.google.devtools.build.lib.sandbox;
 
 import static com.google.common.base.Preconditions.checkNotNull;
-import static com.google.common.base.Preconditions.checkState;
 import static com.google.devtools.build.lib.vfs.Dirent.Type.DIRECTORY;
 import static com.google.devtools.build.lib.vfs.Dirent.Type.SYMLINK;
 
@@ -62,21 +61,6 @@ public final class SandboxHelpers {
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
   private static final AtomicBoolean warnedAboutMovesBeingCopies = new AtomicBoolean(false);
-  /**
-   * If true, materialize virtual inputs only inside the sandbox, not the output tree. This flag
-   * exists purely to support rolling this out as the defaut in a controlled manner.
-   */
-  private final boolean delayVirtualInputMaterialization;
-
-  /**
-   * Constructs a new collection of helpers.
-   *
-   * @param delayVirtualInputMaterialization whether to materialize virtual inputs only inside the
-   *     sandbox
-   */
-  public SandboxHelpers(boolean delayVirtualInputMaterialization) {
-    this.delayVirtualInputMaterialization = delayVirtualInputMaterialization;
-  }
 
   /**
    * Writes a virtual input file so that the final file is always consistent to all readers.
@@ -365,38 +349,21 @@ public static final class SandboxInputs {
         new AtomicInteger();
 
     private final Map<PathFragment, Path> files;
-    // Virtual inputs that are not materialized during {@link #processInputFiles}
-    private final Set<VirtualActionInput> virtualInputsWithDelayedMaterialization;
-    // Virtual inputs that are materialized during {@link #processInputFiles}.
-    private final Map<VirtualActionInput, byte[]> materializedVirtualInputs;
+    private final Map<VirtualActionInput, byte[]> virtualInputs;
     private final Map<PathFragment, PathFragment> symlinks;
 
     private static final SandboxInputs EMPTY_INPUTS =
-        new SandboxInputs(
-            ImmutableMap.of(), ImmutableSet.of(), ImmutableMap.of(), ImmutableMap.of());
+        new SandboxInputs(ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of());
 
     public SandboxInputs(
         Map<PathFragment, Path> files,
-        Set<VirtualActionInput> virtualInputsWithDelayedMaterialization,
-        Map<VirtualActionInput, byte[]> materializedVirtualInputs,
+        Map<VirtualActionInput, byte[]> virtualInputs,
         Map<PathFragment, PathFragment> symlinks) {
-      checkState(
-          virtualInputsWithDelayedMaterialization.isEmpty() || materializedVirtualInputs.isEmpty(),
-          "Either virtualInputsWithDelayedMaterialization or materializedVirtualInputs should be"
-              + " empty.");
       this.files = files;
-      this.virtualInputsWithDelayedMaterialization = virtualInputsWithDelayedMaterialization;
-      this.materializedVirtualInputs = materializedVirtualInputs;
+      this.virtualInputs = virtualInputs;
       this.symlinks = symlinks;
     }
 
-    public SandboxInputs(
-        Map<PathFragment, Path> files,
-        Set<VirtualActionInput> virtualInputsWithDelayedMaterialization,
-        Map<PathFragment, PathFragment> symlinks) {
-      this(files, virtualInputsWithDelayedMaterialization, ImmutableMap.of(), symlinks);
-    }
-
     public static SandboxInputs getEmptyInputs() {
       return EMPTY_INPUTS;
     }
@@ -425,8 +392,6 @@ public Map<PathFragment, PathFragment> getSymlinks() {
     private static byte[] materializeVirtualInput(
         VirtualActionInput input, Path execroot, boolean isExecRootSandboxed) throws IOException {
       if (input instanceof EmptyActionInput) {
-        // TODO(b/150963503): We can turn this into an unreachable code path when the old
-        //  !delayVirtualInputMaterialization code path is deleted.
         return new byte[0];
       }
 
@@ -447,31 +412,8 @@ private static byte[] materializeVirtualInput(
       return writeVirtualInputTo(input, outputPath);
     }
 
-    /**
-     * Materializes virtual files inside the sandboxed execroot once it is known.
-     *
-     * <p>These are files that do not have to exist in the execroot: we can materialize them only
-     * inside the sandbox, which means we can create them <i>before</i> we grab the output tree lock
-     * (but assuming we do so inside the sandbox only).
-     *
-     * @param sandboxExecRoot the path to the <i>sandboxed</i> execroot
-     * @return digests of written virtual inputs
-     * @throws IOException if any virtual input cannot be materialized
-     */
-    public ImmutableMap<VirtualActionInput, byte[]> materializeVirtualInputs(Path sandboxExecRoot)
-        throws IOException {
-      if (!materializedVirtualInputs.isEmpty()) {
-        return ImmutableMap.copyOf(materializedVirtualInputs);
-      }
-
-      ImmutableMap.Builder<VirtualActionInput, byte[]> digests =
-          ImmutableMap.builderWithExpectedSize(virtualInputsWithDelayedMaterialization.size());
-      for (VirtualActionInput input : virtualInputsWithDelayedMaterialization) {
-        byte[] digest =
-            materializeVirtualInput(input, sandboxExecRoot, /*isExecRootSandboxed=*/ false);
-        digests.put(input, digest);
-      }
-      return digests.buildOrThrow();
+    public ImmutableMap<VirtualActionInput, byte[]> getVirtualInputDigests() {
+      return ImmutableMap.copyOf(virtualInputs);
     }
 
     /**
@@ -481,19 +423,13 @@ public ImmutableMap<VirtualActionInput, byte[]> materializeVirtualInputs(Path sa
     public SandboxInputs limitedCopy(Set<PathFragment> allowed) {
       return new SandboxInputs(
           Maps.filterKeys(files, allowed::contains),
-          ImmutableSet.of(),
           ImmutableMap.of(),
           Maps.filterKeys(symlinks, allowed::contains));
     }
 
     @Override
     public String toString() {
-      return "Files: "
-          + files
-          + "\nVirtualInputs: "
-          + virtualInputsWithDelayedMaterialization
-          + "\nSymlinks: "
-          + symlinks;
+      return "Files: " + files + "\nVirtualInputs: " + virtualInputs + "\nSymlinks: " + symlinks;
     }
   }
 
@@ -519,66 +455,37 @@ private static byte[] writeVirtualInputTo(VirtualActionInput input, Path target)
    * Returns the inputs of a Spawn as a map of PathFragments relative to an execRoot to paths in the
    * host filesystem where the input files can be found.
    *
-   * <p>This does not (and must not) write any {@link VirtualActionInput}s found because we do not
-   * yet know where they should be written to. We have a path to an {@code execRoot}, but this path
-   * should be treated as read-only because we may not be holding its lock. The caller should use
-   * {@link SandboxInputs#materializeVirtualInputs(Path)} to later write these inputs when it knows
-   * where they should be written to.
-   *
    * @throws IOException if processing symlinks fails
    */
   public SandboxInputs processInputFiles(Map<PathFragment, ActionInput> inputMap, Path execRoot)
       throws IOException {
     Map<PathFragment, Path> inputFiles = new TreeMap<>();
-    Set<VirtualActionInput> virtualInputsWithDelayedMaterialization = new HashSet<>();
     Map<PathFragment, PathFragment> inputSymlinks = new TreeMap<>();
-    Map<VirtualActionInput, byte[]> materializedVirtualInputs = new HashMap<>();
+    Map<VirtualActionInput, byte[]> virtualInputs = new HashMap<>();
 
     for (Map.Entry<PathFragment, ActionInput> e : inputMap.entrySet()) {
       PathFragment pathFragment = e.getKey();
       ActionInput actionInput = e.getValue();
 
-      // TODO(b/150963503): Make delayVirtualInputMaterialization the default and remove the
-      // alternate code path.
-      if (delayVirtualInputMaterialization) {
-        if (actionInput instanceof VirtualActionInput) {
-          if (actionInput instanceof EmptyActionInput) {
-            inputFiles.put(pathFragment, null);
-          } else {
-            virtualInputsWithDelayedMaterialization.add((VirtualActionInput) actionInput);
-          }
-        } else if (actionInput.isSymlink()) {
-          Path inputPath = execRoot.getRelative(actionInput.getExecPath());
-          inputSymlinks.put(pathFragment, inputPath.readSymbolicLink());
-        } else {
-          Path inputPath = execRoot.getRelative(actionInput.getExecPath());
-          inputFiles.put(pathFragment, inputPath);
-        }
-      } else {
-        if (actionInput instanceof VirtualActionInput) {
-          byte[] digest =
-              SandboxInputs.materializeVirtualInput(
-                  (VirtualActionInput) actionInput, execRoot, /* isExecRootSandboxed=*/ true);
-          materializedVirtualInputs.put((VirtualActionInput) actionInput, digest);
-        }
+      if (actionInput instanceof VirtualActionInput) {
+        byte[] digest =
+            SandboxInputs.materializeVirtualInput(
+                (VirtualActionInput) actionInput, execRoot, /* isExecRootSandboxed=*/ true);
+        virtualInputs.put((VirtualActionInput) actionInput, digest);
+      }
 
-        if (actionInput.isSymlink()) {
-          Path inputPath = execRoot.getRelative(actionInput.getExecPath());
-          inputSymlinks.put(pathFragment, inputPath.readSymbolicLink());
-        } else {
-          Path inputPath =
-              actionInput instanceof EmptyActionInput
-                  ? null
-                  : execRoot.getRelative(actionInput.getExecPath());
-          inputFiles.put(pathFragment, inputPath);
-        }
+      if (actionInput.isSymlink()) {
+        Path inputPath = execRoot.getRelative(actionInput.getExecPath());
+        inputSymlinks.put(pathFragment, inputPath.readSymbolicLink());
+      } else {
+        Path inputPath =
+            actionInput instanceof EmptyActionInput
+                ? null
+                : execRoot.getRelative(actionInput.getExecPath());
+        inputFiles.put(pathFragment, inputPath);
       }
     }
-    return new SandboxInputs(
-        inputFiles,
-        virtualInputsWithDelayedMaterialization,
-        materializedVirtualInputs,
-        inputSymlinks);
+    return new SandboxInputs(inputFiles, virtualInputs, inputSymlinks);
   }
 
   /** The file and directory outputs of a sandboxed spawn. */

src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java

@@ -199,7 +199,7 @@ private void setup(CommandEnvironment cmdEnv, SpawnStrategyRegistry.Builder buil
     SandboxOptions options = checkNotNull(env.getOptions().getOptions(SandboxOptions.class));
     sandboxBase = computeSandboxBase(options, env);
 
-    SandboxHelpers helpers = new SandboxHelpers(options.delayVirtualInputMaterialization);
+    SandboxHelpers helpers = new SandboxHelpers();
 
     // Do not remove the sandbox base when --sandbox_debug was specified so that people can check
     // out the contents of the generated sandbox directories.

src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java

@@ -334,17 +334,6 @@ public ImmutableSet<Path> getInaccessiblePaths(FileSystem fs) {
               + " grows to the size specified by this flag when the server is idle.")
   public int asyncTreeDeleteIdleThreads;
 
-  @Option(
-      name = "experimental_delay_virtual_input_materialization",
-      defaultValue = "false",
-      documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
-      effectTags = {OptionEffectTag.EXECUTION},
-      help =
-          "If set to true, creates virtual inputs (like params files) only inside the sandbox, "
-              + "not in the execroot, which fixes a race condition when using the dynamic "
-              + "scheduler. This flag exists purely to support rolling this bug fix out.")
-  public boolean delayVirtualInputMaterialization;
-
   @Option(
       name = "incompatible_legacy_local_fallback",
       defaultValue = "true",

src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java

@@ -182,8 +182,6 @@ public Path getStatisticsPath() {
   public void createFileSystem() throws IOException {
     sandboxScratchDir.createDirectory();
 
-    inputs.materializeVirtualInputs(sandboxScratchDir);
-
     Set<PathFragment> dirsToCreate = new HashSet<>(writableDirs);
     for (PathFragment output : outputs.files()) {
       dirsToCreate.add(output.getParentDirectory());

src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java

@@ -74,8 +74,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
             context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
             execRoot);
 
-    readablePaths.materializeVirtualInputs(execRoot);
-
     ImmutableSet.Builder<Path> writablePaths = ImmutableSet.builder();
     writablePaths.addAll(getWritableDirs(execRoot, environment));
     for (ActionInput output : spawn.getOutputFiles()) {

src/main/java/com/google/devtools/build/lib/worker/WorkerExecRoot.java

@@ -63,8 +63,6 @@ public void createFileSystem(
     // we haven't seen what would break if we make it strict.
     SandboxHelpers.createDirectories(dirsToCreate, workDir, /* strict=*/ false);
     createInputs(inputsToCreate, inputs, workDir);
-
-    inputs.materializeVirtualInputs(workDir);
   }
 
   static void createInputs(Iterable<PathFragment> inputsToCreate, SandboxInputs inputs, Path dir)

src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java

@@ -31,7 +31,6 @@
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.runtime.commands.events.CleanStartingEvent;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers;
-import com.google.devtools.build.lib.sandbox.SandboxOptions;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.worker.WorkerPool.WorkerPoolConfig;
 import com.google.devtools.common.options.OptionsBase;
@@ -147,11 +146,10 @@ public void buildStarting(BuildStartingEvent event) {
   public void registerSpawnStrategies(
       SpawnStrategyRegistry.Builder registryBuilder, CommandEnvironment env) {
     checkNotNull(workerPool);
-    SandboxOptions sandboxOptions = env.getOptions().getOptions(SandboxOptions.class);
     LocalEnvProvider localEnvProvider = LocalEnvProvider.forCurrentOs(env.getClientEnv());
     WorkerSpawnRunner spawnRunner =
         new WorkerSpawnRunner(
-            new SandboxHelpers(sandboxOptions.delayVirtualInputMaterialization),
+            new SandboxHelpers(),
             env.getExecRoot(),
             workerPool,
             env.getReporter(),

src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java

@@ -373,19 +373,13 @@ WorkResponse execInWorkerWorkerAsResource(
     WorkResponse response;
     WorkRequest request;
     ActionExecutionMetadata owner = spawn.getResourceOwner();
-    ImmutableMap<VirtualActionInput, byte[]> virtualInputDigests;
+    ImmutableMap<VirtualActionInput, byte[]> virtualInputDigests =
+        inputFiles.getVirtualInputDigests();
+
     Stopwatch setupInputsStopwatch = Stopwatch.createStarted();
 
     try (SilentCloseable c =
         Profiler.instance().profile(ProfilerTask.WORKER_SETUP, "Preparing inputs")) {
-      try {
-        virtualInputDigests = inputFiles.materializeVirtualInputs(execRoot);
-      } catch (IOException e) {
-        restoreInterrupt(e);
-        String message = "IOException while materializing virtual inputs:";
-        throw createUserExecException(e, message, Code.VIRTUAL_INPUT_MATERIALIZATION_FAILURE);
-      }
-
       try {
         context.prefetchInputsAndWait();
       } catch (IOException e) {
@@ -521,19 +515,13 @@ WorkResponse execInWorkerClassic(
     WorkResponse response;
     WorkRequest request;
     ActionExecutionMetadata owner = spawn.getResourceOwner();
-    ImmutableMap<VirtualActionInput, byte[]> virtualInputDigests;
+    ImmutableMap<VirtualActionInput, byte[]> virtualInputDigests =
+        inputFiles.getVirtualInputDigests();
+
     try {
       Stopwatch setupInputsStopwatch = Stopwatch.createStarted();
       try (SilentCloseable c =
           Profiler.instance().profile(ProfilerTask.WORKER_SETUP, "Preparing inputs")) {
-        try {
-          virtualInputDigests = inputFiles.materializeVirtualInputs(execRoot);
-        } catch (IOException e) {
-          restoreInterrupt(e);
-          String message = "IOException while materializing virtual inputs:";
-          throw createUserExecException(e, message, Code.VIRTUAL_INPUT_MATERIALIZATION_FAILURE);
-        }
-
         try {
           context.prefetchInputsAndWait();
         } catch (IOException e) {

src/test/java/com/google/devtools/build/lib/sandbox/AbstractContainerizingSandboxedSpawnTest.java

@@ -377,7 +377,7 @@ private static SandboxInputs createSandboxInputs(
     }
     return new SandboxInputs(
         filesMap,
-        /*virtualInputs=*/ ImmutableSet.of(),
+        /*virtualInputs=*/ ImmutableMap.of(),
         symlinks.entrySet().stream()
             .collect(
                 toImmutableMap(