fix: process peer relations last

It was found that when removing applications, hooks for some relations
rely on the application data populated by peer relations.

When peer relations are departed/broken first, these hooks fail,
blocking teardown.

It is reasonable to process existing relations peer-last to prevent this
situation. Hooks for relation creation and joining are unaffected.

worker/uniter/relation/resolver.go

@@ -74,8 +74,33 @@ func (r *relationsResolver) NextOp(
 		return nil, errors.Trace(err)
 	}
 
-	// Check whether we need to fire a hook for any of the relations.
+	// Collect peer relations and defer their processing until after other
+	// relations. This is simpler than implementing a sort based on the type.
+	// Processing them last ensures that upon application removal, the hooks
+	// for other relations can rely on the peer relations still being present.
+	var peerRelations []int
+
+	// Check whether we need to fire a hook for any of the non-peer relations.
 	for relationID, relationSnapshot := range remoteState.Relations {
+		if isPeer, _ := r.stateTracker.IsPeerRelation(relationID); isPeer {
+			peerRelations = append(peerRelations, relationID)
+			continue
+		}
+
+		op, err := r.processRelationSnapshot(relationID, relationSnapshot, remoteState, opFactory)
+		if err != nil {
+			if errors.Is(err, resolver.ErrNoOperation) {
+				continue
+			}
+			return nil, errors.Trace(err)
+		}
+		return op, nil
+	}
+
+	// Process the deferred peer relations.
+	for _, relationID := range peerRelations {
+		relationSnapshot := remoteState.Relations[relationID]
+
 		op, err := r.processRelationSnapshot(relationID, relationSnapshot, remoteState, opFactory)
 		if err != nil {
 			if errors.Is(err, resolver.ErrNoOperation) {

worker/uniter/relation/resolver_test.go

@@ -1359,7 +1359,8 @@ func (s *mockRelationResolverSuite) TestHookRelationJoined(c *gc.C) {
 	s.expectIsKnown(1)
 	s.expectIsImplicitFalse(1)
 	s.expectStateUnknown(1)
-	s.expectIsPeerRelationFalse(1)
+
+	s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil).Times(2)
 
 	relationsResolver := s.newRelationResolver(s.mockRelStTracker, s.mockSupDestroyer)
 	op, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{})
@@ -1402,7 +1403,8 @@ func (s *mockRelationResolverSuite) TestHookRelationChangedApplication(c *gc.C)
 	s.expectIsKnown(1)
 	s.expectIsImplicitFalse(1)
 	s.expectState(relationState)
-	s.expectIsPeerRelationFalse(1)
+
+	s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil).Times(2)
 
 	relationsResolver := s.newRelationResolver(s.mockRelStTracker, s.mockSupDestroyer)
 	op, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{})
@@ -1441,6 +1443,8 @@ func (s *mockRelationResolverSuite) TestHookRelationChangedSuspended(c *gc.C) {
 	s.expectState(relationState)
 	s.expectLocalUnitAndApplicationLife()
 
+	s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil)
+
 	relationsResolver := s.newRelationResolver(s.mockRelStTracker, s.mockSupDestroyer)
 	op, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{})
 	c.Assert(err, jc.ErrorIsNil)
@@ -1478,6 +1482,8 @@ func (s *mockRelationResolverSuite) TestHookRelationDeparted(c *gc.C) {
 	s.expectState(relationState)
 	s.expectLocalUnitAndApplicationLife()
 
+	s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil)
+
 	relationsResolver := s.newRelationResolver(s.mockRelStTracker, s.mockSupDestroyer)
 	op, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{})
 	c.Assert(err, jc.ErrorIsNil)
@@ -1495,22 +1501,35 @@ func (s *mockRelationResolverSuite) TestHookRelationBroken(c *gc.C) {
 			1: {
 				Life: life.Dying,
 			},
+			2: {
+				Life: life.Dying,
+			},
 		},
 	}
-	relationState := relation.State{
+
+	defer s.setupMocks(c).Finish()
+
+	s.expectSyncScopes(remoteState)
+
+	relationState1 := relation.State{
 		RelationId:         1,
 		Members:            map[string]int64{},
 		ApplicationMembers: map[string]int64{},
 		ChangedPending:     "",
 	}
-	defer s.setupMocks(c).Finish()
-	s.expectSyncScopes(remoteState)
 	s.expectIsKnown(1)
 	s.expectIsImplicitFalse(1)
-	s.expectState(relationState)
-	s.expectIsPeerRelationFalse(1)
+	s.expectState(relationState1)
 	s.expectStateFound(1)
 	s.expectRemoteApplication(1, "")
+	s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil).Times(2)
+
+	// The `Relations` map in the remote state snapshot (as with all Go maps)
+	// has random iteration order. This will sometimes be called
+	// (relation ID 2 first) and sometimes not (ID 1 first). The test here is
+	// that in all cases, the next operation is for ID 1 (non-peer) - it is
+	// always enqueued ahead of ID 2, which is a peer relation.
+	s.mockRelStTracker.EXPECT().IsPeerRelation(2).Return(true, nil).MaxTimes(1)
 
 	relationsResolver := s.newRelationResolver(s.mockRelStTracker, s.mockSupDestroyer)
 	op, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{})
@@ -1543,10 +1562,11 @@ func (s *mockRelationResolverSuite) TestHookRelationBrokenWhenSuspended(c *gc.C)
 	s.expectIsKnown(1)
 	s.expectIsImplicitFalse(1)
 	s.expectState(relationState)
-	s.expectIsPeerRelationFalse(1)
 	s.expectStateFound(1)
 	s.expectRemoteApplication(1, "")
 
+	s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil).Times(2)
+
 	relationsResolver := s.newRelationResolver(s.mockRelStTracker, s.mockSupDestroyer)
 	op, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{})
 	c.Assert(err, jc.ErrorIsNil)
@@ -1577,9 +1597,10 @@ func (s *mockRelationResolverSuite) TestHookRelationBrokenOnlyOnce(c *gc.C) {
 	s.expectIsKnown(1)
 	s.expectIsImplicitFalse(1)
 	s.expectState(relationState)
-	s.expectIsPeerRelationFalse(1)
 	s.expectStateFoundFalse(1)
 
+	s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil).Times(2)
+
 	relationsResolver := s.newRelationResolver(s.mockRelStTracker, s.mockSupDestroyer)
 	_, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{})
 	c.Assert(errors.Cause(err), gc.Equals, resolver.ErrNoOperation)
@@ -1606,6 +1627,8 @@ func (s *mockRelationResolverSuite) TestImplicitRelationNoHooks(c *gc.C) {
 	s.expectIsKnown(1)
 	s.expectIsImplicit(1)
 
+	s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil)
+
 	relationsResolver := s.newRelationResolver(s.mockRelStTracker, s.mockSupDestroyer)
 	_, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{})
 	c.Assert(errors.Cause(err), gc.Equals, resolver.ErrNoOperation)
@@ -1643,13 +1666,14 @@ func (s *mockRelationResolverSuite) TestPrincipalDyingDestroysSubordinates(c *gc
 	s.expectIsKnown(1)
 	s.expectIsImplicitFalse(1)
 	s.expectState(relationState)
-	s.expectIsPeerRelationFalse(1)
 	s.expectHasContainerScope(1)
 	s.expectStateFound(1)
 	s.expectRemoteApplication(1, "")
 	destroyer := mocks.NewMockSubordinateDestroyer(ctrl)
 	destroyer.EXPECT().DestroyAllSubordinates().Return(nil)
 
+	s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil).Times(2)
+
 	relationsResolver := s.newRelationResolver(s.mockRelStTracker, destroyer)
 	op, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{})
 	c.Assert(err, jc.ErrorIsNil)
@@ -1698,11 +1722,6 @@ func (s *mockRelationResolverSuite) expectState(st relation.State) {
 	exp.State(st.RelationId).Return(&st, nil)
 }
 
-func (s *mockRelationResolverSuite) expectIsPeerRelationFalse(id int) {
-	exp := s.mockRelStTracker.EXPECT()
-	exp.IsPeerRelation(id).Return(false, nil)
-}
-
 func (s *mockRelationResolverSuite) expectLocalUnitAndApplicationLife() {
 	exp := s.mockRelStTracker.EXPECT()
 	exp.LocalUnitAndApplicationLife().Return(life.Alive, life.Alive, nil)