Make TrackedSession bookkeeping thread-safe by alesdvorakcz · Pull Request #3070 · JasperFx/wolverine

alesdvorakcz · 2026-06-11T08:42:33Z

What & why

When tracked sessions run with IncludeExternalTransports(), envelope records arrive concurrently from parallel transport listener threads (in our case ~90 Rabbit MQ queues on one host). EnvelopeHistory backed its records with an unsynchronized List<T>, so concurrent Add + LINQ iteration intermittently threw a NullReferenceException from inside the tracking bookkeeping itself, surfacing through AssertNoExceptionsWereThrown() and failing whatever test happened to have an active session (~1 in 4 runs of a ~50-test integration suite for us):

System.AggregateException : One or more errors occurred. (Object reference not set to an instance of an object.)
---- System.NullReferenceException
   at System.Linq.Enumerable.TryGetLast[TSource](IEnumerable`1 source, Func`2 predicate, Boolean& found)
   at Wolverine.Tracking.EnvelopeHistory.RecordCrossApplication(EnvelopeRecord record)
   at Wolverine.Runtime.WolverineRuntime.Received(Envelope envelope)
   at Wolverine.Runtime.HandlerPipeline.TryDeserializeEnvelope(Envelope envelope)
   ...

TrackedSession's _statuses and _exceptions lists have the same unguarded add/enumerate pattern (Record(...) / LogException(...) append from listener threads while AllRecordsInOrder() / AssertNoExceptionsWereThrown() enumerate).

The fix

EnvelopeHistory: all members that touch _records now synchronize on a private lock; the Records property returns a snapshot array instead of the live list.
TrackedSession: _statuses and _exceptions are appended under a lock on the collection instance, and all read sites go through snapshot helpers.

This is test-support code on cold paths, so the locking has no practical overhead.

Testing

New regression test envelope_history_thread_safety hammers RecordCrossApplication + the read paths from 8 threads. Against the previous implementation it fails deterministically (in under 150 ms) with the exact NullReferenceException from the report above; with this change it passes.
All 43 existing CoreTests.Tracking tests pass on net9.0 and net10.0.

Fixes #3069

EnvelopeHistory backed its records with an unsynchronized List<T>, but with IncludeExternalTransports() records arrive concurrently from parallel transport listener threads (e.g. multiple Rabbit MQ queues). Concurrent Add + LINQ iteration intermittently threw NullReferenceException from RecordCrossApplication, surfacing through AssertNoExceptionsWereThrown and failing whatever test had an active tracked session. TrackedSession's _statuses and _exceptions lists had the same unguarded add/enumerate pattern. All EnvelopeHistory members now synchronize on a private lock (Records returns a snapshot), and the session lists are guarded by locking with snapshot reads. Includes a regression stress test that reproduces the NullReferenceException deterministically against the previous implementation.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR hardens Wolverine tracking against concurrency issues when multiple transport listener threads record activity for the same envelope/session.

Changes:

Add synchronization around TrackedSession status/exception collections via snapshot reads + locked writes
Make EnvelopeHistory thread-safe by guarding _records with a private lock and returning record snapshots
Add a stress test reproducing concurrent listener recording

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.

File	Description
src/Wolverine/Tracking/TrackedSession.cs	Avoid concurrent enumeration/modification by snapshotting and locking around exception/status collections
src/Wolverine/Tracking/EnvelopeHistory.cs	Serialize access to `_records` via a private lock; return snapshots to prevent concurrent iteration faults
src/Testing/CoreTests/Tracking/envelope_history_thread_safety.cs	Add a multi-writer stress test to validate thread safety

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

alesdvorakcz · 2026-06-11T10:18:03Z

+    private EnvelopeRecord[] statusesSnapshot()
+    {
+        lock (_statuses)
+        {
+            return _statuses.ToArray();
+        }
+    }
+
+    private Exception[] exceptionsSnapshot()
+    {
+        lock (_exceptions)
+        {
+            return _exceptions.ToArray();
+        }
+    }


The camelCase is on purpose — the private methods in these files already use it
(lastOf, markLastCompleted, writeGrid, ...), so I just matched the surrounding code.
Can rename if the maintainers prefer PascalCase here.

Copilot is lying. Our naming convention is camelCasing for private members:)

alesdvorakcz · 2026-06-11T10:18:41Z

+        lock (_statuses)
+        {
+            return _statuses.ToArray();
+        }


Locking on the collections themselves is safe here — they're private and never leave the
class. But I have no strong feelings about it, happy to add dedicated lock fields if that's
the style maintainers prefer.

alesdvorakcz · 2026-06-11T10:20:57Z

        var conditions = $"Conditions:\n{_conditions.Select(x => x.ToString())!.Join("\n")}";
        var activity = $"Activity:\n{AllRecordsInOrder().Select(x => x.ToString()).Join("\n")}";
