launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/branchscan-deleted-branch2-602323 into lp:launchpad.

Commit message:
Log a message instead of oopsing if a scanned branch is deleted.

Requested reviews:
  Ian Booth (wallyworld)
Related bugs:
  Bug #602323 in Launchpad itself: "branch scanner throws LostObjectError if the branch it is scanning is deleted"
  https://bugs.launchpad.net/launchpad/+bug/602323

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/branchscan-deleted-branch2-602323/+merge/133397

== Implementation ==

Ensure branch scan job does not oops if a scanned branch is deleted. Simply catch the lost object exception and log a warning.

Changes in this new version:

- use standard job metadata to store the branch name instead of a custom attribute

== Tests ==

Write a new TestBranchScanJob test.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/model/branchjob.py
  lib/lp/code/model/tests/test_branchjob.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/branchscan-deleted-branch2-602323/+merge/133397
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/code/model/branchjob.py'
--- lib/lp/code/model/branchjob.py	2012-11-07 04:43:07 +0000
+++ lib/lp/code/model/branchjob.py	2012-11-08 04:35:24 +0000
@@ -44,6 +44,7 @@
     SQLObjectNotFound,
     StringCol,
     )
+from storm.exceptions import LostObjectError
 from storm.expr import (
     And,
     SQL,
@@ -317,18 +318,27 @@
     @classmethod
     def create(cls, branch):
         """See `IBranchScanJobSource`."""
-        branch_job = BranchJob(branch, cls.class_job_type, {})
+        branch_job = BranchJob(
+            branch, cls.class_job_type, {'branch_name': branch.unique_name})
         return cls(branch_job)
 
+    def __init__(self, branch_job):
+        super(BranchScanJob, self).__init__(branch_job)
+        self._cached_branch_name = self.metadata['branch_name']
+
     def run(self):
         """See `IBranchScanJob`."""
         from lp.services.scripts import log
         with server(get_ro_server(), no_replace=True):
-            lock = try_advisory_lock(
-                LockType.BRANCH_SCAN, self.branch.id, Store.of(self.branch))
-            with lock:
-                bzrsync = BzrSync(self.branch, log)
-                bzrsync.syncBranchAndClose()
+            try:
+                with try_advisory_lock(
+                    LockType.BRANCH_SCAN, self.branch.id,
+                    Store.of(self.branch)):
+                    bzrsync = BzrSync(self.branch, log)
+                    bzrsync.syncBranchAndClose()
+            except LostObjectError:
+                log.warning('Skipping branch %s because it has been deleted.'
+                    % self._cached_branch_name)
 
     @classmethod
     @contextlib.contextmanager

=== modified file 'lib/lp/code/model/tests/test_branchjob.py'
--- lib/lp/code/model/tests/test_branchjob.py	2012-11-07 04:43:07 +0000
+++ lib/lp/code/model/tests/test_branchjob.py	2012-11-08 04:35:24 +0000
@@ -174,6 +174,26 @@
 
         self.assertEqual(db_branch.revision_count, 5)
 
+    def test_branch_deleted(self):
+        """Ensure a job for a deleted branch completes with logged message."""
+        self.useBzrBranches(direct_database=True)
+
+        db_branch, bzr_tree = self.create_branch_and_tree()
+        # XXX: AaronBentley 2010-08-06 bug=614404: a bzr username is
+        # required to generate the revision-id.
+        with override_environ(BZR_EMAIL='me@xxxxxxxxxxx'):
+            bzr_tree.commit('First commit', rev_id='rev1')
+            LaunchpadZopelessLayer.commit()
+
+        expected_message = (
+            'Skipping branch %s because it has been deleted.'
+            % db_branch.unique_name)
+        job = BranchScanJob.create(db_branch)
+        db_branch.destroySelf()
+        with self.expectedLog(expected_message):
+            with dbuser(config.branchscanner.dbuser):
+                job.run()
+
     def test_run_with_private_linked_bug(self):
         """Ensure the job scans a branch with a private bug in the revprops."""
         self.useBzrBranches(direct_database=True)