sdk-go: single-flight discovery resolve + reject invalid ports#376
Merged
Conversation
…r probes Concurrent Resolver.Resolve calls for the same address previously raced: each goroutine that missed the in-process memo would probe the network independently, producing N redundant HTTP requests and N rename races on the on-disk cache file. Add an inflight map[string]*resolveCall guarded by the existing Resolver mutex. The first caller for an address is elected to run the disk-check + probe + cache.put path; concurrent callers register as waiters, block on the elected probe's result, and receive the same NodeInfo or the same error. Waiters respect their own context. Failed probes are not memoized so a subsequent call retries the network. The split mirrors Python's resolve() / _resolve_uncached() shape so the two SDKs stay in sync. Tests mirror sdk/python/tests/test_discovery.py::TestResolverSingleFlight: same-addr coalesces, different addrs probe independently, errors propagate to all waiters, and a failed probe is not cached.
…tion time net/url.Parse accepts api_base_url values like https://example.com:bad/api/v1 without complaint, so the failure surfaced later inside the HTTP client as an opaque transport error rather than a discovery-domain message. Validate the parsed port with strconv.ParseUint(_, 10, 16) so anything that is not empty or a valid uint16 is rejected up front with a domain-y error naming the offending field. Mirrors sdk/python/tests/test_discovery.py::TestResolveRejectsInvalidPortApiBaseUrl.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Go SDK discovery resolver to better align with Python SDK discovery behavior by coalescing concurrent resolves and tightening api_base_url port validation.
Changes:
- Adds per-address single-flight tracking for
Resolver.Resolve. - Adds port range validation for resolved
api_base_urlvalues. - Adds resolver tests for invalid ports and concurrent resolve behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
sdk/go/discovery/discovery.go |
Adds in-flight resolve coordination and port validation. |
sdk/go/discovery/discovery_test.go |
Adds tests for invalid ports and single-flight resolver behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Close call.done before releasing Resolver.mu so a new caller cannot register a fresh probe for the same address while existing waiters are still blocked on the just-completed call. - Correct the validate() port-check comment: url.Parse already rejects non-numeric ports; the strconv.ParseUint check guards against numeric ports outside the uint16 range (e.g. :99999). - Add TestResolveErrorsOnOutOfRangePortInAPIBaseURL to exercise the strconv.ParseUint branch; the existing :bad test exercises url.Parse's rejection path. - Guard gate release in the two gated single-flight tests with sync.Once + t.Cleanup so an assertion failure cannot leave the handler blocked and hang srv.Close().
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
Aligns the Go SDK discovery resolver with the Python SDK improvements landed in #374.
Resolver.Resolvenow coalesces concurrent calls for the same address: the first caller probes, every other caller waits for that probe's result and sees the sameNodeInfo(or the same error). Waiters respect their own context. Failed probes are not memoized.api_base_urlvalues with non-numeric ports (e.g.https://example.com:bad/api/v1) are rejected at validation time with a discovery-domain error containingport, rather than failing later as an opaque transport error.Both behaviors match the Python SDK; the Go tests mirror the matching
sdk/python/tests/test_discovery.pytest classes one-for-one.Test plan
make lintcleanmake testgreen (all packages)go test -race -count=100clean on the four new single-flight testsgo test -raceclean on the fullsdk/go/discoverypackageTestResolveErrorsOnNonNumericPortInAPIBaseURLexercises the validator change and verifies the failure is not memoizedFixes #375