Merge lp:~allenap/launchpad/handle-status-ro-crash-bug-905853 into lp:launchpad

[Gavin Panella]

[r=gmb,julian-edwards][bug=905853] In buildmaster, always shift into a read-write database transaction access mode before updating PackageBuild statuses.

This fixes the problem described in the bug, but it also fixes the tests to (a) run correctly (i.e. with AsynchronousDeferredRunTest) and (b) run with a read-only transaction access mode by default. Combined, these brought out the problems described in the bug. This branch also (c) changes DatabaseTransactionPolicy.__enter__() to only care about open transactions when we're moving from a read-write access mode to read-only.

[Julian Edwards (julian-edwards)]

<bigjools> allenap: in the tests in your branch, it's probably worth refactoring the bit that sets properties on objects in a r/w transaction
<allenap> bigjools: Erm, which bit?
<allenap> bigjools: Like in test_handleStatus_OK_sets_build_log?
<bigjools> allenap: line 72/83 of the diff
<bigjools> allenap: I suspect we'll need to do that a lot more in the future
<allenap> bigjools: I don't know what a better way would be. I could instead enter read-only mode in each test individually (via a fixture) I guess.
<bigjools> allenap: I was thinking just a test helper
<bigjools> like setattr
<bigjools> but does the whole transactionny thing
<allenap> bigjools: With the removeSecurityProxy thing too I assume.
<bigjools> allenap: no, the caller can do that
<allenap> bigjools: Okay, I think I have a cool way to do that.

review: Approve

[Graham Binns (gmb)]

Nice branch; I've no complaints.

review: Approve (code)

=== modified file 'buildout-templates/bin/retest.in'
--- buildout-templates/bin/retest.in	2011-12-22 14:35:39 +0000
+++ buildout-templates/bin/retest.in	2012-01-03 12:26:30 +0000
@@ -28,7 +28,7 @@
 import os
 import re
 import sys
-from itertools import takewhile
+from itertools import takewhile, imap
 
 ${python-relative-path-setup}
 
@@ -38,6 +38,14 @@
 # Regular expression to match numbered stories.
 STORY_RE = re.compile("(.*)/\d{2}-.*")
 
+# Regular expression to remove terminal color escapes.
+COLOR_RE = re.compile("\x1b[[][0-9;]+m")
+
+
+def decolorize(text):
+    """Remove all ANSI terminal color escapes from `text`."""
+    return COLOR_RE.sub("", text)
+
 
 def get_test_name(test):
     """Get the test name of a failed test.
@@ -91,7 +99,8 @@
 
 
 if __name__ == '__main__':
-    tests = extract_tests(fileinput.input())
+    lines = imap(decolorize, fileinput.input())
+    tests = extract_tests(lines)
     if len(tests) >= 1:
         run_tests(tests)
     else:
 
=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2011-12-30 06:14:56 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2012-01-03 12:26:30 +0000
@@ -629,7 +629,8 @@
         from_addr, to_addrs, raw_msg = stub.test_emails.pop()
         foo_bar = "Foo Bar <[e-mail]>"
         daniel = "Daniel Silverstone <[e-mail]>"
-        self.assertEqual([e.strip() for e in to_addrs], [foo_bar, daniel])
+        self.assertContentEqual(
+            [foo_bar, daniel], [e.strip() for e in to_addrs])
         self.assertTrue(
             "NEW" in raw_msg, "Expected email containing 'NEW', got:\n%s"
             % raw_msg)
@@ -663,7 +664,8 @@
         from_addr, to_addrs, raw_msg = stub.test_emails.pop()
         daniel = "Daniel Silverstone <[e-mail]>"
         foo_bar = "Foo Bar <[e-mail]>"
-        self.assertEqual([e.strip() for e in to_addrs], [foo_bar, daniel])
+        self.assertContentEqual(
+            [foo_bar, daniel], [e.strip() for e in to_addrs])
         self.assertTrue("Waiting for approval" in raw_msg,
                         "Expected an 'upload awaits approval' email.\n"
                         "Got:\n%s" % raw_msg)
 
=== modified file 'lib/lp/buildmaster/interfaces/builder.py'
--- lib/lp/buildmaster/interfaces/builder.py	2012-01-01 02:58:52 +0000
+++ lib/lp/buildmaster/interfaces/builder.py	2012-01-03 12:26:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0211,E0213
@@ -195,6 +195,8 @@
 
     def setSlaveForTesting(proxy):
         """Sets the RPC proxy through which to operate the build slave."""
+        # XXX JeroenVermeulen 2011-11-09, bug=888010: Don't use this.
+        # It's a trap.  See bug for details.
 
     def verifySlaveBuildCookie(slave_build_id):
         """Verify that a slave's build cookie is consistent.
 
=== modified file 'lib/lp/buildmaster/manager.py'
--- lib/lp/buildmaster/manager.py	2012-01-01 02:58:52 +0000
+++ lib/lp/buildmaster/manager.py	2012-01-03 12:26:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Soyuz buildd slave manager logic."""
@@ -34,6 +34,7 @@
     BuildBehaviorMismatch,
     )
 from lp.buildmaster.model.builder import Builder
+from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 
 
 BUILDD_MANAGER_LOG_NAME = "slave-scanner"
@@ -111,13 +112,17 @@
     # algorithm for polling.
     SCAN_INTERVAL = 15
 
-    def __init__(self, builder_name, logger):
+    def __init__(self, builder_name, logger, clock=None):
         self.builder_name = builder_name
         self.logger = logger
+        if clock is None:
+            clock = reactor
+        self._clock = clock
 
     def startCycle(self):
         """Scan the builder and dispatch to it or deal with failures."""
         self.loop = LoopingCall(self.singleCycle)
+        self.loop.clock = self._clock
         self.stopping_deferred = self.loop.start(self.SCAN_INTERVAL)
         return self.stopping_deferred
 
@@ -138,51 +143,58 @@
         1. Print the error in the log
         2. Increment and assess failure counts on the builder and job.
         """
-        # Make sure that pending database updates are removed as it
-        # could leave the database in an inconsistent state (e.g. The
-        # job says it's running but the buildqueue has no builder set).
+        # Since this is a failure path, we could be in a broken
+        # transaction.  Get us a fresh one.
         transaction.abort()
 
         # If we don't recognise the exception include a stack trace with
         # the error.
         error_message = failure.getErrorMessage()
-        if failure.check(
+        familiar_error = failure.check(
             BuildSlaveFailure, CannotBuild, BuildBehaviorMismatch,
-            CannotResumeHost, BuildDaemonError, CannotFetchFile):
-            self.logger.info("Scanning %s failed with: %s" % (
-                self.builder_name, error_message))
+            CannotResumeHost, BuildDaemonError, CannotFetchFile)
+        if familiar_error:
+            self.logger.info(
+                "Scanning %s failed with: %s",
+                self.builder_name, error_message)
         else:
-            self.logger.info("Scanning %s failed with: %s\n%s" % (
+            self.logger.info(
+                "Scanning %s failed with: %s\n%s",
                 self.builder_name, failure.getErrorMessage(),
-                failure.getTraceback()))
+                failure.getTraceback())
 
         # Decide if we need to terminate the job or fail the
         # builder.
         try:
             builder = get_builder(self.builder_name)
