Bound each registry HTTP request with an overall deadline - #208
Closed
bdehamer wants to merge 1 commit into
Closed
Conversation
The connection-establishment timeouts added in #206 (dial, TLS handshake, response header) only govern setting up a brand-new connection, and ResponseHeaderTimeout stops once headers arrive. None of them bound a stall during the response *body* read on an already-established connection — so a wedged reused keep-alive connection could hang a manifest/referrers/blob fetch past the retry envelope all the way to the outer admission deadline (the incident: reason=canceled, step=descriptor, status=0, idle CPU, no network error). Add deadlineRoundTripper: a RoundTripper decorator that wraps each registry request in context.WithTimeout and wraps resp.Body so the deadline also covers body reads (the same mechanism http.Client.Timeout uses internally). go-containerregistry v0.21.9 exposes only remote.WithTransport — no http.Client hook — so the wall must be a decorator. It is applied in GetRemoteOptions over the shared *http.Transport, so #206's connection-phase timeouts still apply beneath it, and it protects every caller of that transport. New flag -registry-request-timeout (default 0 = derive from bundle-timeout, a safe ceiling that won't fire before the per-attempt context). Only negative values are rejected. The registry timeout overrides are grouped into a registryTimeouts struct so configureBundleFetcher stays within revive's argument-count limit. Note on scope: an earlier draft of this change also added HTTP/2 keepalive (ReadIdleTimeout/PingTimeout). It was dropped after verifying the entire dxcrprod fetch path is HTTP/1.1 — the azurecr.io registry frontend (OpenResty) and the blob backend it 307-redirects to (*.blob.core.windows.net, Azure Blob Storage) both decline h2 via ALPN. With no HTTP/2 connection anywhere in the path, keepalive would be inert; the overall request wall is the fix that actually applies. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
bdehamer
force-pushed
the
bdehamer-registry-transport-hardening
branch
from
August 25, 2026 19:25
123d877 to
ffdd901
Compare
bdehamer
changed the title
bdehamer
added a commit
that referenced
this pull request
Aug 26, 2026
* Tune registry connection pool to self-heal dead connections The connection-establishment timeouts in #206 only bound setting up a *new* connection. A connection that a load balancer or gateway silently drops while it sits idle in the keep-alive pool is invisible to them: the next request that reuses it stalls until the outer admission deadline (the class-H "reused connection stall" — idle pods, status=0, no network error). ACR's whole fetch path is HTTP/1.1 (the azurecr.io OpenResty frontend and the *.blob.core.windows.net backend both decline h2), so HTTP/2 keepalive PINGs don't apply — the lever that does is the TCP/pool lifecycle. Tighten three transport knobs so a wedged idle connection is retired before it can be reused, and cap how many can pile up: - dialer KeepAlive 30s -> 10s: keep pooled connections warm so an idle intermediary (e.g. an Azure Load Balancer with a ~4-minute idle cutoff) is less likely to reap them silently. - IdleConnTimeout 90s -> 10s: we close an idle connection well before those cutoffs, so a silently-dropped one is retired by us instead of lingering to stall the next request. - MaxIdleConns/PerHost 100/50 -> 25/25: bound dead-connection accumulation. Sized from logs — peak in-flight fetches per pod is single-digit (5-7 across normal, busy, and incident windows), and each fetch touches two hosts (registry + the blob backend it 307-redirects to), so ~25 covers the reuse working set with headroom. Exposed as fetcher package vars with sensible defaults and optional override flags (-registry-dial-keep-alive, -registry-idle-conn-timeout, -registry-max-idle-conns, -registry-max-idle-conns-per-host), validated and wired in a new configureRegistryPool helper before the transport is built. Zero keeps net/http semantics (no limit / use default); only negatives are rejected. Defaults are safe, so no deployment change is required to get the fix. Complementary to #208 (overall per-request wall): that bounds a request once wedged; this stops connections from becoming wedge-prone. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Clarify keep-alive zero semantics; de-duplicate pool test Address review feedback on the connection-pool tuning: - registry-dial-keep-alive is a net.Dialer.KeepAlive, not an http.Transport field, so 0 selects Go's default keep-alive period (~15s), not "the net/http default". Correct the flag help, the configureRegistryPool doc (which distinguishes dialer keep-alive from the http.Transport pool fields), and the DialKeepAlive var comment. - Drop the duplicate package-var assertions in TestNewRegistryTransportUsesResolvedTimeouts: it now asserts only the default values, and TestNewRegistryTransportAppliesPoolTuning already proves the fields are wired (not hardcoded) via non-default values. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.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
1 participant
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
Adds an overall per-request wall to the shared registry HTTP transport, closing a gap left by the connection-establishment timeouts in #206.
#206's timeouts (dial, TLS handshake, response header) only govern setting up a brand-new connection, and
ResponseHeaderTimeoutstops once headers arrive. None of them bound a stall during the response body read on an already-established connection. So a wedged reused keep-alive connection could hang a manifest/referrers/blob fetch past the retry envelope all the way to the outer admission deadline — the incident signature:reason=canceled,step=descriptor,status=0, idle CPU, no network error logged.Change
deadlineRoundTripper(pkg/fetcher/bundle.go) — aRoundTripperdecorator that wraps each registry request incontext.WithTimeoutand wrapsresp.Bodyso the deadline also covers body reads (the same mechanismhttp.Client.Timeoutuses internally). go-containerregistry v0.21.9 exposes onlyremote.WithTransport— nohttp.Clienthook — so the wall must be a decorator. It's applied inGetRemoteOptionsover the shared*http.Transport, so #206's connection-phase timeouts still apply beneath it, and it protects every caller of that transport.cancelOnCloseBodyreleases the deadline exactly once onBody.Close().-registry-request-timeout(default0= derive from-bundle-timeout, a safe ceiling that won't fire before the per-attempt context). Only negatives are rejected.registryTimeoutsstruct soconfigureBundleFetcherstays within revive's argument-count limit (6).Test plan
pkg/fetcher/bundle_test.go:deadlineRoundTripper— header stall, body-read stall, passthrough + cancel-on-close (no leak), andtimeout<=0pure passthrough;resolveRequestTimeout(derive vs override).cmd/aaop/aaop_test.go:configureBundleFetcherwires-registry-request-timeoutand rejects it when negative.Local:
go build ./...,go test ./... -race,go vet ./pkg/fetcher/... ./cmd/aaop/...(clean),golangci-lint run ./...→ 0 issues.Rollout
Default is safe (
0derives from-bundle-timeout), so no deployment flag change is required. Complementary to #206.