fix(hubs): keep stored ingestion and final retention on redeploy

[Michael Flanakin (flanakin)]

🛠️ Description

Copy-FileToAzureBlob.ps1 unconditionally overwrote settings.json's retention.ingestion.months / retention.final.months with whatever the deployment script's Bicep parameters passed. Bicep always resolves a value for an optional parameter (13 if omitted), so the script couldn't tell an explicit redeploy value from a silently defaulted one. A redeploy that dropped a customized value reset it to 13, and the next purge run aged out historical data (#2206).

Fix: a value already stored in settings.json is kept on redeploy. The requested value applies only when nothing is stored (new hub, or settings from before retention existed). Stored retention is read before defaults are seeded, so a new hub honors the requested value.

Scope: ingestion and final retention only. msexports.days (read by the exports app to delete files) and raw.days / the Data Explorer softdelete policy (applied from Bicep parameters, not settings.json) are not covered and need their own change.

To change retention after deploy, edit config/settings.json in the hub storage account (documented in data processing).

Fixes #2206

📋 Checklist

🔬 How did you test this change?

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

HubsRetentionGuard.Tests.ps1 runs the real script with stubbed Az cmdlets and asserts the uploaded JSON: stored higher and lower values are kept, a pre-retention settings file is seeded from the request, and a new hub honors the requested value. Two of the four tests fail against the previous script.

📦 Deploy to test?

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

🤖 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)]

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.

[Michael Flanakin (flanakin)]

🤖 [AI][Claude] PR Update Summary

Addressed: 1 of 6 open thread(s)

• ✅ Implemented: 1 (test regex anchor bug — both regression guards were unfailable)
• 🙋 Needs maintainer: 5 (all touch the retention-guard logic itself, data lifecycle, or redeploy behavior — out of scope for autonomous changes)

Implemented: Fixed the \s*$ anchor in HubsRetentionGuard.Tests.ps1 (lines 39/44) so the two 'should not unconditionally overwrite' assertions can actually fail. Confirmed via a revert-and-rerun check. No production code touched.

Left for maintainer review (not implemented — each involves retention-logic/data-lifecycle judgment):

1. raw/msexports retention fields are unguarded and have real purge/softdelete consequences on redeploy (same FinOps Hub upgraded from v13 to v14- two months data is missing in dashboard #2206 failure mode, different fields).
2. The max-guard sits behind hardcoded seeding, clamping fresh deploys to a floor of 13 months — first-run no longer honors a requested lower value.
3. PR author's own comment pushing back on the max-guard approach entirely ('the issue isn't about lowering it, the issue is changing it') — unresolved design question.
4. No documented escape hatch for intentionally lowering retention.
5. The 'first-run behavior unchanged' tests assert a dead code path (tied to item 2).

Also flagging: the PR is currently in a conflicting merge state against dev (mergeStateStatus: DIRTY) — left untouched since conflict resolution requires explicit approval and wasn't part of the thread responses.

[Roland Krummenacher (RolandKrummenacher)]

Regex fix verified, resolved that thread.

On the approach question in the L137 thread: agree that "keep the stored value" is cleaner than max. Either way, the guard has to run before the hardcoded months = 13 seeding (L72-83, L98-110). Right now the if (!($json.retention.ingestion)) branch never runs, so L137 is effectively Max(13, requested) and a new hub requesting 6 months stores 13 (still reproduces at e952c89). With "keep the stored value" as-is, the seeded 13 would always win, so the reorder is needed in both designs.

Also still open:

• raw and msexports retention still overwrite unconditionally (L124, L147). Please either scope the title/description to ingestion/final or link a follow-up.
• A short note on how to lower retention deliberately (edit settings.json).
• The "first-run behavior unchanged" tests still match the branch that can't execute; a mocked-Az test with no existing blob and ingestionRetentionInMonths=6 asserting 6 is written would cover the seeding bug.
• The branch conflicts with dev (docs-mslearn/toolkit/changelog.md only); please merge origin/dev.

[github-actions]

Hubs + ADX (managed) deployment failed. View logs.

[github-actions]

Hubs + ADX (managed) deployment failed. View logs.

[github-actions]

Hubs + ADX (managed) deployed to pr-2288-adx.

[Roland Krummenacher (RolandKrummenacher)]

Re-review — PR #2288 (head 2edaa7f)

Everything from my earlier rounds is addressed:

• Stored retention is now read before the defaults are seeded, so a new hub that requests 6 months gets 6, not 13.
• "Keep the stored value" replaces the Max guard, as agreed on 10/13.
• The tests run the real script with stubbed Az cmdlets. I ran them locally: all 4 pass against this head, and the two tests that keep stored values fail against the dev script. They catch #2206.
• Scope is limited to ingestion and final retention, and raw / msexports are tracked in #2348.
• data-processing.md and the changelog now explain how to change retention after deployment.

Approving. Two non-blocking notes are inline. The first one also covers docs-mslearn/toolkit/hubs/template.md, which isn't part of this diff.

[Roland Krummenacher (RolandKrummenacher)]

Non-blocking: this note is good, but the parameter reference still describes these parameters as if they always apply. In docs-mslearn/toolkit/hubs/template.md (lines 95 and 97), ingestionRetentionInMonths and dataExplorerFinalRetentionInMonths only say "Number of months of data to retain …". After this PR, both values apply only when nothing is stored yet. Someone who redeploys from the portal and picks 36 months to increase retention is now ignored without any message. Consider adding a sentence to both rows, for example: "Applied only when no value is stored in config/settings.json. To change it later, edit that file."

[Roland Krummenacher (RolandKrummenacher)]

Non-blocking: these process-level environment variables stay set after the tests finish, so they can affect other tests in the same Pester session (npm run pester runs all unit tests in one process). Consider saving the previous values and restoring them in an AfterAll, or removing them in the finally block below.