Skip to content

Commit

Permalink
[7.1.1] Fix race condition and add more logging for null entry error …
Browse files Browse the repository at this point in the history
…message (#21692)

Attempts to address NPE reported in:
bazelbuild/bazel-skylib#488 (comment)
and
#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 #21668.

Commit
59dbf7a

PiperOrigin-RevId: 615739651
Change-Id: Ida90e334d1d1f890cf204d272134726bb1f70eb9

Co-authored-by: Pedro <plf@google.com>
  • Loading branch information
bazel-io and oquenchil authored Mar 14, 2024
1 parent 7c7c5c3 commit 49166bd
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 49166bd

Please sign in to comment.