Merge lp:~fwereade/juju-core/use-prepare-leave-scope into lp:~go-bot/juju-core/trunk

[William Reade]

state: set departed on dying units' relations

This ended up including:

  * stopping using magic strings in cleanup
  * drawing a clear distinction between "joined" and "in scope" (which
    accounts for most of the lines of changes)

...but did *not* include:

  * fixing the name of the Uniter.JoinedRelations API, because I want to
    wait for API versioning

https://codereview.appspot.com/94540043/

state: set departed on dying units' relations

This ended up including:

  * stopping using magic strings in cleanup
  * drawing a clear distinction between "joined" and "in scope" (which
    accounts for most of the lines of changes)

...but did *not* include:

  * fixing the name of the Uniter.JoinedRelations API, because I want to
    wait for API versioning

https://codereview.appspot.com/94540043/

=== modified file 'state/apiserver/uniter/uniter.go'
--- state/apiserver/uniter/uniter.go	2014-05-13 04:30:48 +0000
+++ state/apiserver/uniter/uniter.go	2014-05-22 09:18:59 +0000
@@ -694,8 +694,8 @@
 	return result, nil
 }
 
-func joinedRelationTags(unit *state.Unit) ([]string, error) {
-	relations, err := unit.JoinedRelations()
+func relationsInScopeTags(unit *state.Unit) ([]string, error) {
+	relations, err := unit.RelationsInScope()
 	if err != nil {
 		return nil, err
 	}
@@ -706,7 +706,9 @@
 	return tags, nil
 }
 
-// JoinedRelations returns the tags of all relations each supplied unit has joined.
+// JoinedRelations returns the tags of all relations for which each supplied unit
+// has entered scope. It should be called RelationsInScope, but it's not convenient
+// to make that change until we have versioned APIs.
 func (u *UniterAPI) JoinedRelations(args params.Entities) (params.StringsResults, error) {
 	result := params.StringsResults{
 		Results: make([]params.StringsResult, len(args.Entities)),
@@ -724,7 +726,7 @@
 			var unit *state.Unit
 			unit, err = u.getUnit(entity.Tag)
 			if err == nil {
-				result.Results[i].Result, err = joinedRelationTags(unit)
+				result.Results[i].Result, err = relationsInScopeTags(unit)
 			}
 		}
 		result.Results[i].Error = common.ServerError(err)

=== modified file 'state/apiserver/uniter/uniter_test.go'
--- state/apiserver/uniter/uniter_test.go	2014-05-13 04:30:48 +0000
+++ state/apiserver/uniter/uniter_test.go	2014-05-22 09:18:59 +0000
@@ -1065,9 +1065,7 @@
 			{rel.Tag()},
 		},
 	}
-	result, err := s.uniter.JoinedRelations(args)
-	c.Assert(err, gc.IsNil)
-	c.Assert(result, gc.DeepEquals, params.StringsResults{
+	expect := params.StringsResults{
 		Results: []params.StringsResult{
 			{Result: []string{rel.Tag()}},
 			{Error: apiservertesting.ErrUnauthorized},
@@ -1076,7 +1074,16 @@
 			{Error: apiservertesting.ErrUnauthorized},
 			{Error: apiservertesting.ErrUnauthorized},
 		},
-	})
+	}
+	check := func() {
+		result, err := s.uniter.JoinedRelations(args)
+		c.Assert(err, gc.IsNil)
+		c.Assert(result, gc.DeepEquals, expect)
+	}
+	check()
+	err = relUnit.PrepareLeaveScope()
+	c.Assert(err, gc.IsNil)
+	check()
 }
 
 func (s *uniterSuite) TestReadSettings(c *gc.C) {

=== modified file 'state/cleanup.go'
--- state/cleanup.go	2014-05-13 04:30:48 +0000
+++ state/cleanup.go	2014-05-22 09:18:59 +0000
@@ -8,17 +8,28 @@
 	"labix.org/v2/mgo/txn"
 )
 
+type cleanupKind string
+
+const (
+	// SCHEMACHANGE: the names are expressive, the values not so much.
+	cleanupRelationSettings            cleanupKind = "settings"
+	cleanupUnitsForDyingService        cleanupKind = "units"
+	cleanupDyingUnit                   cleanupKind = "dyingUnit"
+	cleanupServicesForDyingEnvironment cleanupKind = "services"
+	cleanupForceDestroyedMachine       cleanupKind = "machine"
+)
+
 // cleanupDoc represents a potentially large set of documents that should be
 // removed.
 type cleanupDoc struct {
 	Id     bson.ObjectId `bson:"_id"`
-	Kind   string
+	Kind   cleanupKind
 	Prefix string
 }
 
 // newCleanupOp returns a txn.Op that creates a cleanup document with a unique
 // id and the supplied kind and prefix.
