Skip to content

Invoke-DbaDbLogShipping - avoid Azure blob name collision - #10679

Draft
simonyang08 wants to merge 1 commit into
dataplat:developmentfrom
simonyang08:codex/dbatools-10667-azure-unique
Draft

simonyang08 wants to merge 1 commit into
dataplat:developmentfrom
simonyang08:codex/dbatools-10667-azure-unique

Conversation

@simonyang08

Copy link
Copy Markdown

Issue

#10667: the Azure log shipping integration test failed on PR #10665 with Cannot open backup device ... Operating system error 50 even though #10665 only changed three SSIS message strings. The reason was that three PRs (#10664, #10665, #10666) hit the same Invoke-DbaDbLogShipping call inside the same wall-clock second, all produced the same FullBackup_PreLogShipping_<yyyyMMddHHmmss>.bak blob name, and the second runner's upload lost the race.

Root cause

Two cooperating causes:

  1. public/Invoke-DbaDbLogShipping.ps1 builds the Azure timestamp with second resolution (Get-Date -Format "yyyyMMddHHmmss").
  2. .github/scripts/gh-actions.ps1 pins both test database names (dbatoolsci_logship_azure, dbatoolsci_logship_addsecondary) for every run, so concurrent jobs and reruns share database and blob names.

Fix

  • Production: the Azure timestamp now uses millisecond precision (yyyyMMddHHmmssfff). Two callers inside the same second produce different blob names regardless of how the caller picks the database. The other Get-Date calls in the file are schedule start dates that never touch file names.
  • CI: both test database names are scoped with GITHUB_RUN_ID (+ GITHUB_RUN_ATTEMPT for reruns), so concurrent jobs and reruns never share a database or a blob. The --prefix cleanup continues to work because the prefix is still rooted in dbatoolsci_logship_azure / dbatoolsci_logship_addsecondary.

Tests

New tests/Invoke-DbaDbLogShipping.Azure.Tests.ps1 (Pester 6, -Tag UnitTests) reads the production source and the CI script and asserts the millisecond specifier and the per-run GITHUB_RUN_ID scoping. These assertions catch a future revert to second resolution or a hardcoded database name without needing the Azure fixture, matching the source-content style already used in dbatools.Tests.ps1 compliance checks.

Verified locally: the new spec is green under Pester 6.1.0, and the existing Invoke-DbaDbLogShipping / Remove-DbaDbLogShipping specs still pass with the module imported.

Scope

3 files, 92 insertions, 3 deletions. No new parameters, no new dependencies.

Fixes #10667

…0667)

When Invoke-DbaDbLogShipping backs up to Azure blob storage it builds the
backup file name from a Get-Date timestamp with second resolution. Two CI
runs that hit the cmdlet within the same wall-clock second produced the
same FullBackup_PreLogShipping blob name on the shared Azure container, so
the second run failed with 'Cannot open backup device ... Operating system
error 50' even when nothing in the PR was actually wrong.

Two minimal changes close the bug:

  * Production: build the Azure timestamp with millisecond precision
    (yyyyMMddHHmmssfff). Two callers that enter the cmdlet inside the
    same second now produce different blob names, regardless of how the
    caller picks the database name.
  * CI: scope the dbatoolsci_logship_azure database name with
    GITHUB_RUN_ID and GITHUB_RUN_ATTEMPT in .github/scripts/gh-actions.ps1,
    so concurrent linux-tests jobs (and reruns) never reuse the same
    database or blob.

The Azure integration test still needs a real SQL Server plus the shared
Azure container to actually run, so the regression is asserted on the
shape of the inputs that drive the blob name: a new unit test reads the
production source and the CI script and verifies the timestamp carries
the millisecond 'fff' format specifier and the database name carries
GITHUB_RUN_ID. This catches a future revert to second resolution or a
revert to the hard-coded database name without requiring the Azure
fixture.

Signed-off-by: simonyang08 <ppt5928@gmail.com>
@andreasjordan

Copy link
Copy Markdown
Collaborator

Thanks for picking this up. I filed #10667, so here is a review from that side. Short version: the change to gh-actions.ps1 is the fix and is right; the command change and the new test file should go.

.github/scripts/gh-actions.ps1 - good

Both Azure test database names get the run id and attempt suffix, which is exactly what the issue proposed. Nothing else uses the names literally, and the blob cleanup lists by --prefix $dbName, so it follows along. Two things to be aware of:

  • The cleanup now sweeps only its own run's blobs. The first Azure test runs its cleanup after the assertions without a finally, so a run that fails there leaves its blobs in the shared container for good. Before, the next run's broad prefix deleted them - which was itself racy against a concurrent run, so the narrowing is the right call, but orphaned blobs will accumulate over time.
  • Nothing in CI has executed this yet: integration-tests.yml triggers on push only, not on pull_request, and the fork's ci-azure job is queued in its own pool. The Azure path runs for the first time on the development push after the merge. That is fine for a change of this size, just worth knowing.

public/Invoke-DbaDbLogShipping.ps1 - please drop

$Timestamp is built once and also names the on-prem backup file (FullBackup_<db>_PreLogShipping_<ts>.bak, line 1691), so the millisecond format changes user-visible file names for every log shipping setup, not only Azure as the description says. The issue said no command change is needed: the run-unique database name is what removes the race, a finer timestamp only makes the window smaller. If you want to keep it, the description has to say what it changes for on-prem users.

tests/Invoke-DbaDbLogShipping.Azure.Tests.ps1 - please drop

tests/CLAUDE.md rules out source, AST and text assertions as coverage unless explicitly approved, and this file is exactly that: it reads the command source and the CI script as text and asserts on regexes. It also assumes the repository layout ($PSScriptRoot/.. to reach .github/scripts), it fails the header step of the layout check the test files follow (comment block before param, #Requires spacing, Describe "$CommandName - ..."), and the timestamp assertion passes for any Get-Date format in the file that contains fff, not for the blob name line. The dbatools.Tests.ps1 precedent is a module-level compliance file, not a per-command test. The CI script change is proven by the CI run itself, that is the test here.

Minor

  • The commit headline ends with (#10667), which will read as a pull request reference after the squash; the issue belongs in the body as Fixes #10667, which the description already has.
  • The branch is nine commits behind development, merges cleanly.

created by Claude and reviewed by Andreas Jordan

@andreasjordan

Copy link
Copy Markdown
Collaborator

A personal question, @simonyang08: Are you a user of dbatools or just someone (or an AI bot) looking for things to fix?

@potatoqualitee - If you agree I would open a separate pr with the changes to .github/scripts/gh-actions.ps1 and close this pr.

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.

CI: the Azure log shipping test collides across concurrent runs because its backup name is only second-unique

3 participants