Skip to content

Restore-DbaDatabase - Stop escaping the caller and resurrect the StopAt tests - #10636

Open
andreasjordan wants to merge 1 commit into
developmentfrom
fix-restoredbadatabase-stopat-fixture
Open

Restore-DbaDatabase - Stop escaping the caller and resurrect the StopAt tests#10636
andreasjordan wants to merge 1 commit into
developmentfrom
fix-restoredbadatabase-stopat-fixture

Conversation

@andreasjordan

Copy link
Copy Markdown
Collaborator

Summary

Two fixes that belong together: a flow-control defect in Restore-DbaDatabase that has corrupted callers since at least 2020, and the resurrection of the StopAt tests whose failure it made undiagnosable.

The command defect

Two Stop-Function calls in the end block carried -Continue although no loop encloses them (the catch around Test-DbaBackupInformation and the catch around Invoke-DbaAdvancedRestore). Stop-Function -Continue executes PowerShell's continue, and with no enclosing loop in the function, that flow-control unwinds out of the command into the caller:

  • a user restoring in a foreach over instances silently skips their next iteration when one restore fails without -EnableException;
  • under Pester, it corrupts the runner's internal loops and surfaces as Cannot bind argument to parameter 'ErrorRecord' because it is null - the exact crash the old TODO comment above the skipped StopAt tests documented as "reason currently unknown".

Both sites now Stop-Function + return, with comments stating why -Continue is forbidden there. Reproduced before the fix with a plain script: a marker line after the restore call was never reached while the finally still ran - the signature of escaped flow control, not of an exception.

The StopAt tests

Skipped since the 2025 Pester rewrite because their static fixture (appveyor-lab\sql2008-backups\StopAt) was broken from its very first 2020 commit - StopAt_22.trn never existed in git, and regenerating 2008-era files would need a SQL Server 2008 instance. The tests now generate their own chain in BeforeAll: a steps table, two marked transactions named dbatoolstest (STOPATMARK references the transaction name; the WITH MARK string is only a description - the first rewrite attempt proved this the hard way), a timestamp captured between the marks for StopAfterDate, and assertions that follow the generated layout instead of the old magic values.

Semantics pinned: stop at mark includes the marked transaction (step 2), -StopBefore excludes it (step 1), -StopAfterDate with a between-marks timestamp selects the second mark (step 4). A mark in the last log file recovers the database in the same statement, so no separate -Recover may follow it - encoded as a comment.

Deliberately not changed

After stopping at a mark mid-chain, the command still attempts the remaining log files, which SQL Server refuses ("log too recent to apply") - with -EnableException a successful stop-at-mark therefore still throws on the tail file. Whether the restore loop should end gracefully once the stop is reached is a design question worth its own issue.

Same file as #10631, different hunks (this end-block region vs. the backup-information splats); the branches merge cleanly in either order.

Tests

Lab harness on SQL03\SQL2019: 81 tests, 77 passed, 0 failed, 3 skipped (Azure contexts), no warnings. Before the flow-control fix, the same file aborted the entire Pester run with the null-ErrorRecord crash - which doubles as the red-on-old proof.

created by Claude and reviewed by Andreas Jordan

🤖 Generated with Claude Code

…At tests

Two Stop-Function calls in the end block carried -Continue although no
loop encloses them. When a restore or its verification failed without
-EnableException, the continue unwound out of the command and consumed
an iteration of whatever loop the caller was running in - a caller
restoring in a foreach silently skipped their next instance, and under
Pester it corrupted the test runner, surfacing as "Cannot bind argument
to parameter ErrorRecord because it is null". That crash is what kept
the StopAt tests undiagnosable since 2020. Both sites now stop and
return.

The StopAt tests themselves were skipped because their static fixture
was broken from its first commit - StopAt_22.trn never made it into the
appveyor-lab repo. They now generate their own chain in BeforeAll: two
transactions named dbatoolstest carry marks, with a timestamp captured
between them, and the assertions follow the layout of the generated
steps table instead of magic values. Two traps encoded in comments:
STOPATMARK references the transaction name, not the WITH MARK
description, and a mark in the last log file recovers the database in
the same statement, so no separate -Recover may follow.

Verified via the lab harness on SQL03\SQL2019: 81 tests, 77 passed,
0 failed, 3 skipped (the Azure contexts), no warnings. Before the
flow-control fix the same file aborted the whole Pester run with the
null-ErrorRecord crash.

(do Restore-DbaDatabase)

Co-Authored-By: Claude Fable 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 the complete current head, both end-block control-flow fixes, generated StopAt chain and assertions, cleanup, and exact-head CI evidence. No material correctness or compatibility issues found.

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