fix: add mutex locking to KeyRegistry to prevent data races#1905
Conversation
KeyRegistry embedded sync.Mutex but only getCert() held it. Background key rotation goroutines write keys/mostRecentKey while HTTP handlers read them concurrently, causing data races. Add mutex acquisition to registerNewKey, latestPrivateKey, and new accessor methods (privateKeys, keyLen, mostRecentKeyTime). Replace direct field access in controller and main with thread-safe accessors. Signed-off-by: John Allberg <john@ayoy.se>
smuda
force-pushed
the
fix/keyregistry-race-condition
branch
from
7c80c58 to
854cecd
Compare
|
This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution. |
|
Closing this after the MR went stale. |
|
@smuda Thank you for your contribution! I want to be transparent: this is a project I work on in my free time, so reviewing and merging PRs takes longer than it would in a fully staffed environment. We also follow a monthly release cadence, which means there is a fair amount of work to coordinate each cycle. If you need to close the PR from your side, I completely understand — but I would be genuinely sad to lose the contribution. I hope you can bear with me and give it a little more time. This is an open source project maintained with a lot of care and passion, and every contribution matters. Thank you for your patience and for being part of the community! |
|
@alvneiayu Of course I can reopen the PR! I just thought that you deemed it uninteresting but didn't want to say so. I do have a problem with managing my PRs and don't want to lose track of them, being unresponsive to the maintainers. |
|
This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution. |
alvneiayu
added
backlog
2 participants
Description of the change
KeyRegistry embedded sync.Mutex but only getCert() held it. Background key rotation goroutines write keys/mostRecentKey while HTTP handlers read them concurrently, causing data races. Add mutex acquisition to registerNewKey, latestPrivateKey, and new accessor methods (privateKeys, keyLen, mostRecentKeyTime). Replace direct field access in controller and main with thread-safe accessors.
Benefits
No data race.
Possible drawbacks
It's possible to end up with lower throughput, which is the reason I changed mutex to RWMutex.
The embedded sync.Mutex was promoted, meaning external code could call kr.Lock() directly. The named mu field removes that.
Applicable issues