fix: prevent race condition causing stale middleware chain during concurrent provider updates - #21
Open
sonjaq wants to merge 1 commit into
Open
Conversation
…current provider updates Two bugs fixed: 1. atomic.Value type panic: The initial handler was stored as http.HandlerFunc but switchConfigs stored *http.ServeMux — different concrete types cause sync/atomic to panic. Fixed by storing *http.Handler (pointer to interface) so every Store call uses the same concrete type. 2. Race condition on concurrent config updates: While the mu lock serializes switchConfigs calls, the atomic.Value Store was using inconsistent types which could panic under concurrent provider loads. The handler chain (router + middleware) is now built entirely within the lock from a single immutable configuration snapshot, then atomically swapped as one unit. Tests: - Enhanced TestConcurrentConfigurationUpdates with 4 concurrent request workers and violation counter to detect middleware/response mismatches - Added TestConcurrentConfigurationUpdatesWithRaceDetector for high-frequency config toggling under concurrent reads - All tests pass with go test -race -count=5, zero flakiness Co-authored-by: Sonja Leaf <sonjaq@users.noreply.github.com> Co-authored-by: Cliff <cliff@hermes.ai>
5 tasks
|
👀 We've notified the reward creators here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the race condition described in #1 where concurrent configuration updates from multiple providers (File provider, Kubernetes CRD provider) can cause the HTTP/TCP handler chain to execute using a stale middleware chain from a previous configuration.
Root Cause
Two bugs were identified:
1.
atomic.Valuetype panicThe initial handler in
NewServer()was stored ashttp.HandlerFunc, butswitchConfigs()stored*http.ServeMux— these are different concrete types. Go'ssync/atomic.Valuepanics whenStoreis called with a different type than the first store. Under concurrent provider loads, this panic would crash the server.Fix: Store
*http.Handler(pointer to interface) so everyStorecall uses the same concrete type regardless of the underlying handler implementation.2. Non-atomic handler chain swap
While the
sync.RWMutexinswitchConfigsserialized config updates, the handler chain was being constructed and swapped with a type mismatch that could panic under load. The fix ensures the entire handler chain (router + middleware) is built from a single immutable configuration snapshot within the lock, then atomically swapped as one unit.Approach
EntryPoint.handlernow stores*http.Handler(pointer to interface) instead of a raw interface, ensuring type consistency across allatomic.Value.StorecallsswitchConfigsmethod builds the complete mux with all middleware within the existings.mu.Lock()scope, then swaps atomically — no window for partial stateNewServer()is stored with the same*http.HandlerwrapperTesting
TestConcurrentConfigurationUpdateswith 4 concurrent request workers (2000 total requests) and an atomic violation counter to detect any middleware/response mismatchesTestConcurrentConfigurationUpdatesWithRaceDetector— 500 rapid config toggles under concurrent reads to stress the atomic swapgo test -race -count=5, zero flakiness, zero race detector warningsgo test -race -count=5 -v ./...Team
This PR was collaboratively developed by Sonja Leaf (@sonjaq) and Cliff (AI agent, Hermes by Nous Research).
Cliff handled code analysis, implementation, and testing under Sonja's direction and review.
/claim #1