Skip to content

GH-5160, GH-5165, GH-5166: [DeduplicatedWithResponse] ClaimInTransaction, plus two Marten commit-failure fixes - #5169

Merged
jeremydmiller merged 5 commits into
mainfrom
gh-5160-5165-5166-deduplication
Oct 23, 2026
Merged

jeremydmiller merged 5 commits into
mainfrom
gh-5160-5165-5166-deduplication

Conversation

@jeremydmiller

Copy link
Copy Markdown
Member

Closes #5160. Closes #5165. Closes #5166.

This PR fixes all three open issues from @uniquelau. It builds on their PR #5161, and its three commits for #5160 are included unchanged, under their authorship. This PR supersedes #5161. Laurence Gillian is a co-author of the commit for #5165 and #5166.

Merge with --merge, not --squash, so Laurence keeps authorship of the #5160 commits.

#5160: [DeduplicatedWithResponse(ClaimInTransaction = true)]

This is #5161 as written: commits 282abccd1, 2cfee6bed and 148fd57b2, with main merged in. #5161's description covers the design. In short:

  • It's opt-in, and only for Marten endpoints that commit through Wolverine's transactional middleware.
  • The claim and its response are written in the endpoint's own transaction.
  • A concurrent loser is answered from the winner's claim.

#5161's open question (should this be automatic, as [Deduplicated] is) is still open; this PR keeps it opt-in.

#5165: a failure after Marten's database commit released the claim

Marten runs session listeners' AfterCommitAsync inside SaveChangesAsync, after the database commit. Wolverine's outbox flush is one of those listeners. On the default claim-and-release path, a throw there left the work committed and released the claim, so the retry ran the endpoint again.

  • DatabaseCommitMarker (new, Wolverine.Marten) is a session listener inserted at index 0 of the session's listeners. Marten runs listeners in list order, so it records the commit before the outbox flush or any user listener can throw.
  • IDatabaseCommitHookProvider (new, internal) is a sibling seam to ITransactionalReplayableDeduplicationProvider. Only Marten implements it. The HTTP chain uses it to enlist the marker, with a callback that sets HttpHandler.MarkDeduplicatedDatabaseCommitted.
  • ReleaseUnansweredDeduplicatedResponseFrame gets a catch { KeepDeduplicatedClaimIfCommitted(ctx); throw; }, but only when the hook applies. On a failure over a committed database transaction, the claim is kept, unanswered. Repeats get 409 until it expires instead of running the work again. That is the same outcome as a failed response record today, and the fix [DeduplicatedWithResponse] on Marten: a failure inside SaveChangesAsync after the database commit releases the claim, so the retry runs the work again #5165 suggested.
  • A request that completes normally keeps its current rule. A 4xx answer or a null resource still gives the claim back.
  • Endpoints on other stores generate the same code as before.

#5166: a concurrent [WriteAggregate] loser under [Deduplicated] got a 500

RefuseDuplicateClaimAtCommitFrame only matches a unique violation on the deduplication table. A concurrent [WriteAggregate] loser fails on the stream version before it reaches the claim's INSERT, so it escaped as a 500.

  • Marten now uses its own commit-race wrapper, RefuseDuplicateMartenClaimAtCommitFrame. It keeps Deduplication should ride the native transaction on Marten, Polecat and Fisher — batched optimistic check, no compensating release #4505's classifier catch, and adds a second catch that runs only while the commit is in progress: CommitStarted && !Committed, from the same DatabaseCommitMarker.
    • In that window, it checks on a fresh query session (MartenDeduplicationFailures.IsClaimedAfterFailedCommitAsync) whether the key is now claimed.
    • If it is, the request gets the endpoint's configured refusal: 409 on HTTP, a discard on a handler.
    • If not, the original failure is rethrown unchanged.
  • Failures in the endpoint itself, or after the database commit, are never looked up, so a request's own committed claim can't be mistaken for a concurrent win.
  • No public interface changes. IMartenDeduplicator is untouched; the lookup is a static on MartenDeduplicationFailures.

Polecat probably has the same gap as #5166 (it uses the same core frame), but it is not touched here.

Tests

New tests:

Mutation checks, each applied and then reverted:

Run locally:

  • dotnet build wolverine.slnx -c Release -f net9.0: 0 errors
  • Wolverine.Http.Tests, FullyQualifiedName~eduplicat: 101/101
  • MartenTests, FullyQualifiedName~eduplicat: 17/17
  • Wolverine.Http.FSharpTests, including the coverage ratchet: 11/11

Docs: docs/guide/http/deduplication.md (claim-and-release on Marten) and docs/guide/durability/idempotency.md (the [WriteAggregate] race).

🤖 Generated with Claude Code

