[Internal] DTS: Adds logic to merge session data from DTC response into SessionContainer#5705
Conversation
After a CommitDistributedTransaction, per-operation session tokens in the
JSON response body were never pushed into the SDK's SessionContainer.
Subsequent reads with Session consistency on the affected collections
could therefore fail with ReadSessionNotAvailable (404/1002).
Changes:
- DistributedTransactionCommitter: add MergeSessionTokens() private
helper, called after FromResponseMessageAsync returns. Iterates
(result[i], operation[i]) pairs, builds a minimal header collection
carrying the per-operation session token, and calls
sessionContainer.SetSessionToken(collectionRid, collectionFullname, headers)
mirroring the pattern used by GatewayStoreModel.CaptureSessionTokenAndHandleSplitAsync
for TransactionalBatch.
- DistributedTransactionOperationResult (pre-existing bugs uncovered by
tests):
* Change [JsonConstructor] constructor from protected to public
System.Text.Json reflection mode requires a public constructor.
* Change ResourceBodyBase64 from internal to public [JsonInclude]
members must be public in reflection mode.
- New unit tests in DistributedTransactionCommitterTests:
* MergesSessionTokensIntoSessionContainer
* SkipsMerge_WhenSessionTokenIsNull
* PassesCorrectCollectionToSetSessionToken (multi-collection, Mock<ISessionContainer>)
* MergesSessionTokens_OnFailureResponse
Merge session data from DTC response into SessionContainer
After a CommitDistributedTransaction, per-operation session tokens in the
JSON response body were never pushed into the SDK's SessionContainer.
Subsequent reads with Session consistency on the affected collections
could therefore fail with ReadSessionNotAvailable (404/1002).
Changes:
- DistributedTransactionCommitter: add MergeSessionTokens() private
helper, called after FromResponseMessageAsync returns. Iterates
(result[i], operation[i]) pairs, builds a minimal header collection
carrying the per-operation session token, and calls
sessionContainer.SetSessionToken(collectionRid, collectionFullname, headers)
mirroring the pattern used by GatewayStoreModel.CaptureSessionTokenAndHandleSplitAsync
for TransactionalBatch.
- DistributedTransactionOperationResult (pre-existing bugs uncovered by
tests):
* Change [JsonConstructor] constructor from protected to public
System.Text.Json reflection mode requires a public constructor.
* Change ResourceBodyBase64 from internal to public [JsonInclude]
members must be public in reflection mode.
- New unit tests in DistributedTransactionCommitterTests:
* MergesSessionTokensIntoSessionContainer
* SkipsMerge_WhenSessionTokenIsNull
* PassesCorrectCollectionToSetSessionToken (multi-collection, Mock<ISessionContainer>)
* MergesSessionTokens_OnFailureResponse
Co-Authored-By: Copilot <223556219+Copilot@users.noreply.github.com>
| headers.Clear(); | ||
| headers[HttpConstants.HttpHeaders.SessionToken] = result.SessionToken; | ||
|
|
||
| sessionContainer.SetSessionToken( |
There was a problem hiding this comment.
Optimization: Each operation inside does a locking, wondering if a new API for batch updates for a collection will help.
/cc: @FabianMeiswinkel
There was a problem hiding this comment.
Can you please create a follow-up GitHub issue to track it.
kirankumarkolli
left a comment
There was a problem hiding this comment.
Waiting for Exclude sessionCapture for 404/1002
- Exclude 404/ReadSessionNotAvailable from session token capture in MergeSessionTokens, mirroring GatewayStoreModel.CaptureSessionTokenAndHandleSplitAsync. A 404/1002 response means the session was not found, so merging such a token would propagate stale data. - Add [JsonInclude] and public getter to SubStatusCode so STJ reflection mode can deserialize per-operation sub-status codes from the DTC JSON response body (required for the 404/1002 check above). - Add a note about the per-call locking in SetSessionToken as a future optimization opportunity. - Clarify the ResourceBodyBase64 XML doc: explains why it must be public (STJ reflection mode requires [JsonInclude] members to be public) and why the base64 decoding logic is needed (DTC server encodes resource bodies as base64 in the JSON response).
| } | ||
| catch (Exception ex) | ||
| { | ||
| DefaultTrace.TraceWarning($"DTC session token merge failed (non-fatal): {ex.Message}"); |
There was a problem hiding this comment.
What are the possible errors and is ignoring fine?
Can you please validate similar flows
There was a problem hiding this comment.
DTS commit has already succeeded on the server, so a failure here should not propagate back to the caller as a transaction error. That's why we are ignoring it here. If we want to propagate it, then we need to make it clear that the DTS commit has succeeded and there's an error during session token merge.
Possible exceptions:
IndexOutOfRangeException/FormatExceptionfrom SessionContainer.SetSessionToken if the server returns a malformed session token.FormatExceptionfromResourceId.Parseif the collection RID is malformed.
GatewayStoreModel.CaptureSessionTokenAndHandleSplitAsync does NOT wrap SetSessionToken in a try/catch, so an exception there would propagate even though the server write already succeeded.
There was a problem hiding this comment.
That's a good call out and for now we can continue with current model and let the call stack covey the failure. Lets also please track this work item.
There was a problem hiding this comment.
Very little value with tracing as CX already sees the final failure. try-catch is not really needed.
| requestMessage.UseGatewayMode = true; | ||
| } | ||
|
|
||
| internal static void MergeSessionTokens( |
There was a problem hiding this comment.
Optimization: Follow-up limit session merge for only SESSION, MM
…github.com/Azure/azure-cosmos-dotnet-v3 into users/Meghana-Palaparthi/dtc-session-merge
Pull Request Template
Description
After a CommitDistributedTransaction, per-operation session tokens in the JSON response body were never pushed into the SDK's SessionContainer. Subsequent reads with Session consistency on the affected collections could therefore fail with ReadSessionNotAvailable (404/1002).
Changes:
DistributedTransactionCommitter: add MergeSessionTokens() private helper, called after FromResponseMessageAsync returns. Iterates (result[i], operation[i]) pairs, builds a minimal header collection carrying the per-operation session token, and callssessionContainer.SetSessionToken(collectionRid, collectionFullname, headers)mirroring the pattern used byGatewayStoreModel.CaptureSessionTokenAndHandleSplitAsync()for TransactionalBatch.DistributedTransactionCommitterTestsSessionContainerType of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #IssueNumber