NCBC-4261: Use the staged txn.aux.uf user flags when committing - #149
Draft
davidkelly wants to merge 1 commit into
Draft
NCBC-4261: Use the staged txn.aux.uf user flags when committing#149davidkelly wants to merge 1 commit into
davidkelly wants to merge 1 commit into
Conversation
Motivation ========== Transactions record the user's content flags at txn.aux.uf when staging, but that field was never read back. On commit the flags came from the in-memory StagedMutation or were re-derived from the transcoder, so committing content we didn't stage (lost-transaction cleanup) or re-reading during ambiguity resolution could persist the wrong flags. Note that txn.aux.uf was found to be incorrectly storing byte-reversed (little-endian), so the common-flags byte would not have been recognized properly by other sdks. So sdk 3.8.0-3.9.4 transactions which were lost and cleaned by other SDKs would misread the format. Conversely, a transacton staged by the above versions of .net would be misread by this or future .net sdks. Modification ============ Parse txn.aux.uf on lookup and carry it as the staged content wrapper's flags, so both the ambiguity path and the Cleaner inherit the correct staged flags: - Flags: add symmetric ToUInt32/FromUInt32 helpers, in network byte order so the common-flags byte is in the top byte matching other SDKs, plus a shared JsonCommonFlags fallback. - DocumentRepository.LookupDocumentAsync: parse txn.aux.uf into the staged wrapper's flags; keep the live body flags for unstaged (pre-transaction) content. - LookupInContentAsWrapper: accept an optional flags override, used only for staged content. - AttemptContext: fall back to JsonCommonFlags (not new Flags(), which is DataFormat.Reserved) when no staged flags are present. - Cleaner: use the staged flags on the replace path. - FixedFlagsTranscoder: pin the persisted flags on the raw-insert paths (cleaner tombstone-revive and the legacy no-ReplaceBodyWithXattr insert), since InsertAsync has no flags option. Results ======= 154 transactions unit tests pass, plus the full transactions FIT suite (one unrelated, known flake). The encoding change is isolated to txn.aux.uf: the KV wire path (Flags.Write/Read) and transcoders are unchanged, and their tests still pass.
jeffrymorris
requested review from
Copilot and
jeffrymorris
and removed request for
Copilot
July 24, 2026 19:24
There was a problem hiding this comment.
Pull request overview
This PR fixes transactions commit/cleanup behavior to persist the staged user flags recorded in txn.aux.uf, rather than re-deriving flags from in-memory mutations or live body flags, ensuring correct flags during ambiguity resolution and lost-transaction cleanup.
Changes:
- Add
Flags.ToUInt32/FromUInt32plus a sharedFlags.JsonCommonFlagsfallback for when persisted staged flags are unavailable. - Parse
txn.aux.ufduringLookupDocumentAsyncand propagate the staged flags through the staged content wrapper. - Introduce
FixedFlagsTranscoderto pin persisted flags for raw-insert commit paths where there is no per-operation flags override; update cleaner/commit paths to use staged flags.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Couchbase.UnitTests/Transactions/StagedUserFlagsTests.cs | Adds end-to-end and helper coverage for parsing txn.aux.uf and ensuring staged vs unstaged flags behavior. |
| tests/Couchbase.UnitTests/Transactions/FixedFlagsTranscoderTests.cs | Adds unit tests for the new transcoder decorator behavior. |
| tests/Couchbase.UnitTests/Core/IO/Operations/FlagsTests.cs | Adds round-trip and byte-order tests for ToUInt32/FromUInt32 and JsonCommonFlags. |
| src/Couchbase/Core/IO/Operations/Flags.cs | Adds JsonCommonFlags and the ToUInt32/FromUInt32 helpers using big-endian/network byte order. |
| src/Couchbase/Client/Transactions/Internal/IContentAsWrapper.cs | Allows overriding wrapper flags so staged content can surface staged (xattr) flags instead of live body flags. |
| src/Couchbase/Client/Transactions/Internal/FixedFlagsTranscoder.cs | Adds a transcoder decorator to force persisted flags on insert paths. |
| src/Couchbase/Client/Transactions/DataAccess/DocumentRepository.cs | Writes staged flags via ToUInt32, parses txn.aux.uf on lookup, and pins insert-path flags via FixedFlagsTranscoder. |
| src/Couchbase/Client/Transactions/Cleanup/Cleaner.cs | Uses staged flags for replace paths and FixedFlagsTranscoder for insert paths during cleanup. |
| src/Couchbase/Client/Transactions/AttemptContext.cs | Uses JsonCommonFlags as the fallback instead of new Flags() (Reserved). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+29
to
+34
| private static TransactionXattrs XattrsWithAux(string? auxJson) => new() | ||
| { | ||
| AuxiliaryData = auxJson is null | ||
| ? null | ||
| : JsonDocument.Parse(auxJson).RootElement.Clone() | ||
| }; |
Comment on lines
+379
to
+383
| if (txnXattrs?.AuxiliaryData is { ValueKind: JsonValueKind.Object } aux | ||
| && aux.TryGetProperty("uf", out var ufElement) | ||
| && ufElement.TryGetUInt32(out var uf)) | ||
| { | ||
| result.TransactionXattrs = lookupInResult.ContentAs<TransactionXattrs>(txnIndex); | ||
| return Flags.FromUInt32(uf); |
Contributor
Author
There was a problem hiding this comment.
True! But also all other sdks wrote it in the big-endian. We must match other sdk, so we have to change it. the only risk is if we are cleaning a txn written by us from before this change. The much, much bigger risk is we don't change and therefore remain broken with respect to all other sdks, and cannot interoperate.
A better debate than making the fix would be when to make it. Do we put this in now, or do we wait for a minor and call it out. The reality is that this only really burns us if there are lost transactions (or concurrently transactions from an older sdk) that we need to commit, and this is really rare.
Contributor
There was a problem hiding this comment.
Perhaps create a Jira ticket do this in a minor release?
jeffrymorris
requested changes
Jul 24, 2026
jeffrymorris
left a comment
jeffrymorris
left a comment
Contributor
There was a problem hiding this comment.
The big endian/little endian change is a blocker for this PR.
davidkelly
marked this pull request as draft
August 6, 2026 20:43
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
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.
Motivation
Transactions record the user's content flags at
txn.aux.uf when staging, but that field was never read back. On commit the flags came from the in-memory
StagedMutation or were re-derived from the transcoder, so committing content we didn't stage (lost-transaction cleanup) or re-reading during ambiguity resolution could persist the wrong flags.
Modification
Parse txn.aux.uf on lookup and carry it as the staged content wrapper's flags, so both the ambiguity path and the Cleaner inherit the correct staged flags:
Results
154 transactions unit tests pass, plus the full
transactions FIT suite (one unrelated, known flake).