-            builder.gotFailure()
-            if builder.currentjob is not None:
-                build_farm_job = builder.getCurrentBuildFarmJob()
-                build_farm_job.gotFailure()
-                self.logger.info(
-                    "builder %s failure count: %s, "
-                    "job '%s' failure count: %s" % (
+            transaction.commit()
+
+            with DatabaseTransactionPolicy(read_only=False):
+                builder.gotFailure()
+
+                if builder.currentjob is None:
+                    self.logger.info(
+                        "Builder %s failed a probe, count: %s",
+                        self.builder_name, builder.failure_count)
+                else:
+                    build_farm_job = builder.getCurrentBuildFarmJob()
+                    build_farm_job.gotFailure()
+                    self.logger.info(
+                        "builder %s failure count: %s, "
+                        "job '%s' failure count: %s",
                         self.builder_name,
                         builder.failure_count,
                         build_farm_job.title,
-                        build_farm_job.failure_count))
-            else:
-                self.logger.info(
-                    "Builder %s failed a probe, count: %s" % (
-                        self.builder_name, builder.failure_count))
-            assessFailureCounts(builder, failure.getErrorMessage())
-            transaction.commit()
+                        build_farm_job.failure_count)
+
+                assessFailureCounts(builder, failure.getErrorMessage())
+                transaction.commit()
         except:
             # Catastrophic code failure! Not much we can do.
+            transaction.abort()
             self.logger.error(
                 "Miserable failure when trying to examine failure counts:\n",
                 exc_info=True)
-            transaction.abort()
 
     def checkCancellation(self, builder):
         """See if there is a pending cancellation request.
@@ -236,14 +248,9 @@
         """
         # We need to re-fetch the builder object on each cycle as the
         # Storm store is invalidated over transaction boundaries.
-
         self.builder = get_builder(self.builder_name)
 
         def status_updated(ignored):
-            # Commit the changes done while possibly rescuing jobs, to
-            # avoid holding table locks.
-            transaction.commit()
-
             # See if we think there's an active build on the builder.
             buildqueue = self.builder.getBuildQueue()
 
@@ -253,14 +260,10 @@
                 return self.builder.updateBuild(buildqueue)
 
         def build_updated(ignored):
-            # Commit changes done while updating the build, to avoid
-            # holding table locks.
-            transaction.commit()
-
             # If the builder is in manual mode, don't dispatch anything.
             if self.builder.manual:
                 self.logger.debug(
-                    '%s is in manual mode, not dispatching.' %
+                    '%s is in manual mode, not dispatching.',
                     self.builder.name)
                 return
 
@@ -278,22 +281,33 @@
                 job = self.builder.currentjob
                 if job is not None and not self.builder.builderok:
                     self.logger.info(
-                        "%s was made unavailable, resetting attached "
-                        "job" % self.builder.name)
-                    job.reset()
+                        "%s was made unavailable; resetting attached job.",
+                        self.builder.name)
                     transaction.commit()
+                    with DatabaseTransactionPolicy(read_only=False):
+                        job.reset()
+                        transaction.commit()
                 return
 
             # See if there is a job we can dispatch to the builder slave.
 
+            # XXX JeroenVermeulen 2011-10-11, bug=872112: The job's
+            # failure count will be reset once the job has started
+            # successfully.  Because of intervening commits, you may see
+            # a build with a nonzero failure count that's actually going
+            # to succeed later (and have a failure count of zero).  Or
+            # it may fail yet end up with a lower failure count than you
+            # saw earlier.
             d = self.builder.findAndStartJob()
 
             def job_started(candidate):
                 if self.builder.currentjob is not None:
                     # After a successful dispatch we can reset the
                     # failure_count.
-                    self.builder.resetFailureCount()
                     transaction.commit()
+                    with DatabaseTransactionPolicy(read_only=False):
+                        self.builder.resetFailureCount()
+                        transaction.commit()
                     return self.builder.slave
                 else:
                     return None
@@ -372,6 +386,7 @@
         self.logger = self._setupLogger()
         self.new_builders_scanner = NewBuildersScanner(
             manager=self, clock=clock)
+        self.transaction_policy = DatabaseTransactionPolicy(read_only=True)
 
     def _setupLogger(self):
         """Set up a 'slave-scanner' logger that redirects to twisted.
@@ -390,16 +405,28 @@
         logger.setLevel(level)
         return logger
 
+    def enterReadOnlyDatabasePolicy(self):
+        """Set the database transaction policy to read-only.
+
+        Any previously pending changes are committed first.
+        """
+        transaction.commit()
+        self.transaction_policy.__enter__()
+
+    def exitReadOnlyDatabasePolicy(self, *args):
+        """Reset database transaction policy to the default read-write."""
+        self.transaction_policy.__exit__(None, None, None)
+
     def startService(self):
         """Service entry point, called when the application starts."""
+        # Avoiding circular imports.
+        from lp.buildmaster.interfaces.builder import IBuilderSet
+
+        self.enterReadOnlyDatabasePolicy()
 
         # Get a list of builders and set up scanners on each one.
-
-        # Avoiding circular imports.
-        from lp.buildmaster.interfaces.builder import IBuilderSet
-        builder_set = getUtility(IBuilderSet)
-        builders = [builder.name for builder in builder_set]
-        self.addScanForBuilders(builders)
+        self.addScanForBuilders(
+            [builder.name for builder in getUtility(IBuilderSet)])
         self.new_builders_scanner.scheduleScan()
 
         # Events will now fire in the SlaveScanner objects to scan each
@@ -420,6 +447,7 @@
         # stopped, so we can wait on them all at once here before
         # exiting.
         d = defer.DeferredList(deferreds, consumeErrors=True)
+        d.addCallback(self.exitReadOnlyDatabasePolicy)
         return d
 
     def addScanForBuilders(self, builders):
 
=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2012-01-02 11:21:14 +0000
+++ lib/lp/buildmaster/model/builder.py	2012-01-03 12:26:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009,2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0611,W0212
@@ -66,6 +66,7 @@
     SQLBase,
     sqlvalues,
     )
+from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 from lp.services.helpers import filenameToContentType
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
@@ -545,6 +546,8 @@
 
     def setSlaveForTesting(self, proxy):
         """See IBuilder."""
+        # XXX JeroenVermeulen 2011-11-09, bug=888010: Don't use this.
+        # It's a trap.  See bug for details.
         self._testing_slave = proxy
         del get_property_cache(self).slave
 
@@ -673,10 +676,13 @@
                 bytes_written = out_file.tell()
                 out_file.seek(0)
 
-                library_file = getUtility(ILibraryFileAliasSet).create(
-                    filename, bytes_written, out_file,
-                    contentType=filenameToContentType(filename),
-                    restricted=private)
+                transaction.commit()
+                with DatabaseTransactionPolicy(read_only=False):
+                    library_file = getUtility(ILibraryFileAliasSet).create(
+                        filename, bytes_written, out_file,
+                        contentType=filenameToContentType(filename),
+                        restricted=private)
+                    transaction.commit()
             finally:
                 # Remove the temporary file.  getFile() closes the file
                 # object.
@@ -714,7 +720,7 @@
     def acquireBuildCandidate(self):
         """Acquire a build candidate in an atomic fashion.
 
-        When retrieiving a candidate we need to mark it as building
+        When retrieving a candidate we need to mark it as building
         immediately so that it is not dispatched by another builder in the
         build manager.
 
@@ -724,12 +730,15 @@
         can be in this code at the same time.
 
         If there's ever more than one build manager running at once, then
-        this code will need some sort of mutex.
+        this code will need some sort of mutex, or run in a single
+        transaction.
         """
         candidate = self._findBuildCandidate()
         if candidate is not None:
-            candidate.markAsBuilding(self)
             transaction.commit()
+            with DatabaseTransactionPolicy(read_only=False):
+                candidate.markAsBuilding(self)
+                transaction.commit()
         return candidate
 
     def _findBuildCandidate(self):
@@ -792,13 +801,17 @@
         store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
         candidate_jobs = store.execute(query).get_all()
 
-        for (candidate_id,) in candidate_jobs:
-            candidate = getUtility(IBuildQueueSet).get(candidate_id)
-            job_class = job_classes[candidate.job_type]
-            candidate_approved = job_class.postprocessCandidate(
-                candidate, logger)
-            if candidate_approved:
-                return candidate
+        transaction.commit()
+        with DatabaseTransactionPolicy(read_only=False):
+            for (candidate_id,) in candidate_jobs:
+                candidate = getUtility(IBuildQueueSet).get(candidate_id)
+                job_class = job_classes[candidate.job_type]
+                candidate_approved = job_class.postprocessCandidate(
+                    candidate, logger)
+                if candidate_approved:
+                    transaction.commit()
+                    return candidate
+            transaction.commit()
 
         return None
 
 
=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/model/buildfarmjobbehavior.py	2011-12-30 02:24:09 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehavior.py	2012-01-03 12:26:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0211,E0213
@@ -16,6 +16,7 @@
 import socket
 import xmlrpclib
 
+import transaction
 from twisted.internet import defer
 from zope.component import getUtility
 from zope.interface import implements
@@ -30,6 +31,7 @@
     IBuildFarmJobBehavior,
     )
 from lp.services import encoding
+from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.librarian.interfaces.client import ILibrarianClient
 
@@ -69,6 +71,25 @@
         if slave_build_cookie != expected_cookie:
             raise CorruptBuildCookie("Invalid slave build cookie.")
 
+    def _getBuilderStatusHandler(self, status_text, logger):
+        """Look up the handler method for a given builder status.
+
+        If status is not a known one, logs an error and returns None.
+        """
+        builder_status_handlers = {
+            'BuilderStatus.IDLE': self.updateBuild_IDLE,
+            'BuilderStatus.BUILDING': self.updateBuild_BUILDING,
+            'BuilderStatus.ABORTING': self.updateBuild_ABORTING,
+            'BuilderStatus.ABORTED': self.updateBuild_ABORTED,
+            'BuilderStatus.WAITING': self.updateBuild_WAITING,
+            }
+        handler = builder_status_handlers.get(status_text)
+        if handler is None:
+            logger.critical(
+                "Builder on %s returned unknown status %s; failing it.",
+                self._builder.url, status_text)
+        return handler
+
     def updateBuild(self, queueItem):
         """See `IBuildFarmJobBehavior`."""
         logger = logging.getLogger('slave-scanner')
@@ -76,6 +97,7 @@
         d = self._builder.slaveStatus()
 
         def got_failure(failure):
+            transaction.abort()
             failure.trap(xmlrpclib.Fault, socket.error)
             info = failure.value
             info = ("Could not contact the builder %s, caught a (%s)"
@@ -83,27 +105,22 @@
             raise BuildSlaveFailure(info)
 
         def got_status(slave_status):
-            builder_status_handlers = {
-                'BuilderStatus.IDLE': self.updateBuild_IDLE,
-                'BuilderStatus.BUILDING': self.updateBuild_BUILDING,
-                'BuilderStatus.ABORTING': self.updateBuild_ABORTING,
-                'BuilderStatus.ABORTED': self.updateBuild_ABORTED,
-                'BuilderStatus.WAITING': self.updateBuild_WAITING,
-                }
-
             builder_status = slave_status['builder_status']
-            if builder_status not in builder_status_handlers:
-                logger.critical(
-                    "Builder on %s returned unknown status %s, failing it"
-                    % (self._builder.url, builder_status))
-                self._builder.failBuilder(
+            status_handler = self._getBuilderStatusHandler(
+                builder_status, logger)
+            if status_handler is None:
+                error = (
                     "Unknown status code (%s) returned from status() probe."
                     % builder_status)
-                # XXX: This will leave the build and job in a bad state, but
-                # should never be possible, since our builder statuses are
-                # known.
-                queueItem._builder = None
-                queueItem.setDateStarted(None)
+                transaction.commit()
+                with DatabaseTransactionPolicy(read_only=False):
+                    self._builder.failBuilder(error)
+                    # XXX: This will leave the build and job in a bad
+                    # state, but should never be possible since our
+                    # builder statuses are known.
+                    queueItem._builder = None
+                    queueItem.setDateStarted(None)
+                    transaction.commit()
                 return
 
             # Since logtail is a xmlrpclib.Binary container and it is
@@ -113,9 +130,8 @@
             # will simply remove the proxy.
             logtail = removeSecurityProxy(slave_status.get('logtail'))
 
-            method = builder_status_handlers[builder_status]
             return defer.maybeDeferred(
-                method, queueItem, slave_status, logtail, logger)
+                status_handler, queueItem, slave_status, logtail, logger)
 
         d.addErrback(got_failure)
         d.addCallback(got_status)
@@ -127,22 +143,32 @@
         Log this and reset the record.
         """
         logger.warn(
-            "Builder %s forgot about buildqueue %d -- resetting buildqueue "
-            "record" % (queueItem.builder.url, queueItem.id))
-        queueItem.reset()
+            "Builder %s forgot about buildqueue %d -- "
+            "resetting buildqueue record.",
+            queueItem.builder.url, queueItem.id)
+        transaction.commit()
+        with DatabaseTransactionPolicy(read_only=False):
+            queueItem.reset()
+            transaction.commit()
 
     def updateBuild_BUILDING(self, queueItem, slave_status, logtail, logger):
         """Build still building, collect the logtail"""
-        if queueItem.job.status != JobStatus.RUNNING:
-            queueItem.job.start()
-        queueItem.logtail = encoding.guess(str(logtail))
+        transaction.commit()
+        with DatabaseTransactionPolicy(read_only=False):
+            if queueItem.job.status != JobStatus.RUNNING:
+                queueItem.job.start()
+            queueItem.logtail = encoding.guess(str(logtail))
+            transaction.commit()
 
     def updateBuild_ABORTING(self, queueItem, slave_status, logtail, logger):
         """Build was ABORTED.
 
         Master-side should wait until the slave finish the process correctly.
         """
-        queueItem.logtail = "Waiting for slave process to be terminated"
+        transaction.commit()
+        with DatabaseTransactionPolicy(read_only=False):
+            queueItem.logtail = "Waiting for slave process to be terminated"
+            transaction.commit()
 
     def updateBuild_ABORTED(self, queueItem, slave_status, logtail, logger):
         """ABORTING process has successfully terminated.
@@ -150,11 +176,16 @@
         Clean the builder for another jobs.
         """
         d = queueItem.builder.cleanSlave()
+
         def got_cleaned(ignored):
-            queueItem.builder = None
-            if queueItem.job.status != JobStatus.FAILED:
-                queueItem.job.fail()
-            queueItem.specific_job.jobAborted()
+            transaction.commit()
+            with DatabaseTransactionPolicy(read_only=False):
+                queueItem.builder = None
+                if queueItem.job.status != JobStatus.FAILED:
+                    queueItem.job.fail()
+                queueItem.specific_job.jobAborted()
+                transaction.commit()
+
         return d.addCallback(got_cleaned)
 
     def extractBuildStatus(self, slave_status):
 
=== modified file 'lib/lp/buildmaster/model/packagebuild.py'
--- lib/lp/buildmaster/model/packagebuild.py	2011-12-30 06:14:56 +0000
+++ lib/lp/buildmaster/model/packagebuild.py	2012-01-03 12:26:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -24,6 +24,7 @@
     Storm,
     Unicode,
     )
