Skip to content

GH-4167: one BatchingProcessor per batched message type, not one per racing caller - #4168

Merged
jeremydmiller merged 1 commit into
mainfrom
gh-4167-batching-processor-race
Aug 27, 2026
Merged

jeremydmiller merged 1 commit into
mainfrom
gh-4167-batching-processor-race

Conversation

@jeremydmiller

Copy link
Copy Markdown
Member

Found while verifying the JasperFx fix for #4167 (JasperFx/jasperfx#714). This is a live bug on main today — it does not depend on that fix.

The bug

BatchingOptions.BuildHandler is an unguarded lazy init, and it runs on the message-handling path, not at bootstrap:

if (_handler != null) return _handler;
_handler = builder.Build(runtime, Batcher, this);

Callers reach it through HandlerPipeline's LightweightCache<Type, IExecutor>, whose indexer does not lock — two concurrent misses each invoke the factory and each returns its own instance.

For a stateless executor that is harmless, which is why the cache is fine everywhere else. A BatchingProcessor is not stateless: each instance owns a separate BatchingChannel buffer, its own flush Timer, and two Blocks with live worker tasks.

Instrumented — two instances on two threads, and a two-member batch arrives as two batches:

BuildHandler BUILT instance 11865849 for ExpiryItem on tid=24
BuildHandler BUILT instance 34717384 for ExpiryItem on tid=19
TriggerBatch count=1 tid=21
TriggerBatch count=1 tid=6
→ 2 batch(es): [one] | [two]

Two consequences:

  1. Batches silently fragment — members of one logical batch are split across buffers and flushed separately.
  2. The loser leaks. It is never handed back to any caller, so nothing disposes it; its Timer and worker tasks live for the process lifetime.

The fix

Double-checked lock, with _handler made volatile for the unlocked read.

Tests

concurrent_BuildHandler_yields_one_shared_processor races 8 callers into BuildHandler and asserts reference equality. It fails on every run without the fix, against the currently referenced JasperFx 2.56.0 — deliberately written to exercise the race directly rather than through message publishing, so it does not depend on the JasperFx release to be meaningful.

The end-to-end companion (concurrent_first_messages_still_assemble_a_single_batch) still passes on 2.56.0 even unfixed, because inline Block continuations serialize the first messages. It becomes a real guard after the JasperFx bump; the comment says so.

Also: Bug_3399's wait condition

A latent test race the same inline execution was hiding. A WaitFor* condition replaces quiescence rather than adding to it, so WaitForMessageToBeReceivedAt released the session at receipt, before any handler ran, and the assertion raced it. Probed: Batched=0 immediately, Batched=1 after 1s — the handler runs, the test just asked the wrong question.

Now waits for execution, of both separated chains for the batched array; waiting on one only trades a receive/execute race for a which-chain-won race.

Verification

Follow-up

The GH-4167 local-queue reproduction test is not in this PR — it asserts the publisher thread never runs the handler, which cannot pass until JasperFx ships #714. It should land with the JasperFx version bump, as that bump's acceptance test.

🤖 Generated with Claude Code

…racing caller

BatchingOptions.BuildHandler was an unguarded `if (_handler != null)` lazy init,
and it runs on the message-handling path rather than at bootstrap. Callers reach
it through HandlerPipeline's LightweightCache<Type, IExecutor>, whose indexer
does not lock: two concurrent misses each invoke the factory AND each returns
its own instance.

That is harmless for a stateless executor, which is why the cache is fine
everywhere else. A BatchingProcessor is not stateless -- each instance owns a
separate BatchingChannel buffer, its own flush Timer, and two Blocks with live
worker tasks. Two instances means members of one logical batch are split across
two buffers and flushed as separate batches, and the losing instance is never
handed back to anyone, so nothing ever disposes it: its timer and worker tasks
leak for the life of the process.

Instrumented, two instances were built on two threads and a two-member batch
arrived as [one] | [two].

Fixed with a double-checked lock; _handler is now volatile for the unlocked
read. concurrent_BuildHandler_yields_one_shared_processor asserts reference
equality across 8 racing callers and fails on every run without this change,
against the currently referenced JasperFx 2.56.0 -- this race is reachable
today, it is not introduced by the JasperFx fix.

What the JasperFx fix (#714) changes is only how easy it is to hit: its
inline Block continuations were running the first messages on the publisher's
thread and serializing them, which is why the end-to-end batch test still passes
on 2.56.0 and only becomes a real guard after the bump.

Also fixes Bug_3399's wait condition, which was a latent test race the same
inline execution was hiding. A WaitFor* condition REPLACES quiescence rather
than adding to it, so WaitForMessageToBeReceivedAt released the session at
receipt, before any handler ran. It now waits for execution -- of BOTH separated
chains for the batched array, since waiting on one just trades a receive/execute
race for a which-chain-won race.

Verified: CoreTests 2664 passed / 2 skipped / 0 failed against a locally built
JasperFx carrying #714, and green on stock 2.56.0. Full wolverine.slnx
builds clean under -c Release -f net9.0.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jeremydmiller
jeremydmiller merged commit 29d49a6 into main Aug 27, 2026
39 checks passed
jeremydmiller added a commit that referenced this pull request Aug 27, 2026
Picks up #714: a Block no longer runs its action on the publisher's
thread. Block built its channel with AllowSynchronousContinuations = true, so a
reader parked in WaitToReadAsync was resumed by TryWrite on the publishing
thread, and Post() executed the action inline instead of enqueuing it.

For Wolverine that meant a buffered local queue got no parallelism at all after
its workers went idle: a burst of 20 published messages all ran inline and
serialized on the publishing thread, and that thread -- a broker listener loop,
an HTTP request -- stalled for the full duration of each handler.

Adds the local-queue reproduction from GH-4167 as this bump's acceptance test.
It was deliberately held out of #4168 because it cannot pass against 2.56.0 and
would have made CI red; it passes from 2.57.0 onward.

Note the reported ".NET 10 regression" framing is only half right, and the test
comments record why: an UNBOUNDED channel (what a buffered local queue uses,
GH-3287) ran continuations inline on every runtime. dotnet/runtime#116021
changed the BOUNDED case, which is what broker-backed BufferedReceivers and
DurableReceiver use.

The companion Wolverine fix for the batching race this exposes is already on
main (29d49a6), so this bump is safe to take.

Verified against the published 2.57.0 packages, not a local build: CoreTests
2664 passed / 2 skipped / 0 failed, and wolverine.slnx clean under
-c Release -f net9.0 after a cleared HTTP cache and full restore.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.

1 participant