1402374 - Revisions are not automatically closed when the change is merged

Status: RESOLVED FIXED

(Conduit :: Phabricator, defect)

Description

[Israel Madueme [:imadueme]]

When a revision is accepted and landed manually or via the `arc land` command Phabricator's expected behavior is to detect the new commit in the repository and automatically close the Revision linked in the commit message.

Currently on https://phabricator.services.mozilla.com/ Revisions are not automatically being closed when merged.

Comment 1

[Israel Madueme [:imadueme]]

I suspect that production machine has difficulty synchronizing all the repositories it tracks - since mozilla-central is likely taking the most resources and time causing the sync of other repos to suffer.

I will investigate to see if the only problem is slow syncing or if there is another underlying problem with auto-closing revisions.

Comment 2

[Phabricator Automation]

Comment on attachment 8911659 [details]
Bug 1402374 - Clobber README to force Phab to import lando-api.

Piotr Zalewa [:zalun] has approved the revision.

https://phabricator.services.mozilla.com/D81#1590

Comment 3

[Phabricator Automation]

Comment on attachment 8911659 [details]
Bug 1402374 - Clobber README to force Phab to import lando-api.

David Lawrence [:dkl] has approved the revision.

https://phabricator.services.mozilla.com/D81#1592

Comment 4

[Israel Madueme [:imadueme]]

Attachment: p1-revision-importing-not-closed.png

Comment 5

[Israel Madueme [:imadueme]]

Attachment: p2-2-commits-still-importing.PNG

Comment 6

[Israel Madueme [:imadueme]]

Attachment: p3-commit-obviously-detected.PNG

Comment 7

[Israel Madueme [:imadueme]]

Attachment: p4-commit-still-importing.PNG

Comment 8

[Israel Madueme [:imadueme]]

Attachment: p5-test-import-in-test-env.png

Comment 9

[Israel Madueme [:imadueme]]

Attachment: p6-imported-fine-within-2-minutes-of-closing-manually.PNG

Comment 10

[Israel Madueme [:imadueme]]

Attachment: p7-commit-with-no-revision-imports-instantly.PNG

Comment 11

[Israel Madueme [:imadueme]]

Attachment: p8-phacility-import-status-before-commit.png

Comment 12

[Israel Madueme [:imadueme]]

Attachment: p9-phacility-import-status-successful-after-commit.png

Comment 13

[Israel Madueme [:imadueme]]

Attachment: p10-arc-auto-close-with-accepted-revision.PNG

Comment 14

[Israel Madueme [:imadueme]]

Attachment: p11-arc-no-auto-close-with-no-acceptance.PNG

Comment 15

[Israel Madueme [:imadueme]]

This is a report of my investigation as to why Revisions do not auto close on https://phabricator.services.mozilla.com.

# Preface:
- In Phabricator Diffusion there is a setting to choose which branches on which repos will automatically close revisions when a commit created from the revision patch is merged.
- Currently this feature is not working properly on our deployed instance for any repos (NSS, LandoAPI, LandoUI, and probably all the others).
- See Phabricator Docs on Autoclosing [1]


# Test 1 - Landing an Accepted Revision on https://phabricator.services.mozilla.com
- Confirmed that Autoclosing is enabled on the master branch of Lando API in Phabricator [2].
- Landed D57 [3] with `arc land --revision D57`.
- arc land simply takes the patch, applies it locally, and pushes to origin master. So it is no different than manually patching and pushing the commit.
- Expected: Phabricator detects the new commit in the Lando API Repo [1][2] and autocloses the D57 revision.
- Actual: 
  - arc land successfully completes and the new commit is in the repo.
  - Phabricator Diffusion's sync triggers detects the new commit, but, marks it as "Importing", see [pic 01].
  - With the assumption that it must first finish importing before autoclosing, I wait for the import to finish.
  - The import never finishes, observed at least 8 hours after the landing. 

  