-func (st *State) newCleanupOp(kind, prefix string) txn.Op {
+func (st *State) newCleanupOp(kind cleanupKind, prefix string) txn.Op {
 	doc := &cleanupDoc{
 		Id:     bson.NewObjectId(),
 		Kind:   kind,
@@ -50,14 +61,16 @@
 		var err error
 		logger.Debugf("running %q cleanup: %q", doc.Kind, doc.Prefix)
 		switch doc.Kind {
-		case "settings":
-			err = st.cleanupSettings(doc.Prefix)
-		case "units":
-			err = st.cleanupUnits(doc.Prefix)
-		case "services":
-			err = st.cleanupServices()
-		case "machine":
-			err = st.cleanupMachine(doc.Prefix)
+		case cleanupRelationSettings:
+			err = st.cleanupRelationSettings(doc.Prefix)
+		case cleanupUnitsForDyingService:
+			err = st.cleanupUnitsForDyingService(doc.Prefix)
+		case cleanupDyingUnit:
+			err = st.cleanupDyingUnit(doc.Prefix)
+		case cleanupServicesForDyingEnvironment:
+			err = st.cleanupServicesForDyingEnvironment()
+		case cleanupForceDestroyedMachine:
+			err = st.cleanupForceDestroyedMachine(doc.Prefix)
 		default:
 			err = fmt.Errorf("unknown cleanup kind %q", doc.Kind)
 		}
@@ -80,7 +93,7 @@
 	return nil
 }
 
-func (st *State) cleanupSettings(prefix string) error {
+func (st *State) cleanupRelationSettings(prefix string) error {
 	// Documents marked for cleanup are not otherwise referenced in the
 	// system, and will not be under watch, and are therefore safe to
 	// delete directly.
@@ -95,9 +108,10 @@
 	return nil
 }
 
-// cleanupServices sets all services to Dying, if they are not already Dying
-// or Dead. It's expected to be used when an environment is destroyed.
-func (st *State) cleanupServices() error {
+// cleanupServicesForDyingEnvironment sets all services to Dying, if they are
+// not already Dying or Dead. It's expected to be used when an environment is
+// destroyed.
+func (st *State) cleanupServicesForDyingEnvironment() error {
 	// This won't miss services, because a Dying environment cannot have
 	// services added to it. But we do have to remove the services themselves
 	// via individual transactions, because they could be in any state at all.
@@ -115,9 +129,10 @@
 	return nil
 }
 
-// cleanupUnits sets all units with the given prefix to Dying, if they are not
-// already Dying or Dead. It's expected to be used when a service is destroyed.
-func (st *State) cleanupUnits(prefix string) error {
+// cleanupUnitsForDyingService sets all units with the given prefix to Dying,
+// if they are not already Dying or Dead. It's expected to be used when a
+// service is destroyed.
+func (st *State) cleanupUnitsForDyingService(prefix string) error {
 	// This won't miss units, because a Dying service cannot have units added
 	// to it. But we do have to remove the units themselves via individual
 	// transactions, because they could be in any state at all.
@@ -135,10 +150,38 @@
 	return nil
 }
 
-// cleanupMachine systematically destroys and removes all entities that
-// depend upon the supplied machine, and removes the machine from state. It's
+// cleanupDyingUnit marks the unit as departing from all its joined relations,
+// allowing related units to start converging to a state in which that unit is
+// gone as quickly as possible.
+func (st *State) cleanupDyingUnit(name string) error {
+	unit, err := st.Unit(name)
+	if errors.IsNotFound(err) {
+		return nil
+	} else if err != nil {
+		return err
+	}
+	relations, err := unit.RelationsJoined()
+	if err != nil {
+		return err
+	}
+	for _, relation := range relations {
+		relationUnit, err := relation.Unit(unit)
+		if errors.IsNotFound(err) {
+			continue
+		} else if err != nil {
+			return err
+		}
+		if err := relationUnit.PrepareLeaveScope(); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+// cleanupForceDestroyedMachine systematically destroys and removes all entities
+// that depend upon the supplied machine, and removes the machine from state. It's
 // expected to be used in response to destroy-machine --force.
-func (st *State) cleanupMachine(machineId string) error {
+func (st *State) cleanupForceDestroyedMachine(machineId string) error {
 	machine, err := st.Machine(machineId)
 	if errors.IsNotFound(err) {
 		return nil
@@ -179,7 +222,7 @@
 	// instance that would otherwise be ignored when in provisioner-safe-mode.
 }
 
-// cleanupContainers recursively calls cleanupMachine on the supplied
+// cleanupContainers recursively calls cleanupForceDestroyedMachine on the supplied
 // machine's containers, and removes them from state entirely.
 func (st *State) cleanupContainers(machine *Machine) error {
 	containerIds, err := machine.Containers()
@@ -189,7 +232,7 @@
 		return err
 	}
 	for _, containerId := range containerIds {
-		if err := st.cleanupMachine(containerId); err != nil {
+		if err := st.cleanupForceDestroyedMachine(containerId); err != nil {
 			return err
 		}
 		container, err := st.Machine(containerId)

=== modified file 'state/cleanup_test.go'
--- state/cleanup_test.go	2014-05-13 04:30:48 +0000
+++ state/cleanup_test.go	2014-05-22 09:18:59 +0000
@@ -44,7 +44,6 @@
 
 	// Run the cleanup, and check that units are all destroyed as appropriate.
 	s.assertCleanupRuns(c)
-	s.assertDoesNotNeedCleanup(c)
 	err = units[0].Refresh()
 	c.Assert(err, gc.IsNil)
 	c.Assert(units[0].Life(), gc.Equals, state.Dying)
@@ -52,6 +51,10 @@
 	c.Assert(err, jc.Satisfies, errors.IsNotFound)
 	err = units[2].Refresh()
 	c.Assert(err, jc.Satisfies, errors.IsNotFound)
+
+	// Run a final cleanup to clear the cleanup scheduled for the unit that
+	// became dying.
+	s.assertCleanupCount(c, 1)
 }
 
 func (s *CleanupSuite) TestCleanupEnvironmentServices(c *gc.C) {
@@ -108,12 +111,10 @@
 	// Destroy the service, check the relation's still around.
 	err = pr.svc.Destroy()
 	c.Assert(err, gc.IsNil)
-	s.assertNeedsCleanup(c)
-	s.assertCleanupRuns(c)
+	s.assertCleanupCount(c, 2)
 	err = rel.Refresh()
 	c.Assert(err, gc.IsNil)
 	c.Assert(rel.Life(), gc.Equals, state.Dying)
-	s.assertDoesNotNeedCleanup(c)
 
 	// The unit leaves scope, triggering relation removal.
 	err = pr.ru0.LeaveScope()
@@ -126,8 +127,7 @@
 	c.Assert(settings, gc.DeepEquals, map[string]interface{}{"some": "settings"})
 
 	// ...but they are on cleanup.
-	s.assertCleanupRuns(c)
-	s.assertDoesNotNeedCleanup(c)
+	s.assertCleanupCount(c, 1)
 	_, err = pr.ru1.ReadSettings("riak/0")
 	c.Assert(err, gc.ErrorMatches, `cannot read settings for unit "riak/0" in relation "riak:ring": settings not found`)
 }
@@ -164,12 +164,11 @@
 	s.assertNeedsCleanup(c)
 
 	// Clean up, and check that the unit has been removed...
-	s.assertCleanupRuns(c)
-	s.assertDoesNotNeedCleanup(c)
+	s.assertCleanupCount(c, 2)
 	assertRemoved(c, pr.u0)
 
 	// ...and the unit has departed relation scope...
-	assertNotInScope(c, pr.ru0)
+	assertNotJoined(c, pr.ru0)
 
 	// ...but that the machine remains, and is Dead, ready for removal by the
 	// provisioner.
@@ -218,8 +217,7 @@
 	s.assertNeedsCleanup(c)
 
 	// Clean up, and check that the container has been removed...
-	s.assertCleanupRuns(c)
-	s.assertDoesNotNeedCleanup(c)
+	s.assertCleanupCount(c, 2)
 	err = container.Refresh()
 	c.Assert(err, jc.Satisfies, errors.IsNotFound)
 
@@ -240,6 +238,67 @@
 	assertLife(c, machine, state.Dead)
 }
 
+func (s *CleanupSuite) TestCleanupDyingUnit(c *gc.C) {
+	// Create active unit, in a relation.
+	prr := NewProReqRelation(c, &s.ConnSuite, charm.ScopeGlobal)
+	err := prr.pru0.EnterScope(nil)
+	c.Assert(err, gc.IsNil)
+
+	// Destroy provider unit 0; check it's Dying, and a cleanup has been scheduled.
+	err = prr.pu0.Destroy()
+	c.Assert(err, gc.IsNil)
+	err = prr.pu0.Refresh()
+	c.Assert(err, gc.IsNil)
+	assertLife(c, prr.pu0, state.Dying)
+	s.assertNeedsCleanup(c)
+
+	// Check it's reported in scope until cleaned up.
+	assertJoined(c, prr.pru0)
+	s.assertCleanupCount(c, 1)
+	assertInScope(c, prr.pru0)
+	assertNotJoined(c, prr.pru0)
+
+	// Destroy the relation, and check it sticks around...
+	err = prr.rel.Destroy()
+	c.Assert(err, gc.IsNil)
+	assertLife(c, prr.rel, state.Dying)
+
+	// ...until the unit is removed, and really leaves scope.
+	err = prr.pu0.EnsureDead()
+	c.Assert(err, gc.IsNil)
+	err = prr.pu0.Remove()
+	c.Assert(err, gc.IsNil)
+	assertNotInScope(c, prr.pru0)
+	assertRemoved(c, prr.rel)
+}
+
+func (s *CleanupSuite) TestCleanupDyingUnitAlreadyRemoved(c *gc.C) {
+	// Create active unit, in a relation.
+	prr := NewProReqRelation(c, &s.ConnSuite, charm.ScopeGlobal)
+	err := prr.pru0.EnterScope(nil)
+	c.Assert(err, gc.IsNil)
+
+	// Destroy provider unit 0; check it's Dying, and a cleanup has been scheduled.
+	err = prr.pu0.Destroy()
+	c.Assert(err, gc.IsNil)
+	err = prr.pu0.Refresh()
+	c.Assert(err, gc.IsNil)
+	assertLife(c, prr.pu0, state.Dying)
+	s.assertNeedsCleanup(c)
+
+	// Remove the unit, and the relation.
+	err = prr.pu0.EnsureDead()
+	c.Assert(err, gc.IsNil)
+	err = prr.pu0.Remove()
+	c.Assert(err, gc.IsNil)
+	err = prr.rel.Destroy()
+	c.Assert(err, gc.IsNil)
+	assertRemoved(c, prr.rel)
+
+	// Check the cleanup still runs happily.
+	s.assertCleanupCount(c, 1)
+}
+
 func (s *CleanupSuite) TestNothingToCleanup(c *gc.C) {
 	s.assertDoesNotNeedCleanup(c)
 	s.assertCleanupRuns(c)
@@ -262,3 +321,15 @@
 	c.Assert(err, gc.IsNil)
 	c.Assert(actual, jc.IsFalse)
 }
+
+// assertCleanupCount is useful because certain cleanups cause other cleanups
+// to be queued; it makes more sense to just run cleanup again than to unpick
+// object destruction so that we run the cleanups inline while running cleanups.
+func (s *CleanupSuite) assertCleanupCount(c *gc.C, count int) {
+	for i := 0; i < count; i++ {
+		c.Logf("checking cleanups %d", i)
+		s.assertNeedsCleanup(c)
+		s.assertCleanupRuns(c)
+	}
+	s.assertDoesNotNeedCleanup(c)
+}

=== modified file 'state/environ.go'
--- state/environ.go	2014-05-13 04:30:48 +0000
+++ state/environ.go	2014-05-22 09:18:59 +0000
@@ -105,7 +105,7 @@
 		Id:     e.doc.UUID,
 		Update: bson.D{{"$set", bson.D{{"life", Dying}}}},
 		Assert: isEnvAliveDoc,
-	}, e.st.newCleanupOp("services", "")}
+	}, e.st.newCleanupOp(cleanupServicesForDyingEnvironment, "")}
 	err := e.st.runTransaction(ops)
 	switch err {
 	case nil, txn.ErrAborted:

=== modified file 'state/machine.go'
--- state/machine.go	2014-05-13 04:30:48 +0000
+++ state/machine.go	2014-05-22 09:18:59 +0000
@@ -398,7 +398,7 @@
 			C:      m.st.machines.Name,
 			Id:     m.doc.Id,
 			Assert: bson.D{{"jobs", bson.D{{"$nin", []MachineJob{JobManageEnviron}}}}},
-		}, m.st.newCleanupOp("machine", m.doc.Id)}
+		}, m.st.newCleanupOp(cleanupForceDestroyedMachine, m.doc.Id)}
 		if err := m.st.runTransaction(ops); err != txn.ErrAborted {
 			return err
 		}

=== modified file 'state/relation.go'
--- state/relation.go	2014-05-13 04:30:48 +0000
+++ state/relation.go	2014-05-22 09:18:59 +0000
@@ -216,7 +216,7 @@
 			Update: bson.D{{"$inc", bson.D{{"relationcount", -1}}}},
 		})
 	}
