Page MenuHomePhabricator

Duplicated comments on JIRA when differential revisions are linked to JIRA issues
Closed, InvalidPublic

Description

When updating differential revisions if several users are subscribed (as in, the creator and the reviewer), each update triggers a comment in JIRA for each of the subscribers, thus creating multiple comments, one for each subscriber.

Reproduction steps:
Just link JIRA and phabricator, link more than one account with JIRA and add the linked accounts as subscribers for a review. Make a comment in the review and see the multiple comments on JIRA.

Version info:
Phabricator ef839192aa5a (latest modification to the relevant file was in Nov 12 2015)
JIRA v6.3.13#6344-sha1:62d2b41 (although this is probably unimportant)

Related to T5422.

Revisions and Commits

Event Timeline

I suspect this will not reproduce for us given the reproduction instructions. The code is structured like this:

foreach (objects as object) {
  foreach (accounts as account) {
    try {
      make_comment();
      make_link();
      break; // <<< Important!
    } catch (Exception) {
      phlog(...);
    }
  }
}

Particularly, note the break;.

The intended meaning of this block is "try each account until one succeeds; when it does, stop trying".

I would guess that what you are actually seeing is that an exception in make_link() causing the make_comment() code to fire again, incorrectly.

You could verify this by turning off link posting. That is, if you follow these steps:

  • Revert to clean master.
  • Disable the "post a link" option in the JIRA adapter.
  • Make an update to an object with multiple subscriber accounts, per reproduction instructions here.

...I suspect you will see only one comment post.

In this vein, the immediate error arising while creating the link is probably somewhere in bin/phd log. Resolving this error would likely resolve the problem.

Our behavior here is also incorrect, but the assumption here and in D18001 (that we didn't consider this case and that the code makes no effort to account for it) isn't correct either, and thus the resolution in D18001 may not be the best solution available to us.

If I'm right about this, I'd like to understand what the actual error occurring on link posting is before choosing a solution. We probably can not identify this error on our own, given only the instructions and information you've provided.

If I'm not right about this, I'd like to understand why the code isn't already working as intended before choosing a solution. From a plain reading of the code, the break; means it should not exhibit the behavior you describe.

We can make an effort to confirm that we can't reproduce this, but setup is a pain and I'm reasonably sure the effort will be fruitless, so this is difficult to prioritize.

All that makes a lot of sense, I will try what you suggest and see if that was the source of the evil.

franjesus added a comment.EditedMay 24 2017, 12:13 PM

Indeed there are the predicted error messages in the phd log. I will try to explore more and see what is wrong. It is probably related to the issue I reported (T12726), likely some of the fields are incorrectly created by arcanist in the first place.

I will revert to master and see if the behaviour happens only with reviews created by the "broken windows user" that I talk about on the other issue.

[24-May-2017 11:08:41 Europe/Madrid] [2017-05-24 11:08:41] EXCEPTION: (HTTPFutureHTTPResponseStatus) [HTTP/403] 
{"errorMessages":["Issue linking is currently disabled."],"errors":{}} at [<phutil>/src/future/http/BaseHTTPFuture.php:339]
[24-May-2017 11:08:41 Europe/Madrid] arcanist(head=master, ref.master=3c4735795a29), phabricator(head=master, ref.master=ef839192aa5a), phutil(head=master, ref.master=a900d7b63e95)
[24-May-2017 11:08:41 Europe/Madrid]   #0 <#2> BaseHTTPFuture::parseRawHTTPResponse(string) called at [<phutil>/src/future/http/HTTPSFuture.php:418]
[24-May-2017 11:08:41 Europe/Madrid]   #1 <#2> HTTPSFuture::isReady() called at [<phutil>/src/future/Future.php:37]
[24-May-2017 11:08:41 Europe/Madrid]   #2 <#2> Future::resolve() called at [<phutil>/src/future/http/BaseHTTPFuture.php:279]
[24-May-2017 11:08:41 Europe/Madrid]   #3 <#2> BaseHTTPFuture::resolvex() called at [<phutil>/src/future/oauth/PhutilOAuth1Future.php:279]
[24-May-2017 11:08:41 Europe/Madrid]   #4 <#2> PhutilOAuth1Future::resolveJSON() called at [<phabricator>/src/applications/doorkeeper/worker/DoorkeeperJIRAFeedWorker.php:250]
[24-May-2017 11:08:41 Europe/Madrid]   #5 <#2> DoorkeeperJIRAFeedWorker::postLink(PhabricatorExternalAccount, string) called at [<phabricator>/src/applications/doorkeeper/worker/DoorkeeperJIRAFeedWorker.php:104]
[24-May-2017 11:08:41 Europe/Madrid]   #6 phlog(HTTPFutureHTTPResponseStatus) called at [<phabricator>/src/applications/doorkeeper/worker/DoorkeeperJIRAFeedWorker.php:110]
[24-May-2017 11:08:41 Europe/Madrid]   #7 DoorkeeperJIRAFeedWorker::publishFeedStory() called at [<phabricator>/src/applications/doorkeeper/worker/DoorkeeperFeedWorker.php:180]
[24-May-2017 11:08:41 Europe/Madrid]   #8 DoorkeeperFeedWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:123]
[24-May-2017 11:08:41 Europe/Madrid]   #9 PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php:171]
[24-May-2017 11:08:41 Europe/Madrid]   #10 PhabricatorWorkerActiveTask::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php:22]
[24-May-2017 11:08:41 Europe/Madrid]   #11 PhabricatorTaskmasterDaemon::run() called at [<phutil>/src/daemon/PhutilDaemon.php:219]
[24-May-2017 11:08:41 Europe/Madrid]   #12 PhutilDaemon::execute() called at [<phutil>/scripts/daemon/exec/exec_daemon.php:131]

Our behavior here is also legitimately wrong, and we can look at fixing this upsteam once we understand what's actually going wrong. I think if you dig in a bit you'll find a set of reproduction steps that are something like this:

  • Create two accounts, A and B.
  • Create a JIRA adapter with "Comment" and "Link" enabled.
  • Link A and B to JIRA.
  • <<<In JIRA, go toggle some setting which disables linking for those users, or disables linking for the entire JIRA install, or whatever else.>>>
  • Add A and B as reviewers on change R (then post a comment or whatever).
  • Observe: Comments post twice (what happens with links?)

We need to know what the missing step is -- it might be a JIRA setting like "Disable linking"; it might be an account setting like "Prevent this user from linking tickets to external objects"; it might be an issue setting like not giving the user permission on the issue; or it might be something else like you haven't paid Atlassian enough Link Tax. I'm not familiar enough with JIRA to offer much of a guess.

If that's roughly what's happened, we can look at solutions based on what the underlying issue is -- maybe testing for settings, permissions, etc.

Next steps:

  • We're waiting for the missing JIRA configuration, described above, that will let us reproduce this issue and move forward.
  • If we don't receive the missing step, we can't move forward. We'll close this report after it spends a few more days in limbo.
  • If you identify the missing step in the future (maybe you just don't have time now, or can't figure it out after some digging) feel free to file a new report at that time, even if we close this one.
epriestley closed this task as Invalid.Jun 2 2017, 11:20 AM

We haven't received more information on this, so we don't know how to reproduce it and can't move forward. Feel free to file a new issue in the future with complete reproduction steps, per above.