From fccafe9e2298182dc5184c2c22a3817cd64fb2a8 Mon Sep 17 00:00:00 2001 From: Andreas Jordan Date: Sat, 29 Aug 2026 16:28:38 +0200 Subject: [PATCH 1/2] Import-DbaSpConfigure - Stop eating the caller loop and skipping its own cleanup The four begin block guards (three sysadmin checks and the missing-file check) called Stop-Function -Continue, but no loop encloses them: without -EnableException the continue escaped the command, consumed an iteration of whatever loop the caller was running in, and - because the whole command aborted - skipped the end block that disconnects the connections the command had already opened for itself, leaking them on every error path. Part of the #10638 inventory, same fix shape as #10636-#10640: stop and return; the process block already guards with Test-FunctionInterrupt and the end block cleanup now runs on these paths too. Verified via the lab harness: 15 tests, 0 failed, lab left clean; the unfixed command fails the new loop-counter test with "Expected 3, but got 0". References #10638 (do Import-DbaSpConfigure) Co-Authored-By: Claude Fable 5 --- public/Import-DbaSpConfigure.ps1 | 15 +++++++++++---- tests/Import-DbaSpConfigure.Tests.ps1 | 15 +++++++++++++++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/public/Import-DbaSpConfigure.ps1 b/public/Import-DbaSpConfigure.ps1 index d7fe2d6c286..f34bf8e9f4d 100644 --- a/public/Import-DbaSpConfigure.ps1 +++ b/public/Import-DbaSpConfigure.ps1 @@ -135,7 +135,11 @@ function Import-DbaSpConfigure { } if (-not (Test-SqlSa -SqlInstance $sourceserver -SqlCredential $SourceSqlCredential)) { - Stop-Function -Message "Not a sysadmin on $sourceserver. Quitting." -Category PermissionDenied -Target $sourceserver -Continue + # No -Continue on these guards: the begin block has no enclosing loop, so the continue + # would escape the command, eat an iteration of whatever loop the caller runs in, and + # skip the connection cleanup in the end block. + Stop-Function -Message "Not a sysadmin on $sourceserver. Quitting." -Category PermissionDenied -Target $sourceserver + return } try { @@ -151,7 +155,8 @@ function Import-DbaSpConfigure { } if (-not (Test-SqlSa -SqlInstance $destserver -SqlCredential $DestinationSqlCredential)) { - Stop-Function -Message "Not a sysadmin on $destserver. Quitting." -Category PermissionDenied -Target $destserver -Continue + Stop-Function -Message "Not a sysadmin on $destserver. Quitting." -Category PermissionDenied -Target $destserver + return } $source = $sourceserver.DomainInstanceName @@ -170,11 +175,13 @@ function Import-DbaSpConfigure { } if (!(Test-SqlSa -SqlInstance $server -SqlCredential $SqlCredential)) { - Stop-Function -Message "Not a sysadmin on $server. Quitting." -Category PermissionDenied -Target $server -Continue + Stop-Function -Message "Not a sysadmin on $server. Quitting." -Category PermissionDenied -Target $server + return } if (-not (Test-Path $Path)) { - Stop-Function -Message "File $Path Not Found" -Category InvalidArgument -Target $Path -Continue + Stop-Function -Message "File $Path Not Found" -Category InvalidArgument -Target $Path + return } } diff --git a/tests/Import-DbaSpConfigure.Tests.ps1 b/tests/Import-DbaSpConfigure.Tests.ps1 index ba4ffc68b5c..323e061e121 100644 --- a/tests/Import-DbaSpConfigure.Tests.ps1 +++ b/tests/Import-DbaSpConfigure.Tests.ps1 @@ -50,6 +50,21 @@ Describe $CommandName -Tag IntegrationTests { $PSDefaultParameterValues.Remove("*-Dba*:EnableException") } + Context "A missing file does not eat an iteration of the caller's loop" { + It "Warns and completes every iteration" { + # The begin block guards used to run Stop-Function -Continue without an enclosing loop - + # the continue escaped the command and consumed an iteration of this very loop, so the + # counter fell short (#10638). + $loopCount = 0 + foreach ($i in 1..3) { + $null = Import-DbaSpConfigure -SqlInstance $TestConfig.InstanceSingle -Path "$exportPath\does-not-exist.sql" -WarningAction SilentlyContinue + $loopCount++ + } + $loopCount | Should -Be 3 + $WarnVar | Should -BeLike "*Not Found*" + } + } + Context "The connection of the caller is left alone when importing from a file (#10554)" { BeforeAll { $PSDefaultParameterValues["*-Dba*:EnableException"] = $true From 643bd568479e2ae08d16caeea4f2d8776acec11b Mon Sep 17 00:00:00 2001 From: Andreas Jordan Date: Sun, 30 Aug 2026 15:19:27 +0200 Subject: [PATCH 2/2] Import-DbaSpConfigure - Close owned connections before the interrupt return Review follow-up: the end block returned on Test-FunctionInterrupt before the ownership disconnects, so a begin-block guard (missing file, failed sysadmin check) skipped the cleanup of connections the command had already opened. The disconnects now run first; only the finished message stays suppressed on the interrupt path. Probed on the lab while building the regression: SMO's auto-disconnect returns every connection this command opens itself after each batch, on the pooled path and on a Pooling=False connection string alike - the skipped disconnect was not observable through any reachable input shape on this stack, so the new guard-path session-count test pins the invariant without a red-on-old, stated in its comment. (do Import-DbaSpConfigure) Co-Authored-By: Claude Fable 5 --- public/Import-DbaSpConfigure.ps1 | 10 +++-- tests/Import-DbaSpConfigure.Tests.ps1 | 56 +++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/public/Import-DbaSpConfigure.ps1 b/public/Import-DbaSpConfigure.ps1 index f34bf8e9f4d..8ad64747594 100644 --- a/public/Import-DbaSpConfigure.ps1 +++ b/public/Import-DbaSpConfigure.ps1 @@ -301,9 +301,10 @@ function Import-DbaSpConfigure { } } end { - if (Test-FunctionInterrupt) { return } - - # Only close the connections that were opened here. See #10554. + # Only close the connections that were opened here, and close them before the interrupt + # return below: a begin-block guard sets the interrupt after a connection was already + # opened (a missing -Path, a failed sysadmin check), and returning first would leak it + # on every guard path. See #10554. if ($isNewServerConnection) { $server.ConnectionContext.Disconnect() } @@ -314,6 +315,9 @@ function Import-DbaSpConfigure { $destserver.ConnectionContext.Disconnect() } + # Only the finished message stays suppressed when the command was interrupted. + if (Test-FunctionInterrupt) { return } + If ($Pscmdlet.ShouldProcess("console", "Showing finished message")) { Write-Message -Level Output -Message "SQL Server configuration options migration finished." } diff --git a/tests/Import-DbaSpConfigure.Tests.ps1 b/tests/Import-DbaSpConfigure.Tests.ps1 index 323e061e121..1380f382186 100644 --- a/tests/Import-DbaSpConfigure.Tests.ps1 +++ b/tests/Import-DbaSpConfigure.Tests.ps1 @@ -261,4 +261,60 @@ SELECT name, value, value_in_use FROM sys.configurations WHERE name IN ('cost th $WarnVar[-1] | Should -Match "Some configuration options will be updated once SQL Server is restarted" } } + + Context "A guard interrupt still closes the connection the command opened (#10554)" { + BeforeAll { + # This pins the invariant that a guard interrupt leaves no session of the command + # behind. Probed while writing it: SMO's auto-disconnect returns the physical + # connection after every batch for every connection the command opens itself - the + # skipped end-block disconnect was therefore not observable on any reachable input + # shape, and this test also passes on the unfixed code. It stands guard for the day a + # connection is held open eagerly. The application name marks the session so the count + # below finds exactly this one; Pooling=False makes a survivor impossible to miss. + $guardAppName = "dbatoolsci_spconfigure_guard_$(Get-Random)" + $guardConnectionString = "Data Source=$($TestConfig.InstanceSingle);Integrated Security=True;Trust Server Certificate=True;Pooling=False;Application Name=$guardAppName" + + # The missing file is the guard under test: the begin block has already opened the + # connection when it stops, so the end block cleanup must run despite the interrupt. + $splatGuardImport = @{ + SqlInstance = $guardConnectionString + Path = "$exportPath\does-not-exist.sql" + WarningAction = "SilentlyContinue" + } + $null = Import-DbaSpConfigure @splatGuardImport + # Invoke-DbaQuery below writes to $WarnVar as well, so it has to be kept here. + $guardWarnings = $WarnVar + + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + $guardSessionQuery = @" +SELECT COUNT(*) AS SessionCount FROM sys.dm_exec_sessions WHERE program_name = '$guardAppName' +"@ + $guardSessionCount = (Invoke-DbaQuery -SqlInstance $TestConfig.InstanceSingle -Query $guardSessionQuery).SessionCount + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + AfterAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + # On a defective command the non-pooled session survives - close the cached connection + # and kill any remaining marked session so nothing leaks into later tests. + $guardEntry = Get-DbaConnectedInstance | Where-Object ConnectionString -match $guardAppName + if ($guardEntry) { + $null = $guardEntry.ConnectionObject | Disconnect-DbaInstance + } + $null = Get-DbaProcess -SqlInstance $TestConfig.InstanceSingle -Program $guardAppName -WarningAction SilentlyContinue | Stop-DbaProcess -WarningAction SilentlyContinue + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + It "warns about the missing file" { + $guardWarnings | Should -BeLike "*Not Found*" + } + + It "closes the non-pooled connection it opened although the guard interrupted the command" { + $guardSessionCount | Should -Be 0 + } + } }