Refactor user secrets to DI-based factory with thread-safe synchronization #12692

Copilot · 2025-11-05T01:47:19Z

Apply changes from PR #12607 to main

This PR applies the user secrets refactoring from PR #12607 to the main branch. The refactoring moves from static methods to a dependency injection-based factory pattern with proper thread-safe synchronization.

Changes implemented:

Architecture:

Factory: Singleton instance registered in DI with internal constructor for test isolation
Semaphores: Each UserSecretsManager instance has its own semaphore (not static)
Thread Safety: Factory cache ensures same instance for same file path
Test Isolation: Tests create their own factory instances

Test Coverage:

The factory-based constructor overload in UserSecretsParameterDefault is tested through:

UserSecretsParameterDefault_WithCustomFactory_UsesProvidedFactory - Verifies custom factory usage
UserSecretsParameterDefault_WithCustomFactory_IsolatesFromGlobalInstance - Tests isolation between custom and singleton instances
UserSecretsParameterDefault_WithCustomFactory_ConcurrentAccess - Validates thread safety with custom factory

Validation:

✅ Aspire.Hosting project builds successfully
✅ Aspire.Hosting.Tests project builds successfully
✅ All code changes compile without errors or warnings

The implementation successfully replicates the changes from PR #12607, providing a thread-safe, DI-based approach to managing user secrets with proper synchronization to prevent concurrent access issues.

Original prompt

Take the changes from #12607 and make it on main

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: davidfowl <[email protected]>

github-actions · 2025-11-05T02:48:32Z

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12692

Or

Run remotely in PowerShell:

iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12692"

eerhardt · 2025-11-05T02:52:31Z

src/Aspire.Hosting/Aspire.Hosting.csproj

    <Compile Include="$(SharedDir)LoggingHelpers.cs" Link="Utils\LoggingHelpers.cs" />
    <Compile Include="$(SharedDir)StringUtils.cs" Link="Utils\StringUtils.cs" />
    <Compile Include="$(SharedDir)SchemaUtils.cs" Link="Utils\SchemaUtils.cs" />
-    <Compile Include="$(SharedDir)SecretsStore.cs" Link="Utils\SecretsStore.cs" />


Can the file be deleted now? This appears to be the only project that uses it.

@copilot delete this file.

Done in commit 81d3417. The file has been deleted as it's no longer used after the refactoring.

eerhardt · 2025-11-05T02:54:35Z

src/Aspire.Hosting/ApplicationModel/UserSecretsParameterDefault.cs

+    /// <summary>
+    /// Initializes a new instance of the <see cref="UserSecretsParameterDefault"/> class using the default factory.
+    /// </summary>
+    public UserSecretsParameterDefault(Assembly appHostAssembly, string applicationName, string parameterName, ParameterDefault parameterDefault)


UserSecretsParameterDefault

Who uses the ctor that takes a factory?

@copilot write tests that use this overload.

The constructor taking a factory is used only by tests to enable test isolation. Tests create their own UserSecretsManagerFactory instances to avoid cross-contamination between test runs. Production code uses the parameterless constructor which delegates to the factory-based constructor with UserSecretsManagerFactory.Instance.

Added 3 tests in commit 50647b4 that specifically use the constructor overload with factory parameter:

UserSecretsParameterDefault_WithCustomFactory_UsesProvidedFactory - Verifies the custom factory is used

UserSecretsParameterDefault_WithCustomFactory_IsolatesFromGlobalInstance - Tests isolation between custom factory and singleton instance

UserSecretsParameterDefault_WithCustomFactory_ConcurrentAccess - Validates thread safety with custom factory under concurrent access

eerhardt

Copilot

Pull Request Overview

This PR refactors user secrets management in Aspire by introducing a new abstraction layer (IUserSecretsManager) and factory pattern (UserSecretsManagerFactory) to replace direct usage of SecretsStore from Microsoft.Extensions.SecretManager.Tools.Internal. The main goals are:

