Merge lp:~jtv/launchpad/bug-717969 into lp:launchpad

[Jeroen T. Vermeulen]

Read-only transactions by default in buildmaster.

= Bug 717969 =

This is an oversized branch. Don't just pick it up out of a sense of duty; if it's too big or you're not certain about the domain-specific stuff, contact one of the Soyuz experts — Julian, Steve, or William.

The buildmaster was showing very strange symptoms. In a nutshell, cases where observed where it went through steps A, B, and C; we could see that steps A and C were completed successfully; but the expected results from step B were just not there.

The cause turns out to be a dangerous mix of global state: our database transaction management is global to a thread (all of the thread's changes are committed or all are aborted) but our use of twisted makes a single python thread/process flit back and forth between servicing multiple logical threads of execution.

Each of those threads of execution might, from its point of view: make some changes to ORM-backed objects, then initiate an asynchronous request (talking to the librarian or a build slave), block for the result to arrive, make some more ORM changes, block again, and later commit or abort the database transaction.

That is purely from the point of view of a logical thread of execution. Twisted experts are quick to point out that “nothing blocks” in Twisted, but that is exactly the problem: while one logical thread of execution “blocks,” another gets to execute. And who's to say whether that other one might not commit or abort the first one's changes prematurely? With the numbers of builders we drive, it's bound to happen regularly.

We discussed a few architectural solutions: farming off the ORM changes to a worker thread, doing all the work in threads, and so on. The real problem is that the ORM changes are inlined in the buildmaster, but largely hidden away in the various build-related class hierarchies. Twisted is meant for glue between systems, not for interleaving complex and dispersed stateful machinery with the glue's callback structure.

Ideally, we should separate the “glue” from the “machinery.” But it's complex and fragile, so the next step from here is to get the code under control to the point where we can reason reliably about it. Once we know more about what the code really does, we'll have more freedom to reorganize it.

This branch represents that next step from here: make the buildmaster run in read-only transaction policy. It will not be able to make any changes to the database (or ORM-backed objects), except in sections of code that are explicitly wrapped in read-write transaction policies. Now we know exactly where the buildmaster may change the database — doing it anywhere else would be an error. We keep the read-write sections absolutely minimal, and try to avoid method calls where Twisted context switches might hide. Unfortunately neither Twisted nor Storm seems to have a facility for forbidding them altogether.

Every read-write section commits immediately. That means more fine-grained state changes. I can't be 100% certain that early commits will not produce any unwanted object states, and I can't be 100% certain that the read-write policies cover all code paths that need them. But we've run several types of build through the patched build farm, and we've stress-tested it against about 5 concurrent builders working all-out. As expected, we found code that still lacked an explicit read-write policy — but only one of them. There may be more, but only production use will find them all.

For Q/A, we once again perform builds on staging and/or dogfood, of as many kinds as we can. Include concurrent builds, and successes as well as failures. Verify that the build products (tarballs, packages, changes files, build logs) all go into the librarian, and that the builds end up in a proper terminal state. The contents of the build logs should be consistent with that end state.

As for tests… sorry, I just run them all!

No lint that I could help, copyrights updated, imports formatted as per policy. Thank you for reading this far.

Jeroen

[Steve Kowalik (stevenk)]

Please also link bug 785679 to this MP, you've fixed it too since you've re-enabled the test.

This broadly looks okay, although I will admit to a large sense of fear and trepidation of this hitting production. You're absolutely right, though, the buildd-manager needs more tests and less magic.

I am wary of the amount of times you are calling transaction.commit() in this branch. Especially in tests, where you are calling into methods that could commit() first and avoid all that duplication.

I am holding off my vote for now, this branch requires some thought. And possibly a large amount of vodka.

[Jeroen T. Vermeulen (jtv)]

> Please also link bug 785679 to this MP, you've fixed it too since you've re-
> enabled the test.

Done. Apparently the test didn't use our custom setup for a Twisted-enabled test.

[Julian Edwards (julian-edwards)]

Thanks a *bunch* for tackling this, I realise it's a bit soul-sapping. If it helps, know that I have spent most of the day going through your code to make sure it's all OK :)

319 + transaction.commit()
320 + with DatabaseTransactionPolicy(read_only=False):
321 + library_file = getUtility(ILibraryFileAliasSet).create(
322 + filename, bytes_written, out_file,
323 + contentType=filenameToContentType(filename),
324 + restricted=private)
325 + transaction.commit()

Should that commit at 319 be an abort()? Or are you trying to make it bomb out deliberately if there's data getting written where it should not be?

349 transaction.commit()
350 + with DatabaseTransactionPolicy(read_only=False):
351 + candidate.markAsBuilding(self)
352 + transaction.commit()

Same here.

And in fact in quite a few places so I won't list them all.

