Fix dev containers to work with worktrees by mdaigle · Pull Request #4137 · dotnet/SqlClient

mdaigle · 2026-04-04T01:29:25Z

Description

Provide a summary of the changes being introduced. Important topics to cover
include:

Description of the functionality.
API changes, backwards compatibility, deprecations, etc.
Documentation, localization.
Bug fixes.
Code hygiene, refactoring, improvements.
Engineering processes (CI, pipelines, test coverage)

High quality descriptions will lead to a smoother review experience.

Issues

Link to any relevant issues, bugs, or discussions (e.g., Closes #123, Fixes issue #456).

Testing

Describe the automated tests (unit, integration) you created or modified.
Provide justification for any gap in automated testing. List any manual testing
steps that were performed to ensure the changes work.

Guidelines

Please review the contribution guidelines before submitting a pull request:

Copilot

Pull request overview

This PR introduces a devcontainer-based development workflow (including SQL Server bootstrap) and expands/adjusts connection-pooling internals to better expose transacted-pool state for tests, alongside new unit tests for WaitHandleDbConnectionPool transaction behavior.

Changes:

Add .devcontainer/ scripts and configuration to provision SQL Server, generate test config, and support git worktrees.
Extend connection-pool interfaces/implementations to expose TransactedConnectionPool and adjust async connection acquisition plumbing.
Add/adjust test infrastructure and GitHub prompt docs (tool scoping + prompt renames/paths).

Reviewed changes

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

Show a summary per file

File	Description
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolTransactionTest.cs	New deterministic transaction/pooling unit tests for WaitHandleDbConnectionPool
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/TransactedConnectionPoolTest.cs	Update mock pool to satisfy new `IDbConnectionPool` surface
src/Microsoft.Data.SqlClient/tests/StressTests/SqlClient.Stress.Tests/SqlClientTestGroup.cs	Adds `System.Transactions` using (currently unused)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs	Expose pool sizing properties internally; expose `TransactedConnectionPool`; clarify transacted-pool routing comment
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/TransactedConnectionPool.cs	Make transacted list/dictionary accessible for tests; refactor field into property
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs	Add `TransactedConnectionPool` to interface; adjust `TryGetConnection` nullability contract
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolOptions.cs	Document `CreationTimeout` meaning
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs	Instantiate/expose `TransactedConnectionPool` implementation
dotnet-tools.json	Add PowerShell (`pwsh`) as a local dotnet tool
.gitignore	Ignore generated devcontainer overlay/env/compose files
.github/prompts/refine-test-overlap.prompt.md	Rename prompt + fix skill/script paths + add tool scoping
.github/prompts/generate-prompt.prompt.md	Document tool scoping in prompt frontmatter + examples
.github/prompts/generate-doc-comments.prompt.md	Rename prompt key
.gitattributes	Force LF for `*.sh` for container bind-mount compatibility
.devcontainer/sqlserver-entrypoint.sh	Generate and share SA password via mounted volume before sqlservr starts
.devcontainer/setup-sqlserver.sh	Write test `config.json` and create Northwind DB inside devcontainer flow
.devcontainer/init.ps1	Host-side init to support git worktrees and generate compose overlays + `.env`
.devcontainer/devcontainer.json	Use multiple compose overrides; add init/post-start commands for tool restore + git safe.directory

Comments suppressed due to low confidence (1)

.devcontainer/devcontainer.json:12

The PR description is still the default template and doesn't describe what this PR is changing (it also appears to include both devcontainer setup and connection-pooling/test changes). Please update the PR description to summarize scope, testing, and any behavior changes so reviewers can validate intent.

{
    "name": "Microsoft.Data.SqlClient",
    "dockerComposeFile": [
        "docker-compose.yml",
        "docker-compose.worktree.yml",
        "docker-compose.local.yml"
    ],
    "service": "devcontainer",
    "workspaceFolder": "/workspaces/SqlClient",
    "features": {
        "ghcr.io/devcontainers/features/powershell:2.0.1": {}
    },

Copilot · 2026-04-04T01:37:25Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs

@@ -106,7 +111,7 @@ internal interface IDbConnectionPool
        /// <param name="userOptions">The user options to use if a new connection must be opened.</param>
        /// <param name="connection">The retrieved connection will be passed out via this parameter.</param>
        /// <returns>True if a connection was set in the out parameter, otherwise returns false.</returns>
-        bool TryGetConnection(DbConnection owningObject, TaskCompletionSource<DbConnectionInternal> taskCompletionSource, DbConnectionOptions userOptions, out DbConnectionInternal? connection);
+        bool TryGetConnection(DbConnection owningObject, TaskCompletionSource<DbConnectionInternal>? taskCompletionSource, DbConnectionOptions userOptions, out DbConnectionInternal? connection);



IDbConnectionPool.TryGetConnection now allows a nullable TaskCompletionSource and nullable out connection. At least WaitHandleDbConnectionPool still uses non-nullable annotations and assigns null in its implementation, which will trigger nullability-mismatch warnings (and the repo treats warnings as errors) and break the build. Update all IDbConnectionPool implementers to match this contract (parameter and out nullability).

Copilot · 2026-04-04T01:37:25Z

...t.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolTransactionTest.cs

+        }
+
+        public override void EnlistTransaction(Transaction? transaction)
+        {
+            if (transaction != null)
+            {
+                EnlistedTransaction = transaction;
+            }
+        }
+


MockDbConnectionInternal overrides EnlistTransaction with a nullable Transaction parameter. The base abstract member is non-nullable; with enable and warnings-as-errors this will produce a nullability-mismatch warning. Make the override signature non-nullable and handle null via a separate code path if needed.

Copilot · 2026-04-04T01:37:25Z

...t.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolTransactionTest.cs

+        protected override void Activate(Transaction? transaction)
+        {
+            EnlistedTransaction = transaction;
+        }
+


MockDbConnectionInternal overrides Activate with a nullable Transaction parameter. The base abstract member is non-nullable; with enable and warnings-as-errors this will produce a nullability-mismatch warning. Align the override signature with the base member (non-nullable Transaction).

Copilot · 2026-04-04T01:37:26Z

...t.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolTransactionTest.cs

+
+        // Assert - connection should be in the transacted pool
+        Assert.True(_pool.TransactedConnectionPool.TransactedConnections.ContainsKey(transaction));
+        Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]);
+


