Repository navigation
Bound the per-address dial of MGS REST calls so a black-holed first address cannot consume the whole request budget - #686
Open
liveinvalley wants to merge 1 commit into
Conversation
Since Go 1.23, net/http no longer cancels an in-flight dial when the request is canceled, so http.Client.Timeout does not bound the dial. The CreateControlChannel/CreateDataChannel requests used the default transport dialer (30s, split evenly across resolved addresses), so with two resolved ssmmessages addresses and the first one black-holed, the whole 15s client budget was spent on the first address and the request never reached the second one. Use a dedicated dialer with Mgs.ControlChannelDialTimeoutMillis (the same setting as the control-channel websocket dial), bound the response headers separately with ResponseHeaderTimeout (15s), and raise Client.Timeout to cover dial + TLS handshake + response headers as a backstop. A dial timeout of 0 keeps the previous behavior. Fixes aws#685 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
liveinvalley
marked this pull request as ready for review
November 3, 2026 20:22
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.
Issue
Fixes #685 (suggestion 1).
Problem
Since Go 1.23,
net/httpno longer cancels an in-flight dial when the request is canceled (commit 334ce510), sohttp.Client.Timeoutdoes not bound the dial.makeRestcall(used byCreateControlChannelandCreateDataChannel) ran on a clone ofhttp.DefaultTransport, whosenet.Dialer{Timeout: 30s}is split evenly across the resolved addresses. With two resolvedssmmessagesaddresses and the first one black-holed, the dial spends 15 s on the first address, which is the whole 15 s client budget. The request never reaches the second address, and the control channel cannot be re-established however often the agent retries. See #685 for the packet capture.Change
In
agent/session/service/service.go, a newnewMgsHttpClientbuilds the client for the MGS REST calls:Transport.DialContextuses a dedicatednet.DialerwithMgs.ControlChannelDialTimeoutMillis(default 20 s, so 10 s per address with two addresses). This is the same setting the control-channel websocket dial has used since 3.3.5390.0, so the token request now fails over the same way the websocket dial does.Transport.ResponseHeaderTimeout = 15sbounds the wait for the service response separately.Client.Timeoutbecomes dial timeout + TLS handshake timeout + 15 s (45 s by default), as a backstop.ControlChannelDialTimeoutMillis = 0(the existing opt-out) keeps the previous behavior: the default transport and a 15 s client timeout.Trade-off: a single REST attempt can now take up to 45 s by default instead of 15 s, but only when connecting is slow (an unreachable first address or a stalled TLS handshake). A service that accepts the connection and does not answer still fails after 15 s, as before, through
ResponseHeaderTimeout.Testing
agent/session/service/service_test.gocheck the client configuration for the default, custom and disabled (0) dial timeouts, an httptest round trip, and thatResponseHeaderTimeoutis enforced.go test ./agent/session/service/... ./agent/session/controlchannel/...andgo vet ./agent/session/service/pass.make checkstylereports no new findings (the onlygo vetfinding, inagent/session/crypto/blockcipher_test.go, exists onmainline).ssm-agent-worker.exebuilt from c6c4ce7 (same setup as runfault2in CreateControlChannel request never reaches the second ssmmessages address when the first is black-holed #685: hosts order [AZ1, AZ2], AZ1 black-holed; defaultControlChannelDialTimeoutMillisof 20000):Reach the retry limit) at T0 + 11.6 s. This change does not touch detection; the delay depends on where the black-hole falls in the 54 s ping cycle (45 s infault2).172.20.5.11:44310.1 s after it started (unchanged).SSM Connection channel status is set to ssmmessagesat T0 + 32.4 s, 20.8 s after detection. In CreateControlChannel request never reaches the second ssmmessages address when the first is black-holed #685 the same fault never recovered in 10 minutes.fault2that connection carried only a TLS handshake and no request.ssm-agent-worker.loghas noERRORand noClient.Timeout exceededlines for the whole run.fault2). Commands sent 4 s after the black-hole was removed succeeded over that same AZ2 connection.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.