1674310 - Can't re-submit a certain revision after abort during rebase, IndexError: list index out of range

Status: RESOLVED FIXED

(Conduit :: moz-phab, defect)

Description

[Markus Stange [:mstange]]

My first attempt to submit a stack of patches failed with CommandError: command 'b'rebase'' failed to complete successfully, halfway into the patch stack. (This bug is not about that failure, but about a follow-on failure. I filed bug 1674313 about the first failure.)

The last revision in that stack that was submitted successfully was https://phabricator.services.mozilla.com/D95250 .
From now on, if I want to re-submit that revision (without making any changes to it), moz-phab fails with IndexError: list index out of range:

% moz-phab submit 2fe3e73a0327 2fe3e73a0327 --trace
DEBUG    2020-10-30 00:03:44,209 MozPhab (0.1.92)
DEBUG    2020-10-30 00:03:44,210 found hg repo in /Users/mstange/code/mozilla
DEBUG    2020-10-30 00:03:44,471 $ config
DEBUG    2020-10-30 00:03:44,481 hg extensions: absorb, blackbox, clang-format, evolve, firefoxtree, fsmonitor, histedit, js-format, push-to-try, rebase, share, shelve
DEBUG    2020-10-30 00:03:44,482 $ log -T '{node}
' -r 2fe3e73a0327
DEBUG    2020-10-30 00:03:45,056 ['2fe3e73a0327067c258b0b17092bb777d365ab40']
DEBUG    2020-10-30 00:03:45,056 $ log -T '{node}
' -r 2fe3e73a0327
DEBUG    2020-10-30 00:03:45,229 ['2fe3e73a0327067c258b0b17092bb777d365ab40']
DEBUG    2020-10-30 00:03:45,230 https://phabricator.services.mozilla.com/api/user.whoami {}
DEBUG    2020-10-30 00:03:45,553 BMO API call: https://bugzilla.mozilla.org/rest/whoami
DEBUG    2020-10-30 00:03:45,563 GET https://bugzilla.mozilla.org/rest/whoami {'X-PHABRICATOR-TOKEN': 'cli-XXXXXXXXXXXXXXXXXXXXXXXXXX'}
DEBUG    2020-10-30 00:03:45,961 BMO API Error 306 - The API key you specified is invalid. Please check that you typed it correctly.
DEBUG    2020-10-30 00:03:45,962 BMOAPIErrori: The API key you specified is invalid. Please check that you typed it correctly.
DEBUG    2020-10-30 00:03:45,966 $ bookmark -T '{active} {bookmark}
'
DEBUG    2020-10-30 00:03:46,004 ['False arcpatch-D51541', 'False arcpatch-D6031', 'False blob-invalidation', 'False bug-905703-robocop-preprocess', 'False bug-925185-java-jars', 'False fx', 'False fx-team', 'False mc']
DEBUG    2020-10-30 00:03:46,004 $ id -q
DEBUG    2020-10-30 00:03:46,349 956a15259666
DEBUG    2020-10-30 00:03:46,350 $ bookmark moz-phab_956a15259666
DEBUG    2020-10-30 00:03:46,932 $ log -T '{rev}
{node}
{date|hgdate}
{author|person}
{author|email}
{desc}--6dbcdab702914fc8bd627f61f4462c5a--
' -r 2fe3e73a0327::2fe3e73a0327
DEBUG    2020-10-30 00:03:47,198 659886
2fe3e73a0327067c258b0b17092bb777d365ab40
1603242211 14400
Markus Stange
[e-mail]
Bug 1674279 - Increase scrollable range of the nested scroll frame so that the accelerated fling does not hit the end of the range before TestFlingAcceleration() can read the velocity. r=botond

This fixes APZScrollHandoffTester.ScrollgrabFlingAcceleration2.

Differential Revision: https://phabricator.services.mozilla.com/D95250--6dbcdab702914fc8bd627f61f4462c5a--

