Consider invalid entries in the marker file reason for refetch

Especially due to #23127, older marker files may contain entries that don't even parse correctly. Instead of crashing Bazel, we should signal a refetch.

Fixes #23322.

Closes #23336.

PiperOrigin-RevId: 666416942
Change-Id: Ie5507654f69825d93f9523d9c209d417fb3cdaf6

src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java

@@ -26,6 +26,7 @@
 import com.google.devtools.build.lib.actions.FileValue;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.cmdline.LabelConstants;
+import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.LabelValidator;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction;
@@ -76,25 +77,32 @@ public abstract static class Parser {
 
  /**
  * Parses a recorded input from the post-colon substring that identifies the specific input: for
- * example, the {@code MY_ENV_VAR} part of {@code ENV:MY_ENV_VAR}.
+ * example, the {@code MY_ENV_VAR} part of {@code ENV:MY_ENV_VAR}. Returns null if the parsed
+ * part is invalid.
  */
  public abstract RepoRecordedInput parse(String s);
  }
 
  private static final Comparator<RepoRecordedInput> COMPARATOR =
- Comparator.comparing((RepoRecordedInput rri) -> rri.getParser().getPrefix())
- .thenComparing(RepoRecordedInput::toStringInternal);
+ (o1, o2) ->
+ o1 == o2
+ ? 0
+ : Comparator.comparing((RepoRecordedInput rri) -> rri.getParser().getPrefix())
+ .thenComparing(RepoRecordedInput::toStringInternal)
+ .compare(o1, o2);
 
  /**
  * Parses a recorded input from its string representation.
  *
  * @param s the string representation
- * @return The parsed recorded input object, or {@code null} if the string representation is
- * invalid
+ * @return The parsed recorded input object, or {@link #NEVER_UP_TO_DATE} if the string
+ * representation is invalid
  */
- @Nullable
  public static RepoRecordedInput parse(String s) {
  List<String> parts = Splitter.on(':').limit(2).splitToList(s);
+ if (parts.size() < 2) {
+ return NEVER_UP_TO_DATE;
+ }
  for (Parser parser :
  new Parser[] {
  File.PARSER, Dirents.PARSER, DirTree.PARSER, EnvVar.PARSER, RecordedRepoMapping.PARSER
@@ -103,7 +111,7 @@ public static RepoRecordedInput parse(String s) {
  return parser.parse(parts.get(1));
  }
  }
- return null;
+ return NEVER_UP_TO_DATE;
  }
 
  /**
@@ -172,6 +180,42 @@ public abstract boolean isUpToDate(
  Environment env, BlazeDirectories directories, @Nullable String oldValue)
  throws InterruptedException;
 
+ /** A sentinel "input" that's always out-of-date to signify parse failure. */
+ public static final RepoRecordedInput NEVER_UP_TO_DATE =
+ new RepoRecordedInput() {
+ @Override
+ public boolean equals(Object obj) {
+ return this == obj;
+ }
+
+ @Override
+ public int hashCode() {
+ return 12345678;
+ }
+
+ @Override
+ public String toStringInternal() {
+ throw new UnsupportedOperationException("this sentinel input should never be serialized");
+ }
+
+ @Override
+ public Parser getParser() {
+ throw new UnsupportedOperationException("this sentinel input should never be parsed");
+ }
+
+ @Override
+ public SkyKey getSkyKey(BlazeDirectories directories) {
+ // Return a random SkyKey to satisfy the contract.
+ return PrecomputedValue.STARLARK_SEMANTICS.getKey();
+ }
+
+ @Override
+ public boolean isUpToDate(
+ Environment env, BlazeDirectories directories, @Nullable String oldValue) {
+ return false;
+ }
+ };
+
  /**
  * Represents a filesystem path stored in a way that is repo-cache-friendly. That is, if the path
  * happens to point inside the current Bazel workspace (in either the main repo or an external
@@ -213,12 +257,12 @@ public final String toString() {
  return repoName().map(repoName -> repoName + "//" + path()).orElse(path().toString());
  }
 
- public static RepoCacheFriendlyPath parse(String s) {
+ public static RepoCacheFriendlyPath parse(String s) throws LabelSyntaxException {
  if (LabelValidator.isAbsolute(s)) {
  int doubleSlash = s.indexOf("//");
  int skipAts = s.startsWith("@@") ? 2 : s.startsWith("@") ? 1 : 0;
  return createInsideWorkspace(
- RepositoryName.createUnvalidated(s.substring(skipAts, doubleSlash)),
+ RepositoryName.create(s.substring(skipAts, doubleSlash)),
  PathFragment.create(s.substring(doubleSlash + 2)));
  }
  return createOutsideWorkspace(PathFragment.create(s));
@@ -264,7 +308,12 @@ public String getPrefix() {
 
  @Override
  public RepoRecordedInput parse(String s) {
- return new File(RepoCacheFriendlyPath.parse(s));
+ try {
+ return new File(RepoCacheFriendlyPath.parse(s));
+ } catch (LabelSyntaxException e) {
+ // malformed inputs cause refetch
+ return NEVER_UP_TO_DATE;
+ }
  }
  };
 
@@ -356,7 +405,12 @@ public String getPrefix() {
 
  @Override
  public RepoRecordedInput parse(String s) {
- return new Dirents(RepoCacheFriendlyPath.parse(s));
+ try {
+ return new Dirents(RepoCacheFriendlyPath.parse(s));
+ } catch (LabelSyntaxException e) {
+ // malformed inputs cause refetch
+ return NEVER_UP_TO_DATE;
+ }
  }
  };
 
@@ -439,7 +493,12 @@ public String getPrefix() {
 
  @Override
  public RepoRecordedInput parse(String s) {
- return new DirTree(RepoCacheFriendlyPath.parse(s));
+ try {
+ return new DirTree(RepoCacheFriendlyPath.parse(s));
+ } catch (LabelSyntaxException e) {
+ // malformed inputs cause refetch
+ return NEVER_UP_TO_DATE;
+ }
  }
  };
 
@@ -576,8 +635,12 @@ public String getPrefix() {
  @Override
  public RepoRecordedInput parse(String s) {
  List<String> parts = Splitter.on(',').limit(2).splitToList(s);
- return new RecordedRepoMapping(
- RepositoryName.createUnvalidated(parts.get(0)), parts.get(1));
+ try {
+ return new RecordedRepoMapping(RepositoryName.create(parts.get(0)), parts.get(1));
+ } catch (LabelSyntaxException | IndexOutOfBoundsException e) {
+ // malformed inputs cause refetch
+ return NEVER_UP_TO_DATE;
+ }
  }
  };
 
@@ -632,9 +695,14 @@ public boolean isUpToDate(
  throws InterruptedException {
  RepositoryMappingValue repoMappingValue =
  (RepositoryMappingValue) env.getValue(getSkyKey(directories));
- return repoMappingValue != RepositoryMappingValue.NOT_FOUND_VALUE
- && RepositoryName.createUnvalidated(oldValue)
- .equals(repoMappingValue.getRepositoryMapping().get(apparentName));
+ try {
+ return repoMappingValue != RepositoryMappingValue.NOT_FOUND_VALUE
+ && RepositoryName.create(oldValue)
+ .equals(repoMappingValue.getRepositoryMapping().get(apparentName));
+ } catch (LabelSyntaxException e) {
+ // malformed old value causes refetch
+ return false;
+ }
  }
  }
 }

src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java

@@ -703,19 +703,25 @@ private static String readMarkerFile(
 
  boolean firstLine = true;
  for (String line : lines) {
+ if (line.isEmpty()) {
+ continue;
+ }
  if (firstLine) {
  markerRuleKey = line;
  firstLine = false;
  } else {
  int sChar = line.indexOf(' ');
  if (sChar > 0) {
  RepoRecordedInput input = RepoRecordedInput.parse(unescape(line.substring(0, sChar)));
- if (input == null) {
- // ignore invalid entries.
+ if (!input.equals(RepoRecordedInput.NEVER_UP_TO_DATE)) {
+ recordedInputValues.put(input, unescape(line.substring(sChar + 1)));
  continue;
  }
- recordedInputValues.put(input, unescape(line.substring(sChar + 1)));
  }
+ // On parse failure, just forget everything else and mark the whole input out of date.
+ recordedInputValues.clear();
+ recordedInputValues.put(RepoRecordedInput.NEVER_UP_TO_DATE, "");
+ break;
  }
  }
  return markerRuleKey;

src/test/shell/bazel/starlark_repository_test.sh

@@ -2864,6 +2864,46 @@ EOF
  expect_log "I see: something"
 }
 
+function test_bad_marker_file_ignored() {
+ # when reading a file in another repo, we should watch it.
+ local outside_dir="${TEST_TMPDIR}/outside_dir"
+ mkdir -p "${outside_dir}"
+ echo nothing > ${outside_dir}/data.txt
+
+ create_new_workspace
+ cat > $(setup_module_dot_bazel) <<EOF
+foo = use_repo_rule("//:r.bzl", "foo")
+foo(name = "foo")
+bar = use_repo_rule("//:r.bzl", "bar")
+bar(name = "bar", data = "nothing")
+EOF
+ touch BUILD
+ cat > r.bzl <<EOF
+def _foo(rctx):
+ rctx.file("BUILD", "filegroup(name='foo')")
+ # intentionally grab a file that's not directly addressable by a label
+ otherfile = rctx.path(Label("@bar//subpkg:BUILD")).dirname.dirname.get_child("data.txt")
+ print("I see: " + rctx.read(otherfile))
+foo=repository_rule(_foo)
+def _bar(rctx):
+ rctx.file("subpkg/BUILD")
+ rctx.file("data.txt", rctx.attr.data)
+bar=repository_rule(_bar, attrs={"data":attr.string()})
+EOF
+
+ bazel build @foo >& $TEST_log || fail "expected bazel to succeed"
+ expect_log "I see: nothing"
+
+ local marker_file=$(bazel info output_base)/external/@+_repo_rules+foo.marker
+ # the marker file for this repo should contain a reference to "@@+_repo_rules+bar". Mangle that.
+ sed -i'' -e 's/@@+_repo_rules+bar/@@LOL@@LOL/g' ${marker_file}
+
+ # Running Bazel again shouldn't crash, and should result in a refetch.
+ bazel shutdown
+ bazel build @foo >& $TEST_log || fail "expected bazel to succeed"
+ expect_log "I see: nothing"
+}
+
 function test_file_watching_in_undefined_repo() {
  create_new_workspace
  cat > $(setup_module_dot_bazel) <<EOF