Delete marker file before fetching an external repository

[fmeum]

Fetching a repository is a long-running operation that can easily be
interrupted. If it is and the marker file exists on disk, a new
evaluation of the RepositoryDelegatorFunction may treat this repository
as valid even though it is in an inconsistent state.

Clearing the marker file before initiating the fetch and only recreating
it after the fetch is complete prevents this scenario.

Fixes #8993.

[philwo]

LGTM, but let's ask Yun, too! :D

[meteorcloudy]

Hmm, I don't think this is necessary. Because:

• If fetchRepository is executed, then that means the existing marker file isn't consistent anymore.
• When fetchRepository is interrupted, the marker file shouldn't be rewritten because we do that after fetchRepository is done.
• Therefore, the next time you execute this SkyFunction, the marker data should still be inconsistent and fetchRepository should be re-executed.

Do you have a minimal reproducible case that we can look into?

[fmeum]

Hmm, I don't think this is necessary. Because:

• If fetchRepository is executed, then that means the existing marker file isn't consistent anymore.
• When fetchRepository is interrupted, the marker file shouldn't be rewritten because we do that after fetchRepository is done.

This is correct.

• Therefore, the next time you execute this SkyFunction, the marker data should still be inconsistent and fetchRepository should be re-executed.

As far as I can tell, this is not true when dealing with a managed directory: The marker file only tracks the existence of the managed directory, not its contents. If the SkyFunction is interrupted at a point where the managed directory has been created but its contents aren't consistent with what the ruleset expects, it will not rerun fetchRepository, but the build likely fails.

Every project using rules_nodejs's yarn_install rule should serve as a reproducer. For example, try the following:

1. git clone https://github.com/bazelbuild/rules_nodejs
2. cd examples/react_webpack
3. bazel build //...
4. rm -rf node_modules
5. bazel build //..., interrupting the build soon after it shows Fetching @npm; Running yarn install on //:package.json. If your internet connection or local yarn cache is too fast, this might require adding new and large dependencies to package.json so that you have a window to interrupt the execution of the repository rule.
6. bazel build //... should fail with an error such as

ERROR: error loading package '': Every .bzl file must have a corresponding package, but '@npm//webpack-cli:index.bzl' does not have one. Please create a BUILD file in the same or any parent directory. Note that this BUILD file does not need to do anything except exist.

At that point, the build failure is persistent and can only be resolved by deleting node_modules (or running bazel clean --expunge).

[meteorcloudy]

I see, that makes sense now, basically unfinished fetchRepository could accidentally cause the previous marker data to be considered consistent due to the use of managed directories.

[meteorcloudy]

Thanks for the fix and explaination!

[brentleyjones]

This is a nice little bug fix. Think we can cherry-pick it into 5.0?

[fmeum]

This is a nice little bug fix. Think we can cherry-pick it into 5.0?

I submitted it for both 5.0.0 and 4.2.2.