https://claude.ai/code/session_01DCggXgxnG3EBXoR9rrq37U

uniquelau and others added 5 commits October 23, 2026 13:49
… the claim and its response inside the endpoint's transaction

An opt-in ClaimInTransaction on [DeduplicatedWithResponse]. On an endpoint
that commits through a Marten session with Wolverine's transactional
middleware, on a single-database store with the PostgreSQL message store in
the same database, the claim and its stored response are written as one row,
queued onto the session before SaveChangesAsync. A rollback, or a crash
before the commit, leaves no claim; a commit leaves it answered. There is no
release and no record left to fail afterwards. The default is unchanged.

- Before the endpoint runs, the chain answers from any committed claim.
- A commit that fails is answered from a claim a concurrent request committed
  meanwhile, on the claim's primary key or on the endpoint's own work (a
  stream version conflict); otherwise it is rethrown. A failure after this
  request's own database commit (the outbox flush runs inside
  SaveChangesAsync on Marten) is told apart by the claim token and rethrown.
- An endpoint that asks for it but cannot take it keeps claim-and-release
  and logs a warning, including one whose response is produced after the
  commit (UpdatedAggregate). The same-database check is repeated at runtime
  for pre-generated code.
- The provider seam (ITransactionalReplayableDeduplicationProvider) and the
  ownership query are internal; no existing interface changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JxXDGaqdYTzZ54ZmdzUwH4
…lure after the database commit

From the Copilot review on #5161, both reproduced before fixing.

- MartenDeduplicatedResponses cached the claims table by the session's
  NpgsqlDataSource alone. An application that hands one data source to its
  main and ancillary stores (StoreOptions.Connection(dataSource)) got the
  first store's table for both: the ancillary claim landed where it is
  never read, and the repeat ran the endpoint again and failed at commit.
  The cache is now keyed by data source and store marker.
- A Marten session listener that throws from AfterCommitAsync fails
  SaveChangesAsync after the database commit. The exception now has a test:
  it reaches the caller, the answered claim and the work stay committed
  once, and the retry replays without running the endpoint.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JxXDGaqdYTzZ54ZmdzUwH4
…e, as [Deduplicated] does (GH-4505)

Aligns the in-transaction path with [Deduplicated] on Marten.

- The existing-claim lookup now goes through the Marten session, supplied by
  the provider as TransactionalDeduplication.Check is. It is a query handler
  returning the stored claim, and it joins the chain's batched query when
  the key is required, so a [WriteAggregate] endpoint pays one round trip
  for the lookup and FetchForWriting together. An optional key stays
  standalone, so a request without one costs no round trip.
- The runtime seam is now IMartenDeduplicatedResponses, a public interface
  with an internal implementation, as IMartenDeduplicator is.
- HTTP computes the hashed storage id and answers from the provider's
  lookup; the separate-connection lookup frame is gone.

The race handling deliberately stays different. A probe of [Deduplicated]
showed that its concurrent [WriteAggregate] loser fails on the stream
version before reaching the claim, and gets a 500 rather than the 409.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JxXDGaqdYTzZ54ZmdzUwH4
…e SaveChangesAsync can still throw

Marten runs its session listeners' AfterCommitAsync inside SaveChangesAsync, after the
database commit, Wolverine's outbox flush among them. From outside the call a failure
there looked the same as a failed commit. A DatabaseCommitMarker listener, inserted at
the front of the session's listeners, now records when the commit started and when the
database commit succeeded.

GH-5165: on a claim-and-release [DeduplicatedWithResponse] Marten endpoint, a failure
after the database commit released the claim, so the client's retry ran the committed
work again. The chain now hears the commit (internal IDatabaseCommitHookProvider seam,
Marten only) and keeps the claim unanswered on a failure over committed work: repeats
get 409 until it expires, the same as a failed record. A request that completes
normally keeps the existing rule.

GH-5166: on [Deduplicated] with Marten (GH-4505), a concurrent [WriteAggregate] loser
fails on the stream version before reaching the claim's INSERT, so the unique-violation
classifier never matched and it escaped as a 500. A commit that fails for any other
reason is now checked on a fresh query session: if the key is claimed, it gets the same
refusal (409 / discard), otherwise the failure is rethrown unaltered. Failures before
the commit or after the database commit are never looked up.

Co-Authored-By: Laurence Gillian <lau@eddlondon.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DCggXgxnG3EBXoR9rrq37U
@uniquelau

Copy link
Copy Markdown
Contributor

Thanks for picking these up, and for keeping the #5160 commits as they were. Two things for #5169, both with a follow-up I can send once it has merged.

1. On the ClaimInTransaction path, a version Marten assigns during the commit isn't in the replay.

