Repository navigation
Fix silent write-loss in mt_upsert_<doc> + ProjectLatest double-store regression - #4328
Merged
jeremydmiller merged 4 commits intoMay 4, 2026
Conversation
…dateRevision Reproduces a bug in the generated mt_upsert_<doc> function for documents with UseNumericRevisions = true. Under READ COMMITTED contention, the function's INSERT ... ON CONFLICT ... DO UPDATE ... WHERE revision > mt_version clause can silently skip the update when a concurrent transaction has already bumped mt_version, but the function still returns the (now-bumped) final_version. Marten interprets the non-zero return as success, SaveChangesAsync does not throw, and AfterCommitAsync fires for a write that never landed. Test races N=50 sessions all loading the same doc at v=1 and calling UpdateRevision(doc, 2). Exactly one SaveChanges should succeed; every other must surface ConcurrencyException. Test currently fails on the unfixed code with multiple "successful" SaveChanges against a row whose mt_version only moved once. Pool is pre-warmed before the race because cold-pool connection setup serializes the parallel SaveChanges enough to miss the race window. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ents The generated upsert function for documents with UseNumericRevisions = true issued INSERT ... ON CONFLICT ... DO UPDATE ... WHERE revision > mt_version and then unconditionally read mt_version back into final_version and returned it. Under READ COMMITTED contention, when a concurrent transaction had already moved mt_version past the caller's revision, the conditional UPDATE silently matched zero rows but the function still returned the now-bumped final_version. Marten interprets any non-zero return as success, so SaveChangesAsync did not throw, AfterCommitAsync fired, and callers (e.g. Wolverine sagas) accepted the result for a write that never landed. Capture GET DIAGNOSTICS ROW_COUNT immediately after the ON CONFLICT and RETURN 0 when no row was actually updated, so the lost-race case is reported as a stale revision. Marten then surfaces a ConcurrencyException as documented. Regression test in src/DocumentDbTests/Concurrency/numeric_revisioning.cs (concurrent_update_revision_does_not_silently_lose_writes) now passes; all 22 tests in the suite stay green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the GET DIAGNOSTICS / re-SELECT approach with a single INSERT ... ON CONFLICT ... DO UPDATE ... RETURNING mt_version INTO final_version. When the conflict update is filtered by the revision > mt_version guard, RETURNING produces no row and final_version stays NULL, signalling the write-loss case. Same semantics as before, one fewer round of plpgsql control flow and one fewer round-trip to read mt_version. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ProjectLatest was queueing an upsert at the projected revision via session.Store(snapshot), and then the configured inline projection queued a second upsert at the same revision during SaveChangesAsync. Under the previous mt_upsert_<doc> behaviour the second write was a silent no-op so the test passed; once the upsert function correctly returns 0 when ON CONFLICT ... DO UPDATE ... WHERE revision > mt_version filters the row out, that second upsert surfaces as a ConcurrencyException and project_latest_tests.inline_projection_stores_ document_on_project_latest fails. The inline projection persists the projected document on its own during SaveChangesAsync, so the explicit Store in the inline plan was always redundant. FetchAsyncPlan still calls session.Store because async projections do not run inline. Co-Authored-By: Arjan Vermunt <a.vermunt@dutchwebsolutions.nl> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jeremydmiller
deleted the
fix/numeric-revisions-upsert-and-projectlatest-double-store
branch
May 4, 2026 16:25
This was referenced May 4, 2026
This was referenced May 5, 2026
This was referenced May 12, 2026
Merged
This was referenced May 14, 2026
This was referenced May 25, 2026
This was referenced Jun 8, 2026
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.
Supersedes #4325. Picks up @ArieGato's three commits unchanged and adds one
follow-up commit that fixes the regression those changes expose in
project_latest_tests.inline_projection_stores_document_on_project_latest.What #4325 fixes
The generated
mt_upsert_<doc>for documents withUseNumericRevisions = trueran
INSERT ... ON CONFLICT ... DO UPDATE ... WHERE revision > mt_versionand then unconditionally re-read
mt_version. Under READ COMMITTED contentionthe conditional UPDATE could match zero rows while the function still returned
the now-bumped
mt_version, so Marten saw a non-zero return as success,SaveChangesAsyncdid not throw, andAfterCommitAsyncfired for a write thatnever landed.
The fix folds the read into the same statement
(
DO UPDATE SET ... RETURNING mt_version INTO final_version) andRETURN COALESCE(final_version, 0)so that a filtered conflict update produces0, which Marten already treats as
ConcurrencyException.Regression test in
numeric_revisioning.cs(concurrent_update_revision_does_not_silently_lose_writes)fails on master, passes here.
What this PR adds on top
FetchInlinedPlan.ProjectLatestwas queueing an upsert at the projectedrevision via
session.Store(snapshot), and the configured inline projectionqueued a second upsert at the same revision during
SaveChangesAsync. Underthe old write-loss behaviour the second write silently became a no-op. With
the upsert function now reporting 0 when its
WHEREfilter skips the row,the second upsert correctly surfaces as
ConcurrencyException— and theexisting test
inline_projection_stores_document_on_project_lateststartedto fail in CI.
The inline projection persists the projected document itself during
SaveChangesAsync, so the explicitStorein the inline fetch plan wasalways redundant; it just happened to be harmless under the buggy upsert
function.
FetchAsyncPlankeeps itssession.Storebecause asyncprojections don't run inline.
Test plan
Run on net10.0 against PostgreSQL 16:
EventSourcingTestsfilterproject_latest_tests— 6/6 pass (was 5 pass / 1 fail on the previous PR head)DocumentDbTestsfilternumeric_revisioning|Concurrency— 66/66 (includes the new race regression test from Fix silent write-loss in mt_upsert_<doc> for numeric-revisioned documents #4325)EventSourcingTestsfilternumeric_revisioning|Concurrency— 21/21EventSourcingTestsfilterAggregation|Projection— 409/409EventSourcingTestsfilterFetchForWriting|FetchLatest|ProjectLatest|InlineProjection|inline_aggregation— 212/212EventSourcingTestsfilterBug_3310_inline_projections|SubscriptionScopedSession|MultiStream— 22/22PatchingTests— 114/115 (1 pre-existing skip)MultiTenancyTestsfilterrevisioning|Concurrency|Aggregation— 1/1CoreTestsfiltermigrate_from_guid_to_int_based_revisions— 1/1🤖 Generated with Claude Code