Cosmos: flow transactional batch failures through the execution strategy so they can be retried#38597
Cosmos: flow transactional batch failures through the execution strategy so they can be retried#38597AndriySvyryd with Copilot wants to merge 10 commits into
Conversation
…angesAsync Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes Cosmos transactional batch failures bypassing the execution strategy retry pipeline by throwing on batch failure (instead of returning a “failed result”) and moving the execution-strategy scope to wrap the full SaveChanges batch sequence, allowing transient failures to reach ShouldRetryOn and be retried.
Changes:
- Replace the transactional batch “result” return pattern with a
CosmosTransactionalBatchExceptionthrown fromProcessBatchResponse. - Change
ICosmosClientWrapper.ExecuteTransactionalBatchAsyncto returnTaskand execute once (no per-batch execution strategy wrapping in the client). - Wrap the full set of transactional batches in a single execution strategy invocation inside
CosmosDatabaseWrapper, with retry skipping of already-committed operations, and add a functional test verifying the exception reaches the execution strategy.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.Cosmos.FunctionalTests/CosmosTransactionalBatchTest.cs | Adds a functional test asserting transactional batch failures flow through the execution strategy and nothing is committed. |
| src/EFCore.Cosmos/Storage/Internal/ICosmosClientWrapper.cs | Updates the transactional batch API to return Task rather than a result object. |
| src/EFCore.Cosmos/Storage/Internal/CosmosTransactionalBatchException.cs | Introduces an internal exception type carrying errored entries and the underlying CosmosException. |
| src/EFCore.Cosmos/Storage/Internal/CosmosDatabaseWrapper.cs | Moves execution strategy retry scope to encompass the entire batch set and adds committed-operation skipping across retries. |
| src/EFCore.Cosmos/Storage/Internal/CosmosClientWrapper.cs | Executes transactional batches once and throws CosmosTransactionalBatchException on failure (instead of returning a failure result). |
…e transactional batch Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
…fected; add test Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
…currencyException for suppression Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
…pper.WrapUpdateException Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…nalBatchResponse Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
…specific exceptions in fallback Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
|
|
||
| // CosmosException has an internal ctor that accepts a Headers object (which contains RetryAfter). | ||
| // We use it to preserve the retry-after hint from a failed TransactionalBatchResponse. | ||
| // Signature (SDK 3.x): CosmosException(HttpStatusCode, string, string, Headers, ITrace, Error, Exception) |
There was a problem hiding this comment.
Done — removed the // Signature (SDK 3.x): ... comment line.
| private static readonly ConstructorInfo? CosmosExceptionInternalCtor = typeof(CosmosException) | ||
| .GetConstructors(BindingFlags.NonPublic | BindingFlags.Instance) | ||
| .FirstOrDefault(static c => | ||
| { | ||
| var parms = c.GetParameters(); | ||
| return parms.Length == 7 | ||
| && parms[0].ParameterType == typeof(HttpStatusCode) // statusCode | ||
| && parms[1].ParameterType == typeof(string) // message | ||
| && parms[2].ParameterType == typeof(string) // stackTrace | ||
| && parms[3].ParameterType.FullName == "Microsoft.Azure.Cosmos.Headers" // headers | ||
| && parms[4].ParameterType.Name == "ITrace" // trace | ||
| && parms[5].ParameterType.Name == "Error" // error | ||
| && parms[6].ParameterType == typeof(Exception); // innerException | ||
| }); |
There was a problem hiding this comment.
@copilot Make sure this pattern works in NativeAOT mode
There was a problem hiding this comment.
Fixed. The field initializer now checks RuntimeFeature.IsDynamicCodeSupported before running reflection, so the lookup is skipped entirely in NativeAOT builds (where the private constructor would be trimmed anyway). The reflection itself is extracted into a private helper method FindCosmosExceptionInternalCtor annotated with [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2075", ...)] to suppress any trimmer warning. In both NativeAOT and trimmed builds, CosmosExceptionInternalCtor will be null and CreateCosmosException will use the public constructor as the fallback.
… RuntimeFeature.IsDynamicCodeSupported for NativeAOT Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Cosmos transactional batch failures returned a result object from
ProcessBatchResponserather than throwing, so the failure surfaced as aDbUpdateExceptionoutside any execution-strategy scope and could never be retried by anExecutionStrategy(ShouldRetryOnwas never called).Changes
CosmosClientWrapper.ProcessBatchResponsenow computes the errored entries (those whose per-item status code matches the batch-level error code), wraps theCosmosExceptioninto aDbUpdateConcurrencyException(PreconditionFailed) orDbUpdateException(Conflict / other), and throws it directly.CosmosTransactionalBatchResultandCosmosTransactionalBatchExceptionare removed.ExecuteTransactionalBatchAsyncno longer wraps itself in the execution strategy; it runs once and returnsTask(interface updated).SaveChangesAsync—CosmosDatabaseWrappertakes an injectedIExecutionStrategyand wraps the whole set of batches in a singleExecuteAsync. ABatchExecutionState.CommittedOperationscounter lets a retry skip already-committed operations while preserving the original ordering of single-entry saves interleaved with transactional batches. The execution strategy unwrapsDbUpdateExceptionviaExecutionStrategy.CallOnWrappedExceptionto reach the innerCosmosExceptionbefore callingShouldRetryOn, so transient failures are correctly identified and retried.DbUpdateConcurrencyExceptionfrom a batch is suppressed viaOptimisticConcurrencyExceptionAsync,RowsAffectedis not incremented (matchingSaveAsyncsingle-entry behavior), whileCommittedOperationsstill advances so a subsequent retry skips the already-processed operation.Because a transactional batch is atomic, a failed batch commits nothing and is retried in place; deterministic transaction serialization across attempts makes index-based skipping of committed operations valid.
Result
The repro from the issue now behaves as expected — a transient batch failure reaches the strategy and is retried:
Two functional tests verify the behavior:
SaveChanges_transactional_batch_failure_flows_through_execution_strategy— a conflicting batch reachesShouldRetryOnand yieldsRetryLimitExceededException, with nothing committed.SaveChanges_suppressed_concurrency_exception_in_transactional_batch_reports_zero_rows_affected— a stale-ETag batch update suppressed by an interceptor returns 0 rows affected and leaves the store unchanged.