Address review follow-ups on the async enclave attestation gate

[cheenamalhotra]

Stacked on #4541. Addresses the remaining open review comments there.

Changes

Comment	Change
Copilot: unused using	Removed using Microsoft.Data.Common; from AzureAttestationBasedEnclaveProvider.cs and VirtualSecureModeEnclaveProviderBase.cs. They became unused when the redundant overrides were deleted.
@mdaigle: Task.Delay units	Both async retry loops now use TimeSpan.FromSeconds(...) instead of x * 1000.
@priyankatiwari08 (3): wrong comment	ThreadRetryCache stores thread IDs for the sync path, not the attestation url and nonce. Comment corrected.
@priyankatiwari08 (1): sync lock timeout never restored	Confirmed intentional, now documented. The async gate uses LockTimeoutMaxInMilliseconds directly and never reads or writes the sync path's adaptive lockTimeoutInMilliseconds. The decoupling is symmetric: async callers cannot degrade that value for sync callers, and sync contention cannot collapse the async timeout to zero.
@priyankatiwari08 (2): missing tests	Added the two suggested tests (below).

Tests

• CreateEnclaveSessionAsync_WhenGateWaitTimesOut_AttestsAnyway - a caller that cannot take the gate within the timeout attests on its own instead of failing or deadlocking, and does not release a gate it never held.
• GetEnclaveSessionAsync_AfterInvalidation_ReattestsAndReturnsNewSession - after invalidation the next async caller re-attests and gets a new session, which is then cached.

To support the first test, AsyncAttestationGateTimeoutInMilliseconds is a protected virtual hook on EnclaveProviderBase so a test provider can shorten the wait. Production providers use the default 15s.

The fake provider also tracks a high-water mark of concurrent attestations. Collapsing and fallthrough are now asserted directly (MaxConcurrentAttestations of 1 vs 2) instead of inferred from timing, and the fallthrough test parks the gate holder on a TaskCompletionSource the test controls rather than sleeping. Verified both tests fail if the fallthrough is removed.

Not included

@mdaigle's larger suggestion to converge the sync and async paths onto one primitive was marked "Not for this PR". It needs the sync path reshaped first (acquire/release within CreateEnclaveSession, post-gate cache re-check, dropping the cross-call handoff and the timeout mutation) and a change to EnclaveSessionCache.CreateSession to return an existing entry rather than overwrite. Worth a follow-up issue.

Checklist

• Tests added or updated
• Public API changes documented (no public API change)
• Verified against customer repro (n/a)
• Ensure no breaking changes introduced

[Copilot]

🟡 Changes recommended

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

Pull request overview

Addresses review follow-ups for the asynchronous enclave attestation gate.

Changes:

• Uses explicit TimeSpan retry delays and removes unused imports.
• Documents sync/async gate separation and corrects retry-cache documentation.
• Adds timeout, concurrency, and invalidation tests.

File summaries

File	Description
SqlColumnEncryptionEnclaveProviderAsyncShould.cs	Adds gate and invalidation tests.
VirtualSecureModeEnclaveProviderBase.cs	Removes an unused import.
VirtualSecureModeEnclaveProvider.cs	Clarifies retry-delay units.
EnclaveProviderBase.cs	Documents and exposes the testable gate timeout.
AzureAttestationBasedEnclaveProvider.cs	Removes an import and clarifies retry-delay units.

Review details

• Files reviewed: 5/5 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.

[Copilot]

🔵 Needs a closer look

Review details

Suppressed comments (2)

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

src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/AlwaysEncrypted/SqlColumnEncryptionEnclaveProviderAsyncShould.cs:788

• Add an XML summary for this new test helper. Test helper methods in this repository are required to document their behavior and side effects; this one updates both the active-attestation count and its high-water mark.

This issue also appears on line 809 of the same file.

            private void EnterAttestation()

src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/AlwaysEncrypted/SqlColumnEncryptionEnclaveProviderAsyncShould.cs:809

• Add the required XML summary for this new test helper as well, so its counter side effect is documented consistently with the other helpers in this file.

            private void ExitAttestation() => Interlocked.Decrement(ref _concurrentAttestations);

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[priyankatiwari08]

Concerns, all in test code. Nothing blocking.

• Failed assert strands the static gate. In CreateEnclaveSessionAsync_WhenGateWaitTimesOut_AttestsAnyway, if WaitForAttestationCountAsync(2) or the MaxConcurrentAttestations assert fails, hold.SetResult(true) never runs and holder parks forever holding s_asyncAttestationGate. Every later async attestation in the assembly then eats the 15s fallthrough, so ConcurrentColdStart fails too — one failure cascades. Wrap in try { ... } finally { hold.TrySetResult(true); await Task.WhenAll(holder, blocked); }.

• Static gate, instance timeout hook. s_asyncAttestationGate is static but AsyncAttestationGateTimeoutInMilliseconds is an instance member, so the effective timeout on a process-wide semaphore depends on whichever provider is waiting. Fine today because xUnit serialises within a class; add a [Collection] marker so a future parallelism change doesn't silently make these flaky.

• AttestationStarted is never reset or disposed and is only ever Set(), so it can't be used to wait for a second attestation start. Doesn't affect these two tests, but the helper looks reusable.

• WaitForAttestationCountAsync doc says "Spins until" but it awaits Task.Delay(10).

[priyankatiwari08]

This is the one thing I'd like changed before merge. Between here and hold.SetResult(true) there is no try/finally, and at this point the holder task is parked inside s_asyncAttestationGate, which is static.

If WaitForAttestationCountAsync(2) throws its 30s timeout assert, or this Assert.Equal fails, hold is never completed. The holder never returns from CreateEnclaveSessionCoreAsync, so CreateEnclaveSessionAsync's finally never runs and the semaphore is never released. Because the gate is static, that leak outlives this test: every later async attestation in the assembly then blocks for the full GateTimeoutInMilliseconds and falls through, so CreateEnclaveSessionAsync_ConcurrentColdStart_CompletesWithoutDeadlock's Assert.Equal(1, provider.MaxConcurrentAttestations) would start failing too. One real failure turns into a cascade of confusing unrelated failures, which is exactly the debugging experience this PR is otherwise trying to remove.

Wrapping the body from the Task.Run for holder down through the awaits in a try with finally { hold.TrySetResult(true); } fixes it. TrySetResult rather than SetResult also avoids an InvalidOperationException masking the original assertion failure if both paths run.

[priyankatiwari08]

No objection to the seam — EnclaveProviderBase is internal, so this is not a public API addition, and the default keeps production behaviour identical.

One asymmetry worth a word in the comment: the gate this timeout applies to (s_asyncAttestationGate) is static, but the timeout is an instance member. So the wait duration for a process-wide semaphore is determined per-provider-instance. That is a non-issue in production because every provider inherits the same LockTimeoutMaxInMilliseconds, but it is exactly the kind of thing a future provider could override to a small value and thereby weaken collapsing for every other provider's callers as well. A one-line note here saying "instance-scoped override of a process-wide gate; production providers must not narrow this" would prevent that.