launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

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

Commit message:
Ensure branch scan job does not oops if a scanned branch is deleted.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
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-branch-602323/+merge/133175

== Implementation ==

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

== 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-branch-602323/+merge/133175
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/branchscan-deleted-branch-602323 into lp:launchpad.

=== modified file 'lib/lp/code/model/branchjob.py'
--- lib/lp/code/model/branchjob.py	2012-09-28 06:25:44 +0000
+++ lib/lp/code/model/branchjob.py	2012-11-07 03:15:24 +0000
@@ -44,6 +44,7 @@
     SQLObjectNotFound,
     StringCol,
     )
+from storm.exceptions import LostObjectError
 from storm.expr import (
     And,
     SQL,
@@ -239,6 +240,8 @@
 
     def __init__(self, branch_job):
         self.context = branch_job
+        if branch_job.branch:
+            self.branch_unique_name = branch_job.branch.unique_name
 
     def __repr__(self):
         branch = self.branch
@@ -324,11 +327,16 @@
         """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:
+                lock = try_advisory_lock(
+                    LockType.BRANCH_SCAN, self.branch.id,
+                    Store.of(self.branch))
+                with lock:
+                    bzrsync = BzrSync(self.branch, log)
+                    bzrsync.syncBranchAndClose()
+            except LostObjectError:
+                log.warning('Skipping branch %s because it has been deleted.'
+                    % self.branch_unique_name)
 
     @classmethod
     @contextlib.contextmanager

=== modified file 'lib/lp/code/model/tests/test_branchjob.py'
--- lib/lp/code/model/tests/test_branchjob.py	2012-09-18 18:36:09 +0000
+++ lib/lp/code/model/tests/test_branchjob.py	2012-11-07 03:15: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)
+            with self.expectedLog(expected_message):
+                job = BranchScanJob.create(db_branch)
+                db_branch.destroySelf()
+                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)