Skip to content

Commit 9b75de7

Browse files
jenae-janzenjenae-janzen
authored
Merge pull request #478 from netlify/erikw/lambda-error-msg
Feat: Don't retry function upload on 400 or 422 status
2 parents ddba7fc + 25f5b63 commit 9b75de7

File tree

2 files changed

+145
-15
lines changed

2 files changed

+145
-15
lines changed

‎go/porcelain/deploy.go‎

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,8 @@ type DeployOptions struct {
8282
BuildDir string
8383
LargeMediaEnabled bool
8484

85-
IsDraft bool
85+
IsDraft bool
86+
SkipRetry bool
8687

8788
Title string
8889
Branch string
@@ -348,12 +349,14 @@ func (n *Netlify) DoDeploy(ctx context.Context, options *DeployOptions, deploy *
348349
return deploy, nil
349350
}
350351

351-
if err := n.uploadFiles(ctx, deploy, options.files, options.Observer, fileUpload, options.UploadTimeout); err != nil {
352+
skipRetry := options.SkipRetry
353+
354+
if err := n.uploadFiles(ctx, deploy, options.files, options.Observer, fileUpload, options.UploadTimeout, skipRetry); err != nil {
352355
return nil, err
353356
}
354357

355358
if options.functions != nil {
356-
if err := n.uploadFiles(ctx, deploy, options.functions, options.Observer, functionUpload, options.UploadTimeout); err != nil {
359+
if err := n.uploadFiles(ctx, deploy, options.functions, options.Observer, functionUpload, options.UploadTimeout, skipRetry); err != nil {
357360
return nil, err
358361
}
359362
}
@@ -405,7 +408,7 @@ func (n *Netlify) WaitUntilDeployLive(ctx context.Context, d *models.Deploy) (*m
405408
return n.waitForState(ctx, d, "ready")
406409
}
407410

408-
func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *deployFiles, observer DeployObserver, t uploadType, timeout time.Duration) error {
411+
func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *deployFiles, observer DeployObserver, t uploadType, timeout time.Duration, skipRetry bool) error {
409412
sharedErr := &uploadError{err: nil, mutex: &sync.Mutex{}}
410413
sem := make(chan int, n.uploadLimit)
411414
wg := &sync.WaitGroup{}
@@ -435,7 +438,7 @@ func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *depl
435438
select {
436439
case sem <- 1:
437440
wg.Add(1)
438-
go n.uploadFile(ctx, d, file, observer, t, timeout, wg, sem, sharedErr)
441+
go n.uploadFile(ctx, d, file, observer, t, timeout, wg, sem, sharedErr, skipRetry)
439442
case <-ctx.Done():
440443
log.Info("Context terminated, aborting file upload")
441444
return errors.Wrap(ctx.Err(), "aborted file upload early")
@@ -455,7 +458,7 @@ func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *depl
455458
return sharedErr.err
456459
}
457460

458-
func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundle, c DeployObserver, t uploadType, timeout time.Duration, wg *sync.WaitGroup, sem chan int, sharedErr *uploadError) {
461+
func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundle, c DeployObserver, t uploadType, timeout time.Duration, wg *sync.WaitGroup, sem chan int, sharedErr *uploadError, skipRetry bool) {
459462
defer func() {
460463
wg.Done()
461464
<-sem
@@ -543,10 +546,16 @@ func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundl
543546
context.GetLogger(ctx).WithError(operationError).Errorf("Failed to upload file %v", f.Name)
544547
apiErr, ok := operationError.(deployApiError)
545548

546-
if ok && apiErr.Code() == 401 {
547-
sharedErr.mutex.Lock()
548-
sharedErr.err = operationError
549-
sharedErr.mutex.Unlock()
549+
if ok {
550+
if apiErr.Code() == 401 {
551+
sharedErr.mutex.Lock()
552+
sharedErr.err = operationError
553+
sharedErr.mutex.Unlock()
554+
}
555+
556+
if skipRetry && (apiErr.Code() == 400 || apiErr.Code() == 422) {
557+
operationError = backoff.Permanent(operationError)
558+
}
550559
}
551560
}
552561

‎go/porcelain/deploy_test.go‎

Lines changed: 126 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,7 @@ func TestUploadFiles_Cancelation(t *testing.T) {
287287
for _, bundle := range files.Files {
288288
d.Required = append(d.Required, bundle.Sum)
289289
}
290-
err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute)
290+
err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false)
291291
require.ErrorIs(t, err, gocontext.Canceled)
292292
}
293293

@@ -317,10 +317,131 @@ func TestUploadFiles_Errors(t *testing.T) {
317317
for _, bundle := range files.Files {
318318
d.Required = append(d.Required, bundle.Sum)
319319
}
320-
err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute)
320+
err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false)
321321
require.Equal(t, err.Error(), "[PUT /deploys/{deploy_id}/files/{path}][500] uploadDeployFile default &{Code:0 Message:}")
322322
}
323323