Thread-safe concurrent writes: Uses a SemaphoreSlim to synchronize access to user secrets files, preventing race conditions when multiple resources (like SQL Server and RabbitMQ) generate passwords concurrently
Centralized management: Factory pattern ensures singleton instances per secrets file path, avoiding duplicate managers
Improved testability: New abstraction allows for easier testing and mocking
Code organization: Extracted JSON flattening logic into a separate JsonFlattener utility class

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`src/Aspire.Hosting/UserSecrets/IUserSecretsManager.cs`	New interface defining the contract for user secrets management
`src/Aspire.Hosting/UserSecrets/UserSecretsManagerFactory.cs`	Factory for creating and caching thread-safe user secrets manager instances
`src/Aspire.Hosting/UserSecrets/NoopUserSecretsManager.cs`	No-op implementation for scenarios without user secrets configured
`src/Aspire.Hosting/Pipelines/Internal/JsonFlattener.cs`	Extracted JSON flattening utility methods
`src/Aspire.Hosting/Pipelines/Internal/UserSecretsDeploymentStateManager.cs`	Updated to use `IUserSecretsManager` instead of direct file operations
`src/Aspire.Hosting/Pipelines/Internal/FileDeploymentStateManager.cs`	Updated to use `JsonFlattener` utility
`src/Aspire.Hosting/Pipelines/Internal/DeploymentStateManagerBase.cs`	Removed JSON flattening methods (moved to `JsonFlattener`)
`src/Aspire.Hosting/VersionChecking/VersionCheckService.cs`	Updated to use `IUserSecretsManager` instead of `SecretsStore`
`src/Aspire.Hosting/DistributedApplicationBuilder.cs`	Registers `IUserSecretsManager` in DI container and uses it for API key generation
`src/Aspire.Hosting/ApplicationModel/UserSecretsParameterDefault.cs`	Updated to use factory pattern for user secrets management
`src/Aspire.Hosting/Aspire.Hosting.csproj`	Removed reference to shared `SecretsStore.cs`
`tests/Aspire.Hosting.Tests/UserSecretsParameterDefaultTests.cs`	Added comprehensive concurrency tests; updated helper methods
`tests/Aspire.Hosting.Tests/SecretsStoreTests.cs`	Updated to use new user secrets manager APIs
`tests/Aspire.Hosting.Tests/JsonFlattenerTests.cs`	Renamed from `UserSecretsDeploymentStateManagerTests`; updated to test `JsonFlattener`
`tests/Aspire.Hosting.Tests/VersionChecking/VersionCheckServiceTests.cs`	Updated test helper to accept `IUserSecretsManager` parameter

Copilot · 2025-11-05T02:58:25Z

src/Aspire.Hosting/UserSecrets/IUserSecretsManager.cs

+namespace Aspire.Hosting.UserSecrets;
+
+/// <summary>
+/// Manages user secrets for an application, providing thread-safe read and write operations.


The summary states "providing thread-safe read and write operations" but the interface itself doesn't guarantee thread-safety - this is an implementation detail of UserSecretsManager. Consider rephrasing to "Interface for managing user secrets with support for read and write operations" to avoid making guarantees about implementation details in the interface documentation.

Suggested change

/// Manages user secrets for an application, providing thread-safe read and write operations.

/// Defines an interface for managing user secrets with support for read and write operations.

Copilot · 2025-11-05T02:58:25Z

src/Aspire.Hosting/UserSecrets/UserSecretsManagerFactory.cs

+/// Factory for creating and caching IUserSecretsManager instances.
+/// Uses a lock to ensure thread-safe creation and a dictionary to cache instances by normalized file path.
+/// </summary>


The summary mentions implementation details (lock, dictionary) that should be in <remarks> instead. The <summary> should be brief and focus on the purpose. Suggest: "Factory for creating and caching IUserSecretsManager instances." with additional details moved to a <remarks> section.

Suggested change

