Consider invalid entries in the marker file reason for refetch

[Wyverald]

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.

[Wyverald]

Note that arguably the correct thing to do here is to always refetch instead of ignoring invalid entries -- although I'm not sure how much trouble we should go through for this corner case.

[fmeum]

Note that arguably the correct thing to do here is to always refetch instead of ignoring invalid entries -- although I'm not sure how much trouble we should go through for this corner case.

I would prefer that. If it saves us from debugging just one future incrementality issue with repo rules, it will have been worth it.

[Wyverald]

Fair point! Updated.

[moroten]

I ran some tests before I did see this fix, appending FILE:@bad#+~//:label foobar to marker files:

8.0.0-pre.20240826.1 is good
8.0.0-pre.20240821.2 is bad
7.3.1 is bad
7.3.0 is bad
7.2.1 is bad
7.1.2 is bad
7.0.2 is good
6.5.0 is good
6.4.0 is good
6.3.2 is good
6.2.1 is good
6.1.2 is good
6.0.0 is good

I've also observed src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java:

        content = FileSystemUtils.readContent(markerPath, UTF_8);
        String markerRuleKey = readMarkerFile(content, recordedInputValues);
        boolean verified = false;
        if (Preconditions.checkNotNull(ruleKey).equals(markerRuleKey)) {
          verified = handler.verifyRecordedInputs(rule, directories, recordedInputValues, env);
          if (env.valuesMissing()) {
            return null;
          }
        }

Because the ruleKey includes a marker version number, I feel like the safest way to be forward compatible is to skip parsing the rest of the marker file if the fingerprint differs. At the moment, all the parser code must be bug free in parsing any future format. @Wyverald What do you think about moving that check into readMarkerFile, passing ruleKey as parameter?

Please cherry-pick d62e0a0 into 7.4.0.
@bazel-io flag

[meteorcloudy]

@bazel-io fork 7.4.0

[Wyverald]

@Wyverald What do you think about moving that check into readMarkerFile, passing ruleKey as parameter?

Yeah this seems fine. If we pass in ruleKey, we can even just get rid of the String return and just return recordedInputValues directly, which is much cleaner.

Please cherry-pick d62e0a0 into 7.4.0.

I don't understand why this is necessary. We don't expect people to manually mess with their marker files.

[moroten]

Please cherry-pick d62e0a0 into 7.4.0.

I don't understand why this is necessary. We don't expect people to manually mess with their marker files.

The reason for cherry-picking is when switching branches. Switching from new branch using a new Bazel version to a branch using 7.1.x and 7.2.x will make Bazel crash if --incompatible_use_plus_in_repo_names was used. In the same way, the risk of crashing Bazel 7.4.x is reduced with this patch, when a future version of Bazel changes the format of the marker files again.

[Wyverald]

I see, that's a valid enough reason.