Fix | Scope configurable retry logic assembly resolution to opt-in callers - #4547
Draft
priyankatiwari08 wants to merge 1 commit into
Draft
Fix | Scope configurable retry logic assembly resolution to opt-in callers#4547priyankatiwari08 wants to merge 1 commit into
priyankatiwari08 wants to merge 1 commit into
Conversation
SqlConfigurableRetryLogicLoader subscribed a handler to AssemblyLoadContext.Default.Resolving in its constructor and never removed it. Because SqlConfigurableRetryLogicManager builds that loader on the default RetryLogicProvider path, simply reading SqlCommand.RetryLogicProvider or SqlConnection.RetryLogicProvider installed a permanent, process-wide assembly resolution hook. The hook then participated in resolving every assembly the host application failed to find, even though the application had not configured any custom retry logic type. It also probed Environment.CurrentDirectory, which is ambient process state unrelated to where the application's binaries live, so assemblies could be resolved from an unintended location. Applications observed this as load failures, and in #2214 as a stack overflow, originating inside SqlClient for assemblies unrelated to SqlClient. Changes: - Probe AppContext.BaseDirectory instead of Environment.CurrentDirectory. - Subscribe the resolving handler only for the duration of the Type.GetType call in LoadType, and remove it in a finally block. - Skip type resolution entirely when no retryLogicType is configured. retryLogicType is optional while retryMethod is required, so configurations selecting a built-in retry method previously still ran the custom type resolution path. Together these mean the handler is never installed unless the application explicitly configured a custom retry logic type, and is gone again as soon as that type has been resolved. Only .NET is affected; the .NET Framework code path does not use AssemblyLoadContext. Refs #2214, #2134 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e8018b20-0e43-4de9-93dd-c8080a81a801
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes SqlConfigurableRetryLogicLoader’s .NET AssemblyLoadContext.Default.Resolving usage so it only applies to explicit opt-in callers (custom retry-logic type resolution) and no longer probes the process working directory.
Changes:
- Switch configurable retry-logic assembly probing from
Environment.CurrentDirectorytoAppContext.BaseDirectory. - Scope
AssemblyLoadContext.Default.Resolvingsubscription to the duration of theType.GetType(...)call (subscribe +finallyunsubscribe). - Skip custom type resolution entirely when
retryLogicTypeis not configured (use built-in factory directly). - Add unit + functional regression tests validating the handler is not left attached and that current-directory probing is not enabled.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Reliability/SqlConfigurableRetryLogicLoader.cs | Scopes the resolving handler to opt-in type resolution and switches probing to AppContext.BaseDirectory. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConfigurableRetryLogicLoaderTest.cs | New unit tests validating no SqlConfigurableRetryLogicLoader resolving delegates remain attached. |
| src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConfigurableRetryLogicTest.cs | Adds functional regression test ensuring retry-logic initialization does not enable current-directory assembly probing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
5
to
9
| using System; | ||
| using System.IO; | ||
| using System.Reflection; | ||
| using System.Threading.Tasks; | ||
| using Xunit; |
Comment on lines
+102
to
+106
| string probeDirectory = Path.Combine(Path.GetTempPath(), "mds-crl-plant-" + Guid.NewGuid().ToString("N")); | ||
| Directory.CreateDirectory(probeDirectory); | ||
| string originalCurrentDirectory = Environment.CurrentDirectory; | ||
|
|
||
| try |
priyankatiwari08
marked this pull request as draft
August 19, 2026 09:54
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.
Summary
SqlConfigurableRetryLogicLoadersubscribed a handler toAssemblyLoadContext.Default.Resolvingin its constructor and never removed it.SqlConfigurableRetryLogicManagerbuilds that loader lazily on the defaultRetryLogicProviderpath, so simply readingSqlCommand.RetryLogicProviderorSqlConnection.RetryLogicProvider— which any app using retry logic does — installed a permanent, process-wide assembly resolution hook.Two consequences:
Environment.CurrentDirectory. The working directory is ambient process state that any code in the process can change and that bears no relationship to where the application's binaries live, so assemblies could be resolved from an unintended location.Only .NET is affected — the .NET Framework code path does not use
AssemblyLoadContext.Changes
AppContext.BaseDirectoryinstead ofEnvironment.CurrentDirectory.Type.GetTypecall inLoadType, removing it in afinally.retryLogicTypeis configured.On (3): in
AppConfigManager,retryLogicTypeis optional whileretryMethodisIsRequired = true. So a config that selected a built-in retry method still went through the custom-type resolution path and installed the handler. It now short-circuits straight toSqlConfigurableRetryFactory.Net effect: the handler is never installed unless the application explicitly configured a custom retry logic type, and it is removed again as soon as that type is resolved.
Tests
SqlConfigurableRetryLogicLoaderTest(UnitTests — needsInternalsVisibleTo, which FunctionalTests does not have). Asserts no delegate declared bySqlConfigurableRetryLogicLoaderremains attached toAssemblyLoadContext.Default.Resolvingfor three cases: no configuration, config withoutretryLogicType, and config with an unresolvableretryLogicType.RetryLogicProviderDoesNotEnableCurrentDirectoryAssemblyProbing(FunctionalTests). Writes a non-assembly file named<name>.dllinto a temp directory, makes it the working directory, and assertsAssembly.LoadreportsFileNotFoundExceptionrather thanBadImageFormatException.Both were confirmed to fail against the pre-fix code and pass after.
Validated on
net462,net8.0,net9.0, andnet10.0.Checklist
internal), so noref/updates requiredSuggested release note entry
Related
Refs #2214, #2134