-        var exceptions = $"Exceptions:\n{_exceptions.Select(x => x.ToString()).Join("\n")}";
+        var exceptions = $"Exceptions:\n{exceptionsSnapshot().Select(x => x.ToString()).Join("\n")}";

        return $"{conditions}\n\n{activity}\\{exceptions}";


Nice catch, but this PR only changed the line above it. Looks like it was meant to be \n\n.
I think it is out of scope for this PR, but if you want, I am happy to fix it.

alesdvorakcz · 2026-06-11T10:21:59Z

+    public IEnumerable<EnvelopeRecord> Records
+    {
+        get
+        {
+            lock (_lock)
+            {
+                return _records.ToArray();
+            }
+        }
+    }


Agreed, the signature could say this better. I can change it to EnvelopeRecord[] or IReadOnlyList<EnvelopeRecord> or leave it as is — whichever @jeremydmiller prefers.

…3070) (#4585) EnvelopeHistory kept its EnvelopeRecords in a bare List<T> with no synchronisation while records arrive concurrently. A resize racing a read threw out of IsComplete(), surfacing as an AggregateException from TrackedSession.AssertNoExceptionsWereThrown() with no hint of the real cause. #3069 reported it through RecordCrossApplication with RabbitMQ. #3968 confirmed the same race independently on the purely LOCAL path with no external transport, reached through WolverineRuntime.MessageSucceeded -- so it races on handler-completion threads too, not only transport-listener threads. Their trigger was MultipleHandlerBehavior.Separated: one ExecuteAndWaitAsync fanning out to many envelopes completing at once. Reproduced roughly 1 run in 7 of a ~1850-test suite, ~3% over a longer history. The reporter verified by reflection that no shipped 5.x has the fix: 5.39.3 and 5.40.1 both lack the _lock field, and 5.40.1 published ~8 weeks AFTER 6.8.0 carried it. So the only route for a 5.x user was a major-version upgrade to fix test-harness flakiness. The locking topology matches 6.x member for member -- verified mechanically, not by eye: - the two writers, RecordLocally and RecordCrossApplication, hold the lock across their whole body - IsComplete, Has, MessageFor, Message and Records take it for the read - the three private helpers (lastOf, both markLastCompleted overloads) stay unlocked, because they are only ever called from inside an already-locked writer Records also now hands out a snapshot rather than the live list. On 5.x it was `public IEnumerable<EnvelopeRecord> Records => _records;`, which lets a caller enumerate the list while a recording thread is adding to it -- the same race one level removed. This is test-support code (Wolverine.Tracking is not in a production path), no public API changes. ## On the tests Three, driven hard and directly: a 3%-of-full-suite race is no use as a regression test. All three fail on the unfixed class on EVERY run -- checked three times over, with the baseline's own build asserted clean first so it could not be a stale binary. Worth recording why the first one needed strengthening. As first written it exercised IsComplete(), Has() and Records.Count(), and it PASSED unfixed: IsComplete and Has walk the list with an indexed for loop, and List<T> answers Count without enumerating, so none of the three throw under concurrent mutation -- they can read a torn value, but quietly. Only LINQ over the list raises "Collection was modified", so the test now drives Message and MessageFor, which enumerate. The indexed readers are kept for coverage but are not what makes the test fail. Full CoreTests on 5.0: 1695 green. Claude-Session: https://claude.ai/code/session_01VDUrBeB4tTnKj4AExCS1nj Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings June 11, 2026 08:42

Copilot AI reviewed Jun 11, 2026

View reviewed changes

jeremydmiller merged commit 83cc427 into JasperFx:main Jun 11, 2026
24 checks passed

vpetrevski mentioned this pull request Aug 16, 2026

Backport EnvelopeHistory thread-safety fix (#3069) to 5.x? #3968

Open

jeremydmiller mentioned this pull request Sep 23, 2026

GH-3968: make EnvelopeHistory thread-safe on 5.x (backport of #3069/#3070) #4585

Merged

Navigation Menu

Repository navigation

Make TrackedSession bookkeeping thread-safe - #3070

Make TrackedSession bookkeeping thread-safe#3070
jeremydmiller merged 1 commit into
JasperFx:mainfrom
alesdvorakcz:thread-safe-tracked-session-bookkeeping

alesdvorakcz commented Jun 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

alesdvorakcz Jun 11, 2026

Uh oh!

jeremydmiller Jun 11, 2026

Uh oh!

alesdvorakcz Jun 11, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alesdvorakcz Jun 11, 2026

Uh oh!

alesdvorakcz Jun 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

alesdvorakcz commented Jun 11, 2026

What & why

The fix

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

alesdvorakcz Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

jeremydmiller Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

alesdvorakcz Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alesdvorakcz Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

alesdvorakcz Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants