Fix inbox marker conflicts committing duplicate work inside ambient transactions#291
Merged
Conversation
…ad of committing duplicate work
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Owner
Author
|
Successfully created backport PR for |
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
1 participant
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
When the relational inbox store runs inside a transaction it doesn't own (e.g. wrapped by another transactional consume filter), a concurrent duplicate delivery that lost the race on the idempotency marker insert was silently swallowed and reported as "skipped" — but since the store can't roll back a transaction it doesn't own, the transaction's actual owner would go on to commit that delivery's business writes anyway. Net effect: a duplicate delivery's side effects got committed while being reported as deduplicated.
The fix rethrows the conflict instead of catching it. The transaction owner then rolls back (discarding the losing delivery's writes), the transport redelivers, and the existing pre-check deduplicates it on the next attempt. The already-correct own-transaction path (used when the store opens its own transaction) is unchanged.
Also fixes a smaller issue: a custom idempotency key selector returning
""was treated differently from returningnull, bypassing the documented fallback to the message id and landing in the reject/missing-key path instead.Includes docs on recommended consume-filter registration order and tests covering both the fixed and already-correct code paths.
Test plan
dotnet build— full solution, 0 errorsVulthil.Messaging.Inbox.Tests(net10.0) — 9/9 passed, including a new test proving the empty-string key selector now falls back to the message id (fails on unfixed main)Vulthil.Messaging.Inbox.Relational.Tests(net10.0) — 8/8 passed, including a new test proving the ambient-transaction conflict now propagates instead of letting duplicate work commit (fails on unfixed main) and a concurrency test pinning the already-correct own-transaction pathBackport to v1.0: recommended — internal-only correctness fix for a defect present since 1.0.0.