Commands disconnect SQL Server connections they do not own

[andreasjordan]

Eight sites in public/ and private/ close a connection the caller owns. Invoke-DbaQuery is the one that started this and is described in full first; the complete inventory follows below, together with the places that already get it right.

The damage is invisible on a pooled connection - SMO transparently reopens it from the pool. It is real on anything session-scoped: -NonPooledConnection, a dedicated admin connection, temp tables, SET options, session context, sp_getapplock. It also makes the database-context leak in #10555 look unreproducible, because the accidental reconnect resets the current database.

Invoke-DbaQuery

When Invoke-DbaQuery decides it cannot reuse the passed-in SMO server object, it builds its own connection with NonPooledConnection = $true and closes it afterwards:

# if the given connection started out open, don't close it.
if ($connDbaInstanceParams.NonPooledConnection -and -not $startedWithAnOpenConnection) {
    $null = $server | Disconnect-DbaInstance -Verbose:$false
}

public/Invoke-DbaQuery.ps1:544-548

The assumption behind the comment does not hold: Connect-DbaInstance frequently hands back the same object that was passed in instead of a new connection, because it only copies the context when something actually differs (public/Connect-DbaInstance.ps1:699-745). In that case Invoke-DbaQuery disconnects the caller's connection.

Two ways into it, both verified on SQL Server 2022 with Connect-DbaInstance -NonPooledConnection:

1. -Database naming the database the connection is already on. $startedWithAnOpenConnection compares ConnectionContext.DatabaseName (public/Invoke-DbaQuery.ps1:503), which is empty for a connection built by Connect-DbaInstance without -Database, while Connect-DbaInstance compares ConnectionContext.CurrentDatabase (public/Connect-DbaInstance.ps1:712), which is master. The two disagree, so nothing is copied and the caller's connection is closed.
2. A -SqlCredential holding a Windows account. That sets ConnectAsUserName, which makes $startedWithAnOpenConnection false unconditionally (public/Invoke-DbaQuery.ps1:505). Then even Invoke-DbaQuery without -Database closes the caller's connection.

Steps to Reproduce

$server = Connect-DbaInstance -SqlInstance $instance -NonPooledConnection
$server.ConnectionContext.IsOpen                                  # True

$null = Invoke-DbaQuery -SqlInstance $server -Query "SELECT 1"
$server.ConnectionContext.IsOpen                                  # True

$null = Invoke-DbaQuery -SqlInstance $server -Database master -Query "SELECT 1"
$server.ConnectionContext.IsOpen                                  # False   <-- closed by dbatools

--- Windows auth (ConnectAsUserName []) ---
  IsOpen before                         : True
  IsOpen after Invoke-DbaQuery          : True
  IsOpen after -Database master         : False
--- SqlCredential with a Windows account (ConnectAsUserName [Admin@DOMAIN]) ---
  IsOpen before                         : True
  IsOpen after Invoke-DbaQuery          : False
  IsOpen after -Database master         : False

Session state is lost with it, because SMO transparently reopens the connection on next use:

$server = Connect-DbaInstance -SqlInstance $instance -NonPooledConnection
$null = $server.ConnectionContext.ExecuteNonQuery("CREATE TABLE #t (id int)")
$null = Invoke-DbaQuery -SqlInstance $server -Database master -Query "SELECT 1"
$server.ConnectionContext.ExecuteScalar("SELECT COUNT(*) FROM #t")
# Invalid object name '#t'.

The same defect elsewhere

