Connect-DbaInstance - Give the cloned server ownership of its database connection

[andreasjordan]

When an existing server object is passed in together with a different -Database, the connection context is copied and the database connection was then created with GetDatabaseConnection:

$connContext = $connContext.GetDatabaseConnection($Database, $false)

GetDatabaseConnection opens the connection on the intermediate copy and returns a different ConnectionContext, so the server object that is handed back never owns that connection. Disconnect-DbaInstance can only reach the context of the server it is given, so nothing ever closed it.

The session therefore stayed open for the life of the process, sitting in the target database and holding a shared DATABASE lock on it. On model that is enough to make a later CREATE DATABASE on the same instance fail:

Could not obtain exclusive lock on database 'model'. Retry the operation later.
CREATE DATABASE failed.

which surfaced as an intermittent failure in whatever test file happened to run next, with no connection to the command that caused it.

Where the connection goes

Running the steps of the old code one at a time and counting sessions on the instance after each:

0. server connection only                         sessions=1  [62:master]
1. after ConnectionContext.Copy()                 sessions=1
2. after setting NonPooledConnection              sessions=1
3. after GetDatabaseConnection(model)             sessions=2  [62:master, 64:master]
4. after New-Object Smo.Server                    sessions=2
5. after reading CurrentDatabase [model]          sessions=2
6. after Disconnect-DbaInstance                   sessions=2   <-- closes nothing
7. after disconnecting the intermediate copy      sessions=1   <-- this owned it

Step 3 opens the connection on the copy. Step 3 also reassigns $connContext, which drops the only reference to the object that owns it. Step 6 shows the consequence: the server we return has nothing to close. Step 7 shows who did own it - and that disconnecting the copy is not a usable fix either, because that is the working connection.

The change

Setting DatabaseName on the copy keeps the connection with the context that the returned server owns, so Disconnect-DbaInstance closes it. This is also what the connection string paths of this command already do, so the server object path now behaves like the others.

History of the line, and why each earlier fix still holds

The line has been touched four times, and none of the reasons are lost by this change:

PR	What it did	Status now
#6904 (2020)	Introduced it in the new code path, as ConnectionContext.Copy().GetDatabaseConnection($Database), next to a # TODO: Do we have to check if its the same database?	Not a fix for a reported bug, it was the chosen implementation
#7548, fixes #7546 (2021)	Moved the call to be the last change to the copied context, because GetDatabaseConnection opens the connection and later property assignments would come too late	Dissolved. Setting DatabaseName does not open a connection - the trace above shows nothing opens until the first query. The assignment is kept in the same last position anyway
#8025 (2021)	Added a save and restore around it, because GetDatabaseConnection resets StatementTimeout	No longer needed and removed. StatementTimeout is set earlier on the copy and simply stays. clones when using parameter StatementTimeout covers it
#9505 (2024)	Added the second argument, GetDatabaseConnection($Database, $false), to force a non-pooled connection. Without it, Backup-DbaDatabase got a cached connection to an already dropped database and the context change silently did not happen. Also added the warning when CurrentDatabase does not match	Verified not to regress, see below. The warning is kept

#9505 is the one worth care, because $false forced a non-pooled connection and setting DatabaseName does not. It holds because DatabaseName puts Initial Catalog into the connection string, so the database is part of the pool key: a pooled connection then comes from that database's pool instead of being taken from the original database's pool and switched.

Replaying the exact #9505 scenario against this change - create a database, connect with it as context, drop it, then ask for a clone on master:

1. connected with -Database dbatoolsci_ctx_853144975 -> CurrentDatabase = dbatoolsci_ctx_853144975
2. dropped dbatoolsci_ctx_853144975
3. clone with -Database master -> CurrentDatabase = master
   warning raised          : none
   a query really runs in  : master

And with pooling left on everywhere, which is the case $false used to opt out of:

base NonPooledConnection = False
clone CurrentDatabase    = tempdb
clone NonPooledConnection= False
query runs in            = tempdb
original still in        = master
warning raised           : none

The test #9505 added, clones when using Backup-DabInstace, is still in the suite and passes.

Measured

The clearest way to see it is to repeat the call. Eight cycles of connect to a database from an existing server object, each followed by Disconnect-DbaInstance, counting the module's sessions on the instance after every cycle:

Before - sessions accumulate without bound, and every new one sits in the target database:

cycle 1: 4 sessions [58:master, 60:model, 61:master, 64:model]
cycle 2: 5 sessions [58:master, 60:model, 61:master, 64:model, 73:model]
cycle 3: 4 sessions [...]
cycle 4: 5 sessions [...]
cycle 5: 5 sessions [...]
cycle 6: 6 sessions [58:master, 60:model, 61:master, 64:model, 73:model, 76:model]
cycle 7: 6 sessions [...]
cycle 8: 7 sessions [58:master, 60:model, 61:master, 64:model, 73:model, 76:model, 77:model]

After - flat, and the same spids are reused every cycle:

cycle 1: 3 sessions [58:master, 60:model, 61:master]
cycle 2: 3 sessions [58:master, 60:model, 61:master]
...
cycle 8: 3 sessions [58:master, 60:model, 61:master]

