Fix memory leak: RuntimeConfigProvider._changeTokenRegistration never disposed#3404
Open
aaronburtle wants to merge 2 commits intomainfrom
Open
Fix memory leak: RuntimeConfigProvider._changeTokenRegistration never disposed#3404aaronburtle wants to merge 2 commits intomainfrom
aaronburtle wants to merge 2 commits intomainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a resource leak in the service startup/config hot-reload path by ensuring RuntimeConfigProvider’s change-token registration is properly disposed when the host/service provider is torn down.
Changes:
- Make
RuntimeConfigProviderimplementIDisposableand guard against double-dispose while disposing theChangeToken.OnChangeregistration. - Update DI registrations to use singleton factory registrations (
AddSingleton<T>(sp => instance)) so the container owns and disposes the singleton. - Align affected test setups to use the same DI registration pattern.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Service/Startup.cs | Registers RuntimeConfigProvider via factory so DI disposes it at shutdown. |
| src/Core/Configurations/RuntimeConfigProvider.cs | Implements IDisposable and disposes _changeTokenRegistration. |
| src/Service.Tests/SqlTests/SqlTestBase.cs | Updates test DI registration to container-owned singleton factory. |
| src/Service.Tests/Mcp/McpQueryTimeoutTests.cs | Updates test DI registration to container-owned singleton factory. |
| src/Service.Tests/Mcp/EntityLevelDmlToolConfigurationTests.cs | Updates test DI registration to container-owned singleton factory. |
| src/Service.Tests/Mcp/DescribeEntitiesFilteringTests.cs | Updates test DI registration to container-owned singleton factory. |
| src/Service.Tests/Mcp/AggregateRecordsToolTests.cs | Updates test DI registration to container-owned singleton factory. |
| src/Service.Tests/CosmosTests/TestBase.cs | Updates test DI registrations for loader/provider to factory form for disposal. |
| src/Service.Tests/Authentication/JwtTokenAuthenticationUnitTests.cs | Updates test DI registration to container-owned singleton factory. |
| src/Service.Tests/Authentication/Helpers/WebHostBuilderHelper.cs | Updates test DI registrations to container-owned singleton factory. |
Aniruddh25
approved these changes
Apr 3, 2026
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?
Closes #3403
What is this change?
RuntimeConfigProviderhas aDispose()method that cleans up its_changeTokenRegistration(aChangeToken.OnChangesubscription), but this method was never called due to two compounding issues:The class did not implement the
IDisposableinterface it only had a publicDispose()method. The DI container only auto-disposes services that implementIDisposableorIAsyncDisposable.It was registered as a pre-created instance via
services.AddSingleton(configProvider). The .NET DI container does not dispose pre-created instances; only instances it creates itself via type or factory registrations.We also correctly register the loader in our tests using the same pattern (Loader was correctly registered in
Startup).How was this tested?
Run against our test suite.