fix(core): fix waiter bug that caused it to return nil nil

CHANGELOG.md

@@ -11,6 +11,8 @@
     - `v1api`:
       - **New:** Add package which can be used for communication with the STACKIT automation v1 API
       - **Feature:** Add waiter method for the API
+- `core`: [v0.27.1](core/CHANGELOG.md#v0271)
+  - **Bugfix:** `WaitWithContext` no longer returns `(nil, nil)` after a single retryable `502`/`504` error
 - `iaas`:
   - [v1.14.4](services/iaas/CHANGELOG.md#v1144)
     - **Dependencies:** Bump STACKIT SDK resourcemanager module from `v0.25.1` to `v0.26.0`

core/CHANGELOG.md

@@ -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
 

core/VERSION

@@ -1 +1 @@
-v0.27.0
+v0.27.1

core/wait/wait.go

@@ -102,8 +102,7 @@ func (h *AsyncActionHandler[T]) WaitWithContext(ctx context.Context) (res *T, er
 			if err != nil {
 				return res, err
 			}
-		}
-		if done {
+		} else if done {
 			return res, nil
 		}
 

core/wait/wait_test.go

@@ -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)
+		}
+	})
+}
+
 func TestHandleError(t *testing.T) {
 	for _, tt := range []struct {
 		desc              string

core/wait/waiterhelper.go

@@ -44,12 +44,12 @@ 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)
 		if err != nil {
-			return true, nil, err
+			return false, nil, err
 		}
 
 		// 1. Check if the operation succeeded

core/wait/waiterhelper_test.go

@@ -1,9 +1,12 @@
 package wait
 
 import (
+	"context"
 	"fmt"
 	"net/http"
 	"testing"
+	"testing/synctest"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 
@@ -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,
 		},
@@ -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,
 		},
@@ -104,22 +107,22 @@ 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,
 		},
 		{
 			name:         "Failure - GetState failed",
 			fetchResult:  &MockResource{Error: fmt.Errorf("can not read state")},
 			activeStates: []string{"READY"},
-			wantFinished: true,
+			wantFinished: false,
 			wantErr:      true,
 		},
 	}
@@ -161,3 +164,178 @@ 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 504 Error followed by Active State",
+			fetchResponses: []fetchResponse{
+				{res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusGatewayTimeout}},
+				{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)
+				}
+			})
+		})
+	}
+}