+import transaction
 from zope.component import getUtility
 from zope.interface import (
     classProvides,
@@ -50,6 +51,7 @@
 from lp.services.config import config
 from lp.services.database.enumcol import DBEnum
 from lp.services.database.lpstorm import IMasterStore
+from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 from lp.services.helpers import filenameToContentType
 from lp.services.librarian.browser import ProxiedLibraryFileAlias
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
@@ -177,19 +179,24 @@
     def storeBuildInfo(build, librarian, slave_status):
         """See `IPackageBuild`."""
         def got_log(lfa_id):
+            dependencies = slave_status.get('dependencies')
+            if dependencies is not None:
+                dependencies = unicode(dependencies)
+
             # log, builder and date_finished are read-only, so we must
             # currently remove the security proxy to set them.
             naked_build = removeSecurityProxy(build)
-            naked_build.log = lfa_id
-            naked_build.builder = build.buildqueue_record.builder
-            # XXX cprov 20060615 bug=120584: Currently buildduration includes
-            # the scanner latency, it should really be asking the slave for
-            # the duration spent building locally.
-            naked_build.date_finished = datetime.datetime.now(pytz.UTC)
-            if slave_status.get('dependencies') is not None:
-                build.dependencies = unicode(slave_status.get('dependencies'))
-            else:
-                build.dependencies = None
+
+            transaction.commit()
+            with DatabaseTransactionPolicy(read_only=False):
+                naked_build.log = lfa_id
+                naked_build.builder = build.buildqueue_record.builder
+                # XXX cprov 20060615 bug=120584: Currently buildduration
+                # includes the scanner latency.  It should really be asking
+                # the slave for the duration spent building locally.
+                naked_build.date_finished = datetime.datetime.now(pytz.UTC)
+                build.dependencies = dependencies
+                transaction.commit()
 
         d = build.getLogFromSlave(build)
         return d.addCallback(got_log)
@@ -290,22 +297,41 @@
 
     def handleStatus(self, status, librarian, slave_status):
         """See `IPackageBuild`."""
+        # Avoid circular imports.
         from lp.buildmaster.manager import BUILDD_MANAGER_LOG_NAME
+
         logger = logging.getLogger(BUILDD_MANAGER_LOG_NAME)
         send_notification = status in self.ALLOWED_STATUS_NOTIFICATIONS
         method = getattr(self, '_handleStatus_' + status, None)
         if method is None:
-            logger.critical("Unknown BuildStatus '%s' for builder '%s'"
-                            % (status, self.buildqueue_record.builder.url))
-            return
+            logger.critical(
+                "Unknown BuildStatus '%s' for builder '%s'",
+                status, self.buildqueue_record.builder.url)
+            return None
+
         d = method(librarian, slave_status, logger, send_notification)
         return d
 
+    def _destroy_buildqueue_record(self, unused_arg):
+        """Destroy this build's `BuildQueue` record."""
+        transaction.commit()
+        with DatabaseTransactionPolicy(read_only=False):
+            self.buildqueue_record.destroySelf()
+            transaction.commit()
+
     def _release_builder_and_remove_queue_item(self):
         # Release the builder for another job.
         d = self.buildqueue_record.builder.cleanSlave()
         # Remove BuildQueue record.
-        return d.addCallback(lambda x: self.buildqueue_record.destroySelf())
+        return d.addCallback(self._destroy_buildqueue_record)
+
+    def _notify_if_appropriate(self, appropriate=True, extra_info=None):
+        """If `appropriate`, call `self.notify` in a write transaction."""
+        if appropriate:
+            transaction.commit()
+            with DatabaseTransactionPolicy(read_only=False):
+                self.notify(extra_info=extra_info)
+                transaction.commit()
 
     def _handleStatus_OK(self, librarian, slave_status, logger,
                          send_notification):
@@ -321,16 +347,19 @@
             self.buildqueue_record.specific_job.build.title,
             self.buildqueue_record.builder.name))
 
-        # If this is a binary package build, discard it if its source is
-        # no longer published.
+        # If this is a binary package build for a source that is no
+        # longer published, discard it.
         if self.build_farm_job_type == BuildFarmJobType.PACKAGEBUILD:
             build = self.buildqueue_record.specific_job.build
             if not build.current_source_publication:
-                build.status = BuildStatus.SUPERSEDED
+                transaction.commit()
+                with DatabaseTransactionPolicy(read_only=False):
+                    build.status = BuildStatus.SUPERSEDED
+                    transaction.commit()
                 return self._release_builder_and_remove_queue_item()
 
-        # Explode before collect a binary that is denied in this
-        # distroseries/pocket
+        # Explode rather than collect a binary that is denied in this
+        # distroseries/pocket.
         if not self.archive.allowUpdatesToReleasePocket():
             assert self.distro_series.canUploadToPocket(self.pocket), (
                 "%s (%s) can not be built for pocket %s: illegal status"
@@ -375,18 +404,26 @@
             # files from the slave.
             if successful_copy_from_slave:
                 logger.info(
-                    "Gathered %s %d completely. Moving %s to uploader queue."
-                    % (self.__class__.__name__, self.id, upload_leaf))
+                    "Gathered %s %d completely. "
+                    "Moving %s to uploader queue.",
+                    self.__class__.__name__, self.id, upload_leaf)
                 target_dir = os.path.join(root, "incoming")
-                self.status = BuildStatus.UPLOADING
+                resulting_status = BuildStatus.UPLOADING
             else:
                 logger.warning(
-                    "Copy from slave for build %s was unsuccessful.", self.id)
-                self.status = BuildStatus.FAILEDTOUPLOAD
-                if send_notification:
-                    self.notify(
-                        extra_info='Copy from slave was unsuccessful.')
+                    "Copy from slave for build %s was unsuccessful.",
+                    self.id)
                 target_dir = os.path.join(root, "failed")
+                resulting_status = BuildStatus.FAILEDTOUPLOAD
+
+            transaction.commit()
+            with DatabaseTransactionPolicy(read_only=False):
+                self.status = resulting_status
+                transaction.commit()
+
+            if not successful_copy_from_slave:
+                self._notify_if_appropriate(
+                    send_notification, "Copy from slave was unsuccessful.")
 
             if not os.path.exists(target_dir):
                 os.mkdir(target_dir)
@@ -394,10 +431,6 @@
             # Release the builder for another job.
             d = self._release_builder_and_remove_queue_item()
 
-            # Commit so there are no race conditions with archiveuploader
-            # about self.status.
-            Store.of(self).commit()
-
             # Move the directory used to grab the binaries into
             # the incoming directory so the upload processor never
             # sees half-finished uploads.
@@ -421,14 +454,15 @@
         set the job status as FAILEDTOBUILD, store available info and
         remove Buildqueue entry.
         """
-        self.status = BuildStatus.FAILEDTOBUILD
+        transaction.commit()
+        with DatabaseTransactionPolicy(read_only=False):
+            self.status = BuildStatus.FAILEDTOBUILD
+            transaction.commit()
 
         def build_info_stored(ignored):
-            if send_notification:
-                self.notify()
+            self._notify_if_appropriate(send_notification)
             d = self.buildqueue_record.builder.cleanSlave()
-            return d.addCallback(
-                lambda x: self.buildqueue_record.destroySelf())
+            return d.addCallback(self._destroy_buildqueue_record)
 
         d = self.storeBuildInfo(self, librarian, slave_status)
         return d.addCallback(build_info_stored)
@@ -441,16 +475,16 @@
         MANUALDEPWAIT, store available information, remove BuildQueue
         entry and release builder slave for another job.
         """
-        self.status = BuildStatus.MANUALDEPWAIT
+        with DatabaseTransactionPolicy(read_only=False):
+            self.status = BuildStatus.MANUALDEPWAIT
+            transaction.commit()
 
         def build_info_stored(ignored):
             logger.critical("***** %s is MANUALDEPWAIT *****"
                             % self.buildqueue_record.builder.name)
-            if send_notification:
-                self.notify()
+            self._notify_if_appropriate(send_notification)
             d = self.buildqueue_record.builder.cleanSlave()
-            return d.addCallback(
-                lambda x: self.buildqueue_record.destroySelf())
+            return d.addCallback(self._destroy_buildqueue_record)
 
         d = self.storeBuildInfo(self, librarian, slave_status)
         return d.addCallback(build_info_stored)
