fix: recover SOAP requests after Dataverse BCDR redirects - #539
fix: recover SOAP requests after Dataverse BCDR redirects#539Suyash Kumar Patel (suyash1208) wants to merge 1 commit into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| bool redirectRecovered = !crossHostRedirectRecoveryAttempted && | ||
| _connectionSvc.TryRecoverFromCrossHostRedirectAsync(ex, requestServiceUri, requestTrackingId).ConfigureAwait(false).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
Possible deadlock: this sync-path recovery call isn't wrapped in Task.Run like the retry just below.
Unlike the async overload (which awaits this call at line 1913), here .GetAwaiter().GetResult() blocks the calling thread while TryRecoverFromCrossHostRedirectAsync invokes the consumer-supplied GetAccessTokenAsync. On a thread with a SynchronizationContext (WinForms/WPF/classic ASP.NET), if that callback resumes on the captured context it deadlocks against the blocked caller.
The retry a few lines below already avoids exactly this by offloading to the thread pool via Task.Run(...). Suggest matching that pattern here:
| bool redirectRecovered = !crossHostRedirectRecoveryAttempted && | |
| _connectionSvc.TryRecoverFromCrossHostRedirectAsync(ex, requestServiceUri, requestTrackingId).ConfigureAwait(false).GetAwaiter().GetResult(); | |
| bool redirectRecovered = !crossHostRedirectRecoveryAttempted && | |
| Task.Run(() => _connectionSvc.TryRecoverFromCrossHostRedirectAsync(ex, requestServiceUri, requestTrackingId)) | |
| .ConfigureAwait(false).GetAwaiter().GetResult(); |
| if (response.ResponseUri != null) | ||
| { | ||
| redirectAuthority = response.ResponseUri; |
There was a problem hiding this comment.
Untested branch: the WebException/HttpWebResponse 401 path (including this ResponseUri authority) has no coverage.
The new recovery tests only construct MessageSecurityException (ServiceClientTests.cs:284, 326), so this branch is never exercised. Since TryGetRedirectAuthority is private static, a test would drive a WebException wrapping a fake HttpWebResponse through TryRecoverFromCrossHostRedirectAsync.
Worth covering because recovery here hinges on ResponseUri being the final, post-redirect URI (the --d host). If it is ever the original request URI, TryCreateTrustedRedirectServiceUri(current, current) sees the same host and rejects it, so recovery silently no-ops.
Suggest adding a test for (a) ResponseUri = https://contoso--d.crm.dynamics.com/... returning the --d authority, and (b) a null-ResponseUri case that falls back to the WWW-Authenticate resource_id.
|
|
||
| DisposeWebProxy(previousProxy); | ||
|
|
There was a problem hiding this comment.
Bug: this can crash a concurrent request with ObjectDisposedException.
Recovery swaps in a new proxy and then disposes the old one here. But another thread on the same ServiceClient may still be using that old proxy inside Execute. The request path locks on _lockObject; this recovery uses a different lock (_redirectRecoveryLock) and disposes outside it, so the two do not coordinate and the in-flight request gets ObjectDisposedException.
I reproduced this with a small standalone program using the same swap-and-dispose pattern: the current approach throws ObjectDisposedException under concurrency, and not disposing eliminates it.
Simplest fix — do not dispose the old proxy; drop the reference and let GC reclaim it. The active proxy is still disposed normally at end-of-life (Dispose()), so this only defers cleanup of the one proxy a rare failover swaps out.
| DisposeWebProxy(previousProxy); |
Keep the catch-path DisposeWebProxy(replacementProxy) — it disposes only the new, not-yet-published proxy on failure, which no other thread can hold. If deterministic disposal is required, instead do the swap+dispose under _lockObject so no request is in flight (more involved, since recovery is async).
| return false; | ||
| } | ||
|
|
||
| bool recoveryAlreadyCompleted; |
There was a problem hiding this comment.
Untested branches: recovery-already-completed race, empty-token, and token-acquisition-throws paths.
Only the "happy path" (fresh trusted redirect + successful token) and the different-org rejection are covered by CrossHostRedirect_RecoveryReplacesProxyAndRequestsRedirectToken / CrossHostRedirect_RecoveryRejectsDifferentOrganization (ServiceClientTests.cs:~284, ~326). Three other branches inside this method have no test coverage:
recoveryAlreadyCompleted(lines 3785-3793) - a second concurrent request hitting the redirect after another request already swapped the endpoint. Returnstruewithout re-requesting a token; worth a test assertingGetAccessTokenAsyncis not invoked a second time.- Empty/null token from
GetAccessTokenAsync(lines 3821-3830) - falls back tofalse(no recovery), leaving the original request to fail as before. Worth asserting recovery is a no-op rather than throwing. GetAccessTokenAsyncthrowing (lines 3832-3840) - caught and logged, recovery returnsfalse. Worth asserting the exception doesn't propagate and the caller's original exception still surfaces.
Since TryRecoverFromCrossHostRedirectAsync is internal, these are all reachable via InternalsVisibleTo from the existing test project using a fake GetAccessTokenAsync delegate.
| bool isThrottled = false; | ||
| retry = ShouldRetry(req, ex, retryCount, out isThrottled) && !cancellationToken.IsCancellationRequested; | ||
| if (retry) | ||
| bool redirectRecovered = !crossHostRedirectRecoveryAttempted && |
There was a problem hiding this comment.
Untested: "recovery succeeded but the retried request still fails" scenario (both async and sync loops).
Both redirectRecovered branches here (and the mirrored one in the sync Execute overload around line 2047) set retry = true and loop back to re-issue the request against the new endpoint - but neither existing CrossHostRedirect_* test drives an actual retry through the outer while loop; they call TryRecoverFromCrossHostRedirectAsync directly and assert on its return value/side effects, not on the surrounding retry orchestration.
Worth a test where the retried request (post-recovery) still throws (e.g. the --d failover host is itself unreachable, or its own token validation still rejects). Since crossHostRedirectRecoveryAttempted is set to true before the retry, the interesting assertion is that the second failure does not attempt cross-host recovery again (falls through to ShouldRetry/normal retry-or-throw handling) rather than looping indefinitely or masking the second exception.
Summary
Dataverse BCDR can redirect the SOAP organization endpoint from the canonical organization host to the same organization's temporary
--dhost. WCF follows that cross-host redirect but clears theAuthorizationheader, so the temporary host returns401and ServiceClient currently treats the request as a terminal failure.This change uses that final authentication failure as a bounded recovery signal for
ExternalTokenManagementconnections.Changes
WWW-Authenticatechallenge.--dhost, with the same DNS suffix and port and no user information.Scope and safety
AuthenticationType.ExternalTokenManagement.--dare rejected before requesting a token.Validation
net8.0.net462ServiceClient build succeeded with 0 warnings and 0 errors.