fix: don't double-count file uploads in worker pool (#313)

* fix: don't double-count file uploads in worker pool

* Remove or consolidate auth setup in tests

Author: vbrown608

go.mod

@@ -16,6 +16,6 @@ require (
 	github.com/pkg/errors v0.9.1
 	github.com/rsc/goversion v1.2.0
 	github.com/sirupsen/logrus v1.6.0
-	github.com/stretchr/testify v1.6.1
+	github.com/stretchr/testify v1.7.0
 	golang.org/x/net v0.0.0-20201224014010-6772e930b67b // indirect
 )

go.sum

@@ -401,6 +401,7 @@ github.com/spf13/viper v1.6.2 h1:7aKfF+e8/k68gda3LOjo5RxiUqddoFxVq4BKBPrxk5E=
 github.com/spf13/viper v1.6.2/go.mod h1:t3iDnF5Jlj76alVNuyFBk5oUMCvsrkbvZK0WQdfDi5k=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
+github.com/stretchr/objx v0.2.0 h1:Hbg2NidpLE8veEBkEZTL3CvlkUIVzuU9jDplZO54c48=
 github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE=
 github.com/stretchr/testify v1.1.4/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
@@ -410,6 +411,8 @@ github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJy
 github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
 github.com/stretchr/testify v1.6.1 h1:hDPOHmpOpP40lSULcqw7IrRb/u7w6RpDC9399XyoNd0=
 github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
+github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY=
+github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 github.com/subosito/gotenv v1.2.0 h1:Slr1R9HxAlEKefgq5jn9U+DnETlIUa6HfgEzj0g5d7s=
 github.com/subosito/gotenv v1.2.0/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw=
 github.com/tidwall/pretty v1.0.0 h1:HsD+QiTn7sK6flMKIvNmpqz1qrpP3Ps6jOKIKMooyg4=

go/porcelain/deploy.go

@@ -379,18 +379,18 @@ func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *depl
 		}
 	}
 
-	context.GetLogger(ctx).Infof("Uploading %v files", count)
+	log := context.GetLogger(ctx)
+	log.Infof("Uploading %v files", count)
 
 	for _, sha := range required {
 		if files, exist := files.Hashed[sha]; exist {
 			for _, file := range files {
 				select {
 				case sem <- 1:
-					sem <- 1
 					wg.Add(1)
-
 					go n.uploadFile(ctx, d, file, observer, t, timeout, wg, sem, sharedErr)
 				case <-ctx.Done():
+					log.Info("Context terminated, aborting file upload")
 					return errors.Wrap(ctx.Err(), "aborted file upload early")
 				}
 			}

go/porcelain/deploy_test.go

@@ -17,7 +17,6 @@ import (
 	"github.com/netlify/open-api/v2/go/plumbing/operations"
 	"github.com/netlify/open-api/v2/go/porcelain/context"
 
-	"github.com/go-openapi/runtime"
 	apiClient "github.com/go-openapi/runtime/client"
 	"github.com/go-openapi/strfmt"
 	"github.com/stretchr/testify/assert"
@@ -93,23 +92,15 @@ func TestOpenAPIClientWithWeirdResponse(t *testing.T) {
 	}))
 	defer server.Close()
 
-	httpClient := http.DefaultClient
-	authInfo := runtime.ClientAuthInfoWriterFunc(func(r runtime.ClientRequest, _ strfmt.Registry) error {
-		r.SetHeaderParam("User-Agent", "buildbot")
-		r.SetHeaderParam("Authorization", "Bearer 1234")
-		return nil
-	})
-
 	hu, _ := url.Parse(server.URL)
-	tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, httpClient)
+	tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, http.DefaultClient)
 	client := NewRetryable(tr, strfmt.Default, 1)
 
 	body := ioutil.NopCloser(bytes.NewReader([]byte("hello world")))
 	params := operations.NewUploadDeployFileParams().WithDeployID("1234").WithPath("foo/bar/biz").WithFileBody(body)
-	_, operationError := client.Operations.UploadDeployFile(params, authInfo)
+	_, operationError := client.Operations.UploadDeployFile(params, nil)
 	require.Error(t, operationError)
 	require.Equal(t, "[PUT /deploys/{deploy_id}/files/{path}][408] uploadDeployFile default  &{Code:408 Message:a message}", operationError.Error())
