Skip to content

Cleanup, output and -WhatIf - Fix the side findings of the progress work in 10 commands - #10791

Merged
potatoqualitee merged 15 commits into
developmentfrom
side-findings-10776-fixes
Oct 22, 2026
Merged

potatoqualitee merged 15 commits into
developmentfrom
side-findings-10776-fixes

Conversation

@andreasjordan

@andreasjordan andreasjordan commented Oct 17, 2026 •

Copy link
Copy Markdown
Collaborator

Part of #10776: everything except the runspace pool, which is #10789. Together the two PRs cover every checkbox of the issue.

One commit per command, as in the tier PRs, so each can be reviewed on its own. Three later commits correct the tests of Invoke-DbaDbLogShipRecovery and New-DbaComputerCertificate after the lab runs and fix the name guessing of New-DbaComputerCertificate. Every fix has a regression test that fails on the old command, except where noted.

Sync-DbaAvailabilityGroup

The dedicated admin connection opened for Copy-DbaCredential, Copy-DbaDbMail and Copy-DbaLinkedServer was closed only after the three commands. A throw, Ctrl+C or Select-Object -First in between left it open, and SQL Server allows only one dedicated admin connection per instance. The finally that completes the progress bar now closes it too, when the command opened it.

Test: one SQL credential, only Credentials synced, Select-Object -First 1 stops the pipeline at the first copied credential. Afterwards no session may be left on the admin endpoint of the primary.

Invoke-DbaDbDataGenerator

The copy of the connection the inserts use was closed after the try/finally around the database loop. The three returns inside it, a throw under -EnableException and a stopped pipeline left one session open per call. The close moved into the finally. A failing close warns there instead of calling Stop-Function, because a throw from a finally would replace the error on its way out.

Test: five rows for a unique INT column limited to 1 and 2 make the command stop with "Could not generate a unique value". Three such calls must not add a session of the test process.

Invoke-DbaDbDataMasking

  • The identity column, the masking index and the unique values table the command adds per table were removed only at the end of the table loop. Every Stop-Function -Continue after the identity column was added (failed action or composite update, failed index, -ExcludeColumn on a unique index), the "no columns to process" continue, a throw and a stopped pipeline left them in the user's table. They are removed in a finally now. A failed removal warns instead of continuing, so a failed index drop no longer skips the column drop (found later, not in the issue).
  • tempdb.dbo.DeterministicValues is dropped in a finally around the work of the instance.
  • "You cannot use both composites and actions" was followed by masking the table anyway. The table is skipped now.
  • The UPDATE statements of the actions (on failure) and of the composites (on every run) were written to the pipeline, and the drop of a leftover DeterministicValues table wrote a $null.

Most of this diff is indentation from the two new try blocks; git diff -w shows 42 changed lines in the command.

Tests: a leftover DeterministicValues table, a composite, an action that divides by zero, composites and actions in one table, and two new assertions in the existing pipeline-stop context (DeterministicValues dropped, no identity column or index left).

Invoke-DbaAdvancedRestore

  • "Failed to restore db ..., stopping" was a Stop-Function -Continue followed by a break. Without -EnableException the continue won, so the restore went on with the next backup file of the database whose restore had just failed. Without the -Continue, the break stops the database and the command goes on with the next one, as Restfixes #5475 ("break to outer loop") intended.
  • -VerifyOnly returned the result of the first backup file with a return, which left the whole end block: the other backup files and databases were never verified, and a connection the command opened was not closed.

Restore-DbaDatabase calls the command with EnableException, so the first point only affected direct callers of the Get-DbaBackupInformation | ... | Invoke-DbaAdvancedRestore pipeline. The second affected both.

  • Found in review (fffabc7): with the verification going on to the last backup file, -VerifyOnly reached the file that recovers, and the branch for -KeepCDC/-ErrorBrokerConversations came before the one for -VerifyOnly and executed its own restore statement. Two full backups with -VerifyOnly -KeepCDC verified the first and restored the second, with -WithReplace over an existing database. A single full backup took that branch before this PR already. That branch and the page restore branch now leave out -VerifyOnly, so a verification never executes a restore statement.

