search

cockroachdb / cockroach

CockroachDB - the open source, cloud-native distributed SQL database.
https://www.cockroachlabs.com
Other
29.52k stars 3.7k forks source link

kvserver: prepareLocalResult is brittle and under-tested #105269

Open tbg opened 1 year ago

tbg commented 1 year ago

When, during proposal application, we attempt a reproposal but this fails, we signal the proposal with a (final) error. However, we fail to clear the local side effects. I think this can cause intents to be resolved that aren't committed yet. TestFailureToProcessCommandClearsLocalResult` exercises this general path, except not with the failing reproposal - it lets the reproposal succeed and simply verifies that the original command's local result is cleared. But if the reproposal fails, a different path is taken and the local result is not cleared, but it ought to be.

We should update our test coverage and fix this bug.

Found in #97779

Jira issue: CRDB-28955

Epic CRDB-39898

blathers-crl[bot] commented 1 year ago

cc @cockroachdb/replication

tbg commented 1 year ago

Reading the master code, I was wrong - there is no bug, I introduced (and caught) it on https://github.com/cockroachdb/cockroach/pull/97779. But the code is pretty brittle and the test coverage is missing so leaving this issue open to track that.

blathers-crl[bot] commented 10 months ago

Hi @erikgrinaker, please add branch-* labels to identify which branch(es) this release-blocker affects.

:owl: Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

erikgrinaker commented 10 months ago

Marking a release blocker to make sure we add test coverage and catch any lingering bugs here before the release.

erikgrinaker commented 8 months ago

We won't have time to get to this, dropping GA-blocker.