@@ -463,20 +497,29 @@
         job as CHROOTFAIL, store available information, remove BuildQueue
         and release the builder.
         """
-        self.status = BuildStatus.CHROOTWAIT
+        with DatabaseTransactionPolicy(read_only=False):
+            self.status = BuildStatus.CHROOTWAIT
+            transaction.commit()
 
         def build_info_stored(ignored):
-            logger.critical("***** %s is CHROOTWAIT *****" %
-                            self.buildqueue_record.builder.name)
-            if send_notification:
-                self.notify()
+            logger.critical(
+                "***** %s is CHROOTWAIT *****",
+                self.buildqueue_record.builder.name)
+
+            self._notify_if_appropriate(send_notification)
             d = self.buildqueue_record.builder.cleanSlave()
-            return d.addCallback(
-                lambda x: self.buildqueue_record.destroySelf())
+            return d.addCallback(self._destroy_buildqueue_record)
 
         d = self.storeBuildInfo(self, librarian, slave_status)
         return d.addCallback(build_info_stored)
 
+    def _reset_buildqueue_record(self, ignored_arg=None):
+        """Reset the `BuildQueue` record, in a write transaction."""
+        transaction.commit()
+        with DatabaseTransactionPolicy(read_only=False):
+            self.buildqueue_record.reset()
+            transaction.commit()
+
     def _handleStatus_BUILDERFAIL(self, librarian, slave_status, logger,
                                   send_notification):
         """Handle builder failures.
@@ -490,11 +533,8 @@
         self.buildqueue_record.builder.failBuilder(
             "Builder returned BUILDERFAIL when asked for its status")
 
-        def build_info_stored(ignored):
-            # simply reset job
-            self.buildqueue_record.reset()
         d = self.storeBuildInfo(self, librarian, slave_status)
-        return d.addCallback(build_info_stored)
+        return d.addCallback(self._reset_buildqueue_record)
 
     def _handleStatus_GIVENBACK(self, librarian, slave_status, logger,
                                 send_notification):
@@ -514,7 +554,7 @@
             # the next Paris Summit, infinity has some ideas about how
             # to use this content. For now we just ensure it's stored.
             d = self.buildqueue_record.builder.cleanSlave()
-            self.buildqueue_record.reset()
+            self._reset_buildqueue_record()
             return d
 
         d = self.storeBuildInfo(self, librarian, slave_status)
 
=== added file 'lib/lp/buildmaster/testing.py'
--- lib/lp/buildmaster/testing.py	1970-01-01 00:00:00 +0000
+++ lib/lp/buildmaster/testing.py	2012-01-03 12:26:30 +0000
@@ -0,0 +1,59 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Testing helpers for buildmaster code."""
+
+__metaclass__ = type
+__all__ = [
+    "BuilddManagerTestFixture",
+    ]
+
+from contextlib import contextmanager
+
+import fixtures
+import transaction
+
+from lp.services.database.transaction_policy import DatabaseTransactionPolicy
+
+
+class BuilddManagerTestFixture(fixtures.Fixture):
+    """Helps provide an environment more like `BuilddManager` provides.
+
+    This mimics the default transaction access policy of `BuilddManager`,
+    though it can be configured with a different policy by passing in `store`
+    and/or `read_only`. See `BuilddManager.enterReadOnlyDatabasePolicy`.
+
+    Because this will shift into a read-only database transaction access mode,
+    individual tests that need to do more setup can use the `extraSetUp()`
+    context manager to temporarily shift back to a read-write mode.
+    """
+
+    def __init__(self, store=None, read_only=True):
+        super(BuilddManagerTestFixture, self).__init__()
+        self.policy = DatabaseTransactionPolicy(
+            store=store, read_only=read_only)
+
+    def setUp(self):
+        # Commit everything done so far then shift into a read-only
+        # transaction access mode by default.
+        super(BuilddManagerTestFixture, self).setUp()
+        transaction.commit()
+        self.policy.__enter__()
+        self.addCleanup(self.policy.__exit__, None, None, None)
+
+    @staticmethod
+    @contextmanager
+    def extraSetUp():
+        """Temporarily enter a read-write transaction to do extra setup.
+
+        For example:
+
+          with self.extraSetUp():
+              removeSecurityProxy(self.build).date_finished = None
+
+        On exit it will commit the changes and restore the previous
+        transaction access mode.
+        """
+        with DatabaseTransactionPolicy(read_only=False):
+            yield
+            transaction.commit()
 
=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py	2011-12-30 06:14:56 +0000
+++ lib/lp/buildmaster/tests/test_builder.py	2012-01-03 12:26:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test Builder features."""
@@ -15,6 +15,7 @@
     AsynchronousDeferredRunTestForBrokenTwisted,
     SynchronousDeferredRunTest,
     )
+import transaction
 from twisted.internet.defer import (
     CancelledError,
     DeferredList,
@@ -60,8 +61,10 @@
     TrivialBehavior,
     WaitingSlave,
     )
+from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.config import config
 from lp.services.database.sqlbase import flush_database_updates
