Source code updates resulted in reverting 3 PRs in dotnet/runtime

[jeffhandley]

Pull request

dotnet/runtime#118514

Type of problem

Incorrect contents in the PR

Description of the issue

The [main] Source code updates from dotnet/dotnet (dotnet/runtime#118514) PR looked correct when we scanned through it, but we overlooked that some of the changes it included were actually reverting a few recent PRs into dotnet/runtime rather than reflecting those changes.

These PRs were reverted with this code flow:

• fixing a regression in regions related to UOH allocations during BGC (dotnet/runtime#118567)
• Make FilePatternMatch.Stem non-nullable. (dotnet/runtime#118410)
• Ensure we track MakeGenericType (dotnet/runtime#118479)

Each of those PRs was merged during the PT evening of August 11. The code flow PR was created August 7 and the bef141ffdc5f5d87678e81b2fb271e5d3c7326d7 commit (Backflow from https://github.com/dotnet/dotnet / ca3dd2c build 278938) was added into the PR on the PT morning of August 12, with that "Backflow" commit causing the revert.

@mmitche created a PR to Revert backflow and re-apply VMR build (dotnet/runtime#118657), which will remedy this particular error, and he filed Local backflow doesn't operate like the VMR backflow (dotnet/arcade-services#5147) to capture another issue observed while fixing this.

/cc @Maoni0 @mangod9 @tannergooding @MichalStrehovsky

[SimonZhao888]

Backflow PR: dotnet/sdk#50254

[adamzip]

I took a look at the situation. It seems that 5 commits from the repo were reverted with the last backflow.

Here's a snippet from the diagram of recent codeflows:

• The runtime main branch is on the left, VMR main branch is on the right (from top to bottom: most recent to oldest)
• The green arrow is the last forward flow that happened before the problematic backflow
• The red arrow is the problematic backflow
• In orange are the 5 commits that the backflow reverted (*)

The problematic backflow PR was open for a while, and backflowed several builds while it was open. The first few backflows were normal, but then came the one that caused the reverts: dotnet/runtime@bef141f

It was committed on Aug 12th at 7:31PM (GMT+2).

See the full graph link

https://edotor.net/?engine=dot#deflate:eNq9XEuPG0lyvs+vIPpkA7OjfD+86AXWPtiHXR9mbV/GCyLyJRFDkQ02NfKs4f/uL6uyHmqyd4rNQgMSpGQyIjMqMiK+iMhi2n080dOnzb9u/ve7zeZEh5/T7vT4H//8+83mw4fN+fiEv5twPJ+Pnzdlf/y6Of6ST7Tf48vPT/vdIT8/Fto/5/77fzmfaPfx03lDp9Px6zO+dPxyfvpyPp5SPj3m9DE/l93p+dx/+9+PKT9vjodumX+IlfPu8LHR/iOID/jC5qfnT/SUH8Pxf77fPJ9/3efHstvvc/p+U/+Nx/3x9Pj10+6c8cHxcO4/CPsvbXygz/nx4Y+nHe0f+k+ed3/Lj5z99fffffddt48fuy2D8MvnQ7fmM7532py+HM67z3lzyk/H5x1k+HXzdXf+tPnLv/1xs6eQ988bOqTNf/74pyro8O2fuqnHhzZ+mG9zXxf6Ne/xHL+vdI8Pn87np+d/+vDhIxh/CT/E4+cP6Xg+5POHgR7b3HRb2G63bOT+5+PzGZ/GfKgb//x5d35YyvEDPnqiU/5gvbOpKOeZ0VLzkEIkEUzSjrksDDFtguOe/fDDDyLp6J0wJsgYTbasqODxgVeFkRCmSMZ5MQqbONPpYz4/PmzDHofpYa6Uh6qVh0GLD4Maw3GfvhGzaFlKNqOwbXyThHgiHxqdV1xaGaLUVKKgInNkQvPovLTRW5HX2TUf90vMWiHVBg9uIwo5QeK/Dz8J0TT1/NfbddXYFG6DUC4G8toSlIG/TKhsTUicKcOxZFtdaBu5TMxnVbhj2WevRCxBhxxUTqKsI3WMPOAhj7K38c26anRKKyLNomYshhJIsUhOJpE4BeuCc+vsWoz7tdxFa02nK81s0SxDV/IOVTUuOmsdsyaXioghJMucCiIVHYyGSpjEim3xANW6KCUznJIrzArjCdQKj0R7TWkloYtWgeQoehvfrKpGp521jBlBxsIh+BRyMT5JA6dUikmJr7Prab/cspyU7VSVWeAqe6jK3KGqxkU7H5WWlBSpRDml4LTM0JOgkJimiBXb4oYnKlwGxZQrLhuWTTGZfHbCWQUlryO0ocB0mU5pG9+sqkYnU4Enh9dwIfBsfYTDkOR00HAeimdxdddj2Ho4Ahd8zC8lofjzK6J8v3nKh6+7dP70KL9x7AZhgs8cez++3bH3dAHG4yn4oHNhxhQOtx50govTQmZL8Z3EchynQqhRrDa+WaxGJzJ3okj4BZU81znC0wduIs9OC2VeiVfrixUzZ8WUybX349tde08HMzPcIijBsaViDfMScmVyPOVYmHq3Q8hhqcpPh7Af334IezpWYvE5UEb44gXu3WtprEKwdiZxpcM7iRWEsfBbo1htfLNYja4kRsmaaBGRndU2JBGCNwSJbSkirQSa1Bw4KC9D591VSUAqHN6d6zvce2Mjs8DmGYf/RjAV1T1I7+BDSAMzBWWxZFtdBReS0s7FnBhXpA0IEdIM99Zqn/xKoMl7PGI7yd6Pb7esnk7E5KzQCQgXAmjObSQtnQR4DwzAYiWo58HLiWnXbXzzrhsdoB0yBhLwBoYV+LoiAmc+SG4Qebmy6+x6sggDrRZJOGGbWIR1Jt5+pBoh4xEmTxhILZAuAU4HaUkYLp015BWOVFsukmS6gnQlcwa68yYEm1iykhcihLF1xEzSwxelUdg2vlk5jc7YlITypgDbcZUEEkLKysjga1gqbiXlTFBACsBf7zvzp5q3xc787R3m39gAmEEonCkevc42FB4lAc5xfGaQEdWcqa0OEJGRFCb8YYJrZSVQe4wBVL4A7dJKujI8kp+S+Ta+XVc9nTSiOB5iyoHD0RknkHBAWoEcqsDMwjq7nhm+CVIg1FRdBR2ZTQq68neoqnEJJXsvXKiZbMqOlygNS5pqkE1QS8GKbXEYmpKWB68cADuAd7KIvoGACJVH/iXXEToT0Itxo+htfLOqGp1RJgWCIF4WHW0FdTh4OHqKMFot05v2S9HpBAzXmRVASfSdWbF7zKpnA/AN5cAHAv0wyTwTwit8bmVCoq6YqKWIfnWuEXmVQ+St6azlPlE0RWXSjiTpslLSJOGYg5tcYBvfrKtG50UQwHMyaJVjjbJcCjiFCOlEYoWttOsJiOLcO2F6s+JWCOCue8tGjY2DHzDOwsEpE6OIFh4jAdBwxsllID4s2VZ31c9l54oCgEgSpsZkJYQOq+jSrCN1gNXaECa02o9vR6s9nZKGjA84mQWICKFW2sASMJvi8BHEV3KBfPLZWXpfSm9Y2gTAfarK4vcUjno2lrjWWgJRMKS4LBuhM/TkFeM2S+4dlmyrG7jNXOEIi0AetlhWlQY11kigs18pXpFysqSpENPGNyur0UUVQzQaEJwCMxb5Bo5ZhMdgRkFj8j5E1Jf0/+V4OOR47iRoJf3dYUObX/LpvIu038RPtDt8o9nf/eFC3q4T8SlTejwcD/n7zdBbQHZ27TBPHOpBWUjsZ2SjVSykHTzexMEvpnUzqtFzLqQdouLEwS2mtTOqMboupB2A08TBLqY1M6oRgC1dt4HriYNZTKvn6w4gfenZaBnXxEEvph1yzNnZGPK3hRzUjHbMWJeaRKtGTBzUYtqhQDOzi6G2sVT2VrmaOIxFn6XW0Up6sycwVMOWStFqnTMbGcqES09sKwLPpBjqp0v9w/zEDiXlhbRDt2DmJZbTzqjGrsNS3bWG0ozDYlo+19fQmFqqr9Z0nDnxxbRzjzQ2L3+TtjWMJ9IFDgmh7U+5XGlW/9eff1zYqK7fHCI5/n97g5oSff7b7qkF+R4p/PL5tHWstiX5lqzxhtxbu9Xfsp/Sjp5rcC56ZazyljvupY5eJJVl1DYFU6gW89pGZDE8Z14kjxl5VhEeSCohBYtIE4W/G8pWmaE0HjXxqUrexjcJWMFRo9MsSV44IG2RminmgGaZ4sBLFravX6mS37ZpDVeYRdxqxYrQNOtct5ke1fazQLVqCah9RW2NC5L3YILJTEVhkP4lUgThlNAS6C+JWoRpiwdiSOYDI85JcUYpI82E7iQgSu2ylVXUJl2QSpVZstiNb1Zbo/NMpshxMn0yhjSPAONGQTxy0pVwdzGzbppSyUjXtknoWjSdNbHbTKe2NluzfHeH3hqboA2DqaTCkbLjPNbWrvCQkysoTnVd7La6lMg2VSxJGcYsPisZoF4HnXUhSjytordIMiWc0LF03o9v1lujE8lrUj6UzCxZpS1S4kAGqbGMhSuxxqZddUQybrPAQ2J+1tFuM31Hu5+909waF5mM44S0mGTMKQMzkBRBMgVnSZFEvdPTFs9ehxSE49qaqJ0LWUlhFLSdkKoVkquoTQsWkYlPjqYf36y2RqeFtSXyYkUK8O0qEZORNGZ1MtFlvcKmPROEB7YtysXIxKxV1WY6tbXZam7mDr01NsobCxWYGLNGbENyTIUhxHEPvTJSta/QVmeOW+YDNyX6lH00sMca1LQT0dYUexW9WeJSzFL/Nr5Zb41OYq9IvETQOJgCn3g4iKKIiuVKm7KGueHpWVIGObhQIdhZA6jN9AW2fnZxMfQVvTU2riQBMRw3CkeQhyzhL3U2GqBb8GQTlmyrW45zGpy0OsQQva0tfooW59d6kVVYB5XAoE0sbNYN7sY3663RIaQpnoQsQSdmdImIAEI6VbQnhxDN1nCTqmSh7BZYB4iOZr2hNtPrrZ+F3vQ9auu5ZGmyVwlAKzCru6IZt7IkXbhXZGJXF+0XtxBXaig5S+uBQvEoEAODSbmYAKfq10ElGoBV0YRK+vHtqKSnY14zCZ8PEMnJcoTpWJvYAqCMSQCyNdRWQsDTC1vN6jn2szZRm2lX67rZam53ocmeDTBxEJK7EDwzzHsjoIRYa9qe53pnC0u21ZVViGoR7jJp2JfN1lvvpTMZZkiApqvoLUudROCz6nA3vllvjc4HG4UjlV3xkrIVujgnsevq5y1A8CqoROBxia1UzvBCs55Rm+n01maX3rN7RW2NSxQqaytz0RpZmTYpEktFIGYFxYLjqUMl3eKY4s6rDPcKG+UVn2nmhQkuIjPKWayiNjxoydjUgWnjm9XW6LINIRgPYMKlVxFhGGjZVGGQzzm1SuZiAL4lvGQkH4uLs/ZRm+nU1mbv9JKNi3eI2DiZWsgESA+jK8wbeEUpMxde53rZoV9cwbqiECLjAEuPOEhWEsaBFSdNkOuAEslChGVMXrIf3+4lezoRAP6jiC5KMpRS8Mo4OHzDkK7Cs6yBgIWV1gja4gkCErh5J6lN9ebWT9dO0j3m1nOJTiYOWzPGAC7q4oOumBlhPNQr5B2YbIu7wmvaYxDcI0IasJgnVQzDM6lKN3ydJECzCI87JQH9+PYkoKczjEflEbGLNCzUu3eW28hwXAEpgcLyGuamncIhgN6kQK6x5ZOPb1NNb9304gtrryquY1PqneRgPfI3IHyBVDwAHhdk3KEEBILaAWyrx9qMzybKFCPAdPDIIljiEtl495bDOsUSB4yUxKxT3Y9vVlyjEwWunedcyIaEKM4EokMpXBjgE5v9GuEtu+xCKluXEViM3vKpWtKmOsW16XtxSWPjbESOI4nzjGzU+0jIzBK8YTSwOJNDbd32qycotr6GgYCRuJKJCZidzNYZ4ZDGebZO+sbgkst0JaKNb0/fejoczphraYtHQOHIoDOdFVBlcUny+y8adhZXbOaUtj46wRQsbko+21SnuDZ9b5mrsSFrXKTsYs2qPQTxVkTEM+k9TNKqevGorc50wtcRzjX8K2cFyoraCO4SGSGMpHUUh0QriylItPHtiuvprNdRGUM+BUnMOs1rybzkwGTyCAxrwCmOjMhrv/UOx59oy6eCSZvqFddP31nnalyMUF5Zo2Qk+LpYfC3f+SIrQHHG8JrAtcULDq+lKLk2KtcCpnc6IxxGQ6F2v+0qekMIVX72OlQb36y3RucK4yxLqp0MEwxHaoDQLl3gHF4ixzWKBYWCC0gAMjJfZtSWTwG6TfX1yX566UW/1+qTPRcHtMUUHCLAPzER4SO7m5cy2SKc0b4mcP3ikDILpxNiPdTrvAUQlWQzHg5Ttca5it7wrFVkU+LdxjfrrdFFXu/QGginkawiPgcE8sIc5WQ4KbcGDkbuG2JiW/KMFAeknAombaq/9ddPQ2/uHkfZ2KTohdCsXna29fUnIVxBMOOAIkTKpOoo2+pRsYg8yFvk6qTJIbAjDfQ5S1fv/7l1MCVAYEHsmQyuH99ucD0d0m6ALE1ZZ19wGJEdWJ0lK7Zw2J9Zo1wgg0dwCduaaGhjt3wqmbSpvpHTT98LTRqben8iQFWcC2TjOSMYhJrdwTdax3R3q6ytjpyHJMAjkoISlVQIeVkZr60vAWpN6zRyais22Mni2vj2ZKCn89YLOHIN5AtwEpiRmpMXSshCSHZ0XKNkkjSD59oWJxLhXz6rmfRTfUugn77TUzYuVkUDkFHffpVBy3q3OVjBQ3EWNidjTeLa4kgaBLJt0txk5rKC5IVDeVoiyEPN63RyjPeBzdOgfny7wfV0iAEyFWOiSska76iowhEPFEWllb2z/fTtZcB68+Hv3QV8Rce/+8NL0X/rlsa3J7zRXzs9SxhdcRfjjkYjWrajwVk2+muOaAmjK5FnekaDP162oyHuNvprMW0JoysYZtzRGNoXPqOG4Br9NXS0hNEVNDw9owEkLtvRkAs0+ms4ewmjK4nVuKMx3Vi4o5ZWNvprKdsSRldy9GlHQ+a6bEdDhaLRX8v+Fz2jy3LPuKOxCLLQ+luxa3hGVwpJSxhdKRxO1j/U05btaCibNvprJclFz+iiAj1uaCzMLtvQUH4fHtFlaXsJn8tGxrifsb6/bD9DF2dw2JcdkkW+6KIhNu5nbBMtVFjrBg6u6LLTtuj5XPRVJ30N7cZl+xmaysPzuWzYLooeF/35cT9j13qhG2qXE4bgcdn4X8Ln8p7H5ISG2w8LTb5dcmn0Vy6QLDvPL68LTQY/3KJZtp/hrtR4ni/uIS3hc3ntbNzPeBlr4Xlud+4a/ZX7bEv4XN5enM7zcKlv2X6Gq5uN/sq1yEX6urj2Ou5nvBv6W3wqMh20dHmL9u9TN4x7Oj4//252Azj2oHd3PDz3t4Hjfhd/prDP3UXg7kZwxyin6de6Lu9CvxBk+u2Buo3Dc/29r8O5/wmwYWMfTzkfKmrf7Z+Op/MjJurmu/9/fb/fcHrlUvmLk/LuAr39h45eeb/ihWt4d4He/oMRr7yu8sL3vr+G3vyrCq+8u/MiuL27QG//wYVXXoJ6gR7eXaC3/0jBK2+TvYBn7y/Qm9/kf+V1wBf4990Fevs71K+8WPki43l/L/fmF42vvUl7mVS+u0Bvfxn3OsJ8+brXEoFOtVY3ivN1Jk5+v/v81zPkl2+ovrM4b7+aBHH+7/8BTLdjOg==

[adamzip]

Seems that what happened is that when the backflow PR stayed open and new builds were backflowing in, either one of two situations occurred:

1. If the most recent backflow commit in the PR came after the most recently merged forward flow into the VMR, then all the commits that were merged to the repo between those two events will be reverted by the upcoming backflow commit into the PR
2. If the situation is reversed and the backflow commit comes after the most recent merged forward flow, then the upcoming backflow commit in the PR won't revert anything, and will instead try to "re-add" things that have already been merged to the repo.

The runtime PR here has 3 backflows. The second commit falls in the second category (potentially not a dangerous situation), and the third one falls into the first category.

[SimonZhao888]

Backflow PR: dotnet/sdk#50274

[SimonZhao888]

Backflow PR: dotnet/sdk#50358

[SimonZhao888]

Backflow PR: dotnet/sdk#50403

[SimonZhao888]

Backflow PR: dotnet/sdk#50427

[adamzip]

@premun @dkurepa I believe the best approach to dealing with this issue is the following:

• When flowing additional flows into an existing PR, the last flow should come from the PR head branch and not the target branch. (this means that any flow into an existing PR will use the "same direction" strategy)
• Before we flow additional commits into an existing PR, we should detect the last flow on the target branch that came after the commit where the PR branched-off. Then we merge all the target's branch commits that were included in that flow into the PR head branch. (*)
• We apply the flow as a same-direction flow just like we normally would. If a conflict occur, we don't attempt to rectify it by re-applying previous flows, but we just halt additional flows on this PR and write a PR comment to say so.

I'll start implementing those changes tomorrow.

(*) this is actually a necessary step, because if we don't do this, we are in violation of "re-applying" changes that already exist on the PR target branch, which can lead to the same type of reverts that we see in #5046. By merging the target branch up until the last opposite-flown point, we are including changes that were already flown to the source repo in the merge-base, and we avoid 3-way diff bugs.

[adamzip]

@premun @dkurepa I believe the best approach to dealing with this issue is the following:

This approach has failing cases as well.

1. Backflow PR is open with some random changes, and is not merged
2. On repo main branch, add "newLine" to A.txt
   A.txt:

   existingLine
newLine

3. Repo forward flows A.txt and it is merged on VMR
4. Devs working on a feature in the VMR realise that newLine is no longer needed, so they remove it in the VMR
   A.txt:

   existingLine
<empty>

5. A new flow goes into the open backflow PR, containing some other irrelevant changes

Result: this last backflow won't bring the deletion of line2 in A.txt because the diff between the previous backflow and the current backflow on the VMR shows no changes for A.txt. Now the repo and the VMR are diverged until someone explicitly deletes line2 in the repo as well.

I've looked into the possibility of using opposite-direction flows in this scenario, but it seems that there too, no matter where we branch out of, we risk to run into lost changes compared to the last forward flow or the last backflow. I'll post more details about this case.

If neither of the strategies work here, we might just have to pause future flows into an existing codeflow PR as soon as soon as a codeflow of the other direction has been merged.

[jkotas]

this last backflow won't bring the deletion of line2 in A.txt because the diff between the previous backflow and the current backflow on the VMR shows no changes for A.txt. Now the repo and the VMR are diverged until someone explicitly deletes line2 in the repo as well.

Right, that's not good. It is important that the system is self-healing and automatically recovers from any accidental diffs introduced by manually resolved merge conficts, operator mistakes, bugs, etc. Discussed at dotnet/arcade#14195 (comment)

I think the problem with the current algorithm are the optimizations for happy cases discussed in dotnet/arcade#14195 (comment). If we hit the happy cases, it works fine. If we end up hitting the fallbacks, it starts breaking down.