fix: replace lock() with SemaphoreSlim in ChatSession, SessionManager bundle lock, and GroupSession

- ChatSession: introduce _historyLock (SemaphoreSlim) to replace lock(_messageHistory) in GetMessageHistory, GetMessageCount, and ClearMessageHistoryInternal; remove incorrect lock(_sessionLock) in SetMetadata since Metadata is already ConcurrentDictionary; dispose _historyLock in Dispose()
- SessionManager: change _localBundleLock from object to SemaphoreSlim(1,1); replace all four lock(_localBundleLock) blocks with Wait()/WaitAsync()+Release() patterns; use await WaitAsync in ReplenishOPKsAsync (async method); dispose _localBundleLock in Dispose()
- GroupSession: change _seenMessageIds value type from HashSet<string> to ConcurrentDictionary<string, byte> (concurrent set); remove lock(senderMessageIds) blocks in ValidateGroupMessage and RecordMessageSeen; use TryAdd/ContainsKey for thread-safe check-and-add
- Add ChatSession_ConcurrentHistoryAccess_DoesNotDeadlock test

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: rbenzing

LibEmiddle.Tests.Unit/ChatSessionTests.cs

@@ -1,5 +1,6 @@
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 using System;
+using System.Collections.Generic;
 using System.Linq;
 using System.Threading.Tasks;
 using LibEmiddle.Core;
@@ -403,6 +404,17 @@ public async Task ChatSession_Dispose_ShouldTerminateSession()
             await Assert.ThrowsExceptionAsync<ObjectDisposedException>(() => _aliceChatSession.ActivateAsync());
         }
 