Site	Where the connection comes from	Guard
Invoke-DbaQuery.ps1:547	Connect-DbaInstance -NonPooledConnection	ownership inferred from parameters, see above
Get-DbaDbExtentDiff.ps1:191	Connect-DbaInstance -NonPooledConnection	none
Install-DbaMaintenanceSolution.ps1:895	Connect-DbaInstance -NonPooledConnection	none
Update-DbaMaintenanceSolution.ps1:185	Connect-DbaInstance -NonPooledConnection	none
Invoke-DbaAdvancedRestore.ps1:601 and :609	Connect-DbaInstance (plain)	none - and it disconnects after every backup file
Import-DbaBinaryFile.ps1:279	$tbl.Parent.Parent from a piped-in table	none - the command never opens a connection at all
Add-DbaRegServerGroup.ps1:155	$reggroup.ParentServer	none - never opened it either
Import-DbaSpConfigure.ps1:243-246	Connect-DbaInstance (plain)	none

The three unguarded -NonPooledConnection sites carry the comment "Close non-pooled connection as this is not done automatically. If it is a reused Server SMO, connection will be opened again automatically on next request." - so the reopen was known; what the comment misses is that the session dies with it.

Invoke-DbaAdvancedRestore has the widest reach, because Restore-DbaDatabase connects once and passes that server object down. Note it connects with -Database master, so a caller object that is not on master gets copied first and is safe - it is the caller who is already on master whose connection gets closed.

Verified against SQL Server 2022 with Connect-DbaInstance -NonPooledConnection, using a temp table as the session marker and Get-DbaDatabase as a control:

Get-DbaDbExtentDiff                            session survived: False
Update-DbaMaintenanceSolution                  session survived: False
Restore-DbaDatabase (advanced restore)         session survived: False
Get-DbaDatabase (control, no disconnect)       session survived: True

private/functions/Invoke-DbaDbCorruption.ps1:159 and :168 disconnect unconditionally as well, but the function is private and effectively test-only.

Where this is already done right

These are the patterns to copy rather than invent something new:

• $dacOpened - Export-DbaCredential, Export-DbaLinkedServer, Export-DbaInstance, Copy-DbaCredential, Copy-DbaLinkedServer, Copy-DbaDbMail, Invoke-DbaDbDecryptObject, Sync-DbaAvailabilityGroup, Start-DbaMigration. A caller-supplied dedicated admin connection is detected (Test-DacConnection / $dacConnected) and reused, and only a connection the command opened itself is closed.
• $startedWithANonPooledConnection - Write-DbaDbTableData.ps1:227. The same command also restores the original database context at :807-810, which is prior art for Database-scoped SMO calls silently change the current database of the shared connection #10555.
• $Newconnection - private/functions/Test-DbaRestoreVersion.ps1:88.

Connect-DbaInstance's own internal retries and private/scripts/updateTeppAsync.ps1 disconnect connections they genuinely own and need no change.

History

The disconnect was added for #6210 (Invoke-DbaQuery leaking connections it created), and the ConnectAsUserName condition comes from #7725 / #8491. Both are right in themselves - the gap is only that "did I create this connection?" is inferred from parameters instead of from what Connect-DbaInstance actually returned.

Suggested fix

Import-DbaBinaryFile and Add-DbaRegServerGroup never open a connection, so for those the disconnect can simply go away.

The remaining six need to know whether the connection is theirs, and the rule is the same everywhere: a command may only close what it opened itself. Connect-DbaInstance already knows this - it sets $isNewConnection internally - so the choice is between

1. surfacing that, so any command can ask instead of infer, which also keeps the next command from getting it wrong, or
2. a per-command flag, hand-rolled six more times in the shape of the existing $dacOpened / $startedWithANonPooledConnection, or
3. an object identity check where the original input is still in scope, for example in Invoke-DbaQuery:

if ($connDbaInstanceParams.NonPooledConnection -and -not [object]::ReferenceEquals($server, $instance.InputObject)) {
    $null = $server | Disconnect-DbaInstance -Verbose:$false
}

Option 1 looks like the better deal given that eight sites got this wrong independently.

While in there: the DatabaseName vs CurrentDatabase mismatch between Invoke-DbaQuery and Connect-DbaInstance is worth aligning on its own - it also causes an unnecessary second connection for -Database master on a connection that is already on master.

