From 7d87996d2c2018f0c6dd9b200482320d0e40f024 Mon Sep 17 00:00:00 2001
From: Googler <plf@google.com>
Date: Thu, 26 Oct 2023 09:01:14 -0700
Subject: [PATCH] Fix bug in reuse_sandbox_directories

#19935 gives a clear repro. The problem appears when a stashed sandbox
contains a directory whose same path is a regular file in a later
execution. If we try to reuse the stashed sandbox we trigger an error if
the old directory contained any files.

This was due to simply calling delete() without checking first if it
was a directory. The fix is to call deleteTree() instead in those cases.
directory.

Fixes #19935

RELNOTES:none
PiperOrigin-RevId: 576889004
Change-Id: I73b145cd574b83c473ffaccd90b675eb5f086c0e
---
 .../devtools/build/lib/sandbox/SandboxHelpers.java |  2 ++
 .../build/lib/sandbox/SandboxHelpersTest.java      | 14 +++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
index 7c140fc5dff2eb..fb72d591e0781e 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
@@ -184,6 +184,8 @@ private static void cleanRecursively(
         if (SYMLINK.equals(dirent.getType())
             && absPath.readSymbolicLink().equals(destination.get())) {
           inputsToCreate.remove(pathRelativeToWorkDir);
+        } else if (absPath.isDirectory()) {
+          absPath.deleteTree();
         } else {
           absPath.delete();
         }
diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java
index e5cca82a0b46a3..8df844739de17e 100644
--- a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java
+++ b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java
@@ -293,7 +293,19 @@ public void cleanExisting_updatesDirs() throws IOException, InterruptedException
     // outputDir only exists partially
     execRootPath.getRelative(outputDir).getParentDirectory().createDirectoryAndParents();
     execRootPath.getRelative("justSomeDir/thatIsDoomed").createDirectoryAndParents();
-    SandboxHelpers.cleanExisting(rootDir, inputs, inputsToCreate, dirsToCreate, execRootPath);
+    // `thiswillbeafile/output` simulates a directory that was in the stashed dir but whose same
+    // path is used later for a regular file.
+    scratch.dir("/execRoot/thiswillbeafile/output");
+    scratch.file("/execRoot/thiswillbeafile/output/file1");
+    dirsToCreate.add(PathFragment.create("thiswillbeafile"));
+    PathFragment input4 = PathFragment.create("thiswillbeafile/output");
+    SandboxInputs inputs2 =
+        new SandboxInputs(
+            ImmutableMap.of(input1, inputTxt, input2, inputTxt, input3, inputTxt, input4, inputTxt),
+            ImmutableMap.of(),
+            ImmutableMap.of(),
+            ImmutableMap.of());
+    SandboxHelpers.cleanExisting(rootDir, inputs2, inputsToCreate, dirsToCreate, execRootPath);
     assertThat(dirsToCreate).containsExactly(inputDir2, inputDir3, outputDir);
     assertThat(execRootPath.getRelative("existing/directory/with").exists()).isTrue();
     assertThat(execRootPath.getRelative("partial").exists()).isTrue();