fix: replace lock() with SemaphoreSlim in KeyManager and OPKManager async paths

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

Author: rbenzing

LibEmiddle.Tests.Unit/KeyManagementTests.cs

@@ -8,6 +8,7 @@
 using System;
 using System.Threading.Tasks;
 using System.Collections.Generic;
+using System.Linq;
 using Microsoft.VisualStudio.TestPlatform.ObjectModel.DataCollection;
 
 namespace LibEmiddle.Tests.Unit
@@ -230,6 +231,26 @@ public async Task KeyCache_ShouldBeThreadSafe()
             await _keyManager.DeleteKeyAsync(keyId);
         }
 
+        /// <summary>
+        /// Tests that concurrent store/retrieve operations do not deadlock with SemaphoreSlim
+        /// </summary>
+        [TestMethod]
+        public async Task KeyManager_ConcurrentCacheAccess_DoesNotDeadlock()
+        {
+            // 20 concurrent tasks each store and retrieve a unique key
+            var tasks = Enumerable.Range(0, 20).Select(i => Task.Run(async () =>
+            {
+                var keyId = $"concurrent-test-key-{i}";
+                var keyData = new byte[32];
+                System.Security.Cryptography.RandomNumberGenerator.Fill(keyData);
+                await _keyManager.StoreKeyAsync(keyId, keyData);
+                var retrieved = await _keyManager.RetrieveKeyAsync(keyId);
+                Assert.IsNotNull(retrieved, $"Key {keyId} should be retrievable after store");
+            }));
+
+            await Task.WhenAll(tasks).WaitAsync(TimeSpan.FromSeconds(10));
+        }
+
         #endregion
     }
 }

LibEmiddle/KeyManagement/KeyManager.cs

@@ -17,7 +17,7 @@ public class KeyManager : IKeyManager, IDisposable
         private readonly ConcurrentDictionary<string, CachedKey> _keyCache = new();
         private readonly TimeSpan _cacheExpiration = TimeSpan.FromMinutes(10);
         private readonly Timer _cacheCleanupTimer;
-        private readonly object _cacheLock = new object();
+        private readonly SemaphoreSlim _cacheLock = new SemaphoreSlim(1, 1);
         private volatile bool _disposed;
 
         /// <summary>