Test

Per tests/CLAUDE.md, one regression test per fixed command: connect with -NonPooledConnection, create a temp table, run the command against that server object, then assert $server.ConnectionContext.IsOpen is still $true and the temp table is still there. A pooled connection cannot show this - it passes either way.

---

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

[andreasjordan]

Implementation plan

One finding first, because it changes the shape of the "surface $isNewConnection" option from what the issue text suggests.

The mechanism cannot be a NoteProperty

The obvious way to surface it is Add-Member next to ComputerName / IsAzure in Connect-DbaInstance.ps1:1307-1311. That would be wrong: the flag would live on an object that gets passed around. A command connects (IsNewConnection = $true), then calls Get-DbaDatabase -SqlInstance $server, whose own Connect-DbaInstance call stamps $false onto that same object, and the outer command then skips its own cleanup. That is #6210 all over again.

A plain identity check (ReferenceEquals($server, $instance.InputObject)) is wrong in one case as well: for a SqlConnection input, Connect-DbaInstance builds a new Server object around the caller's connection and correctly sets $isNewConnection = $false (:636-639). Identity says "mine", ownership says "not mine", and closing that connection would be a new bug.

So the signal has to be per call, not per object: an opt-in output variable, in the shape of -ErrorVariable.

Change 1 - Connect-DbaInstance

Add [string]$IsNewConnectionVariable to the param block and set it immediately before each of the two emit points, :1264 (-SqlConnectionOnly) and :1316:

if ($IsNewConnectionVariable) {
    $PSCmdlet.SessionState.PSVariable.Set($IsNewConnectionVariable, $isNewConnection)
}

$isNewConnection is already correct at both points: $false for Server and SqlConnection inputs, $true for string, connection string and RegisteredServer inputs, and flipped to $true at :738 and :746 when a context copy or a fresh connection is made. No logic changes, only exposure. With several instances the variable reflects the last one emitted - worth documenting, and all six consumers below connect one instance at a time.

Change 2 - the six consumers

The same edit everywhere: request the variable, then guard the disconnect.

$splatConnect = @{
    SqlInstance             = $instance
    SqlCredential           = $SqlCredential
    NonPooledConnection     = $true
    IsNewConnectionVariable = "isNewConnection"
}
$server = Connect-DbaInstance @splatConnect
# ...
if ($isNewConnection) {
    $null = $server | Disconnect-DbaInstance
}

Command	Connect / disconnect	Note
Invoke-DbaQuery.ps1	:510-524 / :545	replaces the $startedWithAnOpenConnection inference for the disconnect; that variable then only serves the reuse decision
Get-DbaDbExtentDiff.ps1	:109 / :191	same foreach ($instance) scope
Install-DbaMaintenanceSolution.ps1	:460 / :895	same scope
Update-DbaMaintenanceSolution.ps1	:132 / :185	same scope
Invoke-DbaAdvancedRestore.ps1	:261 in begin / :601 and :609 in end	same function scope; the per-backup-file disconnect at :601 should go regardless, reconnecting between files serves nothing
Import-DbaSpConfigure.ps1	:124, :135, :149 in process / :243-246 in end	three connections, three variables; with a pipeline of several instances the end block already only sees the last one, a pre-existing wart worth a comment

Change 3 - the two that own nothing

Delete the disconnect: Import-DbaBinaryFile.ps1:279 (server comes from a piped-in table, and it fires once per file) and Add-DbaRegServerGroup.ps1:154-156 (server comes from $reggroup.ParentServer).

Tests

Per tests/CLAUDE.md, behavioral at the real boundary. For Connect-DbaInstance, that the variable is $true for a string instance and $false when a connected server object is passed back in. For each fixed command, the shape used for the inventory above:

$server = Connect-DbaInstance -SqlInstance $instance -NonPooledConnection
$null = $server.ConnectionContext.ExecuteNonQuery("CREATE TABLE #marker (id int)")
$null = <command> -SqlInstance $server ...
$server.ConnectionContext.ExecuteScalar("SELECT COUNT(*) FROM #marker") | Should -Be 0

A pooled connection passes either way, so the test has to be non-pooled.

Pull request split

One command per pull request, in this order:

1. Connect-DbaInstance and Invoke-DbaQuery together - the parameter is dead code without its first consumer
2. Get-DbaDbExtentDiff
3. Install-DbaMaintenanceSolution
4. Update-DbaMaintenanceSolution
5. Invoke-DbaAdvancedRestore - the riskiest, because Restore-DbaDatabase rides on it
6. Import-DbaSpConfigure
7. the two deletions, grouped, since neither has a mechanism to review

Risks

Connect-DbaInstance is the most used function in the module, so the parameter has to be inert when it is not passed - hence the if ($IsNewConnectionVariable) guard rather than always setting something. $PSCmdlet.SessionState.PSVariable.Set is PowerShell v3 safe. The $dacOpened family and Write-DbaDbTableData stay as they are; migrating them to the new mechanism would be a later cleanup, not part of this.

---

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

[andreasjordan]

Correction to the plan above, found while implementing it in #10559: the mechanism has to be a [ref], not a variable name.

$PSCmdlet.SessionState.PSVariable.Set() works for a caller outside of dbatools, but not for a caller inside it - which is the case that matters here. Both commands share the module session state, so the value lands in the local scope of Connect-DbaInstance instead of the scope of the calling command. Set-Variable -Scope 1 does not help either, because from a module function the parent scope is the module scope, not the caller.

external caller, name : set via SessionState
in-module caller, name : unset
in-module caller, ref  : set via ref

So the parameter is -IsNewConnectionReference and takes [ref]$variable, and the consumers look like this:

$isNewConnection = $false
$splatConnect = @{
    SqlInstance              = $instance
    SqlCredential            = $SqlCredential
    NonPooledConnection      = $true
    IsNewConnectionReference = [ref]$isNewConnection
}
$server = Connect-DbaInstance @splatConnect
# ...
if ($isNewConnection) {
    $null = $server | Disconnect-DbaInstance
}

The variable has to exist before it is passed as [ref], which is welcome here anyway: a command that does not call Connect-DbaInstance on every path needs the $false default so that a stale value from an earlier loop iteration cannot close a connection of the caller.

Everything else in the plan stands, including the pull request split.

---

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

[andreasjordan]

Correction to the inventory

The table in the issue body lists Add-DbaRegServerGroup as a single site. That is wrong, and the reason is worth recording: the grep I built the table from matched Disconnect-DbaInstance and ConnectionContext.Disconnect(), but dropped the generic .Disconnect() alternative. Everything that disconnects through $serverstore.ServerConnection or $parentserver.ServerConnection was therefore invisible.

Add-DbaRegServerGroup is one of seven sites in the registered server commands that share one mechanism, including two Get- commands. Removing its disconnect alone does not even fix its own symptom, because it calls Get-DbaRegServerGroup, which disconnects as well. That family is now #10572 and should be fixed there as one change, not as one of the per-command pull requests here.

Where the rest stands

Site	State
Invoke-DbaQuery	merged with #10564
Get-DbaDbExtentDiff	#10567
Update-DbaMaintenanceSolution	#10568, also fixes #10569
Install-DbaMaintenanceSolution	#10570
Import-DbaBinaryFile	#10571
Add-DbaRegServerGroup	moved to #10572 with the rest of its family
Import-DbaSpConfigure	open
Invoke-DbaAdvancedRestore	open

Three of the five sites done so far had a database context leak hiding behind the disconnect, and Import-DbaBinaryFile turned out to fail outright on a connection that is already open. The disconnect is rarely the whole story - it is worth checking what it was covering up before removing it.

---

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