Skip to content

Commit

Permalink
storage: don't leak committed protos to pushers on reproposal
Browse files Browse the repository at this point in the history
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:

cockroachdb#34025 (comment)

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 cockroachdb#34025.

[before]: cockroachdb#30792 (comment)

Release note: None
  • Loading branch information
tbg committed Feb 6, 2019
1 parent b924283 commit 1fb8d87
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion pkg/storage/replica_raft.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down

0 comments on commit 1fb8d87

Please sign in to comment.