586 -from lp.registry.interfaces.pocket import (
587 - PackagePublishingPocket,
588 - )
589 +from lp.registry.interfaces.pocket import PackagePublishingPocket

Why? :)

634 - d = method(librarian, slave_status, logger, send_notification)
635 - return d
636 + return None
637 + else:
638 + return method(librarian, slave_status, logger, send_notification)

I'd prefer if you left this with the local "d" variable because it makes it clear it's returning a Deferred. This is important because anything that reminds the caller is a good thing, especially morons like me who cocked it up once :)

945 - self.useFixture(BuilddSlaveTestSetup())

All of the other tests don't need this, and it's pretty slow to set up, which is why I recently moved it out of setUp(). Is there a good reason to put it back?

Finally - I'd love to see a test that tries to recreate the original bug. I think it's actually quite straightforward, I'll try and explain the steps:

 * Set up a build connected to a BrokenSlave so that when it's polled it'll abort().
 * Create and get a build at the stage where it's inside storeBuildInfo() but waiting on the Deferred to fire that returns the log.
 * Fire the reactor up so the outstanding Deferreds are processed - the abort() should get called before the log Deferred fires. That will abort all the work done in _handleStatus_OK() so is easy to test for.

Hope that makes sense!

review: Needs Fixing

[Jeroen T. Vermeulen (jtv)]

> Thanks a *bunch* for tackling this, I realise it's a bit soul-sapping. If it
> helps, know that I have spent most of the day going through your code to make
> sure it's all OK :)

That helps a lot. And I'm well aware that reviewing this stuff is more soul-sapping than writing it is. Thank you for wading through your 1290 lines of personal hell!

> 319 + transaction.commit()
> 320 + with DatabaseTransactionPolicy(read_only=False):
> 321 + library_file =
> getUtility(ILibraryFileAliasSet).create(
> 322 + filename, bytes_written, out_file,
> 323 + contentType=filenameToContentType(filename),
> 324 + restricted=private)
> 325 + transaction.commit()
>
> Should that commit at 319 be an abort()? Or are you trying to make it bomb
> out deliberately if there's data getting written where it should not be?

And there you put your finger on the deciding consideration, which I had totally forgotten in the meantime. Yes, thanks to DatabaseTransactionPolicy, a commit is actually better than an abort. It will reveal violations of the policy in a much more direct and helpful way than an abort would.

(Also, I believe the ORM makes abort slower because it needs to scan the live objects for changes that need undoing).

> 349 transaction.commit()
> 350 + with DatabaseTransactionPolicy(read_only=False):
> 351 + candidate.markAsBuilding(self)
> 352 + transaction.commit()
>
> Same here.
>
> And in fact in quite a few places so I won't list them all.

I went with the commit throughout, which only required 2 changes.

> 586 -from lp.registry.interfaces.pocket import (
> 587 - PackagePublishingPocket,
> 588 - )
> 589 +from lp.registry.interfaces.pocket import PackagePublishingPocket
>
> Why? :)

Automated: utilities/format-new-and-modified-imports.

> 634 - d = method(librarian, slave_status, logger,
> send_notification)
> 635 - return d
> 636 + return None
> 637 + else:
> 638 + return method(librarian, slave_status, logger,
> send_notification)
>
> I'd prefer if you left this with the local "d" variable because it makes it
> clear it's returning a Deferred. This is important because anything that
> reminds the caller is a good thing, especially morons like me who cocked it up
> once :)

You're right. Had I come across this a bit later in the branch, when I had more grey hairs but also more twisted/buildmanager experience, I would have kept the “d” as well. Fixed.

> 945 - self.useFixture(BuilddSlaveTestSetup())
>
> All of the other tests don't need this, and it's pretty slow to set up, which
> is why I recently moved it out of setUp(). Is there a good reason to put it
> back?

Probably a misguided step in changing the test cases over from that weirdly-named Twisted thing to our own TestCaseWithFactory. Fixed.

> Finally - I'd love to see a test that tries to recreate the original bug. I
> think it's actually quite s...

[Julian Edwards (julian-edwards)]

review: Approve

=== modified file 'lib/lp/buildmaster/interfaces/builder.py'
--- lib/lp/buildmaster/interfaces/builder.py	2011-07-14 21:49:37 +0000
+++ lib/lp/buildmaster/interfaces/builder.py	2011-12-08 16:19:26 +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
@@ -25,10 +25,10 @@
     export_as_webservice_entry,
     export_read_operation,
     exported,
+    operation_for_version,
     operation_parameters,
+    operation_returns_collection_of,
     operation_returns_entry,
