Fix | Scope configurable retry logic assembly resolution to opt-in callers

[priyankatiwari08]

Summary

SqlConfigurableRetryLogicLoader subscribed a handler to AssemblyLoadContext.Default.Resolving in its constructor and never removed it.

SqlConfigurableRetryLogicManager builds that loader lazily on the default RetryLogicProvider path, so simply reading SqlCommand.RetryLogicProvider or SqlConnection.RetryLogicProvider — which any app using retry logic does — installed a permanent, process-wide assembly resolution hook.

Two consequences:

1. It applied to everything. Once installed, the handler participated in resolving every assembly the host application failed to find, even though the application had never configured a custom retry logic type. Apps saw failures (and in Stack overflow caused by SqlConfigurableRetryLogicLoader.Defaul_resolving #2214 a stack overflow) surfacing from inside SqlClient for assemblies that have nothing to do with SqlClient.
2. It probed the wrong directory. The handler resolved against 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.

Scope

This is the mitigation intended for the hotfix branches: 7.1.0, backported to 7.0 and 6.1 so the issue is resolved on all production branches. It deliberately does not change the underlying design of how retry logic assemblies are loaded.

The two larger design questions raised in review — loading configured retry logic into a dedicated AssemblyLoadContext, and replacing type-name resolution with an explicit registration API — are tracked separately in #4623 for 8.0.

Changes

#	Change
1	Probe AppContext.BaseDirectory instead of Environment.CurrentDirectory.
2	Subscribe the resolving handler only when a custom retryLogicType is configured, and only for the duration of resolving and constructing that provider.
3	Skip type resolution entirely when no retryLogicType is configured.

On (3): in AppConfigManager, retryLogicType is optional while retryMethod is IsRequired = 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 to SqlConfigurableRetryFactory.

On (2): the handler spans both LoadType and CreateInstance. LoadType only produces a Type — the configured type's constructor and retry method run inside CreateInstance, and a provider that touches its own private dependencies while being constructed needs the handler installed at that point.

Net effect: the handler is never installed unless the application explicitly configured a custom retry logic type, and it is removed again once that provider has been constructed.

On the one case scoping does not cover

A provider that loads a private dependency after it has been constructed — at retry time — will no longer find that dependency through the driver's handler. We intentionally do not keep the process-wide handler installed for that case because the driver should not alter assembly resolution for the whole application on behalf of configurable retry logic.

An affected provider can reference the dependency normally so it lands in deps.json, or register a resolving handler in the application. The dedicated AssemblyLoadContext tracked in #4623 closes this case properly.

Tests

• New SqlConfigurableRetryLogicLoaderTest (UnitTests — needs InternalsVisibleTo, which FunctionalTests does not have). Covers: no configuration, config without retryLogicType, whitespace-only retryLogicType, and an unresolvable retryLogicType — asserting in each case that no handler declared by the loader remains attached to AssemblyLoadContext.Default.Resolving.
• New RetryLogicTypeResolution_KeepsAssemblyProbingEnabledWhileProviderIsConstructed. Plants a retry factory whose constructor asserts, from inside CreateInstance, that the handler is installed, and asserts it is gone once resolution returns — so both the add and the remove are observed, at the point in the sequence that matters.
• New AssemblyResolutionSubscription_DisposeRemovesAssemblyProbingHandler. Directly verifies that disposing the scoped subscription removes its handler.
• New RetryLogicProviderDoesNotEnableCurrentDirectoryAssemblyProbing (FunctionalTests). Writes a non-assembly file named <name>.dll into a temp directory, makes it the working directory, and asserts Assembly.Load reports FileNotFoundException rather than BadImageFormatException.

The directory and handler-leak tests were confirmed to fail against the pre-fix code and pass after.

Validated on net462, net8.0, net9.0, and net10.0.

Checklist

• Tests added
• No public API changes (all affected types are internal), so no ref/ updates required
• No breaking changes to the resolution result — configured custom retry types still resolve to the same provider
• Verified against the reported scenario in Stack overflow caused by SqlConfigurableRetryLogicLoader.Defaul_resolving #2214

Suggested release note entry

Fixed SqlConfigurableRetryLogicLoader installing a permanent, process-wide AssemblyLoadContext.Default.Resolving handler that probed the current working directory. The handler is now installed only while a configured custom retry logic type is being resolved and constructed, and probes the application base directory. (#2214)

Related

Refs #2214, #2134
Follow-up design work tracked in #4623

[Copilot]

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.CurrentDirectory to AppContext.BaseDirectory.
• Scope AssemblyLoadContext.Default.Resolving subscription to the duration of the Type.GetType(...) call (subscribe + finally unsubscribe).
• Skip custom type resolution entirely when retryLogicType is 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.

[benrr101]

Since it's still a draft, leaving a comment

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[paulmedynski]

These changes don't remove the attack surface - they narrow it. Can we do better?

Also, we're missing tests that prove we add and then remove Default_Resolving from the default ALC under all of the code paths, and that we don't add it at all in some cases.

[codecov]

Codecov Report

❌ Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 63.76%. Comparing base (bd36b43) to head (6476d99).
⚠️ Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
...ent/Reliability/SqlConfigurableRetryLogicLoader.cs	95.65%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4547      +/-   ##
==========================================
- Coverage   64.75%   63.76%   -1.00%     
==========================================
  Files         288      284       -4     
  Lines       44418    67920   +23502     
==========================================
+ Hits        28763    43307   +14544     
- Misses      15655    24613    +8958     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	63.76% <95.65%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConfigurableRetryLogicTest.cs:127

• Cleanup is described as best-effort, but File.Delete can throw exceptions other than IOException (e.g., UnauthorizedAccessException). To keep this regression test from failing due to cleanup issues in restrictive environments, catch a broader exception (or explicitly include UnauthorizedAccessException) in the cleanup block.

                catch (IOException)

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[Copilot]

🟡 Changes recommended

A few small correctness/robustness issues were found in the changed regions (whitespace-only config handling, a trace-message typo, and functional-test cleanup resilience).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConfigurableRetryLogicTest.cs:125

• The test’s best-effort cleanup only catches IOException. File.Delete can also throw UnauthorizedAccessException (e.g., read-only output directory or restricted permissions), which would make the test fail for cleanup rather than behavior under test.

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The PR description’s stated handler lifetime does not match the implementation (it remains installed through provider construction), and the description should be updated to avoid misleading future readers.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

[paulmedynski]

Restricting probing to the app directory is a breaking change regardless of the new app context switch. Do we need to update our API docs to explain this?

[priyankatiwari08]

@paulmedynski two updates following our discussion with the team:

Handler lifetime: We will not provide a compatibility path that restores the process-wide handler. #4623 tracks the proper long-term solution.

Docs: The probing directory was never documented as a contract, so no documentation change is needed. The release note already calls out the new base-directory behavior.

Ready for another look.

[Copilot]

🟡 Changes recommended

The resolving handler can remain subscribed even after falling back to the built-in factory (and a best-effort cleanup block should catch UnauthorizedAccessException), so a small refinement is needed to fully align with the scoping goal and keep tests robust.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConfigurableRetryLogicTest.cs:125

• The cleanup in this functional test is intended to be best-effort, but File.Delete can throw UnauthorizedAccessException (e.g., file marked read-only or permission issues). Catch it as well so cleanup failures don’t fail the test.

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The new functional test’s “best effort” cleanup should also swallow UnauthorizedAccessException to avoid failing due to cleanup/environment permissions rather than product behavior.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConfigurableRetryLogicTest.cs:125

• The cleanup in this new functional test is intended to be best-effort, but File.Delete can also throw UnauthorizedAccessException (e.g., permission/attribute issues). If that happens, the test would fail due to cleanup rather than validating the loader behavior. Consider swallowing UnauthorizedAccessException here as well (similar to the UnitTests helper).

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[paulmedynski]

Changes look good.

Here are the possible ways these changes could break existing apps:

Scenario	Likelihood	Legitimate	Mitigation
Private deps loaded after construction	Low	Yes	Add to deps.json or register handler in app
CurrentDirectory probing for custom types	Low	Questionable	Move assembly to AppContext.BaseDirectory
Accidental benefit from process-wide hook	Very Low	No	Register proper handler in app

Are we OK with this?