fix(core): rollback per-index entry when ChannelRegistry id-uniqueness check loses race (fixes #268)#280
Merged
Conversation
This was referenced May 22, 2026
Kiryuumaru
added a commit
that referenced
this pull request
May 22, 2026
…to (channelId, index) pair (fixes #228) (#344) ChannelRegistry.UnregisterChannel called _idToIndex.TryRemove(channelId, out _) unconditionally. If a caller passed a (index, channelId) pair where _idToIndex[channelId] mapped to a *different* (legitimate) index — for example, while cleaning up a RegisterReadChannel call that failed on the per-id race — the legitimate channel's ID lookup was destroyed, leaving the legitimate channel reachable only by index. GetReadChannelById / GetWriteChannelById would return null for a channel that was still alive in the registry. The ghost-channel half of #228 was already fixed in #280 (rollback in RegisterRead/WriteChannel). Today both current call sites of UnregisterChannel pass matching pairs, so the bug is latent — but the suggested fix from #228 is a defensive hardening that prevents future regressions. Fix: remove via ConcurrentDictionary's KeyValuePair overload, which only removes if the value still matches the supplied index. Tests: - UnregisterChannel_MismatchedIndex_DoesNotRemoveLegitimateIdMapping — asserts a mismatched (index, channelId) call leaves the legitimate GetReadChannelById lookup intact. - UnregisterChannel_MatchingPair_RemovesIdMapping — asserts the normal path still removes the ID mapping. All 437 mux unit tests pass.
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.
Summary
Fixes #268 —
ChannelRegistry.RegisterWriteChannel/RegisterReadChannelleak the per-index entry into_writeChannels/_readChannelswhen the second-step_idToIndex.TryAddfails on ID collision. The throw exits with a half-registered channel: the caller observesMultiplexerException(ChannelExists)and discards their reference, but the registry retains the orphan at the allocated index.AllocateChannelIndexis monotonic and never recycles, so the index leaks too.Bug
Two-step insert with no rollback:
When two
OpenChannel("dup")calls race, both allocate distinct indices (4 and 6) and both insert into_writeChannelssuccessfully. Whichever loses on_idToIndex.TryAdd("dup", _)throws — and_writeChannels[loser_index]is now populated with a channel the caller no longer holds a reference to. Consequences:GetWriteChannel(loser_index)returns the orphan.GetAllWriteChannels()enumerates it (mux teardown does redundant work; events fire to nobody).MaxDataChannel, sustained races can exhaust it.Fix
Roll back the per-index insert on second-step failure. Mirror for both
RegisterWriteChannelandRegisterReadChannel. UseConcurrentDictionary.TryRemove(KeyValuePair)so we only remove the exact entry we just inserted (defensive against any concurrent rewrite of the same index by another caller, though the index allocator is monotonic so this is belt-and-suspenders).Regression Tests
Added
tests/NetConduit.UnitTests/ChannelRegistryTests.cswith two tests exercising both code paths directly through the registry's internal API (NetConduit.UnitTests hasInternalsVisibleTo):RegisterWriteChannel_IdCollision_RollsBackIndexEntryRegisterReadChannel_IdCollision_RollsBackIndexEntryEach test registers a channel at index N with ID
"dup", then attempts to register a second channel at index M (≠ N) with the same ID, asserts the throw, and assertsGetWriteChannel(M) == null(resp.GetReadChannel(M) == null).Pre-fix verification: stashed the production change, re-ran the two tests — both fail with
Assert.Null() Failure: Value is not nullreturning the orphan channel. Post-fix: both pass.Verification
dotnet build→ 0 warnings, 0 errors across net8/net9/net10dotnet test→ 1014 passed / 1 skipped (MemoryLeak_SubMuxChaos_MemoryStaysBounded), 3 known integration flakes confirmed passing in isolation (TcpMultiplexerFactoryLeakTests.CreateOptions_EndpointOverload_FactoryCancelledDuringConnect_DoesNotLeakHandles,WebSocketMultiplexerTests.CreateOptions_ConnectsAndTransfersData,IpcMultiplexerTests.MultipleChannels_TransferData)Files Changed
src/NetConduit/Internal/ChannelRegistry.cs— rollback per-index entry on id-collision throwtests/NetConduit.UnitTests/ChannelRegistryTests.cs— regression tests for both register paths