+        [TestMethod]
+        public async Task ChatSession_ConcurrentHistoryAccess_DoesNotDeadlock()
+        {
+            var tasks = Enumerable.Range(0, 10).Select(_ => Task.Run(() =>
+            {
+                var history = _aliceChatSession.GetMessageHistory(10, 0);
+                Assert.IsNotNull(history);
+            }));
+            await Task.WhenAll(tasks).WaitAsync(TimeSpan.FromSeconds(5));
+        }
+
         [TestMethod]
         public async Task ChatSession_MessagePagination_ShouldReturnCorrectSubset()
         {

LibEmiddle/Messaging/Chat/ChatSession.cs

@@ -45,6 +45,7 @@ public class ChatSession : IChatSession, ISession, IDisposable
 
         // Collections
         private readonly List<MessageRecord> _messageHistory = new();
+        private readonly SemaphoreSlim _historyLock = new(1, 1);
         private const int MaxTrackedMessages = 100;
         public ConcurrentDictionary<string, string> Metadata { get; } = new();
 
@@ -499,20 +500,30 @@ public bool IsValid()
         public IReadOnlyCollection<MessageRecord> GetMessageHistory(int limit = 100, int startIndex = 0)
         {
             ThrowIfDisposed();
-            lock (_messageHistory)
+            _historyLock.Wait();
+            try
             {
                 return _messageHistory.Skip(Math.Max(0, startIndex)).Take(Math.Max(0, limit)).ToList().AsReadOnly();
             }
+            finally
+            {
+                _historyLock.Release();
+            }
         }
 
         /// <summary> Gets the count of messages in the history. </summary>
         public int GetMessageCount()
         {
             ThrowIfDisposed();
-            lock (_messageHistory)
+            _historyLock.Wait();
+            try
             {
                 return _messageHistory.Count;
             }
+            finally
+            {
+                _historyLock.Release();
+            }
         }
 
         /// <summary> Clears the message history, zeroing plaintext content before removal. </summary>
@@ -524,7 +535,8 @@ public int ClearMessageHistory()
 
         private int ClearMessageHistoryInternal()
         {
-            lock (_messageHistory)
+            _historyLock.Wait();
+            try
             {
                 int count = _messageHistory.Count;
                 foreach (var record in _messageHistory)
@@ -534,6 +546,10 @@ private int ClearMessageHistoryInternal()
                 _messageHistory.Clear();
                 return count;
             }
+            finally
+            {
+                _historyLock.Release();
+            }
         }
 
         /// <summary>
@@ -601,11 +617,8 @@ public void SetMetadata(string key, string value)
         {
             ThrowIfDisposed();
             if (string.IsNullOrEmpty(key)) throw new ArgumentException("Metadata key cannot be null or empty.", nameof(key));
-            // Dictionary is not thread-safe by default, lock if concurrent access is possible
-            lock (_sessionLock) // Or use ConcurrentDictionary for Metadata
-            {
-                Metadata[key] = value;
-            }
+            // Metadata is a ConcurrentDictionary — writes are already thread-safe without locking.
+            Metadata[key] = value;
         }
 
         /// <summary> Raises the StateChanged event. </summary>
@@ -890,7 +903,17 @@ protected virtual void Dispose(bool disposing)
                 }
                 finally
                 {
-                    // Dispose the semaphore outside of any lock usage
+                    // Dispose the history semaphore
+                    try
+                    {
+                        _historyLock.Dispose();
+                    }
+                    catch (ObjectDisposedException)
+                    {
+                        // Already disposed
+                    }
+
+                    // Dispose the session semaphore outside of any lock usage
                     try
                     {
                         lockToDispose?.Dispose();

LibEmiddle/Messaging/Group/GroupSession.Validation.cs

@@ -38,17 +38,14 @@ private bool ValidateGroupMessage(EncryptedGroupMessage message)
         if (!string.IsNullOrEmpty(message.MessageId))
         {
             string senderId = GetMemberId(message.SenderIdentityKey!);
-            var senderMessageIds = _seenMessageIds.GetOrAdd(senderId, _ => new HashSet<string>());
+            var senderMessageIds = _seenMessageIds.GetOrAdd(senderId, _ => new ConcurrentDictionary<string, byte>());
 
-            lock (senderMessageIds)
+            if (senderMessageIds.ContainsKey(message.MessageId))
             {
-                if (senderMessageIds.Contains(message.MessageId))
-                {
-                    LoggingManager.LogSecurityEvent(nameof(GroupSession), "Message ID replay detected", isAlert: true);
-                    return false;
-                }
-                // NOTE: Do NOT add to senderMessageIds here — that happens after successful decryption
+                LoggingManager.LogSecurityEvent(nameof(GroupSession), "Message ID replay detected", isAlert: true);
+                return false;
             }
+            // NOTE: Do NOT add to senderMessageIds here — that happens after successful decryption
         }
 
         // When the message carries a MessageId the _seenMessageIds check above provides complete replay
@@ -118,19 +115,16 @@ private void RecordMessageSeen(EncryptedGroupMessage message)
         // Register the message ID so subsequent presentations of the same message are rejected
         if (!string.IsNullOrEmpty(message.MessageId))
         {
-            var senderMessageIds = _seenMessageIds.GetOrAdd(senderId, _ => new HashSet<string>());
-            lock (senderMessageIds)
-            {
-                senderMessageIds.Add(message.MessageId);
+            var senderMessageIds = _seenMessageIds.GetOrAdd(senderId, _ => new ConcurrentDictionary<string, byte>());
+            senderMessageIds.TryAdd(message.MessageId, 0);
 
-                // Cap the set at 1000 entries per sender to prevent unbounded memory growth
-                if (senderMessageIds.Count > 1000)
+            // Cap the set at 1000 entries per sender to prevent unbounded memory growth
+            if (senderMessageIds.Count > 1000)
+            {
+                var oldestIds = senderMessageIds.Keys.Take(senderMessageIds.Count - 1000).ToList();
+                foreach (var oldId in oldestIds)
                 {
-                    var oldestIds = senderMessageIds.Take(senderMessageIds.Count - 1000).ToList();
-                    foreach (var oldId in oldestIds)
-                    {
-                        senderMessageIds.Remove(oldId);
-                    }
+                    senderMessageIds.TryRemove(oldId, out _);
                 }
             }
         }

LibEmiddle/Messaging/Group/GroupSession.cs

@@ -36,7 +36,7 @@ public sealed partial class GroupSession : IGroupSession, ISession, IDisposable
     // Message tracking for replay protection
     private readonly ConcurrentDictionary<string, long> _lastSeenSequence = new();
     private readonly ConcurrentDictionary<string, long> _joinTimestamps = new();
-    private readonly ConcurrentDictionary<string, HashSet<string>> _seenMessageIds = new();
+    private readonly ConcurrentDictionary<string, ConcurrentDictionary<string, byte>> _seenMessageIds = new();
 
     // Enhanced Group Management
     private readonly ConcurrentDictionary<string, GroupInvitation> _activeInvitations = new();

LibEmiddle/Sessions/SessionManager.cs

@@ -55,7 +55,7 @@ public class SessionManager(
         /// the existing OPK count. Updated via <see cref="RegisterLocalKeyBundleAsync"/>.
         /// </summary>
         private X3DHKeyBundle? _localKeyBundle;
-        private readonly object _localBundleLock = new object();
+        private readonly SemaphoreSlim _localBundleLock = new(1, 1);
 
         private readonly ConcurrentDictionary<string, ISession> _activeSessions = new();
 
@@ -643,10 +643,15 @@ public Task RegisterLocalKeyBundleAsync(X3DHKeyBundle bundle)
         {
             ArgumentNullException.ThrowIfNull(bundle);
 
-            lock (_localBundleLock)
+            _localBundleLock.Wait();
+            try
             {
                 _localKeyBundle = bundle;
             }
+            finally
+            {
+                _localBundleLock.Release();
+            }
 
             // Configure the replenishment callback now that we have a bundle reference.
             _opkManager.SetReplenishmentCallback(async count =>
@@ -664,10 +669,15 @@ public Task RegisterLocalKeyBundleAsync(X3DHKeyBundle bundle)
         public IReadOnlyList<uint> GetAvailableOPKIds()
         {
             X3DHKeyBundle? bundle;
-            lock (_localBundleLock)
+            _localBundleLock.Wait();
+            try
             {
                 bundle = _localKeyBundle;
             }
+            finally
+            {
+                _localBundleLock.Release();
+            }
 
             if (bundle == null)
                 return Array.Empty<uint>();
@@ -682,10 +692,15 @@ public IReadOnlyList<uint> GetAvailableOPKIds()
         private async Task ReplenishOPKsAsync(int count)
         {
             X3DHKeyBundle? bundle;
-            lock (_localBundleLock)
+            await _localBundleLock.WaitAsync().ConfigureAwait(false);
+            try
             {
                 bundle = _localKeyBundle;
             }
+            finally
+            {
+                _localBundleLock.Release();
+            }
 
             if (bundle == null)
             {
@@ -699,7 +714,8 @@ private async Task ReplenishOPKsAsync(int count)
                 var tempBundle = await _x3dhProtocol.CreateKeyBundleAsync(_identityKeyPair, count);
 
                 // Copy the new OPKs (public + private) into the existing bundle.
-                lock (_localBundleLock)
+                await _localBundleLock.WaitAsync().ConfigureAwait(false);
+                try
                 {
                     for (int i = 0; i < tempBundle.OneTimePreKeyIds.Count; i++)
                     {
@@ -718,6 +734,10 @@ private async Task ReplenishOPKsAsync(int count)
                         }
                     }
                 }
+                finally
+                {
+                    _localBundleLock.Release();
+                }
 
                 int available = _opkManager.GetAvailableCount(bundle.OneTimePreKeyIds);
                 LoggingManager.LogInformation("SessionManager",
@@ -866,6 +886,7 @@ protected virtual void Dispose(bool disposing)
             if (disposing)
             {
                 _operationLock.Dispose();
+                _localBundleLock.Dispose();
 
                 // Dispose all active sessions
                 foreach (var session in _activeSessions.Values)