Tests: -VerifyOnly over a full and a log backup returns two "Verify successful"; a restore into a directory below a file fails once and stops there; -VerifyOnly -KeepCDC over two full backups, formatted to a new database name and new file names as Restore-DbaDatabase hands them over for -VerifyOnly, returns two "Verify successful" and creates no database.

Invoke-DbaAdvancedInstall

  • The failed copy of the configuration file to the remote computer ended the command with a bare return, unlike the other failures, so Install-DbaInstance got no result for that computer. It returns the result object with the reason in Notes now, removes the session and removes the local configuration file, whose cleanup sits in the finally of the installation that is never reached.
  • "Failed to get reboot status from" used $computer, which the command never sets.

Tests: three assertions in the existing "When the computer cannot be reached" context.

Invoke-DbaDbLogShipRecovery

  • The RESTORE ... WITH RECOVERY ran through Query(), which returns a $null for a statement without a result set, so the output of every successful recovery started with a $null.
  • "You must specify a -Database or -Force for all databases" used $server.name as its target, which is only set further down.

Tests: a real log shipping secondary on the single instance (set up like in Invoke-DbaDbLogShipping.Tests.ps1) is recovered and must return exactly one result; the stop without -Database names the instance.

Install-DbaInstance

When Resolve-DbaNetworkName failed, $fullComputerName stayed empty, so Test-PendingReboot failed on the empty name and the warning read "Failed to get reboot status from " without a computer name. The command falls back to the name it was given now, as Update-DbaInstance does.

Test: one assertion in the existing "When the computer cannot be reached" context.

New-DbaComputerCertificate

The request folder was removed at the very end, after the certificate had been written to the pipeline, so a throw and a stopped pipeline (Ctrl+C, Select-Object -First) left it in the temp folder. The finally of every computer removes it now.

The -WhatIf item of #10776 turned out to be no defect: -WhatIf sets $WhatIfPreference inside the function, so New-Item and Set-Content never create the folder. The lab run against the old command confirmed it, so that checkbox can be ticked as "not a defect".

Also found while re-checking it, not in the issue: for a name that does not resolve, the full name was guessed from $ComputerName, which holds all the names of the call, so two names gave "first second.domain". It uses the name of the current computer now.

Tests: the existing pipeline-stop context checks that the request folder is gone (old command: it is left behind). A new -WhatIf context with -CaServer/-CaName (no Active Directory, certreq never runs) guards that nothing is left behind there either; it passes on the old command too. A third context passes two names that do not resolve, with -WhatIf, and checks that each guess names only its own computer.

Install-DbaSqlPackage

The download ran before the first ShouldProcess, so -WhatIf downloaded SqlPackage, and the verification afterwards warned that the installation had failed. The download has a ShouldProcess of its own now, and -WhatIf skips the verification.

Test: -WhatIf -Force into the test drive must download and install nothing, return nothing and not warn.

Save-DbaKbUpdate

Invoke-WebRequest reports a failed download as an error that ends only its own statement, so the command neither warned nor threw under -EnableException: the caller got a plain error record and no output. The download is in a try now, and the catch reports the link with Stop-Function and goes on with the next link.

Tests: a new context for the warning without -EnableException. The existing progress context provoked the throw with -ErrorAction Stop; the command handles the failure itself now, so it uses -EnableException.

New-DbaAvailabilityGroup (test only)

