refactor(transport): node-aware HTTP requests with helpers + safer response cleanup#82
refactor(transport): node-aware HTTP requests with helpers + safer response cleanup#82
Conversation
…sponse cleanup
- Introduce resolveBaseURL, newNodeRequest, and doTrusted to build node-aware requests
and centralize transport behavior.
- Migrate ForwardGet/Set/Remove, Health, FetchMerkle, and ListKeys to the new helpers.
- Drain and close HTTP response bodies to improve connection reuse and avoid leaks.
- hypercache: add fallback size estimation for backends without a serializer.
- cluster: derive node ID using binary.LittleEndian instead of manual bit shifts.
- Style: minor formatting/tidy changes across non-functional areas.
build: bump Go toolchain to 1.26.0; update golangci-lint to v2.10.1 and buf to 1.65.0
chore(deps): upgrade modules (e.g., fiber v3.1.0, sectools v1.2.3, x/{crypto,net,sys,text}); refresh go.sum
There was a problem hiding this comment.
Pull request overview
This pull request refactors the distributed HTTP transport layer to improve code organization, connection reuse, and maintainability. The PR introduces helper functions for building node-aware HTTP requests and centralizes transport behavior, while also updating build toolchain versions and dependencies.
Changes:
- Refactored HTTP transport in
pkg/backend/dist_http_transport.gowith new helper functions (resolveBaseURL,newNodeRequest,doTrusted) to centralize URL construction and request handling - Improved HTTP response body handling by draining and closing bodies to enable connection reuse across all transport methods
- Updated Go toolchain to 1.26.0, golangci-lint to v2.10.1, and buf to 1.65.0; upgraded dependencies (fiber v3.1.0, sectools v1.2.3, x/{crypto,net,sys,text})
- Simplified node ID derivation in
internal/cluster/node.gousingbinary.LittleEndian.PutUint64instead of manual bit shifting - Added fallback size estimation for backends without a serializer in
hypercache.go - Added
waitForDistNodeHealthhelper in integration tests to ensure nodes are healthy before proceeding - Various formatting improvements (blank lines) across test files for consistency
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/backend/dist_http_transport.go | Major refactor: removed direct URL string concatenation, added resolveBaseURL, newNodeRequest, and doTrusted helpers; improved response body cleanup with drain-then-close pattern |
| internal/cluster/node.go | Simplified node ID derivation from xxhash using binary.LittleEndian.PutUint64 instead of manual bit shifting |
| hypercache.go | Added default case in setItemSize for backends without a serializer |
| tests/integration/dist_rebalance_test.go | Added waitForDistNodeHealth helper to poll health endpoint before tests proceed |
| go.mod, go.sum | Updated Go version to 1.26.0; upgraded dependencies |
| Makefile, .project-settings.env, .pre-commit/*, .golangci.yaml | Updated toolchain versions (Go 1.26.0, golangci-lint v2.10.1, buf v1.65.0) |
| tests/*.go | Formatting improvements (added blank lines for readability) |
| pkg/backend/*.go | Minor formatting improvements (added blank lines) |
| pkg/cache/.go, pkg/eviction/.go | Formatting improvements |
| service.go | Formatting improvement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _, copyErr := io.Copy(io.Discard, resp.Body) | ||
| if copyErr != nil { | ||
| // Best-effort drain to keep connections reusable. | ||
| _ = copyErr | ||
| } | ||
|
|
||
| closeErr := resp.Body.Close() | ||
| if closeErr != nil { | ||
| // Best-effort close on deferred cleanup. | ||
| _ = closeErr | ||
| } |
There was a problem hiding this comment.
The deferred cleanup assigns errors to variables (copyErr, closeErr) but then immediately discards them with blank assignments (_ = copyErr, _ = closeErr). This pattern is verbose and adds no value. Consider simplifying to:
defer func() {
_, _ = io.Copy(io.Discard, resp.Body)
_ = resp.Body.Close()
}()This achieves the same best-effort cleanup more concisely.
| _, copyErr := io.Copy(io.Discard, resp.Body) | |
| if copyErr != nil { | |
| // Best-effort drain to keep connections reusable. | |
| _ = copyErr | |
| } | |
| closeErr := resp.Body.Close() | |
| if closeErr != nil { | |
| // Best-effort close on deferred cleanup. | |
| _ = closeErr | |
| } | |
| // Best-effort drain to keep connections reusable. | |
| _, _ = io.Copy(io.Discard, resp.Body) | |
| // Best-effort close on deferred cleanup. | |
| _ = resp.Body.Close() |
| _, copyErr := io.Copy(io.Discard, resp.Body) | ||
| if copyErr != nil { | ||
| // Best-effort drain to keep connections reusable. | ||
| _ = copyErr | ||
| } | ||
|
|
||
| closeErr := resp.Body.Close() | ||
| if closeErr != nil { | ||
| // Best-effort close on deferred cleanup. | ||
| _ = closeErr | ||
| } |
There was a problem hiding this comment.
The deferred cleanup assigns errors to variables (copyErr, closeErr) but then immediately discards them with blank assignments (_ = copyErr, _ = closeErr). This pattern is verbose and adds no value. Consider simplifying to:
defer func() {
_, _ = io.Copy(io.Discard, resp.Body)
_ = resp.Body.Close()
}()This achieves the same best-effort cleanup more concisely.
| _, copyErr := io.Copy(io.Discard, resp.Body) | |
| if copyErr != nil { | |
| // Best-effort drain to keep connections reusable. | |
| _ = copyErr | |
| } | |
| closeErr := resp.Body.Close() | |
| if closeErr != nil { | |
| // Best-effort close on deferred cleanup. | |
| _ = closeErr | |
| } | |
| // Best-effort drain to keep connections reusable. | |
| _, _ = io.Copy(io.Discard, resp.Body) | |
| // Best-effort close on deferred cleanup. | |
| _ = resp.Body.Close() |
| _, copyErr := io.Copy(io.Discard, resp.Body) | ||
| if copyErr != nil { | ||
| // Best-effort drain to keep connections reusable. | ||
| _ = copyErr | ||
| } | ||
|
|
||
| closeErr := resp.Body.Close() | ||
| if closeErr != nil { | ||
| // Best-effort close on deferred cleanup. | ||
| _ = closeErr | ||
| } |
There was a problem hiding this comment.
The deferred cleanup assigns errors to variables (copyErr, closeErr) but then immediately discards them with blank assignments (_ = copyErr, _ = closeErr). This pattern is verbose and adds no value. Consider simplifying to:
defer func() {
_, _ = io.Copy(io.Discard, resp.Body)
_ = resp.Body.Close()
}()This achieves the same best-effort cleanup more concisely.
| _, copyErr := io.Copy(io.Discard, resp.Body) | |
| if copyErr != nil { | |
| // Best-effort drain to keep connections reusable. | |
| _ = copyErr | |
| } | |
| closeErr := resp.Body.Close() | |
| if closeErr != nil { | |
| // Best-effort close on deferred cleanup. | |
| _ = closeErr | |
| } | |
| // Best-effort drain to keep connections reusable and then close the body. | |
| _, _ = io.Copy(io.Discard, resp.Body) | |
| _ = resp.Body.Close() |
| target, err := url.JoinPath(base.String(), strings.TrimPrefix(endpointPath, "/")) | ||
| if err != nil { | ||
| return nil, ewrap.Wrap(err, "join base url path") | ||
| } | ||
|
|
||
| targetURL, err := url.Parse(target) | ||
| if err != nil { | ||
| return nil, ewrap.Wrap(err, "parse target url") | ||
| } | ||
|
|
There was a problem hiding this comment.
After calling url.JoinPath on line 415 which returns a string, the code parses it again on line 420 to get a *url.URL. This is redundant since you already have a parsed *url.URL from line 410 (base). Consider constructing the target URL by creating a copy of base and modifying its Path field, then setting RawQuery if needed. This would avoid the extra Parse call and potential parsing errors. For example:
targetURL := *base // shallow copy
targetURL.Path = path.Join(targetURL.Path, strings.TrimPrefix(endpointPath, "/"))
if len(query) > 0 {
targetURL.RawQuery = query.Encode()
}
req, err := http.NewRequestWithContext(ctx, method, targetURL.String(), body)| target, err := url.JoinPath(base.String(), strings.TrimPrefix(endpointPath, "/")) | |
| if err != nil { | |
| return nil, ewrap.Wrap(err, "join base url path") | |
| } | |
| targetURL, err := url.Parse(target) | |
| if err != nil { | |
| return nil, ewrap.Wrap(err, "parse target url") | |
| } | |
| // Construct target URL by copying the parsed base URL and adjusting its path/query. | |
| targetURL := *base // shallow copy | |
| basePath := strings.TrimRight(targetURL.Path, "/") | |
| endpoint := strings.TrimLeft(endpointPath, "/") | |
| if basePath == "" { | |
| targetURL.Path = "/" + endpoint | |
| } else if endpoint == "" { | |
| targetURL.Path = basePath | |
| } else { | |
| targetURL.Path = basePath + "/" + endpoint | |
| } |
|
|
||
| func (t *DistHTTPTransport) doTrusted(hreq *http.Request) (*http.Response, error) { | ||
| // URL is built from the internal cluster resolver and scheme/host validated in newNodeRequest. | ||
| resp, err := t.client.Do(hreq) // #nosec G704 -- trusted cluster node URL, not user-supplied arbitrary endpoint |
There was a problem hiding this comment.
The #nosec directive uses G704 (audit errors not checked), but the error from client.Do is properly handled by wrapping and returning it. If the intent was to suppress warnings about using a URL from a variable, the correct gosec rule would be G107. Consider either using #nosec G107 if that's the concern, or removing the directive entirely since the error is properly handled and the URL is validated in resolveBaseURL.
| resp, err := t.client.Do(hreq) // #nosec G704 -- trusted cluster node URL, not user-supplied arbitrary endpoint | |
| resp, err := t.client.Do(hreq) // #nosec G107 -- trusted cluster node URL, not user-supplied arbitrary endpoint |
| _, copyErr := io.Copy(io.Discard, resp.Body) | ||
| if copyErr != nil { | ||
| // Best-effort drain to keep connections reusable. | ||
| _ = copyErr | ||
| } | ||
|
|
||
| closeErr := resp.Body.Close() | ||
| if closeErr != nil { | ||
| // Best-effort close on deferred cleanup. | ||
| _ = closeErr | ||
| } |
There was a problem hiding this comment.
The deferred cleanup assigns errors to variables (copyErr, closeErr) but then immediately discards them with blank assignments (_ = copyErr, _ = closeErr). This pattern is verbose and adds no value. Consider simplifying to:
defer func() {
_, _ = io.Copy(io.Discard, resp.Body)
_ = resp.Body.Close()
}()This achieves the same best-effort cleanup more concisely.
| _, copyErr := io.Copy(io.Discard, resp.Body) | |
| if copyErr != nil { | |
| // Best-effort drain to keep connections reusable. | |
| _ = copyErr | |
| } | |
| closeErr := resp.Body.Close() | |
| if closeErr != nil { | |
| // Best-effort close on deferred cleanup. | |
| _ = closeErr | |
| } | |
| // Best-effort drain to keep connections reusable. | |
| _, _ = io.Copy(io.Discard, resp.Body) | |
| // Best-effort close on deferred cleanup. | |
| _ = resp.Body.Close() |
| _, copyErr := io.Copy(io.Discard, resp.Body) | ||
| if copyErr != nil { | ||
| // Best-effort drain to keep connections reusable. | ||
| _ = copyErr | ||
| } | ||
|
|
||
| closeErr := resp.Body.Close() | ||
| if closeErr != nil { | ||
| // Best-effort close on deferred cleanup. | ||
| _ = closeErr | ||
| } |
There was a problem hiding this comment.
The deferred cleanup assigns errors to variables (copyErr, closeErr) but then immediately discards them with blank assignments (_ = copyErr, _ = closeErr). This pattern is verbose and adds no value. Consider simplifying to:
defer func() {
_, _ = io.Copy(io.Discard, resp.Body)
_ = resp.Body.Close()
}()This achieves the same best-effort cleanup more concisely.
| _, copyErr := io.Copy(io.Discard, resp.Body) | |
| if copyErr != nil { | |
| // Best-effort drain to keep connections reusable. | |
| _ = copyErr | |
| } | |
| closeErr := resp.Body.Close() | |
| if closeErr != nil { | |
| // Best-effort close on deferred cleanup. | |
| _ = closeErr | |
| } | |
| // Best-effort drain to keep connections reusable. | |
| _, _ = io.Copy(io.Discard, resp.Body) | |
| // Best-effort close on deferred cleanup. | |
| _ = resp.Body.Close() |
| _, copyErr := io.Copy(io.Discard, resp.Body) | ||
| if copyErr != nil { | ||
| // Best-effort drain to keep connections reusable. | ||
| _ = copyErr | ||
| } | ||
|
|
||
| closeErr := resp.Body.Close() | ||
| if closeErr != nil { | ||
| // Best-effort close on deferred cleanup. | ||
| _ = closeErr | ||
| } |
There was a problem hiding this comment.
The deferred cleanup assigns errors to variables (copyErr, closeErr) but then immediately discards them with blank assignments (_ = copyErr, _ = closeErr). This pattern is verbose and adds no value. Consider simplifying to:
defer func() {
_, _ = io.Copy(io.Discard, resp.Body)
_ = resp.Body.Close()
}()This achieves the same best-effort cleanup more concisely.
| _, copyErr := io.Copy(io.Discard, resp.Body) | |
| if copyErr != nil { | |
| // Best-effort drain to keep connections reusable. | |
| _ = copyErr | |
| } | |
| closeErr := resp.Body.Close() | |
| if closeErr != nil { | |
| // Best-effort close on deferred cleanup. | |
| _ = closeErr | |
| } | |
| _, _ = io.Copy(io.Discard, resp.Body) | |
| _ = resp.Body.Close() |
1 participant
build: bump Go toolchain to 1.26.0; update golangci-lint to v2.10.1 and buf to 1.65.0
chore(deps): upgrade modules (e.g., fiber v3.1.0, sectools v1.2.3, x/{crypto,net,sys,text}); refresh go.sum