324+
func TestUploadFiles422Error_SkipsRetry(t *testing.T) {
325+
attempts := 0
326+
ctx := gocontext.Background()
327+
328+
server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) {
329+
defer func() {
330+
attempts++
331+
}()
332+
333+
rw.Header().Set("Content-Type", "application/json; charset=utf-8")
334+
rw.WriteHeader(http.StatusUnprocessableEntity)
335+
rw.Write([]byte(`{"message": "Unprocessable Entity", "code": 422 }`))
336+
}))
337+
defer server.Close()
338+
339+
// File upload:
340+
hu, _ := url.Parse(server.URL)
341+
tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, http.DefaultClient)
342+
client := NewRetryable(tr, strfmt.Default, 1)
343+
client.uploadLimit = 1
344+
ctx = context.WithAuthInfo(ctx, apiClient.BearerToken("token"))
345+
346+
// Create some files to deploy
347+
dir, err := ioutil.TempDir("", "deploy")
348+
require.NoError(t, err)
349+
defer os.RemoveAll(dir)
350+
require.NoError(t, ioutil.WriteFile(filepath.Join(dir, "foo.html"), []byte("Hello"), 0644))
351+
352+
files, err := walk(dir, nil, false, false)
353+
require.NoError(t, err)
354+
d := &models.Deploy{}
355+
for _, bundle := range files.Files {
356+
d.Required = append(d.Required, bundle.Sum)
357+
}
358+
// Set SkipRetry to true
359+
err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, true)
360+
require.ErrorContains(t, err, "Code:422 Message:Unprocessable Entity")
361+
require.Equal(t, attempts, 1)
362+
}
363+
364+
func TestUploadFunctions422Error_SkipsRetry(t *testing.T) {
365+
attempts := 0
366+
ctx := gocontext.Background()
367+
368+
server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) {
369+
defer func() {
370+
attempts++
371+
}()
372+
373+
rw.Header().Set("Content-Type", "application/json; charset=utf-8")
374+
rw.WriteHeader(http.StatusUnprocessableEntity)
375+
rw.Write([]byte(`{"message": "Unprocessable Entity", "code": 422 }`))
376+
}))
377+
defer server.Close()
378+
379+
// Function upload:
380+
hu, _ := url.Parse(server.URL)
381+
tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, http.DefaultClient)
382+
client := NewRetryable(tr, strfmt.Default, 1)
383+
client.uploadLimit = 1
384+
apiCtx := context.WithAuthInfo(ctx, apiClient.BearerToken("token"))
385+
386+
dir, err := ioutil.TempDir("", "deploy")
387+
functionsPath := filepath.Join(dir, ".netlify", "functions")
388+
os.MkdirAll(functionsPath, os.ModePerm)
389+
require.NoError(t, err)
390+
defer os.RemoveAll(dir)
391+
require.NoError(t, ioutil.WriteFile(filepath.Join(functionsPath, "foo.js"), []byte("module.exports = () => {}"), 0644))
392+
393+
files, _, _, err := bundle(ctx, functionsPath, mockObserver{})
394+
require.NoError(t, err)
395+
d := &models.Deploy{}
396+
for _, bundle := range files.Files {
397+
d.RequiredFunctions = append(d.RequiredFunctions, bundle.Sum)
398+
}
399+
// Set SkipRetry to true
400+
err = client.uploadFiles(apiCtx, d, files, nil, functionUpload, time.Minute, true)
401+
require.ErrorContains(t, err, "Code:422 Message:Unprocessable Entity")
402+
require.Equal(t, attempts, 1)
403+
}
404+
405+
func TestUploadFiles400Error_NoSkipRetry(t *testing.T) {
406+
attempts := 0
407+
ctx := gocontext.Background()
408+
409+
server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) {
410+
defer func() {
411+
attempts++
412+
}()
413+
414+
rw.Header().Set("Content-Type", "application/json; charset=utf-8")
415+
rw.WriteHeader(http.StatusBadRequest)
416+
rw.Write([]byte(`{"message": "Bad Request", "code": 400 }`))
417+
return
418+
}))
419+
defer server.Close()
420+
421+
hu, _ := url.Parse(server.URL)
422+
tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, http.DefaultClient)
423+
client := NewRetryable(tr, strfmt.Default, 1)
424+
client.uploadLimit = 1
425+
ctx = context.WithAuthInfo(ctx, apiClient.BearerToken("token"))
426+
427+
// Create some files to deploy
428+
dir, err := ioutil.TempDir("", "deploy")
429+
require.NoError(t, err)
430+
defer os.RemoveAll(dir)
431+
require.NoError(t, ioutil.WriteFile(filepath.Join(dir, "foo.html"), []byte("Hello"), 0644))
432+
433+
files, err := walk(dir, nil, false, false)
434+
require.NoError(t, err)
435+
d := &models.Deploy{}
436+
for _, bundle := range files.Files {
437+
d.Required = append(d.Required, bundle.Sum)
438+
}
439+
// Set SkipRetry to false
440+
err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false)
441+
require.ErrorContains(t, err, "Code:400 Message:Bad Request")
442+
require.Greater(t, attempts, 1)
443+
}
444+
324445
func TestUploadFiles_SkipEqualFiles(t *testing.T) {
325446
ctx := gocontext.Background()
326447

@@ -377,11 +498,11 @@ func TestUploadFiles_SkipEqualFiles(t *testing.T) {
377498
d.Required = []string{files.Sums["a.html"]}
378499
d.RequiredFunctions = []string{functions.Sums["a"]}
379500

380-
err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute)
501+
err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false)
381502
require.NoError(t, err)
382503
assert.Equal(t, 1, serverRequests)
383504

384-
err = client.uploadFiles(ctx, d, functions, nil, functionUpload, time.Minute)
505+
err = client.uploadFiles(ctx, d, functions, nil, functionUpload, time.Minute, false)
385506
require.NoError(t, err)
386507
assert.Equal(t, 2, serverRequests)
387508
}
@@ -437,7 +558,7 @@ func TestUploadFunctions_RetryCountHeader(t *testing.T) {
437558
d.RequiredFunctions = append(d.RequiredFunctions, bundle.Sum)
438559
}
439560

440-
require.NoError(t, client.uploadFiles(apiCtx, d, files, nil, functionUpload, time.Minute))
561+
require.NoError(t, client.uploadFiles(apiCtx, d, files, nil, functionUpload, time.Minute, false))
441562
}
442563

443564
func TestBundle(t *testing.T) {

0 commit comments

Comments
 (0)