Fix intermittent hot-reload test failures caused by metadata initialization race condition#3302
Open
aaronburtle wants to merge 5 commits intomainfrom
Open
Fix intermittent hot-reload test failures caused by metadata initialization race condition#3302aaronburtle wants to merge 5 commits intomainfrom
aaronburtle wants to merge 5 commits intomainfrom
Conversation
aaronburtle
requested review from
Alekhya-Polavarapu,
Aniruddh25,
JerryNixon,
RubenCerna2079,
anushakolan,
rusamant,
sourabh1007,
souvikghosh04,
stuartpa and
vadeveka
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Improves reliability of MSSQL hot-reload integration tests by adding post–hot-reload readiness polling/retry logic, addressing a race where config validation completes before metadata providers are fully re-initialized.
Changes:
- Added REST polling helper (
WaitForRestEndpointAsync) and updated tests to wait for REST readiness after hot-reload. - Added retry loop for the GraphQL request in
HotReloadConfigRuntimePathsEndToEndTestto tolerate transient post–hot-reload failures. - Updated connection-string and database-type hot-reload tests to use the shared REST polling helper.
src/Service.Tests/Configuration/HotReload/ConfigurationHotReloadTests.cs
Show resolved
Hide resolved
src/Service.Tests/Configuration/HotReload/ConfigurationHotReloadTests.cs
Show resolved
Hide resolved
src/Service.Tests/Configuration/HotReload/ConfigurationHotReloadTests.cs
Outdated
Show resolved
Hide resolved
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
souvikghosh04
approved these changes
Mar 20, 2026
Contributor
souvikghosh04
left a comment
souvikghosh04
left a comment
There was a problem hiding this comment.
thanks for the fix. approved with suggestions.
src/Service.Tests/Configuration/HotReload/ConfigurationHotReloadTests.cs
Outdated
Show resolved
Hide resolved
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Aniruddh25
reviewed
Mar 20, 2026
| await Task.Delay(delayMilliseconds); | ||
| } | ||
|
|
||
| // Return the last response (undisposed) so the caller can inspect/assert on it. |
Collaborator
There was a problem hiding this comment.
The caller should dispose the last response then explicitly, or use the using clause.
Aniruddh25
requested changes
Mar 20, 2026
Collaborator
Aniruddh25
left a comment
Aniruddh25
left a comment
There was a problem hiding this comment.
Should dispose the last response.
github-project-automation
bot
moved this from In Progress
to Review In Progress
in Data API builder
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
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.
Why make this change?
Related to #2992
Some flakey tests were being masked by faster initialization when the task they were in was isolated. When we refactored the tests to lower the overall runtime in this PR #3245 these flakey tests were eventually revealed.
The root cause was a race condition: after a successful hot-reload,
WaitForConditionAsyncdetects the "Validated hot-reloaded configuration file" console message and returns, but the engine's metadata providers have not fully re-initialized yet. An immediate HTTP request can arrive before the metadata is ready, causing a 500 error.What is this change?
Added retry logic to the three non-ignored tests that make HTTP calls expecting success responses immediately after hot-reload:
HotReloadConfigRuntimePathsEndToEndTest: REST and GraphQL calls now retry up to 5 times with 1 second delaysHotReloadConfigConnectionString: REST call now usesWaitForRestEndpointAsynchelperHotReloadConfigDatabaseType: REST call now usesWaitForRestEndpointAsynchelperAdded a shared
WaitForRestEndpointAsynchelper method that polls a REST endpoint until it returns the expected status code (5 retries, 1 second delay).This follows the same retry pattern already established in
HotReloadConfigDataSource, which had this exact fix applied previously.How was this tested?
Ran a batch of 10 pipeline runs which all succeeded.