fix(hubs): never lower retention on redeploy

[Michael Flanakin (flanakin)]

🛠️ Description

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

Fix: take the max of stored and incoming retention instead of overwriting. Growing retention is always safe; shrinking it silently purges data and is hard to reverse.

Fixes #2206

📷 Screenshots

Not applicable — PowerShell script logic change, no UI.

📋 Checklist

🔬 How did you test this change?

• 🤏 Lint tests
• 🤞 PS -WhatIf / az validate
• 👍 Manually deployed + verified
• 💪 Unit tests
• 🙌 Integration tests

Added HubsRetentionGuard.Tests.ps1 asserting the max-guard is present and the old unconditional-overwrite pattern is gone, plus that first-run seeding (no existing settings.json) still honors the requested value. Full unit suite passes (2283 passed, 0 failed).

📦 Deploy to test?

• Hubs + ADX (managed)
• Hubs + Fabric (manual) — URI:
• Hubs (manual)
• Hubs (no data)
• Workbooks
• Alerts

🙋‍♀️ Do any of the following that apply?

• 🚨 This is a breaking change.
• 🤏 The change is less than 20 lines of code.

📑 Did you update docs/changelog.md?

• ✅ Updated changelog (required for dev PRs)
• ➡️ Will add log in a future PR (feature branch PRs only)
• ❎ Log not needed (small/internal change)

📖 Did you update documentation?

• ✅ Public docs in docs (required for dev)
• ✅ Public docs in docs-mslearn (required for dev)
• ✅ Internal dev docs in docs-wiki (required for dev)
• ✅ Internal dev docs in src (required for dev)
• ➡️ Will add docs in a future PR (feature branch PRs only)
• ❎ Docs not needed (small/internal change)

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Roland Krummenacher (RolandKrummenacher)]

The [Math]::Max guard is the right call — growing retention is safe, shrinking it silently purges data. Three things before this goes in, one of which means the PR as titled doesn't quite do what it says.

The same failure mode is still open on raw, in a path this guard structurally cannot reach. Details inline — short version is that retention.raw.days is written to settings.json and read by nothing, while the real raw retention is an ADX softdelete policy applied straight from the Bicep param at deploy time. Happy for that to be a follow-up rather than scope creep here, but "never lower retention on redeploy" currently overpromises.

Two of the six new tests can never fail. The Should -Not -Match anchors are wrong, so both regression guards pass whether or not the bug comes back. Inline.

No way to deliberately lower retention. After this, retention is a one-way ratchet — a customer who wants to reduce it to cut storage cost can't do it by redeploying, and there's no documented alternative. On a FinOps toolkit that's a pointed limitation, and the PR has no doc or changelog change. Even a line in the hubs docs saying "edit settings.json in the hub storage account to lower it" would close this.

Requesting changes mainly for the test anchors, since those are load-bearing for the regression this PR exists to prevent.

[Roland Krummenacher (RolandKrummenacher)]

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/.

[Roland Krummenacher (RolandKrummenacher)]

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.

[Roland Krummenacher (RolandKrummenacher)]

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.

[Roland Krummenacher (RolandKrummenacher)]

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.

[Michael Flanakin (flanakin)]

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.

[Roland Krummenacher (RolandKrummenacher)]

Re-reviewed at 845afc8. The only change since the last pass is the changelog entry, so all three earlier threads are still open. Re-running the analysis against the full script and every consumer of settings.json turned up something more serious than those: the guard sits behind a hardcoded default, so this PR makes it impossible to configure retention below 13 on a fresh deploy.

The guard never sees a virgin object

Both paths that reach the guard pre-seed retention with a hardcoded 13 — the no-blob literal (~L70-86) and the pre-0.4 backfill (~L94-114). By the time line 137 runs, $json.retention.ingestion is always truthy, so:

deploy	requested	pre-PR stored	this PR stores
fresh	ingestion 6 / final 3	6 / 3	13 / 13
existing 36mo	default (13)	13 (the #2206 bug)	36

I ran the head script against stubbed Az cmdlets to confirm both rows. The #2206 fix does work — that's the second row, and it's worth keeping. But a customer who deploys a new hub asking for 6 months silently gets 13, and there is no redeploy that can bring it back down.

The corollary is that the Add-Member branches the guard falls back to are unreachable, which is why the two "first-run behavior unchanged" tests pass: they assert dead code, on precisely the case this PR breaks.

ingestion is not a purge control

I grepped every consumer of the retention settings. retention.ingestion.months has exactly one reader, and it isn't a purge:

ManagedExports/app.bicep:237  @subtractFromTime(startOfMonth(utcNow()), ...retention.ingestion.months, 'Month')

That's the managed-export backfill start date. The field that actually drives the purge pipeline is final:

Analytics/app.bicep:1244      @coalesce(...retention.final.months, 999)

So of the two fields this PR guards, only final is the #2206 mechanism. Ratcheting ingestion upward has the opposite of the intended effect: every redeploy re-backfills more months of exports than the caller asked for. The inline note ("shrinking silently ages out historical data the next time the purge pipeline runs") and the changelog entry both attribute purge behavior to ingestion that it doesn't have.

The unguarded sibling with a live consumer

My earlier thread flagged raw (still open, still valid — no settings.json consumer, real retention comes from the ADX softdelete policy). Re-reading the consumers, msexports is the one that matters more here, and it's still an unconditional overwrite at L124:

Exports/app.bicep:1636  @lessOrEquals(coalesce(...retention.msexports.days, 0), 0)

A redeploy that drops a customized msexportRetentionInDays resets it to the 0 default, and the pipeline goes back to deleting export files immediately after ingestion. Same failure mode as #2206, same trigger, unguarded.

Suggested shape

Distinguishing "caller asked for this" from "Bicep defaulted this" is the actual root cause, and a max-guard can't do it. Options, roughly in order of preference:

1. Make the Bicep params nullable (param ingestionRetentionInMonths int?) and have the script skip any field whose env var is empty. That fixes #2206 for every field including msexports and raw, keeps lowering supported, and drops the need for a guard entirely.
2. Keep the max-guard but move it ahead of the hardcoded seeding, and apply it only to final. Fresh deploys then honor the requested value, and the fields it doesn't apply to keep working as documented.

Either way, if increase-only stays for any field, it needs a documented way out (the escape-hatch thread) and the title/changelog need to say which field it covers.

---

Also: the PR is currently in a conflicting merge state against dev.

Still open from the last pass — none addressed: the \s*$ anchors that make both regression guards unfailable, the raw scope question, and the missing escape hatch for increase-only retention.

[Roland Krummenacher (RolandKrummenacher)]

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.

[Roland Krummenacher (RolandKrummenacher)]

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.