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 once, following the documented NextPageLink verbatim, with 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. Data integrity now rests on them rather than on re-fetching: if the instability returns, the run aborts loudly instead of publishing partial data. That is the intended trade and the signal to reopen the support case.

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, 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. Because 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. Pre-existing on dev at four call sites.

🧪 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 two matching runs are fresh evidence the API is stable.

Resulting CSV verified: 92,550 rows, sorted, lowercased, no duplicates; guard passed against the cached 92,282.

• Unit tests: 17 pass, including a regression test pinning the Retry-After indexer bug (asserts 30, not the fallback 20).
• PSScriptAnalyzer: clean.
• Full unit suite: 774 pass. The 5 failures in Start-FinOpsCostExport/DocsLinks reproduce identically on a clean dev worktree and are unrelated.

📝 Notes for reviewers

• The shardcounts.json filename and Get-ShardShortfall name are deliberately retained even though sharding is gone — the workflow, the open-data CI path filter, and the packaging exclusion in Package-Toolkit.ps1 all key off that name. Renaming is a separate change.
• No regenerated open data is included here. Validation wrote to a scratch copy; the data refresh belongs in the workflow's own PR.
• timeout-minutes is left at 60. Projected runtime is ~12-13 minutes.
• The workflow's checkout step is pinned to ref: dev, so a workflow_dispatch test run only exercises this change after merge.

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