-	cleanupOp := r.st.newCleanupOp("settings", fmt.Sprintf("r#%d#", r.Id()))
+	cleanupOp := r.st.newCleanupOp(cleanupRelationSettings, fmt.Sprintf("r#%d#", r.Id()))
 	return append(ops, cleanupOp), nil
 }
 

=== modified file 'state/relationunit.go'
--- state/relationunit.go	2014-05-20 13:05:58 +0000
+++ state/relationunit.go	2014-05-22 09:18:59 +0000
@@ -326,13 +326,26 @@
 	return fmt.Errorf("cannot leave scope for %s: inconsistent state", desc)
 }
 
-// InScope returns whether the relation unit has entered scope or not.
+// InScope returns whether the relation unit has entered scope and not left it.
 func (ru *RelationUnit) InScope() (bool, error) {
+	return ru.inScope(nil)
+}
+
+// Joined returns whether the relation unit has entered scope and neither left
+// it nor prepared to leave it.
+func (ru *RelationUnit) Joined() (bool, error) {
+	return ru.inScope(bson.D{{"departing", bson.D{{"$ne", true}}}})
+}
+
+// inScope returns whether a scope document exists satisfying the supplied
+// selector.
+func (ru *RelationUnit) inScope(sel bson.D) (bool, error) {
 	key, err := ru.key(ru.unit.Name())
 	if err != nil {
 		return false, err
 	}
-	count, err := ru.st.relationScopes.FindId(key).Count()
+	sel = append(sel, bson.D{{"_id", key}}...)
+	count, err := ru.st.relationScopes.Find(sel).Count()
 	if err != nil {
 		return false, err
 	}

=== modified file 'state/relationunit_test.go'
--- state/relationunit_test.go	2014-05-20 13:05:58 +0000
+++ state/relationunit_test.go	2014-05-22 09:18:59 +0000
@@ -35,11 +35,25 @@
 }
 
 func assertNotInScope(c *gc.C, ru *state.RelationUnit) {
+	assertNotJoined(c, ru)
 	ok, err := ru.InScope()
 	c.Assert(err, gc.IsNil)
 	c.Assert(ok, jc.IsFalse)
 }
 
