Fix IOException in MSSQL pipeline by disposing config file watchers#3243
Fix IOException in MSSQL pipeline by disposing config file watchers#3243aaronburtle wants to merge 9 commits intomainfrom
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
aaronburtle
requested review from
Alekhya-Polavarapu,
Aniruddh25,
JerryNixon,
RubenCerna2079,
anushakolan,
rusamant,
sourabh1007,
souvikghosh04,
stuartpa and
vadeveka
as code owners
There was a problem hiding this comment.
Pull request overview
This PR addresses intermittent MSSQL pipeline test failures caused by lingering config file watchers holding file handles during test cleanup, leading to IOException on File.Delete. It introduces explicit disposal of config file watcher resources and adds a retry-based safety net in test cleanup.
Changes:
- Implement
IDisposableonConfigFileWatcherto stop events, unsubscribe handlers, and dispose the underlying watcher. - Implement
IDisposableonFileSystemRuntimeConfigLoaderso DI can dispose the ownedConfigFileWatcherwhen the test server shuts down. - Add exponential backoff retries around test config file deletion in
CleanupAfterEachTest.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Service.Tests/Configuration/ConfigurationTests.cs | Adds retry/backoff around deleting the custom config file during test cleanup to reduce flakiness from transient file locks. |
| src/Config/FileSystemRuntimeConfigLoader.cs | Makes the loader disposable so the DI container can clean up the hot-reload watcher on shutdown. |
| src/Config/ConfigFileWatcher.cs | Makes the watcher disposable to stop notifications and release OS resources/file handles. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
souvikghosh04
left a comment
souvikghosh04
left a comment
There was a problem hiding this comment.
Approved with a follow-up comment
| @@ -710,9 +710,25 @@ type Moon { | |||
| [TestCleanup] | |||
| public void CleanupAfterEachTest() | |||
There was a problem hiding this comment.
The true auto-dispose (as mentioned in PR description) of FileSystemRuntimeConfigLoader when TestServer shuts down can be done with using block. tests in ConfigurationTests.cs create TestServer without using. If the server isn't explicitly disposed, the DI container's IDisposable cleanup may never run and the file watcher could be undisposed. retry in CleanupAfterEachTest covers this as a safety net but the more correct fix would be ensuring those TestServer instances are disposed. This could be a follow-up PR if not possible in this PR.
There was a problem hiding this comment.
Agreed, the 20 or so TestServer instances that don't have using won't trigger the auto-dispose, Ill follow up on this in a future PR to keep the scope of this one from growing, and the CleanupAfterEachTest should cover us in the meantime.
There was a problem hiding this comment.
@aaronburtle,
If we dont add using in this same change, the PR's title is not serving its purpose. It is not disposing the file watchers. It is only implementing the dispose function.
I think all those FileSystemRuntimeConfigLoader should be actually disposed as well in this same PR.
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
… of github.com:Azure/data-api-builder into dev/aaronburtle/MSSQLIntegrationTestPipelineCleanupFix
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
| @@ -710,9 +710,25 @@ type Moon { | |||
| [TestCleanup] | |||
| public void CleanupAfterEachTest() | |||
There was a problem hiding this comment.
@aaronburtle,
If we dont add using in this same change, the PR's title is not serving its purpose. It is not disposing the file watchers. It is only implementing the dispose function.
I think all those FileSystemRuntimeConfigLoader should be actually disposed as well in this same PR.
Why make this change?
Closes #2992
Several configuration tests create a TestServer that sets up a
ConfigFileWatcherto support hot-reload. When the file watcher detects a change, its callback reads the file, holding a read handle. NeitherConfigFileWatchernorFileSystemRuntimeConfigLoaderimplementedIDisposable, so when the TestServer is disposed, the DI container could not clean up the file watcher. The watcher remained active, and if its callback was mid-read whenCleanupAfterEachTestcalledFile.Delete, the delete failed with anIOExceptionWhat is this change?
ConfigFileWatchernow implementsIDisposable. It stops raising events, unsubscribes the Changed handler, and disposes the underlyingIFileSystemWatcher.FileSystemRuntimeConfigLoadernow implementsIDisposable. It disposes the ownedConfigFileWatcher. As a DI singleton, it is now automatically disposed when the TestServer shuts down.CleanupAfterEachTestretries with exponential backoff. As a safety net,File.Deleteis retried up to 3 times onIOException(2s, 4s, 8s delays), matching the existing retry pattern used elsewhere.How was this tested?
Run against the pipeline. Several pipeline runs were initiated to add certainty that the fix is working, since this issue is intermittent.
https://dev.azure.com/sqldab/Data%20API%20builder%20Dependency%20Packages/_build/results?buildId=18957&view=results
https://dev.azure.com/sqldab/Data%20API%20builder%20Dependency%20Packages/_build/results?buildId=18958&view=results
https://dev.azure.com/sqldab/Data%20API%20builder%20Dependency%20Packages/_build/results?buildId=18952&view=results
https://dev.azure.com/sqldab/Data%20API%20builder%20Dependency%20Packages/_build/results?buildId=18959&view=results
https://dev.azure.com/sqldab/Data%20API%20builder%20Dependency%20Packages/_build/results?buildId=18960&view=results