fix(agent): resolve Connect CA data race, cache watcher timer leak, and FSM restore error check - #23854
fix(agent): resolve Connect CA data race, cache watcher timer leak, and FSM restore error check#23854mandar1045 wants to merge 2 commits into
Conversation
…store error handling - leader_connect_ca.go: Hold stateLock across primaryRoots.Roots iteration in secondaryGetActivePrimaryCARoot() to avoid data race. - watch.go: Replace time.After with time.NewTimer and timer.Stop() in notifyBlockingQuery and notifyPollingQuery to prevent memory leaks on context cancellation. - fsm.go: Check return error of storageRestoration.Commit() in FSM.Restore() before swapping state store pointer.
|
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes Have you signed the CLA already but the status is still pending? Recheck it. |
1 similar comment
|
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes Have you signed the CLA already but the status is still pending? Recheck it. |
|
Hi team! CLA signed and changelog entry added. Really excited to contribute to Consul! Appreciate your time reviewing this PR whenever you get a chance—happy to make any adjustments or add more tests if needed! |
Summary
Found a few issues going through the connect CA / cache / raft paths, fixing all three here since they're small and unrelated enough that separate PRs felt like overkill.
Data race in Connect CA -
secondaryGetActivePrimaryCARoot()inagent/consul/leader_connect_ca.gowas releasingstateLockbefore iterating overc.primaryRoots.Roots. If a root update came in mid-iteration you'd get a data race. Fixed by holding the lock through the iteration.Timer leak in cache watchers -
notifyBlockingQueryandnotifyPollingQueryinagent/cache/watch.gowere usingtime.After, which doesn't get cleaned up if the watch context gets canceled before the timer fires. Under enough watch churn this leaks. Swapped totime.NewTimerwith an explicittimer.Stop().Unchecked error in Raft FSM -
FSM.Restore()inagent/consul/fsm/fsm.gowasn't checking the error fromstorageRestoration.Commit()before swapping in the new state store pointer. So a failed commit could silently leave you with a bad/partial state store. Now it bails out on the error instead of swapping pointers.None of these are huge, but all three felt worth fixing together since they touch stuff that's easy to overlook until it bites you under load.
Testing
go test -race ./agent/consul/go test -race ./agent/cache/Ran both a handful of times to make sure the race fix actually holds up and not just passing by luck.