+func assertJoined(c *gc.C, ru *state.RelationUnit) {
+	assertInScope(c, ru)
+	ok, err := ru.Joined()
+	c.Assert(err, gc.IsNil)
+	c.Assert(ok, jc.IsTrue)
+}
+
+func assertNotJoined(c *gc.C, ru *state.RelationUnit) {
+	ok, err := ru.Joined()
+	c.Assert(err, gc.IsNil)
+	c.Assert(ok, jc.IsFalse)
+}
+
 func (s *RelationUnitSuite) TestReadSettingsErrors(c *gc.C) {
 	riak := s.AddTestingService(c, "riak", s.AddTestingCharm(c, "riak"))
 	u0, err := riak.AddUnit()
@@ -94,7 +108,7 @@
 		}
 	}
 	assertSettings(pr.u0, normal)
-	assertInScope(c, pr.ru0)
+	assertJoined(c, pr.ru0)
 
 	// Check that EnterScope when scope already entered does not touch
 	// settings at all.
@@ -102,7 +116,7 @@
 	err = pr.ru0.EnterScope(changed)
 	c.Assert(err, gc.IsNil)
 	assertSettings(pr.u0, normal)
-	assertInScope(c, pr.ru0)
+	assertJoined(c, pr.ru0)
 
 	// Leave scope, check settings are still as accessible as before.
 	err = pr.ru0.LeaveScope()
@@ -115,7 +129,7 @@
 	err = pr.ru0.EnterScope(changed)
 	c.Assert(err, gc.IsNil)
 	assertSettings(pr.u0, changed)
-	assertInScope(c, pr.ru0)
+	assertJoined(c, pr.ru0)
 
 	// Leave and re-enter with nil nettings, and check they overwrite to become
 	// an empty map.
@@ -125,14 +139,14 @@
 	err = pr.ru0.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
 	assertSettings(pr.u0, map[string]interface{}{})
-	assertInScope(c, pr.ru0)
+	assertJoined(c, pr.ru0)
 
 	// Check that entering scope for the first time with nil settings works correctly.
 	assertNotInScope(c, pr.ru1)
 	err = pr.ru1.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
 	assertSettings(pr.u1, map[string]interface{}{})
-	assertInScope(c, pr.ru1)
+	assertJoined(c, pr.ru1)
 }
 
 func (s *RelationUnitSuite) TestProReqSettings(c *gc.C) {
@@ -154,7 +168,7 @@
 	node.Set("meme", "foul-bachelor-frog")
 	_, err = node.Write()
 	c.Assert(err, gc.IsNil)
-	assertInScope(c, prr.pru0)
+	assertJoined(c, prr.pru0)
 
 	// Check settings can be read by every RU.
 	for _, ru := range rus {
@@ -184,7 +198,7 @@
 	node.Set("meme", "foul-bachelor-frog")
 	_, err = node.Write()
 	c.Assert(err, gc.IsNil)
-	assertInScope(c, prr.pru0)
+	assertJoined(c, prr.pru0)
 
 	// Check settings can be read by RUs in the same container.
 	rus0 := RUs{prr.pru0, prr.rru0}
@@ -229,13 +243,13 @@
 	err = pru.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
 	assertSubCount(1)
-	assertInScope(c, pru)
+	assertJoined(c, pru)
 
 	// Enter principal scope again and check no more subordinates created.
 	err = pru.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
 	assertSubCount(1)
-	assertInScope(c, pru)
+	assertJoined(c, pru)
 
 	// Leave principal scope, then re-enter, and check that still no further
 	// subordinates are created.
@@ -245,7 +259,7 @@
 	err = pru.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
 	runits := assertSubCount(1)
-	assertInScope(c, pru)
+	assertJoined(c, pru)
 
 	// Set the subordinate to Dying, and enter scope again; because the scope
 	// is already entered, no error is returned.
@@ -254,7 +268,7 @@
 	c.Assert(err, gc.IsNil)
 	err = pru.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
-	assertInScope(c, pru)
+	assertJoined(c, pru)
 
 	// Leave scope, then try to enter again with the Dying subordinate.
 	err = pru.LeaveScope()
@@ -275,7 +289,7 @@
 	err = pru.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
 	assertSubCount(1)
-	assertInScope(c, pru)
+	assertJoined(c, pru)
 }
 
 func (s *RelationUnitSuite) TestDestroyRelationWithUnitsInScope(c *gc.C) {
@@ -287,11 +301,11 @@
 	assertNotInScope(c, pr.ru0)
 	err := pr.ru0.EnterScope(map[string]interface{}{"some": "settings"})
 	c.Assert(err, gc.IsNil)
-	assertInScope(c, pr.ru0)
+	assertJoined(c, pr.ru0)
 	assertNotInScope(c, pr.ru1)
 	err = pr.ru1.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
-	assertInScope(c, pr.ru1)
+	assertJoined(c, pr.ru1)
 	err = pr.svc.Destroy()
 	c.Assert(err, gc.IsNil)
 	err = rel.Refresh()
@@ -309,7 +323,7 @@
 	c.Assert(err, gc.ErrorMatches, `cannot read settings for unit "riak/2" in relation "riak:ring": settings not found`)
 
 	// ru0 leaves the scope; check that service Destroy is still a no-op.
-	assertInScope(c, pr.ru0)
+	assertJoined(c, pr.ru0)
 	err = pr.ru0.LeaveScope()
 	c.Assert(err, gc.IsNil)
 	assertNotInScope(c, pr.ru0)
@@ -328,7 +342,7 @@
 	assertSettings()
 
 	// The final unit leaves the scope, and cleans up after itself.
-	assertInScope(c, pr.ru1)
+	assertJoined(c, pr.ru1)
 	err = pr.ru1.LeaveScope()
 	c.Assert(err, gc.IsNil)
 	assertNotInScope(c, pr.ru1)
@@ -353,11 +367,11 @@
 	assertNotInScope(c, pr.ru0)
 	err := pr.ru0.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
-	assertInScope(c, pr.ru0)
+	assertJoined(c, pr.ru0)
 	assertNotInScope(c, pr.ru1)
 	err = pr.ru1.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
-	assertInScope(c, pr.ru1)
+	assertJoined(c, pr.ru1)
 
 	// One unit becomes Dying, then re-enters the scope; this is not an error,
 	// because the state is already as requested.
@@ -365,7 +379,7 @@
 	c.Assert(err, gc.IsNil)
 	err = pr.ru0.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
-	assertInScope(c, pr.ru0)
+	assertJoined(c, pr.ru0)
 
 	// Two units leave...
 	err = pr.ru0.LeaveScope()
@@ -384,7 +398,7 @@
 	assertNotInScope(c, pr.ru2)
 	err = pr.ru2.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
-	assertInScope(c, pr.ru2)
+	assertJoined(c, pr.ru2)
 
 	// ...but Dying units cannot.
 	err = pr.u3.Destroy()
@@ -421,7 +435,7 @@
 	node, err := pr.ru0.Settings()
 	c.Assert(err, gc.IsNil)
 	c.Assert(node.Map(), gc.DeepEquals, map[string]interface{}{"foo": "bar"})
-	assertInScope(c, pr.ru0)
+	assertJoined(c, pr.ru0)
 
 	// ru1 enters; check change is observed.
 	assertNotInScope(c, pr.ru1)
@@ -429,20 +443,20 @@
 	c.Assert(err, gc.IsNil)
 	s.assertScopeChange(c, w0, []string{"riak/1"}, nil)
 	s.assertNoScopeChange(c, w0)
-	assertInScope(c, pr.ru1)
+	assertJoined(c, pr.ru1)
 
 	// ru1 enters again, check no problems and no changes.
 	err = pr.ru1.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
 	s.assertNoScopeChange(c, w0)
-	assertInScope(c, pr.ru1)
+	assertJoined(c, pr.ru1)
 
 	// Stop watching; ru2 enters.
 	testing.AssertStop(c, w0)
 	assertNotInScope(c, pr.ru2)
 	err = pr.ru2.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
-	assertInScope(c, pr.ru2)
+	assertJoined(c, pr.ru2)
 
 	// Start watch again, check initial event.
 	w0 = pr.ru0.WatchScope()
@@ -451,7 +465,7 @@
 	s.assertNoScopeChange(c, w0)
 
 	// ru1 leaves; check event.
-	assertInScope(c, pr.ru1)
+	assertJoined(c, pr.ru1)
 	err = pr.ru1.LeaveScope()
 	c.Assert(err, gc.IsNil)
 	s.assertScopeChange(c, w0, nil, []string{"riak/1"})
@@ -489,7 +503,7 @@
 		s.assertScopeChange(c, w, []string{"mysql/0"}, nil)
 	}
 	s.assertNoScopeChange(c, ws...)
-	assertInScope(c, prr.pru0)
+	assertJoined(c, prr.pru0)
 
 	// req0 enters; check detected only by pro RUs.
 	assertNotInScope(c, prr.rru0)
@@ -502,7 +516,7 @@
 		s.assertScopeChange(c, w, []string{"wordpress/0"}, nil)
 	}
 	s.assertNoScopeChange(c, ws...)
-	assertInScope(c, prr.rru0)
+	assertJoined(c, prr.rru0)
 
 	// Stop watches; remaining RUs enter.
 	for _, w := range ws {
@@ -511,11 +525,11 @@
 	assertNotInScope(c, prr.pru1)
 	err = prr.pru1.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
-	assertInScope(c, prr.pru1)
+	assertJoined(c, prr.pru1)
 	assertNotInScope(c, prr.rru1)
 	err = prr.rru1.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
-	assertInScope(c, prr.rru0)
+	assertJoined(c, prr.rru0)
 
 	// Start new watches, check initial events.
 	ws = prr.watches()
@@ -531,7 +545,7 @@
 	s.assertNoScopeChange(c, ws...)
 
 	// pru0 leaves; check detected only by req RUs.
-	assertInScope(c, prr.pru0)
+	assertJoined(c, prr.pru0)
 	err = prr.pru0.LeaveScope()
 	c.Assert(err, gc.IsNil)
 	for _, w := range rws() {
@@ -541,7 +555,7 @@
 	assertNotInScope(c, prr.pru0)
 
 	// rru0 leaves; check detected only by pro RUs.
-	assertInScope(c, prr.rru0)
+	assertJoined(c, prr.rru0)
 	err = prr.rru0.LeaveScope()
 	c.Assert(err, gc.IsNil)
 	for _, w := range pws() {
@@ -570,7 +584,7 @@
 	c.Assert(err, gc.IsNil)
 	s.assertScopeChange(c, ws[2], []string{"mysql/0"}, nil)
 	s.assertNoScopeChange(c, ws...)
-	assertInScope(c, prr.pru0)
+	assertJoined(c, prr.pru0)
 
 	// req1 enters; check detected only by same-container pro.
 	assertNotInScope(c, prr.rru1)
@@ -578,7 +592,7 @@
 	c.Assert(err, gc.IsNil)
 	s.assertScopeChange(c, ws[1], []string{"logging/1"}, nil)
 	s.assertNoScopeChange(c, ws...)
-	assertInScope(c, prr.rru1)
+	assertJoined(c, prr.rru1)
 
 	// Stop watches; remaining RUs enter scope.
 	for _, w := range ws {
@@ -601,11 +615,11 @@
 	s.assertScopeChange(c, ws[2], []string{"mysql/0"}, nil)
 	s.assertScopeChange(c, ws[3], []string{"mysql/1"}, nil)
 	s.assertNoScopeChange(c, ws...)
-	assertInScope(c, prr.pru1)
-	assertInScope(c, prr.rru0)
+	assertJoined(c, prr.pru1)
+	assertJoined(c, prr.rru0)
 
 	// pru0 leaves; check detected only by same-container req.
-	assertInScope(c, prr.pru0)
+	assertJoined(c, prr.pru0)
 	err = prr.pru0.LeaveScope()
 	c.Assert(err, gc.IsNil)
 	s.assertScopeChange(c, ws[2], nil, []string{"mysql/0"})
@@ -613,7 +627,7 @@
 	assertNotInScope(c, prr.pru0)
 
 	// rru0 leaves; check detected only by same-container pro.
-	assertInScope(c, prr.rru0)
+	assertJoined(c, prr.rru0)
 	err = prr.rru0.LeaveScope()
 	c.Assert(err, gc.IsNil)
 	s.assertScopeChange(c, ws[0], nil, []string{"logging/0"})
@@ -678,6 +692,7 @@
 	s.assertScopeChange(c, w0, nil, []string{"wordpress/0"})
 	s.assertNoScopeChange(c, w0)
 	assertInScope(c, prr.rru0)
+	assertNotJoined(c, prr.rru0)
 
 	// rru1 leaves, and the relation is destroyed; it's not removed, because
 	// rru0 keeps it alive until it really leaves scope.

=== modified file 'state/service.go'
--- state/service.go	2014-05-13 04:30:48 +0000
+++ state/service.go	2014-05-22 09:18:59 +0000
@@ -189,7 +189,7 @@
 	// about is that *some* unit is, or is not, keeping the service from
 	// being removed: the difference between 1 unit and 1000 is irrelevant.
 	if s.doc.UnitCount > 0 {
-		ops = append(ops, s.st.newCleanupOp("units", s.doc.Name+"/"))
+		ops = append(ops, s.st.newCleanupOp(cleanupUnitsForDyingService, s.doc.Name+"/"))
 		notLastRefs = append(notLastRefs, bson.D{{"unitcount", bson.D{{"$gt", 0}}}}...)
 	} else {
 		notLastRefs = append(notLastRefs, bson.D{{"unitcount", 0}}...)

=== modified file 'state/service_test.go'
--- state/service_test.go	2014-05-21 00:15:27 +0000
+++ state/service_test.go	2014-05-22 09:18:59 +0000
@@ -1102,6 +1102,13 @@
 		}
 	}
 
+	// Check for queued unit cleanups, and run them.
+	dirty, err = s.State.NeedsCleanup()
+	c.Assert(err, gc.IsNil)
+	c.Assert(dirty, gc.Equals, true)
+	err = s.State.Cleanup()
+	c.Assert(err, gc.IsNil)
+
 	// Check we're now clean.
 	dirty, err = s.State.NeedsCleanup()
 	c.Assert(err, gc.IsNil)

=== modified file 'state/unit.go'
--- state/unit.go	2014-05-13 04:30:48 +0000
+++ state/unit.go	2014-05-22 09:18:59 +0000
@@ -322,12 +322,13 @@
 	// the number of tests that have to change and defer that improvement to
 	// its own CL.
 	minUnitsOp := minUnitsTriggerOp(u.st, u.ServiceName())
+	cleanupOp := u.st.newCleanupOp(cleanupDyingUnit, u.doc.Name)
 	setDyingOps := []txn.Op{{
 		C:      u.st.units.Name,
 		Id:     u.doc.Name,
 		Assert: isAliveDoc,
 		Update: bson.D{{"$set", bson.D{{"life", Dying}}}},
-	}, minUnitsOp}
+	}, cleanupOp, minUnitsOp}
 	if u.doc.Principal != "" {
 		return setDyingOps, nil
 	} else if len(u.doc.Subordinates) != 0 {
@@ -482,25 +483,43 @@
 	return names
 }
 
-// JoinedRelations returns the relations for which the unit is in scope.
-func (u *Unit) JoinedRelations() ([]*Relation, error) {
+// RelationsJoined returns the relations for which the unit has entered scope
+// and neither left it nor prepared to leave it
+func (u *Unit) RelationsJoined() ([]*Relation, error) {
+	return u.relations(func(ru *RelationUnit) (bool, error) {
+		return ru.Joined()
+	})
+}
+
+// RelationsInScope returns the relations for which the unit has entered scope
+// and not left it.
+func (u *Unit) RelationsInScope() ([]*Relation, error) {
+	return u.relations(func(ru *RelationUnit) (bool, error) {
+		return ru.InScope()
+	})
+}
+
+type relationPredicate func(ru *RelationUnit) (bool, error)
+
+// relations implements RelationsJoined and RelationsInScope.
+func (u *Unit) relations(predicate relationPredicate) ([]*Relation, error) {
 	candidates, err := serviceRelations(u.st, u.doc.Service)
 	if err != nil {
 		return nil, err
 	}
-	var joinedRelations []*Relation
+	var filtered []*Relation
 	for _, relation := range candidates {
 		relationUnit, err := relation.Unit(u)
 		if err != nil {
 			return nil, err
 		}
-		if inScope, err := relationUnit.InScope(); err != nil {
+		if include, err := predicate(relationUnit); err != nil {
 			return nil, err
-		} else if inScope {
-			joinedRelations = append(joinedRelations, relation)
+		} else if include {
+			filtered = append(filtered, relation)
 		}
 	}
-	return joinedRelations, nil
+	return filtered, nil
 }
 
 // DeployerTag returns the tag of the agent responsible for deploying

=== modified file 'state/unit_test.go'
--- state/unit_test.go	2014-05-13 04:30:48 +0000
+++ state/unit_test.go	2014-05-22 09:18:59 +0000
@@ -1018,7 +1018,7 @@
 	c.Assert(principal, gc.Equals, "")
 }
 
-func (s *UnitSuite) TestJoinedRelations(c *gc.C) {
+func (s *UnitSuite) TestRelations(c *gc.C) {
 	wordpress0 := s.unit
 	mysql := s.AddTestingService(c, "mysql", s.AddTestingCharm(c, "mysql"))
 	mysql0, err := mysql.AddUnit()
@@ -1028,35 +1028,48 @@
 	rel, err := s.State.AddRelation(eps...)
 	c.Assert(err, gc.IsNil)
 
-	assertJoinedRelations := func(unit *state.Unit, expect ...*state.Relation) {
-		actual, err := unit.JoinedRelations()
-		c.Assert(err, gc.IsNil)
+	assertEquals := func(actual, expect []*state.Relation) {
 		c.Assert(actual, gc.HasLen, len(expect))
 		for i, a := range actual {
 			c.Assert(a.Id(), gc.Equals, expect[i].Id())
 		}
 	}
-	assertJoinedRelations(wordpress0)
-	assertJoinedRelations(mysql0)
+	assertRelationsJoined := func(unit *state.Unit, expect ...*state.Relation) {
+		actual, err := unit.RelationsJoined()
+		c.Assert(err, gc.IsNil)
+		assertEquals(actual, expect)
+	}
+	assertRelationsInScope := func(unit *state.Unit, expect ...*state.Relation) {
+		actual, err := unit.RelationsInScope()
+		c.Assert(err, gc.IsNil)
+		assertEquals(actual, expect)
+	}
+	assertRelations := func(unit *state.Unit, expect ...*state.Relation) {
+		assertRelationsInScope(unit, expect...)
+		assertRelationsJoined(unit, expect...)
+	}
+	assertRelations(wordpress0)
+	assertRelations(mysql0)
 
 	mysql0ru, err := rel.Unit(mysql0)
 	c.Assert(err, gc.IsNil)
 	err = mysql0ru.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
-	assertJoinedRelations(wordpress0)
-	assertJoinedRelations(mysql0, rel)
+	assertRelations(wordpress0)
+	assertRelations(mysql0, rel)
 
 	wordpress0ru, err := rel.Unit(wordpress0)
 	c.Assert(err, gc.IsNil)
 	err = wordpress0ru.EnterScope(nil)
 	c.Assert(err, gc.IsNil)
-	assertJoinedRelations(wordpress0, rel)
-	assertJoinedRelations(mysql0, rel)
+	assertRelations(wordpress0, rel)
+	assertRelations(mysql0, rel)
 
-	err = mysql0ru.LeaveScope()
+	err = mysql0ru.PrepareLeaveScope()
 	c.Assert(err, gc.IsNil)
-	assertJoinedRelations(wordpress0, rel)
-	assertJoinedRelations(mysql0)
+	assertRelations(wordpress0, rel)
+	assertRelationsInScope(mysql0, rel)
+	assertRelationsJoined(mysql0)
 }
 
 func (s *UnitSuite) TestRemove(c *gc.C) {