-    operation_returns_collection_of,
-    operation_for_version,
     )
 from lazr.restful.fields import (
     Reference,
@@ -50,12 +50,12 @@
 from lp.app.validators.name import name_validator
 from lp.app.validators.url import builder_url_validator
 from lp.registry.interfaces.role import IHasOwner
-from lp.soyuz.interfaces.processor import IProcessor
 from lp.services.fields import (
     Description,
     PersonChoice,
     Title,
     )
+from lp.soyuz.interfaces.processor import IProcessor
 
 
 class BuildDaemonError(Exception):
@@ -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	2011-10-31 09:23:47 +0000
+++ lib/lp/buildmaster/manager.py	2011-12-08 16:19:26 +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."""
@@ -23,10 +23,6 @@
 from zope.component import getUtility
 
 from lp.buildmaster.enums import BuildStatus
-from lp.buildmaster.interfaces.buildfarmjobbehavior import (
-    BuildBehaviorMismatch,
-    )
-from lp.buildmaster.model.builder import Builder
 from lp.buildmaster.interfaces.builder import (
     BuildDaemonError,
     BuildSlaveFailure,
@@ -34,6 +30,11 @@
     CannotFetchFile,
     CannotResumeHost,
     )
+from lp.buildmaster.interfaces.buildfarmjobbehavior import (
+    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	2011-11-23 12:02:59 +0000
+++ lib/lp/buildmaster/model/builder.py	2011-12-08 16:19:26 +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
@@ -76,6 +76,7 @@
     specific_job_classes,
     )
 from lp.registry.interfaces.person import validate_public_person
+from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
 from lp.services.propertycache import (
@@ -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):

=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/model/buildfarmjobbehavior.py	2011-06-09 10:50:25 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehavior.py	2011-12-08 16:19:26 +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,8 +16,8 @@
 import socket
 import xmlrpclib
 
+import transaction
 from twisted.internet import defer
-
 from zope.component import getUtility
 from zope.interface import implements
 from zope.security.proxy import removeSecurityProxy
@@ -32,6 +32,7 @@
     IBuildFarmJobBehavior,
     )
 from lp.services import encoding
+from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 from lp.services.job.interfaces.job import JobStatus
 
 
@@ -70,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')
@@ -77,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)"
@@ -84,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
@@ -114,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)
@@ -128,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.
@@ -151,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-08-31 19:41:51 +0000
+++ lib/lp/buildmaster/model/packagebuild.py	2011-12-08 16:19:26 +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
@@ -9,11 +9,11 @@
     ]
 
 
+from cStringIO import StringIO
 import datetime
 import logging
 import os.path
 
-from cStringIO import StringIO
 from lazr.delegates import delegates
 import pytz
 from storm.expr import Desc
@@ -24,6 +24,7 @@
     Storm,
     Unicode,
     )
+import transaction
 from zope.component import getUtility
 from zope.interface import (
     classProvides,
@@ -43,8 +44,8 @@
     MAIN_STORE,
     )
 from lp.buildmaster.enums import (
+    BuildFarmJobType,
     BuildStatus,
-    BuildFarmJobType,
     )
 from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJobSource
 from lp.buildmaster.interfaces.packagebuild import (
@@ -57,9 +58,8 @@
     BuildFarmJobDerived,
     )
 from lp.buildmaster.model.buildqueue import BuildQueue
-from lp.registry.interfaces.pocket import (
-    PackagePublishingPocket,
-    )
+from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 from lp.soyuz.adapters.archivedependencies import (
     default_component_dependency_name,
     )
@@ -179,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)
@@ -292,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):
@@ -323,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"
@@ -377,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)
@@ -396,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.
@@ -423,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)
@@ -448,11 +480,9 @@
         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)
@@ -468,17 +498,24 @@
         self.status = BuildStatus.CHROOTWAIT
 
         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.
@@ -492,11 +529,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):
@@ -516,7 +550,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)

=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py	2011-11-20 23:37:23 +0000
+++ lib/lp/buildmaster/tests/test_builder.py	2011-12-08 16:19:26 +0000
@@ -155,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)

=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
--- lib/lp/buildmaster/tests/test_manager.py	2011-11-20 23:37:23 +0000
+++ lib/lp/buildmaster/tests/test_manager.py	2011-12-08 16:19:26 +0000
@@ -1,40 +1,36 @@
-# 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
 import xmlrpclib
 
+from lpbuildd.tests import BuilddSlaveTestSetup
 from testtools.deferredruntest import (
     assert_fails_with,
     AsynchronousDeferredRunTest,
     )
-
 import transaction
-
 from twisted.internet import (
     defer,
     reactor,
     task,
     )
-from twisted.internet.task import (
-    deferLater,
-    )
+from twisted.internet.task import deferLater
 from twisted.python.failure import Failure
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
-from lpbuildd.tests import BuilddSlaveTestSetup
-
 from canonical.config import config
+from canonical.database.constants import UTC_NOW
 from canonical.launchpad.ftests import (
     ANONYMOUS,
     login,
     )
-from lp.services.log.logger import BufferLogger
 from canonical.testing.layers import (
     LaunchpadScriptLayer,
     LaunchpadZopelessLayer,
@@ -50,14 +46,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.database.transaction_policy import DatabaseTransactionPolicy
+from lp.services.log.logger import BufferLogger
 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
 from lp.testing import (
     TestCase,
@@ -65,10 +65,13 @@
     )
 from lp.testing.factory import LaunchpadObjectFactory
 from lp.testing.fakemethod import FakeMethod
-from lp.testing.sampledata import BOB_THE_BUILDER_NAME
-
-
-class TestSlaveScannerScan(TestCase):
+from lp.testing.sampledata import (
+    BOB_THE_BUILDER_NAME,
+    FROG_THE_BUILDER_NAME,
+    )
+
+
+class TestSlaveScannerScan(TestCaseWithFactory):
     """Tests `SlaveScanner.scan` method.
 
     This method uses the old framework for scanning and dispatching builds.
@@ -83,6 +86,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')
@@ -90,6 +95,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."""
 
@@ -100,6 +114,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
@@ -114,14 +145,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
@@ -137,17 +168,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)
@@ -160,20 +189,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]')
@@ -186,6 +213,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)
@@ -227,6 +255,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)
@@ -262,25 +291,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)
@@ -288,10 +319,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.
@@ -300,25 +331,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]
 
@@ -466,6 +499,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-09-10 08:59:08 +0000
+++ lib/lp/buildmaster/tests/test_packagebuild.py	2011-12-08 16:19:26 +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`."""
@@ -22,9 +22,7 @@
     LaunchpadFunctionalLayer,
     LaunchpadZopelessLayer,
     )
