Skip to content

Commit 5bdae9f

Browse files
Metadata Retry: Addresses review feedback
- Preserve original stack trace when ShouldRetryResult.ExceptionToThrow references the same instance as the captured exception (mirrors ShouldRetryResult.ThrowIfDoneTrying in BackoffRetryUtility). - Add test for MaxAttemptsHardCap defensive guard. - Add test for ShouldRetryAsync-throws path surfacing the original operation exception. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 2b4a217 commit 5bdae9f

2 files changed

Lines changed: 81 additions & 0 deletions

File tree

Microsoft.Azure.Cosmos/src/MetadataRetryHelper.cs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,8 +139,16 @@ internal static async Task<T> ExecuteAsync<T>(
139139
{
140140
// Honor ShouldRetryResult.ExceptionToThrow if the policy has specified a
141141
// wrapper/translated exception (matches BackoffRetryUtility.ThrowIfDoneTrying).
142+
// When the policy hands back the same reference as the original exception,
143+
// rethrow via the captured ExceptionDispatchInfo so the original stack trace
144+
// is preserved (mirrors ShouldRetryResult.ThrowIfDoneTrying).
142145
if (shouldRetry?.ExceptionToThrow != null)
143146
{
147+
if (shouldRetry.ExceptionToThrow == capturedException.SourceException)
148+
{
149+
capturedException.Throw();
150+
}
151+
144152
throw shouldRetry.ExceptionToThrow;
145153
}
146154

Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Tests/MetadataRetryHelperTests.cs

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -461,6 +461,79 @@ await Assert.ThrowsExceptionAsync<ArgumentOutOfRangeException>(
461461
TimeSpan.FromSeconds(-1),
462462
CancellationToken.None));
463463
}
464+
465+
/// <summary>
466+
/// Regression guard for the defensive <c>MaxAttemptsHardCap</c>. A misconfigured retry
467+
/// policy that always returns <see cref="ShouldRetryResult.RetryAfter"/> would otherwise
468+
/// spin indefinitely. The helper must terminate with an <see cref="InvalidOperationException"/>
469+
/// after the hard cap (20) is exceeded.
470+
/// </summary>
471+
[TestMethod]
472+
[Owner("ntripician")]
473+
public async Task ExecuteAsync_ExceedsMaxAttemptsHardCap_ThrowsInvalidOperationException()
474+
{
475+
Mock<IDocumentClientRetryPolicy> policy = new Mock<IDocumentClientRetryPolicy>();
476+
policy
477+
.Setup(p => p.ShouldRetryAsync(It.IsAny<Exception>(), It.IsAny<CancellationToken>()))
478+
.ReturnsAsync(ShouldRetryResult.RetryAfter(TimeSpan.Zero));
479+
480+
int attempts = 0;
481+
482+
InvalidOperationException thrown = await Assert.ThrowsExceptionAsync<InvalidOperationException>(
483+
() => MetadataRetryHelper.ExecuteAsync<int>(
484+
(_) =>
485+
{
486+
attempts++;
487+
throw new DocumentClientException(
488+
"always fail",
489+
HttpStatusCode.ServiceUnavailable,
490+
SubStatusCodes.Unknown);
491+
},
492+
policy.Object,
493+
CancellationToken.None));
494+
495+
Assert.AreEqual(20, attempts, "Helper must stop at the defensive attempt cap of 20.");
496+
StringAssert.Contains(thrown.Message, "defensive attempt cap");
497+
}
498+
499+
/// <summary>
500+
/// Regression guard: when the retry policy itself throws while evaluating
501+
/// <c>ShouldRetryAsync</c>, the helper must surface the ORIGINAL operation
502+
/// exception (with its stack trace preserved) rather than the policy's
503+
/// internal failure. The policy error is logged but should not be visible
504+
/// to callers — diagnosing a metadata read failure should never be
505+
/// distorted by a buggy retry policy.
506+
/// </summary>
507+
[TestMethod]
508+
[Owner("ntripician")]
509+
public async Task ExecuteAsync_PolicyThrowsDuringShouldRetry_SurfacesOriginalException()
510+
{
511+
Mock<IDocumentClientRetryPolicy> policy = new Mock<IDocumentClientRetryPolicy>();
512+
policy
513+
.Setup(p => p.ShouldRetryAsync(It.IsAny<Exception>(), It.IsAny<CancellationToken>()))
514+
.ThrowsAsync(new InvalidOperationException("policy internal error"));
515+
516+
int attempts = 0;
517+
518+
DocumentClientException thrown = await Assert.ThrowsExceptionAsync<DocumentClientException>(
519+
() => MetadataRetryHelper.ExecuteAsync<int>(
520+
(_) =>
521+
{
522+
attempts++;
523+
throw new DocumentClientException(
524+
"original 503",
525+
HttpStatusCode.ServiceUnavailable,
526+
SubStatusCodes.Unknown);
527+
},
528+
policy.Object,
529+
CancellationToken.None));
530+
531+
Assert.AreEqual(1, attempts);
532+
StringAssert.Contains(
533+
thrown.Message,
534+
"original 503",
535+
"Helper must surface the ORIGINAL operation exception, not the policy's internal failure.");
536+
}
464537
}
465538
}
466539

0 commit comments

Comments
 (0)