+from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.log.logger import BufferLogger
 from lp.services.webapp.interfaces import (
@@ -152,7 +155,7 @@
         d = lostbuilding_builder.updateStatus(BufferLogger())
         def check_slave_status(failure):
             self.assertIn('abort', slave.call_log)
-            # 'Fault' comes from the LostBuildingBrokenSlave, this is
+            # 'Fault' comes from the LostBuildingBrokenSlave.  This is
             # just testing that the value is passed through.
             self.assertIsInstance(failure.value, xmlrpclib.Fault)
         return d.addBoth(check_slave_status)
@@ -531,6 +534,26 @@
         # And the old_candidate is superseded:
         self.assertEqual(BuildStatus.SUPERSEDED, build.status)
 
+    def test_findBuildCandidate_postprocesses_in_read_write_policy(self):
+        # _findBuildCandidate invokes BuildFarmJob.postprocessCandidate,
+        # which may modify the database.  This happens in a read-write
+        # transaction even if _findBuildCandidate itself runs in a
+        # read-only transaction policy.
+
+        # PackageBuildJob.postprocessCandidate will attempt to delete
+        # security builds.
+        pub = self.publisher.getPubSource(
+            sourcename="gedit", status=PackagePublishingStatus.PUBLISHED,
+            archive=self.factory.makeArchive(),
+            pocket=PackagePublishingPocket.SECURITY)
+        pub.createMissingBuilds()
+        transaction.commit()
+        with DatabaseTransactionPolicy(read_only=True):
+            removeSecurityProxy(self.frog_builder)._findBuildCandidate()
+            # The test is that this passes without a "transaction is
+            # read-only" error.
+            transaction.commit()
+
     def test_acquireBuildCandidate_marks_building(self):
         # acquireBuildCandidate() should call _findBuildCandidate and
         # mark the build as building.
 
=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
--- lib/lp/buildmaster/tests/test_manager.py	2012-01-01 02:58:52 +0000
+++ lib/lp/buildmaster/tests/test_manager.py	2012-01-03 12:26:30 +0000
@@ -1,8 +1,9 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the renovated slave scanner aka BuilddManager."""
 
+from collections import namedtuple
 import os
 import signal
 import time
@@ -34,15 +35,18 @@
     SlaveScanner,
     )
 from lp.buildmaster.model.builder import Builder
+from lp.buildmaster.model.packagebuild import PackageBuild
 from lp.buildmaster.tests.harness import BuilddManagerTestSetup
 from lp.buildmaster.tests.mock_slaves import (
     BrokenSlave,
     BuildingSlave,
     make_publisher,
     OkSlave,
+    WaitingSlave,
     )
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.services.config import config
+from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 from lp.services.log.logger import BufferLogger
 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
 from lp.testing import (
@@ -58,7 +62,10 @@
     LaunchpadZopelessLayer,
     ZopelessDatabaseLayer,
     )
-from lp.testing.sampledata import BOB_THE_BUILDER_NAME
+from lp.testing.sampledata import (
+    BOB_THE_BUILDER_NAME,
+    FROG_THE_BUILDER_NAME,
+    )
 
 
 class TestSlaveScannerScan(TestCase):
@@ -76,6 +83,8 @@
         'bob' builder.
         """
         super(TestSlaveScannerScan, self).setUp()
+        self.read_only = DatabaseTransactionPolicy(read_only=True)
+
         # Creating the required chroots needed for dispatching.
         test_publisher = make_publisher()
         ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
@@ -83,6 +92,15 @@
         test_publisher.setUpDefaultDistroSeries(hoary)
         test_publisher.addFakeChroots()
 
+    def _enterReadOnly(self):
+        """Go into read-only transaction policy."""
+        self.read_only.__enter__()
+        self.addCleanup(self._exitReadOnly)
+
+    def _exitReadOnly(self):
+        """Leave read-only transaction policy."""
+        self.read_only.__exit__(None, None, None)
+
     def _resetBuilder(self, builder):
         """Reset the given builder and its job."""
 
@@ -93,6 +111,23 @@
 
         transaction.commit()
 
+    def getFreshBuilder(self, slave=None, name=BOB_THE_BUILDER_NAME,
+                        failure_count=0):
+        """Return a builder.
+
+        The builder is taken from sample data, but reset to a usable state.
+        Be careful: this is not a proper factory method.  Identical calls
+        return (and reset) the same builder.  Don't rely on that though;
+        maybe someday we'll have a proper factory here.
+        """
+        if slave is None:
+            slave = OkSlave()
+        builder = getUtility(IBuilderSet)[name]
+        self._resetBuilder(builder)
+        builder.setSlaveForTesting(slave)
+        builder.failure_count = failure_count
+        return builder
+
     def assertBuildingJob(self, job, builder, logtail=None):
         """Assert the given job is building on the given builder."""
         from lp.services.job.interfaces.job import JobStatus
@@ -107,14 +142,14 @@
         self.assertEqual(build.status, BuildStatus.BUILDING)
         self.assertEqual(job.logtail, logtail)
 
-    def _getScanner(self, builder_name=None):
+    def _getScanner(self, builder_name=None, clock=None):
         """Instantiate a SlaveScanner object.
 
         Replace its default logging handler by a testing version.
         """
         if builder_name is None:
             builder_name = BOB_THE_BUILDER_NAME
-        scanner = SlaveScanner(builder_name, BufferLogger())
+        scanner = SlaveScanner(builder_name, BufferLogger(), clock=clock)
         scanner.logger.name = 'slave-scanner'
 
         return scanner
@@ -130,17 +165,15 @@
     def testScanDispatchForResetBuilder(self):
         # A job gets dispatched to the sampledata builder after it's reset.
 
-        # Reset sampledata builder.
-        builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
-        self._resetBuilder(builder)
-        builder.setSlaveForTesting(OkSlave())
-        # Set this to 1 here so that _checkDispatch can make sure it's
-        # reset to 0 after a successful dispatch.
-        builder.failure_count = 1
+        # Obtain a builder.   Initialize failure count to 1 so that
+        # _checkDispatch can make sure that a successful dispatch resets
+        # the count to 0.
+        builder = self.getFreshBuilder(failure_count=1)
 
         # Run 'scan' and check its result.
         self.layer.txn.commit()
         self.layer.switchDbUser(config.builddmaster.dbuser)
+        self._enterReadOnly()
         scanner = self._getScanner()
         d = defer.maybeDeferred(scanner.scan)
         d.addCallback(self._checkDispatch, builder)
@@ -153,20 +186,18 @@
         to the asynchonous dispatcher and the builder remained active
         and IDLE.
         """
-        self.assertTrue(slave is None, "Unexpected slave.")
+        self.assertIs(None, slave, "Unexpected slave.")
 
         builder = getUtility(IBuilderSet).get(builder.id)
         self.assertTrue(builder.builderok)
-        self.assertTrue(builder.currentjob is None)
+        self.assertIs(None, builder.currentjob)
 
     def testNoDispatchForMissingChroots(self):
         # When a required chroot is not present the `scan` method
         # should not return any `RecordingSlaves` to be processed
         # and the builder used should remain active and IDLE.
 
-        # Reset sampledata builder.
-        builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
-        self._resetBuilder(builder)
+        builder = self.getFreshBuilder()
 
         # Remove hoary/i386 chroot.
         login('[e-mail]')
@@ -179,6 +210,7 @@
 
         # Run 'scan' and check its result.
         self.layer.switchDbUser(config.builddmaster.dbuser)
+        self._enterReadOnly()
         scanner = self._getScanner()
         d = defer.maybeDeferred(scanner.singleCycle)
         d.addCallback(self._checkNoDispatch, builder)
@@ -220,6 +252,7 @@
 
         # Run 'scan' and check its result.
         self.layer.switchDbUser(config.builddmaster.dbuser)
+        self._enterReadOnly()
         scanner = self._getScanner()
         d = defer.maybeDeferred(scanner.scan)
         d.addCallback(self._checkJobRescued, builder, job)
@@ -255,25 +288,27 @@
 
         # Run 'scan' and check its result.
         self.layer.switchDbUser(config.builddmaster.dbuser)
+        self._enterReadOnly()
         scanner = self._getScanner()
         d = defer.maybeDeferred(scanner.scan)
         d.addCallback(self._checkJobUpdated, builder, job)
         return d
 
     def test_scan_with_nothing_to_dispatch(self):
-        factory = LaunchpadObjectFactory()
-        builder = factory.makeBuilder()
+        builder = self.factory.makeBuilder()
         builder.setSlaveForTesting(OkSlave())
+        transaction.commit()
+        self._enterReadOnly()
         scanner = self._getScanner(builder_name=builder.name)
         d = scanner.scan()
         return d.addCallback(self._checkNoDispatch, builder)
 
     def test_scan_with_manual_builder(self):
         # Reset sampledata builder.
-        builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
-        self._resetBuilder(builder)
-        builder.setSlaveForTesting(OkSlave())
+        builder = self.getFreshBuilder()
         builder.manual = True
+        transaction.commit()
+        self._enterReadOnly()
         scanner = self._getScanner()
         d = scanner.scan()
         d.addCallback(self._checkNoDispatch, builder)
@@ -281,10 +316,10 @@
 
     def test_scan_with_not_ok_builder(self):
         # Reset sampledata builder.
-        builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
-        self._resetBuilder(builder)
-        builder.setSlaveForTesting(OkSlave())
+        builder = self.getFreshBuilder()
         builder.builderok = False
+        transaction.commit()
+        self._enterReadOnly()
         scanner = self._getScanner()
         d = scanner.scan()
         # Because the builder is not ok, we can't use _checkNoDispatch.
@@ -293,25 +328,27 @@
         return d
 
     def test_scan_of_broken_slave(self):
-        builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
-        self._resetBuilder(builder)
-        builder.setSlaveForTesting(BrokenSlave())
-        builder.failure_count = 0
+        builder = self.getFreshBuilder(slave=BrokenSlave())
+        transaction.commit()
+        self._enterReadOnly()
         scanner = self._getScanner(builder_name=builder.name)
         d = scanner.scan()
         return assert_fails_with(d, xmlrpclib.Fault)
 
     def _assertFailureCounting(self, builder_count, job_count,
                                expected_builder_count, expected_job_count):
+        # Avoid circular imports.
+        from lp.buildmaster import manager as manager_module
+
         # If scan() fails with an exception, failure_counts should be
         # incremented.  What we do with the results of the failure
         # counts is tested below separately, this test just makes sure that
         # scan() is setting the counts.
         def failing_scan():
             return defer.fail(Exception("fake exception"))
+
         scanner = self._getScanner()
         scanner.scan = failing_scan
-        from lp.buildmaster import manager as manager_module
         self.patch(manager_module, 'assessFailureCounts', FakeMethod())
         builder = getUtility(IBuilderSet)[scanner.builder_name]
 
@@ -459,6 +496,60 @@
         d.addCallback(check_cancelled, builder, buildqueue)
         return d
 
+    def makeFakeFailure(self):
+        """Produce a fake failure for use with SlaveScanner._scanFailed."""
+        FakeFailure = namedtuple('FakeFailure', ['getErrorMessage', 'check'])
+        return FakeFailure(
+            FakeMethod(self.factory.getUniqueString()),
+            FakeMethod(True))
+
+    def test_interleaved_success_and_failure_do_not_interfere(self):
+        # It's possible for one builder to fail while another continues
+        # to function properly.  When that happens, the failed builder
+        # may cause database changes to be rolled back.  But that does
+        # not affect the functioning builder.
+        clock = task.Clock()
+
+        broken_builder = self.getFreshBuilder(
+            slave=BrokenSlave(), name=BOB_THE_BUILDER_NAME)
+        broken_scanner = self._getScanner(builder_name=broken_builder.name)
+        good_builder = self.getFreshBuilder(
+            slave=WaitingSlave(), name=FROG_THE_BUILDER_NAME)
+        good_build = self.factory.makeBinaryPackageBuild(
+            distroarchseries=self.factory.makeDistroArchSeries())
+
+        # The good build is being handled by the good builder.
+        buildqueue = good_build.queueBuild()
+        buildqueue.builder = good_builder
+
+        removeSecurityProxy(good_build.build_farm_job).date_started = UTC_NOW
+
+        # The good builder requests information from a successful build,
+        # and up receiving it, updates the build's metadata.
+        # Our dependencies string goes into the build, and its
+        # date_finished will be set.
+        dependencies = self.factory.getUniqueString()
+        PackageBuild.storeBuildInfo(
+            good_build, None, {'dependencies': dependencies})
+        clock.advance(1)
+
+        # The broken scanner experiences a failure before the good
+        # scanner is receiving its data.  This aborts the ongoing
+        # transaction.
+        # As a somewhat weird example, if the builder changed its own
+        # title, that change will be rolled back.
+        original_broken_builder_title = broken_builder.title
+        broken_builder.title = self.factory.getUniqueString()
+        broken_scanner._scanFailed(self.makeFakeFailure())
+
+        # The work done by the good scanner is retained.  The
+        # storeBuildInfo code committed it.
+        self.assertEqual(dependencies, good_build.dependencies)
+        self.assertIsNot(None, good_build.date_finished)
+
+        # The work done by the broken scanner is rolled back.
+        self.assertEqual(original_broken_builder_title, broken_builder.title)
+
 
 class TestCancellationChecking(TestCaseWithFactory):
     """Unit tests for the checkCancellation method."""
 
=== modified file 'lib/lp/buildmaster/tests/test_packagebuild.py'
--- lib/lp/buildmaster/tests/test_packagebuild.py	2011-12-30 06:14:56 +0000
+++ lib/lp/buildmaster/tests/test_packagebuild.py	2012-01-03 12:26:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `IPackageBuild`."""
@@ -12,6 +12,7 @@
 import tempfile
 
 from storm.store import Store
+from testtools.deferredruntest import AsynchronousDeferredRunTest
 from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
@@ -26,8 +27,10 @@
     IPackageBuildSet,
     IPackageBuildSource,
     )
+from lp.buildmaster.model.builder import BuilderSlave
 from lp.buildmaster.model.buildfarmjob import BuildFarmJob
 from lp.buildmaster.model.packagebuild import PackageBuild
+from lp.buildmaster.testing import BuilddManagerTestFixture
 from lp.buildmaster.tests.mock_slaves import WaitingSlave
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.config import config
@@ -281,12 +284,10 @@
 
 
 class TestHandleStatusMixin:
-    """Tests for `IPackageBuild`s handleStatus method.
-
-    This should be run with a Trial TestCase.
-    """
+    """Tests for `IPackageBuild`s handleStatus method."""
 
     layer = LaunchpadZopelessLayer
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=20)
 
     def makeBuild(self):
         """Allow classes to override the build with which the test runs."""
@@ -303,7 +304,7 @@
         self.build.buildqueue_record.setDateStarted(UTC_NOW)
         self.slave = WaitingSlave('BuildStatus.OK')
         self.slave.valid_file_hashes.append('test_file_hash')
-        builder.setSlaveForTesting(self.slave)
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(self.slave))
 
         # We overwrite the buildmaster root to use a temp directory.
         tempdir = tempfile.mkdtemp()
@@ -321,6 +322,8 @@
         removeSecurityProxy(self.build).verifySuccessfulUpload = FakeMethod(
             result=True)
 
+        self.useFixture(BuilddManagerTestFixture())
+
     def assertResultCount(self, count, result):
         self.assertEquals(
             1, len(os.listdir(os.path.join(self.upload_root, result))))
@@ -344,7 +347,7 @@
         def got_status(ignored):
             self.assertEqual(BuildStatus.FAILEDTOUPLOAD, self.build.status)
             self.assertResultCount(0, "failed")
