Skip to content

test: poll for connection401Error in startWithHttp401PreventsSubsequentStart test#337

Draft
devin-ai-integration[bot] wants to merge 1 commit intomainfrom
devin/1774461449-fix-flaky-startWithHttp401PreventsSubsequentStart
Draft

test: poll for connection401Error in startWithHttp401PreventsSubsequentStart test#337
devin-ai-integration[bot] wants to merge 1 commit intomainfrom
devin/1774461449-fix-flaky-startWithHttp401PreventsSubsequentStart

Conversation

@devin-ai-integration
Copy link
Contributor

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Related issues

Follow-up to b8c2f2a which fixed the same race condition in the sibling test startWithHttp401ShutsDownSink. This test was identified as the cause of the CI failure on PR #326.

Describe the solution you've provided

The startWithHttp401PreventsSubsequentStart test is flaky due to a race condition in StreamingDataSource.onError(). The production code calls resultCallback.onError() (line 136, which unblocks the test's awaitError()) before setting connection401Error = true (line 138). The test thread can resume and call sds.start(callback2) before the background thread reaches line 138, causing the second start() to pass the !connection401Error guard and produce an unexpected error callback.

This PR adds a polling loop (up to 1 second, probing every 50ms) that waits for connection401Error to be set before making the final assertion. Since connection401Error is private, the loop probes by calling sds.start() and checking whether it's a no-op (no error produced).

⚠️ Review note: The polling loop calls sds.start(probe) repeatedly while connection401Error is not yet set. Each probe that slips through the guard will create a real connection to the test HTTP server and receive a 401. Verify you're comfortable with these transient side effects during the polling window.

Describe alternatives you've considered

An alternative would be to fix the ordering in the production code (StreamingDataSource.onError()) so that connection401Error = true is set before resultCallback.onError() is called, eliminating the race at the source. This would be a cleaner fix, but the polling-loop-in-test approach is the established pattern in this codebase (see startWithHttp401ShutsDownSink and AssertHelpers.assertPolledFunctionReturnsValue), so this PR follows that convention.

Additional context

Only one test file is changed; no production code is modified.

Link to Devin session: https://app.devin.ai/sessions/b09435a9a0094a57bf716742eeb865ff

…ntStart test

Co-Authored-By: Aaron Zeisler <azeisler@launchdarkly.com>
@devin-ai-integration
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants