kv: spurious ambiguous errors can be returned

[andrewbaptist]

Describe the problem

The test kv/gracefuldraining fails without disabling the max gossip frequency.

Customers expect to not hit ambiguous errors during clean drains and shutdowns. This is a somewhat expected and rare error and we expect customers to retry, but it is still a regression in behavior.

To Reproduce

Remove the --tolerate-errors line from the test. It will fail ~50% of the time.

Expected behavior
Customers expect to not hit ambiguous errors during clean drains and shutdowns. This is a somewhat expected and rare error and we expect customers to retry, but it is still a regression in behavior.

Jira issue: CRDB-41538

Epic CRDB-39956

[blathers-crl]

Hi @andrewbaptist, please add branch-* labels to identify which branch(es) this C-bug affects.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[andrewbaptist]

There are a few different failure modes. I'll put them in different commennts.

[andrewbaptist]

Failure mode 1:

1. DistSender has a stale cache (s1, s2, s3). The current range is on s1, s4, s5.
2. sendToReplicas attempts the three in the cache for a Put / EndTxn and correctly receives replica not found from s2 and s3 and creates an AmbiguousError from s1 (since it doesn't respond).
3. sendPartialBatch doesn’t retry the batch request since it wasn’t a SendError

There are a few ways to handle this, but the fundamental problem is the “contract” between sendToReplicas and sendPartialBatch. Specifically sendToReplicas stops retrying if the descriptor changes, but still returns the “worst” error which in this case was an ambiguous error. sendPartialBatch will never retry ambiguous errors.

The current contract between these methods is too weak. Specifically in the case of both a RangeMismatchError and an AmbiguousError

Some options:

• If we get both types of errors, return the RangeMismatchError, and modify the “original” BatchRequest to sendToReplicas to set CanForwardReadTimestamp rather than the cloned copy
• Return “all” errors from sendToReplicas and move the processing of selectBestError up a level. Currently we are mixing the “best error” with “should retry” and losing information.

[andrewbaptist]

Failure mode 2:

1. DistSender has a stale cache (s1, s2, s3). The current range is on s1, s2, s5. with s5 as the leaseholder.
2. sendToReplicas attempts the three in the cache for aPut / EndTxn and receives RangeMismatchError from s3 and NotLeaseHolderError from s2 and creates an AmbiguousError from s1 (since it doesn't respond).
3. sendPartialBatch doesn’t retry the batch request since it wasn’t a SendError

The rest of the analysis is basically the same.