Return 408 for cancelled bundle transaction cleanup#5553
Draft
mikaelweave wants to merge 3 commits into
Draft
Conversation
… bundles
Bug #188321: Batch/Transaction bundles fail with 'SqlTransaction has
completed; it is no longer usable' race condition.
Root cause is two interacting bugs:
Bug A - SqlServerFhirDataStore.MergeInternalAsync passes an already-
cancelled CancellationToken to MergeResourcesCommitTransactionAsync in
the error catch block. The cancelled token causes OpenAsync to throw
OperationCanceledException immediately, masking the original exception
and leaving server-side transaction state unclean.
Fix A: Pass CancellationToken.None so the abort call always completes.
Bug B - SqlTransactionScope.Dispose() (healthcare-shared-components)
disposes SqlConnection before SqlTransaction. This zombies the
transaction, causing SqlTransaction.Dispose()'s internal Rollback() to
throw InvalidOperationException('This SqlTransaction has completed').
During C# using-block unwinding this replaces the active exception
(FhirTransactionCancelledException), which BundleHandler then catches
and returns HTTP 500 instead of HTTP 408.
Fix B: Check cancellationToken.IsCancellationRequested in the
IsCompletedTransactionException catch block and throw
FhirTransactionCancelledException (408) instead of
FhirTransactionFailedException (500) when the client cancelled.
Fix C (defense-in-depth): Expand the transaction abort guard in both
sequential (BundleHandler) and parallel (BundleHandlerParallelOperations)
paths to abort on any 4xx/5xx status code, not only when
entry.Response.Outcome != null (outcome is null when client disconnects
mid-stream and the response body is empty).
Also adds a unit test covering the full failure chain:
2-entry transaction bundle + cancelled token + SqlTransaction zombie
exception -> asserts FhirTransactionCancelledException (408).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5553 +/- ##
==========================================
+ Coverage 77.02% 77.33% +0.30%
==========================================
Files 983 993 +10
Lines 36007 36421 +414
Branches 5469 5519 +50
==========================================
+ Hits 27736 28167 +431
+ Misses 6927 6892 -35
- Partials 1344 1362 +18 🚀 New features to boost your workflow:
|
Remove broader transaction abort-guard changes so PR #5553 only keeps the cancellation mapping, cleanup token handling, and targeted regression test for bug 188321. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mikaelweave
changed the title
Keep PR #5553 scoped to BundleHandler cancellation response mapping and its regression test. The SqlServerFhirDataStore cleanup token change is split to a separate branch for independent review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes Bug #188321 for the BundleHandler response-mapping failure mode: transaction bundle processing can return HTTP 500 when the client disconnects and cleanup hits
SqlTransaction has completed; it is no longer usable.This PR is intentionally limited to the BundleHandler cancellation response mapping and its targeted regression test. The SqlServerFhirDataStore cleanup-token change was split into a separate PR.
Changes
BundleHandler.cs: whenIsCompletedTransactionException()is caught and the request token is cancelled, throwFhirTransactionCancelledExceptionso the response is HTTP 408 instead of HTTP 500.BundleHandlerTests.cs: addGivenATransaction_WithCancellationAndSqlTransactionZombied_ReturnsCancelledExceptionto cover the cancelled-token + zombied-transaction path.Non-goals
SqlServerFhirDataStore.cs; that cleanup-token change is split to a separate PR.OperationOutcome.Testing
dotnet test src\Microsoft.Health.Fhir.Api.UnitTests\Microsoft.Health.Fhir.R4.Api.UnitTests.csproj --filter "FullyQualifiedName~GivenATransaction_WithCancellationAndSqlTransactionZombied_ReturnsCancelledException" --verbosity minimalRelated
ADO Bug #188321