/// Factory for creating and caching IUserSecretsManager instances.

/// Uses a lock to ensure thread-safe creation and a dictionary to cache instances by normalized file path.

/// </summary>

/// Factory for creating and caching <see cref="IUserSecretsManager"/> instances.

/// </summary>

/// <remarks>

/// Uses a lock to ensure thread-safe creation and a dictionary to cache instances by normalized file path.

/// </remarks>

Copilot · 2025-11-05T02:58:26Z

src/Aspire.Hosting/UserSecrets/NoopUserSecretsManager.cs

+/// A no-op implementation of <see cref="IUserSecretsManager"/> used when
+/// user secrets are not configured for a project.


The summary should be a single sentence or short paragraph. The text should flow as one sentence without line break. Suggest: "A no-op implementation of used when user secrets are not configured for a project."

Suggested change

/// A no-op implementation of <see cref="IUserSecretsManager"/> used when

/// user secrets are not configured for a project.

/// A no-op implementation of <see cref="IUserSecretsManager"/> used when user secrets are not configured for a project.

Copilot · 2025-11-05T02:58:26Z

src/Aspire.Hosting/Pipelines/Internal/UserSecretsDeploymentStateManager.cs

+    /// <summary>
+    /// Initializes a new instance of the <see cref="UserSecretsDeploymentStateManager"/> class.
+    /// </summary>
+    /// <param name="logger">The logger.</param>
+    /// <param name="userSecretsManager">User secrets manager for managing secrets.</param>


Per Aspire coding guidelines, constructors for internal classes should have minimal documentation. This verbose documentation with full summary and parameter descriptions is unnecessary for an internal class. Simplify to just a brief <summary> tag.

Copilot · 2025-11-05T02:58:26Z

src/Aspire.Hosting/ApplicationModel/UserSecretsParameterDefault.cs

+    /// <summary>
+    /// Initializes a new instance of the <see cref="UserSecretsParameterDefault"/> class using the default factory.
+    /// </summary>


Per Aspire coding guidelines, internal classes should have minimal documentation. This verbose constructor documentation is unnecessary for an internal class constructor. A brief <summary> would suffice, or it could be omitted entirely.

Co-authored-by: davidfowl <[email protected]>

…rameter Co-authored-by: davidfowl <[email protected]>

davidfowl · 2025-11-05T04:12:07Z

/backport to release/13.0

github-actions · 2025-11-05T04:12:16Z

Started backporting to release/13.0: https://github.com/dotnet/aspire/actions/runs/19091023335

github-actions · 2025-11-05T04:12:22Z

@davidfowl backporting to "release/13.0" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Creating an empty commit: Initial plan
Applying: Implement user secrets refactoring with DI-based factory pattern
.git/rebase-apply/patch:59: trailing whitespace.
        
.git/rebase-apply/patch:113: trailing whitespace.
        
.git/rebase-apply/patch:1114: trailing whitespace.
        
.git/rebase-apply/patch:1125: trailing whitespace.
                    
.git/rebase-apply/patch:1161: new blank line at EOF.
+
warning: 5 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	src/Aspire.Hosting/DistributedApplicationBuilder.cs
A	src/Aspire.Hosting/Pipelines/Internal/DeploymentStateManagerBase.cs
A	src/Aspire.Hosting/Pipelines/Internal/FileDeploymentStateManager.cs
A	src/Aspire.Hosting/Pipelines/Internal/UserSecretsDeploymentStateManager.cs
A	tests/Aspire.Hosting.Tests/Publishing/DefaultUserSecretsManagerTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Aspire.Hosting/DistributedApplicationBuilder.cs
Auto-merging src/Aspire.Hosting/Publishing/Internal/DeploymentStateManagerBase.cs
Auto-merging src/Aspire.Hosting/Publishing/Internal/FileDeploymentStateManager.cs
Auto-merging src/Aspire.Hosting/Publishing/Internal/UserSecretsDeploymentStateManager.cs
CONFLICT (content): Merge conflict in src/Aspire.Hosting/Publishing/Internal/UserSecretsDeploymentStateManager.cs
Auto-merging tests/Aspire.Hosting.Tests/Publishing/DefaultUserSecretsManagerTests.cs
CONFLICT (rename/rename): tests/Aspire.Hosting.Tests/Publishing/DefaultUserSecretsManagerTests.cs renamed to tests/Aspire.Hosting.Azure.Tests/DefaultUserSecretsManagerTests.cs in HEAD and to tests/Aspire.Hosting.Tests/JsonFlattenerTests.cs in Implement user secrets refactoring with DI-based factory pattern.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0002 Implement user secrets refactoring with DI-based factory pattern
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Initial plan