# Test 2 - Repeat test 1
- Created another new Revision D81 [4] which has almost nothing in the diff.
- `arc land --revision D81`
- Outcome:
  - Same result as test 1, except now the commits for D57 and D81 are both still importing. See [pic 02].
  - The commit is obviously detected, see [pic 03][pic 04], but, for some reason is still "Importing"...
 
 
# Test 3 - Repeat Test 1 in local environment http://phabricator.test
- I suspected at first that it could be due to our configuration on the production machine or perhaps that because of the load of syncing mozilla-central and nss on production Phabricator was not able to keep up with importing all the commits.
- I started to doubt this since the commits were there, just not fully "imported" so I tested in my local environment without any load.
- The test was done by using a fork of lando-api on Github so I could push real commits which Phabricator could watch.
- Outcome:
  - Same result in local test environment, see [pic 05].

  
# Test 4 - Close the Revision manually
- Looking through the history of a few repos on production, I noticed that most commits finished importing eventually. This was odd since we noted that Revisions can go days without being autoclosed.
- I manually closed D57 and D81 on production and waited for 2 minutes to let Phabricator schedule a repo sync as it usually does.
- Outcome: Within 2 minutes both Revisions finished "Importing". See [pic 06].
- I begin to suspect that Phabricator gets stuck on actually autoclosing the revision as part of the import process.


# Test 5 - Push a commit without a revision
- To test if Phabricator actually gets stuck doing something with a revision, I decided to push a commit that had no revision connected to it.
- Outcome: The revision is imported instantly (or as soon as Diffusion sync triggers), see [pic 07].
- This and Test 4 would explain why the repo history in Diffision is all stuck "Importing"


# Test 6 - Repeat Test 1 on a Phacility hosted Phabricator instance
- To test whether it is a problem with Phabricator itself, our repos, our configuration, or our specific instance with extensions I decided to try the same thing on a Phacility hosted Phabricator instance [5]. 
- Outcome: I repeated it a couple times and in both cases the commit is imported instantly and the Revision is auto-closed. See [pic 08][pic 09].
- My configuration and repos were the same so it is probably a bug with our instance.


# Note 1 - arc land auto-close
- When using arc land one of the last steps it will do is close the revision (i.e. instead of waiting for the commit to be detected, arc just closes it right there) see [pic 10].
- This doesn't happen if you force landing a revision that wasn't accepted (see [pic 11]) and this shouldn't prevent us from fixing the reason why Phabricator doesn't autoclose a Revision when the commit is detected.
- I bring this up to confirm that the hosted Phacility instance successfully closed the Revision by detecting the commit, and not solely because of the arc command.


# Recommended Next Steps
Considering that the Phacility instance does work properly, and considering that our Phabricator instance is significantly behind upstream. I propose we upgrade our Phabricator instance and the retest this problem. Hopefully it is an upstream issue that has already been fixed.

Afterwards we may contact Phacility support to get their thoughts on what could be causing this.

=================================================
[1]: https://secure.phabricator.com/book/phabricator/article/diffusion_autoclose/
[2]: https://phabricator.services.mozilla.com/source/lando-api/manage/branches/
[3]: https://phabricator.services.mozilla.com/D57
[4]: https://phabricator.services.mozilla.com/D81
[5]: https://test-qwukxo4kcv26.phacility.com/

[pic 01]: https://bug1402374.bmoattachments.org/attachment.cgi?id=8911878
[pic 02]: https://bug1402374.bmoattachments.org/attachment.cgi?id=8911883
[pic 03]: https://bug1402374.bmoattachments.org/attachment.cgi?id=8911884
[pic 04]: https://bug1402374.bmoattachments.org/attachment.cgi?id=8911885
[pic 05]: https://bug1402374.bmoattachments.org/attachment.cgi?id=8911886
[pic 06]: https://bug1402374.bmoattachments.org/attachment.cgi?id=8911887
[pic 07]: https://bug1402374.bmoattachments.org/attachment.cgi?id=8911888
[pic 08]: https://bug1402374.bmoattachments.org/attachment.cgi?id=8911889
[pic 09]: https://bug1402374.bmoattachments.org/attachment.cgi?id=8911890
[pic 10]: https://bug1402374.bmoattachments.org/attachment.cgi?id=8911891
[pic 11]: https://bug1402374.bmoattachments.org/attachment.cgi?id=8911892

Comment 16

[Israel Madueme [:imadueme]]

Correction in Test 5 
"is stuck" -> "isn't stuck"

Comment 17

[Mark Côté [:mcote]]

Thanks, this is a great investigation so far!

However I actually have my doubts that it's just because we're behind.  The next step should be upgrading locally.  This will also help us confirm that our extensions will still work after this upgrade.

Comment 18

[Israel Madueme [:imadueme]]

Investigation Complete.

In Bug 1405613 I upgraded my local Phabricator version to the most recent commit on Phabricator's stable branch. Auto-closing of revisions was still busted based on the steps to reproduce I laid out, as so I had to dig deeper. There are actually 2 ways that auto-closing can be broken, fixes for both have been identified.


## 1. `arc land` command doesn't close the revision as it's last step.

The arc land command closes the revision as the last step of it's process. For some reason, my old (outdated) version of arc wasn't doing so. When I upgraded (which involves upgrading Phabricator, arc, and libphutil) arc was correctly closing the revision at the end of the arc land command. 

This does not solve the problem for people who manually push their commits to the repo and expect Phabricator to detect the changes. That is solved by the second way Phabricator autocloses revisions.


## 2. Diffusion gets stuck on 'Importing' a commit and so it never gets to the autoclosing step.

When a commit is pushed to a repository, Phabricator runs some background tasks on the commit, e.g. importing the commit metadata and autoclosing the associated revision if there is one.

To cut to the point, one of these background tasks was failing and so Phabricator was not able to reach the auto-close step. https://secure.phabricator.com/book/phabricator/article/repository_imports/ was extremely helpful in debugging this problem.

Looking at http://phabricator.test/daemon and the `./bin/phd log` in the Phabricator container, I was able to trace down the problem to the `PhabricatorRepositoryGitCommitMessageParserWorker` task failing with this error:

```
EXCEPTION: (PhutilProxyException) Error while executing Task ID 41037. {>} (AphrontQueryException) #1048: Column 'viewPolicy' cannot be null at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:355]
...
```

I traced this back to our patch here: https://github.com/mozilla-services/phabricator-extensions/blob/master/patches/phabricator/1382312_1.patch which modifies the viewPolicy of the DifferentialDiff class. Presumably, when Phabricator is running background tasks, there is no actor present and so it is unable to set the viewPolicy correctly.

I again rebuilt my local instance without that patch (which was only for setting the default on the web UI anyway, no not very important) and once I did that autoclosing worked just fine when pushing directly to the repo. Diffusion was able to import the commit very quickly and once it did it closed the associated Revision.


## Recommended Steps
- I recommend we upgrade our Phabricator instance to the latest version and test that our extensions continue to work, as I've started in Bug 1405613.

- I recommend we update this https://github.com/mozilla-services/phabricator-extensions/blob/master/patches/phabricator/1382312_1.patch patch so that if it cannot detect an actor it defaults to the current viewPolicy of the Diff and if it doesn't detect that it defaults to the "normal" viewPolicy that Phabricator would set without our patch.

Comment 19

[David Walsh :davidwalsh]

Nice!  I was shown that "Column 'viewPolicy' cannot be null" by ckolos a few weeks ago, and was sure we needed to add a default there.  Then I lose track of the task due to head injury.  I'll add a card for this task and we can get it done this week.

Comment 20

[Israel Madueme [:imadueme]]

This has been fixed in https://phabricator.services.mozilla.com/D97 and will be live in the next Phabricator deploy this week.