test: poll for shutDownCalled in startWithHttp401PreventsSubsequentStart test#332
Draft
kinyoklion wants to merge 3 commits intomainfrom
Draft
test: poll for shutDownCalled in startWithHttp401PreventsSubsequentStart test#332kinyoklion wants to merge 3 commits intomainfrom
kinyoklion wants to merge 3 commits intomainfrom
Conversation
…art test The startWithHttp401PreventsSubsequentStart test is flaky due to a race condition. In StreamingDataSource.onError(), the production code calls resultCallback.onError() (which unblocks the test's callback.awaitError()) before setting connection401Error and calling dataSourceUpdateSink.shutDown(). The test thread can resume and call the second sds.start() before connection401Error is set, causing the second start to proceed and produce a callback. This adds a short polling loop (up to 1 second, checking every 10ms) to wait for shutDownCalled to become true before testing the second start, matching the pattern used in the startWithHttp401ShutsDownSink fix. Co-Authored-By: rlamb@launchdarkly.com <kingdewman@gmail.com>
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: rlamb@launchdarkly.com <kingdewman@gmail.com>
Co-Authored-By: rlamb@launchdarkly.com <kingdewman@gmail.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
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.
Requirements
Related issues
Follow-up to #329, which fixed the same race condition in the sibling test
startWithHttp401ShutsDownSink.Describe the solution you've provided
The
startWithHttp401PreventsSubsequentStarttest is flaky due to a race condition inStreamingDataSource.onError(). The production code callsresultCallback.onError()before settingconnection401Error = trueand callingdataSourceUpdateSink.shutDown(). Whencallback1.awaitError()returns, the test thread can immediately callsds.start(callback2)before the background thread has setconnection401Error, causing the second start to proceed and produce a callback — failing the assertion.The fix adds a polling loop (up to 1s, checking every 10ms) that waits for
dataSourceUpdateSink.shutDownCalledto become true before calling the secondstart(). SinceshutDown()is called afterconnection401Error = truein the production code, this guarantees the flag is set. This matches the pattern already used in thestartWithHttp401ShutsDownSinktest (merged via #329).Describe alternatives you've considered
Reordering the production code in
StreamingDataSource.onError()to setconnection401Errorand callshutDown()before firingresultCallback.onError()would eliminate the race at the source. However, this is a test-only race condition and the production ordering (notify first, then clean up) is intentional, so fixing the test is the safer approach.Additional context
Key review points:
shutDownCalledbecoming true guaranteesconnection401Erroris also true (it does — seeStreamingDataSource.javalines 137-139, whereconnection401Error = trueis set beforeshutDown()is called).@Rule Timeout.seconds(10)).CI verification
CI passed on 3 consecutive runs (initial push + 2 empty-commit re-triggers) to confirm the flake is resolved.
Link to Devin session: https://app.devin.ai/sessions/e45834672c2345108fb56b5fd044a400
Requested by: @kinyoklion