From dc5e1f4cdc582ea5dfb180950a0057247f4c168e Mon Sep 17 00:00:00 2001 From: Jonas Schlecht Date: Tue, 15 Sep 2026 16:16:40 +0200 Subject: [PATCH 1/4] fix(core): fix waiter bug that caused it to return nil nil Relates to STACKITTPR-844 --- CHANGELOG.md | 2 + core/CHANGELOG.md | 3 + core/VERSION | 2 +- core/wait/wait.go | 4 + core/wait/wait_test.go | 40 ++++++++ core/wait/waiterhelper.go | 2 +- core/wait/waiterhelper_test.go | 176 ++++++++++++++++++++++++++++++++- 7 files changed, 222 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f241087fda..31e86aae5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,8 @@ - **Improvement:** Add HTTP 429 rate limit error responses in API operations - **Breaking Change:** Field `Expires` in `AccessKey` model is now `NullableString` - **Breaking Change:** Field `Expires` in `CreateAccessKeyPayload` model is now a `time.time` pointer +- `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`: diff --git a/core/CHANGELOG.md b/core/CHANGELOG.md index 61dc112528..f872c34cee 100644 --- a/core/CHANGELOG.md +++ b/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 diff --git a/core/VERSION b/core/VERSION index 0a8bf80d69..04e94578bd 100644 --- a/core/VERSION +++ b/core/VERSION @@ -1 +1 @@ -v0.27.0 +v0.27.1 diff --git a/core/wait/wait.go b/core/wait/wait.go index 86db3fa3a1..ece35cf70a 100644 --- a/core/wait/wait.go +++ b/core/wait/wait.go @@ -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 } if done { return res, nil diff --git a/core/wait/wait_test.go b/core/wait/wait_test.go index 5abbf2fe1e..57f5a97b4d 100644 --- a/core/wait/wait_test.go +++ b/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 diff --git a/core/wait/waiterhelper.go b/core/wait/waiterhelper.go index 6cfa041ab3..63c30776e2 100644 --- a/core/wait/waiterhelper.go +++ b/core/wait/waiterhelper.go @@ -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) diff --git a/core/wait/waiterhelper_test.go b/core/wait/waiterhelper_test.go index fe727aa708..773baf35e2 100644 --- a/core/wait/waiterhelper_test.go +++ b/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,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, }, @@ -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) + } + }) + }) + } +} From d5a7fa0e20d65d418e65b8b4ce9a9e4ab36d3fc0 Mon Sep 17 00:00:00 2001 From: Jonas Schlecht Date: Tue, 22 Sep 2026 16:00:42 +0200 Subject: [PATCH 2/4] chore: apply PR feedback --- core/wait/waiterhelper.go | 2 +- core/wait/waiterhelper_test.go | 14 +++++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/core/wait/waiterhelper.go b/core/wait/waiterhelper.go index 63c30776e2..fbad5003d6 100644 --- a/core/wait/waiterhelper.go +++ b/core/wait/waiterhelper.go @@ -49,7 +49,7 @@ func (w *WaiterHelper[T, S]) Wait() AsyncActionCheck[T] { state, err := w.GetState(instance) if err != nil { - return true, nil, err + return false, nil, err } // 1. Check if the operation succeeded diff --git a/core/wait/waiterhelper_test.go b/core/wait/waiterhelper_test.go index 773baf35e2..22b405df67 100644 --- a/core/wait/waiterhelper_test.go +++ b/core/wait/waiterhelper_test.go @@ -122,7 +122,7 @@ func TestWaiterHelper_Wait(t *testing.T) { name: "Failure - GetState failed", fetchResult: &MockResource{Error: fmt.Errorf("can not read state")}, activeStates: []string{"READY"}, - wantFinished: true, + wantFinished: false, wantErr: true, }, } @@ -194,6 +194,18 @@ func TestWaiterHelper_WaitWithContext(t *testing.T) { 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{ From 441d26db1f733561a4ada860a1903d57d6266d8c Mon Sep 17 00:00:00 2001 From: Jonas Schlecht Date: Wed, 23 Sep 2026 10:23:42 +0200 Subject: [PATCH 3/4] refactor: replace done reassignment with else if branch --- core/wait/wait.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/core/wait/wait.go b/core/wait/wait.go index ece35cf70a..960b19974c 100644 --- a/core/wait/wait.go +++ b/core/wait/wait.go @@ -102,12 +102,7 @@ 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 - } - if done { + } else if done { return res, nil } From 8e92aab1d3b58df9dc0cba890315daa12e845892 Mon Sep 17 00:00:00 2001 From: Jonas Schlecht Date: Wed, 23 Sep 2026 10:26:31 +0200 Subject: [PATCH 4/4] chore: rebase changes --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 31e86aae5b..c97343248e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,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` @@ -26,8 +28,6 @@ - **Improvement:** Add HTTP 429 rate limit error responses in API operations - **Breaking Change:** Field `Expires` in `AccessKey` model is now `NullableString` - **Breaking Change:** Field `Expires` in `CreateAccessKeyPayload` model is now a `time.time` pointer -- `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`: