launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-store-upload-job-commit into lp:launchpad.

Commit message:
Consistently commit transactions in SnapStoreUploadJob.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1833424 in Launchpad itself: "snap:build:0.1 doesn't trigger on store_upload_status == 'Uploaded'"
  https://bugs.launchpad.net/launchpad/+bug/1833424

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-store-upload-job-commit/+merge/369243

We were previously ensuring that the transaction is committed when the job fails, but not when raising a retryable exception or after sending event notifications.  The latter case caused some webhook deliveries not to be sent.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-store-upload-job-commit into lp:launchpad.

=== modified file 'lib/lp/snappy/model/snapbuildjob.py'
--- lib/lp/snappy/model/snapbuildjob.py	2019-06-19 14:04:34 +0000
+++ lib/lp/snappy/model/snapbuildjob.py	2019-06-24 12:07:53 +0000
@@ -1,4 +1,4 @@
-# Copyright 2016-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Snap build jobs."""
@@ -280,11 +280,15 @@
     # Ideally we'd just override Job._set_status or similar, but
     # lazr.delegates makes that difficult, so we use this to override all
     # the individual Job lifecycle methods instead.
-    def _do_lifecycle(self, method_name, *args, **kwargs):
+    def _do_lifecycle(self, method_name, manage_transaction=False,
+                      *args, **kwargs):
         old_store_upload_status = self.snapbuild.store_upload_status
-        getattr(super(SnapStoreUploadJob, self), method_name)(*args, **kwargs)
+        getattr(super(SnapStoreUploadJob, self), method_name)(
+            *args, manage_transaction=manage_transaction, **kwargs)
         if self.snapbuild.store_upload_status != old_store_upload_status:
             notify(SnapBuildStoreUploadStatusChangedEvent(self.snapbuild))
+            if manage_transaction:
+                transaction.commit()
 
     def start(self, *args, **kwargs):
         self._do_lifecycle("start", *args, **kwargs)
@@ -329,39 +333,42 @@
         """See `IRunnableJob`."""
         client = getUtility(ISnapStoreClient)
         try:
-            if "status_url" not in self.store_metadata:
-                self.status_url = client.upload(self.snapbuild)
-                # We made progress, so reset attempt_count.
-                self.attempt_count = 1
-            if self.store_url is None:
-                # This is no longer strictly necessary as the store is handling
-                # releases via the release intent, but we export various fields
-                # via the api, so once this is called, we're done with
-                # this task
-                self.store_url, self.store_revision = (
-                    client.checkStatus(self.status_url))
-            self.error_message = None
-        except self.retry_error_types:
-            raise
-        except Exception as e:
-            if (isinstance(e, SnapStoreError) and e.can_retry and
-                    self.attempt_count <= self.max_retries):
-                raise RetryableSnapStoreError(e.message, detail=e.detail)
-            self.error_message = str(e)
-            self.error_messages = getattr(e, "messages", None)
-            self.error_detail = getattr(e, "detail", None)
-            if isinstance(e, UnauthorizedUploadResponse):
-                mailer = SnapBuildMailer.forUnauthorizedUpload(self.snapbuild)
-                mailer.sendAll()
-            elif isinstance(e, BadRefreshResponse):
-                mailer = SnapBuildMailer.forRefreshFailure(self.snapbuild)
-                mailer.sendAll()
-            elif isinstance(e, UploadFailedResponse):
-                mailer = SnapBuildMailer.forUploadFailure(self.snapbuild)
-                mailer.sendAll()
-            elif isinstance(e, (BadScanStatusResponse, ScanFailedResponse)):
-                mailer = SnapBuildMailer.forUploadScanFailure(self.snapbuild)
-                mailer.sendAll()
+            try:
+                if "status_url" not in self.store_metadata:
+                    self.status_url = client.upload(self.snapbuild)
+                    # We made progress, so reset attempt_count.
+                    self.attempt_count = 1
+                if self.store_url is None:
+                    # This is no longer strictly necessary as the store is
+                    # handling releases via the release intent, but we
+                    # export various fields via the API, so once this is
+                    # called, we're done with this task.
+                    self.store_url, self.store_revision = (
+                        client.checkStatus(self.status_url))
+                self.error_message = None
+            except self.retry_error_types:
+                raise
+            except Exception as e:
+                if (isinstance(e, SnapStoreError) and e.can_retry and
+                        self.attempt_count <= self.max_retries):
+                    raise RetryableSnapStoreError(e.message, detail=e.detail)
+                self.error_message = str(e)
+                self.error_messages = getattr(e, "messages", None)
+                self.error_detail = getattr(e, "detail", None)
+                mailer_factory = None
+                if isinstance(e, UnauthorizedUploadResponse):
+                    mailer_factory = SnapBuildMailer.forUnauthorizedUpload
+                elif isinstance(e, BadRefreshResponse):
+                    mailer_factory = SnapBuildMailer.forRefreshFailure
+                elif isinstance(e, UploadFailedResponse):
+                    mailer_factory = SnapBuildMailer.forUploadFailure
+                elif isinstance(e,
+                                (BadScanStatusResponse, ScanFailedResponse)):
+                    mailer_factory = SnapBuildMailer.forUploadScanFailure
+                if mailer_factory is not None:
+                    mailer_factory(self.snapbuild).sendAll()
+                raise
+        except Exception:
             # The normal job infrastructure will abort the transaction, but
             # we want to commit instead: the only database changes we make
             # are to this job's metadata and should be preserved.

