Skip to content

Commit

Permalink
Fix race condition and add more logging for null entry error message
Browse files Browse the repository at this point in the history
Attempts to address NPE reported in: bazelbuild/bazel-skylib#488 (comment) and bazelbuild#21665 (comment)

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: bazelbuild/bazel-skylib#488 (comment).

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

Closes bazelbuild#21668.

PiperOrigin-RevId: 615739651
Change-Id: Ida90e334d1d1f890cf204d272134726bb1f70eb9
  • Loading branch information
oquenchil authored and bazel-io committed Mar 14, 2024
1 parent 7c7c5c3 commit a53e923
Showing 1 changed file with 26 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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.
Expand Down

0 comments on commit a53e923

Please sign in to comment.