API request failures seem to be able to return empty errors

[lachlanmunro]

Describe the bug

• UpdateUptimeTest request failures seem to be able to return an empty error ("").
• GetUptimeTest requests seem to be able to return an empty uptime test object with no error.

Additional context
We are running a small go tool to perform OAuth token updates on StatusCake uptime tests.

To Reproduce
We've since found the Debug option within the library but it's too verbose to run all the time. We've built in flags so we can try to get more detail next time if it becomes reproducible/consistent.

Most recently it was Updating an uptime test. We've seen it a fair few times on the GetUptimeTest too, please note the edge case catches in the code below.

You could use the StatusCake API changes that resulted in support request 22508200886017? They triggered it persistently. My hunch is it's a response that doesn't correspond to the expected response format, but that's a complete guess based on similar issues I've seen elsewhere in client libs.

Expected behavior
Library errors should allow clients to provide context failure to StatusCake. An "" error return is probably never be acceptable.

Detail
I've smashed the relevant bits out of a few places into the below representative code:

client, err := NewStatusCakeClient(statusCakeCfg)
if err != nil {
	return "", err
}

testResp, err := client.GetUptimeTest(ctx, id).Execute()
if err != nil {
	return "", fmt.Errorf("failed to get Uptime Test: %w", err)
}

if testResp.Data.Name == "" {
	return "", fmt.Errorf("failed to get Uptime Test: %w", errors.New("response returned without error but has no name"))
}

if testResp.Data.ID == "" {
	return "", fmt.Errorf("failed to get Uptime Test: %w", errors.New("response returned without error but has no ID"))
}

test := testResp.Data
l.Info().Str("name", t.Name).Msgf("Found test: %s", t.Name)
token, err := GetOAuthTokens(ctx, l, oathConfig, refreshToken)
if err != nil {
	return "", fmt.Errorf("failed to get new token: %w", err)
}

if token.AccessToken == "" {
	return token.RefreshToken, errors.New("access token was empty")
}

l.Debug().Str("token", token.AccessToken).Str("refreshtoken", token.RefreshToken).Msgf("Got new token: %s", token.AccessToken)
bytes, err := json.Marshal(&struct {
	Content       string `json:"content-type"`
	Cache         string `json:"cache-control"`
	Authorization string `json:"authorization"`
}{
	Content:       "application/json",
	Cache:         "no-cache",
	Authorization: fmt.Sprintf("Bearer %s", token.AccessToken),
})

header := string(bytes)
l.Debug().Msgf("Got new header state: %s", header)
if err != nil {
	return token.RefreshToken, fmt.Errorf("failed to encode new custom header state: %w", err)
}

r := client.UpdateUptimeTest(ctx, id).CustomHeader(customHeader)
if err := r.Execute(); err != nil {
	return token.RefreshToken, fmt.Errorf("failed to update uptime test: %w", err)
}

return token.RefreshToken, nil

Application logs look like the following, note the lack of error detail, I'd expect an error there that described the issue.

2024-04-17T11:31:29.750+01:00 INF Found test: Product - Prod - API every=600000 id=123456 name="Product - Prod - API"
2024-04-17T11:31:29.891+01:00 ERR Failed to update Uptime Test authentication: failed to update uptime test:  error="failed to update uptime test: " every=600000 id=123456 

[tomasbasham]

We believe there was an issues with a recent deployment to the API. Hopefully this has sine been resolved.

[lachlanmunro]

Hi @tomasbasham

Sorry, perhaps the reference in the report confused the issue.

This issue still remains as it's an issue with the Go code within this library. This client library still has code paths that return what is in effect errors.New(""). And from the GetUptime endpoint we've observed a nil error when it has not succeded in getting an Uptime from the remote API. Although these are likely caused by issues in communication to the StatusCake APIs, it's extremely unusual (if not un-idomatic) to return no error or "" errors on failure irrespective of what the upstream API is doing.

[tomasbasham]

Ah right. I believe I understand now. Sorry for the confusion. It looks like either the API itself is not returning and error, or as you suggest the response is not in the right format and not being correctly unmarshaled.

I've assumed the specific problem being reported is this:

error="failed to update uptime test: "

That there is no message after the colon. Is that right?

[lachlanmunro]

Yes, it meant we had no further information when the Update started to fail to provide to your teams when we hit the issue. We've since implemented to be able to turn on the "Debug" flag against the client and log bodies, but we can't really leave that running generally so it doesn't help with intermittent issues.

The other thing we wanted to report in the above was these return statements within the above code:

testResp, err := client.GetUptimeTest(ctx, id).Execute()
if err != nil {
	return "", fmt.Errorf("failed to get Uptime Test: %w", err)
}

if testResp.Data.Name == "" {
	return "", fmt.Errorf("failed to get Uptime Test: %w", errors.New("response returned without error but has no name"))
}

if testResp.Data.ID == "" {
	return "", fmt.Errorf("failed to get Uptime Test: %w", errors.New("response returned without error but has no ID"))
}

We've seen these hit before tail end of last year which seemed odd. For them to be hit there was no error returned from getting the UptimeTest but the response had no Name/ID.

[fitzy101]

We have also observed these empty errors - specifically when using the Terraform provider. It is very confusing.

Tracing the code, the only place within the GetUptimeTestExecute execution path where an error value may be non-nil but print an empty error message is:

statuscake-go/api_uptime.go

Lines 837 to 848 in f733556

	if res.StatusCode >= 300 {
	var v APIError
	if err := a.client.decode(&v, responseBody, responseContentType); err != nil {
	return returnValue, APIError{
	Status: res.StatusCode,
	Message: "failed to deserialise error response",
	parent: err,
	}
	}
	v.Status = res.StatusCode
	return returnValue, v
	}

Let's say the response's status code was >= 300 and we hit this block. An attempt is made to decode the response body into v.

I don't have the full context of the various paths through the decode function, but it looks like there is a chance at least one of the return nil exits could be hit without decoding anything into the instance of the APIError struct.

For whatever reason, assume that the return value from decode is nil. The error returned now potentially has no Message or parent members populated with a value (v.Status however is set on ln 846).

When the Error() func is called, the string result will be empty:

statuscake-go/error.go

Lines 53 to 59 in f733556

	func (e APIError) Error() string {
	if e.parent != nil {
	return e.parent.Error() + ": " + e.Message
	}
	return e.Message
	}

Here's a minimal repro:

package main

import "fmt"

func main() {
	err := foo()
	if err, ok := err.(FooError); ok && err.Status == 1 {
		fmt.Println("err.Status == 1")
	}
	if err != nil {
		fmt.Printf("err: %s\n", err)
	}
}

func foo() error {
	var f FooError
	f.Status = 2
	return f
}

type FooError struct {
	Message string
	Status  int
	parent  error
}

func (fe FooError) Error() string {
	return fe.Message
}

func (fe FooError) Unwrap() error {
	return fe.parent
}

Running this program prints:

err: 

[fitzy101]

Oh, I see that this SDK is generated code. I would otherwise submit a PR to see if the error could be more helpful.

In this case, it is likely on the API side - maybe an empty error is being returned for some endpoints?

[genisd]

I've triaged the issue.

The statuscake API has a transient issue where it at times returns a 200 with an empty body.

This client (rightflly so one could say) doesn't catch that to retry the request.

[genisd]

In addition 5xx are not retried.

I'm waiting for their support as far as the empty body 200 goes.

Awaiting that we have code to retry 5xx errors (which in my investigation were extremely rare), the 200 empty body was more prominent).

So we will offer PRs for improvements, just waiting for them to respond on the 200 empty body issue,

[genisd]

So the empty 200 body issue (i.e. api failure disguised as a 200) appears to have been resolved. I haven't received positive confirmation from Statuscake on this, but I can no longer reproduce it. It was easy to trigger/reproduce before.

During my triage i found that 5xx errors alebit very rare, do occur and are not retried at all.
I created this PR for it: #47