1400318 - Servo backout PR service attempts bad merge

Status: RESOLVED FIXED

(Developer Services :: Servo VCS Sync, enhancement)

Description

[Gregory Szorc [:gps]]

A recent backout on Autoland broke the servo backout pr service.

Sep 15 08:02:05 servo-vcs-sync.mozops.net systemd[1]: Starting Generate Servo PRs from Firefox backouts...
Sep 15 08:02:08 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: updating /home/servo-sync/backout-autoland to f7b896fc8778
Sep 15 08:02:11 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: processing backout f7b896fc8778: Backed out changeset e2f8c9f76b71 for Hazard failure. r=backout
Sep 15 08:02:12 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: f7b896fc8778 backing out e2f8c9f76b71: https://github.com/servo/servo/pull/18515
Sep 15 08:02:12 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: reverting all local changes and purging /home/servo-sync/backout-autoland
Sep 15 08:02:12 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: executing: hg --quiet revert --no-backup --all
Sep 15 08:02:14 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: executing: hg purge --all
Sep 15 08:02:19 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: $ git fetch https://github.com/servo/servo.git +master:refs/upstream/master
Sep 15 08:02:23 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: From https://github.com/servo/servo
Sep 15 08:02:23 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: 812cac78f0..1677a2b34c  master     -> refs/upstream/master
Sep 15 08:02:23 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: $ git checkout -B gecko-backout
Sep 15 08:02:27 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: Reset branch 'gecko-backout'
Sep 15 08:02:27 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: $ git clean -d --force
Sep 15 08:02:27 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: $ git merge refs/upstream/master
Sep 15 08:02:29 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: Removing tests/wpt/metadata/old-tests/submission/Opera/script_scheduling/029.html.ini
Sep 15 08:02:29 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: Removing tests/wpt/metadata/old-tests/submission/Opera/script_scheduling/028.html.ini
Sep 15 08:02:29 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: Removing tests/wpt/metadata/html/browsers/browsing-the-web/navigating-across-documents/javascript-url-query-fragment-compo
Sep 15 08:02:29 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: Removing tests/wpt/metadata/html/browsers/browsing-the-web/navigating-across-documents/015.html.ini
Sep 15 08:02:29 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: Removing tests/wpt/metadata/html/browsers/browsing-the-web/navigating-across-documents/014.html.ini
Sep 15 08:02:29 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: Removing tests/wpt/metadata/html/browsers/browsing-the-web/navigating-across-documents/013.html.ini
Sep 15 08:02:29 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: Removing tests/wpt/metadata/dom/nodes/Document-contentType/contentType/contenttype_javascripturi.html.ini
Sep 15 08:02:29 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: Removing tests/wpt/metadata/XMLHttpRequest/open-url-javascript-window.htm.ini
Sep 15 08:02:29 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: Removing tests/wpt/metadata-css/css-transforms-1_dev/html/transform-rounding-001.htm.ini
Sep 15 08:02:29 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: Auto-merging components/style/values/specified/length.rs
Sep 15 08:02:29 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: CONFLICT (content): Merge conflict in components/style/values/specified/length.rs
Sep 15 08:02:29 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: Auto-merging components/style/values/animated/mod.rs
Sep 15 08:02:29 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: Removing components/style/stylesheets/memory.rs
Sep 15 08:02:29 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: Auto-merging components/style/properties/gecko.mako.rs
Sep 15 08:02:29 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: Automatic merge failed; fix conflicts and then commit the result.
Sep 15 08:02:29 servo-vcs-sync.mozops.net servo-backout-pr-cli[18655]: failed to create pull-request, retrying: Command '[u'git', u'merge', u'refs/upstream/master']' returned non-zero exit stat

The reason appears to be that the current state of the checkout is non-deterministic. We probably want to use a `git reset` somewhere to ensure things are consistent.

Comment 1

[Gregory Szorc [:gps]]

Attachment: vcssync: make branch checkout more robust (bug 1400318);

Before, we relied on `git merge` to advance our branch/working tree.

While this is the typical end-user workflow for advancing a branch
through remote changes, it isn't the most robust mechanism for doing
so.

This commit rewrites the branch checkout logic to first sanitize
the working tree via `git reset --hard` + `git clean`. Then we create
our new branch pointed explicitly at a target ref and check it out.
No `git merge` necessary.

Review commit: https://reviewboard.mozilla.org/r/180346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180346/

Comment 2

[:glob ✱]