@@ -87,7 +87,7 @@ public async Task<bool> StoreKeyAsync(string keyId, byte[] key, string? password
                 // If storage was successful, update the cache
                 if (success)
                 {
-                    CacheKey(keyId, key);
+                    await CacheKeyAsync(keyId, key).ConfigureAwait(false);
                 }
 
                 return success;
@@ -108,7 +108,8 @@ public async Task<bool> StoreKeyAsync(string keyId, byte[] key, string? password
             try
             {
                 // Check the cache first with secure handling
-                lock (_cacheLock)
+                await _cacheLock.WaitAsync().ConfigureAwait(false);
+                try
                 {
                     if (_keyCache.TryGetValue(keyId, out CachedKey? cachedKey))
                     {
@@ -124,14 +125,18 @@ public async Task<bool> StoreKeyAsync(string keyId, byte[] key, string? password
                         }
                     }
                 }
+                finally
+                {
+                    _cacheLock.Release();
+                }
 
                 // Not in cache, retrieve from storage
                 byte[]? key = await _cryptoProvider.RetrieveKeyAsync(keyId, password);
 
                 // If key was found, update the cache
                 if (key != null)
                 {
-                    CacheKey(keyId, key);
+                    await CacheKeyAsync(keyId, key).ConfigureAwait(false);
                 }
 
                 if (key == null)
@@ -162,13 +167,18 @@ public async Task<bool> DeleteKeyAsync(string keyId, string? password = null)
             try
             {
                 // Remove from cache with secure disposal
-                lock (_cacheLock)
+                await _cacheLock.WaitAsync().ConfigureAwait(false);
+                try
                 {
                     if (_keyCache.TryRemove(keyId, out CachedKey? cachedKey))
                     {
                         cachedKey.Dispose();
                     }
                 }
+                finally
+                {
+                    _cacheLock.Release();
+                }
 
                 // Delete from storage
                 return await _cryptoProvider.DeleteKeyAsync(keyId);
@@ -314,9 +324,10 @@ public async Task<TimeSpan> GetTimeUntilRotationAsync(string keyId, TimeSpan rot
         /// </summary>
         /// <param name="keyId">The identifier for the key.</param>
         /// <param name="key">The key to cache.</param>
-        private void CacheKey(string keyId, byte[] key)
+        private async Task CacheKeyAsync(string keyId, byte[] key)
         {
-            lock (_cacheLock)
+            await _cacheLock.WaitAsync().ConfigureAwait(false);
+            try
             {
                 // Remove existing cached key if present
                 if (_keyCache.TryRemove(keyId, out CachedKey? existingKey))
@@ -327,6 +338,10 @@ private void CacheKey(string keyId, byte[] key)
                 // Store new cached key with expiration
                 _keyCache[keyId] = new CachedKey(key, _cacheExpiration);
             }
+            finally
+            {
+                _cacheLock.Release();
+            }
         }
 
         /// <summary>
@@ -336,7 +351,8 @@ private void CleanupCache(object? state)
         {
             try
             {
-                lock (_cacheLock)
+                _cacheLock.Wait();
+                try
                 {
                     var expiredKeys = new List<string>();
 
@@ -358,6 +374,10 @@ private void CleanupCache(object? state)
                         }
                     }
                 }
+                finally
+                {
+                    _cacheLock.Release();
+                }
             }
             catch (Exception ex)
             {
@@ -388,16 +408,22 @@ protected virtual void Dispose(bool disposing)
                 _cacheCleanupTimer.Dispose();
 
                 // Clear and dispose the key cache securely
-                lock (_cacheLock)
+                _cacheLock.Wait();
+                try
                 {
                     foreach (var cachedKey in _keyCache.Values)
                     {
                         cachedKey.Dispose();
                     }
                     _keyCache.Clear();
                 }
+                finally
+                {
+                    _cacheLock.Release();
+                }
 
-                // Dispose the key storage
+                // Dispose the semaphore and key storage
+                _cacheLock.Dispose();
                 _keyStorage.Dispose();
             }
 

LibEmiddle/KeyManagement/OPKManager.cs

@@ -32,7 +32,7 @@ public sealed class OPKManager : IDisposable
         // -----------------------------------------------------------------------
 
         private readonly string _storageFilePath;
-        private readonly object _lock = new object();
+        private readonly SemaphoreSlim _lock = new SemaphoreSlim(1, 1);
         private readonly HashSet<uint> _consumedIds;
 
         // Replenishment callback — kept as null until set; called on a background task.
@@ -86,10 +86,15 @@ public void SetReplenishmentCallback(Func<int, Task> callback)
         public bool IsConsumed(uint opkId)
         {
             ThrowIfDisposed();
-            lock (_lock)
+            _lock.Wait();
+            try
             {
                 return _consumedIds.Contains(opkId);
             }
+            finally
+            {
+                _lock.Release();
+            }
         }
 
         /// <summary>
@@ -112,14 +117,19 @@ public bool TryConsume(uint opkId, IReadOnlyList<uint>? allKnownIds = null)
             ThrowIfDisposed();
             bool added;
             int availableCount;
-            lock (_lock)
+            _lock.Wait();
+            try
             {
                 // HashSet.Add returns false when the element is already present.
                 // Both the check and the insertion happen inside one lock acquisition,
                 // so two concurrent arrivals with the same OPK ID cannot both succeed.
                 added = _consumedIds.Add(opkId);
                 availableCount = ComputeAvailableCount(allKnownIds);
             }
+            finally
+            {
+                _lock.Release();
+            }
 
             if (added)
             {
@@ -154,11 +164,16 @@ public void MarkConsumed(uint opkId, IReadOnlyList<uint>? allKnownIds = null)
             bool changed;
             int availableCount;
 
-            lock (_lock)
+            _lock.Wait();
+            try
             {
                 changed = _consumedIds.Add(opkId);
                 availableCount = ComputeAvailableCount(allKnownIds);
             }
+            finally
+            {
+                _lock.Release();
+            }
 
             if (changed)
             {
@@ -181,7 +196,8 @@ public IReadOnlyList<uint> FilterAvailable(IReadOnlyList<uint> allIds)
             ThrowIfDisposed();
             ArgumentNullException.ThrowIfNull(allIds);
 
-            lock (_lock)
+            _lock.Wait();
+            try
             {
                 var result = new List<uint>(allIds.Count);
                 foreach (uint id in allIds)
@@ -191,6 +207,10 @@ public IReadOnlyList<uint> FilterAvailable(IReadOnlyList<uint> allIds)
                 }
                 return result;
             }
+            finally
+            {
+                _lock.Release();
+            }
         }
 
         /// <summary>
@@ -200,10 +220,15 @@ public IReadOnlyList<uint> FilterAvailable(IReadOnlyList<uint> allIds)
         public int GetAvailableCount(IReadOnlyList<uint>? allKnownIds)
         {
             ThrowIfDisposed();
-            lock (_lock)
+            _lock.Wait();
+            try
             {
                 return ComputeAvailableCount(allKnownIds);
             }
+            finally
+            {
+                _lock.Release();
+            }
         }
 
         /// <summary>
@@ -214,10 +239,15 @@ public int ConsumedCount
             get
             {
                 ThrowIfDisposed();
-                lock (_lock)
+                _lock.Wait();
+                try
                 {
                     return _consumedIds.Count;
                 }
+                finally
+                {
+                    _lock.Release();
+                }
             }
         }
 
@@ -255,10 +285,15 @@ private void PersistToDisk()
             try
             {
                 List<uint> snapshot;
-                lock (_lock)
+                _lock.Wait();
+                try
                 {
                     snapshot = new List<uint>(_consumedIds);
                 }
+                finally
+                {
+                    _lock.Release();
+                }
 
                 string json = JsonSerializer.Serialize(snapshot);
                 // Write atomically via a temp file then replace.
@@ -337,10 +372,11 @@ private void ThrowIfDisposed()
         // IDisposable
         // -----------------------------------------------------------------------
 
-        /// <summary>Disposes the manager (no-op: no unmanaged resources).</summary>
+        /// <summary>Disposes the manager and releases resources.</summary>
         public void Dispose()
         {
             _disposed = true;
+            _lock.Dispose();
         }
     }
 }