Better handling of fatal upload errors

[Michael Nelson]

Currently when a person uploads a bad source, such as a changes file without a mandatory binary field, a FatalUploadError is raised, and an oops created but the user never receives an email response.

Examples:
http://pastebin.ubuntu.com/344658/

After uploading the package to dogfood, we were able to see the (obvious) issue:
http://pastebin.ubuntu.com/344696/

  FatalUploadError: Unable to find mandatory field 'Binary' in the changes file.
   OOPS-1921BUILDMASTER1, OOPS-1921BUILDMASTER2, OOPS-1921PPA6

  FatalUploadError: Unable to find mandatory field 'Files' in the changes file.
   OOPS-1921PPA4, OOPS-1921PPA5

  FatalUploadError: Unable to find mandatory field 'Date' in the changes file.
   OOPS-2021FTPMASTER5

  FatalUploadError: File /srv/launchpad.net/ppa-queue/incoming/upload-ftp-20110728-172729-005330/~ferramroberto/varie/ubuntu/grsync_1.2.0-1~lffl~maverick~ppa_source.changes is signed with a deactivated key 85C20848
   OOPS-2035PPA5

  FatalUploadError: Signing key 0D9EDC5F633B05D4763342C376D4635FB2A34D75 not registered in launchpad.
   OOPS-2047PPA10

We should handle this exception and notify the user via email.

[Michael Nelson (michael.nelson)]

Pre-implementation notes requiring feedback:

After a brief survey of the code in lp.archiveuploader.changesfile.ChangesFile.__init__(), it seems that we generate an oops (rather than rejecting the upload with a nice email) if:

 1. The changes file can't be parsed correctly,
 2. *any* of the mandatory fields are not present (currently source, binary, architecture, version, distribution, maintainer, files, changes),
 3. the format of the changes file is not supported, or
 4. the signature could not be verified (for a number of reasons).

I assume (2) is happening because if we can't verify the maintainer (one of the mandatory fields), we know we can't email them, but I'm not sure why we've included the other fields in this check.

So, the plan is to update (2) above simply to:

2. The maintainer field is not present (hrm, should really be changed-by? What's the reason that changed-by is not already a mandatory field in this check?)

and remove (3) altogether from __init__, and then add two new methods:

ChangesFile.checkMandatoryFields()
ChangesFile.checkChangesFormat()

which can be used by NascentUpload.process (which calls run_and_reject_on_errors).

The result will be that we only oops in situations where we don't have a signer or maintainer email.

Note: at first I was confused why we don't include changed-by as a mandatory field, but realised it might be because it's not required by policy (http://www.debian.org/doc/debian-policy/ch-controlfields.html#s-debianchangesfiles). But we do raise an UploadError during checkAddresses later if it's not present (which doesn't generate an oops. but rejects the upload, which will try to email the signer as per normal).

Note: currently PackageUpload._getRecipients() always includes either the signer or the changed-by address. But even though these may be present, an email will still not be sent if they have no preferredEmail etc.

[William Grant (wgrant)]

The OOPS is being handled separately, in bug #862032. This bug is now about trying harder to notify; in some cases we have a valid signature, so we can notify the signer even if the rest of the file is corrupt.

[Robert Collins (lifeless)]

Note that this would be much easier if we knew the uploader and thus could just notify their account. We do for SFTP uploads. I'm inclined to suggest that we deprecate FTP uploads and spend no effort to make them better - diminishing returns, and a known complex stack which is hard for new users (exactly the users that will make bad packages) to work with,

[Robert Collins (lifeless)]

Also, @bigjools, are you hacking on this still?

[Julian Edwards (julian-edwards)]

Nope, removed myself.

[Julian Edwards (julian-edwards)]

"I'm inclined to suggest that we deprecate FTP uploads"

That will not happen - you will encounter *serious* resistance from Ubuntu people.

[Francis J. Lacoste (flacoste)]

@julian resistance on what ground?

[Julian Edwards (julian-edwards)]

* dput with SFTP provides no progress information
* client support in Ubuntu is recent and the dput.cf defaults to FTP so it's not reasonable to disable it in LP until that's changed, including stable releases
* the dput.cf for SFTP has to have the user's LP name in it, so is hard to have a default at all
* Debian's dput doesn't have SFTP support yet

It's not insurmountable but has both political and technical issues.

[Robert Collins (lifeless)]

On Thu, Jan 5, 2012 at 5:45 AM, Julian Edwards
<email address hidden> wrote:
> * dput with SFTP provides no progress information
> * client support in Ubuntu is recent and the dput.cf defaults to FTP so it's not reasonable to disable it in LP until that's changed, including stable releases
> * the dput.cf for SFTP has to have the user's LP name in it, so is hard to have a default at all
> * Debian's dput doesn't have SFTP support yet
>
> It's not insurmountable but has both political and technical issues.

FWIW I talked with cjwatson about this before adding that comment.

Dput could default to sftp, prompt for the username, and fallback to
FTP if none was supplied or there was no terminal on which to prompt.
If it succeeded with a username, it would save it in a personal
dotfile and use it the next time.

-Rob

[Launchpad QA Bot (lpqabot)]

Fixed in stable r18472 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/stable/revision/18472>.