The response is serialized just before SaveChangesAsync. Marten writes a document's Guid [Version] and its IRevisioned revision onto the object during the commit, so the first client gets the real values and every repeat gets the pre-commit ones. I ran this on #5169's head (a3d808f6c), with [DeduplicatedWithResponse(DeduplicationScope.User, ClaimInTransaction = true)] on an endpoint that Store()s the document and returns it:

Returned document First response Replay
Guid [Version] "version":"01a16935-d45f-…" "version":"00000000-0000-0000-0000-000000000000"
IRevisioned "version":1 "version":0

The #5161 description lists this as a trade-off, but I don't think it's one worth keeping: a client using that version for its next write would be handed a stale one on every retry. A probe of Marten's operation lifecycle showed the Guid version is set before the batch executes, but the numeric revision only after its command has run. So no single write inside the transaction can carry both.

The fix I'd send keeps the claim in the transaction and records the response again after the commit, as claim-and-release does today. It overwrites the stored response only when the commit changed it, using a token-guarded internal store call, so no public interface changes.

2. For #5165, an alternative that replays instead of refusing.

With DatabaseCommitMarker, a failure after the database commit keeps the claim unanswered, so repeats get 409 until it expires. That's the fix the issue suggested, and it's simpler.

I have a local branch that does it differently. On a Marten chain it queues the response record onto the session, the way FlushOutgoingMessagesOnCommit queues the inbox mark-handled UPDATE. The claim is answered if and only if the work commits, and the retry is replayed rather than refused. The default path then needs the same after-commit correction as point 1, which makes it more code than your marker.

The 409 is a sound answer, so I'm only raising the replay as an option. If you'd like it, I'll rebase it onto #5169 as a follow-up.

@uniquelau

Copy link
Copy Markdown
Contributor

A review of a3d808f6c. The listener ordering, a failed commit still releasing the claim, the fresh-session lookup (right store and table, ancillary included) and the separation from ClaimInTransaction all check out. Five things, each marked with how far I verified it:

1. #5166 under EventAppendMode.Rich: a loser that saves after the winner committed still gets a 500. Reproduced.
DatabaseCommitMarker marks the commit as started in BeforeSaveChangesAsync. Marten runs EventGraph.ProcessEventsAsync before that (DocumentSessionBase.SaveChanges.cs, line 37 against 58). Under Rich mode, StreamAction.PrepareEvents throws EventStreamUnexpectedMaxEventIdException in process, so the new catch's CommitStarted filter is false.
I ran it on this branch with the winner committing fully before the loser called SaveChangesAsync:

AppendMode Winner Loser Events committed
default (QuickWithServerTimestamps) 200 409 1
Quick 200 409 1
Rich 200 500 (EventStreamUnexpectedMaxEventIdException) 1

The default is fine. Rich mode is a common opt-in, though, and idempotency.md states the fix generally.
Suggested fix: mark "commit started" from the generated code, immediately before the SaveChangesAsync frame, rather than from the listener.

2. Marten runs Wolverine's own message batch before the listener loop. Ordering confirmed in Marten 9.51's source; not run.
_messageBatch.AfterCommitAsync (line 74) runs before the AfterCommitAsync loop (line 89). Under ServerlessWithOutbox, that batch is MessageContext.SendAllOutgoingMessagesInlineAsync, so a failing inline send there isn't seen by the marker.

Suggested fix: record "committed" from the earliest post-COMMIT hook Marten exposes, if there is a public one; otherwise note the gap.

3. A user's OnException handler sits inside the new catch. Ordering confirmed by reading the code; not run.
GetOrCreateTryCatchFinallyFrame inserts at Middleware[0] while policies apply, and the deduplication frames go to Middleware[0] later, in AssembleTypes. An OnException handler that turns the post-commit failure into a ProblemDetails therefore returns normally, and the finally (and OnStarting) see work-not-done and release the claim.
Suggested fix: have the release guards honour the committed flag too, at least for a 5xx.

4. EnlistDatabaseCommitHookFrame has no GenerateFSharpCode. Confirmed.
The other frames on a Marten claim-and-release [DeduplicatedWithResponse] chain all emit F#, so F# generation of such an endpoint now hits the default throw. FSharpCoverageRatchet only scans Wolverine and Wolverine.Http, so it can't catch this.

5. The marker counts every commit in the request, not just the middleware's. Plausible; not run.
The flags are never reset. An endpoint that calls SaveChangesAsync itself for an earlier write, followed by a middleware commit that fails in the database, keeps the claim unanswered. The client then gets 409s for the whole window, though the main work rolled back.
Suggested fix: enlist the marker just before the commit frame, or reset the flags in BeforeSaveChangesAsync.

@jeremydmiller
jeremydmiller merged commit f79581e into main Oct 23, 2026
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment