fix(rx): owned-connection releases go through one serial lane — concurrent terminals deadlocked teardown - #5660
Conversation
…rrent terminals deadlocked the permission fold in teardown #5647 (#5135) made an owner's release terminate every subscriber still attached, but scheduled each connection's terminal as its own TaskPool work item. An owner registry is released in ONE sweep (MeshNodeStreamCache disposes every query connection; a hub's ShutDown every registration), so N connections produced N concurrent terminals. The permission fold composes several cache queries under a Zip nested in a CombineLatest; Rx takes a combinator's gate on a terminal and disposes its other sources under it, so one release held the CombineLatest gate waiting for the Zip gate while another held the Zip gate waiting for the CombineLatest gate. The SP disposal then parked behind them (UiContributionCatalog.Dispose -> BehaviorSubject.OnCompleted -> the same CombineLatest), and Plugins teardowns timed out after a passing body (LayoutAreaIdentityTest, GitHubSyncSettingsTabTest on set 3.0.0-ci.9296). Reproduced locally 1 in 12 MTP runs of MeshWeaver.Security.Test with a stack capture of exactly those threads. ReleaseLane: releases posted to one lane run on the ThreadPool one at a time, in order. The mesh root registers one (like AccessService) and every hub resolves it; MeshNodeStreamCache uses the mesh lane; registries outside a hub hold one lane for all their connections. The lane-less CompositeDisposable overload is [Obsolete] (it mints a lane per connection). Test: ReleasingTwoConnectionsAConsumerComposes_NeverDeliversTheirTerminalsConcurrently forces the two releases to meet inside their first gates — red with one work item per release (negative control run by hand), green on a shared lane. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical and moderate findings remain in release-lane error handling, hub lane access, and regression-test synchronization.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
This PR serializes owned-connection release notifications to prevent Rx teardown deadlocks.
Changes:
- Adds mesh- and registry-scoped
ReleaseLane. - Routes owned connections and caches through shared lanes.
- Adds regression coverage and documents release ordering.
| File | Summary |
|---|---|
test/MeshWeaver.Messaging.Hub.Test/OwnedConnectionTest.cs |
Adds concurrent-release regression coverage. |
src/MeshWeaver.PluginCatalog/PackageListingCache.cs |
Uses a registry-specific release lane. |
src/MeshWeaver.Messaging.Hub/ReleaseLane.cs |
Implements serialized release delivery. |
src/MeshWeaver.Messaging.Hub/OwnedConnectionExtensions.cs |
Adds lane-aware ownership APIs. |
src/MeshWeaver.Messaging.Hub/MessageHubConfiguration.cs |
Registers the mesh-scoped lane. |
src/MeshWeaver.Hosting/MeshNodeStreamCache.cs |
Uses the mesh release lane. |
src/MeshWeaver.GitSync/GitHubRepoIdentityResolver.cs |
Uses registry-scoped release ordering. |
src/MeshWeaver.Documentation/Data/Architecture/HubDisposalModel.md |
Documents serialized release behavior. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| subject | ||
| .ObserveOn(TaskPoolScheduler.Default) | ||
| .Subscribe(release => release()); |
There was a problem hiding this comment.
Agreed — fixed in 3b130f3. ReleaseLane.Run now wraps each release: if a subscriber's error continuation throws, the exception is captured and re-raised on a ThreadPool work item of its own (ExceptionDispatchInfo.Throw via ThreadPool.UnsafeQueueUserWorkItem), which is exactly where it surfaced when every release was its own work item — so it stays as loud as before (an unhandled ThreadPool exception), and the lane's ObserveOn observer survives and drains the releases queued behind it. There is no existing scheduler error-boundary helper in src/ to route through; this keeps the pre-existing failure semantics rather than swallowing.
| Volatile.Write(ref aHoldsCombineLatestGate, 1); | ||
| SpinWait.SpinUntil(() => Volatile.Read(ref bHoldsZipGate) == 1, meetBudget); |
There was a problem hiding this comment.
Fair point on the green path — changed in 3b130f3. The wait is kept, but as an OBSERVATION window rather than synchronisation: its result is now recorded and asserted (bMetAWhileItHeldItsGate must be 0 — B's release never entered the consumer while A's held the CombineLatest gate — and -1 would mean A's hook never ran). That is the positive statement of the serialised contract, alongside the existing sourcesReleased assertion that A's teardown actually finished. The bounded meeting is what makes the NEGATIVE control deterministic (with one TaskPool work item per release the two releases meet inside their first gates and deadlock every time — run by hand: red at the sourcesReleased wait); a serialised lane cannot let the second release start while the first runs, so by construction any test that forces the concurrent interleaving has to observe "it never came" over some window on the green path. The 2 s window costs 2 s once, in one test.
…t asserts B never overlapped A's hold Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Plugins-side verification (MeshWeaver.Plugins
(The native xunit runner did not reproduce it in 60+ runs on macOS and in a 2-CPU linux/arm64 container; only the MTP runner — what CI uses — did.) |
The failure
MeshWeaver.Plugins main red on set 3.0.0-ci.9296 (core 8d3fd9f):
LayoutAreaIdentityTest.AuthorizedUser_CanSubscribe_ToLayoutArea(run 35985114586) andGitHubSyncSettingsTabTest.GitHubSyncTab_ShownOnSpace(run 35985209459) "timed out" although the test body passed andMesh.Dispose()completed clean. The CI trace showsDISPOSE_DONEand then noDISPOSE_UNLOADS_*line ever — the hang is in the test base'sServiceProvider.Dispose().The exact cause
Reproduced locally (1 in 12 full
MeshWeaver.Security.Testruns under the MTP runner) and captured withdotnet-stack:OwnedConnectionExtensions.Release(from fix(#5135): an owned connection's release terminates attached subscribers; one upload ceiling #5647 / Refs MCP upload: un-timed legs can hold a tools/call open for minutes, no per-call timeout, no /mcp body-size ceiling, base64 error hides truncation #5135), each delivering oneMeshNodeStreamCachequery connection's terminal, are deadlocked inside the permission fold: one holds aCombineLatestgate and waits inZip.Disposefor theZipgate; another holds thatZipgate (Zip.SecondObserver.OnError) and waits for theCombineLatestgate.AutofacServiceProvider.Dispose→UiContributionCatalog.Dispose→BehaviorSubject.OnCompleted→CombineLatestObserver.OnCompleted) parks on the same gate.#5647 scheduled each connection's release as its own
TaskPoolSchedulerwork item. An owner registry is released in ONE sweep (_queryConnections.Dispose()), so N connections produced N CONCURRENT terminals into consumers that compose several of them; Rx combinators dispose their sources under their gate on a terminal, so two concurrent terminals entering a nested Zip/CombineLatest from opposite ends invert the lock order.Not the #5635 hypothesis: no wait on
StreamEndedEventis involved.The fix
ReleaseLane(Messaging.Hub): releases posted to one lane run on the ThreadPool (still never on the disposing turn, #4530's trade kept) ONE AT A TIME, in order —Subject.Synchronize+ObserveOn(TaskPoolScheduler). The second terminal reaches a consumer the first already tore down.AccessService); every hub resolves it, so the hub overload ofAutoConnectOwnedByuses the mesh lane.MeshNodeStreamCachepasses the mesh lane for all query connections;PackageListingCache/GitHubRepoIdentityResolverhold one lane per registry.AutoConnectOwnedBy(source, owner, lane, ownerName, minObservers); the lane-lessCompositeDisposableoverload stays as an[Obsolete]forwarder (nothing removed; Plugins has no caller).Verification
OwnedConnectionTest.ReleasingTwoConnectionsAConsumerComposes_NeverDeliversTheirTerminalsConcurrentlyforces the two releases to meet inside their first gates. Negative control:Releaserestored to one TaskPool work item per connection → red (teardown never finishes, 36 s wait fails). With the lane → green. WholeOwnedConnectionTestclass 11/11.dotnet build -c Release -warnaserrorclean: Messaging.Hub.Test, Hosting, PluginCatalog (pulls GitSync), Documentation.Test; Documentation.Test guards 628/628.MeshWeaver.Security.Testbuilt against this branch and looped under MTP (results in the PR thread).Docs:
Architecture/HubDisposalModel→ "Those terminals are delivered one at a time".Nothing a per-node hub serves changes; no recycle needed. No i18n change. No public member removed; no interface member added.
🤖 Generated with Claude Code