Forward flow PR dropped/reverted manual in PR change

[dkurepa]

We had a forward flow PR with some manual changes that were dropped/reverted after a new flow dotnet/dotnet#1382.

Need to look into this and fix it

[ViktorHofer]

I also had to do this dotnet/dotnet@7607972 although that change is already in main in both the VMR and dotnet/sdk...

https://github.com/dotnet/dotnet/blob/86f382c0fc7bab593e32ae6e5623c3472a8845d0/src/sdk/src/Layout/redist/targets/RestoreLayout.targets#L129-L133

For some reason the sync undid that...

[dkurepa]

I think I’ve figured out what’s causing this:

1. We open a backflow PR.
2. We open a FF PR.
3. We make some changes to the FF PR.
4. We merge the backflow PR — the order of steps 3 and 4 doesn’t matter.
5. Then, another build updates the FF PR, overwriting the changes we made earlier.

This happens because, during the second forward flow, the latest flow is correctly identified as a backflow. In this scenario, the process essentially does the following:

• Checks out the PR head branch, which includes our changes.
• Wipes the repo from the VMR.
• Creates and applies a patch from the zero commit to the latest one — effectively overwriting any changes we had made.

[dkurepa]

interestingly, this doesn't happen if:

• we create a FF PR, do the changes
• make some manual changes in the VMR and open a backflow PR
• Merge the backflow PR
• Flow the latest build to the already open PR

This is because in this scenario, the last flow is detected as ForwardFlow even tho it technically isn't. We have some logic that says When the last backflow to our repo came from a different branch, we ignore it and return the last forward flow that's there for previews, and it actually triggers for this scenario too, guiding us on a different code path that doesn't wipe the VMR content, just applies the patch from last FF to current FF, so that might be a potential second bug

[dkurepa]

I wonder if we should just try to detect commits not made by the service and cherry pick them on top in the correct order?
Any thoughts on this @mmitche ?

[mmitche]

I wonder if we should just try to detect commits not made by the service and cherry pick them on top in the correct order? Any thoughts on this @mmitche ?

My gut says this will result in additional corner cases, like when those changes were to lines changed in the forward flow PR. Essentially you're re-applying changes out of order.

Checks out the PR head branch, which includes our changes.
Wipes the repo from the VMR.
Creates and applies a patch from the zero commit to the latest one — effectively overwriting any changes we had made.

Step 2 there...what actually happens? It deletes the source and starts over?

[dkurepa]

As agreed, here's the PR that prevents the service from making overwritting updates to the PR if it sees someone else worked on it #5043

[dkurepa]

My gut says this will result in additional corner cases, like when those changes were to lines changed in the forward flow PR. Essentially you're re-applying changes out of order.

In that case we'd get a conflict and stop the flow like we do usually I think. This is what happens in the other ForwardFlow scenario, when we have 2 in a row, if a change was made that conflicts with the Second ForwardFlow update, we abort and write a comment about it, so it wouldn't be different

Step 2 there...what actually happens? It deletes the source and starts over?

Depends on the previous flow at that point, it's not included in the steps I wrote but:

• If previous flow was a backflow, yes, when opening the PR it will delete the repo from the VRM and flow the entire repo to the target commit basically
• If the previous flow was a FF, then it will just apply the patch from the last commit to the target one

But I don't think that matters for this scenario, the problem starts later

[adamzip]

• If previous flow was a backflow, yes, when opening the PR it will delete the repo from the VRM and flow the entire repo to the target commit basically

Why is it necessary to delete the directory and take the repo's contents as they are? afaik no other flow works like that.

It seems that if one of the primary goals of codeflows is to avoid conflicts as much as possible, then there's just no clean solution for applying subsequent updates on top of any codeflow PR that already has manual commits. What if disallowing subsequent codeflow updates on existing PRs was just part of how codeflow is designed?