1502486 - Adding new dependency fails with Phabricator summary contains old dependency

Status: RESOLVED FIXED

(Conduit :: moz-phab, defect, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

Attachment: Trace

Over in bug 1356920, I have a stack of 23 commits that I've been working with.

Recently, I did a change in my approach to the bug, and ended up shuffling some commits around. Some got dropped, new ones got added, and some ordering got changed.

After pushing with moz-phab submit, I manually updated the parent and child relationships using the "Edit Related Revisions" section of the web UI.

Then, after some more revisions, I pushed again, and I ended up getting this weird graph-cycle error:

[2018-10-26 18:01:19] EXCEPTION: (ConduitClientException) ERR-CONDUIT-CORE: Graph cycle detected (type=5, cycle=PHID-DREV-oykjzsbmuuk6cy6n64p5, PHID-DREV-oyojdhjyzhdlcw2tzzvs, PHID-DREV-aix5j6wn6a2mdi5ylzw6, PHID-DREV-blspyifgacr4jw3g23z7, PHID-DREV-zotpqerzdipgfu4k6e2y, PHID-DREV-mixogxpmlguo7hbf7yzm, PHID-DREV-fh6bxwgl6dfucaayg2pl, PHID-DREV-px6ltnzdm574t4tcoumc, PHID-DREV-3gbxtzc4xyjcwlcvsbuy, PHID-DREV-hrgaw4drflr7xe3xrcbt, PHID-DREV-eazg2jaav6oc7kf2wzcr, PHID-DREV-yrg5kscp5qtns6227iyc, PHID-DREV-tlh6p2woib6pg66cs5oe, PHID-DREV-nl4smv55gxgpmbez4ayz, PHID-DREV-yazkexc3mjcshtm5tuox, PHID-DREV-oykjzsbmuuk6cy6n64p5). at [<phutil>/src/conduit/ConduitFuture.php:62]
arcanist(head=master, ref.master=2650e8627a20), phutil(head=master, ref.master=603209bb1756)
  #0 ConduitFuture::didReceiveResult(array) called at [<phutil>/src/future/FutureProxy.php:58]
  #1 FutureProxy::getResult() called at [<phutil>/src/future/FutureProxy.php:35]
  #2 FutureProxy::resolve() called at [<phutil>/src/conduit/ConduitClient.php:64]
  #3 ConduitClient::callMethodSynchronous(string, array) called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:574]
  #4 ArcanistDiffWorkflow::run() called at [<arcanist>/scripts/arcanist.php:394]
ERROR    2018-10-26 14:01:19,950 Traceback (most recent call last):
  File "/Users/mikeconley/tools/moz-phab/moz-phab", line 1927, in main
    args.func(repo, args)
  File "/Users/mikeconley/tools/moz-phab/moz-phab", line 1632, in submit
    config.arc + arc_args, cwd=repo.path, never_log=True
  File "/Users/mikeconley/tools/moz-phab/moz-phab", line 196, in check_call_by_line
    raise subprocess.CalledProcessError(process.returncode, command)
CalledProcessError: Command '['arc', '--trace', 'diff', '--base', 'arc:this', '--allow-untracked', '--no-amend', '--no-ansi', '--message-file', '/var/folders/bq/n5qs5f650bl7kmj4h_fqsw2m0000gn/T/tmpGoe062', '--message', 'Revision updated.', '--update', '7822']' returned non-zero exit status 255


I worked with smacleod in #phabricator, and what I ended up needing to do was to update the Summary's of each of Phabricator review requests to ensure that their "Depends on DXXXX" were correct, since it seems we rely on that as well as the relationship encoded in "Edit Related Revisions".

Anyhow, smacleod said I should file this since he said this is probably something either moz-phab or Phabricator itself should handle.

I've attached a detailed trace log of when I got the error to the bug - smacleod said it'd be handy.

Comment 1

[Steven MacLeod [:smacleod]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #0)
> Anyhow, smacleod said I should file this since he said this is probably
> something either moz-phab or Phabricator itself should handle.
> 
> I've attached a detailed trace log of when I got the error to the bug -
> smacleod said it'd be handy.

As shown in the log the message moz-phab is using correctly specifies the new relationship. I suspect the problem could be due to 1 of two things:
a) We're including the "Differential Revision: <url>" part in the summary we're sending to Phabricator (where I believe arc actually only uses it locally to identify the revision), and it's being sent above our new "Depends on D<#>" line, with the summary otherwise being empty. Maybe this is causing something where we're not actually updating the summary?
b) Phabricator handles the cycle detection in the transaction combining both the old summary and the new. Maybe there is something here with the ordering of items in the transaction list arc is sending? Could just be a Phabricator bug as well.

Comment 2

[Mark Côté [:mcote]]

We'll handle this when we get to bug 1481539 (which I'd like to get to as soon as possible after we get some perf improvements in).  I'll mark it as a blocker of that bug so we don't forget.

Comment 3

[Connor Sheehan [:sheehan]]

We removed Depends on from moz-phab submitted revisions in bug 1529766, so this should be resolved now.