-from lp.archiveuploader.uploadprocessor import (
-    parse_build_upload_leaf_name,
-    )
+from lp.archiveuploader.uploadprocessor import parse_build_upload_leaf_name
 from lp.buildmaster.enums import (
     BuildFarmJobType,
     BuildStatus,
@@ -34,12 +32,11 @@
     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.tests.mock_slaves import WaitingSlave
-from lp.registry.interfaces.pocket import (
-    PackagePublishingPocket,
-    )
+from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.testing import (
     login,
     login_person,
@@ -285,10 +282,7 @@
 
 
 class TestHandleStatusMixin:
-    """Tests for `IPackageBuild`s handleStatus method.
-
-    This should be run with a Trial TestCase.
-    """
+    """Tests for `IPackageBuild`s handleStatus method."""
 
     layer = LaunchpadZopelessLayer
 
@@ -307,7 +301,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()
@@ -348,7 +342,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'},
@@ -390,14 +384,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)
 

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2011-11-09 11:36:44 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2011-12-08 16:19:26 +0000
@@ -13,8 +13,8 @@
 
 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
 
@@ -28,6 +28,7 @@
 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/transaction_policy.py'
--- lib/lp/services/database/transaction_policy.py	2011-09-29 05:49:18 +0000
+++ lib/lp/services/database/transaction_policy.py	2011-12-08 16:19:26 +0000
@@ -133,8 +133,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	2011-11-09 11:36:44 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuild.py	2011-12-08 16:19:26 +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,7 @@
 
 import pytz
 from storm.store import Store
-from twisted.trial.unittest import TestCase as TrialTestCase
+from testtools.deferredruntest import AsynchronousDeferredRunTest
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -25,6 +25,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 (
@@ -53,6 +54,7 @@
     logout,
     TestCaseWithFactory,
     )
+from lp.testing.fakemethod import FakeMethod
 
 
 class TestBinaryPackageBuild(TestCaseWithFactory):
@@ -522,7 +524,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,9 +572,12 @@
 
 
 class TestHandleStatusForBinaryPackageBuild(
-    MakeBinaryPackageBuildMixin, TestHandleStatusMixin, TrialTestCase):
+    MakeBinaryPackageBuildMixin, TestHandleStatusMixin, TestCaseWithFactory):
     """IPackageBuild.handleStatus works with binary builds."""
 
+    layer = LaunchpadZopelessLayer
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=20)
+
 
 class TestBinaryPackageBuildWebservice(TestCaseWithFactory):
     """Test cases for BinaryPackageBuild on the webservice.

=== modified file 'lib/lp/translations/model/translationtemplatesbuildbehavior.py'
--- lib/lp/translations/model/translationtemplatesbuildbehavior.py	2011-10-19 08:00:29 +0000
+++ lib/lp/translations/model/translationtemplatesbuildbehavior.py	2011-12-08 16:19:26 +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`.
@@ -13,9 +13,10 @@
 
 import datetime
 import os
+import tempfile
+
 import pytz
-import tempfile
-
+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)