Phabricator sending spurious "This revision was not accepted when it landed" emails

Has anyone else noticed Phabricator sending emails saying:
  This revision was not accepted when it landed; it landed in state
"Needs Review".
when the review clearly has been accepted by someone?

Some recent examples:
https://reviews.llvm.org/D83952
https://reviews.llvm.org/D80116
https://reviews.llvm.org/D81267

Thanks,
Jay.

Yes, I’ve seen them occasionally, although not always as far as I’ve noticed.

James

I believe that can happen when someone says LGTM in a reply, but doesn’t do the drop-down to Accept Revision.

The flip side is if you do the drop-down and don’t put in a reply, there’s no notice to the mailing list.

–paulr

I’ve seen it a few times recently where I only used the accept drop down box and didn’t make any comment

-Matt

+Mehdi AMINI who’s taking some (shared?) ownership of Phabricator these days.

Mehdi - was Phab updated recently (such that we might’ve picked up new semantics)?

Has anyone else noticed Phabricator sending emails saying:
This revision was not accepted when it landed; it landed in state
“Needs Review”.
when the review clearly has been accepted by someone?

Some recent examples:
https://reviews.llvm.org/D83952
https://reviews.llvm.org/D80116

Hard for me to tell what happened here. I wonder if it’s related to making changes after review/before committing. While that’s common in LLVM, I could imagine a review tool (especially if we picked up a newer version - as I don’t think it’s always had this behavior) might get fussy about that - perhaps it’d be configurable, so it’d say “this was committed with extra changes” but not “This was committed without review”.

Do you have any examples that didn’t have post-approval-pre-commit changes that still got this annotation about being committed without review?

https://reviews.llvm.org/D81267

Last one seems more clear - one of the reviewers (rupprecht) still had the review marked “requires changes”, so it was committed without closure on that.

+Mehdi AMINI who’s taking some (shared?) ownership of Phabricator these days.

Mehdi - was Phab updated recently (such that we might’ve picked up new semantics)?

No: I upgraded the hardware and the OS, but not Phab itself yet.

I have a test instance running with an upgraded Phab though, it may have been sending duplicate emails in the last day or two when I didn’t notice I had the email daemon running.

Has anyone else noticed Phabricator sending emails saying:
This revision was not accepted when it landed; it landed in state
“Needs Review”.
when the review clearly has been accepted by someone?

Some recent examples:
https://reviews.llvm.org/D83952

Seems like this one closed as expected without the message? http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808734.html

https://reviews.llvm.org/D80116

Same here: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808778.html

Can you forward me the email you received for these revisions?

Hard for me to tell what happened here. I wonder if it’s related to making changes after review/before committing. While that’s common in LLVM, I could imagine a review tool (especially if we picked up a newer version - as I don’t think it’s always had this behavior) might get fussy about that - perhaps it’d be configurable, so it’d say “this was committed with extra changes” but not “This was committed without review”.

Do you have any examples that didn’t have post-approval-pre-commit changes that still got this annotation about being committed without review?

https://reviews.llvm.org/D81267

Last one seems more clear - one of the reviewers (rupprecht) still had the review marked “requires changes”, so it was committed without closure on that

Indeed this one shows the message: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200713/807554.html

+Mehdi AMINI who’s taking some (shared?) ownership of Phabricator these days.

Mehdi - was Phab updated recently (such that we might’ve picked up new semantics)?

No: I upgraded the hardware and the OS, but not Phab itself yet.

I have a test instance running with an upgraded Phab though, it may have been sending duplicate emails in the last day or two when I didn’t notice I had the email daemon running.

Has anyone else noticed Phabricator sending emails saying:
This revision was not accepted when it landed; it landed in state
“Needs Review”.
when the review clearly has been accepted by someone?

Some recent examples:
https://reviews.llvm.org/D83952

Seems like this one closed as expected without the message? http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808734.html

In my inbox I have two emails for that review.
(though, also, on the commits list, I do see this: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808735.html - though my mail client (gmail) is rendering this one as in the same thread/doesn’t seem to show the “[Differential]” prefix in the subject - not sure what’s going on there, usually gmail is /too/ ready to group mails by actual subject text, rather than by thread ids in the email hedaers… )

https://reviews.llvm.org/D80116

Same here: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808778.html

Can you forward me the email you received for these revisions?

Similarly, the separate “[Differential]” email seems to be unthreaded and contains the problematic “not accepted when it landed” text: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808779.html

+Mehdi AMINI who’s taking some (shared?) ownership of Phabricator these days.

Mehdi - was Phab updated recently (such that we might’ve picked up new semantics)?

