Praefect: Cancel voters with no vote

Currently voters can only be canceled if they have cast a vote.
Voters that have note yet voted should also have the ability to be
canceled in the event of a node RPC failure. This change updates
`updateVoterState()` to be able to cancel a voter and accurately track
vote counts regardless of whether the voter has voted.

2 files changed, 132 insertions, 9 deletions

diff --git a/internal/praefect/transactions/subtransaction.go b/internal/praefect/transactions/subtransaction.go
index b0d80ff1c..92d370ea9 100644
--- a/internal/praefect/transactions/subtransaction.go
+++ b/internal/praefect/transactions/subtransaction.go
@@ -79,7 +79,9 @@ func (t *subtransaction) stop() error {
 		switch voter.result {
 		case VoteCanceled:
 			// If the vote was canceled already, we cannot stop it.
-			return ErrTransactionCanceled
+			// A single node voter being canceled is not indicative
+			// of all voter's state. Other voters must be checked.
+			continue
 		case VoteStopped:
 			// Similar if the vote was stopped already.
 			return ErrTransactionStopped
@@ -136,9 +138,9 @@ func (t *subtransaction) vote(node string, vote voting.Vote) error {
 	return nil
 }
 
-// updateVoterStates updates undecided voters or cancels existing votes of decided voters if given a
-// `nil` vote. Voters are updated either as soon as quorum was reached or alternatively when all
-// votes were cast.
+// updateVoterState updates undecided voters or cancels existing votes if given a `nil`
+// vote. Voters are updated either as soon as quorum was reached or alternatively when
+// quorum becomes impossible.
 func (t *subtransaction) updateVoterState(voter *Voter, vote *voting.Vote) error {
 	switch voter.result {
 	case VoteUndecided:
@@ -171,10 +173,20 @@ func (t *subtransaction) updateVoterState(voter *Voter, vote *voting.Vote) error
 			return errors.New("subtransaction was already finished")
 		}
 
+		// A voter's result can only be canceled if the subtransaction is still pending.
+		// If a change has already been committed to disk the voter result cannot be
+		// changed since the subtransction is considered complete.
+		voter.result = VoteCanceled
+
 		// Remove the voter's support for the vote so it's not counted towards the
 		// majority. The node is not going to commit the subtransaction anyway.
-		t.voteCounts[*voter.vote] -= voter.Votes
-		voter.result = VoteCanceled
+		if voter.vote != nil {
+			t.voteCounts[*voter.vote] -= voter.Votes
+		}
+
+		// A canceled voter can no longer voter so its vote is
+		// reset after being subtracted from the vote counts.
+		voter.vote = nil
 	}
 
 	defer func() {
diff --git a/internal/praefect/transactions/subtransaction_test.go b/internal/praefect/transactions/subtransaction_test.go
index faa6e39d0..5ba4e8476 100644
--- a/internal/praefect/transactions/subtransaction_test.go
+++ b/internal/praefect/transactions/subtransaction_test.go
@@ -51,7 +51,7 @@ func TestSubtransaction_stop(t *testing.T) {
 		require.Equal(t, VoteFailed, s.votersByNode["3"].result)
 	})
 
-	t.Run("stop of canceled transaction fails", func(t *testing.T) {
+	t.Run("stop of transaction with single canceled voter", func(t *testing.T) {
 		s, err := newSubtransaction([]Voter{
 			{Name: "1", Votes: 1, result: VoteUndecided},
 			{Name: "2", Votes: 1, result: VoteCommitted},
@@ -60,8 +60,8 @@ func TestSubtransaction_stop(t *testing.T) {
 		}, 1)
 		require.NoError(t, err)
 
-		require.Equal(t, s.stop(), ErrTransactionCanceled)
-		require.False(t, s.isDone())
+		require.NoError(t, s.stop())
+		require.True(t, s.isDone())
 	})
 
 	t.Run("stop of stopped transaction fails", func(t *testing.T) {
@@ -603,6 +603,117 @@ func TestSubtransaction_race(t *testing.T) {
 	}
 }
 
+func TestSubtransaction_updateVoterState(t *testing.T) {
+	voters := []Voter{
+		{Name: "1", Votes: 1},
+		{Name: "2", Votes: 1},
+		{Name: "3", Votes: 1},
+	}
+	threshold := uint(2)
+
+	vote := newVote(t, "a")
+
+	for _, tc := range []struct {
+		desc      string
+		voter     *Voter
+		vote      *voting.Vote
+		expVote   *voting.Vote
+		expVotes  uint
+		expResult VoteResult
+		expErrMsg string
+	}{
+		{
+			desc: "Update voter",
+			voter: &Voter{
+				Name: "1", Votes: 1, vote: nil, result: VoteUndecided,
+			},
+			vote:      &vote,
+			expVote:   &vote,
+			expVotes:  1,
+			expResult: VoteUndecided,
+			expErrMsg: "",
+		},
+		{
+			desc: "Cancel voter that has not voted",
+			voter: &Voter{
+				Name: "1", Votes: 1, vote: nil, result: VoteUndecided,
+			},
+			vote:      nil,
+			expVote:   nil,
+			expVotes:  0,
+			expResult: VoteCanceled,
+			expErrMsg: "",
+		},
+		{
+			desc: "Cancel voter that has voted",
+			voter: &Voter{
+				Name: "1", Votes: 1, vote: &vote, result: VoteUndecided,
+			},
+			vote:      nil,
+			expVote:   nil,
+			expVotes:  0,
+			expResult: VoteCanceled,
+			expErrMsg: "",
+		},
+		{
+			desc: "Update canceled voter",
+			voter: &Voter{
+				Name: "1", Votes: 1, vote: nil, result: VoteCanceled,
+			},
+			vote:      nil,
+			expVote:   nil,
+			expVotes:  0,
+			expResult: VoteCanceled,
+			expErrMsg: "transaction has been canceled",
+		},
+		{
+			desc: "Update canceled voter",
+			voter: &Voter{
+				Name: "1", Votes: 1, vote: nil, result: VoteStopped,
+			},
+			vote:      nil,
+			expVote:   nil,
+			expVotes:  0,
+			expResult: VoteStopped,
+			expErrMsg: "transaction has been stopped",
+		},
+		{
+			desc: "Update committed voter",
+			voter: &Voter{
+				Name: "1", Votes: 1, vote: nil, result: VoteCommitted,
+			},
+			vote:      nil,
+			expVote:   nil,
+			expVotes:  0,
+			expResult: VoteCommitted,
+			expErrMsg: "cannot change committed vote",
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			subtransaction, err := newSubtransaction(voters, threshold)
+			require.NoError(t, err)
+
+			if tc.voter.vote != nil {
+				subtransaction.voteCounts[*tc.voter.vote] += tc.voter.Votes
+			}
+
+			err = subtransaction.updateVoterState(tc.voter, tc.vote)
+			if tc.expErrMsg != "" {
+				require.Equal(t, tc.expErrMsg, err.Error())
+			} else {
+				require.NoError(t, err)
+			}
+
+			require.Equal(t, tc.expVote, tc.voter.vote)
+			require.Equal(t, tc.expResult, tc.voter.result)
+
+			if tc.voter.vote != nil {
+				require.Equal(t, tc.expVotes, subtransaction.voteCounts[*tc.voter.vote])
+			}
+		})
+	}
+}
+
 func TestSubtransaction_quorumCheck(t *testing.T) {
 	voteA := newVote(t, "a")
 	voteB := newVote(t, "b")