Repository navigation
fix(messaging): snapshot rule chain — concurrent Remove NREs the dispatch walk (sync-hub flake) - #333
Merged
Merged
Conversation
…dispatch walk MessageHub dispatches a delivery by folding over its rule chain (`rules`, a ThreadSafeLinkedList<AsyncDelivery>). HandleMessageAsync walked the chain via the raw LinkedListNode.Next OUTSIDE the list's lock. ThreadSafeLinkedList locks Add/Remove/First but not that walk, so when a handler-disposable's rules.Remove(node) fires concurrently (rapid sync-hub create/teardown churn under load), LinkedList.Remove→Invalidate nulls the removed node's owning-list reference before its next pointer, and a racing get_Next() dereferences list.head → NullReferenceException. That fails the delivery → DeliveryFailure → the synchronization stream reports [SYNC_STREAM] OnError and propagates a StreamErrorEvent → the client subscriber's .Within(Ns) never matches → timeout. Because it depends on which sync hub is being torn down at that instant, a different sync-hub layout test flakes each bulk run (EditorTest.TestEditorWithDelayed, EditPersistenceTest.EditState_ShouldSurviveDataUpdates, …). Fix: iterate a snapshot taken under the read lock instead of walking live nodes. - ThreadSafeLinkedList.Snapshot() copies the values under the read lock. - MessageHub.HandleMessageAsync folds over the snapshot array; a concurrent Remove can no longer invalidate the iteration. Also semantically correct: a delivery is handled by the rules present when dispatch began. Repro: `dotnet test test/MeshWeaver.Layout.Test` in a loop — ~1-in-5 runs failed with the NRE before, 9+ clean after. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an intermittent CI timeout/flakiness in sync-hub layout tests by eliminating a concurrency race in MessageHub rule-chain dispatch: the rule chain is now iterated from a point-in-time snapshot taken under ThreadSafeLinkedList’s read lock, preventing concurrent Remove() from invalidating a live LinkedListNode.Next walk.
Changes:
- Added
ThreadSafeLinkedList<T>.Snapshot()to safely copy list values under a read lock. - Updated
MessageHub.HandleMessageAsyncto fold over a snapshot array rather than walkingLinkedListNode.Next. - Kept/adjusted the existing guardrail by enforcing a maximum rule-chain length (500) based on the snapshot size.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/MeshWeaver.Messaging.Hub/ThreadSafeLinkedList.cs | Adds a read-locked snapshot API to support safe iteration under concurrent mutations. |
| src/MeshWeaver.Messaging.Hub/MessageHub.cs | Switches dispatch rule iteration to use the snapshot to avoid Remove()-vs-walk races and resulting NREs/timeouts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+756
to
+758
| // Snapshot the rule chain ONCE under the list's read lock (see HandleMessageAsync) so a | ||
| // concurrent rules.Remove during teardown can't NRE the iteration. | ||
| var ruleChain = rules.Snapshot(); |
Contributor
Contributor
Test Results (shard 5)832 tests +212 650 ✅ +212 3m 37s ⏱️ + 1m 52s Results for commit 5a4b6d7. ± Comparison against base commit 339c21f. This pull request removes 5 and adds 217 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Contributor
Test Results (shard 4) 10 files ± 0 10 suites ±0 4m 35s ⏱️ -20s Results for commit 5a4b6d7. ± Comparison against base commit 339c21f. This pull request removes 136 tests.♻️ This comment has been updated with latest results. |
Contributor
Contributor
Contributor
Test Results 56 files ± 0 56 suites ±0 22m 37s ⏱️ + 1m 8s Results for commit 5a4b6d7. ± Comparison against base commit 339c21f. This pull request removes 141 and adds 217 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Hammers MessageHub dispatch while a background task disposes+re-registers rules (each Dispose is a rules.Remove), over 300 round-trips on a 100-rule chain. Proven: fails on the pre-fix raw-LinkedListNode.Next walk (the delivery NREs → the response never lands → 10s timeout) and passes deterministically with the snapshot fix. Addresses the Copilot review on #333. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Thanks @copilot — added (FYI the shard-3 red on the first run was a foreign flake — |
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.
The flake
Sync-hub layout tests (`EditorTest.TestEditorWithDelayed`, `EditPersistenceTest.EditState_ShouldSurviveDataUpdates`, …) intermittently time out on CI — a different one each run. It turned main red once post-merge. Reproduced deterministically by running `MeshWeaver.Layout.Test` in bulk (whole project, one process): ~1 in 5 runs a sync-hub test fails.
Root cause
`MessageHub` dispatches a delivery by folding over its rule chain (`rules`, a `ThreadSafeLinkedList`). `HandleMessageAsync` walked the chain via the raw `LinkedListNode.Next`, outside the list's lock:
```csharp
current = current.Next; // MessageHub.cs — outside ThreadSafeLinkedList's lock
```
`ThreadSafeLinkedList` locks `Add`/`Remove`/`First`, but not this walk. When a handler-disposable's `rules.Remove(node)` fires concurrently (rapid sync-hub create/teardown churn under bulk load), `LinkedList.Remove → Invalidate` nulls the removed node's owning-list reference before its `next` pointer, so a racing `get_Next()` dereferences `list.head` → `NullReferenceException`. That fails the delivery → `DeliveryFailure` → the synchronization stream reports `[SYNC_STREAM] OnError` and propagates a `StreamErrorEvent` → the client subscriber's `.Within(Ns)` never matches → timeout. Which sync hub is being torn down at that instant decides which test flakes.
The fix
Iterate a snapshot taken under the read lock instead of walking live nodes:
Snapshotting is also semantically correct: a delivery is handled by the rules present when dispatch began.
Verification
Note — a separate, rarer flake remains
`TestEditorWithDelayed`'s final `.Within(30s)` wait can still time out under thread-pool starvation: its projection does `Thread.Sleep(100)` (blocks a pool thread) inside `ThrottleImmediate(20ms).Select(result)`. Seen once under a heavily-loaded local box (whole run ballooned 13s→1m4s). This PR fixes the NRE (the likely CI red, since that test uses a sync hub); the thread-pool one is a separate follow-up if it recurs on CI.
🤖 Generated with Claude Code