NRG: Leader resurrects removed peers on restarts

server/raft.go

@@ -953,16 +953,20 @@ func (n *raft) ProposeAddPeer(peer string) error {
 // As a leader if we are proposing to remove a peer assume its already gone.
 func (n *raft) doRemovePeerAsLeader(peer string) {
 	n.Lock()
+	defer n.Unlock()
+	n.doRemovePeerLocked(peer)
+}
+
+func (n *raft) doRemovePeerLocked(peer string) {
 	if n.removed == nil {
 		n.removed = map[string]time.Time{}
 	}
 	n.removed[peer] = time.Now()
 	if _, ok := n.peers[peer]; ok {
 		delete(n.peers, peer)
-		// We should decrease our cluster size since we are tracking this peer and the peer is most likely already gone.
 		n.adjustClusterSizeAndQuorum()
+		n.writePeerState(&peerState{n.peerNames(), n.csz, n.extSt})
 	}
-	n.Unlock()
 }
 
 // ProposeRemovePeer is called to remove a peer from the group.
@@ -3218,19 +3222,7 @@ func (n *raft) applyCommit(index uint64) error {
 			peer := string(e.Data)
 			n.debug("Removing peer %q", peer)
 
-			// Make sure we have our removed map.
-			if n.removed == nil {
-				n.removed = make(map[string]time.Time)
-			}
-			n.removed[peer] = time.Now()
-
-			if _, ok := n.peers[peer]; ok {
-				delete(n.peers, peer)
-				// We should decrease our cluster size since we are tracking this peer.
-				n.adjustClusterSizeAndQuorum()
-				// Write out our new state.
-				n.writePeerState(&peerState{n.peerNames(), n.csz, n.extSt})
-			}
+			n.doRemovePeerLocked(peer)
 
 			// Remove from string intern map.
 			peers.Delete(peer)

server/raft_test.go

@@ -4344,3 +4344,38 @@ func TestNRGProposeRemovePeerLeader(t *testing.T) {
 	require_Equal(t, len(newLeader.node().Peers()), 2)
 	require_False(t, newLeader.node().MembershipChangeInProgress())
 }
+
+func TestNRGLeaderResurrectsRemovedPeers(t *testing.T) {
+	c := createJetStreamClusterExplicit(t, "R3S", 3)
+	defer c.shutdown()
+
+	rg := c.createMemRaftGroup("TEST", 3, newStateAdder)
+	rg.waitOnLeader()
+
+	leader := rg.leader()
+	followers := rg.followers()
+	require_Equal(t, len(followers), 2)
+
+	// Remove one follower
+	require_NoError(t, leader.node().ProposeRemovePeer(followers[0].node().ID()))
+	checkFor(t, 1*time.Second, 10*time.Millisecond, func() error {
+		if leader.node().MembershipChangeInProgress() {
+			return errors.New("membership still in progress")
+		} else {
+			return nil
+		}
+	})
+
+	require_Equal(t, len(leader.node().Peers()), 2)
+
+	// Stop the leader and restart it.
+	// If bug is present: the leader resurrects the previously removed peer.
+	leader.stop()
+	followers[1].stop()
+
+	leader.restart()
+	require_Equal(t, len(leader.node().Peers()), 2)
+
+	followers[1].restart()
+	require_Equal(t, len(leader.node().Peers()), 2)
+}