Skip to content

Fix silent write-loss in mt_upsert_<doc> + ProjectLatest double-store regression - #4328

Merged
jeremydmiller merged 4 commits into
masterfrom
fix/numeric-revisions-upsert-and-projectlatest-double-store
May 4, 2026
Merged

jeremydmiller merged 4 commits into
masterfrom
fix/numeric-revisions-upsert-and-projectlatest-double-store

Conversation

@jeremydmiller

Copy link
Copy Markdown
Member

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 with UseNumericRevisions = true
ran INSERT ... ON CONFLICT ... DO UPDATE ... WHERE revision > mt_version
and then unconditionally re-read mt_version. Under READ COMMITTED contention
the 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,
SaveChangesAsync did not throw, and AfterCommitAsync fired for a write that
never landed.

The fix folds the read into the same statement
(DO UPDATE SET ... RETURNING mt_version INTO final_version) and
RETURN COALESCE(final_version, 0) so that a filtered conflict update produces
0, 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.ProjectLatest was queueing an upsert at the projected
revision via session.Store(snapshot), and the configured inline projection
queued a second upsert at the same revision during SaveChangesAsync. Under
the old write-loss behaviour the second write silently became a no-op. With
the upsert function now reporting 0 when its WHERE filter skips the row,
the second upsert correctly surfaces as ConcurrencyException — and the
existing test inline_projection_stores_document_on_project_latest started
to fail in CI.

The inline projection persists the projected document itself during
SaveChangesAsync, so the explicit Store in the inline fetch plan was
always redundant; it just happened to be harmless under the buggy upsert
function. FetchAsyncPlan keeps its session.Store because async
projections don't run inline.

Test plan

Run on net10.0 against PostgreSQL 16:

  • EventSourcingTests filter project_latest_tests — 6/6 pass (was 5 pass / 1 fail on the previous PR head)
  • DocumentDbTests filter numeric_revisioning|Concurrency — 66/66 (includes the new race regression test from Fix silent write-loss in mt_upsert_<doc> for numeric-revisioned documents #4325)
  • EventSourcingTests filter numeric_revisioning|Concurrency — 21/21
  • EventSourcingTests filter Aggregation|Projection — 409/409
  • EventSourcingTests filter FetchForWriting|FetchLatest|ProjectLatest|InlineProjection|inline_aggregation — 212/212
  • EventSourcingTests filter Bug_3310_inline_projections|SubscriptionScopedSession|MultiStream — 22/22
  • PatchingTests — 114/115 (1 pre-existing skip)
  • MultiTenancyTests filter revisioning|Concurrency|Aggregation — 1/1
  • CoreTests filter migrate_from_guid_to_int_based_revisions — 1/1

🤖 Generated with Claude Code

ArieGato and others added 4 commits May 4, 2026 13:38
…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
jeremydmiller merged commit 5ad9772 into master May 4, 2026
6 checks passed
@jeremydmiller
jeremydmiller deleted the fix/numeric-revisions-upsert-and-projectlatest-double-store branch May 4, 2026 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants