fix(core): fix waiter bug that caused it to return nil nil - #11418
Open
SerseusWasTaken wants to merge 1 commit into
Open
SerseusWasTaken wants to merge 1 commit into
SerseusWasTaken wants to merge 1 commit into
Conversation
Relates to STACKITTPR-844
gorelease reportgenerated on Wed Sep 16 10:16:49 UTC 2026 |
SerseusWasTaken
commented
Sep 16, 2026
Comment on lines
+105
to
+108
| // 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 |
Contributor
Author
There was a problem hiding this comment.
This guard is debatable
SerseusWasTaken
commented
Sep 16, 2026
Comment on lines
+389
to
+427
| // 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) | ||
| } | ||
| }) | ||
| } |
Contributor
Author
There was a problem hiding this comment.
This regression test is not needed if we decide to get rid of the guard
SerseusWasTaken
marked this pull request as ready for review
September 16, 2026 10:50
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes a bug in waithandlerhelper that caused some wait handlers to return (nil, nil).
It also introduces a way to prevent future wait handlers to return the wrong
waitFinishedstate by overriding it tofalsein wait.go. The related regression test proves that the workaround works, while the otherTestWaiterHelper_WaitWithContexttest covers more of the waiter helper handler implementation.Relates to STACKITTPR-844
Checklist
make fmtexamples/directory)make test(will be checked by CI)make lint(will be checked by CI)