Skip to content

Feat: Don't retry function upload on 400 or 422 status #478

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Sep 22, 2023
Merged

Feat: Don't retry function upload on 400 or 422 status #478

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
a12f834
Prototype
Jul 20, 2023
6eaf7b2
fix
Jul 20, 2023
2f75b17
Comment
Jul 20, 2023
7d7c6a7
stop retrying on 400 and 422 status codes
jenae-janzen Aug 23, 2023
03e50c8
remove todo comment
jenae-janzen Aug 23, 2023
279e58b
add SkipRetry ff to options struct
jenae-janzen Aug 23, 2023
7d176fb
small fix
jenae-janzen Aug 23, 2023
de2b8c1
use permanent error
jenae-janzen Aug 29, 2023
9b326af
test
jrwhitmer Aug 30, 2023
6248a62
test part 3
jrwhitmer Aug 30, 2023
49e8d71
ff on for test
jrwhitmer Aug 30, 2023
0116127
test part 4
jrwhitmer Aug 30, 2023
047bf94
test part 5
jrwhitmer Aug 30, 2023
2192c51
test part 6
jrwhitmer Aug 30, 2023
1367935
return permanent error + work on tests
jenae-janzen Aug 31, 2023
75e7e49
move comment out of code
jenae-janzen Aug 31, 2023
107dbe6
save changes
jenae-janzen Sep 18, 2023
0276651
Merge branch 'master' into erikw/lambda-error-msg
jenae-janzen Sep 18, 2023
ff30a75
assert error
jenae-janzen Sep 19, 2023
10097e7
missed one
jenae-janzen Sep 19, 2023
25f5b63
fix tests
jenae-janzen Sep 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions go/porcelain/deploy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ type DeployOptions struct {
BuildDir string
LargeMediaEnabled bool

IsDraft bool
IsDraft bool
SkipRetry bool

Title string
Branch string
Expand Down Expand Up @@ -344,12 +345,14 @@ func (n *Netlify) DoDeploy(ctx context.Context, options *DeployOptions, deploy *
return deploy, nil
}

if err := n.uploadFiles(ctx, deploy, options.files, options.Observer, fileUpload, options.UploadTimeout); err != nil {
skipRetry := options.SkipRetry

if err := n.uploadFiles(ctx, deploy, options.files, options.Observer, fileUpload, options.UploadTimeout, skipRetry); err != nil {
return nil, err
}

if options.functions != nil {
if err := n.uploadFiles(ctx, deploy, options.functions, options.Observer, functionUpload, options.UploadTimeout); err != nil {
if err := n.uploadFiles(ctx, deploy, options.functions, options.Observer, functionUpload, options.UploadTimeout, skipRetry); err != nil {
return nil, err
}
}
Expand Down Expand Up @@ -401,8 +404,9 @@ func (n *Netlify) WaitUntilDeployLive(ctx context.Context, d *models.Deploy) (*m
return n.waitForState(ctx, d, "ready")
}

func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *deployFiles, observer DeployObserver, t uploadType, timeout time.Duration) error {
func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *deployFiles, observer DeployObserver, t uploadType, timeout time.Duration, skipRetry bool) error {
sharedErr := &uploadError{err: nil, mutex: &sync.Mutex{}}
permanentErr := &backoff.PermanentError{Err: nil}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit unclear for me why we instantiate permanentErr here, as could you elaborate?

as far as I know backoff.PermanentError is to be used as return from a backoff.Retry() block

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially we weren't sure how to use PermanentError, but your other comment pointed us in the right direction I think

sem := make(chan int, n.uploadLimit)
wg := &sync.WaitGroup{}

Expand Down Expand Up @@ -431,7 +435,7 @@ func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *depl
select {
case sem <- 1:
wg.Add(1)
go n.uploadFile(ctx, d, file, observer, t, timeout, wg, sem, sharedErr)
go n.uploadFile(ctx, d, file, observer, t, timeout, wg, sem, sharedErr, permanentErr, skipRetry)
case <-ctx.Done():
log.Info("Context terminated, aborting file upload")
return errors.Wrap(ctx.Err(), "aborted file upload early")
Expand All @@ -451,7 +455,7 @@ func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *depl
return sharedErr.err
}

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) {
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, permanentErr *backoff.PermanentError, skipRetry bool) {
defer func() {
wg.Done()
<-sem
Expand Down Expand Up @@ -538,10 +542,16 @@ func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundl
context.GetLogger(ctx).WithError(operationError).Errorf("Failed to upload file %v", f.Name)
apiErr, ok := operationError.(apierrors.Error)

if ok && apiErr.Code() == 401 {
sharedErr.mutex.Lock()
sharedErr.err = operationError
sharedErr.mutex.Unlock()
if ok {
if apiErr.Code() == 401 {
sharedErr.mutex.Lock()
sharedErr.err = operationError
sharedErr.mutex.Unlock()
}

if skipRetry && (apiErr.Code() == 400 || apiErr.Code() == 422) {
operationError = permanentErr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be wrong, but I think we should wrap the existing error in a backoff.PermanentError{} (for which backoff provides the Permanent() function:

Suggested change
operationError = permanentErr
operationError = backoff.Permanent(operationError)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think that does make sense. And then when backoff receives the Permanent error, it should stop retrying, if I understand correctly.

}
}
}

Expand Down
39 changes: 34 additions & 5 deletions go/porcelain/deploy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ func TestUploadFiles_Cancelation(t *testing.T) {
for _, bundle := range files.Files {
d.Required = append(d.Required, bundle.Sum)
}
err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute)
err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we could add a test here which exercises the new code paths we introduced (i.e. checking we don't retry 400 and 422 errors when uploading).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JGAntunes do you have any pointers for the testing? I seem to be a bit stuck here. We are also having trouble running the test suite locally, which is slowing this down a bit 😬

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

about running the test suite locally, if it seems like nothing is running it's probably because of the retries loop.

try running the tests as:

 go test -race github.com/netlify/open-api/v2/go/porcelain --run TestUploadFiles400Errors -v

And you should get some output (hopefully)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip @4xposed!! We got the tests running and can see the output which has been very helpful :D

require.ErrorIs(t, err, gocontext.Canceled)
}

Expand Down Expand Up @@ -317,9 +317,38 @@ func TestUploadFiles_Errors(t *testing.T) {
for _, bundle := range files.Files {
d.Required = append(d.Required, bundle.Sum)
}
err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute)
err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false)
require.Equal(t, err.Error(), "[PUT /deploys/{deploy_id}/files/{path}][500] uploadDeployFile default &{Code:0 Message:}")
}
func TestUploadFiles400Errors(t *testing.T) {
ctx := gocontext.Background()

server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) {
rw.WriteHeader(http.StatusUnprocessableEntity)
Copy link
Contributor

@4xposed 4xposed Aug 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think to properly assert the error message in the requre.Equal in line 350 we need to make the mock server to return a body with a json that has code: and message keys:

Suggested change
rw.WriteHeader(http.StatusUnprocessableEntity)
rw.WriteHeader(http.StatusUnprocessableEntity)
rw.Header().Set("Content-Type", "application/json; charset=utf-8")
rw.Write([]byte(`{"message": "Unprocessable Entity", "code": 422}`))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super helpful and definitely makes sense! -- looking at other test cases I see this is done similarly. However, when we set rw.Write([]byte(`{"message": "Unprocessable Entity", "code": 422}`)) we keep getting the error&{0 } (*models.Error) is not supported by the TextConsumer, can be resolved by supporting TextUnmarshaler interface.

So then the apiError isn't set and then our test skips over the code we're trying to test and keeps retrying 🙈

Looking at other examples in other tests, it seems like it works to set the response this way when calling client.Operations.UploadDeployFile, but not when calling client.uploadFiles -- when calling uploadFiles the error is thrown from UploadDeployFile.

We've been trying to debug this by writing the body different ways, trying to marshal the response etc, but haven't had any success yet 🤔 Is there maybe something silly we're missing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the error is being thrown from: https://github.com/netlify/open-api/blob/master/go/plumbing/operations/operations_client.go#L4131

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think It might be the order on which those are set, mockserver is a bit quirky at times. try if setting the headers in this order helps:

rw.Header().Set("Content-Type", "application/json; charset=utf-8")
rw.WriteHeader(http.StatusUnprocessableEntity)
rw.Write([]byte(`{"message": "Unprocessable Entity", "code": 422 }`))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 @4xposed thanks so much for the tip! That worked and now as far as I can tell, the server is responding as expected. However, now we're hitting a different problem and struggling to debug it.

We now receive the error with the correct code and message, but when we get to this line
apiErr, ok := operationError.(apierrors.Error)
no matter what we try, apiErr is always nil and ok is always false.

operationError is an error that looks like:
error(*github.com/netlify/open-api/v2/go/plumbing/operations.UploadDeployFunctionDefault) *{_statusCode: 422, Payload: *github.com/netlify/open-api/v2/go/models.Error {Code: 422, Message: "Unprocessable Entity"}}

In my debugger, I can access the status code by calling either operationError._statusCode or operationError.Payload.Code, but not by calling operationError.(apierrors.Error). However, I can't call operationError.Payload.Code in the actual code, because Payload/Code isn't defined on the Error type.

So since operationError.(apierrors.Error) never seems to evaluate to anything and there wasn't a test for the original code, we're uncertain if we're doing something wrong here, or if the original code was actually working as intended.

Are we missing something here? Is there a better way to test this (perhaps non-locally?)

cc @jrwhitmer @JGAntunes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@4xposed Thanks, I really appreciate the offer! I'm in Europe now 🎉

Testing manually using the debugger, it looks like everything's working as expected, thanks so much for the fix. The one last thing I'm trying to do to wrap up this PR is to fix my tests:

require.Equal(t, err, "[PUT /deploys/{deploy_id}/files/{path}][422] uploadDeployFile default  &{Code:422 Message: Unprocessable Entity}")

doesn't work because the error's now wrapped by the backoff.Permanent error. Is it sufficient to just requireError(t, err) here? Or is there a better way to assert this that I'm missing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pairing might not be necessary since most of this seems to be working, but I'd appreciate you taking another look when you have the time 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could call err.Unwrap() seems like backoff.PermanentError has a few methods for this: https://github.com/cenkalti/backoff/blob/v4/retry.go#L129

I don't think it's strictly necessary, but it would be nice to at least have some way that we have the right error and not any error, if that makes sense.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll give it a shot! I do agree it would be nice to have the right error if possible

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I used Require. ErrorContains instead and that seemed to do the trick for verifying we have the correct error message

}))
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("bad"))

// 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))

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, true)
require.Equal(t, err.Error(), "[PUT /deploys/{deploy_id}/files/{path}][401] uploadDeployFile default &{Code:401 Message: Unauthorized}")
}

func TestUploadFiles_SkipEqualFiles(t *testing.T) {
ctx := gocontext.Background()
Expand Down Expand Up @@ -377,11 +406,11 @@ func TestUploadFiles_SkipEqualFiles(t *testing.T) {
d.Required = []string{files.Sums["a.html"]}
d.RequiredFunctions = []string{functions.Sums["a"]}

err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute)
err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false)
require.NoError(t, err)
assert.Equal(t, 1, serverRequests)

err = client.uploadFiles(ctx, d, functions, nil, functionUpload, time.Minute)
err = client.uploadFiles(ctx, d, functions, nil, functionUpload, time.Minute, false)
require.NoError(t, err)
assert.Equal(t, 2, serverRequests)
}
Expand Down Expand Up @@ -437,7 +466,7 @@ func TestUploadFunctions_RetryCountHeader(t *testing.T) {
d.RequiredFunctions = append(d.RequiredFunctions, bundle.Sum)
}

require.NoError(t, client.uploadFiles(apiCtx, d, files, nil, functionUpload, time.Minute))
require.NoError(t, client.uploadFiles(apiCtx, d, files, nil, functionUpload, time.Minute, false))
}

func TestBundle(t *testing.T) {
Expand Down