launchpad-reviewers team mailing list archive

[Colin Watson <mp+430166@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:branch-scan-race into launchpad:master.

Commit message:
Fix race between branchChanged requests and branch scan jobs

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1883067 in Launchpad itself: "Error while pushing a bzr branch"
  https://bugs.launchpad.net/launchpad/+bug/1883067

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/430166

If a `branchChanged` XML-RPC request happened to run at the same time as a branch scan job for the same branch, then they may race to update the same `Branch` row, causing one to have to roll back its transaction and try again.  However, a bug in the `transaction` library meant that the exception raised by the before-commit hook to gather job state was incorrectly ignored, and as a result an `InFailedSqlTransaction` exception was raised instead, which isn't retried.

Upgrade to `transaction==3.0.1` to fix this.

Dependencies MP: https://code.launchpad.net/~cjwatson/lp-source-dependencies/+git/lp-source-dependencies/+merge/430165
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:branch-scan-race into launchpad:master.

diff --git a/lib/lp/code/xmlrpc/tests/test_codehosting.py b/lib/lp/code/xmlrpc/tests/test_codehosting.py
index 60089a2..b25fc57 100644
--- a/lib/lp/code/xmlrpc/tests/test_codehosting.py
+++ b/lib/lp/code/xmlrpc/tests/test_codehosting.py
@@ -5,11 +5,14 @@
 
 import datetime
 import os
+import threading
 
 import pytz
 import six
+import transaction
 from breezy import controldir
 from breezy.urlutils import escape
+from psycopg2.extensions import TransactionRollbackError
 from testscenarios import WithScenarios, load_tests_apply_scenarios
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -19,7 +22,10 @@ from lp.app.errors import NotFoundError
 from lp.code.bzr import BranchFormat, ControlFormat, RepositoryFormat
 from lp.code.enums import BranchType
 from lp.code.errors import UnknownBranchTypeError
-from lp.code.interfaces.branch import BRANCH_NAME_VALIDATION_ERROR_MESSAGE
+from lp.code.interfaces.branch import (
+    BRANCH_NAME_VALIDATION_ERROR_MESSAGE,
+    IBranchSet,
+)
 from lp.code.interfaces.branchlookup import IBranchLookup
 from lp.code.interfaces.branchtarget import IBranchTarget
 from lp.code.interfaces.codehosting import (
@@ -39,6 +45,7 @@ from lp.code.xmlrpc.codehosting import (
 )
 from lp.codehosting.inmemory import InMemoryFrontend
 from lp.services.database.constants import UTC_NOW
+from lp.services.features.testing import MemoryFeatureFixture
 from lp.services.scripts.interfaces.scriptactivity import IScriptActivitySet
 from lp.services.webapp.escaping import html_escape
 from lp.services.webapp.interfaces import ILaunchBag
@@ -827,6 +834,45 @@ class CodehostingTest(WithScenarios, TestCaseWithFactory):
             ),
         )
 
+    def test_branchChanged_race_with_scan_job(self):
+        # The branchChanged XML-RPC request may race with something else
+        # that's updating the branch at the same time.
+        if self.layer == FunctionalLayer:
+            self.skipTest("Only implemented for DB testing.")
+
+        self.useFixture(
+            MemoryFeatureFixture(
+                {"jobs.celery.enabled_classes": "BranchScanJob"}
+            )
+        )
+        branch = self.factory.makeAnyBranch()
+        transaction.commit()
+
+        def update_branch(unique_name):
+            login(ANONYMOUS)
+            branch = getUtility(IBranchSet).getByUniqueName(unique_name)
+            removeSecurityProxy(branch).last_mirrored_id = "something"
+            transaction.commit()
+
+        unique_name = branch.unique_name  # begins transaction
+        # Force a race by updating the same branch from another thread.
+        update_thread = threading.Thread(
+            target=update_branch, args=(unique_name,)
+        )
+        update_thread.start()
+        update_thread.join()
+
+        self.codehosting_api.branchChanged(
+            branch.owner.id,
+            branch.id,
+            "",
+            "rev1",
+            *self.arbitrary_format_strings,
+        )
+        # We get this exception from the failed UPDATE, not
+        # InFailedSqlTransaction from a subsequent statement.
+        self.assertRaises(TransactionRollbackError, transaction.commit)
+
     def assertNotFound(self, requester, path):
         """Assert that the given path cannot be found."""
         if requester not in [LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES]:
diff --git a/requirements/launchpad.txt b/requirements/launchpad.txt
index bcf7a27..3f92fef 100644
--- a/requirements/launchpad.txt
+++ b/requirements/launchpad.txt
@@ -169,6 +169,7 @@ testresources==0.2.7
 testscenarios==0.4
 testtools==2.5.0
 timeline==0.0.7
+transaction==3.0.1
 treq==18.6.0
 # lp:~launchpad/twisted:lp-backport
 Twisted==20.3.0+lp9