-
 }
 
 func TestConcurrentFileUpload(t *testing.T) {
@@ -120,21 +111,14 @@ func TestConcurrentFileUpload(t *testing.T) {
 	}))
 	defer server.Close()
 
-	httpClient := http.DefaultClient
-	authInfo := runtime.ClientAuthInfoWriterFunc(func(r runtime.ClientRequest, _ strfmt.Registry) error {
-		r.SetHeaderParam("User-Agent", "buildbot")
-		r.SetHeaderParam("Authorization", "Bearer 1234")
-		return nil
-	})
-
 	hu, _ := url.Parse(server.URL)
-	tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, httpClient)
+	tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, http.DefaultClient)
 	client := NewRetryable(tr, strfmt.Default, 1)
 	for i := 0; i < 30; i++ {
 		go func() {
 			body := ioutil.NopCloser(bytes.NewReader([]byte("hello world")))
 			params := operations.NewUploadDeployFileParams().WithDeployID("1234").WithPath("foo/bar/biz").WithFileBody(body)
-			_, _ = client.Operations.UploadDeployFile(params, authInfo)
+			_, _ = client.Operations.UploadDeployFile(params, nil)
 		}()
 	}
 }
@@ -146,18 +130,11 @@ func TestWaitUntilDeployLive_Timeout(t *testing.T) {
 	}))
 	defer server.Close()
 
-	httpClient := http.DefaultClient
-	authInfo := runtime.ClientAuthInfoWriterFunc(func(r runtime.ClientRequest, _ strfmt.Registry) error {
-		r.SetHeaderParam("User-Agent", "buildbot")
-		r.SetHeaderParam("Authorization", "Bearer 1234")
-		return nil
-	})
-
 	hu, _ := url.Parse(server.URL)
-	tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, httpClient)
+	tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, http.DefaultClient)
 	client := NewRetryable(tr, strfmt.Default, 1)
 
-	ctx := context.WithAuthInfo(gocontext.Background(), authInfo)
+	ctx := context.WithAuthInfo(gocontext.Background(), apiClient.BearerToken("token"))
 	ctx, _ = gocontext.WithTimeout(ctx, 50*time.Millisecond)
 	_, err := client.WaitUntilDeployLive(ctx, &models.Deploy{})
 	assert.Error(t, err)
@@ -191,6 +168,38 @@ func TestWalk_IgnoreNodeModulesInRoot(t *testing.T) {
 	assert.NotNil(t, files.Files["more/node_modules/inner-package"])
 }
 
+func TestUploadFiles_Cancelation(t *testing.T) {
+	ctx, cancel := gocontext.WithCancel(gocontext.Background())
+	server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+		cancel() // Cancel deploy as soon as first file upload is attempted.
+		rw.Header().Set("Content-Type", "application/json; charset=utf-8")
+		rw.Write([]byte(`{ "state": "canceled" }`))
+	}))
+	defer server.Close()
+
+	hu, _ := url.Parse(server.URL)
+	tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, http.DefaultClient)
+	client := NewRetryable(tr, strfmt.Default, 1)
+	client.uploadLimit = 1
+	ctx = context.WithAuthInfo(ctx, apiClient.BearerToken("token"))
+
+	// Create some files to deploy
+	dir, err := ioutil.TempDir("", "deploy")
+	require.NoError(t, err)
+	defer os.RemoveAll(dir)
+	require.NoError(t, ioutil.WriteFile(filepath.Join(dir, "foo.html"), []byte("Hello"), 0644))
+	require.NoError(t, ioutil.WriteFile(filepath.Join(dir, "bar.html"), []byte("World"), 0644))
+
+	files, err := walk(dir, nil, false, false)
+	require.NoError(t, err)
+	d := &models.Deploy{}
+	for _, bundle := range files.Files {
+		d.Required = append(d.Required, bundle.Sum)
+	}
+	err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute)
+	require.ErrorIs(t, err, gocontext.Canceled)
+}
+
 func TestReadZipRuntime(t *testing.T) {
 	runtime, err := readZipRuntime("../internal/data/hello-rs-function-test.zip")
 	if err != nil {