CI failing with Git checkout failed with exit code: 128

[ericstj]

I've seen this happen in various different legs of the CI. Here's one:
https://dev.azure.com/dnceng/public/_build/results?buildId=452699

error: Could not read 8be6ae3a888131720cdebd36f27eb58be0553e27
fatal: internal error in revision walk
##[warning]Git checkout failed on shallow repository, this might because of git fetch with depth '5' doesn't include the checkout commit 'refs/remotes/pull/457/merge'. Please reference documentation (http://go.microsoft.com/fwlink/?LinkId=829603)
##[error]Git checkout failed with exit code: 128

It looks to me like this is due to the way we are merging PRs. If I understand this correctly, it looks like it's complaining that it doesn't have the commit the PR was based from. Should we increase this depth or merge PRs in a different way during CI?

[ericstj]

/cc @safern @ViktorHofer

[ViktorHofer]

cc @trylek @jkoritzinsky

[trylek]

We always knew this is a heuristic value without real solid background. As I understand this, fetchDepth:5 basically means that HEAD through HEAD~4 gets pulled down for each branch. Assume that, at the time of scheduling the job, the head is at a hypothetical commit 1000. If by the time we get to the checkout job and we specify fetchDepth:5 the HEAD is at 1005, we're in trouble because just commits 1001 through 1005 get fetched and subsequently syncing to the commit 1000 fails. I suspect this can be very easily aggravated by the current slowdown in machine assignment, I believe @jaredpar claimed they never hit any issues in Roslyn with fetchDepth equal to 5 before.

[jaredpar]

I believe @jaredpar claimed they never hit any issues in Roslyn with fetchDepth equal to 5 before.

That is true but we also have less PR volume through our repository than dotnet/runtime does. If we're seeing this then we should up the fetch depth until it stops happening. It's a heuristic so no way to be definitive about this.

[trylek]

I concur in general but I doubt it merits investigating before the current infra issues have been addressed. I would suggest let's put this on a back burner for about a week before things settle down and let's reassess afterwards unless this is a massive problem hitting everyone which is something I'm not aware of.

[ericstj]

I've seen it come up in 3 different PRs. I opened the issue because it's enough of a recurring nuisance that I wanted to make sure we tracked it. If we don't fix this, what's the workaround? Will rerunning the failing legs fix it? I tried this once and it didn't seem to fix it, was i just losing the race again?

[trylek]

Thinking about this more I doubt retrying can fix jobs like these, I guess it'll just make them worse. I'm OK with removing the fetchDepth parameter from the checkout step if there's general agreement this is what we want to do. It's actually questionable whether this was a real improvement in the first place as it's rather a trade-off - when you don't specify fetchDepth, GitHub just throws the entire branch history at you; in contrast, with fetchDepth, you receive fewer data but GitHub is more heavily loaded to repack the changes according to the filtering. So it's something like GitHub server perf vs. network bandwidth to some extent.

[jaredpar]

Rather than removing the paramater I would just increase until we get rid of the failures. Restricting the fetch depth does save us in PR times (due to reduced download). Other repositories, including coreclr and corefx, used it successfully for some time. It's just the activity of the combined repository is making our guard of 5 not big enough. I'd start with 20 or so and see if it goes away. My expectation is that it will.

[safern]

Note that I've only seen these failures on the installer builds which currently doesn't use the single checkout step and then use the bundle to do the full checkout on subsequent jobs.

I haven't seen the checkout bundle step fail with this error. So it might just go away once: #705 is merged as it moves the installer builds to use the checkout job?

[trylek]

For a short-term workaround, while I understand it's somewhat annoying I believe it to be fixable by closing and reopening the PR in question which is a technique that's been widely used forever to work around arbitrary non-deterministic infra issues.

[jaredpar]

Changing the fetch depth is trivial though. It's a single entry in the build definition. Is there a reason I'm missing where we wouldn't just up it to 10 / 20 and see if the problem goes away?

In general I dislike approaches like that but this is fundamentally a heuristic hence the "best" way to approach the problem.