Refactor user secrets management to use DI-based factory pattern with thread-safe synchronization

src/Shared/SecretsStore.cs

@@ -16,20 +16,39 @@ namespace Microsoft.Extensions.SecretManager.Tools.Internal;
 /// </summary>
 internal sealed class SecretsStore
 {
+    // Static lock dictionary to synchronize access per userSecretsId
+    private static readonly Dictionary<string, SemaphoreSlim> s_locks = new();
+    private static readonly object s_locksLock = new();
+
     private readonly string _secretsFilePath;
     private readonly Dictionary<string, string?> _secrets;
+    private readonly string _userSecretsId;
 
     public SecretsStore(string userSecretsId)
     {
         ArgumentNullException.ThrowIfNull(userSecretsId);
 
+        _userSecretsId = userSecretsId;
         _secretsFilePath = PathHelper.GetSecretsPathFromSecretsId(userSecretsId);
 
         EnsureUserSecretsDirectory();
 
         _secrets = Load(_secretsFilePath);
     }
 
+    private static SemaphoreSlim GetLock(string userSecretsId)
+    {
+        lock (s_locksLock)
+        {
+            if (!s_locks.TryGetValue(userSecretsId, out var semaphore))
+            {
+                semaphore = new SemaphoreSlim(1, 1);
+                s_locks[userSecretsId] = semaphore;
+            }
+            return semaphore;
+        }
+    }
+
     public string? this[string key] => _secrets[key];
 
     public int Count => _secrets.Count;
@@ -49,39 +68,50 @@ public SecretsStore(string userSecretsId)
 
     public void Save()
     {
-        EnsureUserSecretsDirectory();
-
-        var contents = new JsonObject();
-        if (_secrets is not null)
+        var semaphore = GetLock(_userSecretsId);
+        semaphore.Wait();
+        try
         {
-            foreach (var secret in _secrets.AsEnumerable())
+            // Reload from disk to merge with any concurrent changes
+            var currentSecrets = Load(_secretsFilePath);
+
+            // Merge our changes with what's on disk
+            foreach (var kvp in _secrets)
+            {
+                currentSecrets[kvp.Key] = kvp.Value;
+            }
+
+            EnsureUserSecretsDirectory();
+
+            var contents = new JsonObject();
+            foreach (var secret in currentSecrets)
             {
                 contents[secret.Key] = secret.Value;
             }
-        }
 
-        // Create a temp file with the correct Unix file mode before moving it to the expected _filePath.
-        if (!OperatingSystem.IsWindows())
-        {
-            var tempFilename = Path.GetTempFileName();
-            File.Move(tempFilename, _secretsFilePath, overwrite: true);
-        }
+            // Create a temp file with the correct Unix file mode before moving it to the expected _filePath.
+            if (!OperatingSystem.IsWindows())
+            {
+                var tempFilename = Path.GetTempFileName();
+                File.Move(tempFilename, _secretsFilePath, overwrite: true);
+            }
 
-        var json = contents.ToJsonString(new()
-        {
-            WriteIndented = true
-        });
+            var json = contents.ToJsonString(new()
+            {
+                WriteIndented = true
+            });
 
-        File.WriteAllText(_secretsFilePath, json, Encoding.UTF8);
+            File.WriteAllText(_secretsFilePath, json, Encoding.UTF8);
+        }
+        finally
+        {
+            semaphore.Release();
+        }
     }
 
     private void EnsureUserSecretsDirectory()
     {
-        var directoryName = Path.GetDirectoryName(_secretsFilePath);
-        if (!string.IsNullOrEmpty(directoryName) && !Directory.Exists(directoryName))
-        {
-            Directory.CreateDirectory(directoryName);
-        }
+        EnsureUserSecretsDirectory(_secretsFilePath);
     }
 
     private static Dictionary<string, string?> Load(string secretsFilePath)
@@ -130,14 +160,55 @@ public static bool TrySetUserSecret(Assembly? assembly, string name, string valu
             // Save the value to the secret store
             try
             {
-                var secretsStore = new SecretsStore(userSecretsId);
-                secretsStore.Set(name, value);
-                secretsStore.Save();
-                return true;
+                var semaphore = GetLock(userSecretsId);
+                semaphore.Wait();
+                try
+                {
+                    // Load, set, and save in one atomic operation to ensure thread safety
+                    var secretsFilePath = PathHelper.GetSecretsPathFromSecretsId(userSecretsId);
+                    EnsureUserSecretsDirectory(secretsFilePath);
+
+                    var secrets = Load(secretsFilePath);
+                    secrets[name] = value;
+
+                    var contents = new JsonObject();
+                    foreach (var secret in secrets)
+                    {
+                        contents[secret.Key] = secret.Value;
+                    }
+
+                    // Create a temp file with the correct Unix file mode before moving it to the expected _filePath.
+                    if (!OperatingSystem.IsWindows())
+                    {
+                        var tempFilename = Path.GetTempFileName();
+                        File.Move(tempFilename, secretsFilePath, overwrite: true);
+                    }
+
+                    var json = contents.ToJsonString(new()
+                    {
+                        WriteIndented = true
+                    });
+
+                    File.WriteAllText(secretsFilePath, json, Encoding.UTF8);
+                    return true;
+                }
+                finally
+                {
+                    semaphore.Release();
+                }
             }
             catch (Exception) { } // Ignore user secret store errors
         }
 
         return false;
     }
+
+    private static void EnsureUserSecretsDirectory(string secretsFilePath)
+    {
+        var directoryName = Path.GetDirectoryName(secretsFilePath);
+        if (!string.IsNullOrEmpty(directoryName) && !Directory.Exists(directoryName))
+        {
+            Directory.CreateDirectory(directoryName);
+        }
+    }
 }

