From 9dbe983e1e9409c51b17b7c036b767a13dac888c Mon Sep 17 00:00:00 2001 From: Roland Krummenacher Date: Wed, 12 Aug 2026 21:48:24 +0200 Subject: [PATCH 01/13] fix: Replace serviceFamily sharding with a single traversal per price type The weekly Update Commitment Discount Eligibility job exceeded its 60-minute timeout on run 31612943595 and was killed 13 of 29 shards into the Consumption fetch, so it still published nothing. The sharding and multi-pass union existed to work around a period when the Retail Prices API returned an unstable page order. That instability was last observed in early June 2026; MS support case 2606030050003725 closed without a root cause, seven full traversals on 2026-06-29 were byte-identical, and the workaround's own telemetry in run 31612943595 showed every repeat pass across all 22 completed shards adding exactly zero meters. Building the shard list also required a full discovery traversal per price type, which alone consumed 25 of the job's 60 minutes. Each price type is now walked once, following NextPageLink verbatim, with the per-serviceFamily counts tallied in the same pass (every item already carries serviceFamily, so the per-family completeness guard costs nothing extra). Both completeness guards are unchanged; if the instability returns they abort the run loudly rather than publishing partial data. Also fixes two latent bugs found while validating: - Retry-After was never honored. It was read via $Response.Headers['Retry-After'], but HttpResponseHeaders has no string indexer, so that silently evaluated to $null and every 429 fell through to the exponential backoff. Now reads the typed .RetryAfter property, handling both the delta-seconds and HTTP-date forms, clamped so an outsized value cannot stall the job past its timeout. - $cachedShardCounts['Reservation'] threw "Cannot index into a null array" when no baseline sidecar exists. Since the job has never completed a successful run the sidecar has never existed, so this is the path every first run takes; it would have failed the run even with the timeout raised. Validated against the live API: two independent full runs both walked 146,671 Reservation items over 147 pages and 689,723 Consumption items over 690 pages, producing an identical 66,199 RI-eligible and 82,677 SP-eligible meters. Both match the sharded run 31612943595 exactly, per service family as well as in aggregate. Co-Authored-By: Claude Opus 5 (1M context) --- ...te-CommitmentDiscountEligibility.Tests.ps1 | 182 ++++---- .../Update-CommitmentDiscountEligibility.ps1 | 396 +++++++----------- 2 files changed, 252 insertions(+), 326 deletions(-) diff --git a/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 b/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 index 17b94a131..67c18cb8d 100644 --- a/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 +++ b/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 @@ -1,7 +1,7 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. -# Unit tests for the pure guard/shard-list helpers in +# Unit tests for the pure helpers in # src/scripts/Update-CommitmentDiscountEligibility.ps1. That script runs top to bottom # (and calls the live Azure Retail Prices API) when dot-sourced, so rather than execute # it we extract just the function definitions via the AST and evaluate those in isolation. @@ -10,7 +10,7 @@ Describe 'Update-CommitmentDiscountEligibility helpers' { BeforeAll { $scriptPath = Join-Path (Get-Item -Path $PSScriptRoot).Parent.Parent.Parent.Parent.FullName 'src/scripts/Update-CommitmentDiscountEligibility.ps1' $ast = [System.Management.Automation.Language.Parser]::ParseFile($scriptPath, [ref]$null, [ref]$null) - foreach ($name in 'Get-ShardShortfall', 'Add-BaselineShard', 'Get-ServiceFamily', 'Assert-DiscoveryConverged') + foreach ($name in 'Get-ShardShortfall', 'Get-RetryDelay', 'Get-EligibleMeter') { $fn = $ast.FindAll({ param($n) $n -is [System.Management.Automation.Language.FunctionDefinitionAst] -and $n.Name -eq $name }, $true) | Select-Object -First 1 if (-not $fn) { throw "Function $name not found in $scriptPath" } @@ -46,124 +46,146 @@ Describe 'Update-CommitmentDiscountEligibility helpers' { } } - Context 'Add-BaselineShard' { - It 'unions discovered families with the baseline families' { - $u = Add-BaselineShard -Discovered ([string[]]@('Compute', 'Analytics')) -BaselineSection @{ Compute = 1; Tiny = 1 } - $u | Should -Contain 'Tiny' - $u | Should -Contain 'Analytics' - $u | Should -Contain 'Compute' + Context 'Get-RetryDelay' { + # Builds a real HttpResponseMessage -- the same type Invoke-RestMethod surfaces on + # $_.Exception.Response -- so these tests exercise the actual header plumbing + # rather than a hand-rolled stand-in. + BeforeAll { + function Get-TestResponse + { + param([string]$RetryAfter) + $r = [System.Net.Http.HttpResponseMessage]::new(429) + if ($RetryAfter) { $null = $r.Headers.TryAddWithoutValidation('Retry-After', $RetryAfter) } + return $r + } + } + + It 'honors a delta-seconds Retry-After' { + # Regression guard for the original bug: the header was read via + # $Response.Headers['Retry-After'], but HttpResponseHeaders has no string + # indexer, so that silently yielded $null and every 429 fell through to the + # exponential backoff. A returned 30 (not the attempt-1 fallback of 20) + # proves the header is genuinely being read. + Get-RetryDelay -Response (Get-TestResponse -RetryAfter '30') -Attempt 1 | Should -Be 30 + } + + It 'honors the HTTP-date form of Retry-After' { + $when = [DateTimeOffset]::UtcNow.AddSeconds(120).ToString('r') + $delay = Get-RetryDelay -Response (Get-TestResponse -RetryAfter $when) -Attempt 1 + # Second-resolution formatting plus test execution time make this approximate. + $delay | Should -BeGreaterThan 110 + $delay | Should -BeLessOrEqual 121 + } + + It 'clamps an outsized Retry-After to MaxSeconds' { + Get-RetryDelay -Response (Get-TestResponse -RetryAfter '99999') -Attempt 1 -MaxSeconds 300 | Should -Be 300 + } + + It 'falls back to exponential backoff when the header is absent' { + Get-RetryDelay -Response (Get-TestResponse) -Attempt 1 | Should -Be 20 + Get-RetryDelay -Response (Get-TestResponse) -Attempt 3 | Should -Be 80 } - It 'de-duplicates families present in both sets' { - $u = Add-BaselineShard -Discovered ([string[]]@('Compute', 'Compute')) -BaselineSection @{ Compute = 1 } - @($u | Where-Object { $_ -eq 'Compute' }).Count | Should -Be 1 + It 'falls back to exponential backoff when there is no response at all' { + # Network/DNS/timeout errors surface with a null .Response. + Get-RetryDelay -Response $null -Attempt 2 | Should -Be 40 } - It 'is null-safe when no baseline exists' { - $u = Add-BaselineShard -Discovered ([string[]]@('A', 'B')) -BaselineSection $null - $u | Should -HaveCount 2 + It 'falls back to exponential backoff for an HTTP-date already in the past' { + # A stale date must not produce a zero/negative wait and busy-loop the retry. + $past = [DateTimeOffset]::UtcNow.AddSeconds(-60).ToString('r') + Get-RetryDelay -Response (Get-TestResponse -RetryAfter $past) -Attempt 1 | Should -Be 20 } } - Context 'Get-ServiceFamily' { - # Get-ServiceFamily calls Get-RetailPriceSegment, which hits the live API. Stub it - # in the test scope so each "traversal" replays a scripted page of items instead. - # $script:pages is the per-pass item list; $script:passLog records the passes made. + Context 'Get-EligibleMeter' { + # Get-EligibleMeter calls Get-RetailPriceSegment, which hits the live API. Stub it + # in the test scope so the "traversal" replays a scripted list of items instead. BeforeEach { - $script:passLog = 0 function Get-RetailPriceSegment { # Filter/MeterRegion are part of the real signature but irrelevant to the - # stub, which replays scripted pages. Target must be empty to suppress on + # stub, which replays scripted items. Target must be empty to suppress on # PSScriptAnalyzer 1.x (naming the parameter does not work). [Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter', '', Justification = 'Stub mirrors the real Get-RetailPriceSegment signature; only OnItem is exercised')] param($Filter, $MeterRegion, $OnItem) - $script:passLog++ - $items = if ($script:pages.Count -ge $script:passLog) { $script:pages[$script:passLog - 1] } else { $script:pages[-1] } - foreach ($i in $items) { & $OnItem $i } - return @{ Items = @($items).Count; Pages = $script:pagesPerPass } + foreach ($i in $script:items) { & $OnItem $i } + return @{ Items = @($script:items).Count; Pages = 1 } } + + $script:allMeters = { param($item) $item.meterId } } - It 'includes a new one-item family that only a later discovery pass sees' { - # Pass 1 misses 'Tiny' entirely (the unstable page order dropped its only row); - # passes 2+ see it. The unioned shard list must still contain it -- this is the - # case Add-BaselineShard cannot cover, because a brand-new family is in no baseline. - $script:pagesPerPass = 5 - $script:pages = @( - @( @{ serviceFamily = 'Compute' }, @{ serviceFamily = 'Storage' } ), - @( @{ serviceFamily = 'Compute' }, @{ serviceFamily = 'Storage' }, @{ serviceFamily = 'Tiny' } ) + It 'collects meter ids and counts them per service family' { + $script:items = @( + @{ meterId = 'm1'; serviceFamily = 'Compute' }, + @{ meterId = 'm2'; serviceFamily = 'Compute' }, + @{ meterId = 'm3'; serviceFamily = 'Storage' } ) - $r = Get-ServiceFamily -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -MaxPasses 4 -StablePasses 2 + $r = Get-EligibleMeter -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -CollectKey $script:allMeters - $r.Families | Should -Contain 'Tiny' - $r.Families | Should -HaveCount 3 - $r.Converged | Should -BeTrue + $r.Keys.Count | Should -Be 3 + $r.FamilyCounts['Compute'] | Should -Be 2 + $r.FamilyCounts['Storage'] | Should -Be 1 + $r.Items | Should -Be 3 } - It 'reports non-convergence when every pass keeps adding families' { - # A family set that never stabilizes means discovery cannot be trusted to be - # complete, so the caller must refuse to publish rather than guess. - $script:pagesPerPass = 5 - $script:pages = @( - @( @{ serviceFamily = 'A' } ), - @( @{ serviceFamily = 'B' } ), - @( @{ serviceFamily = 'C' } ), - @( @{ serviceFamily = 'D' } ) - ) + It 'lower-cases collected meter ids so the CSV and guard compare consistently' { + $script:items = @( @{ meterId = 'AbC-123'; serviceFamily = 'Compute' } ) - $r = Get-ServiceFamily -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -MaxPasses 4 -StablePasses 2 + $r = Get-EligibleMeter -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -CollectKey $script:allMeters - $r.Converged | Should -BeFalse - $r.Families | Should -HaveCount 4 - $script:passLog | Should -Be 4 # stopped at the cap, did not spin + $r.Keys.ContainsKey('abc-123') | Should -BeTrue } - It 'stops after one pass when the scan fit in a single response' { - # No NextPageLink means the scan was complete and deterministic; repeating it - # would only burn API calls. - $script:pagesPerPass = 1 - $script:pages = @( , @( @{ serviceFamily = 'Compute' } ) ) + It 'counts a meter id seen twice in one family only once' { + $script:items = @( + @{ meterId = 'm1'; serviceFamily = 'Compute' }, + @{ meterId = 'm1'; serviceFamily = 'Compute' } + ) - $r = Get-ServiceFamily -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -MaxPasses 4 -StablePasses 2 + $r = Get-EligibleMeter -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -CollectKey $script:allMeters - $script:passLog | Should -Be 1 - $r.Converged | Should -BeTrue - $r.Families | Should -Contain 'Compute' + $r.Keys.Count | Should -Be 1 + $r.FamilyCounts['Compute'] | Should -Be 1 } - It 'converges without exhausting the cap when the family set is stable' { - $script:pagesPerPass = 5 - $script:pages = @( , @( @{ serviceFamily = 'Compute' }, @{ serviceFamily = 'Storage' } ) ) + It 'records a family that collects nothing as 0 rather than dropping it' { + # The sharded implementation persisted an explicit 0 for such a family; the + # baseline must keep doing so, otherwise a family whose meters all disappear + # looks like a brand-new family to the guard instead of a 100% shortfall. + $script:items = @( + @{ meterId = 'm1'; serviceFamily = 'Compute'; savingsPlan = @(1) }, + @{ meterId = 'm2'; serviceFamily = 'Analytics'; savingsPlan = @() } + ) - $r = Get-ServiceFamily -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -MaxPasses 4 -StablePasses 2 + $r = Get-EligibleMeter -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -CollectKey { + param($item) + if ($item.savingsPlan -and $item.savingsPlan.Count -gt 0) { $item.meterId } else { $null } + } - $r.Converged | Should -BeTrue - $script:passLog | Should -Be 3 # pass 1 adds, passes 2-3 add nothing - $r.Families | Should -HaveCount 2 + $r.Keys.Count | Should -Be 1 + $r.FamilyCounts['Compute'] | Should -Be 1 + $r.FamilyCounts.ContainsKey('Analytics') | Should -BeTrue + $r.FamilyCounts['Analytics'] | Should -Be 0 } - It 'ignores items with no serviceFamily' { - $script:pagesPerPass = 5 - $script:pages = @( , @( @{ serviceFamily = 'Compute' }, @{ serviceFamily = $null } ) ) + It 'buckets items with no service family under (none)' { + $script:items = @( @{ meterId = 'm1'; serviceFamily = $null } ) - $r = Get-ServiceFamily -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -MaxPasses 4 -StablePasses 2 + $r = Get-EligibleMeter -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -CollectKey $script:allMeters - $r.Families | Should -HaveCount 1 + $r.FamilyCounts['(none)'] | Should -Be 1 } - } - Context 'Assert-DiscoveryConverged' { - It 'throws when discovery did not converge' { - { Assert-DiscoveryConverged -Discovery @{ Converged = $false } -PriceType 'Reservation' -MaxPasses 4 } | - Should -Throw -ExpectedMessage '*Reservation serviceFamily discovery did not converge*' - } + It 'returns the page count from the underlying traversal' { + $script:items = @( @{ meterId = 'm1'; serviceFamily = 'Compute' } ) + + $r = Get-EligibleMeter -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -CollectKey $script:allMeters - It 'passes when discovery converged' { - { Assert-DiscoveryConverged -Discovery @{ Converged = $true } -PriceType 'Consumption' -MaxPasses 4 } | - Should -Not -Throw + $r.Pages | Should -Be 1 } } } diff --git a/src/scripts/Update-CommitmentDiscountEligibility.ps1 b/src/scripts/Update-CommitmentDiscountEligibility.ps1 index 28959e84a..3545b530a 100644 --- a/src/scripts/Update-CommitmentDiscountEligibility.ps1 +++ b/src/scripts/Update-CommitmentDiscountEligibility.ps1 @@ -10,30 +10,33 @@ Reserved Instances and/or Savings Plans. Outputs a CSV file that can be used as open data for FinOps Hub ingestion and PowerShell module lookups. - The Azure Retail Prices API does not guarantee a stable total order across paged - requests, so a single NextPageLink/$skip traversal of a large result set silently - drops a scattered fraction of rows and is not reproducible run-to-run. To work - around this, the script: - 1. Discovers the serviceFamily values actually present in the API (one scan per - price type) and shards by those, so the shard list is self-correcting: a - newly added family (or a rename) is fetched automatically instead of being - silently excluded by a hardcoded allowlist. A discovery scan is itself a long - traversal and so is exposed to the ordering drop; for most families the drop - can only lose scattered rows (never all of them), but a tiny family with 1-2 - meters could be missed entirely, so the discovered set is unioned with the - prior run's families (Add-BaselineShard) -- a previously-seen family is always - queried and can never be silently dropped. - 2. Shards each query by those serviceFamily values so individual traversals stay - short (short traversals are far less exposed to the ordering instability), and - 3. Repeats each shard, unioning results by meterId, until the collected set - stops growing (misses are random per pass, so the union converges). - A completeness guard aborts before writing if the fetched total falls materially - below the previously published total -- in aggregate and per family, so a family - that vanishes or under-fetches is caught even if growth elsewhere masks it -- so an - incomplete run can never overwrite good open-data with missing eligibility. - - Pagination follows the documented NextPageLink verbatim; no undocumented $orderby - is used (a multi-field $orderby is now rejected by the API with HTTP 400). + Each price type is fetched in a single traversal that follows the documented + NextPageLink verbatim -- no $top, no $orderby (a multi-field $orderby is rejected + by the API with HTTP 400), no client-side $skip. The same pass records both the + eligible meterIds and a per-serviceFamily count, so the completeness guard below + gets its per-family baseline for free. + + Data integrity rests on the completeness guard, not on re-fetching. Before writing, + the run is compared against the previously published data in aggregate AND per + serviceFamily; either check falling more than -MaxShrinkFraction below its baseline + aborts the run, so an incomplete fetch can never overwrite good open data with + missing eligibility. The per-family check is what catches a single family that + silently under-fetches, which an aggregate-only floor would miss when growth + elsewhere masks it. + + History: this script previously sharded each query by serviceFamily and repeated + every shard until its meterId set stopped growing, to work around a period when the + API returned an unstable page order and a single traversal silently dropped a + scattered fraction of rows. That instability was last observed in early June 2026; + Microsoft support case 2606030050003725 closed without a root cause on 2026-07-06. + Seven full traversals on 2026-06-29 were byte-identical, and the workaround's own + telemetry in CI on 2026-08-12 showed every repeat pass across all 22 shards adding + exactly zero meters while costing 2-3x the traversal volume -- and the shard list + required a full discovery traversal per price type, which alone consumed 25 of the + job's 60 minutes and pushed the run into its timeout. The workaround was therefore + removed in favour of the guards. If the instability returns, the guards abort the + run loudly rather than publishing partial data; that is the intended behaviour and + the signal to reopen the case. .PARAMETER OutputPath Path to the output CSV file. Defaults to src/open-data/CommitmentDiscountEligibility.csv. @@ -62,19 +65,6 @@ $ErrorActionPreference = 'Stop' $apiBase = 'https://prices.azure.com/api/retail/prices?api-version=2023-01-01-preview' -# The serviceFamily values to shard by are discovered at runtime per price type (see -# Get-ServiceFamily), not hardcoded: a positive `serviceFamily eq` allowlist would -# silently drop any family it omits (e.g. a newly added or renamed family), and the -# completeness guard's aggregate floor is too coarse to catch a small family. Discovery -# makes the shard set self-correcting; the per-family guard catches any family that -# disappears between runs. - -# Repeat each shard -- and each serviceFamily discovery scan -- until its unioned set -# stops growing for $StablePasses consecutive passes, or $MaxPassesPerShard is reached -# (treated as non-convergence, which aborts before the write). -$MaxPassesPerShard = 4 -$StablePasses = 2 - function Get-RetailPriceSegment { <# @@ -86,8 +76,8 @@ function Get-RetailPriceSegment Returns @{ Items = ; Pages = }. Retries 429/5xx with backoff (honoring Retry-After) and throws on terminal 4xx. NextPageLink already carries api-version, $filter, $skip, and meterRegion, - so it is followed verbatim. Pages lets the caller detect "fit in one page" - without assuming a fixed server page size. + so it is followed verbatim. Pages lets the caller report how much of the + catalogue a traversal actually covered. #> param( [string]$Filter, @@ -139,8 +129,7 @@ function Get-RetailPriceSegment throw "Failed after $maxRetries retries: $_" } - $retryAfter = if ($errResponse) { $errResponse.Headers['Retry-After'] } else { $null } - $wait = if ($retryAfter) { [int]$retryAfter } else { [Math]::Pow(2, $retries) * 10 } + $wait = Get-RetryDelay -Response $errResponse -Attempt $retries $reason = if ($statusCode -eq 429) { 'Rate limited' } elseif ($statusCode) { "HTTP $statusCode" } else { 'Network error' } Write-Host " $reason, retrying in ${wait}s (attempt $retries/$maxRetries)" Start-Sleep -Seconds $wait @@ -161,228 +150,151 @@ function Get-RetailPriceSegment return @{ Items = $totalItems; Pages = $pageCount } } -function Get-ServiceFamily +function Get-RetryDelay { <# .SYNOPSIS - Scans a query and returns the distinct serviceFamily values present, repeating - the scan and unioning the results until the family set stops growing. Used to - build the shard list dynamically instead of hardcoding it. + Returns the seconds to wait before retrying: the response's Retry-After when + the server supplied a usable one, otherwise an exponential backoff. .DESCRIPTION - Returns @{ Families = ; Converged = }. - - A single traversal is exposed to the API's unstable paging order. Families with - many meters survive it (the drop only loses scattered rows), but a family with - 1-2 meters can lose its only row and be missed entirely. For a family seen in a - previous run Add-BaselineShard is a sufficient backstop, but it can only restore - what is already in the baseline -- a BRAND-NEW tiny family missed by discovery is - in neither the discovered set nor the baseline, so it is never fetched and both - completeness guards pass while the run publishes incomplete data. Repeating the - scan and unioning closes that hole: a family any pass sees enters the shard list. - - Convergence follows the same policy as Invoke-ShardedUnion: stop once no pass has - added a family for $StablePasses consecutive passes, or at $MaxPasses (reported as - Converged = $false so the caller can refuse to publish rather than guess). A scan - that fit in a single response saw every row deterministically and stops at once. + Reads the STRONGLY TYPED HttpResponseHeaders.RetryAfter property. Do not reach + for $Response.Headers['Retry-After'] -- HttpResponseHeaders has no string + indexer, so that expression silently evaluates to $null (it does not throw) and + the header is never honored. GetValues() is no better: it throws when the header + is absent. The typed property parses both legal forms of the header (delta- + seconds and HTTP-date), and is $null when absent. + + The value is clamped to -MaxSeconds so an outsized server value cannot stall the + run past the job timeout, and a non-positive value (a stale HTTP-date already in + the past) falls back to the exponential backoff rather than retrying instantly. #> param( - [string]$Filter, - [string]$MeterRegion, - [string]$ActivityName, - [int]$MaxPasses, - [int]$StablePasses - ) + $Response, + [int]$Attempt, - $families = @{} - $stable = 0 - $pass = 0 - $singlePage = $false + # The API documents no ceiling on Retry-After and the job runs under a fixed + # timeout, so an outsized server value is clamped rather than allowed to stall + # the run until it is killed mid-fetch. + [int]$MaxSeconds = 300 + ) - while ($pass -lt $MaxPasses -and $stable -lt $StablePasses) + $retryAfter = $null + $header = if ($Response) { $Response.Headers.RetryAfter } else { $null } + if ($header.Delta) { - $pass++ - $before = $families.Count - Write-Progress -Activity $ActivityName -Status "Discovering serviceFamily values (pass $pass)..." - $segment = Get-RetailPriceSegment -Filter $Filter -MeterRegion $MeterRegion -OnItem { - param($item) - if ($item.serviceFamily) { $families[$item.serviceFamily] = $true } - } - - # Single response (no NextPageLink): the scan is complete and deterministic, so - # repeating it cannot surface another family. Keyed off the page count rather - # than an item-count threshold, so a server page-size change can't mask a miss. - if ($segment.Pages -le 1) { $singlePage = $true; break } - - if ($families.Count -eq $before) { $stable++ } else { $stable = 0 } - Write-Verbose " Discovery pass $pass`: $($families.Count) families ($($families.Count - $before) new)" + $retryAfter = [int]$header.Delta.TotalSeconds } - - Write-Progress -Activity $ActivityName -Completed - return @{ - Families = [string[]]($families.Keys | Sort-Object) - Converged = ($singlePage -or $stable -ge $StablePasses) + elseif ($header.Date) + { + $retryAfter = [int]([Math]::Ceiling(($header.Date - [DateTimeOffset]::UtcNow).TotalSeconds)) } -} - -function Assert-DiscoveryConverged -{ - <# - .SYNOPSIS - Throws if a serviceFamily discovery scan was still finding new families when it - hit the pass cap. - - .DESCRIPTION - A shard list built from a non-converged discovery cannot be trusted to be - complete: a family that no pass ever saw is never fetched, and if it is also new - (so absent from the baseline that Add-BaselineShard restores from) neither - completeness guard can see the gap -- the run would publish silently partial - data. Called immediately after each discovery so an unpublishable run fails - before spending ~30 minutes of API calls on a fetch whose result is discarded. - #> - param( - [hashtable]$Discovery, - [string]$PriceType, - [int]$MaxPasses - ) - if (-not $Discovery.Converged) + if ($retryAfter -gt 0) { - throw "Aborting: $PriceType serviceFamily discovery did not converge within $MaxPasses passes (still finding new families). The shard list may be missing a family, so the run could publish incomplete data; refusing to continue. Increase `$MaxPassesPerShard if the API is churning." + return [Math]::Min($retryAfter, $MaxSeconds) } + return [int][Math]::Pow(2, $Attempt) * 10 } -function Add-BaselineShard +function Get-EligibleMeter { <# .SYNOPSIS - Unions discovered families with the families seen in the prior run's baseline. + Walks one price type in a single traversal, collecting eligible meterIds and + per-serviceFamily counts in the same pass. .DESCRIPTION - Get-ServiceFamily already repeats and unions its scans, so a family missed by one - pass is normally recovered by the next. This is the second line of defence for - families that were seen before: re-including every family from the prior baseline - guarantees a previously-seen family is always queried even if every discovery pass - misses it. The per-shard guard then catches any known family that genuinely shrank. - Returns the sorted, de-duplicated union. - - Note this cannot help a brand-new family -- it is in no baseline yet -- which is - why discovery itself has to converge rather than relying on this backstop. - #> - param( - [string[]]$Discovered, - [hashtable]$BaselineSection - ) - if (-not $BaselineSection) { return [string[]](@($Discovered) | Sort-Object -Unique) } - return [string[]](@($Discovered) + @($BaselineSection.Keys) | Sort-Object -Unique) -} + Returns @{ Keys = @{ meterId -> $true }; FamilyCounts = @{ family -> count }; + Items = ; Pages = }. -function Invoke-ShardedUnion -{ - <# - .SYNOPSIS - Runs a query sharded by serviceFamily, repeating each shard and unioning the - collected meterId keys until the set stabilizes (or a pass cap is hit). + $CollectKey receives an item and returns the meterId to record, or $null to skip + it (used to gate Savings Plan eligibility on a non-empty savingsPlan). - .DESCRIPTION - Returns a hashtable: Keys = @{ meterId -> $true } for all collected meters; - NonConverged = list of shards that hit the pass cap while still adding keys; - ShardCounts = @{ shard -> collected key count } for the per-shard guard. - $CollectKey receives an item and returns the meterId to record, or $null to - skip it (used to gate Savings Plan eligibility on a non-empty savingsPlan). + FamilyCounts counts DISTINCT COLLECTED meterIds per family, matching what the + sharded implementation persisted, so an existing baseline stays comparable. A + family bucket is created for every family seen even when it collects nothing, so + a family that legitimately has no eligible meters is recorded as 0 rather than + vanishing from the baseline. #> [Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter', '', Justification = 'CollectKey is invoked inside the nested -OnItem closure passed to Get-RetailPriceSegment, which the analyzer cannot trace. Target must be empty to suppress on PSScriptAnalyzer 1.x.')] param( - [string]$BaseFilter, - [string[]]$Shards, + [string]$Filter, [string]$MeterRegion, [scriptblock]$CollectKey, [string]$ActivityName ) - $union = @{} - $nonConverged = @() - $shardCounts = @{} - $shardNum = 0 + $keys = @{} + $familyKeys = @{} + # A scalar `$n++` inside the callback would bind to a NEW local in the scriptblock's + # invocation scope (PowerShell copies on write) and never accumulate, so the tick + # counter lives in a hashtable, whose entries the callback mutates in place. Keyed + # 'Seen' rather than 'Count' to avoid colliding with Hashtable's own Count property. + $tick = @{ Seen = 0 } - foreach ($shard in $Shards) - { - $shardNum++ - # URL-encode the serviceFamily value. In a query string an unescaped '+' is - # decoded server-side to a space, so a family like 'AI + Machine Learning' - # would match nothing and be silently dropped (168 meters today). The server - # decodes the escape back to the literal value before OData parsing. - $encodedShard = [uri]::EscapeDataString($shard) - $shardFilter = "$BaseFilter and serviceFamily eq '$encodedShard'" - $shardKeys = @{} - $stable = 0 - $pass = 0 - $itemsLastPass = 0 - - $singlePage = $false - while ($pass -lt $MaxPassesPerShard -and $stable -lt $StablePasses) - { - $pass++ - $before = $shardKeys.Count - $segment = Get-RetailPriceSegment -Filter $shardFilter -MeterRegion $MeterRegion -OnItem { - param($item) - $key = & $CollectKey $item - if ($key) { $shardKeys[$key.ToLowerInvariant()] = $true } - } - $itemsLastPass = $segment.Items + $segment = Get-RetailPriceSegment -Filter $Filter -MeterRegion $MeterRegion -OnItem { + param($item) - # A shard that fit in a single response (the API returned no NextPageLink) - # delivered its complete, deterministic set -- repeating cannot add anything, - # so stop. Only paginated shards are exposed to the unstable-order drop and - # need repeats. Keyed off the page count rather than a hardcoded page size, - # so a change to the server's default page size cannot mask a real miss. - if ($segment.Pages -le 1) { $singlePage = $true; break } + # Bucket every item's family, not just the collected ones, so a family with zero + # eligible meters is still recorded (as 0) in the persisted baseline. + $family = if ($item.serviceFamily) { $item.serviceFamily } else { '(none)' } + if (-not $familyKeys.ContainsKey($family)) { $familyKeys[$family] = @{} } - $added = $shardKeys.Count - $before - if ($added -eq 0) { $stable++ } else { $stable = 0 } - - $pct = [Math]::Min(100, [Math]::Floor($shardNum / $Shards.Count * 100)) - Write-Progress -Activity $ActivityName -Status "$shard (pass $pass): $($shardKeys.Count) keys" -PercentComplete $pct + $key = & $CollectKey $item + if ($key) + { + $k = $key.ToLowerInvariant() + $keys[$k] = $true + $familyKeys[$family][$k] = $true } - foreach ($k in $shardKeys.Keys) { $union[$k] = $true } - $shardCounts[$shard] = $shardKeys.Count - - if (-not $singlePage -and $stable -lt $StablePasses) + # Write-Progress per item is far too chatty for a 500k-item traversal. + $tick.Seen++ + if ($tick.Seen % 25000 -eq 0) { - $nonConverged += $shard - Write-Warning " Shard '$shard' did not converge within $MaxPassesPerShard passes (still adding keys)." + Write-Progress -Activity $ActivityName -Status "$($tick.Seen) items, $($keys.Count) eligible meters" } - Write-Host " $shard : $($shardKeys.Count) keys ($itemsLastPass items/pass, $pass pass(es))" } Write-Progress -Activity $ActivityName -Completed - return @{ Keys = $union; NonConverged = $nonConverged; ShardCounts = $shardCounts } + + $familyCounts = @{} + foreach ($family in $familyKeys.Keys) { $familyCounts[$family] = $familyKeys[$family].Count } + + return @{ + Keys = $keys + FamilyCounts = $familyCounts + Items = $segment.Items + Pages = $segment.Pages + } } -# Per-shard baseline (sidecar alongside the CSV). The published CSV carries only -# MeterId + the two flags -- no serviceFamily -- so per-shard counts from the last -# good run are persisted here to let the completeness guard catch a single shard +# Per-family baseline (sidecar alongside the CSV). The published CSV carries only +# MeterId + the two flags -- no serviceFamily -- so per-family counts from the last +# good run are persisted here to let the completeness guard catch a single family # that systematically under-fetches (which an aggregate-only check could miss when # growth elsewhere masks it). Committed/pushed beside the CSV so it survives the -# fresh checkout of each scheduled CI run. +# fresh checkout of each scheduled CI run. The 'shardcounts' name predates the removal +# of serviceFamily sharding and is retained because the workflow, the open-data CI path +# filter, and the packaging exclusion all key off it. $ShardCountPath = [System.IO.Path]::ChangeExtension($OutputPath, 'shardcounts.json') function Get-ShardShortfall { <# .SYNOPSIS - Returns per-shard guard violations: families whose collected count fell more + Returns per-family guard violations: families whose collected count fell more than $MaxShrinkFraction below the baseline. Empty when the baseline is absent (first run) or every family is within tolerance. .DESCRIPTION Iterates the BASELINE families, not the current run's, so a family that - disappears entirely this run (e.g. discovery missed it, or a rename) is caught - as a 100% shortfall against its baseline -- iterating the current set would - skip a vanished family and let the silent drop through. New families (present - now, absent from the baseline) need no check and are simply not iterated. + disappears entirely this run is caught as a 100% shortfall against its baseline + -- iterating the current set would skip a vanished family and let the silent + drop through. New families (present now, absent from the baseline) need no check + and are simply not iterated. #> param( [string]$Section, @@ -396,7 +308,7 @@ function Get-ShardShortfall foreach ($shard in $Baseline.Keys) { $prev = $Baseline[$shard] - if (-not $prev) { continue } # previously-empty shard: nothing to compare + if (-not $prev) { continue } # previously-empty family: nothing to compare # Ceiling, not Floor: Floor rounds the bound down, allowing up to ~1 extra row # of shrink -- and for a baseline of 1 it floors to 0, so a drop to 0 (the # family vanishing) would pass. Ceiling preserves the fractional bound and @@ -436,59 +348,45 @@ $cachedShardCounts = $null if (Test-Path $ShardCountPath) { $cachedShardCounts = Get-Content -Path $ShardCountPath -Raw | ConvertFrom-Json -AsHashtable - Write-Verbose " Loaded per-shard baseline from $ShardCountPath" + Write-Verbose " Loaded per-family baseline from $ShardCountPath" } # ----------------------------------------------------------------------- -# Step 2: Reservation-eligible meters (sharded by serviceFamily, union to stable) +# Step 2: Reservation-eligible meters (single traversal) # ----------------------------------------------------------------------- -Write-Host "Discovering Reservation serviceFamily values..." -$riDiscovery = Get-ServiceFamily -Filter "priceType eq 'Reservation'" -MeterRegion 'primary' -ActivityName 'Discovering Reservation families' -MaxPasses $MaxPassesPerShard -StablePasses $StablePasses -Assert-DiscoveryConverged -Discovery $riDiscovery -PriceType 'Reservation' -MaxPasses $MaxPassesPerShard -$riShards = Add-BaselineShard -Discovered $riDiscovery.Families -BaselineSection $cachedShardCounts['Reservation'] -Write-Host " Found $($riShards.Count) serviceFamily values: $($riShards -join ', ')" -Write-Host "Fetching Reservation prices (sharded by serviceFamily)..." -$riResult = Invoke-ShardedUnion -BaseFilter "priceType eq 'Reservation'" -Shards $riShards -MeterRegion 'primary' -ActivityName 'Fetching Reservation prices' -CollectKey { +Write-Host "Fetching Reservation prices..." +$riResult = Get-EligibleMeter -Filter "priceType eq 'Reservation'" -MeterRegion 'primary' -ActivityName 'Fetching Reservation prices' -CollectKey { param($item) $item.meterId } $riMeters = $riResult.Keys +Write-Host " Walked $($riResult.Items) items over $($riResult.Pages) pages across $($riResult.FamilyCounts.Count) service families" Write-Host " RI-eligible meters: $($riMeters.Count)" # ----------------------------------------------------------------------- -# Step 3: Savings Plan-eligible meters (sharded by serviceFamily, union to stable) +# Step 3: Savings Plan-eligible meters (single traversal) # The savingsPlan array is embedded in Consumption items, so we page through # primary Consumption meters and record those with a non-empty savingsPlan. # ----------------------------------------------------------------------- -Write-Host "Discovering Consumption serviceFamily values..." -$spDiscovery = Get-ServiceFamily -Filter "priceType eq 'Consumption'" -MeterRegion 'primary' -ActivityName 'Discovering Consumption families' -MaxPasses $MaxPassesPerShard -StablePasses $StablePasses -Assert-DiscoveryConverged -Discovery $spDiscovery -PriceType 'Consumption' -MaxPasses $MaxPassesPerShard -$spShards = Add-BaselineShard -Discovered $spDiscovery.Families -BaselineSection $cachedShardCounts['Consumption'] -Write-Host " Found $($spShards.Count) serviceFamily values: $($spShards -join ', ')" -Write-Host "Fetching Consumption prices (sharded by serviceFamily; checking for Savings Plan eligibility)..." -$spResult = Invoke-ShardedUnion -BaseFilter "priceType eq 'Consumption'" -Shards $spShards -MeterRegion 'primary' -ActivityName 'Fetching Consumption prices' -CollectKey { +Write-Host "Fetching Consumption prices (checking for Savings Plan eligibility)..." +$spResult = Get-EligibleMeter -Filter "priceType eq 'Consumption'" -MeterRegion 'primary' -ActivityName 'Fetching Consumption prices' -CollectKey { param($item) if ($item.savingsPlan -and $item.savingsPlan.Count -gt 0) { $item.meterId } else { $null } } $spMeters = $spResult.Keys +Write-Host " Walked $($spResult.Items) items over $($spResult.Pages) pages across $($spResult.FamilyCounts.Count) service families" Write-Host " SP-eligible meters: $($spMeters.Count)" # ----------------------------------------------------------------------- -# Step 3b: Completeness guard. Abort before writing if the run did not converge or -# the meter total dropped materially below the published total -- an incomplete run -# must never overwrite good open-data (a missed SP flag would flip Eligible -> Not). +# Step 3b: Completeness guard. Abort before writing if the meter total dropped +# materially below the published total -- an incomplete run must never overwrite +# good open-data (a missed SP flag would flip Eligible -> Not). # ----------------------------------------------------------------------- $seenSet = @{} foreach ($key in $riMeters.Keys) { $seenSet[$key] = $true } foreach ($key in $spMeters.Keys) { $seenSet[$key] = $true } $seenTotal = $seenSet.Count -$nonConverged = @($riResult.NonConverged + $spResult.NonConverged | Sort-Object -Unique) -if ($nonConverged.Count -gt 0) -{ - throw "Aborting before write: shard(s) did not converge within $MaxPassesPerShard passes: $($nonConverged -join ', '). Increase `$MaxPassesPerShard or sub-shard these families by serviceName." -} - if ($cachedTotal -gt 0) { $floor = [Math]::Ceiling($cachedTotal * (1 - $MaxShrinkFraction)) @@ -498,24 +396,30 @@ if ($cachedTotal -gt 0) } } -# Per-shard guard: a single family that systematically under-fetches can be hidden -# from the aggregate check by growth elsewhere, so compare each shard against its +# Per-family guard: a single family that systematically under-fetches can be hidden +# from the aggregate check by growth elsewhere, so compare each family against its # own baseline (when one exists from a prior run). $cachedShardCounts is a hashtable -# (ConvertFrom-Json -AsHashtable), so index its sections by key; null-safe when no -# baseline file exists. +# (ConvertFrom-Json -AsHashtable), so index its sections by key -- but it is $null +# until a baseline file exists, and PowerShell throws "Cannot index into a null array" +# rather than yielding $null, so the section lookup MUST be guarded. The baseline has +# never existed in CI (the job has never completed a successful run), so this is the +# path every first run takes. +$riBaseline = if ($cachedShardCounts) { $cachedShardCounts['Reservation'] } else { $null } +$spBaseline = if ($cachedShardCounts) { $cachedShardCounts['Consumption'] } else { $null } + $shardShortfall = @() -$shardShortfall += Get-ShardShortfall -Section 'Reservation' -Current $riResult.ShardCounts -Baseline $cachedShardCounts['Reservation'] -MaxShrinkFraction $MaxShrinkFraction -$shardShortfall += Get-ShardShortfall -Section 'Consumption' -Current $spResult.ShardCounts -Baseline $cachedShardCounts['Consumption'] -MaxShrinkFraction $MaxShrinkFraction +$shardShortfall += Get-ShardShortfall -Section 'Reservation' -Current $riResult.FamilyCounts -Baseline $riBaseline -MaxShrinkFraction $MaxShrinkFraction +$shardShortfall += Get-ShardShortfall -Section 'Consumption' -Current $spResult.FamilyCounts -Baseline $spBaseline -MaxShrinkFraction $MaxShrinkFraction if ($shardShortfall.Count -gt 0) { - throw "Aborting before write: shard(s) fell more than $([Math]::Round($MaxShrinkFraction * 100))% below baseline: $($shardShortfall -join '; '). A family likely under-fetched; refusing to overwrite published data. Raise -MaxShrinkFraction for a deliberate large change." + throw "Aborting before write: service family/families fell more than $([Math]::Round($MaxShrinkFraction * 100))% below baseline: $($shardShortfall -join '; '). A family likely under-fetched; refusing to overwrite published data. Raise -MaxShrinkFraction for a deliberate large change." } Write-Host "Completeness check passed: $seenTotal meters seen this run (cached $cachedTotal)." # ----------------------------------------------------------------------- -# Step 4: Build the output from this run's converged sets (write fresh) and -# summarize the change versus the previously published CSV. Meters present only in -# the old CSV are dropped (retired); the guard above already ensured the run is whole. +# Step 4: Build the output from this run's sets (write fresh) and summarize the +# change versus the previously published CSV. Meters present only in the old CSV are +# dropped (retired); the guard above already ensured the run is whole. # ----------------------------------------------------------------------- Write-Host "`nBuilding output..." $sortedIds = [string[]]($seenSet.Keys | Sort-Object) @@ -557,10 +461,10 @@ $rows | Export-Csv -Path $OutputPath -UseQuotes Always -NoTypeInformation -Encod Write-Verbose " CSV write completed in $([Math]::Round(([DateTime]::UtcNow - $writeStart).TotalSeconds, 1))s" Write-Host "Wrote $($rows.Count) meters to $OutputPath" -# Persist this run's per-shard counts as the baseline for the next run's guard. +# Persist this run's per-family counts as the baseline for the next run's guard. $newShardCounts = [ordered]@{ - Reservation = $riResult.ShardCounts - Consumption = $spResult.ShardCounts + Reservation = $riResult.FamilyCounts + Consumption = $spResult.FamilyCounts } $newShardCounts | ConvertTo-Json -Depth 4 | Set-Content -Path $ShardCountPath -Encoding utf8 -NoNewline -Write-Host "Wrote per-shard baseline to $ShardCountPath" +Write-Host "Wrote per-family baseline to $ShardCountPath" From 8bb06cd7238b2b855b7ff01e392fbcc5300efa1b Mon Sep 17 00:00:00 2001 From: Roland Krummenacher Date: Thu, 13 Aug 2026 07:31:33 +0200 Subject: [PATCH 02/13] fix: Sort baseline JSON keys and declare the PowerShell 7 requirement Addresses review feedback on #2251. Sort the per-family baseline by key before serializing. Hashtable enumeration order is not guaranteed, and the run that validated this PR wrote the families in neither alphabetical nor insertion order. Because the workflow treats ANY diff in shardcounts.json as "data changed", a reshuffle alone would push a branch and ask for a PR with no count actually having moved. ConvertTo-SortedMap makes the file a function of its contents alone. Add #Requires -Version 7.0. The script uses ConvertFrom-Json -AsHashtable, Export-Csv -UseQuotes, and reads Invoke-RestMethod's error response as an HttpResponseMessage, none of which work on Windows PowerShell 5.1. The workflow runs `shell: pwsh` so CI is unaffected; this only gives a clear message to someone running the script by hand. Unit tests go from 17 to 22, covering sort order, value preservation, empty input, family names with spaces and symbols, and the actual regression: that two maps differing only in insertion order serialize identically. Co-Authored-By: Claude Opus 5 (1M context) --- ...te-CommitmentDiscountEligibility.Tests.ps1 | 41 ++++++++++++++++++- .../Update-CommitmentDiscountEligibility.ps1 | 34 +++++++++++++-- 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 b/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 index 67c18cb8d..46cd255ea 100644 --- a/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 +++ b/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 @@ -10,7 +10,7 @@ Describe 'Update-CommitmentDiscountEligibility helpers' { BeforeAll { $scriptPath = Join-Path (Get-Item -Path $PSScriptRoot).Parent.Parent.Parent.Parent.FullName 'src/scripts/Update-CommitmentDiscountEligibility.ps1' $ast = [System.Management.Automation.Language.Parser]::ParseFile($scriptPath, [ref]$null, [ref]$null) - foreach ($name in 'Get-ShardShortfall', 'Get-RetryDelay', 'Get-EligibleMeter') + foreach ($name in 'Get-ShardShortfall', 'Get-RetryDelay', 'Get-EligibleMeter', 'ConvertTo-SortedMap') { $fn = $ast.FindAll({ param($n) $n -is [System.Management.Automation.Language.FunctionDefinitionAst] -and $n.Name -eq $name }, $true) | Select-Object -First 1 if (-not $fn) { throw "Function $name not found in $scriptPath" } @@ -18,6 +18,45 @@ Describe 'Update-CommitmentDiscountEligibility helpers' { } } + Context 'ConvertTo-SortedMap' { + It 'orders entries by key' { + $m = ConvertTo-SortedMap -Map @{ Storage = 3; Compute = 1; Analytics = 2 } + @($m.Keys) | Should -Be @('Analytics', 'Compute', 'Storage') + } + + It 'preserves every value' { + $m = ConvertTo-SortedMap -Map @{ Storage = 3; Compute = 1 } + $m['Compute'] | Should -Be 1 + $m['Storage'] | Should -Be 3 + } + + It 'serializes identically regardless of insertion order' { + # The actual regression: the workflow treats any diff in the baseline sidecar + # as "data changed", so an unstable key order would push a branch and ask for + # a PR even when no count moved. + $a = [ordered]@{} + 'Storage', 'Compute', 'Analytics' | ForEach-Object { $a[$_] = 1 } + $b = [ordered]@{} + 'Analytics', 'Storage', 'Compute' | ForEach-Object { $b[$_] = 1 } + + $jsonA = ConvertTo-SortedMap -Map ([hashtable]$a) | ConvertTo-Json -Depth 4 + $jsonB = ConvertTo-SortedMap -Map ([hashtable]$b) | ConvertTo-Json -Depth 4 + + $jsonA | Should -BeExactly $jsonB + } + + It 'handles an empty map' { + $m = ConvertTo-SortedMap -Map @{} + $m.Count | Should -Be 0 + } + + It 'sorts family names containing spaces and symbols' { + # Real family names include 'AI + Machine Learning' and 'Management and Governance'. + $m = ConvertTo-SortedMap -Map @{ 'Management and Governance' = 1; 'AI + Machine Learning' = 82 } + @($m.Keys)[0] | Should -Be 'AI + Machine Learning' + } + } + Context 'Get-ShardShortfall' { It 'flags a previously-nonempty family that vanishes (drops to zero)' { $v = Get-ShardShortfall -Section 'Reservation' -Current @{ Compute = 100 } -Baseline @{ Compute = 100; Tiny = 1 } -MaxShrinkFraction 0.15 diff --git a/src/scripts/Update-CommitmentDiscountEligibility.ps1 b/src/scripts/Update-CommitmentDiscountEligibility.ps1 index 3545b530a..0ce70e8d6 100644 --- a/src/scripts/Update-CommitmentDiscountEligibility.ps1 +++ b/src/scripts/Update-CommitmentDiscountEligibility.ps1 @@ -1,6 +1,13 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. +# PowerShell 7+ only: this script uses ConvertFrom-Json -AsHashtable, Export-Csv +# -UseQuotes, and reads Invoke-RestMethod's error response as an HttpResponseMessage +# (Windows PowerShell 5.1 surfaces an HttpWebResponse instead). Declaring it here fails +# fast with a clear message rather than a confusing downstream error. The workflow runs +# `shell: pwsh`, so this only affects someone running the script by hand. +#Requires -Version 7.0 + <# .SYNOPSIS Fetches commitment discount eligibility data from the Azure Retail Prices API. @@ -281,6 +288,26 @@ function Get-EligibleMeter # filter, and the packaging exclusion all key off it. $ShardCountPath = [System.IO.Path]::ChangeExtension($OutputPath, 'shardcounts.json') +function ConvertTo-SortedMap +{ + <# + .SYNOPSIS + Returns an ordered dictionary with the given hashtable's entries sorted by key. + + .DESCRIPTION + Hashtable key enumeration order is not guaranteed, so serializing one directly + can reorder the JSON between runs even when every count is identical. The + workflow treats ANY diff in the baseline sidecar as "data changed" and pushes a + branch asking for a PR, so an unstable key order would manufacture empty + update PRs. Sorting makes the file a function of its contents alone. + #> + param([hashtable]$Map) + + $sorted = [ordered]@{} + foreach ($key in ($Map.Keys | Sort-Object)) { $sorted[$key] = $Map[$key] } + return $sorted +} + function Get-ShardShortfall { <# @@ -461,10 +488,11 @@ $rows | Export-Csv -Path $OutputPath -UseQuotes Always -NoTypeInformation -Encod Write-Verbose " CSV write completed in $([Math]::Round(([DateTime]::UtcNow - $writeStart).TotalSeconds, 1))s" Write-Host "Wrote $($rows.Count) meters to $OutputPath" -# Persist this run's per-family counts as the baseline for the next run's guard. +# Persist this run's per-family counts as the baseline for the next run's guard, with +# the families in sorted order (see ConvertTo-SortedMap). $newShardCounts = [ordered]@{ - Reservation = $riResult.FamilyCounts - Consumption = $spResult.FamilyCounts + Reservation = ConvertTo-SortedMap -Map $riResult.FamilyCounts + Consumption = ConvertTo-SortedMap -Map $spResult.FamilyCounts } $newShardCounts | ConvertTo-Json -Depth 4 | Set-Content -Path $ShardCountPath -Encoding utf8 -NoNewline Write-Host "Wrote per-family baseline to $ShardCountPath" From 528a5f1f4221ebd497e65b80fd53b4decd1e5c90 Mon Sep 17 00:00:00 2001 From: Roland Krummenacher Date: Thu, 13 Aug 2026 08:31:21 +0200 Subject: [PATCH 03/13] feat: Allow dispatching the eligibility job against a branch, with a dry run This job runs weekly and takes roughly fifteen minutes to fail, and its checkout step was pinned to `ref: dev`, so a change to the fetch script could not be exercised end-to-end until after it merged. That is a poor feedback loop for a job that has not completed a successful run in months. Adds two workflow_dispatch inputs: - `ref` (default `dev`) selects the branch to check out and run from, so a script change can be validated from its PR branch. The scheduled run is unaffected: github.event.inputs is null for a schedule, so the expression falls back to dev. - `dry_run` (default false) runs the fetch and the change detection but skips the branch push, so a test leaves no stray opendata/* branches behind. It writes a summary showing what would have changed. dry_run is compared as a string in bash rather than tested in a GitHub `if:` expression, because a `type: boolean` input arrives as the literal "false", which is truthy in expression syntax and would have inverted the check. Only users with write access can dispatch a workflow, so the `ref` input does not widen who can run code in the repo. Verified by extracting the run block and exercising all four paths against a scratch repository: dry run with changes (detects both the modified CSV and a newly created sidecar, pushes nothing), no changes, scheduled run with an empty DRY_RUN, and an explicit DRY_RUN=false. The last two both take the push path. Co-Authored-By: Claude Opus 5 (1M context) --- .../opendata-commitment-eligibility.yml | 50 +++++++++++++++++-- 1 file changed, 47 insertions(+), 3 deletions(-) diff --git a/.github/workflows/opendata-commitment-eligibility.yml b/.github/workflows/opendata-commitment-eligibility.yml index 1ee211297..665be7048 100644 --- a/.github/workflows/opendata-commitment-eligibility.yml +++ b/.github/workflows/opendata-commitment-eligibility.yml @@ -2,8 +2,28 @@ name: Update Commitment Discount Eligibility on: schedule: - - cron: '0 6 * * 1' # Every Monday at 06:00 UTC + - cron: '0 6 * * 1' # Every Monday at 06:00 UTC workflow_dispatch: + inputs: + ref: + # The scheduled run always uses dev. This input exists so a change to the fetch + # script can be exercised end-to-end from its PR branch BEFORE merging, which + # matters for a job that only runs weekly and takes ~15 minutes to fail. Only + # users with write access can dispatch a workflow, so this does not widen who + # can run code in the repo. + description: 'Branch to check out and run the script from' + required: false + default: 'dev' + type: string + dry_run: + # Pair this with a non-dev ref when testing: the fetch and the change detection + # still run, but no opendata/* branch is pushed, so a test does not leave stray + # branches behind. A real run from a non-dev ref would also base its data branch + # on that ref, which is usually not what you want. + description: 'Fetch and detect changes only; do not push a branch' + required: false + default: false + type: boolean permissions: contents: write @@ -13,9 +33,10 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 60 steps: + # github.event.inputs is null for the scheduled run, so this falls back to dev. - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 with: - ref: dev + ref: ${{ github.event.inputs.ref || 'dev' }} - name: Fetch eligibility data from Azure Retail Prices API shell: pwsh @@ -25,6 +46,13 @@ jobs: # a branch and surface a one-click "create PR" link in the job summary for a # maintainer to open. Opening the PR triggers Open Data CI and normal review. - name: Push branch and surface PR link if data changed + env: + # Read as a string and compared as one below. A `type: boolean` input arrives + # here as the literal "true"/"false", and in a GitHub `if:` expression the + # string "false" is truthy -- comparing in bash sidesteps that trap. Empty for + # the scheduled run, which therefore takes the normal push path. + DRY_RUN: ${{ github.event.inputs.dry_run }} + RUN_REF: ${{ github.event.inputs.ref || 'dev' }} run: | # Check for changes to the CSV or the per-shard baseline (handles both # modified and newly-created/untracked files). The baseline is checked too @@ -35,6 +63,22 @@ jobs: exit 0 fi + if [ "$DRY_RUN" = "true" ]; then + { + echo "### Dry run: data changed, nothing pushed" + echo "" + echo "Ran the fetch against \`${RUN_REF}\` and detected changes, but \`dry_run\` was set, so no branch was pushed." + echo "" + echo '```' + git --no-pager diff --stat -- src/open-data/CommitmentDiscountEligibility.csv src/open-data/CommitmentDiscountEligibility.shardcounts.json || true + # --stat covers tracked files only, so list any newly created ones too. + git ls-files --others --exclude-standard -- src/open-data/ | grep -E 'CommitmentDiscountEligibility\.(csv|shardcounts\.json)' | sed 's/^/new file: /' || true + echo '```' + } >> "$GITHUB_STEP_SUMMARY" + echo "Dry run: changes detected, not pushing." + exit 0 + fi + BRANCH="opendata/commitment-eligibility-$(date +%Y%m%d)-${{ github.run_number }}" git config user.name "github-actions[bot]" git config user.email "github-actions[bot]@users.noreply.github.com" @@ -47,7 +91,7 @@ jobs: { echo "### Commitment discount eligibility data updated" echo "" - echo "Pushed branch \`${BRANCH}\`. GitHub Actions cannot open PRs in this repo, so open it manually:" + echo "Pushed branch \`${BRANCH}\` (fetched from \`${RUN_REF}\`). GitHub Actions cannot open PRs in this repo, so open it manually:" echo "" echo "[**Create pull request →**](${PR_URL})" echo "" From bcaff1544674e308489ac092deee899dcfaff7de Mon Sep 17 00:00:00 2001 From: Roland Krummenacher Date: Thu, 13 Aug 2026 09:02:25 +0200 Subject: [PATCH 04/13] docs: Correct the rationale on the baseline null guard The comment claimed the sidecar "has never existed in CI (the job has never completed a successful run)". That is wrong: #2164 committed CommitmentDiscountEligibility.shardcounts.json alongside the CSV on 2026-08-12, so it is present on dev and $cachedShardCounts is not null in a normal CI run. I hit the null path locally only because the validation harness copied just the CSV to a scratch path and not the sidecar. The guard is still correct and still needed -- a fresh -OutputPath or a removed sidecar reaches it -- but it would not have failed the scheduled run, as the commit message for 9dbe983e claimed. Comment-only change; the guard itself is unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- src/scripts/Update-CommitmentDiscountEligibility.ps1 | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/scripts/Update-CommitmentDiscountEligibility.ps1 b/src/scripts/Update-CommitmentDiscountEligibility.ps1 index 0ce70e8d6..70662b561 100644 --- a/src/scripts/Update-CommitmentDiscountEligibility.ps1 +++ b/src/scripts/Update-CommitmentDiscountEligibility.ps1 @@ -427,10 +427,11 @@ if ($cachedTotal -gt 0) # from the aggregate check by growth elsewhere, so compare each family against its # own baseline (when one exists from a prior run). $cachedShardCounts is a hashtable # (ConvertFrom-Json -AsHashtable), so index its sections by key -- but it is $null -# until a baseline file exists, and PowerShell throws "Cannot index into a null array" -# rather than yielding $null, so the section lookup MUST be guarded. The baseline has -# never existed in CI (the job has never completed a successful run), so this is the -# path every first run takes. +# when no baseline sidecar exists, and PowerShell throws "Cannot index into a null +# array" rather than yielding $null, so the section lookup MUST be guarded. The sidecar +# is committed next to the CSV, so a normal CI run does have one; this path is taken +# when running against a fresh -OutputPath (as a local test run does) or if the sidecar +# is ever removed. $riBaseline = if ($cachedShardCounts) { $cachedShardCounts['Reservation'] } else { $null } $spBaseline = if ($cachedShardCounts) { $cachedShardCounts['Consumption'] } else { $null } From 8597881614f4e09c500e96797d182996aef68349 Mon Sep 17 00:00:00 2001 From: Roland Krummenacher Date: Thu, 13 Aug 2026 09:24:50 +0200 Subject: [PATCH 05/13] refactor: Rename the shard vocabulary to family now that sharding is gone With serviceFamily sharding removed, "shard" is a misnomer. Worse, the code was mixing both vocabularies: Get-EligibleMeter returned FamilyCounts, which was stored via $newShardCounts into shardcounts.json and checked by Get-ShardShortfall. Consistent naming is clearer than either half. shardcounts.json -> familycounts.json (git mv, 100% rename) Get-ShardShortfall -> Get-FamilyShortfall $ShardCountPath -> $FamilyCountPath $cachedShardCounts -> $cachedFamilyCounts $newShardCounts -> $newFamilyCounts $shardShortfall -> $familyShortfall The .DESCRIPTION history section still says "sharded" where it describes the design that was removed, which is correct in the past tense. Two spots had to be right, and both were verified rather than assumed: - The existing sidecar is moved with git mv (confirmed 100% rename, content identical after line-ending normalisation) rather than left behind. Renaming only the path in the script would have silently dropped the per-family guard's baseline for a run, since Get-FamilyShortfall returns empty for a missing baseline instead of failing. - The packaging exclusion in Package-Toolkit.ps1 was re-tested live: with -Exclude '*.familycounts.json', a directory containing Regions.json, Other.json, and CommitmentDiscountEligibility.familycounts.json copies the first two only. A stale pattern would have shipped an internal operational baseline in release packages. Also updates the open-data CI path filter, both workflow references, and the regex alternations (keeping the \. escape). 22 unit tests pass, PSScriptAnalyzer clean, both workflow YAMLs parse. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/opendata-ci.yml | 2 +- .../opendata-commitment-eligibility.yml | 12 ++--- ...mentDiscountEligibility.familycounts.json} | 0 ...te-CommitmentDiscountEligibility.Tests.ps1 | 18 +++---- src/scripts/Package-Toolkit.ps1 | 4 +- .../Update-CommitmentDiscountEligibility.ps1 | 47 +++++++++---------- 6 files changed, 41 insertions(+), 42 deletions(-) rename src/open-data/{CommitmentDiscountEligibility.shardcounts.json => CommitmentDiscountEligibility.familycounts.json} (100%) diff --git a/.github/workflows/opendata-ci.yml b/.github/workflows/opendata-ci.yml index 6521f1826..4e8a7c4e3 100644 --- a/.github/workflows/opendata-ci.yml +++ b/.github/workflows/opendata-ci.yml @@ -8,7 +8,7 @@ on: - 'src/open-data/*.json' # Internal operational baselines for the eligibility completeness guard, not # reference data; Build-OpenData ignores them, so they shouldn't trigger CI. - - '!src/open-data/*.shardcounts.json' + - '!src/open-data/*.familycounts.json' permissions: contents: write pull-requests: write diff --git a/.github/workflows/opendata-commitment-eligibility.yml b/.github/workflows/opendata-commitment-eligibility.yml index 665be7048..84bccf1e1 100644 --- a/.github/workflows/opendata-commitment-eligibility.yml +++ b/.github/workflows/opendata-commitment-eligibility.yml @@ -54,11 +54,11 @@ jobs: DRY_RUN: ${{ github.event.inputs.dry_run }} RUN_REF: ${{ github.event.inputs.ref || 'dev' }} run: | - # Check for changes to the CSV or the per-shard baseline (handles both + # Check for changes to the CSV or the per-family baseline (handles both # modified and newly-created/untracked files). The baseline is checked too # so a counts-only update (no eligibility change) is still surfaced. - if git diff --quiet --exit-code -- src/open-data/CommitmentDiscountEligibility.csv src/open-data/CommitmentDiscountEligibility.shardcounts.json 2>/dev/null && \ - ! git ls-files --others --exclude-standard -- src/open-data/ | grep -qE 'CommitmentDiscountEligibility\.(csv|shardcounts\.json)'; then + if git diff --quiet --exit-code -- src/open-data/CommitmentDiscountEligibility.csv src/open-data/CommitmentDiscountEligibility.familycounts.json 2>/dev/null && \ + ! git ls-files --others --exclude-standard -- src/open-data/ | grep -qE 'CommitmentDiscountEligibility\.(csv|familycounts\.json)'; then echo "No changes detected" exit 0 fi @@ -70,9 +70,9 @@ jobs: echo "Ran the fetch against \`${RUN_REF}\` and detected changes, but \`dry_run\` was set, so no branch was pushed." echo "" echo '```' - git --no-pager diff --stat -- src/open-data/CommitmentDiscountEligibility.csv src/open-data/CommitmentDiscountEligibility.shardcounts.json || true + git --no-pager diff --stat -- src/open-data/CommitmentDiscountEligibility.csv src/open-data/CommitmentDiscountEligibility.familycounts.json || true # --stat covers tracked files only, so list any newly created ones too. - git ls-files --others --exclude-standard -- src/open-data/ | grep -E 'CommitmentDiscountEligibility\.(csv|shardcounts\.json)' | sed 's/^/new file: /' || true + git ls-files --others --exclude-standard -- src/open-data/ | grep -E 'CommitmentDiscountEligibility\.(csv|familycounts\.json)' | sed 's/^/new file: /' || true echo '```' } >> "$GITHUB_STEP_SUMMARY" echo "Dry run: changes detected, not pushing." @@ -83,7 +83,7 @@ jobs: git config user.name "github-actions[bot]" git config user.email "github-actions[bot]@users.noreply.github.com" git checkout -b "$BRANCH" - git add src/open-data/CommitmentDiscountEligibility.csv src/open-data/CommitmentDiscountEligibility.shardcounts.json + git add src/open-data/CommitmentDiscountEligibility.csv src/open-data/CommitmentDiscountEligibility.familycounts.json git commit -m "chore: Update commitment discount eligibility data" git push origin "$BRANCH" diff --git a/src/open-data/CommitmentDiscountEligibility.shardcounts.json b/src/open-data/CommitmentDiscountEligibility.familycounts.json similarity index 100% rename from src/open-data/CommitmentDiscountEligibility.shardcounts.json rename to src/open-data/CommitmentDiscountEligibility.familycounts.json diff --git a/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 b/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 index 46cd255ea..ff12fe6d7 100644 --- a/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 +++ b/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 @@ -10,7 +10,7 @@ Describe 'Update-CommitmentDiscountEligibility helpers' { BeforeAll { $scriptPath = Join-Path (Get-Item -Path $PSScriptRoot).Parent.Parent.Parent.Parent.FullName 'src/scripts/Update-CommitmentDiscountEligibility.ps1' $ast = [System.Management.Automation.Language.Parser]::ParseFile($scriptPath, [ref]$null, [ref]$null) - foreach ($name in 'Get-ShardShortfall', 'Get-RetryDelay', 'Get-EligibleMeter', 'ConvertTo-SortedMap') + foreach ($name in 'Get-FamilyShortfall', 'Get-RetryDelay', 'Get-EligibleMeter', 'ConvertTo-SortedMap') { $fn = $ast.FindAll({ param($n) $n -is [System.Management.Automation.Language.FunctionDefinitionAst] -and $n.Name -eq $name }, $true) | Select-Object -First 1 if (-not $fn) { throw "Function $name not found in $scriptPath" } @@ -57,30 +57,30 @@ Describe 'Update-CommitmentDiscountEligibility helpers' { } } - Context 'Get-ShardShortfall' { + Context 'Get-FamilyShortfall' { It 'flags a previously-nonempty family that vanishes (drops to zero)' { - $v = Get-ShardShortfall -Section 'Reservation' -Current @{ Compute = 100 } -Baseline @{ Compute = 100; Tiny = 1 } -MaxShrinkFraction 0.15 + $v = Get-FamilyShortfall -Section 'Reservation' -Current @{ Compute = 100 } -Baseline @{ Compute = 100; Tiny = 1 } -MaxShrinkFraction 0.15 @($v) | Should -HaveCount 1 $v | Should -Match 'Tiny' } - It 'flags a shard that shrinks beyond the tolerance' { - $v = Get-ShardShortfall -Section 'Reservation' -Current @{ Compute = 80 } -Baseline @{ Compute = 100 } -MaxShrinkFraction 0.15 + It 'flags a family that shrinks beyond the tolerance' { + $v = Get-FamilyShortfall -Section 'Reservation' -Current @{ Compute = 80 } -Baseline @{ Compute = 100 } -MaxShrinkFraction 0.15 @($v) | Should -HaveCount 1 } - It 'passes a shard within tolerance' { - $v = Get-ShardShortfall -Section 'Reservation' -Current @{ Compute = 90 } -Baseline @{ Compute = 100 } -MaxShrinkFraction 0.15 + It 'passes a family within tolerance' { + $v = Get-FamilyShortfall -Section 'Reservation' -Current @{ Compute = 90 } -Baseline @{ Compute = 100 } -MaxShrinkFraction 0.15 $v | Should -BeNullOrEmpty } It 'does not flag a brand-new family absent from the baseline' { - $v = Get-ShardShortfall -Section 'Reservation' -Current @{ Compute = 100; New = 5 } -Baseline @{ Compute = 100 } -MaxShrinkFraction 0.15 + $v = Get-FamilyShortfall -Section 'Reservation' -Current @{ Compute = 100; New = 5 } -Baseline @{ Compute = 100 } -MaxShrinkFraction 0.15 $v | Should -BeNullOrEmpty } It 'returns nothing when there is no baseline' { - $v = Get-ShardShortfall -Section 'Reservation' -Current @{ Compute = 1 } -Baseline $null -MaxShrinkFraction 0.15 + $v = Get-FamilyShortfall -Section 'Reservation' -Current @{ Compute = 1 } -Baseline $null -MaxShrinkFraction 0.15 $v | Should -BeNullOrEmpty } } diff --git a/src/scripts/Package-Toolkit.ps1 b/src/scripts/Package-Toolkit.ps1 index d30ee466e..80178a440 100644 --- a/src/scripts/Package-Toolkit.ps1 +++ b/src/scripts/Package-Toolkit.ps1 @@ -237,10 +237,10 @@ function Copy-OpenDataFiles() { Write-Verbose "Copying open data files..." Copy-Item "$PSScriptRoot/../open-data/*.csv" $relDir - # Exclude *.shardcounts.json: these are internal operational baselines for the + # Exclude *.familycounts.json: these are internal operational baselines for the # eligibility completeness guard (not reference data), so they must not ship in # the public release package alongside legitimate open data. - Copy-Item "$PSScriptRoot/../open-data/*.json" $relDir -Exclude '*.shardcounts.json' + Copy-Item "$PSScriptRoot/../open-data/*.json" $relDir -Exclude '*.familycounts.json' } function Copy-OpenDataFolders() diff --git a/src/scripts/Update-CommitmentDiscountEligibility.ps1 b/src/scripts/Update-CommitmentDiscountEligibility.ps1 index 70662b561..d720883c8 100644 --- a/src/scripts/Update-CommitmentDiscountEligibility.ps1 +++ b/src/scripts/Update-CommitmentDiscountEligibility.ps1 @@ -283,10 +283,9 @@ function Get-EligibleMeter # good run are persisted here to let the completeness guard catch a single family # that systematically under-fetches (which an aggregate-only check could miss when # growth elsewhere masks it). Committed/pushed beside the CSV so it survives the -# fresh checkout of each scheduled CI run. The 'shardcounts' name predates the removal -# of serviceFamily sharding and is retained because the workflow, the open-data CI path -# filter, and the packaging exclusion all key off it. -$ShardCountPath = [System.IO.Path]::ChangeExtension($OutputPath, 'shardcounts.json') +# fresh checkout of each scheduled CI run, and excluded from release packages by +# Package-Toolkit.ps1 -- it is an internal operational baseline, not published data. +$FamilyCountPath = [System.IO.Path]::ChangeExtension($OutputPath, 'familycounts.json') function ConvertTo-SortedMap { @@ -308,7 +307,7 @@ function ConvertTo-SortedMap return $sorted } -function Get-ShardShortfall +function Get-FamilyShortfall { <# .SYNOPSIS @@ -332,19 +331,19 @@ function Get-ShardShortfall $violations = @() if (-not $Baseline) { return $violations } - foreach ($shard in $Baseline.Keys) + foreach ($family in $Baseline.Keys) { - $prev = $Baseline[$shard] + $prev = $Baseline[$family] if (-not $prev) { continue } # previously-empty family: nothing to compare # Ceiling, not Floor: Floor rounds the bound down, allowing up to ~1 extra row # of shrink -- and for a baseline of 1 it floors to 0, so a drop to 0 (the # family vanishing) would pass. Ceiling preserves the fractional bound and # flags small-baseline regressions. $floor = [Math]::Ceiling($prev * (1 - $MaxShrinkFraction)) - $cur = if ($Current.ContainsKey($shard)) { $Current[$shard] } else { 0 } + $cur = if ($Current.ContainsKey($family)) { $Current[$family] } else { 0 } if ($cur -lt $floor) { - $violations += "$Section/$shard $cur (baseline $prev, floor $floor)" + $violations += "$Section/$family $cur (baseline $prev, floor $floor)" } } return $violations @@ -371,11 +370,11 @@ if (Test-Path $OutputPath) Write-Host " Previous CSV: $cachedTotal meters" } -$cachedShardCounts = $null -if (Test-Path $ShardCountPath) +$cachedFamilyCounts = $null +if (Test-Path $FamilyCountPath) { - $cachedShardCounts = Get-Content -Path $ShardCountPath -Raw | ConvertFrom-Json -AsHashtable - Write-Verbose " Loaded per-family baseline from $ShardCountPath" + $cachedFamilyCounts = Get-Content -Path $FamilyCountPath -Raw | ConvertFrom-Json -AsHashtable + Write-Verbose " Loaded per-family baseline from $FamilyCountPath" } # ----------------------------------------------------------------------- @@ -425,22 +424,22 @@ if ($cachedTotal -gt 0) # Per-family guard: a single family that systematically under-fetches can be hidden # from the aggregate check by growth elsewhere, so compare each family against its -# own baseline (when one exists from a prior run). $cachedShardCounts is a hashtable +# own baseline (when one exists from a prior run). $cachedFamilyCounts is a hashtable # (ConvertFrom-Json -AsHashtable), so index its sections by key -- but it is $null # when no baseline sidecar exists, and PowerShell throws "Cannot index into a null # array" rather than yielding $null, so the section lookup MUST be guarded. The sidecar # is committed next to the CSV, so a normal CI run does have one; this path is taken # when running against a fresh -OutputPath (as a local test run does) or if the sidecar # is ever removed. -$riBaseline = if ($cachedShardCounts) { $cachedShardCounts['Reservation'] } else { $null } -$spBaseline = if ($cachedShardCounts) { $cachedShardCounts['Consumption'] } else { $null } +$riBaseline = if ($cachedFamilyCounts) { $cachedFamilyCounts['Reservation'] } else { $null } +$spBaseline = if ($cachedFamilyCounts) { $cachedFamilyCounts['Consumption'] } else { $null } -$shardShortfall = @() -$shardShortfall += Get-ShardShortfall -Section 'Reservation' -Current $riResult.FamilyCounts -Baseline $riBaseline -MaxShrinkFraction $MaxShrinkFraction -$shardShortfall += Get-ShardShortfall -Section 'Consumption' -Current $spResult.FamilyCounts -Baseline $spBaseline -MaxShrinkFraction $MaxShrinkFraction -if ($shardShortfall.Count -gt 0) +$familyShortfall = @() +$familyShortfall += Get-FamilyShortfall -Section 'Reservation' -Current $riResult.FamilyCounts -Baseline $riBaseline -MaxShrinkFraction $MaxShrinkFraction +$familyShortfall += Get-FamilyShortfall -Section 'Consumption' -Current $spResult.FamilyCounts -Baseline $spBaseline -MaxShrinkFraction $MaxShrinkFraction +if ($familyShortfall.Count -gt 0) { - throw "Aborting before write: service family/families fell more than $([Math]::Round($MaxShrinkFraction * 100))% below baseline: $($shardShortfall -join '; '). A family likely under-fetched; refusing to overwrite published data. Raise -MaxShrinkFraction for a deliberate large change." + throw "Aborting before write: service family/families fell more than $([Math]::Round($MaxShrinkFraction * 100))% below baseline: $($familyShortfall -join '; '). A family likely under-fetched; refusing to overwrite published data. Raise -MaxShrinkFraction for a deliberate large change." } Write-Host "Completeness check passed: $seenTotal meters seen this run (cached $cachedTotal)." @@ -491,9 +490,9 @@ Write-Host "Wrote $($rows.Count) meters to $OutputPath" # Persist this run's per-family counts as the baseline for the next run's guard, with # the families in sorted order (see ConvertTo-SortedMap). -$newShardCounts = [ordered]@{ +$newFamilyCounts = [ordered]@{ Reservation = ConvertTo-SortedMap -Map $riResult.FamilyCounts Consumption = ConvertTo-SortedMap -Map $spResult.FamilyCounts } -$newShardCounts | ConvertTo-Json -Depth 4 | Set-Content -Path $ShardCountPath -Encoding utf8 -NoNewline -Write-Host "Wrote per-family baseline to $ShardCountPath" +$newFamilyCounts | ConvertTo-Json -Depth 4 | Set-Content -Path $FamilyCountPath -Encoding utf8 -NoNewline +Write-Host "Wrote per-family baseline to $FamilyCountPath" From 055cc2cfedc39f1cbce06e7dcbbd49044feb8365 Mon Sep 17 00:00:00 2001 From: Roland Krummenacher Date: Thu, 13 Aug 2026 16:22:46 +0200 Subject: [PATCH 06/13] fix: Reject a data push from a non-dev ref instead of just discouraging it Addresses review feedback on #2251. The ref/dry_run inputs made the unsafe combination the easiest one to invoke: dry_run defaulted to false, so the natural branch-testing flow (set only `ref`) checked out the feature branch, created the opendata/* branch FROM it, and generated a compare-to-dev URL carrying every feature commit alongside the data update. A comment saying to pair the inputs is not a control. Both of the suggested remedies are applied, because they cover different mistakes: - dry_run now defaults to true, so a real publish is always a deliberate choice rather than the path of least resistance. - A guard step rejects ref != dev unless dry_run is true, which also covers someone who unticks dry_run out of habit. The guard runs BEFORE checkout, so an invalid combination costs seconds instead of the ~35 minutes the fetch now takes. The scheduled run is unaffected: a schedule has no inputs object at all, so DRY_RUN is empty (not the "true" default) and RUN_REF falls back to dev, which takes the normal push path. Verified across all ten input combinations, including the edge cases the string comparison exists to handle: "dev-experiment" and "DEV" are both correctly rejected, and the literal string "false" -- truthy in a GitHub if: expression, which is why this is compared in bash -- does not bypass the guard. Co-Authored-By: Claude Opus 5 (1M context) --- .../opendata-commitment-eligibility.yml | 42 ++++++++++++++----- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/.github/workflows/opendata-commitment-eligibility.yml b/.github/workflows/opendata-commitment-eligibility.yml index 1fa00d1db..18b3ff2b6 100644 --- a/.github/workflows/opendata-commitment-eligibility.yml +++ b/.github/workflows/opendata-commitment-eligibility.yml @@ -8,21 +8,25 @@ on: ref: # The scheduled run always uses dev. This input exists so a change to the fetch # script can be exercised end-to-end from its PR branch BEFORE merging, which - # matters for a job that only runs weekly and takes ~15 minutes to fail. Only + # matters for a job that only runs weekly and takes ~35 minutes to fail. Only # users with write access can dispatch a workflow, so this does not widen who # can run code in the repo. - description: 'Branch to check out and run the script from' + # + # A non-dev ref is TEST-ONLY and is enforced as such by the guard step below: + # pushing a data branch built from a feature ref would carry that ref's code + # commits into the data PR, so the combination is rejected rather than merely + # discouraged. + description: 'Branch to check out and run the script from (non-dev requires dry_run)' required: false default: 'dev' type: string dry_run: - # Pair this with a non-dev ref when testing: the fetch and the change detection - # still run, but no opendata/* branch is pushed, so a test does not leave stray - # branches behind. A real run from a non-dev ref would also base its data branch - # on that ref, which is usually not what you want. + # Defaults to true so the safe path is also the default one. A real data push is + # therefore always an explicit choice, and the scheduled run (where this input is + # absent entirely, not false) is unaffected -- see the DRY_RUN env note below. description: 'Fetch and detect changes only; do not push a branch' required: false - default: false + default: true type: boolean permissions: @@ -31,8 +35,25 @@ permissions: jobs: update: runs-on: ubuntu-latest - timeout-minutes: 60 + # Two full traversals per price type (fetch + verification) run ~35-40 minutes; the + # headroom above that absorbs Retry-After backoff on a throttled run. + timeout-minutes: 90 steps: + # Fail before the ~35-minute fetch rather than after it, so an invalid input + # combination costs seconds instead of most of an hour. + - name: Validate dispatch inputs + env: + DRY_RUN: ${{ github.event.inputs.dry_run }} + RUN_REF: ${{ github.event.inputs.ref || 'dev' }} + run: | + if [ "$RUN_REF" != "dev" ] && [ "$DRY_RUN" != "true" ]; then + echo "::error::ref='$RUN_REF' is not 'dev' and dry_run is not true. A push from a" \ + "non-dev ref would base the opendata/* branch on that ref, so the data PR" \ + "would carry its code commits as well as the data update. Re-run with" \ + "dry_run enabled to test a branch, or with ref='dev' to publish data." + exit 1 + fi + # github.event.inputs is null for the scheduled run, so this falls back to dev. - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -49,8 +70,9 @@ jobs: env: # Read as a string and compared as one below. A `type: boolean` input arrives # here as the literal "true"/"false", and in a GitHub `if:` expression the - # string "false" is truthy -- comparing in bash sidesteps that trap. Empty for - # the scheduled run, which therefore takes the normal push path. + # string "false" is truthy -- comparing in bash sidesteps that trap. For the + # SCHEDULED run there is no inputs object at all, so this is empty (not the + # "true" default), and the run therefore takes the normal push path. DRY_RUN: ${{ github.event.inputs.dry_run }} RUN_REF: ${{ github.event.inputs.ref || 'dev' }} run: | From 99f409ecdd7815c4aecfad7099fdbc8aa9d7eecc Mon Sep 17 00:00:00 2001 From: Roland Krummenacher Date: Thu, 13 Aug 2026 16:23:11 +0200 Subject: [PATCH 07/13] fix: Verify each traversal against a second pass instead of trusting counts Addresses review feedback on #2251. The per-family guard was presented as the replacement for the convergence loop, and it cannot carry that claim. It compares one traversal against the PREVIOUS RUN's counts with a 15% tolerance, so a paging regression still passes when it drops meters uniformly under the threshold, when it drops old meters while new ones in the same family offset the count, or when it misses brand-new meters that have no baseline at all. Step 4 then rewrites the CSV from the incomplete sets. The docstring's promise that instability would "abort loudly instead of publishing partial data" was therefore not true as written. Each price type is now traversed twice and the two meterId sets are compared directly. That is a real completeness signal: it needs no historical baseline, so it catches all three cases above on the run that suffers them. - The published set is the UNION of both passes. A meter missed by exactly one pass is real and belongs in the output; the intersection would let a faulty pass silently delete rows, which is the failure being guarded against. - Per-family SETS are unioned too, not the counts. Taking the max of two counts would undercount a family whose passes each missed a different meter (pass 1 sees {a,b}, pass 2 sees {a,c}: union 3, max 2), so Get-EligibleMeter now returns FamilyKeys and FamilyCounts is derived from it. - The symmetric difference is reported on EVERY run, not just failures. A drift that climbs week over week while staying under the threshold is the early warning that the June-2026 fault is returning, and that is only visible if the passing value is logged. - -MaxVerifyDrift (default 0.1%) separates genuine catalogue churn over the ~20 minutes the two passes span from a real paging fault, which dropped a scattered percentage of rows. Set it to 0 to require exact agreement. Reservation is verified first: at ~3 minutes per pass against Consumption's ~17, an unstable API aborts the run after ~7 minutes rather than ~40. The historical count guards are kept and re-documented as a backstop rather than the completeness signal, because they catch the one thing set comparison structurally cannot -- a DETERMINISTIC omission shared by both passes, which the comparison sees as perfect agreement. Cost is 2x the item volume (~35-40 min, timeout raised 60 -> 90). That is still well inside what the removed workaround cost: it needed a full discovery traversal per price type (25 of 60 minutes on its own) before repeat-until-stable across 22 shards, and it timed out. Two bounded passes is the difference between "no completeness signal" and "a bounded one". Unit tests go from 22 to 32, driving the verification through each failure mode a count comparison cannot see -- an offsetting drop and add that leaves the total unchanged, a meter missed by either pass, the exact-tolerance boundary, per-family set unioning, and the empty-result divide-by-zero path. Co-Authored-By: Claude Opus 5 (1M context) --- ...te-CommitmentDiscountEligibility.Tests.ps1 | 159 +++++++++++- .../Update-CommitmentDiscountEligibility.ps1 | 240 +++++++++++++++--- 2 files changed, 364 insertions(+), 35 deletions(-) diff --git a/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 b/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 index ff12fe6d7..9df60a696 100644 --- a/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 +++ b/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 @@ -10,7 +10,7 @@ Describe 'Update-CommitmentDiscountEligibility helpers' { BeforeAll { $scriptPath = Join-Path (Get-Item -Path $PSScriptRoot).Parent.Parent.Parent.Parent.FullName 'src/scripts/Update-CommitmentDiscountEligibility.ps1' $ast = [System.Management.Automation.Language.Parser]::ParseFile($scriptPath, [ref]$null, [ref]$null) - foreach ($name in 'Get-FamilyShortfall', 'Get-RetryDelay', 'Get-EligibleMeter', 'ConvertTo-SortedMap') + foreach ($name in 'Get-FamilyShortfall', 'Get-RetryDelay', 'Get-EligibleMeter', 'ConvertTo-SortedMap', 'Get-VerifiedEligibleMeter') { $fn = $ast.FindAll({ param($n) $n -is [System.Management.Automation.Language.FunctionDefinitionAst] -and $n.Name -eq $name }, $true) | Select-Object -First 1 if (-not $fn) { throw "Function $name not found in $scriptPath" } @@ -226,5 +226,162 @@ Describe 'Update-CommitmentDiscountEligibility helpers' { $r.Pages | Should -Be 1 } + + It 'exposes the per-family key sets, not just the counts' { + # Get-VerifiedEligibleMeter merges two passes by unioning these sets; counts + # alone cannot be merged correctly. + $script:items = @( + @{ meterId = 'm1'; serviceFamily = 'Compute' }, + @{ meterId = 'm2'; serviceFamily = 'Compute' } + ) + + $r = Get-EligibleMeter -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -CollectKey $script:allMeters + + $r.FamilyKeys['Compute'].ContainsKey('m1') | Should -BeTrue + $r.FamilyKeys['Compute'].ContainsKey('m2') | Should -BeTrue + } + } + + Context 'Get-VerifiedEligibleMeter' { + # The verification traversal is the completeness signal, so these tests drive it + # through the failure modes a historical count comparison cannot see: a uniform + # drop, an offsetting drop/add, and a missed brand-new meter. + BeforeEach { + # Each call to the stub replays the next scripted pass, so pass 1 and pass 2 + # can be made to disagree. + function Get-RetailPriceSegment + { + [Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter', '', + Justification = 'Stub mirrors the real Get-RetailPriceSegment signature; only OnItem is exercised')] + param($Filter, $MeterRegion, $OnItem) + $pass = $script:passes[$script:passIndex] + $script:passIndex++ + foreach ($i in $pass) { & $OnItem $i } + return @{ Items = @($pass).Count; Pages = 1 } + } + + # Defined here rather than at Context scope: Pester does not surface a bare + # function declaration in a Context block to the It blocks inside it. + function Get-TestMeter + { + param([string]$Id, [string]$Family = 'Compute') + return @{ meterId = $Id; serviceFamily = $Family } + } + + $script:passIndex = 0 + $script:allMeters = { param($item) $item.meterId } + } + + It 'returns the meters and zero drift when both passes agree' { + $pass = @((Get-TestMeter 'm1'), (Get-TestMeter 'm2')) + $script:passes = @($pass, $pass) + + $r = Get-VerifiedEligibleMeter -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -MaxVerifyDrift 0.001 -CollectKey $script:allMeters + + $r.Keys.Count | Should -Be 2 + $r.VerifyDrift | Should -Be 0 + $r.OnlyFirst | Should -Be 0 + $r.OnlySecond | Should -Be 0 + } + + It 'recovers a meter that pass 2 missed rather than dropping it' { + # Publishing the intersection would let a faulty pass silently delete rows. + $script:passes = @( + @((Get-TestMeter 'm1'), (Get-TestMeter 'm2')), + @((Get-TestMeter 'm1')) + ) + + $r = Get-VerifiedEligibleMeter -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -MaxVerifyDrift 1.0 -CollectKey $script:allMeters + + $r.Keys.Count | Should -Be 2 + $r.Keys.ContainsKey('m2') | Should -BeTrue + $r.OnlyFirst | Should -Be 1 + $r.OnlySecond | Should -Be 0 + } + + It 'recovers a meter that pass 1 missed' { + $script:passes = @( + @((Get-TestMeter 'm1')), + @((Get-TestMeter 'm1'), (Get-TestMeter 'm2')) + ) + + $r = Get-VerifiedEligibleMeter -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -MaxVerifyDrift 1.0 -CollectKey $script:allMeters + + $r.Keys.Count | Should -Be 2 + $r.OnlyFirst | Should -Be 0 + $r.OnlySecond | Should -Be 1 + } + + It 'aborts when the passes disagree beyond the tolerance' { + $script:passes = @( + @((Get-TestMeter 'm1'), (Get-TestMeter 'm2')), + @((Get-TestMeter 'm1')) + ) + + # Drift is 1/2 = 50%, far above the default. + { Get-VerifiedEligibleMeter -Filter "priceType eq 'Reservation'" -MeterRegion 'primary' -ActivityName 'test' -MaxVerifyDrift 0.001 -CollectKey $script:allMeters } | + Should -Throw -ExpectedMessage '*disagreed on 1 of 2 meters*' + } + + It 'catches an offsetting drop and add that leaves the count unchanged' { + # The failure mode a count-based guard structurally cannot see: same total, + # different members. + $script:passes = @( + @((Get-TestMeter 'm1'), (Get-TestMeter 'm2')), + @((Get-TestMeter 'm1'), (Get-TestMeter 'm3')) + ) + + { Get-VerifiedEligibleMeter -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -MaxVerifyDrift 0.001 -CollectKey $script:allMeters } | + Should -Throw -ExpectedMessage '*disagreed on 2 of 3 meters*' + } + + It 'passes when drift is exactly at the tolerance' { + # 1 of 100 = 1%, compared with -gt so the boundary is inclusive. + $first = 1..99 | ForEach-Object { Get-TestMeter "m$_" } + $script:passes = @( + @($first + @(Get-TestMeter 'm100')), + @($first) + ) + + $r = Get-VerifiedEligibleMeter -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -MaxVerifyDrift 0.01 -CollectKey $script:allMeters + + $r.VerifyDrift | Should -Be 0.01 + $r.Keys.Count | Should -Be 100 + } + + It 'unions per-family sets instead of taking the larger count' { + # Each pass sees two Compute meters, but not the same two. The merged count + # must be 3; max(2,2) would report 2 and understate the family. + $script:passes = @( + @((Get-TestMeter 'm1'), (Get-TestMeter 'm2')), + @((Get-TestMeter 'm1'), (Get-TestMeter 'm3')) + ) + + $r = Get-VerifiedEligibleMeter -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -MaxVerifyDrift 1.0 -CollectKey $script:allMeters + + $r.FamilyCounts['Compute'] | Should -Be 3 + } + + It 'reports zero drift for an empty result rather than dividing by zero' { + $script:passes = @(@(), @()) + + $r = Get-VerifiedEligibleMeter -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -MaxVerifyDrift 0.001 -CollectKey $script:allMeters + + $r.VerifyDrift | Should -Be 0 + $r.Keys.Count | Should -Be 0 + } + + It 'runs a single pass when -SkipVerify is set' { + $script:passes = @( + @((Get-TestMeter 'm1')), + @((Get-TestMeter 'm1'), (Get-TestMeter 'm2')) + ) + + $r = Get-VerifiedEligibleMeter -Filter 'x' -MeterRegion 'primary' -ActivityName 'test' -MaxVerifyDrift 0.001 -SkipVerify -CollectKey $script:allMeters -WarningAction SilentlyContinue + + # Only pass 1 consumed, and the second (disagreeing) pass never ran. + $script:passIndex | Should -Be 1 + $r.Keys.Count | Should -Be 1 + } } } diff --git a/src/scripts/Update-CommitmentDiscountEligibility.ps1 b/src/scripts/Update-CommitmentDiscountEligibility.ps1 index d720883c8..a51a77d60 100644 --- a/src/scripts/Update-CommitmentDiscountEligibility.ps1 +++ b/src/scripts/Update-CommitmentDiscountEligibility.ps1 @@ -17,19 +17,35 @@ Reserved Instances and/or Savings Plans. Outputs a CSV file that can be used as open data for FinOps Hub ingestion and PowerShell module lookups. - Each price type is fetched in a single traversal that follows the documented - NextPageLink verbatim -- no $top, no $orderby (a multi-field $orderby is rejected - by the API with HTTP 400), no client-side $skip. The same pass records both the - eligible meterIds and a per-serviceFamily count, so the completeness guard below - gets its per-family baseline for free. - - Data integrity rests on the completeness guard, not on re-fetching. Before writing, - the run is compared against the previously published data in aggregate AND per - serviceFamily; either check falling more than -MaxShrinkFraction below its baseline - aborts the run, so an incomplete fetch can never overwrite good open data with - missing eligibility. The per-family check is what catches a single family that - silently under-fetches, which an aggregate-only floor would miss when growth - elsewhere masks it. + Each price type is fetched by a traversal that follows the documented NextPageLink + verbatim -- no $top, no $orderby (a multi-field $orderby is rejected by the API with + HTTP 400), no client-side $skip. The same pass records both the eligible meterIds and + the per-serviceFamily key sets, so the historical guards below get their per-family + baseline for free. + + Data integrity rests on TWO independent mechanisms, because neither alone is + sufficient: + + 1. Verification traversal (the completeness signal). Every price type is traversed + TWICE and the two meterId sets are compared directly. This is what actually + detects an incomplete fetch: it needs no historical baseline, so unlike a count + comparison it catches a paging regression that drops meters uniformly, one that + drops old meters while new ones offset the count, and one that misses brand-new + meters entirely. The published set is the UNION of both passes, so a meter missed + by one pass is still recovered. The symmetric difference between the passes is + reported on every run and aborts it above -MaxVerifyDrift. + + A small non-zero difference is expected rather than alarming: the catalogue is + live, so a meter may legitimately appear or disappear during the ~20 minutes + between the start of pass 1 and the end of pass 2. -MaxVerifyDrift separates that + churn from a genuine paging fault. + + 2. Historical guards (the regression net). The run is compared against the previously + published data in aggregate AND per serviceFamily; either falling more than + -MaxShrinkFraction below its baseline aborts before writing. These are retained + because they catch what the verification traversal structurally cannot: a + DETERMINISTIC omission that both passes share, which set comparison sees as + perfect agreement. They are a backstop, not the completeness signal. History: this script previously sharded each query by serviceFamily and repeated every shard until its meterId set stopped growing, to work around a period when the @@ -40,32 +56,62 @@ telemetry in CI on 2026-08-12 showed every repeat pass across all 22 shards adding exactly zero meters while costing 2-3x the traversal volume -- and the shard list required a full discovery traversal per price type, which alone consumed 25 of the - job's 60 minutes and pushed the run into its timeout. The workaround was therefore - removed in favour of the guards. If the instability returns, the guards abort the - run loudly rather than publishing partial data; that is the intended behaviour and - the signal to reopen the case. + job's 60 minutes and pushed the run into its timeout. + + What replaced it is bounded rather than absent: exactly two passes, no shard + discovery, so the cost is 2x the item volume instead of the old unbounded repeat- + until-stable across 22 shards on top of discovery. If the instability returns, the + verification traversal aborts the run loudly rather than publishing partial data; + that is the intended behaviour and the signal to reopen the case. .PARAMETER OutputPath Path to the output CSV file. Defaults to src/open-data/CommitmentDiscountEligibility.csv. + .PARAMETER MaxShrinkFraction + Historical guard tolerance. See the .DESCRIPTION notes on mechanism 2. + + .PARAMETER MaxVerifyDrift + Verification traversal tolerance. See the .DESCRIPTION notes on mechanism 1. + + .PARAMETER SkipVerify + Skips the verification traversal, halving the runtime. For local iteration only -- + it disables the completeness signal, so the output must not be published. + .EXAMPLE ./Update-CommitmentDiscountEligibility.ps1 .EXAMPLE ./Update-CommitmentDiscountEligibility.ps1 -OutputPath ./output/eligibility.csv + + .EXAMPLE + ./Update-CommitmentDiscountEligibility.ps1 -OutputPath ./scratch/eligibility.csv -SkipVerify + Fast local run for iterating on the script. Not publishable -- see -SkipVerify. #> [CmdletBinding()] param( [string]$OutputPath = "$PSScriptRoot/../open-data/CommitmentDiscountEligibility.csv", - # Completeness guard: abort without writing if this run's meter total falls more + # Historical guard: abort without writing if this run's meter total falls more # than this fraction below the previously published row count. Raise it for a # deliberate large change (e.g. an initial migration that sheds retired meters). # Constrained to 0..1: a value >1 would make the floor negative and silently # disable the guard. [ValidateRange(0.0, 1.0)] - [double]$MaxShrinkFraction = 0.15 + [double]$MaxShrinkFraction = 0.15, + + # Verification guard: abort if the two traversals of a price type disagree on more + # than this fraction of their combined meterId set. Sized to pass genuine catalogue + # churn over the ~20 minutes the two passes span (empirically a handful of meters + # out of ~92k, i.e. well under 0.01%) while failing the June-2026 style fault, which + # dropped a scattered PERCENTAGE of rows. Set to 0 to require exact agreement. + [ValidateRange(0.0, 1.0)] + [double]$MaxVerifyDrift = 0.001, + + # Local-iteration escape hatch. Halves the runtime by skipping the verification + # traversal, which is the completeness signal -- output from such a run must not be + # published. CI never sets this. + [switch]$SkipVerify ) $ErrorActionPreference = 'Stop' @@ -212,12 +258,20 @@ function Get-EligibleMeter per-serviceFamily counts in the same pass. .DESCRIPTION - Returns @{ Keys = @{ meterId -> $true }; FamilyCounts = @{ family -> count }; - Items = ; Pages = }. + Returns @{ Keys = @{ meterId -> $true }; FamilyKeys = @{ family -> @{ meterId -> + $true } }; FamilyCounts = @{ family -> count }; Items = ; + Pages = }. $CollectKey receives an item and returns the meterId to record, or $null to skip it (used to gate Savings Plan eligibility on a non-empty savingsPlan). + FamilyKeys is returned alongside FamilyCounts because two passes cannot be merged + from counts alone -- unioning the meterId sets is what makes the merged per-family + count correct rather than an approximation. Taking the max of the two counts would + UNDERCOUNT a family whose passes each missed a different meter (pass 1 sees {a,b}, + pass 2 sees {a,c}: the union is 3, the max is 2). FamilyCounts is derived from the + sets for callers that only need the totals. + FamilyCounts counts DISTINCT COLLECTED meterIds per family, matching what the sharded implementation persisted, so an existing baseline stays comparable. A family bucket is created for every family seen even when it collects nothing, so @@ -272,12 +326,116 @@ function Get-EligibleMeter return @{ Keys = $keys + FamilyKeys = $familyKeys FamilyCounts = $familyCounts Items = $segment.Items Pages = $segment.Pages } } +function Get-VerifiedEligibleMeter +{ + <# + .SYNOPSIS + Traverses one price type twice and returns the union of the two passes, aborting + if they disagree on more than $MaxVerifyDrift of their combined meterId set. + + .DESCRIPTION + This is the completeness signal described in the script's .DESCRIPTION. It is + deliberately independent of any historical baseline: the two passes are compared + against EACH OTHER, so a paging fault is caught on the run that suffers it rather + than inferred from how the totals moved since last week. + + The published set is the UNION, not the intersection: if exactly one pass missed a + meter, the meter is real and belongs in the output. The intersection would let a + faulty pass silently delete rows, which is the very failure this guards against. + + Per-family sets are unioned in the same way so the persisted baseline describes + the data actually written. + + Returns the same shape as Get-EligibleMeter, plus @{ VerifyDrift = ; + OnlyFirst = ; OnlySecond = }. + #> + [Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter', '', + Justification = 'Filter/MeterRegion/CollectKey/ActivityName are forwarded to Get-EligibleMeter via splatting, which the analyzer cannot trace. Target must be empty to suppress on PSScriptAnalyzer 1.x.')] + param( + [string]$Filter, + [string]$MeterRegion, + [scriptblock]$CollectKey, + [string]$ActivityName, + [double]$MaxVerifyDrift, + [switch]$SkipVerify + ) + + $fetchArgs = @{ + Filter = $Filter + MeterRegion = $MeterRegion + CollectKey = $CollectKey + } + + $first = Get-EligibleMeter @fetchArgs -ActivityName $ActivityName + Write-Host " Pass 1: walked $($first.Items) items over $($first.Pages) pages, $($first.Keys.Count) eligible meters across $($first.FamilyCounts.Count) service families" + + if ($SkipVerify) + { + Write-Warning " Verification traversal SKIPPED (-SkipVerify). Output is not publishable." + return $first + @{ VerifyDrift = [double]0; OnlyFirst = 0; OnlySecond = 0 } + } + + $second = Get-EligibleMeter @fetchArgs -ActivityName "$ActivityName (verification)" + Write-Host " Pass 2: walked $($second.Items) items over $($second.Pages) pages, $($second.Keys.Count) eligible meters across $($second.FamilyCounts.Count) service families" + + # Symmetric difference. Counted in both directions because they mean different + # things: OnlySecond is a meter pass 1 missed (or one that appeared mid-run), + # OnlyFirst is a meter pass 2 missed (or one retired mid-run). Either direction is + # evidence of a fault when it is large. + $onlyFirst = 0 + foreach ($key in $first.Keys.Keys) { if (-not $second.Keys.ContainsKey($key)) { $onlyFirst++ } } + $onlySecond = 0 + foreach ($key in $second.Keys.Keys) { if (-not $first.Keys.ContainsKey($key)) { $onlySecond++ } } + + $union = @{} + foreach ($key in $first.Keys.Keys) { $union[$key] = $true } + foreach ($key in $second.Keys.Keys) { $union[$key] = $true } + + # Guard the division: an empty union would otherwise throw rather than report a + # (correctly) zero drift. + $drift = if ($union.Count -gt 0) { ($onlyFirst + $onlySecond) / $union.Count } else { [double]0 } + + Write-Host " Verification: $($union.Count) meters in union, $onlyFirst only in pass 1, $onlySecond only in pass 2 (drift $([Math]::Round($drift * 100, 4))%)" + + if ($drift -gt $MaxVerifyDrift) + { + throw "Aborting before write: the two traversals of '$Filter' disagreed on $($onlyFirst + $onlySecond) of $($union.Count) meters (drift $([Math]::Round($drift * 100, 4))%, max $([Math]::Round($MaxVerifyDrift * 100, 4))%). The API is likely paging inconsistently again (see support case 2606030050003725); refusing to publish a possibly incomplete set." + } + + # Union the per-family sets so the persisted baseline matches the published data. + $mergedFamilyKeys = @{} + foreach ($source in @($first.FamilyKeys, $second.FamilyKeys)) + { + foreach ($family in $source.Keys) + { + if (-not $mergedFamilyKeys.ContainsKey($family)) { $mergedFamilyKeys[$family] = @{} } + foreach ($key in $source[$family].Keys) { $mergedFamilyKeys[$family][$key] = $true } + } + } + + $mergedFamilyCounts = @{} + foreach ($family in $mergedFamilyKeys.Keys) { $mergedFamilyCounts[$family] = $mergedFamilyKeys[$family].Count } + + return @{ + Keys = $union + FamilyKeys = $mergedFamilyKeys + FamilyCounts = $mergedFamilyCounts + # Reported as the totals actually fetched across both passes. + Items = $first.Items + $second.Items + Pages = $first.Pages + $second.Pages + VerifyDrift = $drift + OnlyFirst = $onlyFirst + OnlySecond = $onlySecond + } +} + # Per-family baseline (sidecar alongside the CSV). The published CSV carries only # MeterId + the two flags -- no serviceFamily -- so per-family counts from the last # good run are persisted here to let the completeness guard catch a single family @@ -378,35 +536,39 @@ if (Test-Path $FamilyCountPath) } # ----------------------------------------------------------------------- -# Step 2: Reservation-eligible meters (single traversal) +# Step 2: Reservation-eligible meters (verified traversal) +# Reservation is fetched and verified FIRST because it is by far the cheaper of the +# two (~3 minutes per pass vs ~17). If the API is paging inconsistently, this aborts +# after ~7 minutes instead of after the ~40 the Consumption passes would add. # ----------------------------------------------------------------------- Write-Host "Fetching Reservation prices..." -$riResult = Get-EligibleMeter -Filter "priceType eq 'Reservation'" -MeterRegion 'primary' -ActivityName 'Fetching Reservation prices' -CollectKey { +$riResult = Get-VerifiedEligibleMeter -Filter "priceType eq 'Reservation'" -MeterRegion 'primary' -ActivityName 'Fetching Reservation prices' -MaxVerifyDrift $MaxVerifyDrift -SkipVerify:$SkipVerify -CollectKey { param($item) $item.meterId } $riMeters = $riResult.Keys -Write-Host " Walked $($riResult.Items) items over $($riResult.Pages) pages across $($riResult.FamilyCounts.Count) service families" Write-Host " RI-eligible meters: $($riMeters.Count)" # ----------------------------------------------------------------------- -# Step 3: Savings Plan-eligible meters (single traversal) +# Step 3: Savings Plan-eligible meters (verified traversal) # The savingsPlan array is embedded in Consumption items, so we page through # primary Consumption meters and record those with a non-empty savingsPlan. # ----------------------------------------------------------------------- Write-Host "Fetching Consumption prices (checking for Savings Plan eligibility)..." -$spResult = Get-EligibleMeter -Filter "priceType eq 'Consumption'" -MeterRegion 'primary' -ActivityName 'Fetching Consumption prices' -CollectKey { +$spResult = Get-VerifiedEligibleMeter -Filter "priceType eq 'Consumption'" -MeterRegion 'primary' -ActivityName 'Fetching Consumption prices' -MaxVerifyDrift $MaxVerifyDrift -SkipVerify:$SkipVerify -CollectKey { param($item) if ($item.savingsPlan -and $item.savingsPlan.Count -gt 0) { $item.meterId } else { $null } } $spMeters = $spResult.Keys -Write-Host " Walked $($spResult.Items) items over $($spResult.Pages) pages across $($spResult.FamilyCounts.Count) service families" Write-Host " SP-eligible meters: $($spMeters.Count)" # ----------------------------------------------------------------------- -# Step 3b: Completeness guard. Abort before writing if the meter total dropped -# materially below the published total -- an incomplete run must never overwrite -# good open-data (a missed SP flag would flip Eligible -> Not). +# Step 3b: Historical guards. The verification traversals above are the completeness +# signal; these catch what set comparison structurally cannot -- a DETERMINISTIC +# omission that both passes share, which they see as perfect agreement. Abort before +# writing if the meter total dropped materially below the published total, since an +# incomplete run must never overwrite good open-data (a missed SP flag would flip +# Eligible -> Not). # ----------------------------------------------------------------------- $seenSet = @{} foreach ($key in $riMeters.Keys) { $seenSet[$key] = $true } @@ -441,12 +603,12 @@ if ($familyShortfall.Count -gt 0) { throw "Aborting before write: service family/families fell more than $([Math]::Round($MaxShrinkFraction * 100))% below baseline: $($familyShortfall -join '; '). A family likely under-fetched; refusing to overwrite published data. Raise -MaxShrinkFraction for a deliberate large change." } -Write-Host "Completeness check passed: $seenTotal meters seen this run (cached $cachedTotal)." +Write-Host "Historical guards passed: $seenTotal meters seen this run (cached $cachedTotal)." # ----------------------------------------------------------------------- -# Step 4: Build the output from this run's sets (write fresh) and summarize the -# change versus the previously published CSV. Meters present only in the old CSV are -# dropped (retired); the guard above already ensured the run is whole. +# Step 4: Build the output from this run's verified sets (write fresh) and summarize +# the change versus the previously published CSV. Meters present only in the old CSV +# are dropped (retired); the verification traversal established that the run is whole. # ----------------------------------------------------------------------- Write-Host "`nBuilding output..." $sortedIds = [string[]]($seenSet.Keys | Sort-Object) @@ -482,6 +644,16 @@ Write-Host " Changed: $changed" Write-Host " Unchanged: $unchanged" Write-Host " Removed: $removed (retired meters no longer returned by the API)" +# Surfaced on every run, not just failures: a drift that climbs week over week while +# staying under the threshold is the early warning that the June-2026 paging fault is +# returning, and it is only visible if the passing value is logged too. +if (-not $SkipVerify) +{ + Write-Host "`nVerification drift (pass 1 vs pass 2):" + Write-Host " Reservation: $([Math]::Round($riResult.VerifyDrift * 100, 4))% ($($riResult.OnlyFirst) only in pass 1, $($riResult.OnlySecond) only in pass 2)" + Write-Host " Consumption: $([Math]::Round($spResult.VerifyDrift * 100, 4))% ($($spResult.OnlyFirst) only in pass 1, $($spResult.OnlySecond) only in pass 2)" +} + Write-Verbose "Writing CSV to $OutputPath..." $writeStart = [DateTime]::UtcNow $rows | Export-Csv -Path $OutputPath -UseQuotes Always -NoTypeInformation -Encoding utf8 From 9e38f182ab812f9e50714fe88cad1f4e3c131ef0 Mon Sep 17 00:00:00 2001 From: Roland Krummenacher Date: Thu, 13 Aug 2026 16:23:24 +0200 Subject: [PATCH 08/13] chore: Normalize the committed baseline to sorted key order 8bb06cd7 made the script write the baseline sidecar with sorted keys, but the committed file was still in the arbitrary hashtable order the run that produced it happened to emit. The first run after merge would therefore have rewritten it in sorted order, and since the workflow treats ANY diff in the sidecar as "data changed", that reordering alone would have pushed a branch and asked for a PR -- exactly the empty update PR 8bb06cd7 set out to prevent. Ordering only. Verified that both sections keep the same family count and that every per-family value is byte-identical before and after. Co-Authored-By: Claude Opus 5 (1M context) --- ...tmentDiscountEligibility.familycounts.json | 56 +++++++++---------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/src/open-data/CommitmentDiscountEligibility.familycounts.json b/src/open-data/CommitmentDiscountEligibility.familycounts.json index 6e159becd..d4c8178d1 100644 --- a/src/open-data/CommitmentDiscountEligibility.familycounts.json +++ b/src/open-data/CommitmentDiscountEligibility.familycounts.json @@ -1,44 +1,44 @@ { "Reservation": { - "Management and Governance": 1, - "Security": 1, "AI + Machine Learning": 82, - "Compute": 61368, - "Developer Tools": 2, "Analytics": 131, - "Storage": 1673, + "Compute": 61368, + "Data": 45, "Databases": 5873, - "Data": 45 + "Developer Tools": 2, + "Management and Governance": 1, + "Security": 1, + "Storage": 1673 }, "Consumption": { + "AI + Machine Learning": 0, + "Analytics": 0, + "Azure Arc": 0, + "Azure Communication Services": 0, "Azure Security": 0, - "Other": 99, - "Windows Virtual Desktop": 0, - "Blockchain": 0, - "Storage": 0, - "Power Platform": 0, - "Internet of Things": 0, "Azure Stack": 0, - "Integration": 0, + "Blockchain": 0, + "Compute": 63558, + "Containers": 168, + "Data": 0, "Databases": 18303, - "Management and Governance": 0, - "Azure Communication Services": 0, - "Azure Arc": 0, - "AI + Machine Learning": 0, "Developer Tools": 0, - "Security": 0, + "Gaming": 0, + "Integration": 0, + "Internet of Things": 0, + "Management and Governance": 0, "Microsoft 365 Copilot": 0, - "Compute": 63558, - "Telecommunications": 0, - "Windows 365": 0, - "Networking": 0, - "Containers": 168, - "Web": 0, - "Mixed Reality": 0, "Microsoft Syntex": 0, + "Mixed Reality": 0, + "Networking": 0, + "Other": 99, + "Power Platform": 0, "Quantum Computing": 0, - "Gaming": 0, - "Data": 0, - "Analytics": 0 + "Security": 0, + "Storage": 0, + "Telecommunications": 0, + "Web": 0, + "Windows 365": 0, + "Windows Virtual Desktop": 0 } } \ No newline at end of file From f0eb02d5e8300f02a3cf1d169152d266580a5be0 Mon Sep 17 00:00:00 2001 From: Roland Krummenacher Date: Thu, 13 Aug 2026 17:10:45 +0200 Subject: [PATCH 09/13] docs: Replace the runtime estimates with the measured ones The end-to-end validation (run 31710007476) came in at 44 minutes, not the ~35 I estimated when writing these comments, and the churn window the -MaxVerifyDrift rationale refers to is ~9 minutes for Reservation and ~32 for Consumption rather than a flat ~20. Comments that carry load-bearing numbers should carry the real ones. Also records the measured drift (0% on both price types) next to the default, so the tolerance is justified by an observation rather than by an assertion. No behaviour change; the 90-minute timeout still leaves 46 minutes of headroom. Co-Authored-By: Claude Opus 5 (1M context) --- .../opendata-commitment-eligibility.yml | 8 ++++---- .../Update-CommitmentDiscountEligibility.ps1 | 17 +++++++++-------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/.github/workflows/opendata-commitment-eligibility.yml b/.github/workflows/opendata-commitment-eligibility.yml index 18b3ff2b6..cd0fcd3af 100644 --- a/.github/workflows/opendata-commitment-eligibility.yml +++ b/.github/workflows/opendata-commitment-eligibility.yml @@ -8,7 +8,7 @@ on: ref: # The scheduled run always uses dev. This input exists so a change to the fetch # script can be exercised end-to-end from its PR branch BEFORE merging, which - # matters for a job that only runs weekly and takes ~35 minutes to fail. Only + # matters for a job that only runs weekly and takes ~44 minutes to fail. Only # users with write access can dispatch a workflow, so this does not widen who # can run code in the repo. # @@ -35,11 +35,11 @@ permissions: jobs: update: runs-on: ubuntu-latest - # Two full traversals per price type (fetch + verification) run ~35-40 minutes; the - # headroom above that absorbs Retry-After backoff on a throttled run. + # Two full traversals per price type (fetch + verification) measured 44 minutes on + # run 31710007476. The headroom absorbs Retry-After backoff on a throttled run. timeout-minutes: 90 steps: - # Fail before the ~35-minute fetch rather than after it, so an invalid input + # Fail before the ~44-minute fetch rather than after it, so an invalid input # combination costs seconds instead of most of an hour. - name: Validate dispatch inputs env: diff --git a/src/scripts/Update-CommitmentDiscountEligibility.ps1 b/src/scripts/Update-CommitmentDiscountEligibility.ps1 index a51a77d60..aaeb14b8d 100644 --- a/src/scripts/Update-CommitmentDiscountEligibility.ps1 +++ b/src/scripts/Update-CommitmentDiscountEligibility.ps1 @@ -36,9 +36,9 @@ reported on every run and aborts it above -MaxVerifyDrift. A small non-zero difference is expected rather than alarming: the catalogue is - live, so a meter may legitimately appear or disappear during the ~20 minutes - between the start of pass 1 and the end of pass 2. -MaxVerifyDrift separates that - churn from a genuine paging fault. + live, so a meter may legitimately appear or disappear during the window the two + passes span (~9 minutes for Reservation, ~32 for Consumption). -MaxVerifyDrift + separates that churn from a genuine paging fault. 2. Historical guards (the regression net). The run is compared against the previously published data in aggregate AND per serviceFamily; either falling more than @@ -102,9 +102,10 @@ param( # Verification guard: abort if the two traversals of a price type disagree on more # than this fraction of their combined meterId set. Sized to pass genuine catalogue - # churn over the ~20 minutes the two passes span (empirically a handful of meters - # out of ~92k, i.e. well under 0.01%) while failing the June-2026 style fault, which - # dropped a scattered PERCENTAGE of rows. Set to 0 to require exact agreement. + # churn over the window the two passes span while failing the June-2026 style fault, + # which dropped a scattered PERCENTAGE of rows. Run 31710007476 measured 0% drift on + # both price types (0 meters of 66,199 and 0 of 82,677), so the default is roughly + # two orders of magnitude above observed churn. Set to 0 to require exact agreement. [ValidateRange(0.0, 1.0)] [double]$MaxVerifyDrift = 0.001, @@ -538,8 +539,8 @@ if (Test-Path $FamilyCountPath) # ----------------------------------------------------------------------- # Step 2: Reservation-eligible meters (verified traversal) # Reservation is fetched and verified FIRST because it is by far the cheaper of the -# two (~3 minutes per pass vs ~17). If the API is paging inconsistently, this aborts -# after ~7 minutes instead of after the ~40 the Consumption passes would add. +# two (~3-6 minutes per pass vs ~15-17). If the API is paging inconsistently, this +# aborts after ~12 minutes rather than the full ~44 the Consumption passes would add. # ----------------------------------------------------------------------- Write-Host "Fetching Reservation prices..." $riResult = Get-VerifiedEligibleMeter -Filter "priceType eq 'Reservation'" -MeterRegion 'primary' -ActivityName 'Fetching Reservation prices' -MaxVerifyDrift $MaxVerifyDrift -SkipVerify:$SkipVerify -CollectKey { From ccffc75a4012c0cb59034897f8b22a738932d4b1 Mon Sep 17 00:00:00 2001 From: Roland Krummenacher Date: Mon, 17 Aug 2026 16:07:26 +0200 Subject: [PATCH 10/13] fix(open-data): clamp the exponential retry backoff to MaxSeconds MaxSeconds bounded only the Retry-After path, so the last of the five retries waited 2^5 * 10 = 320s -- past the same 300s ceiling an outsized server value is held to. Also documents why the -SkipVerify return path merges hashtables with `+`. Co-Authored-By: Claude Opus 5 (1M context) --- .../Update-CommitmentDiscountEligibility.Tests.ps1 | 10 ++++++++++ src/scripts/Update-CommitmentDiscountEligibility.ps1 | 10 +++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 b/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 index 9df60a696..5d0c2d8ac 100644 --- a/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 +++ b/src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1 @@ -135,6 +135,16 @@ Describe 'Update-CommitmentDiscountEligibility helpers' { $past = [DateTimeOffset]::UtcNow.AddSeconds(-60).ToString('r') Get-RetryDelay -Response (Get-TestResponse -RetryAfter $past) -Attempt 1 | Should -Be 20 } + + It 'clamps the exponential backoff to MaxSeconds as well' { + # MaxSeconds must bound every wait, not only the header-driven one. Attempt 5 + # is reachable -- it is the last of the 5 retries the fetch loop allows -- and + # 2^5 * 10 = 320s would otherwise overrun the ceiling the header is held to. + Get-RetryDelay -Response (Get-TestResponse) -Attempt 5 -MaxSeconds 300 | Should -Be 300 + Get-RetryDelay -Response $null -Attempt 5 -MaxSeconds 300 | Should -Be 300 + # Below the ceiling the backoff is unchanged. + Get-RetryDelay -Response (Get-TestResponse) -Attempt 4 -MaxSeconds 300 | Should -Be 160 + } } Context 'Get-EligibleMeter' { diff --git a/src/scripts/Update-CommitmentDiscountEligibility.ps1 b/src/scripts/Update-CommitmentDiscountEligibility.ps1 index aaeb14b8d..eb92ebd01 100644 --- a/src/scripts/Update-CommitmentDiscountEligibility.ps1 +++ b/src/scripts/Update-CommitmentDiscountEligibility.ps1 @@ -222,6 +222,10 @@ function Get-RetryDelay The value is clamped to -MaxSeconds so an outsized server value cannot stall the run past the job timeout, and a non-positive value (a stale HTTP-date already in the past) falls back to the exponential backoff rather than retrying instantly. + + -MaxSeconds bounds EVERY wait, not just the header-driven one: the exponential + fallback reaches 2^5 * 10 = 320s on the last of the 5 retries, which would + otherwise exceed the same ceiling the server value is held to. #> param( $Response, @@ -248,7 +252,7 @@ function Get-RetryDelay { return [Math]::Min($retryAfter, $MaxSeconds) } - return [int][Math]::Pow(2, $Attempt) * 10 + return [Math]::Min([int][Math]::Pow(2, $Attempt) * 10, $MaxSeconds) } function Get-EligibleMeter @@ -380,6 +384,10 @@ function Get-VerifiedEligibleMeter if ($SkipVerify) { Write-Warning " Verification traversal SKIPPED (-SkipVerify). Output is not publishable." + # `+` on two hashtables returns a new one carrying the keys of both, so this is + # pass 1's result shape plus the verification fields the caller expects. It throws + # on a duplicate key rather than silently overwriting, which is the behavior we + # want here: Get-EligibleMeter must never start returning these three itself. return $first + @{ VerifyDrift = [double]0; OnlyFirst = 0; OnlySecond = 0 } } From f61c90121caf59d781334788676442634f0a089d Mon Sep 17 00:00:00 2001 From: Roland Krummenacher Date: Mon, 17 Aug 2026 16:07:27 +0200 Subject: [PATCH 11/13] docs: add a changelog entry for the eligibility refresh fix The weekly refresh has not published since the dataset shipped in v14, so the stale data is user-facing and belongs in the changelog. Co-Authored-By: Claude Opus 5 (1M context) --- docs-mslearn/toolkit/changelog.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs-mslearn/toolkit/changelog.md b/docs-mslearn/toolkit/changelog.md index 5a9d011a5..41ed7ead3 100644 --- a/docs-mslearn/toolkit/changelog.md +++ b/docs-mslearn/toolkit/changelog.md @@ -3,7 +3,7 @@ title: FinOps toolkit changelog description: Review the latest features and enhancements in the FinOps toolkit, including updates to FinOps hubs, Power BI reports, and more. author: MSBrett ms.author: brettwil -ms.date: 08/12/2026 +ms.date: 08/17/2026 ms.topic: reference ms.service: finops ms.subservice: finops-toolkit @@ -70,6 +70,7 @@ The following section lists features and enhancements that are currently in deve - **Fixed** - Fixed the commitment discount eligibility dataset refresh so it is reproducible and complete; retired meters now age out and previously missed meters are included ([#2164](https://github.com/microsoft/finops-toolkit/pull/2164)). + - Fixed the weekly commitment discount eligibility refresh timing out before it could publish, which left the dataset unchanged since it first shipped in v14. The refresh now walks each price type directly instead of sharding by service family, and verifies completeness by comparing two independent traversals before writing ([#2251](https://github.com/microsoft/finops-toolkit/pull/2251)). --> From 7b96cb9c77016399532826420b3cb56c07fc6e8b Mon Sep 17 00:00:00 2001 From: Roland Krummenacher Date: Mon, 17 Aug 2026 16:40:27 +0200 Subject: [PATCH 12/13] chore: re-trigger CI The PowerShell Tests workflow did not fire on f61c9012 even though the PR diff matches its 'src/powershell/**' path filter. Empty commit to force a synchronize event. Co-Authored-By: Claude Opus 5 (1M context) From 663389322d76539587b8e26a65691d69c6dd22f2 Mon Sep 17 00:00:00 2001 From: Roland Krummenacher Date: Mon, 17 Aug 2026 17:19:05 +0200 Subject: [PATCH 13/13] ci: install Pester 6 in Open Data CI #2254 made Test-PowerShell.ps1 fail below Pester 6 and pinned that floor in dev.yml, but opendata-ci.yml calls the same script through Build-OpenData.ps1 -Test and installs no modules, so it picked up the ubuntu runner's Pester 5.9.0 and failed during discovery. No PR had triggered Open Data CI since #2254 merged, so this went unnoticed; it would have broken every future PR touching src/open-data. Uses the same Install-Module invocation as Init-Repo.ps1 rather than psmodulecache, which this repo only uses on Windows runners. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/opendata-ci.yml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/workflows/opendata-ci.yml b/.github/workflows/opendata-ci.yml index 76fb15004..fdf3060a6 100644 --- a/.github/workflows/opendata-ci.yml +++ b/.github/workflows/opendata-ci.yml @@ -45,6 +45,15 @@ jobs: git commit -a -m "${{ env.CI_COMMIT_MESSAGE }}" git push } + # Build-OpenData.ps1 -Test calls Test-PowerShell.ps1, which requires Pester 6 (#2254): + # the suite uses -AllowNullOrEmptyForEach, which Pester 5 rejects. The ubuntu runner + # image ships Pester 5.9.0, so the minimum has to be installed explicitly. This is the + # same command Init-Repo.ps1 runs and the one Test-PowerShell.ps1 names in its error. + # dev.yml pins the same floor through psmodulecache, which this repo only uses on + # Windows runners. + - name: Install Pester + shell: pwsh + run: Install-Module -Name Pester -MinimumVersion 6.0.0 -Scope CurrentUser -Repository PSGallery -Force - name: Test Open Data id: test shell: pwsh