coordinator: Explicitly exit early if primary is not dirty

Under some circumstances, the primary node will not be considered dirty
after a transactional mutator. No matter what, we do not have to
schedule any replication jobs in these cases given that there shouldn't
be any changes anyway. But even though we know early on that the primary
is not dirty, we still collect updated and outdated nodes and return
them to the caller. This code flow is a bit confusing and hard to reason
about.

Refactor the code to return early in case the primary is not dirty.

(cherry picked from commit c12022da6d1c65bebd303786a7abcf04ee7d67b3)

2 files changed, 18 insertions, 21 deletions

diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index d10a5f51e..5207301bd 100644
--- a/internal/praefect/coordinator.go
+++ b/internal/praefect/coordinator.go
@@ -785,26 +785,25 @@ func getUpdatedAndOutdatedSecondaries(
 
 	primaryErr := nodeErrors.errByNode[route.Primary.Storage]
 
-	// If there were subtransactions, we only assume some changes were made if one of the subtransactions
-	// was committed.
-	//
-	// If there were no subtransactions, we assume changes were performed only if the primary successfully
-	// processed the RPC. This might be an RPC that is not correctly casting votes thus we replicate everywhere.
-	//
-	// If there were no subtransactions and the primary failed the RPC, we assume no changes have been made and
-	// the nodes simply failed before voting.
-	primaryDirtied = transaction.DidCommitAnySubtransaction() ||
-		(transaction.CountSubtransactions() == 0 && primaryErr == nil)
+	// If there were no subtransactions and the primary failed the RPC, we assume no changes
+	// have been made and the nodes simply failed before voting. We can thus return directly and
+	// notify the caller that the primary is not considered to be dirty.
+	if transaction.CountSubtransactions() == 0 && primaryErr != nil {
+		return false, nil, nil
+	}
+
+	// If there were subtransactions, we assume no changes were made if none of the
+	// subtransactions was committed. This can really only happen for the first subtransaction
+	// given that no new subtransactions are created if the first one wasn't committed. Same as
+	// above, we exit early in this case and notify the caller that the primary is not dirty.
+	if transaction.CountSubtransactions() != 0 && !transaction.DidCommitAnySubtransaction() {
+		return false, nil, nil
+	}
+
+	primaryDirtied = true
 
 	nodesByState := make(map[string][]string)
 	defer func() {
-		// If the primary wasn't dirtied, then we never replicate any changes and thus
-		// shouldn't log nodes as outdated here. While this is duplicates logic defined
-		// elsewhere, it's probably good enough given that we only talk about metrics here.
-		if !primaryDirtied {
-			return
-		}
-
 		ctxlogrus.Extract(ctx).
 			WithField("transaction.primary", route.Primary.Storage).
 			WithField("transaction.secondaries", nodesByState).
diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go
index cf63e70b9..ddd32bf63 100644
--- a/internal/praefect/coordinator_test.go
+++ b/internal/praefect/coordinator_test.go
@@ -1600,14 +1600,12 @@ func TestGetUpdatedAndOutdatedSecondaries(t *testing.T) {
 			},
 		},
 		{
-			desc: "single failing node with replica",
+			desc: "single failing node with replica is not considered modified",
 			primary: node{
 				name:  "primary",
 				state: transactions.VoteFailed,
 			},
-			replicas:         []string{"replica"},
-			subtransactions:  1,
-			expectedOutdated: []string{"replica"},
+			subtransactions: 1,
 		},
 		{
 			desc: "single erred node with replica",