launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/report-pcj-oops-2 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1031092 in Launchpad itself: "OOPSing PPA async copy shows no failure reason in web UI"
  https://bugs.launchpad.net/launchpad/+bug/1031092

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/report-pcj-oops-2/+merge/119209

== Summary ==

https://code.launchpad.net/~cjwatson/launchpad/report-pcj-oops/+merge/117601 was almost right, but it failed to handle the case of an IntegrityError while committing the results of processing the job, since the transaction that changed PPCJ.error_message was then never committed.  This was actually the case encountered by the bug reporter, but I didn't notice it in the test suite because I hadn't managed to write a sufficiently careful test that failed at just the right point and then aborted the transaction.

== Proposed fix ==

If processing an OOPS involves setting PPCJ.error_message, abort before doing so and commit afterwards.

== LOC Rationale ==

+20.  Same as https://code.launchpad.net/~cjwatson/launchpad/report-pcj-oops/+merge/117601.

== Tests ==

bin/test -vvct test_packagecopyjob

== Demo and Q/A ==

Same as https://code.launchpad.net/~cjwatson/launchpad/report-pcj-oops/+merge/117601.
-- 
https://code.launchpad.net/~cjwatson/launchpad/report-pcj-oops-2/+merge/119209
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/report-pcj-oops-2 into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2012-08-08 13:53:44 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2012-08-10 23:28:25 +0000
@@ -516,10 +516,12 @@
     def notifyOops(self, oops):
         """See `IRunnableJob`."""
         if not self.error_message:
+            transaction.abort()
             self.reportFailure(
                 "Launchpad encountered an internal error while copying this"
                 " package.  It was logged with id %s.  Sorry for the"
                 " inconvenience." % oops["id"])
+            transaction.commit()
         super(PlainPackageCopyJob, self).notifyOops(oops)
 
     def run(self):

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2012-08-08 13:53:44 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2012-08-10 23:28:25 +0000
@@ -420,19 +420,37 @@
             "Destination pocket must be 'release' for a PPA.",
             job.error_message)
 
+    def assertOopsRecorded(self, job):
+        self.assertEqual(JobStatus.FAILED, job.status)
+        self.assertThat(
+            job.error_message, MatchesRegex(
+                "Launchpad encountered an internal error while copying this"
+                " package.  It was logged with id .*.  Sorry for the"
+                " inconvenience."))
+
     def test_target_ppa_message_unexpected_error(self):
         # When copying to a PPA archive, unexpected errors are stored in the
         # job's metadata with an apologetic message.
         job = self.makePPAJob()
         removeSecurityProxy(job).attemptCopy = FakeMethod(failure=Exception())
         self.runJob(job)
-        self.assertEqual(JobStatus.FAILED, job.status)
-        self.assertThat(
-            job.error_message,
-            MatchesRegex(
-                "Launchpad encountered an internal error while copying this"
-                " package.  It was logged with id .*.  Sorry for the"
-                " inconvenience."))
+        self.assertOopsRecorded(job)
+
+    def test_target_ppa_message_integrity_error(self):
+        # Even database integrity errors (which cause exceptions on commit)
+        # reliably store an error message in the job's metadata.
+        job = self.makePPAJob()
+        spr = self.factory.makeSourcePackageRelease(archive=job.target_archive)
+
+        def copy_integrity_error():
+            """Force an integrity error."""
+            spr.requestDiffTo(job.requester, spr)
+
+        removeSecurityProxy(job).attemptCopy = copy_integrity_error
+        self.runJob(job)
+        # Abort the transaction to simulate the job runner script exiting.
+        transaction.abort()
+        self.assertOopsRecorded(job)
 
     def test_run(self):
         # A proper test run synchronizes packages.