From a53e923c3fb8e59e42342535bed27db29f283f06 Mon Sep 17 00:00:00 2001
From: Pedro <plf@google.com>
Date: Thu, 14 Mar 2024 05:09:13 -0700
Subject: [PATCH] Fix race condition and add more logging for null entry error
 message

Attempts to address NPE reported in: https://github.com/bazelbuild/bazel-skylib/issues/488#event-11993414211 and https://github.com/bazelbuild/bazel/issues/21665#issuecomment-1993967297

The `put()` call to the runfiles dir map is placed before the call that stashes the corresponding directory to address the race condition described here: https://github.com/bazelbuild/bazel-skylib/issues/488#issuecomment-1964209720.

The exception will now log:
- entries in the runfiles dir map
- environment variables
- stashes on disk

Closes #21668.

PiperOrigin-RevId: 615739651
Change-Id: Ida90e334d1d1f890cf204d272134726bb1f70eb9
---
 .../build/lib/sandbox/SandboxStash.java       | 30 ++++++++++++++++---
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStash.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStash.java
index f0055c3e667710..9ff727de4990a2 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStash.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStash.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.sandbox;
 
+import com.google.common.base.Preconditions;
 import com.google.common.flogger.GoogleLogger;
 import com.google.devtools.build.lib.exec.TreeDeleter;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs;
@@ -95,8 +96,29 @@ private boolean takeStashedSandboxInternal(
           stashExecroot.renameTo(sandboxExecroot);
           stash.deleteTree();
           if (isTestAction(mnemonic)) {
-            Path stashedRunfilesDir =
-                sandboxExecroot.getRelative(stashPathToRunfilesDir.get(stashExecroot));
+            String relativeStashedRunfilesDir = stashPathToRunfilesDir.get(stashExecroot);
+            if (relativeStashedRunfilesDir == null) {
+              StringBuilder errorMessageBuilder = new StringBuilder();
+              errorMessageBuilder.append(
+                  String.format("Current stashExecroot:%s\n", stashExecroot));
+              errorMessageBuilder.append("Stashes:\n");
+              for (Path path : stashes) {
+                errorMessageBuilder.append(path.getPathString()).append("\n");
+              }
+              errorMessageBuilder.append("Environment:\n");
+              for (var entry : environment.entrySet()) {
+                errorMessageBuilder.append(
+                    String.format("%s : %s\n", entry.getKey(), entry.getValue()));
+              }
+              errorMessageBuilder.append("Entries runfiles map:\n");
+              for (var entry : stashPathToRunfilesDir.entrySet()) {
+                errorMessageBuilder.append(
+                    String.format("%s : %s\n", entry.getKey(), entry.getValue()));
+              }
+              Preconditions.checkNotNull(
+                  relativeStashedRunfilesDir, errorMessageBuilder.toString());
+            }
+            Path stashedRunfilesDir = sandboxExecroot.getRelative(relativeStashedRunfilesDir);
             Path currentRunfiles = sandboxExecroot.getRelative(getCurrentRunfilesDir(environment));
             currentRunfiles.getParentDirectory().createDirectoryAndParents();
             stashedRunfilesDir.renameTo(currentRunfiles);
@@ -154,11 +176,11 @@ private void stashSandboxInternal(
               path.getRelative("execroot/" + environment.get("TEST_WORKSPACE") + "/_tmp"));
         }
       }
-      path.getChild("execroot").renameTo(stashPathExecroot);
       if (isTestAction(mnemonic)) {
-        // We only do this after the rename operation has succeeded
+        // We do this before the rename operation to avoid a race condition.
         stashPathToRunfilesDir.put(stashPathExecroot, getCurrentRunfilesDir(environment));
       }
+      path.getChild("execroot").renameTo(stashPathExecroot);
     } catch (IOException e) {
       // Since stash names are unique, this IOException indicates some other problem with stashing,
       // so we turn it off.