Comment on attachment 8908714 [details]
vcssync: make branch checkout more robust (bug 1400318);

https://reviewboard.mozilla.org/r/180346/#review185866

::: vcssync/mozvcssync/github_pr.py:173
(Diff revision 1)
> -        git.cmd('checkout', '-B', branch_name)
> +        git.cmd('reset', '--hard')
>          git.cmd('clean', '-d', '--force')
> -        git.cmd('merge', 'refs/upstream/master')
>  
> +        # Create a branch pointed at upstream and check it out.
> +        git.cmd('checkout', '-B', branch_name, 'refs/upstream/master')

i suspect this will not work correctly if the branch already exists, and you'll end up with divergent heads.

this can happen if a second backout is requested while one is already in flight - in this case the pr should be updated to contain both revisions.

Comment 3

[:glob ✱]

Comment on attachment 8908714 [details]
vcssync: make branch checkout more robust (bug 1400318);

https://reviewboard.mozilla.org/r/180346/#review186042

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8908714 [details]
vcssync: make branch checkout more robust (bug 1400318);

https://reviewboard.mozilla.org/r/180346/#review185866

> i suspect this will not work correctly if the branch already exists, and you'll end up with divergent heads.
> 
> this can happen if a second backout is requested while one is already in flight - in this case the pr should be updated to contain both revisions.

`git checkout -B <branch> <start-point>` is the equivalent of `git branch -f <branch> <start-point>` + `git checkout <branch>`. So it will run if the branch exists.

But, I'm failing to grok the "second backout requested while one is already in flight." I /think/ you are saying that we need the branch to linger around locally so we can keep applying commits there?

How do we know to delete this branch? How do we know to avoid a merge commit (and just do a regular fast forward) of the base revision?

Comment 5

[:glob ✱]

Comment on attachment 8908714 [details]
vcssync: make branch checkout more robust (bug 1400318);

https://reviewboard.mozilla.org/r/180346/#review185866

> `git checkout -B <branch> <start-point>` is the equivalent of `git branch -f <branch> <start-point>` + `git checkout <branch>`. So it will run if the branch exists.
> 
> But, I'm failing to grok the "second backout requested while one is already in flight." I /think/ you are saying that we need the branch to linger around locally so we can keep applying commits there?
> 
> How do we know to delete this branch? How do we know to avoid a merge commit (and just do a regular fast forward) of the base revision?

servo-backout-pr always uses the same branch for backouts to ensure all in-flight backouts are coalesced ("gecko-backout").

coland will be using a branch-per-bug model; my plan there is to delete the branch after the colanded changes are pushed to autoland-integration.

i'm not sure how we could guard against merge commits in this scenario.

Comment 6

[:glob ✱]

spoke with gps about this last night; taking bug to address this from a different angle.

Comment 7

[:glob ✱]

Attachment: servo-backout-pr: make branch checkout more robust (bug 1400318)

Improves the logic around handling of the gecko-backout branch to avoid merge
conflicts.

Test explicitly for an in-flight backout PR and append the backout to that if
it exists.  If an in-flight PR doesn't exist delete the branch and create it
from scratch.

Review commit: https://reviewboard.mozilla.org/r/181634/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181634/

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8910143 [details]
servo-backout-pr: make branch checkout more robust (bug 1400318)

https://reviewboard.mozilla.org/r/181634/#review188102

This seems like a reasonable solution. There are still some very short window edge cases. But if we hit those, I'd be shocked.

Please do address the potentially dirty working tree and index issue before landing. I'd normally not grant r+. But since I'll be traveling and am not sure when I'll get around to review...

::: vcssync/mozvcssync/github_pr.py:162
(Diff revision 1)
>          git.cmd('fetch', upstream_repo.clone_url,
>                  '+master:refs/upstream/master')
>  
> -        # If we're not reusing branches we expect a unique branch for each
> -        # pull request.  This is much easier to manage if we start with a
> -        # clean branch each time.
> +        # It's simpler to just delete and recreate the branch to reset it.
> +        if reset_branch and git.get('branch', '--list', branch_name):
> +            git.cmd('checkout', 'master')

This wants a --force so working tree or index changes don't get in the way. Alternatively, do a `git reset --hard HEAD` before the checkout to ensure the working tree and index are clean with respect to the current checkout.

Comment 9

[:glob ✱]

Comment on attachment 8910143 [details]
servo-backout-pr: make branch checkout more robust (bug 1400318)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181634/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/d2a599dcdb01
servo-backout-pr: make branch checkout more robust r=gps