=== modified file 'lib/lp/snappy/tests/test_snapbuildjob.py'
--- lib/lp/snappy/tests/test_snapbuildjob.py	2019-06-20 13:16:35 +0000
+++ lib/lp/snappy/tests/test_snapbuildjob.py	2019-06-24 12:07:53 +0000
@@ -17,6 +17,7 @@
     MatchesListwise,
     MatchesStructure,
     )
+import transaction
 from zope.interface import implementer
 from zope.security.proxy import removeSecurityProxy
 
@@ -58,6 +59,17 @@
 from lp.testing.mail_helpers import pop_notifications
 
 
+def run_isolated_jobs(jobs):
+    """Run a sequence of jobs, ensuring transaction isolation.
+
+    We abort the transaction after each job to make sure that there is no
+    relevant uncommitted work.
+    """
+    for job in jobs:
+        JobRunner([job]).runAll()
+        transaction.abort()
+
+
 @implementer(ISnapStoreClient)
 class FakeSnapStoreClient:
 
@@ -161,7 +173,7 @@
         client.checkStatus.result = (self.store_url, 1)
         self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
-            JobRunner([job]).runAll()
+            run_isolated_jobs([job])
         self.assertEqual([((snapbuild,), {})], client.upload.calls)
         self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
@@ -182,7 +194,7 @@
         client.upload.failure = ValueError("An upload failure")
         self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
-            JobRunner([job]).runAll()
+            run_isolated_jobs([job])
         self.assertEqual([((snapbuild,), {})], client.upload.calls)
         self.assertEqual([], client.checkStatus.calls)
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
@@ -208,7 +220,7 @@
             "Authorization failed.")
         self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
-            JobRunner([job]).runAll()
+            run_isolated_jobs([job])
         self.assertEqual([((snapbuild,), {})], client.upload.calls)
         self.assertEqual([], client.checkStatus.calls)
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
@@ -254,7 +266,7 @@
             "Proxy error", can_retry=True)
         self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
-            JobRunner([job]).runAll()
+            run_isolated_jobs([job])
         self.assertEqual([((snapbuild,), {})], client.upload.calls)
         self.assertEqual([], client.checkStatus.calls)
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
@@ -272,7 +284,7 @@
         client.upload.result = self.status_url
         client.checkStatus.result = (self.store_url, 1)
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
-            JobRunner([job]).runAll()
+            run_isolated_jobs([job])
         self.assertEqual([((snapbuild,), {})], client.upload.calls)
         self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
@@ -299,7 +311,7 @@
         client.upload.failure = BadRefreshResponse("SSO melted.")
         self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
-            JobRunner([job]).runAll()
+            run_isolated_jobs([job])
         self.assertEqual([((snapbuild,), {})], client.upload.calls)
         self.assertEqual([], client.checkStatus.calls)
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
@@ -350,7 +362,7 @@
             "Failed to upload", detail="The proxy exploded.\n")
         self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
-            JobRunner([job]).runAll()
+            run_isolated_jobs([job])
         self.assertEqual([((snapbuild,), {})], client.upload.calls)
         self.assertEqual([], client.checkStatus.calls)
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
@@ -399,7 +411,7 @@
         client.checkStatus.failure = UploadNotScannedYetResponse()
         self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
-            JobRunner([job]).runAll()
+            run_isolated_jobs([job])
         self.assertEqual([((snapbuild,), {})], client.upload.calls)
         self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
@@ -417,7 +429,7 @@
         client.checkStatus.failure = None
         client.checkStatus.result = (self.store_url, 1)
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
-            JobRunner([job]).runAll()
+            run_isolated_jobs([job])
         self.assertEqual([], client.upload.calls)
         self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
@@ -448,7 +460,7 @@
                 {"message": "Confinement not allowed.", "link": "link2"}])
         self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
-            JobRunner([job]).runAll()
+            run_isolated_jobs([job])
         self.assertEqual([((snapbuild,), {})], client.upload.calls)
         self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
@@ -497,7 +509,7 @@
         client.checkStatus.result = (self.store_url, 1)
         self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
-            JobRunner([job]).runAll()
+            run_isolated_jobs([job])
         self.assertEqual([((snapbuild,), {})], client.upload.calls)
         self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
@@ -520,7 +532,7 @@
             "Proxy error", can_retry=True)
         self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
-            JobRunner([job]).runAll()
+            run_isolated_jobs([job])
         self.assertNotIn("status_url", job.metadata)
         self.assertEqual(timedelta(seconds=60), job.retry_delay)
         job.scheduled_start = None
@@ -529,7 +541,7 @@
         client.checkStatus.failure = UploadNotScannedYetResponse()
         for expected_delay in (15, 15, 30, 30, 60):
             with dbuser(config.ISnapStoreUploadJobSource.dbuser):
-                JobRunner([job]).runAll()
+                run_isolated_jobs([job])
             self.assertIn("status_url", job.snapbuild.store_upload_metadata)
             self.assertIsNone(job.store_url)
             self.assertEqual(
@@ -538,7 +550,7 @@
         client.checkStatus.failure = None
         client.checkStatus.result = (self.store_url, 1)
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
-            JobRunner([job]).runAll()
+            run_isolated_jobs([job])
         self.assertEqual(self.store_url, job.store_url)
         self.assertIsNone(job.error_message)
         self.assertEqual([], pop_notifications())
@@ -556,7 +568,7 @@
         client.checkStatus.result = (self.store_url, 1)
         self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
-            JobRunner([job]).runAll()
+            run_isolated_jobs([job])
 
         previous_upload = client.upload.calls
         previous_checkStatus = client.checkStatus.calls
@@ -570,7 +582,7 @@
 
         # Run the job again
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
-            JobRunner([job]).runAll()
+            run_isolated_jobs([job])
 
         # Release is not called due to release intent in upload
         # but ensure that we have not called upload twice