launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-761439 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #761439 in Launchpad itself: "Delayed copy publication sometimes crashes when reading changes file content"
  https://bugs.launchpad.net/launchpad/+bug/761439

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-761439/+merge/59317

Bug #761439 is caused by process-accepted not committing after each item is processed: sometimes they create library files that later items need to read, which can't happen before the transaction is committed. This branch changes process-accepted to commit after each upload, even on failure.

It seems unintuitive to commit on failure, but process-accepted makes changes to the on-disk archive. We can't roll back filesystem changes, so we need to commit a potentially partial acceptance to the DB too. The test is a bit awkward: a transaction synchronizer keeps track of the number of commits and ensures that only one item is processed per commit.

I've also added some more logging to process-accepted, to ease identification of the current queue item.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-761439/+merge/59317
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-761439 into lp:launchpad.

=== modified file 'lib/lp/soyuz/scripts/processaccepted.py'
--- lib/lp/soyuz/scripts/processaccepted.py	2010-09-23 02:12:27 +0000
+++ lib/lp/soyuz/scripts/processaccepted.py	2011-04-28 05:36:31 +0000
@@ -268,6 +268,8 @@
                     queue_items = distroseries.getQueueItems(
                         PackageUploadStatus.ACCEPTED, archive=archive)
                     for queue_item in queue_items:
+                        self.logger.debug(
+                            "Processing queue item %d" % queue_item.id)
                         try:
                             queue_item.realiseUpload(self.logger)
                         except Exception:
@@ -280,7 +282,14 @@
                             self.logger.error('%s (%s)' % (message,
                                 request.oopsid))
                         else:
+                            self.logger.debug(
+                                "Successfully processed queue item %d" %
+                                queue_item.id)
                             processed_queue_ids.append(queue_item.id)
+                        # Commit even on error; we may have altered the
+                        # on-disk archive, so the partial state must
+                        # make it to the DB.
+                        self.txn.commit()
 
             if not self.options.dryrun:
                 self.txn.commit()

=== modified file 'lib/lp/soyuz/tests/test_processaccepted.py'
--- lib/lp/soyuz/tests/test_processaccepted.py	2011-03-03 00:43:44 +0000
+++ lib/lp/soyuz/tests/test_processaccepted.py	2011-04-28 05:36:31 +0000
@@ -6,6 +6,7 @@
 from cStringIO import StringIO
 
 from debian.deb822 import Changes
+from testtools.matchers import LessThan
 
 from canonical.config import config
 from canonical.launchpad.webapp.errorlog import ErrorReportingUtility
@@ -15,6 +16,7 @@
 from lp.soyuz.enums import (
     ArchivePurpose,
     PackagePublishingStatus,
+    PackageUploadStatus,
     )
 from lp.soyuz.scripts.processaccepted import (
     get_bugs_from_changes_file,
@@ -131,6 +133,42 @@
             published_copy.status, PackagePublishingStatus.PENDING)
         self.assertEqual(copy_source, published_copy.sourcepackagerelease)
 
+    def test_commits_after_each_item(self):
+        # Test that the script commits after each item, not just at the end.
+        uploads = [
+            self.createWaitingAcceptancePackage(
+                distroseries=
+                    self.factory.makeDistroSeries(distribution=self.distro),
+                sourcename='source%d' % i)
+            for i in range(3)]
+
+        class UploadCheckingSynchronizer:
+
+            commit_count = 0
+
+            def beforeCompletion(inner_self, txn):
+                pass
+
+            def afterCompletion(inner_self, txn):
+                if txn.status != 'Committed':
+                    return
+                inner_self.commit_count += 1
+                done_count = len([
+                    upload for upload in uploads
+                    if upload.package_upload.status ==
+                        PackageUploadStatus.DONE])
+                self.assertEqual(
+                    min(len(uploads), inner_self.commit_count),
+                    done_count)
+
+        script = self.getScript([])
+        self.layer.txn.commit()
+        self.layer.switchDbUser(self.dbuser)
+        synch = UploadCheckingSynchronizer()
+        script.txn.registerSynch(synch)
+        script.main()
+        self.assertThat(len(uploads), LessThan(synch.commit_count))
+
 
 class TestBugsFromChangesFile(TestCaseWithFactory):
     """Test get_bugs_from_changes_file."""