Skip to content

fix(hubs): never lower retention on redeploy #2288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Michael Flanakin (flanakin) wants to merge 2 commits into
base: dev
Choose a base branch
from
+68 −3
Open

fix(hubs): never lower retention on redeploy #2288

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89eddbb
fix(hubs): never lower retention on redeploy
flanakin Aug 26, 2026
845afc8
docs: add changelog entry for retention never-shrink fix
flanakin Sep 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions src/powershell/Tests/Unit/HubsRetentionGuard.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.

<#
Regression coverage for the settings.json retention guard (#2206):
Copy-FileToAzureBlob.ps1 unconditionally overwrote retention.ingestion.months / retention.final.months
with whatever the deploymentScript's Bicep parameters passed in. Bicep always resolves a value for an
optional parameter (defaulting to 13 if the caller didn't specify one), so the script cannot tell an
explicit redeploy value from a silently-defaulted one. A redeploy that omitted a previously-customized
retention value therefore silently reset it to the toolkit default, and the next purge pipeline run
aged out historical data older than the new (lower) cutoff -- oldest data first.

The fix: never lower stored retention on redeploy. Growing retention is always safe; shrinking it has a
destructive, hard-to-reverse consequence (data purge), so the script now takes the max of the stored
value and the incoming one instead of overwriting unconditionally.
#>

Describe 'HubsRetentionGuard' {

BeforeAll {
$repoRoot = (Resolve-Path "$PSScriptRoot/../../../..").Path
$scriptPath = Join-Path $repoRoot 'src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Core/Copy-FileToAzureBlob.ps1'
$content = Get-Content -Path $scriptPath -Raw
}

Context 'Never-shrink guard' {

It 'Should take the max of stored and incoming ingestion retention' {
$content | Should -Match '\$json\.retention\.ingestion\.months\s*=\s*\[Math\]::Max\(\$json\.retention\.ingestion\.months,\s*\[Int32\]::Parse\(\$env:ingestionRetentionInMonths\)\)' `
-Because 'a redeploy that omits an explicit retention value must not silently shrink stored retention and purge historical data (#2206)'
}

It 'Should take the max of stored and incoming final retention' {
$content | Should -Match '\$json\.retention\.final\.months\s*=\s*\[Math\]::Max\(\$json\.retention\.final\.months,\s*\[Int32\]::Parse\(\$env:finalRetentionInMonths\)\)' `
-Because 'a redeploy that omits an explicit retention value must not silently shrink stored retention and purge historical data (#2206)'
}

It 'Should not unconditionally overwrite ingestion retention' {
$content | Should -Not -Match '\$json\.retention\.ingestion\.months\s*=\s*\[Int32\]::Parse\(\$env:ingestionRetentionInMonths\)\s*$' `

@RolandKrummenacher Roland Krummenacher (RolandKrummenacher) Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These two negative assertions (this one and the final equivalent on line 44) can never fail, so the regression they're guarding is unprotected.

$ here is end-of-string — -match doesn't set RegexOptions.Multiline — and the trailing \s* can't span the code that follows the assignment. So the pattern only matches if that assignment happens to be the last thing in the file. I checked against a sample with the regression pattern deliberately present mid-file:

PR2288 anchored pattern (\s*$) matches : False
same pattern without the anchor        : True

Both tests pass today, and they'd still pass if someone reverted the fix. Dropping \s*$ from both patterns fixes it.

Minor, while you're here: these are source-text assertions, and the repo's convention for those is Tests/Lint/ (KqlJoinKinds.Tests.ps1, HubsKqlOperators.Tests.ps1) rather than Tests/Unit/.

-Because 'a direct assignment (rather than a max guard) was the source of the #2206 regression'
}

It 'Should not unconditionally overwrite final retention' {
$content | Should -Not -Match '\$json\.retention\.final\.months\s*=\s*\[Int32\]::Parse\(\$env:finalRetentionInMonths\)\s*$' `
-Because 'a direct assignment (rather than a max guard) was the source of the #2206 regression'
}
}

Context 'First-run behavior unchanged' {

It 'Should still seed ingestion retention from the parameter when no retention object exists yet' {

@RolandKrummenacher Roland Krummenacher (RolandKrummenacher) Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These two assert a branch that can't execute.

Add-Member -Name ingestion only runs when $json.retention.ingestion is missing, and by the time the script reaches it the retention object has always been seeded with a hardcoded 13. So "first-run behavior unchanged" is green because it checks source text for a dead branch, not because first-run behavior is unchanged — and first-run behavior is exactly what this PR changes (see the L137 comment).

A test that would have caught it: invoke the script with mocked Az cmdlets, no existing blob, ingestionRetentionInMonths=6, and assert the written JSON contains 6. That also gives the max-guard real behavioral coverage instead of regex-matching the implementation.

$content | Should -Match 'Add-Member -Name ingestion -Value \(ConvertFrom-Json "\{""months"":\$\(\$env:ingestionRetentionInMonths\)\}"\)' `
-Because 'a brand-new settings.json has no stored value to protect, so the first deploy must still honor the requested retention'
}

It 'Should still seed final retention from the parameter when no retention object exists yet' {
$content | Should -Match 'Add-Member -Name final -Value \(ConvertFrom-Json "\{""months"":\$\(\$env:finalRetentionInMonths\)\}"\)' `
-Because 'a brand-new settings.json has no stored value to protect, so the first deploy must still honor the requested retention'
}
}
}
9 changes: 6 additions & 3 deletions src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Core/Copy-FileToAzureBlob.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,13 +125,16 @@ else
}

# Set or update ingestion retention
# NOTE: Bicep always passes a value here (defaulting to 13 if the caller didn't specify one), so this script
# cannot tell an explicit redeploy value from a silently-defaulted one. Never lower retention on redeploy --
# shrinking silently ages out historical data the next time the purge pipeline runs (#2206); growing is safe.
if (!($json.retention.ingestion))
{
$json.retention | Add-Member -Name ingestion -Value (ConvertFrom-Json "{""months"":$($env:ingestionRetentionInMonths)}") -MemberType NoteProperty
}
else
{
$json.retention.ingestion.months = [Int32]::Parse($env:ingestionRetentionInMonths)
$json.retention.ingestion.months = [Math]::Max($json.retention.ingestion.months, [Int32]::Parse($env:ingestionRetentionInMonths))

@RolandKrummenacher Roland Krummenacher (RolandKrummenacher) Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Worth spelling out the consequence in the docs: after this, stored retention can only ever grow. A customer who deliberately wants to lower retention to cut storage cost has no supported path — redeploying with a smaller value is now a no-op, silently.

That's the correct default, but it needs an escape hatch or at least a documented manual one ("edit settings.json in the hub storage account"). Otherwise the next issue is someone asking why their retention change didn't apply.

@flanakin Michael Flanakin (flanakin) Sep 1, 2026

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.

I don't think max is the right approach. If there's an existing value, we need to use that. The issue isn't about lowering it, the issue is changing it.

@RolandKrummenacher Roland Krummenacher (RolandKrummenacher) Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This clamps fresh deploys to a floor of 13.

The else is unconditional in practice. Every path that reaches here has already built retention with a hardcoded "months": 13 — the no-blob literal around L70-86, and the pre-0.4 backfill at L94-114. So $json.retention.ingestion is never falsy at this point, the Add-Member branch above is dead code, and this line evaluates [Math]::Max(13, $requested).

Requesting ingestionRetentionInMonths: 6 on a brand-new hub stores 13. The pre-PR line stored 6. Confirmed by running the head script against stubbed Az cmdlets.

Moving the guard ahead of the seeding blocks (so the stored value is genuinely absent on a first deploy) resolves it, as does making the Bicep params nullable and skipping empty env vars.

}

# Set or update raw retention
Expand All @@ -144,14 +147,14 @@ else
$json.retention.raw.days = [Int32]::Parse($env:rawRetentionInDays)

@RolandKrummenacher Roland Krummenacher (RolandKrummenacher) Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

raw is left unguarded here, and it's the one where a silent shrink does the most damage.

I grepped src/ — nothing reads retention.raw.days back out of settings.json. This line is record-only. The retention that actually takes effect is baked into the KQL at deploy time (Analytics/app.bicep:369):

.alter-merge table ActualCosts_raw policy retention softdelete = $$rawRetentionInDays$$d recoverability = disabled

dataExplorerRawRetentionInDays defaults to 0 (main.bicep:154). So a customer who set it to, say, 30 and later redeploys without repeating the param gets softdelete = 0d recoverability = disabled reapplied to every *_raw table — an immediate and explicitly unrecoverable purge. That's the #2206 failure mode with a worse blast radius, and no settings.json guard can catch it because the value never round-trips through settings.json.

Two options: scope the title/description to the settings.json path, or open a follow-up for the raw policy. Either is fine, but it shouldn't go unrecorded.

@RolandKrummenacher Roland Krummenacher (RolandKrummenacher) Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Still open at 845afc8, and one refinement to what I said above.

I called raw "the one where a silent shrink does the most damage". Among the settings.json fields that's not right — raw.days has no reader at all, so its damage is entirely via the separate ADX softdelete path I described. The unguarded field that does real damage through settings.json is msexports.days, read at Exports/app.bicep:1636 to decide whether export files are deleted after ingestion. A redeploy that drops a customized value resets it to 0 and files start being deleted again — the #2206 failure mode exactly, on a field this PR doesn't touch.

The ADX softdelete point stands unchanged, and so does the ask here: either scope the title/description to the fields actually covered, or record the rest as a follow-up.

}

# Set or update final retention
# Set or update final retention (never lower on redeploy -- see note above)
if (!($json.retention.final))
{
$json.retention | Add-Member -Name final -Value (ConvertFrom-Json "{""months"":$($env:finalRetentionInMonths)}") -MemberType NoteProperty
}
else
{
$json.retention.final.months = [Int32]::Parse($env:finalRetentionInMonths)
$json.retention.final.months = [Math]::Max($json.retention.final.months, [Int32]::Parse($env:finalRetentionInMonths))
}

# Updating settings
Expand Down
Loading