Tests read TransactedConnections without synchronization (e.g., ContainsKey/Count). Production code mutates this Dictionary under a lock and transaction completion callbacks may occur asynchronously; reading it without locking can make these tests flaky or throw during concurrent mutation. Consider locking on TransactedConnections (or exposing a thread-safe snapshot API) when asserting its contents.

Copilot · 2026-04-04T01:37:26Z

...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/TransactedConnectionPool.cs

@@ -123,9 +122,9 @@ internal TransactedConnectionPool(IDbConnectionPool pool)
        TransactedConnectionList? connections;
        bool txnFound = false;

-        lock (_transactedCxns)
+        lock (TransactedConnections)
        {
-            txnFound = _transactedCxns.TryGetValue(transaction, out connections);
+            txnFound = TransactedConnections.TryGetValue(transaction, out connections);
        }



TransactedConnections is now exposed as an internal Dictionary and the implementation locks on the Dictionary instance itself. Because this object is externally accessible, other code/tests can also lock or enumerate it, increasing deadlock risk and making thread-safety harder to reason about. Prefer a private lock object (or ConcurrentDictionary) and expose a read-only/snapshot view for diagnostics/tests.

Copilot · 2026-04-04T01:37:26Z

src/Microsoft.Data.SqlClient/tests/StressTests/SqlClient.Stress.Tests/SqlClientTestGroup.cs

 using System.Threading.Tasks;
 using System.Diagnostics;
 using System.Data;
+using System.Transactions;


System.Transactions is imported but not used anywhere in this file. With warnings treated as errors, unused usings can fail the build depending on analyzer settings; remove this using unless it will be used in a follow-up change.

Suggested change

using System.Transactions;

mdaigle added 5 commits April 2, 2026 22:39

Test | Connection pool transaction tests (#3805)

9046321

Fix line endings

681dc4a

Get git working

268ec7f

Trigger git index refresh on container create

036ee9a

Fix index refresh

3677dff

Copilot AI review requested due to automatic review settings April 4, 2026 01:29

github-project-automation bot added this to SqlClient Board Apr 4, 2026

github-project-automation bot moved this to To triage in SqlClient Board Apr 4, 2026

Copilot started reviewing on behalf of mdaigle April 4, 2026 01:30 View session

Copilot AI reviewed Apr 4, 2026

View reviewed changes

mdaigle changed the title ~~Dev/mdaigle/dev containers~~ Fix dev containers to work with worktrees Apr 6, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dev containers to work with worktrees#4137

Fix dev containers to work with worktrees#4137
mdaigle wants to merge 5 commits intodev/mdaigle/pwsh-via-dotnet-toolfrom
dev/mdaigle/dev-containers

mdaigle commented Apr 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 4, 2026

Uh oh!

Copilot AI Apr 4, 2026

Uh oh!

Copilot AI Apr 4, 2026

Uh oh!

Copilot AI Apr 4, 2026

Uh oh!

Copilot AI Apr 4, 2026

Uh oh!

Copilot AI Apr 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mdaigle commented Apr 4, 2026

Description

Issues

Testing

Guidelines

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants