Conversation
handleReset deleted from the dynamicEndpoints map without holding the mutex, racing against handleDefault's reads. computeEndpointKey mutated a package-global maphash.Hash on every request outside any lock, so concurrent calls interleaved Reset/Write and could return a key derived from another request's method and URL. Replaced the shared hash with a plain string key. Tests now run with -race; without it neither race was detectable in CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
theseion
requested changes
Aug 18, 2026
| }() | ||
| go func() { | ||
| defer wg.Done() | ||
| response, err := client.Get(server.URL + "/concurrent") |
Contributor
There was a problem hiding this comment.
Wouldn't the error be returned here? 404, since reset was called before.
Member
Author
There was a problem hiding this comment.
No — that path never 404s. Everything unmatched falls through to handleDefault on /, which is the black hole: it writes an empty 200. I verified it against a live Handler() with /concurrent unconfigured and got 200.
So the only two outcomes here are 201 while the configuration is live and 200 once /reset has discarded it, depending on how the goroutines interleave. The test was ignoring the status entirely, which is what made the question reasonable — it now asserts the response is one of those two, so a genuine 404 or 500 regression would fail it. Pushed as a follow-up commit.
The unconfigured case falls through to the catch-all handler and returns 200, never 404, so the request can only be 200 or 201 depending on whether /reset has run yet. Assert that rather than ignoring the status. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Two data races on the package-global endpoint configuration state, both reachable from concurrent requests.
handleResetdeleted fromdynamicEndpointswithout holdingdynamicEndpointMutex, racing against the reads inhandleDefault. It now takes the write lock and usesclear().computeEndpointKeymutated a package-globalmaphash.Hash—Reset,WriteString,Sum64— on every request, entirely outside the lock. Concurrent calls interleave those steps, so a key could be computed from another request's method and URL, matching the wrong configured endpoint or missing one. The shared hash is gone; the key is nowmethod + " " + urlanddynamicEndpointsis keyed by string.go testruns with-racenow. Without it neither race was detectable in CI.TestConcurrentConfigurationAccessdrives/configure_reflection,/reset, and a configured endpoint concurrently. Verified it reports DATA RACE with either fix reverted individually, and passes with both applied.🤖 Generated with Claude Code