Work around outdated master merge commit due to PR merge order race condition

[ordian]

There are multiple examples (e.g. https://github.com/paritytech/polkadot/pull/2014) when Updating Substrate commit into a companion PR results in merge conflicts with master in Cargo.lock file. To prevent this, before making this commit, try to create a merge commit from the current master.

[joao-paulo-parity]

To prevent this, before making this commit, try to create a merge commit from the current master.

My findings points to this already being done https://github.com/paritytech/parity-processbot/blob/master/src/companion.rs#L121

Without having the logs from back then to know exactly, I suppose what happened was

• Automatic merge triggered due to the A8-mergeoncegreen label
• Companion commit made, which made the pipeline run again
• At the same time, #2038 was running and finished first
• Pipeline for companion commit finished, although the branch became outdated in relation to master because #2038 got in first

Possible checks to have in that situation:

• Check if target base ref on master is still the same between start and finish of pipeline, or
• Check for 405 status code in the merge request response as described in the API docs

Both should tell if the branch is outdated in relation to master; if it is, then create another merge commit and try again (reference: merge_pull_request).

[ordian]

Yes, there is an inherent race condition when merging two or more PRs and should be a reliable way to regenerate the cargo update -p sp-io commit when the head ref changes.

[ordian]

Here is another example of that race condition:

• https://github.com/paritytech/polkadot/pull/2504
• https://github.com/paritytech/polkadot/pull/2463

[joao-paulo-parity]

Here is another example of that race condition:

• paritytech/polkadot#2504
• paritytech/polkadot#2463

That looks like a different issue to me. Github claims that paritytech/polkadot#2463 was merged at 13:20, while the last non-bot commit before the failure on paritytech/polkadot#2504 was committed at 13:27. i.e. the pull request was already outdated at the time not being the bot's fault.

I think it is wrong to create this "Update Substrate" commit if the checks are not passing as e.g. the work might be in-progress or might have some other issue that wouldn't be fixed by updating Substrate. It also should not automatically merge the companion after pushing the commit for similar reasons. I'll open another issue for that.

[ordian]

I think it is wrong to create this "Update Substrate" commit if the checks are not passing as e.g. the work might be in-progress or might have some other issue that wouldn't be fixed by updating Substrate

This commit is only triggered when the substrate side of the companion is merged. And that has a checks for companion build.

the pull request was already outdated at the time not being the bot's fault.

If you look at the merge commit after Update Substrate commit, it didn't have any conflicts to resolve: https://github.com/paritytech/polkadot/pull/2504/commits/f32812354c03022dfb8bb811234a5301c1bcfa85 and it seems to me that the only conflict was in Cargo.lock file (compare 1 with 2). So my suspicion is that the Update Substrate didn't actually merge master after https://github.com/paritytech/polkadot/pull/2463 was merged, but only at some point before that when the merge command was issued. So this is a race condition.

[joao-paulo-parity]

This commit is only triggered when the substrate side of the companion is merged.

To be clear, the "checks" I mentioned were the ones in the companion PR. That might be why the statement got confusing.

So my suspicion is that the Update Substrate didn't actually merge master after paritytech/polkadot#2463 was merged, but only at some point before that when the merge command was issued. So this is a race condition.

This is feasible as well as the cargo command still has to run after the master merge commit, therefore the time at which the commit is made is not representative of how master was at that point in time.

I consider the behavior around the "Update Substrate" commit to be undesirable. To me it's a different problem than the one posted in the OP (recover from outdated master merge commit due to PR merge order), therefore I've opened #250 for the former.

[ordian]

This commit is only triggered when the substrate side of the companion is merged.

To be clear, the "checks" I mentioned were the ones in the companion PR. That might be why the statement got confusing.

Sorry if I wasn't clear, but what I meant is that the polkadot side simply can't be in "ill state" because it's build status is checked on the substrate side.