Merge pull request #3005 from buildkite/fix-agent-lost-when-using-http2

Work around golang/go#59690

api/auth.go

@@ -5,11 +5,7 @@ import (
 	"net/http"
 )
 
-type canceler interface {
-	CancelRequest(*http.Request)
-}
-
-// authenticatedTransport manages injection of the API token
+// authenticatedTransport manages injection of the API token.
 type authenticatedTransport struct {
 	// The Token used for authentication. This can either the be
 	// organizations registration token, or the agents access token.
@@ -19,19 +15,59 @@ type authenticatedTransport struct {
 	Delegate http.RoundTripper
 }
 
-// RoundTrip invoked each time a request is made
+// RoundTrip invoked each time a request is made.
 func (t authenticatedTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	// Per net/http#RoundTripper:
+	//
+	// "RoundTrip must always close the body, including on errors, ..."
+	reqBodyClosed := false
+	if req.Body != nil {
+		defer func() {
+			if !reqBodyClosed {
+				req.Body.Close()
+			}
+		}()
+	}
+
 	if t.Token == "" {
 		return nil, fmt.Errorf("Invalid token, empty string supplied")
 	}
 
+	// Per net/http#RoundTripper:
+	//
+	// "RoundTrip should not modify the request, except for
+	// consuming and closing the Request's Body. RoundTrip may
+	// read fields of the request in a separate goroutine. Callers
+	// should not mutate or reuse the request until the Response's
+	// Body has been closed."
+	//
+	// But we can pass a _different_ request to t.Delegate.RoundTrip.
+	// req.Clone does a sufficiently deep clone (including Header which we
+	// modify).
+	req = req.Clone(req.Context())
 	req.Header.Set("Authorization", fmt.Sprintf("Token %s", t.Token))
 
+	// req.Body is assumed to be closed by the delegate.
+	reqBodyClosed = true
 	return t.Delegate.RoundTrip(req)
 }
 
-// CancelRequest cancels an in-flight request by closing its connection.
+// CancelRequest forwards the call to t.Delegate, if it implements CancelRequest
+// itself.
 func (t *authenticatedTransport) CancelRequest(req *http.Request) {
-	cancelableTransport := t.Delegate.(canceler)
-	cancelableTransport.CancelRequest(req)
+	canceler, ok := t.Delegate.(interface{ CancelRequest(*http.Request) })
+	if !ok {
+		return
+	}
+	canceler.CancelRequest(req)
+}
+
+// CloseIdleConnections forwards the call to t.Delegate, if it implements
+// CloseIdleConnections itself.
+func (t *authenticatedTransport) CloseIdleConnections() {
+	closer, ok := t.Delegate.(interface{ CloseIdleConnections() })
+	if !ok {
+		return
+	}
+	closer.CloseIdleConnections()
 }

api/client.go

@@ -22,6 +22,7 @@ import (
 
 	"github.com/buildkite/agent/v3/logger"
 	"github.com/google/go-querystring/query"
+	"golang.org/x/net/http2"
 )
 
 const (
@@ -80,33 +81,55 @@ func NewClient(l logger.Logger, conf Config) *Client {
 	}
 
 	httpClient := conf.HTTPClient
-	if conf.HTTPClient == nil {
-
-		// use the default transport as it is optimized and configured for http2
-		// and will avoid accidents in the future
-		tr := http.DefaultTransport.(*http.Transport).Clone()
-
-		if conf.DisableHTTP2 {
-			tr.ForceAttemptHTTP2 = false
-			tr.TLSNextProto = make(map[string]func(authority string, c *tls.Conn) http.RoundTripper)
-			// The default TLSClientConfig has h2 in NextProtos, so the negotiated TLS connection will assume h2 support.
-			// see https://github.com/golang/go/issues/50571
-			tr.TLSClientConfig.NextProtos = []string{"http/1.1"}
-		}
 
-		if conf.TLSConfig != nil {
-			tr.TLSClientConfig = conf.TLSConfig
+	if httpClient != nil {
+		return &Client{
+			logger: l,
+			client: httpClient,
+			conf:   conf,
 		}
+	}
 
-		httpClient = &http.Client{
-			Timeout: 60 * time.Second,
-			Transport: &authenticatedTransport{
-				Token:    conf.Token,
-				Delegate: tr,
-			},
+	// Base any modifications on the default transport.
+	transport := http.DefaultTransport.(*http.Transport).Clone()
+	// Allow override of TLSConfig. This must be set prior to calling
+	// http2.ConfigureTransports.
+	if conf.TLSConfig != nil {
+		transport.TLSClientConfig = conf.TLSConfig
+	}
+
+	if conf.DisableHTTP2 {
+		transport.TLSNextProto = make(map[string]func(string, *tls.Conn) http.RoundTripper)
+		// The default TLSClientConfig has h2 in NextProtos, so the
+		// negotiated TLS connection will assume h2 support.
+		// see https://github.com/golang/go/issues/50571
+		transport.TLSClientConfig.NextProtos = []string{"http/1.1"}
+	} else {
+		// There is a bug in http2 on Linux regarding using dead connections.
+		// This is a workaround. See https://github.com/golang/go/issues/59690
+		//
+		// Note that http2.ConfigureTransports alters its argument in order to
+		// supply http2 functionality, and the http2.Transport does not support
+		// HTTP/1.1 as a protocol, so we get slightly odd-looking code where
+		// we use `transport` later on instead of the just-returned `tr2`.
+		// tr2 is needed merely to configure the http2 option.
+		tr2, err := http2.ConfigureTransports(transport)
+		if err != nil {
+			l.Warn("Failed to configure HTTP2 transports: %v", err)
+		}
+		if tr2 != nil {
+			tr2.ReadIdleTimeout = 30 * time.Second
 		}
 	}
 
+	httpClient = &http.Client{
+		Timeout: 60 * time.Second,
+		Transport: &authenticatedTransport{
+			Token:    conf.Token,
+			Delegate: transport,
+		},
+	}
+
 	return &Client{
 		logger: l,
 		client: httpClient,
@@ -270,6 +293,7 @@ func (c *Client) doRequest(req *http.Request, v any) (*Response, error) {
 	c.logger.Debug("%s %s", req.Method, req.URL)
 
 	resp, err := c.client.Do(req)
+
 	if err != nil {
 		if c.conf.TraceHTTP {
 			tracer.EmitTraceToLog(logger.ERROR)
@@ -377,6 +401,7 @@ func traceHTTPRequest(req *http.Request, t *tracer) *http.Request {
 			t.LogField("reused", strconv.FormatBool(info.Reused))
 			t.LogField("idle", strconv.FormatBool(info.WasIdle))
 			t.LogDuration("idleTime", info.IdleTime)
+			t.LogField("localAddr", info.Conn.LocalAddr().String())
 		},
 		PutIdleConn: func(err error) {
 			t.LogTiming("putIdleConn")

go.mod

@@ -50,6 +50,7 @@ require (
 	go.opentelemetry.io/otel/trace v1.30.0
 	golang.org/x/crypto v0.27.0
 	golang.org/x/exp v0.0.0-20231108232855-2478ac86f678
+	golang.org/x/net v0.29.0
 	golang.org/x/oauth2 v0.23.0
 	golang.org/x/sys v0.25.0
 	golang.org/x/term v0.24.0
@@ -128,7 +129,6 @@ require (
 	go.uber.org/atomic v1.11.0 // indirect
 	go.uber.org/multierr v1.11.0 // indirect
 	golang.org/x/mod v0.17.0 // indirect
-	golang.org/x/net v0.29.0 // indirect
 	golang.org/x/sync v0.8.0 // indirect
 	golang.org/x/text v0.18.0 // indirect
 	golang.org/x/time v0.6.0 // indirect