Fix | Load and resolve retry logic providers into the current AssemblyLoadContext

[edwardneal]

Description

This PR addresses two interrelated bugs, both of which are triggered when SqlClient is loaded into an alternate AssemblyLoadContext. Both of these relate to SqlConfigurableRetryLogicLoader.

The first bug is that when using the managed SNI, SqlClient blocks an unloadable AssemblyLoadContext from being unloaded. This is completely due to the event handlers: it adds a handler to the default ALC's Resolving event, and this strong reference from the default ALC to a type loaded into a secondary ALC prevents that secondary ALC from being unloaded. This is fixed by making sure that the currently-executing ALC unhooks the Resolving handler when it unloads.

The second, related, bug is that we load a configuration's RetryLogicType into the default AssemblyLoadContext. This means that the type will never be unloaded with the secondary AssemblyLoadContext - and because it needs to reference a type in that secondary ALC, the secondary ALC won't unload either. Fixing this means that we need to load RetryLogicType into the current AssemblyLoadContext, not the default one.

To avoid a regression in fixing the second bug, we must also hook to the current AssemblyLoadContext's Resolving handler. This is called to resolve transitive dependencies of the assembly which has to be loaded to locate a configuration's RetryLogicType. The nuance here is that only the current AssemblyLoadContext's Resolving handler is called - not the default handler. If those two differ, we'll find ourselves in a position where a transitive dependency can fail to load (or worse, the wrong dependency may load - different ALCs may have different base paths.)

With these bugs resolved, #1687 is fixed when using the managed SNI.

I don't think that SqlClient can implement a fix to make the native SNI compatible with unloadable AssemblyLoadContexts - there's a rooted SNILoadHandle which calls SniInitialize and SniTerminate. Unless these two methods are signalling and waiting a semaphore in native code, they seem to perform process-wide initialization and teardown. I think having an AssemblyLoadContext remain uncollectible is safer behaviour than having multiple ALCs referring to the process-wide SNI; one might have an in-flight async request, while the other might be unloading and performing SNI teardown.

Issues

Contributes to #1687.

Testing

Existing automated tests (SqlConfigurationManagerReliabilityTest) continue to pass. I've also added a new test, UnloadableAssemblyLoadContextTest. This test covers the scenario where users are loading and unloading AssemblyLoadContexts.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[paulmedynski]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.

[Copilot]

Copilot review overview

🟡 Changes recommended

The new regression test uses default-context SqlClient and never exercises the changed retry-provider resolution paths.

Review effort: Balanced
Findings: 1 · 2

Open (3)

• Test SqlClient resolution in the collectible load context · New
• Add required XML summaries to test classes and methods · New
• Correct typo in test documentation · New

What changed in this PR

Updates configurable retry assembly resolution to use SqlClient’s current AssemblyLoadContext and adds unloadability coverage.

Changes:

• Resolve retry providers and dependencies in the current load context.
• Add a collectible-context integration test and helper library.
• Update existing retry-loader test documentation.

File	Description
UnloadableLibrary.csproj	Defines the collectible test library.
EntryPoint.cs	Exercises synchronous and asynchronous SqlClient operations.
SqlConfigurableRetryLogicLoaderTest.cs	Updates load-context terminology.
UnloadableAssemblyLoadContextTest.cs	Adds collectible-context regression coverage.
Microsoft.Data.SqlClient.ManualTests.csproj	References the new test library.
SqlConfigurableRetryLogicTest.cs	Updates resolving-handler documentation.
SqlConfigurableRetryLogicLoader.cs	Resolves assemblies in the current load context.
Microsoft.Data.SqlClient.slnx	Adds the test library to the solution.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[edwardneal]

Excellent point - my original test case actually had that, but I stripped it out by mistake. Fixed in a495eab, with a specific assertion in 79f51b9 to prove that it doesn't regress.

[mdaigle]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.

[Copilot]

🟡 Changes recommended

The regression test bypasses the changed retry-type loading path and leaks a process-wide AppContext switch.

6 open findings

• Restore process-wide AppContext switch after test · New
• Test SqlClient resolution in the collectible load context
• Document shared test helper methods and contracts · New
• Document collectible-context helper constructor and Load override · New
• Fix typo in test-helper documentation · New
• Add XML summaries to retry provider test hooks · New

2 resolved since last review

• Correct typo in test documentation
• Add required XML summaries to test classes and methods

Previously missed (2)

In code that hasn't changed since last review

Update LoadType summary for current load context

src/​Microsoft.Data.SqlClient/​src/​Microsoft/​Data/​SqlClient/​Reliability/​SqlConfigurableRetryLogicLoader.cs:417

The LoadType XML summary immediately below still says assemblies are resolved in the default load context, which is no longer true after this change. Update it to describe resolution in SqlClient's current load context.

Remove UTF-8 BOM from project file

src/​Microsoft.Data.SqlClient/​tests/​UnloadableLibrary/​UnloadableLibrary.csproj:1

Remove the UTF-8 BOM from this new project file. Repository policy requires UTF-8 without BOM (policy/coding-style.md:16-17), and the other touched source files explicitly remove their BOMs.

🧠 Review effort: Balanced

[edwardneal]

Simplified in bd0ba5b and fixed in ad8edb9 and e2c04e3.

[edwardneal]

Fixed in ad8edb9.

[edwardneal]

Fixed in ad8edb9.

[mdaigle]

It looks like this will leak out of the test into the switch dictionary in the default ALC. Can double check that we're not polluting that?

[edwardneal]

It did, yes - thanks for spotting that @mdaigle. I'd guarded it with a LocalAppContextSwitchesHelper, but that doesn't help when I'm setting the underlying AppContext switch directly! The test code now uses LocalAppContextSwitchesHelper within the unloadable ALC, so we never tamper with these switches in the first place.

[github-actions]

@edwardneal This pull request has been marked as Author attention needed.

When you have addressed the reviewer feedback and are ready for another review, please post a comment with /ready to remove the label and re-engage reviewers.

[edwardneal]

/ready

Apologies for the delay here.