diff --git a/public/Import-DbaSpConfigure.ps1 b/public/Import-DbaSpConfigure.ps1 index d7fe2d6c286..8ad64747594 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 } } @@ -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." } diff --git a/tests/Import-DbaSpConfigure.Tests.ps1 b/tests/Import-DbaSpConfigure.Tests.ps1 index ba4ffc68b5c..1380f382186 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 @@ -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 + } + } }