tests/Aspire.Hosting.Tests/UserSecretsParameterDefaultTests.cs

@@ -71,6 +71,107 @@ public void UserSecretsParameterDefault_GetDefaultValue_DoesntThrowIfSecretsFile
         var _ = userSecretDefault.GetDefaultValue();
     }
 
+    [Fact]
+    public async Task TrySetUserSecret_ConcurrentWrites_PreservesAllSecrets()
+    {
+        var userSecretsId = Guid.NewGuid().ToString("N");
+        ClearUsersSecrets(userSecretsId);
+
+        var testAssembly = AssemblyBuilder.DefineDynamicAssembly(
+            new("TestAssembly"), AssemblyBuilderAccess.RunAndCollect, [new CustomAttributeBuilder(s_userSecretsIdAttrCtor, [userSecretsId])]);
+
+        // Simulate concurrent writes from multiple threads (like SQL Server and RabbitMQ generating passwords)
+        var tasks = new List<Task<bool>>();
+        var secretsToWrite = new Dictionary<string, string>
+        {
+            ["Parameters:sqlserver-password"] = "SqlPassword123!",
+            ["Parameters:rabbitmq-password"] = "RabbitPassword456!",
+            ["Parameters:redis-password"] = "RedisPassword789!",
+            ["Parameters:postgres-password"] = "PostgresPassword012!",
+        };
+
+        foreach (var kvp in secretsToWrite)
+        {
+            var key = kvp.Key;
+            var value = kvp.Value;
+            tasks.Add(Task.Run(() => SecretsStore.TrySetUserSecret(testAssembly, key, value)));
+        }
+
+        var results = await Task.WhenAll(tasks);
+
+        // All writes should succeed
+        Assert.All(results, Assert.True);
+
+        // All secrets should be preserved
+        var userSecrets = GetUserSecrets(userSecretsId);
+        foreach (var kvp in secretsToWrite)
+        {
+            Assert.True(userSecrets.ContainsKey(kvp.Key), $"Secret '{kvp.Key}' was not found in user secrets");
+            Assert.Equal(kvp.Value, userSecrets[kvp.Key]);
+        }
+
+        DeleteUserSecretsFile(userSecretsId);
+    }
+
+    [Fact]
+    public async Task TrySetUserSecret_SqlServerAndRabbitMQ_BothSecretsPreserved()
+    {
+        // This test specifically reproduces the issue described in the bug report
+        var userSecretsId = Guid.NewGuid().ToString("N");
+        ClearUsersSecrets(userSecretsId);
+
+        var testAssembly = AssemblyBuilder.DefineDynamicAssembly(
+            new("TestAssembly"), AssemblyBuilderAccess.RunAndCollect, [new CustomAttributeBuilder(s_userSecretsIdAttrCtor, [userSecretsId])]);
+
+        // Simulate SQL Server and RabbitMQ generating passwords concurrently
+        var sqlTask = Task.Run(() => SecretsStore.TrySetUserSecret(testAssembly, "Parameters:sql-password", "SqlPassword123!"));
+        var rabbitTask = Task.Run(() => SecretsStore.TrySetUserSecret(testAssembly, "Parameters:rabbit-password", "RabbitPassword456!"));
+
+        var results = await Task.WhenAll(sqlTask, rabbitTask);
+
+        // Both writes should succeed
+        Assert.All(results, Assert.True);
+
+        // Both secrets should be in the file
+        var userSecrets = GetUserSecrets(userSecretsId);
+        Assert.True(userSecrets.ContainsKey("Parameters:sql-password"), "SQL Server password was not found");
+        Assert.True(userSecrets.ContainsKey("Parameters:rabbit-password"), "RabbitMQ password was not found");
+        Assert.Equal("SqlPassword123!", userSecrets["Parameters:sql-password"]);
+        Assert.Equal("RabbitPassword456!", userSecrets["Parameters:rabbit-password"]);
+
+        DeleteUserSecretsFile(userSecretsId);
+    }
+
+    [Fact]
+    public async Task TrySetUserSecret_ConcurrentWritesSameKey_LastWriteWins()
+    {
+        var userSecretsId = Guid.NewGuid().ToString("N");
+        ClearUsersSecrets(userSecretsId);
+
+        var testAssembly = AssemblyBuilder.DefineDynamicAssembly(
+            new("TestAssembly"), AssemblyBuilderAccess.RunAndCollect, [new CustomAttributeBuilder(s_userSecretsIdAttrCtor, [userSecretsId])]);
+
+        // Simulate concurrent writes to the same key
+        var tasks = new List<Task<bool>>();
+        for (int i = 0; i < 10; i++)
+        {
+            var value = $"Value{i}";
+            tasks.Add(Task.Run(() => SecretsStore.TrySetUserSecret(testAssembly, "Parameters:test-key", value)));
+        }
+
+        var results = await Task.WhenAll(tasks);
+
+        // All writes should succeed
+        Assert.All(results, Assert.True);
+
+        // The key should exist with one of the values
+        var userSecrets = GetUserSecrets(userSecretsId);
+        Assert.True(userSecrets.ContainsKey("Parameters:test-key"));
+        Assert.NotNull(userSecrets["Parameters:test-key"]);
+
+        DeleteUserSecretsFile(userSecretsId);
+    }
+
     private static void EnsureUserSecretsDirectory(string secretsFilePath)
     {
         var directoryName = Path.GetDirectoryName(secretsFilePath);