Replace serviceFamily sharding with a single traversal per price type

[Roland Krummenacher (RolandKrummenacher)]

🐛 Problem

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. That run was the first to get past the $orderby HTTP 400 fixed in #2164 — the fetch itself works now; it is simply too slow to finish.

🔧 Solution

The serviceFamily sharding and multi-pass union existed to work around a period when the Retail Prices API returned an unstable page order. That workaround is no longer earning its cost:

• The instability was last observed in early June 2026. MS support case 2606030050003725 closed without a root cause, and seven full traversals on 2026-06-29 were byte-identical.
• Run 31612943595's own telemetry shows every repeat pass across all 22 completed shards adding exactly zero meters, while costing 2-3x the traversal volume. No shard needed a 4th pass and none reported non-convergence, meaning pass 1 was already complete everywhere.
• Building the shard list required a full discovery traversal per price type, which alone consumed 25 of the job's 60 minutes and produced only 38 family-name strings.

Each price type is now walked by following the documented NextPageLink verbatim, with per-serviceFamily counts tallied in the same pass — every item already carries serviceFamily, so the per-family bookkeeping costs nothing extra.

Completeness is verified per run, not inferred from history

The first revision of this PR leaned on the two historical guards alone, and MSBrett was right that they cannot carry that weight: comparing one traversal against last week's counts with a 15% tolerance passes a uniform under-fetch, passes an offsetting drop-and-add, and cannot see a brand-new meter that was missed.

So each price type is traversed twice and the two meterId sets are compared directly, which needs no baseline and catches all three cases on the run that suffers them:

• The published set is the union, not the intersection. If only one pass saw a meter, the meter is real. Publishing the intersection would let a faulty pass silently delete rows — the failure being guarded against.
• Per-family sets are unioned, not their counts. Taking the max of two counts undercounts a family whose passes each missed a different meter (pass 1 sees {a,b}, pass 2 sees {a,c}: union 3, max 2).
• Drift is a tolerance, not exact equality — -MaxVerifyDrift defaults to 0.1%. The catalogue is live and the two passes span ~9 minutes for Reservation and ~32 for Consumption, so a meter can legitimately appear or retire between them. -MaxVerifyDrift 0 gives strict equality.

The symmetric difference is logged on every run, not just failures: 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 printed too.

The historical guards are retained as a backstop rather than the primary 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.

Removes Get-ServiceFamily, Assert-DiscoveryConverged, Add-BaselineShard, and Invoke-ShardedUnion.

Two latent bugs fixed along the way

• 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 (it does not throw) and every 429 fell through to the exponential backoff. Now reads the typed .RetryAfter property, handling both the delta-seconds and HTTP-date forms, and -MaxSeconds clamps both paths so no single wait can exceed the ceiling.
• $cachedShardCounts['Reservation'] threw Cannot index into a null array when no baseline sidecar exists. Fix commitment discount eligibility fetch (Retail Prices API pagination) #2164 committed the sidecar alongside the CSV, so this is not the path a normal CI run takes — it is reached by a fresh -OutputPath or a removed sidecar, which is how I hit it locally. The guard is still correct and worth keeping, but it was not blocking the scheduled run. Pre-existing on dev at four call sites.

Determinism and workflow safety (added during review)

• shardcounts.json → familycounts.json, and Get-ShardShortfall → Get-FamilyShortfall. The workflow, the open-data CI path filter, and the Package-Toolkit.ps1 exclusion were all updated to match, so nothing keys off the old name any more.
• The sidecar is serialized through ConvertTo-SortedMap. Hashtable key order is not guaranteed, and the validation run wrote the families in neither alphabetical nor insertion order. Since the workflow treats any diff in the sidecar as "data changed", that would have manufactured empty update PRs.
• workflow_dispatch gained ref and dry_run inputs, so a change to the fetch script can be exercised end-to-end from its PR branch before merging — previously the checkout was pinned to ref: dev, meaning a weekly job that takes 44 minutes to fail could only be tested after merge. dry_run defaults to true, and a guard step that runs before checkout rejects a non-dev ref unless dry_run is set, so a data branch can never be built from a feature ref and carry its code commits into the data PR.
• dry_run is compared as a string in bash rather than in a GitHub if: expression: a type: boolean input arrives as the literal string "false", which is truthy in expression syntax, so if: ${{ !inputs.dry_run }} would have inverted the check.

🧪 Validation

Two independent full runs against the live API produced identical results:

	Items	Pages	Families	Eligible meters
Reservation	146,671	147	9	66,199
Consumption	689,723	690	29	82,677

Both match sharded run 31612943595 exactly, per service family as well as in aggregate (Compute 58,723 / Databases 5,536 / Storage 1,673 / Analytics 132 / AI + ML 82 / Data 49 / Developer Tools 2 / Security 1 / Management and Governance 1). The simplification demonstrably loses nothing, and the matching runs are fresh evidence the API is stable.

Resulting CSV verified: 92,550 rows, sorted, lowercased, no duplicates. The per-family guard ran against #2164's committed baseline rather than skipping it, and passed on real data — Compute −4.3% and Databases −5.7%, both inside the 15% tolerance, with Storage and AI + ML unchanged.

End-to-end in CI, from this branch:

• Run 31710007476 — the two-pass version, dry_run=true: succeeded in 44m00s, nothing pushed, no opendata/* branch left behind. The sharded run it replaces was killed at 60 minutes having completed 13 of 29 shards.
• Run 31710035770 — the guard, dispatched with this branch and dry_run unticked: failed in 25 seconds before checkout, fetch, and push, all three reported skipped.

Also exercised all ten dispatch input combinations against the guard, including dev-experiment and DEV (both correctly rejected — no prefix or case-insensitive match) and the literal string "false".

• Unit tests: 33 pass, including regression tests pinning the Retry-After indexer bug (asserts 30, not the fallback 20), the MaxSeconds clamp on the exponential path, sidecar key-order stability, and each of the three failure modes the verification pass exists to catch.
• PSScriptAnalyzer: clean. Lint.Tests.ps1 and MsLearnDocs.Tests.ps1: 2982 pass.

📝 Notes for reviewers

• Runtime is ~44 minutes, not the ~12-13 I projected from the old run's discovery timings — the verification pass roughly doubles it, which is the deliberate trade for a completeness claim that holds. timeout-minutes is therefore raised from 60 to 90; the headroom absorbs Retry-After backoff on a throttled run.
• No regenerated open data is included here. Validation wrote to a scratch copy; the data refresh belongs in the workflow's own PR.
• The script now declares #Requires -Version 7.0 — it depends on ConvertFrom-Json -AsHashtable, Export-Csv -UseQuotes, and reading the error response as an HttpResponseMessage. CI is unaffected; the workflow already runs shell: pwsh.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR updates the commitment discount eligibility refresh script to complete within the workflow’s 60-minute timeout by removing the prior serviceFamily sharding/multi-pass union approach and replacing it with a single NextPageLink traversal per price type, while keeping the existing aggregate + per-family completeness guards as the data integrity mechanism.

Changes:

• Replace multi-pass sharded fetch with a single traversal per priceType, collecting eligible meter IDs and per-serviceFamily counts in the same pass.
• Fix retry behavior to honor Retry-After via typed headers, including parsing/clamping logic.
• Update unit tests to cover the new helper functions (Get-RetryDelay, Get-EligibleMeter) and the revised fetch/count behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/scripts/Update-CommitmentDiscountEligibility.ps1	Removes sharding/repeat passes; adds single-pass traversal with per-family counting, typed Retry-After handling, and guarded baseline lookups.
src/powershell/Tests/Unit/Update-CommitmentDiscountEligibility.Tests.ps1	Updates AST-extracted helper coverage and adds regression tests for Retry-After and the new traversal/count helper.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[Roland Krummenacher (RolandKrummenacher)]

Added 528a5f1, which touches the workflow rather than the script — flagging it explicitly since it widens this PR beyond the fetch logic.

Motivation: the 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. For a job that runs weekly, takes ~15 minutes to fail, and has not completed a successful run in months, that is a poor feedback loop. Two workflow_dispatch inputs address it:

• ref (default dev) — the branch to check out and run from. 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 change detection but skips the branch push, so testing leaves no stray opendata/* branches. It writes a summary of what would have changed.

One subtlety worth noting for review: dry_run is compared as a string in bash rather than tested in a GitHub if: expression. A type: boolean input arrives as the literal string "false", which is truthy in expression syntax, so if: ${{ !inputs.dry_run }} would have inverted the check and skipped the push whenever the box was left unticked.

I verified this by extracting the run block and exercising all four paths against a scratch repository — dry run with changes (correctly 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 correctly take the push path.

Happy to split this into its own PR if you would rather keep this one to the script.

[Roland Krummenacher (RolandKrummenacher)]

Correction to the PR description. One claim in it is wrong, and I want it on the record rather than buried in a commit.

The description states that because the job has never completed a successful run, the baseline sidecar has never existed, and that the null-index bug "would have failed the run even with the timeout raised." That is incorrect. #2164 committed src/open-data/CommitmentDiscountEligibility.shardcounts.json alongside the CSV on 2026-08-12, so it is present on dev and on this branch, and $cachedShardCounts is not null in a normal CI run.

I hit the null path locally only because my validation harness copied just the CSV to a scratch -OutputPath and not the sidecar — an artifact of my own test setup, which I wrote up as a production finding. The guard is still correct and still worth keeping (a fresh -OutputPath, or a removed sidecar, reaches it), but it was not blocking the scheduled run. Comment corrected in bcaff15.

The good news is that this makes the validation stronger than claimed. Because the sidecar was present, today's dispatch actually exercised the per-family guard against #2164's committed baseline rather than skipping it. It passed, and the comparison is informative:

Family	Baseline (#2164)	This run	Delta
Compute	61,368	58,723	−4.3%
Databases	5,873	5,536	−5.7%
Storage	1,673	1,673	0%
AI + Machine Learning	82	82	0%

Both shrinking families are inside the 15% tolerance, so the guard passed on real data rather than trivially.

End-to-end dispatch result (run 31674572978), run from this branch with dry_run=true:

• Succeeded in 20m39s, against a 60-minute timeout. The sharded run it replaces was killed at 60 minutes having completed 13 of 29 shards.
• 146,671 Reservation items / 147 pages → 66,199 RI-eligible; 689,723 Consumption items / 690 pages → 82,677 SP-eligible — identical to both local validation runs.
• 92,550 meters written; 576 added, 4,359 changed, 308 removed.
• Zero rate-limiting events on the runner.
• Dry run took the correct path (Dry run: changes detected, not pushing.) and left no opendata/* branch behind.

Note that 20m39s exceeds the ~12-13 minute figure I projected from the old run's discovery timings, so that projection was optimistic. It is still comfortably inside the timeout, but worth knowing the real number is closer to a third of the budget than a fifth.

[Brett Wilson (MSBrett)]

Review findings from the single-traversal change.

[Michael Flanakin (flanakin)]

🤖 [AI][Claude] PR Review

Summary: This replaces the old sharded discovery approach for the weekly commitment-discount-eligibility fetch with a two-pass verified traversal, and fixes a real Retry-After parsing bug along the way. The core logic is correct, well tested (15/15 unit tests pass locally, no lint regressions), and the workflow_dispatch safety guard behaves correctly across all input combinations I simulated. No blockers — just a few things worth tightening before merge.

⚠️ Should fix (3)

1. PR description is out of date. It says renaming shardcounts.json is "a separate change" and projects "~12-13 minutes" runtime, but the final commits did rename the file to familycounts.json and the actual measured runtime is ~20 minutes (per your own follow-up comment on this PR). Since GitHub carries this description into the merge commit, worth a quick edit so it matches what actually shipped. The title ("a single traversal") is also a bit of an undersell — the final approach does two passes per price type with drift verification, which is a stronger reliability story than "single traversal" suggests.
2. No changelog entry. The commitment-discount-eligibility feature shipped in v14, and this fixes a bug where the weekly refresh job has never completed successfully since — so open-data consumers have been getting stale data. That's user-facing enough to warrant a changelog entry (e.g., a "Fixed" line in docs-mslearn/toolkit/changelog.md).
3. See inline comment on the timeout comment mismatch (44 min vs. the 20-minute run you reported).

💡 Suggestions (2)

See inline comments — a small clarifying comment for a PowerShell idiom, and a concrete gap in the retry backoff clamp.

[github-actions]

⚠️ Open Data CI task failed: https://github.com/microsoft/finops-toolkit/actions/runs/32040620049