[Michael Flanakin (flanakin)]

🛠️ Description

🤖 [AI] Hub redeploys and upgrades no longer reset retention settings. The fix is on the incoming values: retention parameters are optional, so leaving one out keeps the stored setting, and an explicit value always applies.

• Root cause: every retention param had a Bicep default, so the settings script couldn't tell "caller asked for 13" from "caller said nothing". Redeploys and upgrades silently reset retention.
• Fix the input, not the write: all 4 retention params (exportRetentionInDays, ingestionRetentionInMonths, dataExplorerRawRetentionInDays, dataExplorerFinalRetentionInMonths) are now int?. Omitted = keep the value in config/settings.json; explicit = applied, including lower values. New hubs still get 0/13/0/13. Replaces the earlier "never change a stored value" guard.
• Raw retention: the settings script returns the effective raw.days, and Data Explorer applies that value. The softdelete = 0d "clear" step is gone, so raw tables never drop to 0 during a deploy.
• PowerShell: Deploy-FinOpsHub only passes retention params you specify.
• Portal: default values stay visible. A value is sent only when it was changed from the default. An unchanged or empty value sends null, which the template treats as "keep current". The Data Explorer section explains this.
• Portal limitation: you can't use the portal to set a non-default value back to the default (for example 36 → 13), because "13" looks the same as "unchanged". To do that, edit config/settings.json or run Deploy-FinOpsHub with the value. Not tested in the portal sandbox: how the portal passes a null output (parse('null')).
• Why v15: shipped versions lose data on upgrade (FinOps Hub upgraded from v13 to v14- two months data is missing in dashboard #2206: the 2 oldest months were purged after a v13 → v14 upgrade; Hub redeploys can reset msexports and raw retention #2348: raw tables were reset to 0 days on every deploy). This needs to ship in the next release, not wait.

Fixes #2206, #2348

📋 Checklist

🔬 How did you test this change?

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

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

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) deployed to pr-2288-adx.

[Roland Krummenacher (RolandKrummenacher)]

Re-reviewed c6457e3 after the rebase onto dev. The net diff matches what I approved at 584c2b4 except for ms.date and changelog context. Still approved.

[github-actions]

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

[github-actions]

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

[github-actions]

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

[github-actions]

Test environments cleaned up.