Skip to content

fix(proxy): adopt fasthttp v1.77 adaptor and forward requests as sent - #46

Merged
aaydin-tr merged 2 commits into
mainfrom
fix/fasthttp-2104-corrections
Oct 28, 2026
Merged

aaydin-tr merged 2 commits into
mainfrom
fix/fasthttp-2104-corrections

Conversation

@aaydin-tr

Copy link
Copy Markdown
Owner

Bump fasthttp to v1.77.0 (Go 1.26); HTTP/2 via http.Server.Protocols.

@github-actions

github-actions Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Claude finished @aaydin-tr's task in 2m 27s —— View job


Code review: origin/main...HEAD (PR #46)

Fixed point: origin/main (4f24d46), chosen as the default because this run is non-interactive. Two commits were reviewed (1b6d061, 3eae0dd). No spec lookup was done; the Spec axis was replaced by Correctness, as configured. Items already listed in TODO.md were excluded.

Standards

No hard violations of CLAUDE.md, CONTEXT.md or the ADRs. The new names are written out in full and use the CONTEXT.md vocabulary (moveURICredentialsToAuthorization, dropEmptyBody, reannounceTrailers).

Judgement calls on the "comments stay to one or two lines" rule (CLAUDE.md, Code style):

  • internal/proxy/nethttp_adapter.go:76-78: the dropEmptyBody comment is three lines. Trim it to two. Inline comment posted.
  • internal/proxy/trailers.go:19-23: the reannounceTrailers comment is four lines, and its first sentence restates the function name. Inline comment posted.
  • internal/proxy/backend_wire_test.go: the *ProxyClient is named p. That is a one-letter name that is neither a receiver nor a loop index. It is a nit.

Baseline smells (judgement calls only):

  • Speculative Generality: nethttp_adapter.go:102-108, methodUsuallyLacksBody. It hand-copies net/http's private method list, including PROPFIND and SEARCH. The comment shows this is deliberate. The cost is a stdlib list that has to be kept in sync by hand.
  • Divergent Change (mild): proxy.go:231-237 (preReq) and proxy.go:496 (NewProxyClient). The "forward as sent" policy is split between a per-request step and a client-construction flag (NoDefaultUserAgentHeader). A pointer comment would help a reader see both.
  • Duplicated Code (tests): backend_wire_test.go, proxyThroughNetHttpFrontend and newHTTP2Frontend both build the same adapter wrapper. A small shared helper would remove it.
  • Middle Man (trivial): reannounceTrailers is now a loop over AddTrailerBytes plus a warning log. It is still a reasonable named seam, so no change is suggested.

Correctness

No confirmed bug, security regression or conflict. The sub-agent could not run go build, go vet or go test, because the sandbox denied go commands. It also could not read the fasthttp v1.77 module source, so anything that depends on fasthttpadaptor internals is unverified. CI's -race run is the real check.

Bugs

  • nethttp_adapter.go:79-99 (dropEmptyBody): for GET, HEAD, DELETE, OPTIONS, PROPFIND and SEARCH with an unknown length, Peek(1) blocks the handler goroutine until the client sends a byte or ends the stream. This happens before a Backend is picked, and only server.read_timeout bounds the wait. It mirrors net/http's own client probe, so the impact is low. The 413 cap still applies afterwards.
  • nethttp_adapter.go:110-115 (mustBufferBody): a request with a known length and r.Trailer set (an h2 request with content-length plus trailers) now streams instead of being buffered. Whether the trailer values reach the Backend depends on when fasthttpadaptor reads r.Trailer. TestNetHttpAdapterForwardsAnnouncedTrailers covers it. Please confirm that test passes under -race in CI.
  • proxy.go:139-140 (DoTimeout): after a timeout, fasthttp may keep reading a streamed request body once ServeHTTP has returned. Known-length bodies already behaved this way, so this is not new. It now covers more requests, because trailer-bearing bodies are no longer force-buffered.

Security: no weakening found.

  • The 413 cap holds on both stacks: the declared-length check, plus buffering under MaxBytesReader for unknown lengths.
  • proxy_timeout is untouched.
  • There is still no h2c. startNetHttp serves only through ServeTLS, tls_min_version still reaches TLSConfig, and Protocols is set on http.Server.
  • Hop-by-hop stripping, X-Forwarded-For and the Host override are unchanged in preReq.
  • moveURICredentialsToAuthorization (proxy.go:244) turns request-target userinfo into Authorization: Basic. A client's own Authorization header wins. It adds no new capability, and the access log records only ctx.Path(), so the credentials are not logged.

Conflicts

  • No references to x/net/http2 remain in *.go, *.mod, README or CLAUDE.md, so the x/net removal from go.mod is consistent.
  • NoDefaultUserAgentHeader and SetNoDefaultContentType affect proxied requests only. Health Probes use a different client.
  • The integration-test/echobackend and benchmark modules still pin Go 1.25. They are separate modules and unaffected.
  • The HTTP/2 tuning item (max_concurrent_streams) is already tracked in TODO.md, so it is not reported.

Summary: Standards has 0 hard violations and 6 judgement calls (the worst is the over-long comments at trailers.go:19-23 and nethttp_adapter.go:76-78). Correctness has 0 confirmed findings and 3 unverified notes (the worst is the trailer-forwarding path at nethttp_adapter.go:110-115, which needs the CI -race result).

--- · branch fix/fasthttp-2104-corrections

Comment thread internal/proxy/nethttp_adapter.go Resolved
Comment thread internal/proxy/trailers.go Resolved
@aaydin-tr
aaydin-tr merged commit 718e329 into main Oct 28, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant