Repository navigation
Address review follow-ups on the async enclave attestation gate - #4621
cheenamalhotra wants to merge 10 commits into
Conversation
…rtial)
Implements the enclave-provider portion of Phase 3 of the async Always
Encrypted spec (specs/002-async-always-encrypted/spec.md).
SqlColumnEncryptionEnclaveProvider gains four `virtual` async counterparts
whose default implementations defer to the existing sync overloads, mirroring
the pattern already established for SqlColumnEncryptionKeyStoreProvider in
Phase 1. Because C# forbids `out` parameters on async methods, the two members
that report multiple values return tuples instead (spec Design Decision 4).
The two providers that perform real network I/O explicitly override the
defaults rather than inheriting the blocking fallback, which removes both
sync-over-async blocking calls in this hierarchy:
* AzureAttestationBasedEnclaveProvider now awaits
ConfigurationManager.GetConfigurationAsync instead of blocking on .Result.
* HostGuardianServiceEnclaveProvider now awaits GetStreamAsync,
JsonSerializer.DeserializeAsync and the retry backoff instead of blocking
on .GetAwaiter().GetResult() and Thread.Sleep.
FR-015: the attestation gate (AutoResetEvent) has no awaitable wait, so the
async path gets its own SemaphoreSlim gate via GetEnclaveSessionHelperAsync.
The gates are deliberately independent so that a synchronous caller can never
block a thread for the duration of an awaited attestation round trip. The five
`lock` statements called out in the spec are left as-is: their bodies only
touch MemoryCache and flags, and the awaited attestation happens before session
storage rather than inside those regions.
Unlike the sync path, the async gate is also released when the caller cancels,
since a cancelled caller never goes on to create the session.
FR-010: no existing sync code path is modified. Every change is an insertion.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: cf57fdc0-1c77-40f1-a1d5-7c0bd98c1a89
The enclave provider async tests derive from EnclaveProviderBase and call ThreadRetryCache.Remove, where ThreadRetryCache is a MemoryCache. That type lives in Microsoft.Extensions.Caching.Memory, so the test assembly needs a direct compile reference to it. In Project mode the reference flows transitively through the SqlClient project reference, which is why local builds passed. In Package mode the SqlClient package reference sets ExcludeAssets="compile" so that the compiler binds against the implementation assembly rather than the ref assembly, and that exclusion also suppresses the transitive compile asset. The result was: SqlColumnEncryptionEnclaveProviderAsyncShould.cs(570,21): error CS0012: The type 'MemoryCache' is defined in an assembly that is not referenced. Declaring the PackageReference explicitly fixes Package mode and is accurate in both modes, since the test code really does compile against that type. The version resolves through central package management, which already selects 8.0.1 or 9.0.18 based on the target framework. Verified by reproducing the failure locally in Package mode without this change and confirming it builds and passes with it, on net8.0, net9.0 and net10.0. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf57fdc0-1c77-40f1-a1d5-7c0bd98c1a89
Addresses review feedback on the async enclave provider hierarchy: - The async attestation gate is now taken and released entirely inside CreateEnclaveSessionAsync via a disposable lease, so the semaphore is released in a finally on every exit path (success, failure, cancellation) and ownership never depends on which thread a continuation resumes on. - The async path no longer reads or writes the static, thread-id keyed ThreadRetryCache, so it can no longer leave stale entries that would make a later synchronous caller skip the sync gate. - Concurrent cold starts still collapse into a single attestation because CreateEnclaveSessionAsync re-checks the session cache after taking the gate. - GetEnclaveSessionHelperAsync now early-returns a cached session and performs no gating, so it never waits on another caller's in-flight attestation. - The signing key retry backoff is awaited (Task.Delay) on the async path instead of blocking a thread pool thread with Thread.Sleep; the synchronous path keeps its existing Thread.Sleep behaviour. - Documented the net462 cancellation granularity limit in MakeRequestAsync. - Tests: renamed the mismatched cancellation test, added coverage for gate release on attestation failure, and tightened the concurrent cold-start assertion to exactly one attestation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Follow-up to review feedback on the async enclave provider hierarchy. - EnclaveProviderBase now seals CreateEnclaveSessionAsync. It acquires the async gate, re-checks the session cache once held, and releases in a finally; providers supply only protocol-specific logic via the new protected abstract CreateEnclaveSessionCoreAsync. The gate can no longer be bypassed or leaked by a derived provider, which also removes the publicly-reachable lease type and its disposal contract. - EnclaveProviderBase also seals GetEnclaveSessionAsync, routing through GetEnclaveSessionHelperAsync via the new GeneratesNonceForAttestation hook. This fixes a latent bug: NoneAttestationEnclaveProvider had no async overrides, so it inherited the default that calls the *synchronous* GetEnclaveSession, taking the sync gate that only a later synchronous CreateEnclaveSession would release. An async caller never makes that call, so the sync gate was stranded for its full 15s timeout. Covered by a new regression test that fails (15s stall) without the seal. - NoneAttestationEnclaveProvider: extracted the session-setup parsing into a shared helper used by both the sync and async paths. - Removed the GetAttestationParametersAsync/InvalidateEnclaveSessionAsync overrides from the Azure and VSM providers; they were byte-for-byte identical to the inherited defaults. - Documented that the async path collapses the attestation service call but deliberately does not collapse the per-caller work before it, in the source comment, the doc snippet, and a new assertion on AttestationParametersCount. - Tests: drive the real three-call sequence (including GetAttestationParametersAsync, which produces the client ECDH key) in the Attest/AttestAsync helpers; reuse one provider instance in the gate-release-on-failure test so it does not depend on the gate being static; assert the mixed sync/async race converges on a single cached session. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- Drop the now-unused Microsoft.Data.Common usings from the Azure and VSM providers, left behind when their redundant overrides were removed. - Use the TimeSpan overload of Task.Delay in both async retry loops. - Correct the ThreadRetryCache comment: it records thread IDs for the sync path, not the attestation url and nonce. - Document that the async gate deliberately does not share the sync path's adaptive lock timeout, so neither path can degrade the other. - Make the async gate timeout overridable so tests can drive the timeout fallthrough without waiting out the production timeout. - Add tests for the gate timeout fallthrough and for async re-attestation after a session is invalidated, plus a concurrency high-water mark on the fake so collapsing and fallthrough are asserted directly rather than by timing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 148862f6-f66a-4a89-8436-ec4a008bbea3
There was a problem hiding this comment.
🟡 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
TimeSpanretry 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.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 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
left a comment
priyankatiwari08
left a comment
There was a problem hiding this comment.
Concerns, all in test code. Nothing blocking.
-
Failed assert strands the static gate. In
CreateEnclaveSessionAsync_WhenGateWaitTimesOut_AttestsAnyway, ifWaitForAttestationCountAsync(2)or theMaxConcurrentAttestationsassert fails,hold.SetResult(true)never runs andholderparks forever holdings_asyncAttestationGate. Every later async attestation in the assembly then eats the 15s fallthrough, soConcurrentColdStartfails too — one failure cascades. Wrap intry { ... } finally { hold.TrySetResult(true); await Task.WhenAll(holder, blocked); }. -
Static gate, instance timeout hook.
s_asyncAttestationGateis static butAsyncAttestationGateTimeoutInMillisecondsis 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. -
AttestationStartedis never reset or disposed and is only everSet(), so it can't be used to wait for a second attestation start. Doesn't affect these two tests, but the helper looks reusable. -
WaitForAttestationCountAsyncdoc says "Spins until" but it awaitsTask.Delay(10).
| // Reaching two attestations while the first is still parked is only possible if the second | ||
| // caller gave up on the gate. Without the fallthrough this wait times out. | ||
| await provider.WaitForAttestationCountAsync(2); | ||
| Assert.Equal(2, provider.MaxConcurrentAttestations); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Addressed in 8428f98. The test now releases the held attestation in finally and awaits the holder and blocked tasks.
| // How long a caller waits for the async attestation gate before giving up and attesting on its | ||
| // own. Overridable so that tests can exercise the timeout fallthrough without waiting out the | ||
| // full production timeout; production providers use the default. | ||
| protected virtual int AsyncAttestationGateTimeoutInMilliseconds => LockTimeoutMaxInMilliseconds; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Addressed in 8428f98. The comment now documents that this is an instance-scoped timeout override for a process-wide gate and that production providers must not narrow it.
mdaigle
added
the
Author attention needed
|
@cheenamalhotra 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 |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15ec8a75-4173-44d0-a283-f294f2840802
|
/ready |
github-actions
Bot
removed
the
Author attention needed
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The gate-balance regression check can pass after a leaked gate because its follow-up attestation also falls through on timeout.
Review effort: Balanced
Findings: 1
Open (1)
Resolved since last review (1)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15ec8a75-4173-44d0-a283-f294f2840802
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4621 +/- ##
==========================================
- Coverage 66.79% 64.67% -2.12%
==========================================
Files 292 286 -6
Lines 45326 68400 +23074
==========================================
+ Hits 30276 44241 +13965
- Misses 15050 24159 +9109
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
priyankatiwari08
left a comment
priyankatiwari08
left a comment
There was a problem hiding this comment.
Two things:
- PR description says
using Microsoft.Data.Common;was removed fromAzureAttestationBasedEnclaveProvider.cs, but it's still present on the head and still unused. The diff for that file only contains theTask.Delaychange. - Gate/test changes themselves look correct:
gateAcquiredis only released when the wait succeeded, so the timeout fallthrough can't over-release.
| // own. This instance-scoped seam lets tests exercise the timeout fallthrough without waiting | ||
| // out the production timeout; because the gate is process-wide, production providers must not | ||
| // override it with a shorter timeout. | ||
| protected virtual int AsyncAttestationGateTimeoutInMilliseconds => LockTimeoutMaxInMilliseconds; |
There was a problem hiding this comment.
This is a test-only seam whose safety invariant (never shorten it in production) is enforced only by a comment. Consider making it non-virtual and injecting the timeout via an internal ctor/field so a derived provider can't silently shrink the process-wide gate wait.
There was a problem hiding this comment.
Addressed in 69005d0. The virtual hook is gone: the gate reads a private readonly timeout initialized by an internal test constructor. Production providers keep the parameterless constructor and its 15-second wait. The timeout test now creates separate short-wait and infinite-wait instances, preserving its cancellation-bounded leaked-gate check and cleanup. The focused tests pass on net462, net8.0, net9.0, and net10.0.
Auto-replied by the GitHub Copilot app
Replace the virtual timeout hook with constructor-backed readonly state, preserve the production wait, and remove unused provider imports. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Addressed the remaining feedback in 69005d0: removed the unused imports from both attestation provider files and replaced the virtual timeout hook with constructor-injected readonly state. Production providers still wait 15 seconds. Updated the PR description to match the implementation. The focused tests pass on net462, net8.0, net9.0, and net10.0 (64 passed, zero failed). Reviewer threads are left open for reviewer confirmation. /ready |
Description
Follow-up to #4541. Addresses the remaining review comments on the async enclave attestation gate.
Changes
using Microsoft.Data.Common;fromAzureAttestationBasedEnclaveProvider.csandVirtualSecureModeEnclaveProviderBase.cs. Both imports became unused when the redundant overrides were deleted.Task.DelayunitsTimeSpan.FromSeconds(...)instead of multiplying by 1000.ThreadRetryCachestores thread IDs for the synchronous path, not the attestation URL and nonce.finallycleanup that releases the held attestation and awaits both tasks.There are no public API or production timeout changes.
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, a post-gate cache re-check, removal of the cross-call handoff and timeout mutation, and a change toEnclaveSessionCache.CreateSessionto return an existing entry rather than overwrite it. This remains separate follow-up work.Issues
Follow-up to #4541 and the review discussion on this PR, including the immutable timeout request.
Testing
CreateEnclaveSessionAsync_WhenGateWaitTimesOut_AttestsAnywayverifies timeout fallthrough, concurrent attestations, and a cancellation-bounded infinite follow-up wait that detects a leaked process-wide gate.GetEnclaveSessionAsync_AfterInvalidation_ReattestsAndReturnsNewSessionverifies re-attestation after invalidation and reuse of the replacement session.TaskCompletionSource.Executed before and after the latest review fixes, with identical results:
Passed for all declared driver frameworks (
net462,net8.0,net9.0), with zero warnings and zero errors.Passed: 16 tests per framework on
net462,net8.0,net9.0, andnet10.0; 64 passed in total, zero failures or skips.SQL Server-backed manual tests were not run locally. The latest changes are an internal test-seam refactor and unused-import cleanup; the focused tests exercise both sync and async gate behavior without requiring a live attestation service or enclave-enabled server.
Guidelines
Please review the contribution guidelines before submitting a pull request:
Checklist