DEBUG    2020-10-30 00:03:47,199 $ log -T '{node}
' -r 'children(2fe3e73a0327067c258b0b17092bb777d365ab40)'
DEBUG    2020-10-30 00:03:47,407 ['441969492ca5f5705262d333097a2734095ecaef']
WARNING  2020-10-30 00:03:47,407 Submitting 1 commit for review:
DEBUG    2020-10-30 00:03:47,408 https://phabricator.services.mozilla.com/api/differential.revision.search {'constraints': {'ids': [95250]}, 'attachments': {'reviewers': True}}
DEBUG    2020-10-30 00:03:47,769 https://phabricator.services.mozilla.com/api/differential.diff.search {'constraints': {'phids': ['PHID-DIFF-nt7bsdty75rbibh7wdwm']}, 'attachments': {'commits': True}}
DEBUG    2020-10-30 00:03:48,110 $ bookmark -T '{bookmark}
'
DEBUG    2020-10-30 00:03:48,156 ['arcpatch-D51541', 'arcpatch-D6031', 'blob-invalidation', 'bug-905703-robocop-preprocess', 'bug-925185-java-jars', 'fx', 'fx-team', 'mc', 'moz-phab_956a15259666']
DEBUG    2020-10-30 00:03:48,156 $ bookmark --delete moz-phab_956a15259666
ERROR    2020-10-30 00:03:48,548 Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/mozphab/mozphab.py", line 83, in main
    args.func(repo, args)
  File "/usr/local/lib/python3.8/site-packages/mozphab/commands/submit.py", line 442, in submit
    show_commit_stack(commits, wip=args.wip, validate=True, ignore_reviewers=args.wip)
  File "/usr/local/lib/python3.8/site-packages/mozphab/commands/submit.py", line 198, in show_commit_stack
    != diffs[revision["fields"]["diffPHID"]]["attachments"][
IndexError: list index out of range

Sentry is attempting to send 0 pending error messages
Waiting up to 2 seconds
Press Ctrl-C to quit
%

It seems that the failure is in the change detection, so it's possible that this will work fine once I actually do make a change to that changeset.

Comment 1

[Ian Moody [:Kwan] (UTC+1)]

So this happens firstly because (presumably due to the abort during the original submit) moz-phab never made the differential.setdiffproperty call to write the local:commits metadata to the diff. The bug is that moz-phab isn't resilient to the metadata being missing and just zero-indexes into the ["attachments"]["commits"]["commits"] array that differential.diff.search returns, which in this case is empty (just helped a contributor in a similar situation with D96328 in #conduit).
Currently the only way to get past this is to either abandon that DREV, and remove the line from the commit message to create a new one when next submitting, or make some manual setdiffproperty calls to add suitable dummy data to the diff so that moz-phab can read something.

(As well as coping with missing data on read, it might also be worth doing what phabsend does, and calling setdiffproperty twice, once with the original data right after diff creation and then again after rebase with the new hashes. I presume at the moment it's only doing the post-rebase call, hence missing data if that fails and leads to an abort)

Comment 2

[Florent Viard]

I have opened another issue for a problem that I think is a little bit related to this one:
bug 1676540

And so I think that the following patch to fix it might also resolve the current issue:
https://phabricator.services.mozilla.com/D96655

Comment 3

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1674310 - Write commit metadata to diffs straight after creation. r=zeid,glob

This avoids leaving diffs without the required metadata if moz-phab aborts
during during rebase, which makes diffs unlandable (and also currently
un-updatable by moz-phab, which will be fixed by D96655 / Bug 1676540).

Comment 4

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Florent Viard from comment #2)

I have opened another issue for a problem that I think is a little bit related to this one:
bug 1676540

And so I think that the following patch to fix it might also resolve the current issue:
https://phabricator.services.mozilla.com/D96655

Ah, thanks. I did have a local patch to do that, but yours is a little nicer (just lacking a test, I'll comment on phab), so I've dropped that and I'll keep this bug for my second patch, which prevents rebase aborts by moz-phab leaving DREVs in this state, which is what caused this instance. Yours was indeed caused differently (you using the web upload form for the diff), but they both trigger the same IndexError bug in moz-phab on update attempts.

Comment 5

[:glob ✱]

https://github.com/mozilla-conduit/review/commit/7445df1797ce93061045a1a86b4db1e514e7657c