From 1fb8d8797ee37c8ed3e039ee55225f5e6b2c35f4 Mon Sep 17 00:00:00 2001 From: Tobias Schottdorf Date: Wed, 6 Feb 2019 09:49:13 +0100 Subject: [PATCH] storage: don't leak committed protos to pushers on reproposal A recent commit (master only) reintroduced a bug that we ironically had spent a lot of time on [before]. In summary, it would allow the result of an EndTransaction which would in itself *not* apply to leak and would result in intents being committed even though their transaction ultimately would not: https://github.com/cockroachdb/cockroach/issues/34025#issuecomment-460934278 We've diagnosed this pretty quickly the second time around, but clearly we didn't do a good job at preventing the regression. I can see how this would happen as the method this code is in is notoriously difficult to test for it interfaces so much with everything else that it's difficult to unit test it; one needs to jump through lots of hoops to target it, and so we do it less than we ought to. I believe this wasn't released in any alpha (nor backported anywhere), so no release note is necessary. Fixes #34025. [before]: https://github.com/cockroachdb/cockroach/issues/30792#issuecomment-435856280 Release note: None --- pkg/storage/replica_raft.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/storage/replica_raft.go b/pkg/storage/replica_raft.go index fd0698ead8f4..64ec0f671a21 100644 --- a/pkg/storage/replica_raft.go +++ b/pkg/storage/replica_raft.go @@ -1927,7 +1927,7 @@ func (r *Replica) processRaftCommand( log.Fatalf(ctx, "proposal must return either a reply or an error: %+v", proposal) } response.Intents = proposal.Local.DetachIntents() - response.EndTxns = proposal.Local.DetachEndTxns(response.Err != nil) + response.EndTxns = proposal.Local.DetachEndTxns(pErr != nil) if pErr == nil { lResult = proposal.Local }