Merge pull request #3033 from buildkite/fix-artifact-timeouts

buildkite · Oct 9, 2024 · a038276 · a038276
2 parents a766240 + 6bc50af
commit a038276
Show file tree

Hide file tree

Showing 6 changed files with 18 additions and 4 deletions.
diff --git a/internal/agenthttp/client.go b/internal/agenthttp/client.go
@@ -11,7 +11,8 @@ import (
 	"golang.org/x/net/http2"
 )
 
-// NewClient creates a HTTP client.
+// NewClient creates a HTTP client. Note that the default timeout is 60 seconds;
+// for some use cases (e.g. artifact operations) use [WithNoTimeout].
 func NewClient(opts ...ClientOption) *http.Client {
 	conf := clientConfig{
 		// This spells out the defaults, even if some of them are zero values.
@@ -62,6 +63,7 @@ func WithAuthBearer(b string) ClientOption     { return func(c *clientConfig) {
 func WithAuthToken(t string) ClientOption      { return func(c *clientConfig) { c.Token = t } }
 func WithAllowHTTP2(a bool) ClientOption       { return func(c *clientConfig) { c.AllowHTTP2 = a } }
 func WithTimeout(d time.Duration) ClientOption { return func(c *clientConfig) { c.Timeout = d } }
+func WithNoTimeout(c *clientConfig)            { c.Timeout = 0 }
 func WithTLSConfig(t *tls.Config) ClientOption { return func(c *clientConfig) { c.TLSConfig = t } }
 
 type ClientOption = func(*clientConfig)

diff --git a/internal/artifact/artifactory_downloader.go b/internal/artifact/artifactory_downloader.go
@@ -70,7 +70,10 @@ func (d ArtifactoryDownloader) Start(ctx context.Context) error {
 		"Authorization": fmt.Sprintf("Basic %s", getBasicAuthHeader(username, password)),
 	}
 
-	client := agenthttp.NewClient(agenthttp.WithAllowHTTP2(!d.conf.DisableHTTP2))
+	client := agenthttp.NewClient(
+		agenthttp.WithAllowHTTP2(!d.conf.DisableHTTP2),
+		agenthttp.WithNoTimeout,
+	)
 
 	// We can now cheat and pass the URL onto our regular downloader
 	return NewDownload(d.logger, client, DownloadConfig{

diff --git a/internal/artifact/artifactory_uploader.go b/internal/artifact/artifactory_uploader.go
@@ -78,6 +78,7 @@ func NewArtifactoryUploader(l logger.Logger, c ArtifactoryUploaderConfig) (*Arti
 		conf:   c,
 		client: agenthttp.NewClient(
 			agenthttp.WithAllowHTTP2(!c.DisableHTTP2),
+			agenthttp.WithNoTimeout,
 		),
 		iURL:       parsedURL,
 		Path:       path,

diff --git a/internal/artifact/bk_uploader.go b/internal/artifact/bk_uploader.go
@@ -153,6 +153,7 @@ func (u *bkMultipartUpload) DoWork(ctx context.Context) (*api.ArtifactPartETag,
 
 	client := agenthttp.NewClient(
 		agenthttp.WithAllowHTTP2(!u.conf.DisableHTTP2),
+		agenthttp.WithNoTimeout,
 	)
 
 	resp, err := agenthttp.Do(u.logger, client, req,
@@ -208,6 +209,7 @@ func (u *bkFormUpload) DoWork(ctx context.Context) (*api.ArtifactPartETag, error
 	// Create the client
 	client := agenthttp.NewClient(
 		agenthttp.WithAllowHTTP2(!u.conf.DisableHTTP2),
+		agenthttp.WithNoTimeout,
 	)
 
 	// Perform the request

diff --git a/internal/artifact/downloader.go b/internal/artifact/downloader.go
@@ -198,7 +198,10 @@ func (a *Downloader) createDownloader(artifact *api.Artifact, path, destination
 		})
 
 	default:
-		client := agenthttp.NewClient(agenthttp.WithAllowHTTP2(!a.conf.DisableHTTP2))
+		client := agenthttp.NewClient(
+			agenthttp.WithAllowHTTP2(!a.conf.DisableHTTP2),
+			agenthttp.WithNoTimeout,
+		)
 		return NewDownload(a.logger, client, DownloadConfig{
 			URL:         artifact.URL,
 			Path:        path,

diff --git a/internal/artifact/s3_downloader.go b/internal/artifact/s3_downloader.go
@@ -66,7 +66,10 @@ func (d S3Downloader) Start(ctx context.Context) error {
 	}
 
 	// We can now cheat and pass the URL onto our regular downloader
-	client := agenthttp.NewClient(agenthttp.WithAllowHTTP2(!d.conf.DisableHTTP2))
+	client := agenthttp.NewClient(
+		agenthttp.WithAllowHTTP2(!d.conf.DisableHTTP2),
+		agenthttp.WithNoTimeout,
+	)
 	return NewDownload(d.logger, client, DownloadConfig{
 		URL:         signedURL,
 		Path:        d.conf.Path,