[6.1.7 Cherry-pick] Fix | Scope configurable retry logic assembly resolution to opt-in callers - #4664
Merged
Merged
Conversation
To resolve, run: git cherry-pick 7c5daeb
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
…llers (#4547) * Scope configurable retry logic assembly resolution 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 * Test | Address review feedback on retry logic assembly probing tests Replaces the internals-based assertions in the configurable retry logic regression tests with a behavioural probe, and restores the UTF-8 BOM that was dropped from the functional test file. The unit test previously read AssemblyLoadContext's private _resolving field to check whether a handler was still subscribed. That reflects into runtime internals we do not own, so the value cannot simply be exposed internally as review suggested. The functional test took a different but also problematic approach, mutating Environment.CurrentDirectory, which is process-wide state and unsafe under parallel test execution. Both now plant a file that is not a valid assembly in the loader's probing directory (AppContext.BaseDirectory) under a name no other component could request, then assert that Assembly.Load reports it as not found. A subscribed handler would locate that file and surface BadImageFormatException instead, so the assertion discriminates cleanly while observing only public behaviour and touching no shared process state. Verified by temporarily reintroducing the unconditional subscription: all four unit tests and the functional test fail, and pass again once removed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e8018b20-0e43-4de9-93dd-c8080a81a801 * Test | Cover the successful retry logic type resolution path The existing tests only covered the code paths where the assembly probing handler is never subscribed. The path that legitimately subscribes it, a configured custom retry logic type that actually resolves, was untested, so nothing verified that the handler is removed again afterwards. Add a test that resolves a retry logic factory out of the loader's probing directory and asserts that no probing handler remains subscribed once the loader has been constructed. An invocation counter on the factory confirms the configured type really was resolved and used, rather than the loader silently falling back to the built-in factory. Verified the test is sensitive to both behaviours it covers: pointing the loader's probing directory elsewhere makes it fail, and restoring the unconditional handler subscription makes it fail. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e8018b20-0e43-4de9-93dd-c8080a81a801 * Test | Widen probe file cleanup to non-IO failures File.Delete can fail with UnauthorizedAccessException as well as IOException. Catching only the latter meant a cleanup failure could surface as a test failure that had nothing to do with the behaviour under test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e8018b20-0e43-4de9-93dd-c8080a81a801 * Keep retry logic assembly probing active during provider construction Addresses review feedback that scoping the assembly resolving handler to type resolution alone could break existing consumers whose configured retryLogicType has private dependencies. The handler is now subscribed before LoadType and removed only after CreateInstance has run the configured type's constructor and invoked its retry method, so dependency loads triggered during construction are still resolved. Adds Switch.Microsoft.Data.SqlClient.UseLegacyRetryLogicAssemblyResolution as an escape hatch that restores the process-lifetime handler. The switch restores lifetime only; the probing directory remains AppContext.BaseDirectory, so it cannot re-enable the binary planting vector. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e8018b20-0e43-4de9-93dd-c8080a81a801 * Treat a whitespace-only retryLogicType as not configured A whitespace value has no type to resolve, so it previously installed the resolving handler, attempted resolution and then fell back to the built-in factory. Skipping the subscription reaches the same provider without changing assembly resolution behavior on the application's behalf. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e8018b20-0e43-4de9-93dd-c8080a81a801 * Remove the UseLegacyRetryLogicAssemblyResolution app context switch The switch restored the process-lifetime assembly resolving handler for the one case that scoping cannot cover: a custom retry logic provider whose private dependency is first touched after the provider has been constructed. Shipping a supported way to permanently reinstate a process-wide handler on AssemblyLoadContext.Default works against the point of the change. The driver should not be altering assembly resolution for the whole application on behalf of configurable retry logic, and an affected provider has a simple fix of its own: reference the dependency normally so it lands in deps.json, or register a resolving handler in the application. The handler is now always subscribed only while a configured provider is being resolved and constructed, and only when a custom retry logic type has been configured. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e8018b20-0e43-4de9-93dd-c8080a81a801 * Refactor retry assembly resolution subscription Encapsulate the temporary AssemblyLoadContext resolving handler in an IDisposable subscription so cleanup is tied to a using scope. Remove the handler immediately when custom type resolution falls back to the built-in factory, and add direct unit coverage for disposal. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e8018b20-0e43-4de9-93dd-c8080a81a801 --------- 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.
🟡 Changes recommended
The new unit-test file has two branch-specific compilation blockers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Scopes configurable retry assembly resolution to custom providers and probes from the application base directory.
Changes:
- Adds temporary, disposable assembly-resolution subscriptions.
- Skips custom resolution when no custom type is configured.
- Adds regression coverage for handler lifecycle.
File summaries
| File | Description |
|---|---|
SqlConfigurableRetryLogicLoader.cs |
Implements scoped assembly resolution. |
SqlConfigurableRetryLogicLoaderTest.cs |
Adds loader unit tests. |
SqlConfigurableRetryLogicTest.cs |
Adds functional regression coverage. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
benrr101
changed the title
cheenamalhotra
approved these changes
Sep 8, 2026
priyankatiwari08
approved these changes
Sep 9, 2026
priyankatiwari08
left a comment
priyankatiwari08
left a comment
Contributor
There was a problem hiding this comment.
Reviewed against #4547. The 6.1 backport includes the complete production fix and regression coverage, with appropriate branch-specific adjustments; no code concerns.
paulmedynski
enabled auto-merge (squash)
September 9, 2026 15:12
Contributor
|
/azp run |
|
Azure Pipelines: Successfully started running 2 pipeline(s). 3 pipeline(s) were filtered out due to trigger conditions. |
paulmedynski
approved these changes
Sep 9, 2026
This was referenced Sep 11, 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.
Cherry-pick of #4547 (7c5daeb) into
release/6.1failed due to merge conflicts.Please resolve manually:
git fetch origin git checkout dev/automation/pr-4547-to-6.1.7 git cherry-pick 7c5daeb228a44114fe7d9608c71826c7bf95da7c # resolve conflicts git push origin dev/automation/pr-4547-to-6.1.7 --force