That is the difference between an orphaned non-pooled connection, which nothing can ever reuse or close, and a pooled one that goes back to the pool on disconnect. The sessions that remain after the fix are the pool's: they stay sleeping, they are reused by the next call, and their number does not grow. Connecting from a plain instance name behaves the same way in both versions, which is why that path never had the problem.

At suite scale the same difference: the full 744 file run of 2026-08-15 had 69 files failing with Timeout expired ... prior to obtaining a connection from the pool, and the same run with this fix had zero.

Commands that read a per database view through Invoke-DbaQuery -Database stop leaking without being touched. Get-DbaDbQueryStoreOption -Database model went from three leaked sessions, one of them in model, to one leaked session and none in model.

Tests

tests/Connect-DbaInstance.Tests.ps1 gets a regression test that connects to a database, disconnects again and asserts the session count is back where it started. It was verified to fail against the old implementation (Expected 4, but got 5) and to be the only test that fails there.

Run against SQL Server 2025, including the commands that use this path:

File	Result
Connect-DbaInstance	33 passed, 1 skipped
Invoke-DbaQuery	31 passed
Remove-DbaDbData	13 passed
Get-DbaUserPermission	6 passed
Get-DbaDbRecoveryModel	6 passed
Get-DbaDbQueryStoreOption	4 passed
Disconnect-DbaInstance, Remove-DbaDbAsymmetricKey, Remove-DbaDbCertificate, Remove-DbaDbEncryptionKey	all passed

106 tests, no failures.

---

This text was created by Claude and reviewed by Andreas Jordan.

🤖 Generated with Claude Code

[potatoqualitee]

seems straightforward but there must have been a reason we did it so weirdly. at the same time, it does cause issues. im going to ask gpt to investigate why we likely did it and what the consequences are of the change.

[potatoqualitee]

GPT Pro said on my behalf: I dug through the history because this code was weird enough that I didn't believe we did it accidentally.

The weird code had real reasons

The original GetDatabaseConnection() approach goes back to an Azure SQL problem where directly setting DatabaseName could fail with:

Property DatabaseName cannot be changed or read after a connection string has been set.

Then more fixes accumulated around it:

• Connect-DbaInstance - Reorder changes when connection context is copied #7548: moved GetDatabaseConnection() to the end because it effectively opened/used the connection, after which properties like NonPooledConnection couldn't be changed.
• Invoke-DbaQuery - Remove fixed default for QueryTimeout and use global default from Connect-DbaInstance #8025: saved/restored StatementTimeout because GetDatabaseConnection() returned a new context and lost it.
• Connect-DbaInstance: Fix for issue when changing database context #9505: changed it to GetDatabaseConnection($Database, $false) because SMO could return a cached pooled connection for a database that had already been dropped.

So no, we didn't do this for funsies.

But this PR found a real bug

The ownership problem described in #10584 checks out.

Basically:

A → Copy to B → GetDatabaseConnection creates C → return C

But SMO can open/use B while creating C. We then throw away our reference to B and return a Server containing C.

Disconnect-DbaInstance can only disconnect the returned server's ConnectionContext, which is C. It has no way to find B.

So B can sit there for the life of the process holding a database lock. That explains both the accumulating sessions and the bizarre later failures trying to get an exclusive lock on model.

For normal dbatools connections, changing this to:

$connContext.DatabaseName = $Database

is much cleaner. It also makes several old workarounds unnecessary: no premature connection, no lost StatementTimeout, and the target database becomes Initial Catalog in the connection string, giving it the correct ADO.NET pool.

One thing I think this PR currently breaks

We also support creating a Server from a raw Microsoft.Data.SqlClient.SqlConnection.

Those SMO contexts have an explicit connection string. Copy() preserves it, and SMO specifically refuses to let you set DatabaseName afterward.

So please add a regression test for:

$params = @{
    SqlInstance      = $sqlConnection
    EnableException = $true
}
$server = Connect-DbaInstance @params

$params = @{
    SqlInstance      = $server
    Database         = "tempdb"
    EnableException  = $true
}
$clone = Connect-DbaInstance @params

I expect the second call to fail on the new DatabaseName assignment.

CI doesn't catch this because we test SqlConnection → Server and separately Server → different Database, but not SqlConnection → Server → different Database.

What I'd change before merging

Keep the new DatabaseName approach for contexts where it's mutable. Don't just fall back to the old GetDatabaseConnection() when it isn't, because that recreates the leak.

For explicit-connection-string contexts, construct the final database-specific context directly with the target database already in Initial Catalog. Be careful to preserve authentication state that may live outside the connection string, such as access tokens/SSPI.

I'd also add coverage for -Database + -NonPooledConnection. The old $false explicitly forced the database connection to be nonpooled. The new implementation normally allows pooling. I think that's probably desirable, but it's still a behavioral change and we have commands that depend on a stable session for temp tables/session state.

Finally, I'd make the leak regression run several cycles. With pooling, a sleeping SQL session remaining is legitimate. What matters is that the session count stabilizes instead of growing every time, which your eight-cycle test already demonstrates really well.

Bottom line

The PR is fixing a real bug and I prefer the new design. I wouldn't revert to the old approach. I just want the raw SqlConnection case handled before we merge, plus coverage for nonpooled connections and repeated session growth.