Add async counterparts to the enclave provider hierarchy (Phase 3, partial)

[cheenamalhotra]

Description

Implements the enclave-provider portion of Phase 3 of specs/002-async-always-encrypted/spec.md, building on the SqlColumnEncryptionKeyStoreProvider async members that landed in Phase 1 (#3673).

The motivation is that the Always Encrypted enclave path currently blocks a thread on real network I/O even when the caller invoked an async API. Two call sites perform sync-over-async, and the attestation gate that serialises concurrent cold starts is an AutoResetEvent with a 15 second timeout, so contending callers park a thread pool thread for up to 15 seconds each. This PR lays the async groundwork to remove all three.

API changes, backwards compatibility

None. SqlColumnEncryptionEnclaveProvider is internal abstract and appears in neither netcore/ref/ nor netfx/ref/, so there is no public API surface and no binary compatibility constraint. The hierarchy is closed and entirely internal.

Functionality

1. Four async counterparts on the base type. GetEnclaveSessionAsync, GetAttestationParametersAsync, CreateEnclaveSessionAsync and InvalidateEnclaveSessionAsync. They are virtual, with defaults that defer to the existing sync overloads, so providers with no I/O need no boilerplate. This mirrors the Phase 1 Task.FromCanceled / Task.FromResult / Task.FromException + ADP.IsCatchableExceptionType pattern exactly.

C# forbids out parameters on async methods, so the two members that report multiple values return tuples instead (spec Design Decision 4):

Task<(SqlEnclaveSession SqlEnclaveSession, long Counter, byte[] CustomData, int CustomDataLength)> GetEnclaveSessionAsync(...)
Task<(SqlEnclaveSession SqlEnclaveSession, long Counter)> CreateEnclaveSessionAsync(...)

2. Both HTTP providers are genuinely async. A virtual default would leave a real network call blocking a thread, so the two providers that perform I/O explicitly override rather than inheriting the fallback. This removes both sync-over-async blocking calls in the hierarchy:

Site	Before	After
AzureAttestationBasedEnclaveProvider	GetConfigurationAsync(CancellationToken.None).Result	await GetConfigurationAsync(cancellationToken)
HostGuardianServiceEnclaveProvider	GetStreamAsync(url).ConfigureAwait(false).GetAwaiter().GetResult()	await GetStreamAsync(url, cancellationToken)

The HGS path also awaits JsonSerializer.DeserializeAsync and replaces Thread.Sleep in the retry backoff with Task.Delay. MakeRequestAsync is declared protected abstract rather than virtual so no derived provider can silently inherit a blocking implementation.

Left on the base defaults deliberately: NoneAttestationEnclaveProvider (no I/O), and the CPU bound GetAttestationParameters / InvalidateEnclaveSession operations (ECDH key generation, nonce generation, MemoryCache access).

3. FR-015, attestation gating. The spec calls for converting lock to SemaphoreSlim where an async operation precedes session storage. On inspection, the five lock statements named in the spec were not the ones that needed converting: their bodies only touch MemoryCache and flags, no await occurs inside them, and none can, because the awaited attestation happens before AddEnclaveSessionToCache is called. lock remains correct and cheaper there.

The construct that genuinely blocks is EnclaveProviderBase.sessionLockEvent, an AutoResetEvent with no awaitable wait. GetEnclaveSessionHelperAsync uses a SemaphoreSlim gate instead.

The sync and async gates are deliberately independent, per the requirement never to hold a gate across an awaited network call that a synchronous caller can also block on. The trade off is that a concurrent sync plus async cold start may perform two attestations. The existing design already tolerates duplicates (see the documented lock timeout cases) and the session cache is idempotent.

Behavioral differences worth reviewer attention

1. Cancellation releases the gate. The sync UpdateEnclaveSessionLockStatus releases only when a session was created. The async version also releases when the token is cancelled, otherwise a caller cancelled mid attestation would stall every other async caller for the full 15 second lock timeout. The sync path has no equivalent case because it has no cancellation.
2. SemaphoreSlim.Release() throws when the semaphore is full, unlike AutoResetEvent.Set() which is a harmless no-op. Releases are therefore guarded by an s_isAsyncSessionLockAcquired flag, preserving the sync design's "any caller may signal" semantics while staying idempotent.
3. Sync and async calls must not be mixed within one attestation sequence. GetEnclaveSessionAsync -> GetAttestationParametersAsync -> CreateEnclaveSessionAsync is one unit. Pairing an async Get with a sync Create leaks a gate because the two use different gates. This is documented in the snippet XML <remarks> and is the main thing to watch during Phase 5 integration.
4. ThreadRetryCache is keyed by ManagedThreadId, which is not stable across await boundaries. A caller resuming on a different thread may miss the same thread retry optimisation, costing an extra attestation. Never a correctness or deadlock issue. Worth replacing with a correlation token in Phase 5.

FR-010, sync paths unchanged

Every source change is an insertion. git diff --stat reports +690 / -1 across the five source files, and the single deletion is in the docs XML (a <remarks> line that was expanded). No existing sync statement was moved, reordered or edited. Each new async method was verified statement by statement against its sync twin.

Documentation

XML docs for the new members use <include> into doc/snippets/Microsoft.Data.SqlClient/SqlColumnEncryptionEnclaveProvider.xml, consistent with the existing sync members. The class level <remarks> gained the three call attestation sequence and the do-not-mix-sync-and-async contract. No localization impact.

Not in scope

EnclaveDelegate async dispatchers, SqlSecurityUtility.DecryptSymmetricKeyAsync and the SqlCommand.Encryption.cs call sites are Phase 3 remainder through Phase 5. Nothing calls these new members yet, so there is no end to end behaviour change from this PR. It is purely additive groundwork.

Issues

Implements part of Phase 3 of specs/002-async-always-encrypted/spec.md. Follows on from #3673 (Phase 1).

No GitHub issue is closed by this PR.

Testing

12 new unit tests in src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/AlwaysEncrypted/SqlColumnEncryptionEnclaveProviderAsyncShould.cs, using four in-file test doubles so no SQL Server or attestation service is required:

• default fallback dispatch, and fault propagation through Task.FromException
• cancellation on every async member, including pre-cancelled tokens
• gate release on cancellation. This test was validated by temporarily reverting the fix and confirming it fails with Attestation took 00:00:15.05, so it is a proven regression guard rather than an assumed one
• concurrent async cold start, cache reuse after attestation, and mixed sync and async callers
• the sync concurrency path, as an unchanged behaviour regression guard for FR-010
• HGS MakeRequestAsync cancellation, and mapping of request failure to SqlException

Results:

• 28 async Always Encrypted unit tests pass on net8.0 (12 new plus 16 existing Phase 1), 12 new pass on net9.0
• Library builds clean with 0 warnings for net8.0, net9.0 and net462

No integration or manual tests were added. The new members are not yet reachable from any public API (see "Not in scope"), so there is no end to end path to exercise. Integration coverage against a live enclave belongs with the Phase 5 call site work, where SqlCommand actually invokes these methods.

Additional verification against the spec's hard requirements:

• Every await in library code uses .ConfigureAwait(false) (Design Decision 5)
• CancellationToken is propagated through all new async methods
• All source is under src/Microsoft.Data.SqlClient/src/, none in netfx/src/ or netcore/src/

[Copilot]

Pull request overview

This PR adds internal async counterparts to the Always Encrypted enclave provider hierarchy, enabling non-blocking attestation/session creation (notably for the HTTP-based providers) while preserving existing sync behavior via default virtual fallbacks. It lays groundwork for later phases to wire async enclave attestation into the command execution flow without introducing public API surface changes.

Changes:

• Added GetEnclaveSessionAsync, GetAttestationParametersAsync, CreateEnclaveSessionAsync, and InvalidateEnclaveSessionAsync to SqlColumnEncryptionEnclaveProvider, with default implementations delegating to the sync members and supporting cancellation.
• Introduced an async attestation gate in EnclaveProviderBase (via SemaphoreSlim) and implemented truly-async network paths for Azure Attestation and HGS/VSM signing certificate retrieval.
• Added a new unit test suite validating default fallback behavior, cancellation, and concurrency characteristics across sync/async paths.

Reviewed changes

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

Show a summary per file

File	Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlColumnEncryptionEnclaveProvider.cs	Adds the new internal virtual async members with sync-fallback defaults, cancellation, and exception-to-faulted-task behavior.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/EnclaveProviderBase.cs	Implements async session gating + async helper APIs (tuple returns) to support non-blocking attestation flows.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AzureAttestationBasedEnclaveProvider.cs	Overrides async members to avoid sync-over-async blocking and propagate cancellation through network calls.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/VirtualSecureModeEnclaveProviderBase.cs	Adds async counterparts for VSM attestation logic, including async signing cert retrieval via an abstract MakeRequestAsync.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/VirtualSecureModeEnclaveProvider.cs	Implements truly async HTTP request + retry/backoff + async JSON deserialization for HGS signing cert retrieval.
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/AlwaysEncrypted/SqlColumnEncryptionEnclaveProviderAsyncShould.cs	Adds unit tests for fallback dispatch, cancellation semantics, gate behavior, and concurrency on sync/async paths.
doc/snippets/Microsoft.Data.SqlClient/SqlColumnEncryptionEnclaveProvider.xml	Documents the new async members and the “don’t mix sync/async within one attestation sequence” constraint.

---

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

[codecov]

Codecov Report

❌ Patch coverage is 44.94382% with 196 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.90%. Comparing base (ee529d4) to head (eed54c8).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
.../SqlClient/VirtualSecureModeEnclaveProviderBase.cs	0.00%	93 Missing ⚠️
.../SqlClient/AzureAttestationBasedEnclaveProvider.cs	10.52%	85 Missing ⚠️
...Data/SqlClient/VirtualSecureModeEnclaveProvider.cs	53.84%	12 Missing ⚠️
...rc/Microsoft/Data/SqlClient/EnclaveProviderBase.cs	92.94%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4541      +/-   ##
==========================================
- Coverage   64.78%   62.90%   -1.88%     
==========================================
  Files         288      284       -4     
  Lines       44418    67766   +23348     
==========================================
+ Hits        28774    42628   +13854     
- Misses      15644    25138    +9494     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	62.90% <44.94%> (?)

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 8 out of 8 changed files in this pull request and generated no new comments.

[priyankatiwari08]

A few things worth looking at before this merges, mostly around the new async gate and the shared ThreadRetryCache.

[priyankatiwari08]

currentThreadId is captured before the await on line 233, but CreateEnclaveSessionAsync removes the entry using Thread.CurrentThread.ManagedThreadId after resuming — usually a different thread. So these entries are typically never removed and linger for the full 10-minute TTL.

ThreadRetryCache is static and shared with the sync path, so a stale entry left here makes a later sync caller that happens to land on the same thread ID set sameThreadRetry = true and skip the sync gate. That's a behavior change to the sync path, which FR-010 says should stay unchanged.

Just a thought - can we give the async path its own MemoryCache instance so it can't affect sync callers?

[cheenamalhotra]

Fixed in 575a2a2. The async path no longer touches ThreadRetryCache at all — it isn't read in GetEnclaveSessionHelperAsync and isn't removed from in CreateEnclaveSessionAsync. Thread-id state can't be sound in async code (continuations resume on arbitrary threads), and rather than give the async path its own MemoryCache I removed the need for one: the gate is now taken and released entirely inside CreateEnclaveSessionAsync, so nothing has to be remembered between calls. The sync path is untouched and can no longer be affected by async callers.

[priyankatiwari08]

sqlEnclaveSession is still null when attestation throws, so the gate isn't released on the failure path and other async callers wait out the full 15s timeout.

The rationale given for releasing on cancellation ("a cancelled caller will never go on to create the session") applies equally to a failed caller. Should this also release when we're leaving via an exception?

[cheenamalhotra]

Good catch — fixed in 575a2a2. The gate is now acquired via using (await AcquireAsyncAttestationGateAsync(ct)) around the whole attestation body, so it is released on success, failure and cancellation alike, with no dependence on sqlEnclaveSession being non-null.

[priyankatiwari08]

VerifyTokenSignature does a Thread.Sleep(SigningKeyRetryInSec * 1000) on the SecurityTokenValidationException retry path (line 615). That's a multi-second, non-cancellable thread-pool block sitting inside the new async path.

Shall we move the delay into this do/while as an awaited Task.Delay(..., cancellationToken) so the async path is actually non-blocking.

[cheenamalhotra]

Fixed in 575a2a2. I split the signature check into VerifyTokenSignatureCore (no backoff) and kept the Thread.Sleep in the synchronous VerifyTokenSignature wrapper so sync behaviour is byte-for-byte unchanged. The async loop now calls the core method and awaits Task.Delay(SigningKeyRetryInSec * 1000, cancellationToken), so the backoff is non-blocking and cancellable.

[priyankatiwari08]

On net462 there's no token overload, so an in-flight request isn't cancellable — cancellation is only observed between retries at line 97. That's fine given the framework, but worth a short comment here so callers don't assume uniform cancellation semantics across TFMs.

[cheenamalhotra]

Added in 575a2a2 — the method comment now spells out that on .NET the token is passed to HttpClient and cancels an in-flight request, while on .NET Framework there is no token-accepting GetStreamAsync overload so cancellation is only observed between attempts. The abstract declaration also notes that implementations should document framework-specific limits.

[mdaigle]

✏️ an early return if not null would help with readability

[cheenamalhotra]

Done in 575a2a2 — GetEnclaveSessionHelperAsync now early-returns the cached session, and the rest of the method is flat (no gating left in it at all).

[mdaigle]

I'm really wary of all this thread id caching in an async context. Because we typically use ConfigureAwait(false) a continuation can run a retry on a different thread and may break a lot of assumptions.

What's the benefit of tracking threads here? Why not rewrite the flow so that the semaphore can be returned in a finally block?

[cheenamalhotra]

Agreed, and fixed in 575a2a2 — the thread-id tracking is gone from the async path entirely.

The sync design needed it because the gate is taken in GetEnclaveSession and only released by a later CreateEnclaveSession call, so it had to recognise "this thread already holds it" (the driver may never call CreateEnclaveSession for a non-enclave query). That cross-call ownership is exactly what breaks under ConfigureAwait(false).

The async path no longer spans calls: GetEnclaveSessionHelperAsync just probes the cache (and generates the nonce) with no lock, and the gate is taken and released inside CreateEnclaveSessionAsync:

using (await AcquireAsyncAttestationGateAsync(cancellationToken).ConfigureAwait(false))
{
    // another caller may have attested while we waited
    var session = GetEnclaveSessionFromCache(parameters, out long counter);
    if (session != null) { return (session, counter); }
    ... attest, cache ...
}

The lease is a readonly struct that releases on Dispose, so acquisition and release live in one try/finally scope — no ambient state, no AsyncLocal, and nothing to leak when the driver never gets as far as creating a session.

Collapsing concurrent attestations is preserved (arguably improved): the post-gate cache re-check means N concurrent cold starts now produce exactly one attestation-service call instead of "1..N". The unit test asserts AttestationCount == 1 for 8 concurrent callers, and there's a new test proving the gate is released when attestation throws.

[Copilot]

Pull request overview

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

[mdaigle]

This gate acquisition would be a good candidate to be enforced within the base class. Having a public, concrete method that acquires the lock, then calls out to a protected, abstract that contains the logic. Implementations can implement the protected helper method.

[mdaigle]

This test relies on the fact that the gate is static. We should reuse the same provider instance so that we're not bound to that fact. If we change to a non-static gate in the future this test will still provide the intended coverage.

[mdaigle]

What's our expected behavior here? I don't see any assertion that the sessions are all the same. Sync and async won't contend on the lock, but they will contend on the cache. We could end up with duplicated work if a sync and async call hit an empty cache at the same exact time.

If we want to guarantee that only one writes, we could update the sync path to look at our new semaphore slim and wait on it synchronously.

[mdaigle]

🤖 This review was produced with AI assistance and checked by me before posting.

The rework looks good: taking and releasing the gate entirely inside CreateEnclaveSessionAsync removes the cross-call ownership and the thread-affinity, and dropping ThreadRetryCache from the async path follows from that. Tests pass locally on net8.0.

Three comments below, all non-blocking.

[mdaigle]

🤖 Async cold start no longer collapses the pre-attestation work

Worth settling explicitly before Phase 4 wires EnclaveDelegate up, since nothing calls these members yet.

Moving the gate into CreateEnclaveSessionAsync means GetEnclaveSessionHelperAsync takes no lock, so concurrent cold-start callers no longer queue in call 1. Each one proceeds through GetAttestationParametersAsync (a 384-bit ECDH keygen) and sends serialized attestation parameters on the sp_describe_parameter_encryption RPC, and the server produces enclave attestation info for each. They only serialize at call 3, where all but one return at the post-gate cache re-check.

So N concurrent cold-start queries cost N key generations and N server-side attestation-info productions where sync costs 1. The attestation service round trip, the billed one the file header cares about, is still collapsed. The comment reads as though collapsing is preserved outright, which is the part I'd tighten.

Suggested change

	// Collapsing concurrent attestations is preserved by re-checking the session cache after the
	// gate is taken: the first caller performs the attestation and caches the session, and every
	// caller queued behind it observes that session and returns without contacting the
	// attestation service.
	// Calls to the attestation service are still collapsed: the gate is re-checked against the
	// session cache after it is taken, so the first caller performs the attestation and caches the
	// session, and every caller queued behind it observes that session and returns without
	// contacting the attestation service.
	//
	// Unlike the synchronous path, the work *before* the attestation is no longer collapsed.
	// Because this gate is not held across GetEnclaveSessionAsync, concurrent cold-start callers
	// each generate a Diffie-Hellman key in GetAttestationParametersAsync and each receive enclave
	// attestation info from the server; only the attestation service call itself is shared. This
	// is a deliberate trade: it buys an ownership model that is sound under ConfigureAwait(false)
	// at the cost of some redundant per-caller work during a cold start.

[mdaigle]

🤖 Lease disposal is not idempotent

Both call sites use using (await ...) and are correct, so this is about the extension point rather than current behavior.

Dispose has no guard, so a lease that gets copied and disposed twice calls Release() twice. That throws SemaphoreFullException, and before it does it admits a second caller into the gate. A struct is easy to copy accidentally, and the comment's "must be disposed exactly once" is the only thing enforcing it on a protected member that Phase 4 providers will use.

Making it a class costs one allocation per cold-start attestation. Moot if the gate stops being exposed to subclasses.

Suggested change

	// Represents ownership of the async attestation gate. Disposal releases the gate only when it
	// was actually taken, so a 'using' statement is safe even when the wait timed out. The lease
	// must be disposed exactly once, which 'using' guarantees.
	protected readonly struct AsyncAttestationGateLease : IDisposable
	{
	private readonly bool _acquired;
	internal AsyncAttestationGateLease(bool acquired)
	{
	_acquired = acquired;
	}
	public void Dispose()
	{
	if (_acquired)
	{
	s_asyncAttestationGate.Release();
	}
	}
	}
	// Represents ownership of the async attestation gate. Disposal releases the gate only when it
	// was actually taken, so a 'using' statement is safe even when the wait timed out. Disposal is
	// idempotent: releasing a SemaphoreSlim more often than it was taken throws
	// SemaphoreFullException, and admits a second caller into the gate before it does.
	protected sealed class AsyncAttestationGateLease : IDisposable
	{
	private int _acquired;
	internal AsyncAttestationGateLease(bool acquired)
	{
	_acquired = acquired ? 1 : 0;
	}
	public void Dispose()
	{
	if (Interlocked.Exchange(ref _acquired, 0) == 1)
	{
	s_asyncAttestationGate.Release();
	}
	}
	}

[mdaigle]

🤖 Test helper skips call 2, so the real sequence is never driven

Add GetAttestationParametersAsync to this helper before CreateEnclaveSessionAsync, and pass the returned client ECDH key into create. The driver sequence is GetEnclaveSession, GetAttestationParameters, then CreateEnclaveSession, and the attestation-parameter step creates the ECDH key.

Consider adding a parameter-generation counter to FakeAttestationEnclaveProvider. Then CreateEnclaveSessionAsync_ConcurrentColdStart_CompletesWithoutDeadlock can assert how many times that step ran, instead of relying only on AttestationCount.

[priyankatiwari08]

LGTM. Though I agree with Malcolm's cold-start collapsing point that needs to be addressed.