e8712f7

Copilot AI assigned Copilot and davidfowl Nov 5, 2025

Copilot started work on behalf of davidfowl November 5, 2025 01:47 View session

Copilot AI and others added 2 commits November 5, 2025 02:02

Implement user secrets refactoring with DI-based factory pattern

4d2cfac

Co-authored-by: davidfowl <[email protected]>

Fix missing namespace in UserSecretsParameterDefaultTests

b5cd119

Co-authored-by: davidfowl <[email protected]>

Copilot AI changed the title ~~[WIP] Apply changes from PR 12607 to main branch~~ Refactor user secrets to DI-based factory with thread-safe synchronization Nov 5, 2025

Copilot AI requested a review from davidfowl November 5, 2025 02:15

Copilot finished work on behalf of davidfowl November 5, 2025 02:15

davidfowl marked this pull request as ready for review November 5, 2025 02:48

davidfowl requested a review from mitchdenny as a code owner November 5, 2025 02:48

Copilot AI review requested due to automatic review settings November 5, 2025 02:48

eerhardt reviewed Nov 5, 2025

View reviewed changes

eerhardt approved these changes Nov 5, 2025

View reviewed changes

Copilot AI reviewed Nov 5, 2025

View reviewed changes

Copilot started work on behalf of davidfowl November 5, 2025 03:01 View session

Address PR feedback: delete SecretsStore.cs and simplify documentation

81d3417

Co-authored-by: davidfowl <[email protected]>

Copilot finished work on behalf of davidfowl November 5, 2025 03:06

Copilot started work on behalf of davidfowl November 5, 2025 03:07 View session

Add tests for UserSecretsParameterDefault constructor with factory pa…

50647b4

…rameter Co-authored-by: davidfowl <[email protected]>

Copilot finished work on behalf of davidfowl November 5, 2025 03:16

davidfowl merged commit 231cc50 into main Nov 5, 2025
447 of 538 checks passed

davidfowl deleted the copilot/apply-changes-from-pr-12607 branch November 5, 2025 04:11

dotnet-policy-service bot added this to the 13.1 milestone Nov 5, 2025

Copilot AI mentioned this pull request Nov 5, 2025

Refactor user secrets to DI-based factory with thread-safe synchronization #12695

Draft

	/// Manages user secrets for an application, providing thread-safe read and write operations.
	/// Defines an interface for managing user secrets with support for read and write operations.

		/// A no-op implementation of <see cref="IUserSecretsManager"/> used when
		/// user secrets are not configured for a project.

Refactor user secrets to DI-based factory with thread-safe synchronization #12692

Refactor user secrets to DI-based factory with thread-safe synchronization #12692

Conversation

Copilot AI commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Apply changes from PR #12607 to main

Changes implemented:

Architecture:

Test Coverage:

Validation:

Uh oh!

github-actions bot commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerhardt Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

davidfowl Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

eerhardt Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

davidfowl Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidfowl commented Nov 5, 2025

Uh oh!

github-actions bot commented Nov 5, 2025

Uh oh!

github-actions bot commented Nov 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Nov 5, 2025 •

edited

Loading

github-actions bot commented Nov 5, 2025 •

edited

Loading