From 25eae9108ce729abad7026f4f1b36f917fd554e8 Mon Sep 17 00:00:00 2001 From: Jonas Schlecht Date: Tue, 15 Sep 2026 16:16:40 +0200 Subject: [PATCH] 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 8cce3af2a6..16c63d932b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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`: 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) + } + }) + }) + } +}