tests/New-DbaAvailabilityGroup.Tests.ps1 (#10764) treated a progress Id as completed when any completed record existed. Windows PowerShell writes its own completed record with Id 0 while it loads a module, so the check could pass while a bar was still open. The test now waits for the command's own activity and checks the last record of each Id, like the tests of #10773-#10775.

Verified

In the lab (single instance SQL Server 2019, the copy pair 2022/2025), on Windows PowerShell 5.1 and PowerShell 7.6. CI runs Pester on 5.1 only.

Test file 5.1 7.6
Sync-DbaAvailabilityGroup 11 passed 11 passed
Invoke-DbaDbDataGenerator 26 passed 26 passed
Invoke-DbaDbDataMasking 19 passed 19 passed
Invoke-DbaAdvancedRestore 19 passed 19 passed
Restore-DbaDatabase (caller) 86 passed, 3 skipped 86 passed, 3 skipped
Invoke-DbaAdvancedInstall 12 passed 12 passed
Invoke-DbaDbLogShipRecovery 6 passed (3 runs) 6 passed (2 runs)
Install-DbaInstance 62 passed 62 passed
New-DbaComputerCertificate 38 passed 38 passed
Install-DbaSqlPackage 15 passed 15 passed
Save-DbaKbUpdate 11 passed 11 passed
New-DbaAvailabilityGroup 6 passed 6 passed
dbatools.Tests.ps1 81 passed, 6 skipped -

No warnings in any of them. Against the ten old commands (5.1) the new assertions fail in every file, 27 in all, each for the reason it was written for (sessions left open, a $null or SQL in the output, leftovers in the table and in tempdb, the second backup file restored after a failure, one "Verify successful" instead of two, the empty computer name, the download under -WhatIf, no warning for the failed download, the request folder left behind). The -WhatIf context of New-DbaComputerCertificate is the one new test that passes on the old command, see above.

The first log shipping run on 7.6 failed in its setup: the setup creates the folder of the database below the share from the test session and then asks the instance whether it exists, and an instance that reaches the share over the network sees a new folder only seconds later. The test creates that folder itself now and waits until the instance sees it; the runs after that were green.

After the review fix (5.1): Invoke-DbaAdvancedRestore 21 passed, Restore-DbaDatabase 86 passed and 3 skipped, as before. With the Invoke-DbaAdvancedRestore.ps1 of the reviewed head f128350, the new context fails: one "Verify successful" instead of two, and the database was created by the restore of the second full backup.

Not in this PR

The other findings from re-checking these commands are in the follow-up PR #10792 (Restore-DbaDbSnapshot, the dedicated admin connections of the Copy commands, Start-DbaMigration and the Export commands) and in #10789 (Read-DbaBackupHeader). Invoke-Parallel on $Host and the bare Query() calls elsewhere are listed there as open.

🤖 Generated with Claude Code

andreasjordan and others added 14 commits October 17, 2026 14:09
…in the pipeline stop test

The test counted an Id as completed when any completed record of it existed.
Windows PowerShell writes a completed record with Id 0 of its own while it loads
modules, so an open bar of the command could pass the check. The test now waits
for a record of the command itself, checks the last record of each Id and imports
the manifest instead of the psm1, as the progress tests of #10773-#10775 do.

Red on the command before #10764 and green on development, on 5.1 and 7.6.

Refs #10776

(do New-DbaAvailabilityGroup)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… when a copy command stops

The dedicated admin connection opened for Copy-DbaCredential, Copy-DbaDbMail and Copy-DbaLinkedServer
was closed only after the three commands. A throw, Ctrl+C or Select-Object -First in between left it
open, and SQL Server allows only one dedicated admin connection per instance. The finally that
completes the progress bar now closes it too, when the command opened it itself.

Part of #10776.

(do Sync-DbaAvailabilityGroup)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… when the command stops early

The copy of the connection that the inserts use was closed after the try/finally around the
database loop. The returns inside it (failed read of the existing unique values, no unique value
found after 100 tries, no unique values in the dictionary), a throw under EnableException and a
stopped pipeline skipped the close and left one session open per call. The close moved into the
finally; a failing close warns there instead of calling Stop-Function, because a throw from a
finally would replace the error on its way out.

Part of #10776.

(do Invoke-DbaDbDataGenerator)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… SQL and $null to the pipeline

- The identity column, the masking index and the unique values table that the command adds per table
  were removed only at the end of the table loop. Every Stop-Function -Continue after the identity
  column was added (failed action or composite update, failed index, -ExcludeColumn on a unique
  index), the "no columns to process" continue, a throw under EnableException and a stopped
  pipeline left them in the user's table. They are removed in a finally now, and a failed removal
  warns instead of continuing, so a failed index drop no longer skips the column drop.
- tempdb.dbo.DeterministicValues is dropped in a finally around the work of the instance.
- "You cannot use both composites and actions" was followed by masking the table anyway. The table
  is skipped now.
- The UPDATE statements of the actions (on failure) and of the composites (on every run) were
  written to the pipeline, and the drop of a leftover DeterministicValues table wrote a $null.

Part of #10776.

(do Invoke-DbaDbDataMasking)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and verify every file with -VerifyOnly

- "Failed to restore db ..., stopping" was a Stop-Function -Continue followed by a break. Without
  EnableException the continue won, so the restore went on with the next backup file of the database
  whose restore had just failed. The -Continue is gone, so the break stops the database and the
  command goes on with the next one, as #5475 intended.
- -VerifyOnly returned the result of the first backup file with a return, which left the whole end
  block: the other backup files and databases were never verified, and a connection the command had
  opened itself was not closed. The result is written to the pipeline now.

Restore-DbaDatabase calls the command with EnableException, so the first item only affected direct
callers; -VerifyOnly affected both.

Part of #10776.

(do Invoke-DbaAdvancedRestore, Restore-DbaDatabase)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…le cannot be copied

- The failed copy of the configuration file to the remote computer ended the command with a bare
  return, unlike the other failures, so Install-DbaInstance got no result for that computer. It
  returns the result object with the reason in Notes now, removes the session and removes the local
  configuration file, whose cleanup sits in the finally of the installation that is never reached.
- "Failed to get reboot status from" used $computer, which the command never sets.

Part of #10776.

(do Invoke-DbaAdvancedInstall, Install-DbaInstance)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and name the instance in the error

- The RESTORE ... WITH RECOVERY ran through Query(), which returns a $null for a statement without a
  result set, so the output of every successful recovery started with a $null.
- "You must specify a -Database or -Force for all databases" used $server.name as its target, which
  is only set further down.

Part of #10776.

(do Invoke-DbaDbLogShipRecovery)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t be resolved

When Resolve-DbaNetworkName failed, $fullComputerName stayed empty, so Test-PendingReboot failed on
the empty name and the warning read "Failed to get reboot status from " without a computer name.
The command falls back to the name it was given now, as Update-DbaInstance does.

Part of #10776.

(do Install-DbaInstance)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…, also with -WhatIf

The request folder and its request.inf are created before any ShouldProcess, but the folder was
removed at the end inside a ShouldProcess. So -WhatIf left it in the temp folder, and so did a
throw under EnableException and a stopped pipeline. The finally of every computer removes it now.

Part of #10776.

(do New-DbaComputerCertificate)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The download ran before the first ShouldProcess, so -WhatIf downloaded SqlPackage, and the
verification afterwards warned that the installation had failed, because nothing was installed.
The download has a ShouldProcess of its own now, and -WhatIf skips the verification.

Part of #10776.

(do Install-DbaSqlPackage)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Invoke-WebRequest reports a failed download as an error that ends only its own statement, so the
command neither warned nor threw under EnableException: the caller got a plain error record and no
output. The download is in a try now, and the catch reports the link with Stop-Function and goes on
with the next link.

Part of #10776.

(do Save-DbaKbUpdate)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… the share before the log shipping setup

The setup creates that folder from the test session and then asks the instance whether it exists. An
instance that reaches the share over the network sees a new folder only some seconds later.

(do Invoke-DbaDbLogShipRecovery)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…fter a stopped pipeline

The -WhatIf case of #10776 was no defect: -WhatIf sets $WhatIfPreference in the function, so New-Item
and Set-Content never create the request folder. The real case is a stopped pipeline or a throw,
which skipped the removal after the output. The existing pipeline-stop context checks it now; the
-WhatIf context stays as a guard.

(do New-DbaComputerCertificate)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ts own name

For a name that does not resolve, the full name was guessed from $ComputerName, which holds all the
names of the call, so two names gave "first second.domain".

(do New-DbaComputerCertificate)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@potatoqualitee potatoqualitee left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at f12835092b365ae2161423bd06863b0bfe87efc2. Requesting changes for one material data-safety defect: continuing verification through multiple backups exposes a recovery-options branch that executes RESTORE before checking VerifyOnly.

I independently executed the exact dispatch branches from the base and reviewed head under PowerShell 7.6.7 with a recording SQL boundary. With two full backups and VerifyOnly + KeepCDC, the base only verified the first file; the head verified the first file and submitted RESTORE DATABASE for the last file. The normal Restore-DbaDatabase caller permits and forwards these options. With WithReplace this can overwrite an existing target during a requested integrity check.

The complete 21-file patch, surrounding source, tests, descriptions, discussions, and CI were reviewed. All 20 executed head checks passed and one Windows check was skipped. The new verification test does not combine multiple backups with recovery options. The reproduction was a focused control-flow check; no database was contacted or modified.

Write-Message -Message "VerifyOnly restore Succeeded" -Level Verbose
$restoreComplete = $true
return "Verify successful"
"Verify successful"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Keep all newly reached backups in VerifyOnly mode

Removing the return lets verification reach the last backup, but the dispatch above checks ($KeepCDC -or $ErrorBrokerConversations) -and -not $restore.NoRecovery before VerifyOnly. For two full backups of the same database, the first has NoRecovery=true and is verified; the last has NoRecovery=false and enters the earlier branch, which calls ConnectionContext.ExecuteNonQuery with real RESTORE SQL.

A concrete path is Restore-DbaDatabase over a directory containing multiple full backups with -VerifyOnly -KeepCDC (or Invoke-DbaAdvancedRestore with those backup records). Restore-DbaDatabase retains all records in verification mode and forwards these options. If the existing restore splat also includes -WithReplace, an idle existing target can be overwritten rather than merely verified.

I independently executed the exact base/head dispatch branches using a recording boundary: base recorded only VERIFY first-full.bak; head recorded that followed by EXECUTE RESTORE DATABASE [Probe] FROM DISK='last-full.bak' WITH RECOVERY, REPLACE, KEEP_CDC.

Make VerifyOnly take precedence over every branch that can execute restore SQL while retaining verification of all files. Add a multi-backup regression with KeepCDC or ErrorBrokerConversations that checks every file is verified and no target database is created, restored, or recovered.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, thanks. Fixed in fffabc7: the branch for -KeepCDC/-ErrorBrokerConversations and the page restore branch now require -not $VerifyOnly, so a verification never executes a restore statement. It was reachable before this PR as well: a single full backup is the last file and recovers, so -VerifyOnly -KeepCDC took that branch for it.

The new context "When -VerifyOnly with -KeepCDC verifies two full backups" builds the history the way Restore-DbaDatabase hands it over for -VerifyOnly, without the selection, with a new database name and new file names, and checks that both files are verified and that no database exists afterwards.

Lab, Windows PowerShell 5.1, InstanceSingle SQL Server 2019: Invoke-DbaAdvancedRestore.Tests.ps1 passes 21 of 21, and Restore-DbaDatabase.Tests.ps1, its caller, passes 86 with 3 skipped (the Azure contexts), as before. With the Invoke-DbaAdvancedRestore.ps1 of f128350, the new context fails exactly as you describe: one "Verify successful" instead of two, and the restore of the second full backup created the database.


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

The branch for -KeepCDC and -ErrorBrokerConversations and the branch for page restores came before
the one for -VerifyOnly and execute a restore statement of their own. Now that the verification goes
on to the last backup file, it reached the one that recovers: two full backups with -VerifyOnly
-KeepCDC verified the first and restored the second, with -WithReplace over an existing database.
A single full backup took that branch before as well. Both branches now leave out -VerifyOnly.

The new context verifies two full backups with -KeepCDC under a new database name and checks that
both are verified and no database was created.

(do Invoke-DbaAdvancedRestore)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@potatoqualitee potatoqualitee left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at fffabc709d36d01d641b59e70444fdc1bc9a37be.

The previous VerifyOnly blocker is fixed. Both the recovery-options branch and the page-restore branch now exclude VerifyOnly before executing RESTORE. A focused PowerShell 7.6.7 probe of the actual dispatch code with two backups verified that KeepCDC, ErrorBrokerConversations, Pages, and the ordinary verification path all verify both backups and issue no ExecuteNonQuery calls. The new integration regression also requires that verification creates no destination database.

Reviewed the complete 21-file patch, surrounding source, tests, PR description, existing discussions, cleanup and error paths, and CI. No remaining material defect found. All 21 changed PowerShell files parse successfully. The exact-head check-run inventory has 20 successful checks and three skipped checks. SQL Server and Windows integration suites were not rerun locally.

@potatoqualitee
potatoqualitee merged commit b6d1354 into development Oct 22, 2026
24 checks passed
@potatoqualitee
potatoqualitee deleted the side-findings-10776-fixes branch October 22, 2026 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants