Merge #53275

53275: kv: avoid race in txnSpanRefresher.SendLocked r=nvanbenschoten a=nvanbenschoten

Fixes #53249.

The race was caused by the reintroduction of DeprecatedCanCommitAtHigherTimestamp
in #53220, mixed with the new splitEndTxnAndRetrySend logic introduced in #52885.

Co-authored-by: Nathan VanBenschoten <[email protected]>

pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go

@@ -166,7 +166,30 @@ func (sr *txnSpanRefresher) SendLocked(
 	ba.CanForwardReadTimestamp = sr.canForwardReadTimestampWithoutRefresh(ba.Txn)
 	if rArgs, hasET := ba.GetArg(roachpb.EndTxn); hasET {
 		et := rArgs.(*roachpb.EndTxnRequest)
-		et.DeprecatedCanCommitAtHigherTimestamp = ba.CanForwardReadTimestamp
+		// Assign the EndTxn's DeprecatedCanCommitAtHigherTimestamp flag if it
+		// isn't already set correctly. We don't write blindly because we could
+		// be dealing with a re-issued batch from splitEndTxnAndRetrySend after
+		// a refresh and we don't want to mutate previously issued requests or
+		// we risk a data race (checked by raceTransport). In these cases, we
+		// need to clone the EndTxn request first before mutating.
+		//
+		// We know this is a re-issued batch if the flag is already set and we
+		// need to unset it. We aren't able to detect the case where the flag is
+		// not set and we now need to set it to true, but such cases don't
+		// happen in practice (i.e. we'll never begin setting the flag after a
+		// refresh).
+		//
+		// TODO(nvanbenschoten): this is ugly. If we weren't about to delete
+		// this field, we'd want to do something better. Just delete this ASAP.
+		if et.DeprecatedCanCommitAtHigherTimestamp != ba.CanForwardReadTimestamp {
+			isReissue := et.DeprecatedCanCommitAtHigherTimestamp
+			if isReissue {
+				etCpy := *et
+				ba.Requests[len(ba.Requests)-1].SetInner(&etCpy)
+				et = &etCpy
+			}
+			et.DeprecatedCanCommitAtHigherTimestamp = ba.CanForwardReadTimestamp
+		}
 	}
 
 	// Attempt a refresh before sending the batch.