fix(batched-query): enlist FetchForExclusiveWriting item before awaiting BeginTransactionAsync (#4589)#4590
Merged
jeremydmiller merged 1 commit intoMay 30, 2026
Conversation
…ing BeginTransactionAsync
`BatchedQuery.FetchForExclusiveWriting<T>` (both Guid and string-key
overloads) had `await Parent.BeginTransactionAsync(...)` as its first
statement, before calling `AddItem(handler)` to enlist the query item.
Under concurrency, `BeginTransactionAsync` does not complete
synchronously — `AutoClosingLifetime.StartAsync` performs a real
`NpgsqlConnection.OpenAsync` socket round-trip — so the method yielded
before `AddItem` ran.
Wolverine HTTP's `[Aggregate(LoadStyle = Exclusive)]` codegen calls the
method without immediately awaiting:
var task = batch.Events.FetchForExclusiveWriting<T>(id);
await batch.Execute(ct);
var stream = await task;
Under the race, `Execute` ran with `_items.Count == 0`, returned
immediately, and `task.Result` was never going to be populated
(`item.Result` is set only by `Execute`). The `await` on `task`
wedged forever.
The non-exclusive `FetchForWriting<T>` overloads in this same file
don't have the bug because they're synchronous — `AddItem` always
runs before the caller gets the Task back.
Fix: enlist via `AddItem` synchronously before any `await`, so the
item is in `_items` by the time control returns to the caller. Any
subsequent `Execute()` is then guaranteed to see and process it.
A 20-parallel-loop reproducer against a Wolverine HTTP
`[Aggregate(LoadStyle = Exclusive)]` endpoint hung past 60s before
this fix; passes in 5s with the fix. Full reproducer + diagnostic
trace in the linked issue.
This was referenced May 30, 2026
Merged
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.
Fixes #4589.
What
Reorders the two
BatchedQuery.FetchForExclusiveWriting<T>overloads (Guid and string-key) to callAddItem(handler)synchronously before theawait Parent.BeginTransactionAsync(...).Why
Detail in #4589. TL;DR:
BeginTransactionAsyncfirst, then callsAddItem. Under concurrency,BeginTransactionAsyncyields beforeAddItemruns.[Aggregate(LoadStyle = Exclusive)]codegen callsFetchForExclusiveWritingwithout immediately awaiting it (the Task is captured for later resolution), thenawait batchQuery.Execute(ct).Executeruns while_items.Count == 0, returns immediately, and theBatchQueryItem.Resultis never populated. The awaiter on the captured Task wedges forever.FetchForWriting<T>overloads in this same file (lines 83-131) don't have the bug because they're synchronous —AddItemalways runs before the caller gets the Task back.The fix is the minimum-mechanical-change form of the same insight that already keeps the non-exclusive overloads correct: enlist first, await second.
Diff
public async Task<IEventStream<T>> FetchForExclusiveWriting<T>(Guid id) where T : class { - await Parent.BeginTransactionAsync(CancellationToken.None).ConfigureAwait(false); - + // Enlist synchronously BEFORE the first await so the item is in _items + // by the time control returns to the caller. A subsequent Execute() is + // then guaranteed to see and process the item. _documentTypes.Add(typeof(IEvent)); var plan = Parent.Events.As<EventStore>().FindFetchPlan<T, Guid>(); if (plan.Lifecycle != ProjectionLifecycle.Live) { _documentTypes.Add(typeof(T)); } var handler = plan.BuildQueryHandler(Parent, id, true); + var resultTask = AddItem(handler); - return await AddItem(handler).ConfigureAwait(false); + await Parent.BeginTransactionAsync(CancellationToken.None).ConfigureAwait(false); + return await resultTask.ConfigureAwait(false); }(Same shape for the
string-key overload.)The
CancellationToken.Noneis preserved here to keep the publicIBatchEvents.FetchForExclusiveWriting<T>interface unchanged. The fact that the caller's token is being dropped is a separate concern noted in the issue — happy to follow up with an interface-touching PR for that, or bundle here if you'd prefer.Verification
Reproducer in the issue. A 20-parallel-loop concurrent test against a Wolverine HTTP
[Aggregate(LoadStyle = Exclusive)]endpoint:HANDLER-ENTER)Risks
BatchedQueryinstance. The transaction is still begun beforeExecuteruns (Executeis only called by the caller AFTER all enlistment calls return, and ourBeginTransactionAsyncawait completes before the method returns).FetchForWriting<T>overloads in the same file already use exactly this enlist-first pattern (they're sync, not async, which is why they don't have the bug) — the change here aligns the async overloads with the same correctness invariant.Test coverage
Adding a fixture-level test for this would be ideal but requires Testcontainers + a Wolverine HTTP host to stage the codegen-flavored call site that exposes the race. I haven't included one in this PR to keep the surface minimal; happy to add a smaller-scope unit test that asserts
AddItem-before-BeginTransactionAsyncordering via a stubParent, or to wire up an integration test underMartenTests.AggregateHandlerWorkflow— let me know which (if either) you'd want before merge.