Import-DbaSpConfigure - Stop eating the caller loop and skipping its own cleanup (#10641)

Author: andreasjordan

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
             }
         }
 
@@ -294,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()
         }
@@ -307,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."
         }

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
@@ -246,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
+        }
+    }
 }