No: I upgraded the hardware and the OS, but not Phab itself yet.

I have a test instance running with an upgraded Phab though, it may have been sending duplicate emails in the last day or two when I didn’t notice I had the email daemon running.

Has anyone else noticed Phabricator sending emails saying:
This revision was not accepted when it landed; it landed in state
“Needs Review”.
when the review clearly has been accepted by someone?

Some recent examples:
https://reviews.llvm.org/D83952

Seems like this one closed as expected without the message? http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808734.html

https://reviews.llvm.org/D80116

Same here: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808778.html

Can you forward me the email you received for these revisions?

Hard for me to tell what happened here. I wonder if it’s related to making changes after review/before committing. While that’s common in LLVM, I could imagine a review tool (especially if we picked up a newer version - as I don’t think it’s always had this behavior) might get fussy about that - perhaps it’d be configurable, so it’d say “this was committed with extra changes” but not “This was committed without review”.

Do you have any examples that didn’t have post-approval-pre-commit changes that still got this annotation about being committed without review?

https://reviews.llvm.org/D81267

Last one seems more clear - one of the reviewers (rupprecht) still had the review marked “requires changes”, so it was committed without closure on that

Indeed this one shows the message: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200713/807554.html

dmgreen accepted it after I requested changes. Shouldn’t that override my earlier “requires changes” request? It seems like a bad SPOF of failure to require my LGTM.

FWIW, the reland of the patch is good with me because it includes a variant of the crash repro I provided. I just didn’t LGTM it – I’m not familiar with the patch at all beyond that it caused a crash – which is why I assumed someone else would be able to approve and take it out of the “requires changes” state (e.g. make sure it fixes the crash in the right way).

+Mehdi AMINI who's taking some (shared?) ownership of Phabricator these days.

Mehdi - was Phab updated recently (such that we might've picked up new semantics)?

No: I upgraded the hardware and the OS, but not Phab itself yet.

I have a test instance running with an upgraded Phab though, it may have been sending duplicate emails in the last day or two when I didn't notice I had the email daemon running.

Has anyone else noticed Phabricator sending emails saying:
  This revision was not accepted when it landed; it landed in state
"Needs Review".
when the review clearly has been accepted by someone?

Some recent examples:
https://reviews.llvm.org/D83952

Seems like this one closed as expected without the message? http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808734.html

https://reviews.llvm.org/D80116

Same here: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808778.html

Can you forward me the email you received for these revisions?

Hard for me to tell what happened here. I wonder if it's related to making changes after review/before committing. While that's common in LLVM, I could imagine a review tool (especially if we picked up a newer version - as I don't think it's always had this behavior) might get fussy about that - perhaps it'd be configurable, so it'd say "this was committed with extra changes" but not "This was committed without review".

Do you have any examples that didn't have post-approval-pre-commit changes that still got this annotation about being committed without review?

https://reviews.llvm.org/D81267

Last one seems more clear - one of the reviewers (rupprecht) still had the review marked "requires changes", so it was committed without closure on that

Indeed this one shows the message: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200713/807554.html

dmgreen accepted it after I requested changes. Shouldn't that override my earlier "requires changes" request? It seems like a bad SPOF of failure to require *my* LGTM.

FWIW, the reland of the patch is good with me because it includes a variant of the crash repro I provided. I just didn't LGTM it -- I'm not familiar with the patch at all beyond that it caused a crash -- which is why I assumed someone else would be able to approve and take it out of the "requires changes" state (e.g. make sure it fixes the crash in the right way).

I could go either way there - sometimes if multiple people have
provided feedback/requested changes, we do try to wait to check that
those who requested them sign off that they've been addressed,
otherwise some reviewers can (unintentionally or otherwise) undercut
others.

- Dave

Thanks! I dug a bit and found out that these came from my test instance, I ran an upgraded Phabricator instance and didn’t disable polling the repository: so it closed the revision without having any approval.

Those emails say
"This revision was not accepted when it landed; it landed in state
"Needs Review".
This revision was landed with ongoing or failed builds."
I think the problem here is the part about failed builds. Harbormaster
seemingly is always complaining about failed builds. Might this be the
cause of these emails?

Alex

Based purely on UI clues etc, I think the intent in Phabricator is that a “Request for Changes” can only be overridden by the person who requested them marking the revision as accepted (note how an updated review following such a request has the red cross still coloured, unlike the ticks for an acceptance, implying somehow that it’s still important).

Thanks! I dug a bit and found out that these came from my test instance, I ran an upgraded Phabricator instance and didn't disable polling the repository: so it closed the revision without having any approval.

Makes sense. Thanks for looking into it!

Jay.