Code review comment for lp:~jtv/launchpad/bug-717969

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!