Nit: Use /ping for daemon ready check instead of /version#414
Open
inFocus7 wants to merge 1 commit intoagentregistry-dev:mainfrom
Open
Nit: Use /ping for daemon ready check instead of /version#414inFocus7 wants to merge 1 commit intoagentregistry-dev:mainfrom
/ping for daemon ready check instead of /version#414inFocus7 wants to merge 1 commit intoagentregistry-dev:mainfrom
Conversation
Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the daemon readiness/response checks used by the CLI to call the public /ping endpoint (instead of /version), avoiding confusing version requests and preventing unnecessary auth failures during readiness polling.
Changes:
- Switch daemon “is responding” checks from
GET /versiontoGET /ping. - Tighten readiness success criteria to require HTTP
200 OK(instead of any2xx) for/ping.
Comments suppressed due to low confidence (1)
pkg/daemon/health.go:26
defer resp.Body.Close()is inside a retry loop. If the first request returns a non-200 status, the body won’t be closed untilIsRespondingreturns, and multiple open bodies can accumulate across retries. Close (and ideally drain) the response body immediately within each iteration before continuing/retrying, reservingdeferfor non-loop scopes.
for i := range maxRetries {
resp, err := httpClient.Get(pingURL)
if err == nil {
defer resp.Body.Close()
if resp.StatusCode == http.StatusOK {
return true
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
41
to
46
| for time.Now().Before(deadline) { | ||
| resp, err := httpClient.Get(pingURL) | ||
| if err == nil { | ||
| resp.Body.Close() | ||
| if resp.StatusCode >= 200 && resp.StatusCode < 300 { | ||
| if resp.StatusCode == http.StatusOK { | ||
| return nil |
There was a problem hiding this comment.
WaitForReady closes the response body without reading it. In Go’s HTTP client, not draining the body can prevent connection reuse and lead to repeated TCP connections during the polling loop. Consider discarding the body content (e.g., read to EOF) before closing to improve efficiency.
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.
Description
When using CLI, i noticed random calls to
/versionwhen using it which was confusing (i'd only expect it to be called if I'm trying to see the version). Since/versionis not a "public path" (not in our list of paths to avoid doing AuthN checks), there are also 3 lines of "unauthenticated -> /v0/version" fails each CLI call.We could add
/versionas a public path if we don't believe having build/run info public is an issue, but at the core level if we're checking for a response, we should be hitting between/pingor/health. I chose/pingsince this is doing a response check not actual health checks per daemon-call.Changes
/pingfor response checks200specifically for OK-ness. Huma defaults to a 200 code on responses with a body (which the /ping endpoint provides.)Change Type
Changelog
Additional Notes