Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retry invocation if Bazel exits with code 39 #425

Closed
wants to merge 1 commit into from

Conversation

coeuvre
Copy link
Member

@coeuvre coeuvre commented Feb 15, 2023

With bazelbuild/bazel#17358, Bazel will exit with code 39 if remote cache evicts blobs during the build. With bazelbuild/bazel#17462, Bazel is able to continue the build without bazel clean or bazel shutdown.

This PR makes bazelisk automatically retries the invocation if it sees exit code 39 from Bazel, i.e. "rewinding" on a larger scale.

@coeuvre
Copy link
Member Author

coeuvre commented Feb 15, 2023

cc @tjgq @meisterT

@brentleyjones
Copy link

This will be a very nice change to go along with the Bazel 6.1 changes.

Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should guard this behaviour behind a flag? It could be a bit surprising for users that Bazelisk is automatically retrying Bazel invocation.

Also can we add some documentation and ideally a test for this use case?

func runBazel(bazel string, args []string, out io.Writer) (int, error) {
retries := 0
for {
exitCode, err := runBazelOnce(bazel, args, out)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably print out some message notifying Bazelisk is retrying the bazel invocation?

@coeuvre
Copy link
Member Author

coeuvre commented Feb 22, 2023

The exit code 39 is new in Bazel and is guarded by flag in 6.1.0. I would rather not add a new flag here because we certainly want to retry on 39. We can update documentation to mention this behaviour though.

Adding test is tricky. It's hard to make Bazel exits 39 without directly controlling the remote CAS. We have integration test in Bazel with the help of remote worker. I don't think there is an easy way to mimic it here.

Using a mock also requires code refactor around runBazel (e.g. passing a runner interface). If it sounds good, I can go ahead and add a unit test with mock.

@meteorcloudy
Copy link
Member

Thanks for the context.

If it sounds good, I can go ahead and add a unit test with mock.

Yes, that sounds good enough!

@fweikert
Copy link
Member

I wouldn't complain about a test either :)

@coeuvre
Copy link
Member Author

coeuvre commented Mar 1, 2023

Per internal discussion, we probably want to include the retry in Bazel itself. Closing for now.

@coeuvre coeuvre closed this Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants