Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
## Release (2026-MM-DD)

- `core`: [v0.27.1](core/CHANGELOG.md#v0271)
- **Bugfix:** `WaitWithContext` no longer returns `(nil, nil)` after a single retryable `502`/`504` error
- `telemetrylink`:
- [v0.6.0](services/telemetrylink/CHANGELOG.md#v060)
- `v1api`:
Expand Down
3 changes: 3 additions & 0 deletions core/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
## v0.27.1
- **Bugfix:** `WaitWithContext` no longer returns `(nil, nil)` after a single retryable `502`/`504` error. `WaiterHelper.Wait()` now correctly returns `waitFinished = false` on generic fetch errors

## v0.27.0
- **Feature:** Added experimental paginate package for AIP compliant pagination

Expand Down
2 changes: 1 addition & 1 deletion core/VERSION
Original file line number Diff line number Diff line change
@@ -1 +1 @@
v0.27.0
v0.27.1
4 changes: 4 additions & 0 deletions core/wait/wait.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,10 @@ func (h *AsyncActionHandler[T]) WaitWithContext(ctx context.Context) (res *T, er
if err != nil {
return res, err
}
// the error was retryable and got swallowed by h.handleError, so done represents a failed
// fetch rather than a finished action - poll again instead of returning
// otherwise we might return (nil, nil)
done = false
Comment on lines +105 to +108

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This guard is debatable

}
if done {
return res, nil
Expand Down
40 changes: 40 additions & 0 deletions core/wait/wait_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,46 @@ func TestWaitWithContext(t *testing.T) {
}
}

// TestWaitWithContext_RetryableErrorReportedAsDone is a regression test for a bug where a checkFn
// that reports waitFinished=true alongside a retryable error caused WaitWithContext to return (nil, nil)
// instead of retrying, because `done` stayed true even after handleError swallowed the error.
func TestWaitWithContext_RetryableErrorReportedAsDone(t *testing.T) {
synctest.Test(t, func(t *testing.T) {
type respType struct{ Name string }

numberCheckFnCalls := 0
checkFn := func() (waitFinished bool, response *respType, err error) {
numberCheckFnCalls++
if numberCheckFnCalls == 1 {
// here the return true is the offending line => should be false
return true, nil, &oapierror.GenericOpenAPIError{
StatusCode: RetryHttpErrorStatusCodes[0],
ErrorMessage: "temporary error",
}
}
return true, &respType{Name: "my-resource"}, nil
}
handler := AsyncActionHandler[respType]{
checkFn: checkFn,
throttle: 10 * time.Millisecond,
timeout: 5 * time.Second,
tempErrRetryLimit: 5,
}

resp, err := handler.WaitWithContext(context.Background())

if err != nil {
t.Errorf("expected no error, got %v", err)
}
if resp == nil || resp.Name != "my-resource" {
t.Errorf("expected a resolved response, got %v", resp)
}
if numberCheckFnCalls != 2 {
t.Errorf("expected checkFn to be called twice (initial + retry), got %d calls", numberCheckFnCalls)
}
})
}
Comment on lines +389 to +427

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This regression test is not needed if we decide to get rid of the guard


func TestHandleError(t *testing.T) {
for _, tt := range []struct {
desc string
Expand Down
2 changes: 1 addition & 1 deletion core/wait/waiterhelper.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func (w *WaiterHelper[T, S]) Wait() AsyncActionCheck[T] {
return true, nil, nil
}
}
return true, nil, err
return false, nil, err
}

state, err := w.GetState(instance)
Expand Down
176 changes: 171 additions & 5 deletions core/wait/waiterhelper_test.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
package wait

import (
"context"
"fmt"
"net/http"
"testing"
"testing/synctest"
"time"

"github.com/google/go-cmp/cmp"

Expand Down Expand Up @@ -85,7 +88,7 @@ func TestWaiterHelper_Wait(t *testing.T) {
// If ActiveState is empty, it assumes we are waiting for a deletion
activeStates: nil,
deleteHttpErrorStatusCodes: []int{http.StatusNotFound},
wantFinished: true,
wantFinished: false,
wantErr: true,
wantResponse: nil,
},
Expand All @@ -95,7 +98,7 @@ func TestWaiterHelper_Wait(t *testing.T) {
// If ActiveState is empty, it assumes we are waiting for a deletion
activeStates: nil,
deleteHttpErrorStatusCodes: []int{http.StatusNotFound},
wantFinished: true,
wantFinished: false,
wantErr: true,
wantResponse: nil,
},
Expand All @@ -104,14 +107,14 @@ func TestWaiterHelper_Wait(t *testing.T) {
fetchErr: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadRequest},
// If ActiveState is empty, it assumes we are waiting for a deletion
activeStates: nil,
wantFinished: true,
wantFinished: false,
wantErr: true,
},
{
name: "Success - Error on fetch instance (400 Bad Request)",
name: "Failure - Error on fetch instance (403 Forbidden)",
fetchErr: &oapierror.GenericOpenAPIError{StatusCode: http.StatusForbidden},
activeStates: []string{"READY"},
wantFinished: true,
wantFinished: false,
wantErr: true,
wantResponse: nil,
},
Expand Down Expand Up @@ -161,3 +164,166 @@ func TestWaiterHelper_Wait(t *testing.T) {
})
}
}

func TestWaiterHelper_WaitWithContext(t *testing.T) {
type fetchResponse struct {
res *MockResource
err error
}

tests := []struct {
name string
// fetchResponses is the array of responses the mocked endpoint will answer in sequence
fetchResponses []fetchResponse
activeStates []string
errorStates []string
deleteHttpErrorStatusCodes []int
wantCalls int
wantErr bool
wantResponse *MockResource
}{
{
name: "Success - Retryable 502 Gateway Error followed by Active State",
fetchResponses: []fetchResponse{
{res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadGateway}},
{res: &MockResource{Status: "ACTIVE"}, err: nil},
},
activeStates: []string{"ACTIVE"},
errorStates: []string{"ERROR"},
wantCalls: 2,
wantErr: false,
wantResponse: &MockResource{Status: "ACTIVE"},
},
{
name: "Success - Retryable 502 Gateway Error during Deletion followed by 404",
fetchResponses: []fetchResponse{
{res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadGateway}},
{res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusNotFound}},
},
activeStates: nil,
wantCalls: 2,
wantErr: false,
wantResponse: nil,
},
{
name: "Success - Immediate Active State",
fetchResponses: []fetchResponse{
{res: &MockResource{Status: "ACTIVE"}, err: nil},
},
activeStates: []string{"ACTIVE"},
wantCalls: 1,
wantErr: false,
wantResponse: &MockResource{Status: "ACTIVE"},
},
{
name: "Success - Pending State transitioned to Active State",
fetchResponses: []fetchResponse{
{res: &MockResource{Status: "CREATING"}, err: nil},
{res: &MockResource{Status: "ACTIVE"}, err: nil},
},
activeStates: []string{"ACTIVE"},
wantCalls: 2,
wantErr: false,
wantResponse: &MockResource{Status: "ACTIVE"},
},
{
name: "Success - Deletion (404 Not Found)",
fetchResponses: []fetchResponse{
{res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusNotFound}},
},
activeStates: nil,
wantCalls: 1,
wantErr: false,
wantResponse: nil,
},
{
name: "Failure - Non-retryable HTTP Error (400 Bad Request)",
fetchResponses: []fetchResponse{
{res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadRequest}},
},
activeStates: []string{"ACTIVE"},
wantCalls: 1,
wantErr: true,
wantResponse: nil,
},
{
name: "Failure - Retry limit reached for temporary error",
fetchResponses: []fetchResponse{
{res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadGateway}},
{res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadGateway}},
{res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadGateway}},
{res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadGateway}},
{res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadGateway}},
},
activeStates: []string{"ACTIVE"},
wantCalls: 5,
wantErr: true,
wantResponse: nil,
},
{
name: "Failure - Pending State transitioned to Error State",
fetchResponses: []fetchResponse{
{res: &MockResource{Status: "CREATING"}, err: nil},
{res: &MockResource{Status: "FAILED"}, err: nil},
},
activeStates: []string{"ACTIVE"},
errorStates: []string{"FAILED"},
wantCalls: 2,
wantErr: true,
wantResponse: &MockResource{Status: "FAILED"},
},
{
name: "Failure - Non-GenericOpenAPIError on fetch",
fetchResponses: []fetchResponse{
{res: nil, err: fmt.Errorf("network connection failure")},
},
activeStates: []string{"ACTIVE"},
wantCalls: 1,
wantErr: true,
wantResponse: nil,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// synctest for the fake clock
synctest.Test(t, func(t *testing.T) {
calls := 0
w := &WaiterHelper[MockResource, string]{
FetchInstance: func() (*MockResource, error) {
calls++
if calls <= len(tt.fetchResponses) {
resp := tt.fetchResponses[calls-1]
return resp.res, resp.err
}
return nil, fmt.Errorf("unexpected fetch call %d", calls)
},
GetState: func(m *MockResource) (string, error) {
return m.Status, m.Error
},
DeleteHttpErrorStatusCodes: tt.deleteHttpErrorStatusCodes,
ActiveState: tt.activeStates,
ErrorState: tt.errorStates,
}

handler := New(w.Wait()).SetThrottle(10 * time.Millisecond)
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()

res, err := handler.WaitWithContext(ctx)

if (err != nil) != tt.wantErr {
t.Fatalf("WaitWithContext() error = %v, wantErr %v", err, tt.wantErr)
}

if tt.wantCalls > 0 && calls != tt.wantCalls {
t.Errorf("FetchInstance calls = %d, want %d", calls, tt.wantCalls)
}

if diff := cmp.Diff(tt.wantResponse, res); diff != "" {
t.Errorf("WaitWithContext() response mismatch (-want +got):\n%s", diff)
}
})
})
}
}
Loading