1709608 - Lando allows re-landing committed versions of DREVs, which may result in empty binary files

Status: NEW

(Conduit :: Lando, defect, P2)

Description

[Emilio Cobos Álvarez (:emilio)]

https://phabricator.services.mozilla.com/D113264 had an exif_resolution.jpg image, which was a copy of testing/web-platform/tests/density-size-correction/resources/exif-resolution-valid-hires.jpg. But somehow when I landed the patch it was empty. It seems Phabricator also thinks the file is empty, so this is probably a moz-phab issue.

Comment 1

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

I notice it bounced and got relanded. Did you re-submit with moz-phab after the backout or not? Because the second landing just landed the phabricator version of the previous landing, but it's a "known" issue that when the final landed versions of commits are recorded in phab, they don't include binary files, so you need to re-upload after a backout. So if you didn't re-upload this is a phabricator bug rather than a moz-phab one. (If you did re-upload then it's a moz-phab bug, but the bug is that it didn't think it needed to update that DREV)

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I did not resubmit it, because that revision was totally fine.

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

https://phabricator.services.mozilla.com/D113359 is also affected, I wonder this is some kind of a regression?

Comment 4

[Kagami Rosylight [:saschanaz] (they/them)]

Resubmitting sometimes kills the images and sometimes restores them, as far as I can tell.

Comment 5

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

Re-titling/componenting for the actual issue. Could also make this a phab bug about the fact that commit-sourced versions of DREVs lack the binary files, but I think it's likelier to be handled in Lando sooner if it's made to block landings when the current version of a DREV is sourced from a commit, than waiting for upstream Phab to fix.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

I did not resubmit it, because that revision was totally fine.

Then yeah, the empty binary files are expected. If you compare the last submitted DREV with the version created from the first landing you can see the issue. And the fact that that the first landing was fine.

(In reply to Kagami :saschanaz from comment #3)

https://phabricator.services.mozilla.com/D113359 is also affected, I wonder this is some kind of a regression?

Not a regression, this has always been the case as far as I know.

(In reply to Kagami :saschanaz from comment #4)

Resubmitting sometimes kills the images and sometimes restores them, as far as I can tell.

I'd be very surprised if resubmitting with moz-phab ever kills binary files, do you have an example you can link?

Comment 6

[Kagami Rosylight [:saschanaz] (they/them)]

I'd be very surprised if resubmitting with moz-phab ever kills binary files, do you have an example you can link?

Sure, this submission did: https://phabricator.services.mozilla.com/D113359?vs=435358&id=435402#change-K6J3tLo3ngge

Comment 7

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

(In reply to Kagami :saschanaz from comment #6)

I'd be very surprised if resubmitting with moz-phab ever kills binary files, do you have an example you can link?

Sure, this submission did: https://phabricator.services.mozilla.com/D113359?vs=435358&id=435402#change-K6J3tLo3ngge

That diff, ID 435402, is not a moz-phab submission, that's sourced from a commit. You can tell if you go to the History tab of the Table of Contents and check the "Base" column for text like "rMOZILLACENTRALa3f23a20b5322d0093d9f73189a1f64a9529567e" (or install the extension that'll give you a "Creation Method" column that'll tell you explicitly).

Comment 8

[Kagami Rosylight [:saschanaz] (they/them)]

Ah right, I mean pushing a commit, yes. The commit always had the images but pushing it via moz-phab randomly killed them or restored them.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Here's another example of this happening: https://hg.mozilla.org/integration/autoland/rev/7ce1b7de9a7c

Comment 12

[[e-mail] (:bhearsum)]

A more recent example of this is https://phabricator.services.mozilla.com/D148288. First landed in https://hg.mozilla.org/integration/autoland/rev/59eac90984cb1f248454fc5b87f1dd528547c3f3 (backed out for reasons unrelated to this bug), then relanded in https://hg.mozilla.org/integration/autoland/rev/9651db4a6e3f2dd2a63f6c30d77078f291380a49, which failed to add the binary file correctly (a DLL in this case).

Comment 14

[BMO Github Automation]

Attachment: [mozilla-conduit/lando-api] uplifts: query `differential.querydiffs` to find the latest non-`commit` diff (Bug 1709608) (#364)

Comment 15

[Mathew Hodson]

Was this fixed by https://github.com/mozilla-conduit/lando-api/commit/550bf9494ef7057c72a901a40546e22131f154d3

Comment 16

[Connor Sheehan [:sheehan]]

No, that PR only changed how uplift revisions are created, but this issue can still be triggered on non-uplift revisions.