fix: JetStream consumer lock leak on start sequence error#8230
fix: JetStream consumer lock leak on start sequence error#8230sanbricio wants to merge 1 commit into
Conversation
MauriceVanVeen
left a comment
There was a problem hiding this comment.
Thanks!
Could you add sign-off to your commit? See also: CONTRIBUTING.md#sign-off
…hAssignment Signed-off-by: Santiago Bricio Rojas <sanbriciorojas11@gmail.com>
69245ae to
a1d54d3
Compare
|
@MauriceVanVeen Hi!, thanks for the fast review, sign-off added! |
|
@claude, review. |
|
Claude finished @MauriceVanVeen's task in 2m 37s —— View job Code Review
Fix is correct — the mutex leak is real and the placement of the new
One additional concern — potential resource leak All the other error returns that occur after the consumer object // e.g. line 1213-1216
mset.mu.Unlock()
o.deleteWithoutAdvisory()
return nil, NewJSConsumerNameTooLongError(JSMaxNameLen)
// e.g. line 1251-1254
mset.mu.Unlock()
o.deleteWithoutAdvisory()
return nil, NewJSConsumerBadDurableNameError()
// e.g. line 1259-1263
mset.mu.Unlock()
o.deleteWithoutAdvisory()
return nil, NewJSConsumerStoreFailedError(err)The
Without Suggested fix: Fix this → if err := o.selectStartingSeqNo(); err != nil {
mset.mu.Unlock()
o.deleteWithoutAdvisory() // clean up client, sysc, store, queues
return nil, err
}Minor: no error wrapping Other consumer-creation errors are wrapped with Summary: The mutex unlock fix is correct and addresses the reported deadlock. Consider also adding |
Summary
This PR fixes a lock leak in JetStream consumer creation.
In
addConsumerWithAssignment,mset.muis held while creating a consumer. In the direct/standalone path, ifo.selectStartingSeqNo()returns an error, the function currently returns without releasingmset.mu.This adds the missing
mset.mu.Unlock()before returning the error.Resolves #8229
Impact
Without this unlock, the stream mutex can remain locked after a starting sequence error, which may cause later operations on the same stream to block.
Changes
mset.mu.Unlock()before returning from theselectStartingSeqNo()error path.Detection
This issue was reported by
goconcurrencylint.Testing
Signed-off-by: Santiago Bricio sanbriciorojas11@gmail.com