kvserver: unhandled TransactionPushError when push finds STAGING but known-uncommitted Read Committed transaction

[tbg]

Describe the problem

In #97779 we're introducing reproposals and reproposal errors in kvnemesis.

For some reason, under stress, kvnemesis will now sometimes find a *TransactionPushError bubble up from db.Get and db.Scan (and likely other methods), which should not be possible.

@arulajmani is looking at this in https://cockroachlabs.slack.com/archives/G01G8LK77DK/p1687356168950489.

Jira issue: CRDB-28983

[tbg]

I am merging d7ae4d9 now, which works around this issue - please revert when this issue is addressed.

[nvanbenschoten]

In the test, we see a pusher request (always non-txn'al, but it's not clear that this matters) issue a PushTxnRequest(PUSH_TIMESTAMP) against a weak isolation transaction. Since 5a53d25, these pushes should be able to push the pushee's timestamp. However, we hit this logic:

cockroach/pkg/kv/kvserver/batcheval/cmd_push_txn.go

Lines 216 to 244 in 972c379

	// If the pusher is aware that the pushee's currently recorded attempt at a
	// parallel commit failed, either because it found intents at a higher
	// timestamp than the parallel commit attempt or because it found intents at
	// a higher epoch than the parallel commit attempt, it should not consider
	// the pushee to be performing a parallel commit. Its commit status is not
	// indeterminate.
	if (knownHigherTimestamp || knownHigherEpoch) && reply.PusheeTxn.Status == roachpb.STAGING {
	reply.PusheeTxn.Status = roachpb.PENDING
	reply.PusheeTxn.InFlightWrites = nil
	// If the pusher is aware that the pushee's currently recorded attempt
	// at a parallel commit failed, upgrade PUSH_TIMESTAMPs to PUSH_ABORTs.
	// We don't want to move the transaction back to PENDING, as this is not
	// (currently) allowed by the recovery protocol. We also don't want to
	// move the transaction to a new timestamp while retaining the STAGING
	// status, as this could allow the transaction to enter an implicit
	// commit state without its knowledge, leading to atomicity violations.
	//
	// This has no effect on pushes that fail with a TransactionPushError.
	// Such pushes will still wait on the pushee to retry its commit and
	// eventually commit or abort. It also has no effect on expired pushees,
	// as they would have been aborted anyway. This only impacts pushes
	// which would have succeeded due to priority mismatches. In these
	// cases, the push acts the same as a short-circuited transaction
	// recovery process, because the transaction recovery procedure always
	// finalizes target transactions, even if initiated by a PUSH_TIMESTAMP.
	if pushType == kvpb.PUSH_TIMESTAMP {
	pushType = kvpb.PUSH_ABORT
	}
	}

So we promote the PUSH_TIMESTAMP (which would be allowed) into a PUSH_ABORT (which is not), so command evaluation throws a TransactionPushError. We then catch this error and determine whether to wait in the txnWaitQueue:

cockroach/pkg/kv/kvserver/replica_send.go

Lines 769 to 780 in 972c379

	// On a transaction push error, retry immediately if doing so will enqueue
	// into the txnWaitQueue in order to await further updates to the unpushed
	// txn's status. We check ShouldPushImmediately to avoid retrying
	// non-queueable PushTxnRequests (see #18191).
	dontRetry := r.store.cfg.TestingKnobs.DontRetryPushTxnFailures
	if !dontRetry && ba.IsSinglePushTxnRequest() {
	pushReq := ba.Requests[0].GetInner().(*kvpb.PushTxnRequest)
	dontRetry = txnwait.ShouldPushImmediately(pushReq)
	}
	if dontRetry {
	return g, pErr
	}

Since we once again determine that we should be able to perform the PUSH_TIMESTAMP (this code doesn't know about the PUSH_TIMESTAMP -> PUSH_ABORT promotion), we throw the error up to the caller.