-            self.assertIdentical(None, self.build.buildqueue_record)
+            self.assertIs(None, self.build.buildqueue_record)
 
         d = self.build.handleStatus('OK', None, {
             'filemap': {'/tmp/myfile.py': 'test_file_hash'},
@@ -365,7 +368,8 @@
 
     def test_handleStatus_OK_sets_build_log(self):
         # The build log is set during handleStatus.
-        removeSecurityProxy(self.build).log = None
+        with BuilddManagerTestFixture.extraSetUp():
+            removeSecurityProxy(self.build).log = None
         self.assertEqual(None, self.build.log)
         d = self.build.handleStatus('OK', None, {
                 'filemap': {'myfile.py': 'test_file_hash'},
@@ -386,14 +390,10 @@
 
         def got_status(ignored):
             if expected_notification:
-                self.failIf(
-                    len(pop_notifications()) == 0,
-                    "No notifications received")
+                self.assertNotEqual(
+                    0, len(pop_notifications()), "No notifications received.")
             else:
-                self.failIf(
-                    len(pop_notifications()) > 0,
-                    "Notifications received")
-
+                self.assertContentEqual([], pop_notifications())
         d = self.build.handleStatus(status, None, {})
         return d.addCallback(got_status)
 
@@ -408,7 +408,9 @@
 
     def test_date_finished_set(self):
         # The date finished is updated during handleStatus_OK.
-        removeSecurityProxy(self.build).date_finished = None
+        with BuilddManagerTestFixture.extraSetUp():
+            removeSecurityProxy(self.build).date_finished = None
+
         self.assertEqual(None, self.build.date_finished)
         d = self.build.handleStatus('OK', None, {
                 'filemap': {'myfile.py': 'test_file_hash'},
 
=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2012-01-03 12:26:30 +0000
@@ -13,14 +13,15 @@
 
 from pytz import utc
 from storm.locals import Store
+from testtools.deferredruntest import AsynchronousDeferredRunTest
 import transaction
-from twisted.trial.unittest import TestCase as TrialTestCase
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.errors import NotFoundError
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
+from lp.buildmaster.model.builder import BuilderSlave
 from lp.buildmaster.model.buildfarmjob import BuildFarmJob
 from lp.buildmaster.model.packagebuild import PackageBuild
 from lp.buildmaster.tests.mock_slaves import WaitingSlave
@@ -588,14 +589,11 @@
         self.assertEquals(0, len(notifications))
 
 
-class TestBuildNotifications(TrialTestCase):
+class TestBuildNotifications(TestCaseWithFactory):
 
     layer = LaunchpadZopelessLayer
 
-    def setUp(self):
-        super(TestBuildNotifications, self).setUp()
-        from lp.testing.factory import LaunchpadObjectFactory
-        self.factory = LaunchpadObjectFactory()
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=20)
 
     def prepare_build(self, fake_successful_upload=False):
         queue_record = self.factory.makeSourcePackageRecipeBuildJob()
@@ -608,7 +606,7 @@
                 result=True)
         queue_record.builder = self.factory.makeBuilder()
         slave = WaitingSlave('BuildStatus.OK')
-        queue_record.builder.setSlaveForTesting(slave)
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(slave))
         return build
 
     def assertDeferredNotifyCount(self, status, build, expected_count):
@@ -666,5 +664,5 @@
 
 
 class TestHandleStatusForSPRBuild(
-    MakeSPRecipeBuildMixin, TestHandleStatusMixin, TrialTestCase):
+    MakeSPRecipeBuildMixin, TestHandleStatusMixin, TestCaseWithFactory):
     """IPackageBuild.handleStatus works with SPRecipe builds."""
 
=== modified file 'lib/lp/services/database/tests/test_transaction_policy.py'
--- lib/lp/services/database/tests/test_transaction_policy.py	2012-01-01 02:58:52 +0000
+++ lib/lp/services/database/tests/test_transaction_policy.py	2012-01-03 12:26:30 +0000
@@ -76,16 +76,26 @@
 
         self.assertRaises(InternalError, make_forbidden_update)
 
-    def test_will_not_start_in_ongoing_transaction(self):
-        # You cannot enter a DatabaseTransactionPolicy while already in
-        # a transaction.
+    def test_will_not_go_read_only_when_read_write_transaction_ongoing(self):
+        # You cannot enter a read-only DatabaseTransactionPolicy while in an
+        # active read-write transaction.
         def enter_policy():
-            with DatabaseTransactionPolicy():
+            with DatabaseTransactionPolicy(read_only=True):
                 pass
 
         self.writeToDatabase()
         self.assertRaises(TransactionInProgress, enter_policy)
 
+    def test_will_go_read_write_when_read_write_transaction_ongoing(self):
+        # You can enter a read-write DatabaseTransactionPolicy while in an
+        # active read-write transaction.
+        def enter_policy():
+            with DatabaseTransactionPolicy(read_only=False):
+                pass
+
+        self.writeToDatabase()
+        enter_policy()  # No exception.
+
     def test_successful_exit_requires_commit_or_abort(self):
         # If a read-write policy exits normally (which would probably
         # indicate successful completion of its code), it requires that
 
=== modified file 'lib/lp/services/database/transaction_policy.py'
--- lib/lp/services/database/transaction_policy.py	2011-12-30 06:14:56 +0000
+++ lib/lp/services/database/transaction_policy.py	2012-01-03 12:26:30 +0000
@@ -92,9 +92,18 @@
 
         :raise TransactionInProgress: if a transaction was already ongoing.
         """
-        self._checkNoTransaction(
-            "Entered DatabaseTransactionPolicy while in a transaction.")
+        # We must check the transaction status before checking the current
+        # policy because getting the policy causes a status change.
+        in_transaction = self._isInTransaction()
         self.previous_policy = self._getCurrentPolicy()
+        # If the current transaction is read-write and we're moving to
+        # read-only then we should check for a transaction. If the current
+        # policy is read-only then we don't care if a transaction is in
+        # progress.
+        if in_transaction and self.read_only and not self.previous_policy:
+            raise TransactionInProgress(
+                "Attempting to enter a read-only transaction while holding "
+                "open a read-write transaction.")
         self._setPolicy(self.read_only)
         # Commit should include the policy itself.  If this breaks
         # because the transaction was already in a failed state before
@@ -133,8 +142,11 @@
     def _isInTransaction(self):
         """Is our store currently in a transaction?"""
         pg_connection = self.store._connection._raw_connection
-        status = pg_connection.get_transaction_status()
-        return status != TRANSACTION_STATUS_IDLE
+        if pg_connection is None:
+            return False
+        else:
+            status = pg_connection.get_transaction_status()
+            return status != TRANSACTION_STATUS_IDLE
 
     def _checkNoTransaction(self, error_msg):
         """Verify that no transaction is ongoing.
 
=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuild.py'
--- lib/lp/soyuz/tests/test_binarypackagebuild.py	2012-01-01 02:58:52 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuild.py	2012-01-03 12:26:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test Build features."""
@@ -10,7 +10,6 @@
 
 import pytz
 from storm.store import Store
-from twisted.trial.unittest import TestCase as TrialTestCase
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -18,6 +17,7 @@
 from lp.buildmaster.interfaces.builder import IBuilderSet
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
 from lp.buildmaster.interfaces.packagebuild import IPackageBuild
+from lp.buildmaster.model.builder import BuilderSlave
 from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.buildmaster.tests.mock_slaves import WaitingSlave
 from lp.buildmaster.tests.test_packagebuild import (
@@ -48,6 +48,7 @@
     logout,
     TestCaseWithFactory,
     )
+from lp.testing.fakemethod import FakeMethod
 from lp.testing.layers import (
     DatabaseFunctionalLayer,
     LaunchpadZopelessLayer,
@@ -522,7 +523,9 @@
         self.build = gedit_src_hist.createMissingBuilds()[0]
 
         self.builder = self.factory.makeBuilder()
-        self.builder.setSlaveForTesting(WaitingSlave('BuildStatus.OK'))
+        self.patch(
+            BuilderSlave, 'makeBuilderSlave',
+            FakeMethod(WaitingSlave('BuildStatus.OK')))
         self.build.buildqueue_record.markAsBuilding(self.builder)
 
     def testDependencies(self):
@@ -568,7 +571,7 @@
 
 
 class TestHandleStatusForBinaryPackageBuild(
-    MakeBinaryPackageBuildMixin, TestHandleStatusMixin, TrialTestCase):
+    MakeBinaryPackageBuildMixin, TestHandleStatusMixin, TestCaseWithFactory):
     """IPackageBuild.handleStatus works with binary builds."""
 
 
 
=== modified file 'lib/lp/translations/model/translationtemplatesbuildbehavior.py'
--- lib/lp/translations/model/translationtemplatesbuildbehavior.py	2012-01-01 02:58:52 +0000
+++ lib/lp/translations/model/translationtemplatesbuildbehavior.py	2012-01-03 12:26:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """An `IBuildFarmJobBehavior` for `TranslationTemplatesBuildJob`.
@@ -16,6 +16,7 @@
 import tempfile
 
 import pytz
+import transaction
 from twisted.internet import defer
 from zope.component import getUtility
 from zope.interface import implements
@@ -28,6 +29,7 @@
     )
 from lp.buildmaster.model.buildfarmjobbehavior import BuildFarmJobBehaviorBase
 from lp.registry.interfaces.productseries import IProductSeriesSet
+from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 from lp.translations.interfaces.translationimportqueue import (
     ITranslationImportQueue,
     )
@@ -132,13 +134,16 @@
     def storeBuildInfo(build, queue_item, build_status):
         """See `IPackageBuild`."""
         def got_log(lfa_id):
-            build.build.log = lfa_id
-            build.build.builder = queue_item.builder
-            build.build.date_started = queue_item.date_started
-            # XXX cprov 20060615 bug=120584: Currently buildduration includes
-            # the scanner latency, it should really be asking the slave for
-            # the duration spent building locally.
-            build.build.date_finished = datetime.datetime.now(pytz.UTC)
+            transaction.commit()
+            with DatabaseTransactionPolicy(read_only=False):
+                build.build.log = lfa_id
+                build.build.builder = queue_item.builder
+                build.build.date_started = queue_item.date_started
+                # XXX cprov 20060615 bug=120584: Currently buildduration
+                # includes the scanner latency.  It should really be
+                # asking the slave for the duration spent building locally.
+                build.build.date_finished = datetime.datetime.now(pytz.UTC)
+                transaction.commit()
 
         d = build.getLogFromSlave(build, queue_item)
         return d.addCallback(got_log)