Repository navigation
Restore-DbaDbSnapshot, dedicated admin connection - Fix the follow-up findings of #10776 in 7 commands - #10792
andreasjordan wants to merge 13 commits into
Conversation
…deadlock Only a deadlock counts the tries of the retry loop, and the Stop-Function -Continue after any other failure continued that loop, so without EnableException the same failing restore ran again and again without end (the break after it was unreachable). The labeled -Continue goes on with the next snapshot now. Also fixes the typo Failiure in both messages. (do Restore-DbaDbSnapshot) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The dedicated admin connection opened for the passwords was closed in the end block only. A throw under EnableException, Ctrl+C and Select-Object -First skip it, and so did the throw of the failed decryption in begin. SQL Server allows only one dedicated admin connection per instance. The work of process is in a try now whose finally closes it, and the failed decryption closes it before it stops. (do Copy-DbaCredential) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The dedicated admin connection opened for the passwords of the mail accounts was closed in the end block only, which a throw under EnableException, Ctrl+C and Select-Object -First skip. SQL Server allows only one dedicated admin connection per instance. The work of process is in a try now whose finally closes it. (do Copy-DbaDbMail) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The dedicated admin connection opened for the passwords was closed in the end block only, which a throw under EnableException, Ctrl+C and Select-Object -First skip. SQL Server allows only one dedicated admin connection per instance. The work of process is in a try now whose finally closes it. (do Copy-DbaLinkedServer) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lly of the migration The dedicated admin connection was closed in the end block only, which a throw under EnableException, Ctrl+C and Select-Object -First skip, and the two connection failures after it was opened threw past it under EnableException. SQL Server allows only one dedicated admin connection per instance. The finally that completes the progress bar closes it now, and the two failures close it before they stop. (do Start-DbaMigration) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… there is nothing to export The disconnect sat at the end of the loop over the instances, so the continue for nothing to export, the -Continue for a failed write, a throw under EnableException and a stopped pipeline skipped it and left the dedicated admin connection open. The rest of the loop body is in a try now whose finally disconnects. (do Export-DbaCredential) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…en there is nothing to export The disconnect sat at the end of the loop over the instances, so the continue for nothing to export, the -Continue for a failed decryption or write, a throw under EnableException and a stopped pipeline skipped it and left the dedicated admin connection open. The rest of the loop body is in a try now whose finally disconnects. (do Export-DbaLinkedServer) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ot restore A session that holds the database makes the restore wait for its lock for an unpredictable time before it fails, so the test could time out on the fixed command too. A login with a user in the database and its snapshot but without permission to restore makes the restore fail at once. (do Restore-DbaDbSnapshot) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e as the trigger The login without restore permission does not see the snapshot, so the command ends before the restore. The session that holds the database is a deterministic trigger after all: the restore waits about 20 seconds for its lock and then fails with error 5070. The earlier zero warnings on the old command came from the filter, which did not match the old spelling Failiure. (do Restore-DbaDbSnapshot) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
potatoqualitee
left a comment
potatoqualitee
left a comment
There was a problem hiding this comment.
Reviewed at ac17b73a2978ae136619b7e93778c51bb4a9ab41. Requesting changes for one remaining resource leak in the interruption behavior this PR intends to fix: Copy-DbaCredential can still abandon its owned DAC when cancelled during source credential decryption in begin.
I independently ran the exact command source under PowerShell 7.6.7 with an in-memory connection/decryption boundary, paused after decryption started, and stopped the runspace. The DAC-open flag was true, the disconnect flag false, and the added catch was not entered. This is an incomplete fix of the existing lifetime bug, not a newly introduced regression. Get-DecryptedObject performs real SQL and remote host work at this point, making cancellation here a realistic path.
The complete patch, surrounding source, tests, description, discussions, and CI were reviewed. All 20 executed head checks passed and one Windows check was skipped. The new Copy-DbaCredential test stops after its first output, once process has started, and does not exercise this earlier cancellation. No SQL Server integration suite was rerun locally.
| DateTime = [DbaDateTime](Get-Date) | ||
| # The dedicated admin connection is closed in the finally, not only in end: a throw under EnableException, Ctrl+C | ||
| # and Select-Object -First skip the end block, and SQL Server allows only one dedicated admin connection per instance. | ||
| try { |
There was a problem hiding this comment.
[P2] Protect the owned DAC during begin-stage decryption too
The DAC is opened in begin (176-177), then Get-DecryptedObject runs at 191 before this protected process scope begins. Ctrl+C or PowerShell.Stop() during that SQL/remoting work bypasses the decryption catch, process never starts, and end is skipped. Consequently this finally cannot close the owned DAC, leaving the source instance's single admin connection occupied.
I independently reproduced that path using the exact command source in a stopped PowerShell 7.6.7 runspace: Opened=true, DecryptEntered=true, Closed=false, StopFunctionReached=false. The production Get-DecryptedObject performs SQL queries and Invoke-Command2 during this interval. The existing new test stops after the first copy result, so it only covers cancellation inside process.
Protect the entire owned-DAC lifetime, including initialization/decryption, with cancellation-safe finally cleanup. Since this command takes no pipeline input, moving that work into the protected process scope is one option. Preserve caller-supplied DACs and add a regression that cancels during decryption before any result is emitted.
There was a problem hiding this comment.
Confirmed, thanks. Fixed in 61a8c7c the way you suggested: the command takes no pipeline input, so the connection, the decryption and the list of credentials moved from begin into process, inside the try whose finally closes the connection. The catch of the decryption no longer closes it itself, and the end block is gone. A dedicated admin connection the caller passes in is still reused and not closed.
The new context "When the command is stopped while it decrypts the credentials" creates a credential in an open transaction of the test, which blocks the decryption query on the dedicated admin connection. The test waits until that session is blocked, requests the stop of the command in its own runspace, ends the transaction, and checks that no session is left on the admin endpoint.
Copy-DbaDbMail and Copy-DbaLinkedServer opened theirs in begin too. Nothing there talks to the server after the open, but a stop in that moment would leave the connection open just the same, so 26e9256 and 2ae702a give both the same structure.
Lab, the copy pair SQL Server 2022/2025: Copy-DbaCredential.Tests.ps1 10 of 10, Copy-DbaDbMail.Tests.ps1 15 of 15 and Copy-DbaLinkedServer.Tests.ps1 5 of 5, each on Windows PowerShell 5.1 and on PowerShell 7.6. dbatools.Tests.ps1, which enforces the command structure rules, passes 81 with 6 skipped, as before. With the Copy-DbaCredential.ps1 of ac17b73, the new test fails with one session left on the admin endpoint.
This text was created by Claude and reviewed by Andreas Jordan.
…inside the finally The connection was still opened in begin, which also decrypted the credentials through it. A stop during the decryption skipped process and end, so the finally added to process never ran and the only dedicated admin connection of the instance stayed open. The command takes no pipeline input, so process runs once: the connection, the decryption and the list of credentials moved there, inside the try whose finally closes the connection. The catch of the decryption no longer closes it itself, and the end block is gone. The new context blocks the decryption query with a credential created in an open transaction, stops the command in its own runspace and checks that no dedicated admin connection is left. (do Copy-DbaCredential) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…de the finally As in Copy-DbaCredential, the connection was opened in begin, and a stop between the open and the try in process skipped the finally that closes it. Nothing there talks to the server, but the connection is now opened in the try itself, so its whole lifetime is covered. The command takes no pipeline input, so process runs once, and the end block is gone. (do Copy-DbaDbMail) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…, inside the finally As in Copy-DbaCredential, the connection was opened in begin, and a stop between the open and the try in process skipped the finally that closes it. Nothing there talks to the server, but the connection is now opened in the try itself, so its whole lifetime is covered. The command takes no pipeline input, so process runs once, and the end block is gone. (do Copy-DbaLinkedServer) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
potatoqualitee
left a comment
potatoqualitee
left a comment
There was a problem hiding this comment.
Reviewed at 2ae702a3de713971c4e708a4bedd3a6cf61ad26d. The previous Copy-DbaCredential cancellation blocker is fixed.
Requesting changes for one remaining material cleanup defect in Start-DbaMigration: cancellation while establishing the additional normal source connection can still abandon the command's already-open dedicated admin connection, before the new finally is active. This is an incomplete fix of the existing lifetime problem addressed by this PR.
I independently ran the exact-head Start-DbaMigration source under PowerShell 7.6.7 with an in-memory connection boundary, paused in the second connection call after the DAC opened, and stopped the runspace. The result was DAC opened=true, normal connection entered=true, disconnect calls=0, catch reached=false; an outer caller finally did run, and the error stream was empty. The normal connection performs real SQL work in Connect-DbaInstance, so this is a realistic cancellation interval. No live SQL Server was contacted.
The complete 14-file patch, surrounding source, tests, PR description, existing discussions, ownership/cleanup and multi-input paths were reviewed. All 14 changed PowerShell files parse successfully. The complete exact-head check-run inventory has 20 successful checks and three skipped checks. SQL Server and Windows integration suites were not rerun locally.
| # A throw under EnableException, Ctrl+C and Select-Object -First skip the end block, and SQL Server allows only | ||
| # one dedicated admin connection per instance. | ||
| if ($dacOpened) { |
There was a problem hiding this comment.
[P2] Cover the DAC lifetime during normal-connection initialization
This finally begins too late to cover the owned DAC's entire lifetime. The command opens sourceServerDac at line 387, marks dacOpened at 392, then performs the additional normal Connect-DbaInstance at 394. The initialization block has catch cleanup but no finally; this protected migration block does not start until line 425.
Concrete path: start a password-enabled migration from a source name and cancel while the additional normal connection is being established. PowerShell cancellation bypasses the initialization catch, never reaches this later try, and skips end. The DAC remains open. Connect-DbaInstance actually establishes/validates the normal SQL connection at its ExecuteWithResults call, so cancellation while that work is slow or blocked is realistic.
I independently stopped a runspace running the exact command source after the DAC returned and the normal connection began: the DAC-open flag was true, disconnect calls were zero, catch was not entered, and a caller's enclosing finally did run. SQL Server allows only one DAC per instance, so retaining it can block later administration or password migration.
Enclose the entire owned-DAC lifetime, including normal-connection initialization, in try/finally while retaining the ownership guard for caller-supplied DACs. Add cancellation coverage in this initialization interval. The new test waits for migration progress, after this gap.
There was a problem hiding this comment.
Confirmed, thanks. Fixed in c16c613: the connections and the migration are in one try now, and its finally closes the dedicated admin connection the command opened, so the whole lifetime from the open to the end of the migration is covered. The catch of the connections stays for its error message; the closes I had added to the connection branches are gone, the finally does them. A dedicated admin connection the caller passes in is still left open.
The new context "When the pipeline is stopped while the normal source connection is opened" makes that interval long enough to hit: a logon trigger on the source holds every login of a test login for eight seconds. Logon triggers do not fire for the dedicated admin connection, so the command gets that one at once and then waits in the trigger with the normal one. The test waits until the session of the normal login shows WAITFOR on the server, stops the command in its own runspace, and counts the sessions of the test login on the admin endpoint, which must be 0.
Lab, the copy pair SQL Server 2022/2025: Start-DbaMigration.Tests.ps1 19 passed on Windows PowerShell 5.1 and on PowerShell 7.6. With the Start-DbaMigration.ps1 of 2ae702a, the new test fails with one session left on the admin endpoint.
This text was created by Claude and reviewed by Andreas Jordan.
…hile the normal connection opens The command opens the dedicated admin connection first and the normal source connection after it, in a try whose catch closed the dedicated one on an error. A stopped pipeline skips catch blocks, and the finally that closes the connection only began with the migration. A stop while the normal connection was opened therefore left the dedicated admin connection open, and SQL Server allows only one per instance. One try with that finally now covers the connections and the migration; the closes in the connection branches are gone, the finally does them. The new context holds the normal logins of a test login in a logon trigger on the source, which does not fire for the dedicated admin connection, and stops the pipeline while the normal connection waits there. (do Start-DbaMigration) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Follow-up of #10776: defects found while re-checking the commands of #10789 and #10791 that were not in the issue. One commit per command.
Restore-DbaDbSnapshot: a failed restore ran again without end
The restore retries on a deadlock (error 1205), and only a deadlock counts the tries. Any other failure ran
Stop-Function -Continueinside the retry loop, so without-EnableExceptionthecontinuere-ran the same failing restore, again and again, with a warning each time. Thebreakafter it was unreachable. A labeled-Continuegoes on with the next snapshot now. Also fixes the typo "Failiure" in both messages.Test: a session that stays in the database keeps the restore from getting exclusive access. The restore waits about 20 seconds for its lock and then fails with error 5070, which is no deadlock. The command runs in a runspace of its own and is stopped after a minute, so that a regression cannot hold up the test run. It has to finish and warn exactly once. In the lab, the old command warned every 20 seconds and was still running after 90.
The dedicated admin connection outlived a stopped command
SQL Server allows only one dedicated admin connection per instance, so one that stays open blocks the next command that needs it.
beginand closed it inendonly. A throw under-EnableException, Ctrl+C andSelect-Object -Firstskipend. None of them takes pipeline input, soprocessruns once: the connection is opened there now, inside atrywhosefinallycloses it, and theendblocks are gone. Copy-DbaCredential also decrypts its credentials and reads the list of them in thattry. A dedicated admin connection the caller passes in is reused and not closed, as before. (The first version of this PR kept the open inbegin, so a stop during the decryption still left the connection open, see "After review" below.)endonly. Onetrynow covers the connections and the migration, and itsfinally, which also completes the progress bar, closes it. (The first version began thattrywith the migration, so a stop while the normal connection was opened still left the connection open, see "After review" below.)continuefor "Nothing to export" (an instance without credentials or linked servers, or a filter that matches none), the-Continuefor a failed decryption or write, a throw and a stopped pipeline skipped that. The rest of the loop body is in atrynow, whosefinallydisconnects.Most of the diff in these files is indentation;
git diff -wshows the real change.Tests: each command gets a context that ends the run early (
Select-Object -First 1for the Copy commands, a stopped pipeline for Start-DbaMigration with-WhatIf, a filter that matches nothing for the Export commands) and then counts the sessions on the admin endpoint of the source, which must be 0.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.
No warnings in any of them. Against the old commands (5.1), every new assertion fails: one session left on the admin endpoint for the six connection commands, and Restore-DbaDbSnapshot still running after a minute with two warnings. Run in one process, the old commands also show what a leaked dedicated admin connection does: the commands after it that need one failed with "An existing connection was forcibly closed by the remote host".
The last two commits of Restore-DbaDbSnapshot are test-only. I first replaced the session that holds the database with a login that may not restore, because the old command had shown no warning in a minute. That login does not see the snapshot, and the missing warning came from my filter, which did not match the old spelling "Failiure". The session that holds the database is back, and the filter matches both spellings.
After review: the dedicated admin connection during the decryption (61a8c7c, 26e9256, 2ae702a)
The first version opened the connection in
beginand decrypted the credentials there, before the protectedprocess. A stop during the decryption, which runs SQL and remote host work, skippedprocessandend, and the connection stayed open. Copy-DbaCredential now opens the connection, decrypts and lists the credentials inprocess, inside thetrywhosefinallycloses it. Copy-DbaDbMail and Copy-DbaLinkedServer ran nothing against the server inbeginafter the open, but a stop in that moment left the connection open just the same, so both got the same structure.New context in
Copy-DbaCredential.Tests.ps1: a credential created in an open transaction of the test blocks the decryption query on the dedicated admin connection. The test waits until that session is blocked, requests the stop of the command in its own runspace, ends the transaction, and checks that no session is left on the admin endpoint. It kills a session the command left there, so a regression does not break the tests after it.Lab, the copy pair SQL Server 2022/2025:
Copy-DbaCredential.Tests.ps110 passed,Copy-DbaDbMail.Tests.ps115 andCopy-DbaLinkedServer.Tests.ps15, each on Windows PowerShell 5.1 and on PowerShell 7.6, no warnings;dbatools.Tests.ps181 passed and 6 skipped. With theCopy-DbaCredential.ps1of the reviewed head ac17b73, the new test fails with one session left on the admin endpoint (the test then kills it).After review: Start-DbaMigration while the normal connection is opened (c16c613)
Start-DbaMigration opens the dedicated admin connection first and the normal source connection after it. The
finallythat closes it began with the migration, and a stopped pipeline skips the catch of the connections, so a stop while the normal connection was opened left the dedicated one open. Onetrywith thatfinallynow covers both; the closes in the connection branches are gone.New context in
Start-DbaMigration.Tests.ps1: a logon trigger on the source holds every login of a test login for eight seconds. It does not fire for the dedicated admin connection, so the command opens that one at once and then waits in the trigger with the normal one. The test stops the command at that point and counts the sessions of the test login on the admin endpoint.Lab, the copy pair SQL Server 2022/2025:
Start-DbaMigration.Tests.ps119 passed on Windows PowerShell 5.1 and on PowerShell 7.6,dbatools.Tests.ps181 passed and 6 skipped. With theStart-DbaMigration.ps1of the reviewed head 2ae702a, the new test fails with one session left on the admin endpoint.Not in this PR
private/functions/flowcontrol/Invoke-Parallel.ps1creates its pool on$Hosttoo, with the same effect on API hosts as Read-DbaBackupHeader, Start-/Stop-DbaDbEncryption - Run the threads in a pool without a host and close it in a finally #10789 fixes. There the threads write warnings and progress that a console shows live today (Install-/Update-DbaInstance with two or more computers, Start-/Stop-/Restart-DbaService), and Invoke-Parallel forwards only their errors. Without$Hostit would have to forward the warnings when a thread ends, and the progress of the threads would no longer show live. That is a behaviour change to decide first.Query()with a statement that returns no result set and no$null =in front, so they write a$nullto the pipeline like the two fixed in Cleanup, output and -WhatIf - Fix the side findings of the progress work in 10 commands #10791. That needs a sweep with a look at every site, in a PR of its own.🤖 Generated with Claude Code