Many comments, because I like the sound of my own typing, but very little actionable. I'd appreciate your thoughts but this basically LGTM. Fix any trivials and let me know when it's ready to merge, and I'll approve it. https://codereview.appspot.com/98260043/diff/40001/state/unit.go File state/unit.go (right): https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1325 state/unit.go:1325: if notDead, err := isNotDead(u.st.units, u.doc.Name); err != nil { This one is interesting -- it's a not-strictly-necessary DB hit before running the txn, and I'd ordinarily shy away from that in favour of checking u.doc.Life -- but let's see how it goes. I suspect we'll need to go with the other approach in the end, as we start to care about things like charm versions, but we can wait and see if I'm right about that. https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1331 state/unit.go:1331: sel := bson.D{{"_id", newActionID}} This block is almost redundant, because we should be able to depend on the uniqueness of sequence IDs (even in the case of a new unit being created with the same name as an old one (which is itself a bug (although it only happens when a service is destroyed and recreated))). If it didn't involve an extra DB hit I'd have no problem with it; but as it is I'm gently leaning towards dropping it. If there's a problem here we'll fall out to ErrExcessiveContention anyway, and it should actually be pretty easy to diagnose from there. https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1340 state/unit.go:1340: ops := []txn.Op{{ As discussed yesterday, in this particular case the ops don't have to be rebuilt each time, but it's probably good to keep the familiar structure. https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1351 state/unit.go:1351: return newActionID, err returning the id is not "normal" for this package, but IMO it's a good idea. thanks. https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go File state/unit_test.go (right): https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go#newcode1304 state/unit_test.go:1304: // verify action Id() is of expected form (unit id prefix, + sequence) Add a note that this is temporary, and we shouldn't really leak the actual _id. https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go#newcode1335 state/unit_test.go:1335: unitDead := state.TransactionHook{ s/unitDead/killUnit/ perhaps? https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go#newcode1348 state/unit_test.go:1348: //TODO(jcw4): need to figure out how to inject 'contention' into the transaction Haha. I've thought of a way, and I kinda endorse you doing it, so long as you clearly comment that it'll stop working when lp:1174610 is fixed. In the Before hook, you get st.Unit(name) and make it Dead; in the After hook, you remove the unit and the service, and create a new service with the same name. From the POV of that method the unit will appear to be resurrected. But that's also dirty as sin, and will break when we fix other stuff, so I'm reasonably comfortable skipping it. (The other possibility, which is *maybe* cleaner, is to figure out the action doc id and create/remove a doc with that name in the before/after hooks. But figuring out the id without dirt may be tricky -- expose sequence() in export_test, and subtract 1? Could work, but still depends on knowing _id structure; don't really love either.) https://codereview.appspot.com/98260043/