Code review comment for lp:~jelmer/launchpad/653720-failedtoupload

Hi Jelmer and Steve,

I can't yet see why we have a txn abort in the finally clause of processUploadQueue? (ie. we abort remaining db-writes even if there were no exceptions (which is why this bug is happening in the first place).

AFAICS, the processChangesFile() method is already aborting the transaction when it finds errors (to ensure the NascentUpload isn't stored in the db?). So it shouldn't be necessary to do the extra abort. If it is necessary (because there is something else we're not seeing being written to the DB), then adding the commit() may be committing more than we think?

Long-term, I think we should sort the above out (ie. not have the extra abort in the finally clause, so that the status would be saved to the db. But it may be that Julian is keen for this fix to go out ASAP, in which case I'll approve it as is.

Julian?

{{{
12:31 < noodles> jelmer: sorry, it's not clear to me why we abort remaining transactions in a finally clause of processUploadQueue?
12:31 < bigjools> jml: when you've got free time *haha* we should do a virtual sprint and decide what needs doing next and possibly do it
12:32 < noodles> jelmer: that is, even if all the uploads are processed fine, we still abort remaining transactions?
12:32 < noodles> s/transactions/db-writes
12:33 < jelmer> noodles: in that particular case there shouldn't be any remaining transactions
12:34 < jelmer> noodles: I'm not entirely sure why aborting the remaining transactions is a good idea, other than being cautious
12:34 < noodles> jelmer: there shouldn't, but isn't this branch trying to fix the fact that there is by committing them earlier before the abort is called?
12:36 < jelmer> noodles: Hmm, that's a good point. Perhaps I could do the abort first then se tthe status / upload the log and commit again?
12:38 < noodles> jelmer: or not do the abort at all? I've only looked briefly, but the only thing I can see that we'd want to abort is the setting of build.date_finished (when the upload fails), but that could be easily avoided by putting it in an else-clause of the following conditional.
12:38 < noodles> But maybe there are other reasons for the abort that I've not seen.
12:39 < jelmer> noodles: I guess to prevent broken uploads from users from filling up the database with garbage on broken uploads
12:40 < jelmer> that's the only possible reason I can come up with
12:44 < noodles> jelmer: where is the db-upload created?
12:44 * noodles scans again
12:44 < noodles> Ah, processChangesFile()...
12:49 < noodles> But the transaction is already aborted within processChangesFile during various failures, so it still doesn't explain why we need the abort in the finally clause of processUploadQueue (which is the cause of this bug).
12:52 < jelmer> hmm, true
}}}