kvserver: leaked replica mu #106568

tbg · 2023-07-11T06:28:23Z

Describe the problem

In two instances, we saw a leaked replica mutex:

In both cases, the goroutine acquiring the mutex seems to have exited without releasing it. The second instance underwent an extensive attempt to reproduce the problem (~8k runs over a week) but the issue did not reoccur.

#106254 has a "deadlock detector" that also prints the stack trace of the mutex acquisition. However, it is likely too expensive to be always-on, especially for a hot mutex like Replica.mu.

Another angle could be #105366, i.e. proving through static analysis that all acquisitions are defer-unlocked (and thus in a deadlock scenario, the lock holder would still be around).

In my view we ought to only use a provably safe unlock pattern, so option 2 seems appealing, especially given the rarity of the bug.

Jira issue: CRDB-29618

blathers-crl · 2023-07-11T06:28:33Z

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

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

This one was possible hit. Interestingly, it would be "accidentally" fixed by cockroachdb#104657 as well, since after that PR we would never see a nil `status` here. Touches cockroachdb#106568

106574: kvserver: avoid leaked replica mutex during delegated snapshot r=erikgrinaker a=tbg Prompted by #106568, I went through all callers to `repl.mu.Lock` and `repl.mu.RLock` trying to find places where we're leaking the mutex on certain return paths. I found some, including one that at least seems to possibly explain what we saw in #106568. I also looked into the possibility of panics under a held lock being recovered from at higher levels in the stack, in particular this code: https://github.com/cockroachdb/cockroach/blob/81569be4aeb61620d2fd51c41b416f7d0986698e/pkg/sql/colexecerror/error.go#L27-L30 However, it seems sufficiently restricted to recovering only from errors it knows to be safe to recover from, and in particular it doesn't look like it will ever recover from any panic in kvserver. I will say that this looks a little brittle, though I don't see a much better way of doing what this code is trying to achieve. Release note: None Epic: None Co-authored-by: Tobias Grieger <[email protected]>

This one was possible hit. Interestingly, it would be "accidentally" fixed by cockroachdb#104657 as well, since after that PR we would never see a nil `status` here. Touches cockroachdb#106568

tbg · 2023-07-17T15:11:55Z

Fixed, presumably, in #106574.

tbg added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-kv KV Team labels Jul 11, 2023

tbg added the release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. label Jul 11, 2023

This was referenced Jul 11, 2023

lint: add check to ensure mutex Unlock() calls are deferred #105366

Closed

kvserver: avoid leaked replica mutex during delegated snapshot #106574

Merged

tbg mentioned this issue Jul 11, 2023

roachtest: failover/chaos/read-only failed #106108

Closed

nicktrav added the branch-master Failures and bugs on the master branch. label Jul 11, 2023

exalate-issue-sync bot assigned tbg Jul 13, 2023

tbg mentioned this issue Jul 14, 2023

release-23.1: kvserver: avoid leaked replica mutex during delegated snapshot #106807

Merged

tbg closed this as completed Jul 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvserver: leaked replica mu #106568

kvserver: leaked replica mu #106568

tbg commented Jul 11, 2023 •

edited

Loading

blathers-crl bot commented Jul 11, 2023

tbg commented Jul 17, 2023

kvserver: leaked replica mu #106568

kvserver: leaked replica mu #106568

Comments

tbg commented Jul 11, 2023 • edited Loading

blathers-crl bot commented Jul 11, 2023

tbg commented Jul 17, 2023

tbg commented Jul 11, 2023 •

edited

Loading