From 02a230b0886a3343cb75efdf090484b325a5a1d0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 8 Sep 2026 08:37:21 +0200 Subject: [PATCH] fix(azure): error when the Retail Prices page cap is hit with pages remaining pricing.FetchAll stopped after maxPages and returned the pages read so far with a nil error, so every Azure GetOfferingDetails consumer read a truncated price list as a complete one and reported a meter as absent when it was on a later page. The self-referential-link guard in the same loop already errors; the cap now does too. The cap stays at 50. Measured on 2026-09-08 the API pages at about 1,000 items, so 50 pages is roughly 50,000; the largest filter any client issues fits in one page and the whole VM catalogue for one region is 16. The DefaultMaxPages comment claimed 100 items per page; corrected. Closes #1963 Co-Authored-By: claude-flow Claude-Session: https://claude.ai/code/session_01Fu9uWjxtDFx5HDKeMRt1jC --- .../azure/internal/pricing/retail_prices.go | 19 +++++-- .../internal/pricing/retail_prices_test.go | 54 +++++++++++++------ providers/azure/services/cache/client.go | 2 +- providers/azure/services/compute/client.go | 4 +- providers/azure/services/cosmosdb/client.go | 2 +- providers/azure/services/database/client.go | 2 +- 6 files changed, 57 insertions(+), 26 deletions(-) diff --git a/providers/azure/internal/pricing/retail_prices.go b/providers/azure/internal/pricing/retail_prices.go index e71acdb59..1f98d77a9 100644 --- a/providers/azure/internal/pricing/retail_prices.go +++ b/providers/azure/internal/pricing/retail_prices.go @@ -42,10 +42,15 @@ type Page[T any] struct { // service clients use this default. const DefaultPageTimeout = 10 * time.Second -// DefaultMaxPages caps the NextPageLink loop. The Azure Retail Prices API -// paginates at 100 items per page, so 50 pages is 5000 items — more than -// any realistic SKU/region/term query. The cap is purely a defence against -// a server bug returning a NextPageLink that never empties. +// DefaultMaxPages caps the NextPageLink loop. The Retail Prices API +// (api-version 2023-01-01-preview) pages at about 1,000 items, so 50 +// pages is roughly 50,000 items. The largest filter any client issues +// (a region-wide service catalogue, cosmosdb/search) measured 1 page on +// 2026-09-08; the whole Virtual Machines catalogue for one region, which +// no client requests, measured 16. The cap is a defence against a +// server bug returning a NextPageLink that never empties. Reaching it +// with a NextPageLink still pending is an error, never a truncated +// result (#1963). const DefaultMaxPages = 50 // FetchAll walks the Retail Prices API starting at initialURL, appending @@ -54,7 +59,8 @@ const DefaultMaxPages = 50 // - a per-page timeout (pageTimeout) that's independent of the caller's // ctx, so one slow page can't consume the caller's whole budget; // - a max-pages cap (maxPages) against a server bug returning an -// infinite NextPageLink chain; +// infinite NextPageLink chain; hitting it with pages remaining is +// an error, not a truncated result; // - a seen-URL guard against a self-referential NextPageLink. // // ctx's cancellation still propagates via context.WithTimeout(ctx, ...), @@ -84,6 +90,9 @@ func FetchAll[T any](ctx context.Context, httpClient HTTPClient, initialURL stri nextURL = page.NextPageLink } + if nextURL != "" { + return nil, fmt.Errorf("pricing API result exceeds the %d-page cap (%d items read, NextPageLink still set)", maxPages, len(all)) + } return all, nil } diff --git a/providers/azure/internal/pricing/retail_prices_test.go b/providers/azure/internal/pricing/retail_prices_test.go index 6c1ec092f..553194ed0 100644 --- a/providers/azure/internal/pricing/retail_prices_test.go +++ b/providers/azure/internal/pricing/retail_prices_test.go @@ -60,6 +60,22 @@ func okJSONResponse(body string) *http.Response { } } +// scriptPageChain scripts n pages at https://prices.example/page, +// each holding one item named after its letter and linking to the next; +// the last page has an empty NextPageLink. +func scriptPageChain(client *fakeHTTPClient, n int) { + for i := 0; i < n; i++ { + url := "https://prices.example/page" + string(rune('a'+i)) + next := "" + if i < n-1 { + next = "https://prices.example/page" + string(rune('a'+i+1)) + } + client.responses[url] = okJSONResponse( + `{"Items":[{"name":"` + string(rune('a'+i)) + `"}],"NextPageLink":"` + next + `"}`, + ) + } +} + // TestFetchAll_MergesPages pins the multi-page walk: page 1 has a non- // empty NextPageLink, page 2 has an empty NextPageLink, all items are // merged into the returned slice in order. @@ -94,29 +110,35 @@ func TestFetchAll_RejectsSelfReferentialNextPageLink(t *testing.T) { assert.Contains(t, err.Error(), "self-referential") } -// TestFetchAll_HonoursMaxPagesCap covers the defensive cap: if the server -// returns a genuinely unbounded chain of fresh NextPageLinks, the walker -// must stop after maxPages instead of running forever. -func TestFetchAll_HonoursMaxPagesCap(t *testing.T) { +// TestFetchAll_ErrorsWhenCapReachedWithPagesRemaining is the regression +// test for #1963: a chain longer than maxPages must produce an error, not +// the first maxPages pages with a nil error. The walker must still stop +// fetching at the cap. +func TestFetchAll_ErrorsWhenCapReachedWithPagesRemaining(t *testing.T) { client := newFakeHTTPClient() - for i := 0; i < 10; i++ { - url := "https://prices.example/page" + string(rune('a'+i)) - next := "" - if i < 9 { - next = "https://prices.example/page" + string(rune('a'+i+1)) - } - client.responses[url] = okJSONResponse( - `{"Items":[{"name":"` + string(rune('a'+i)) + `"}],"NextPageLink":"` + next + `"}`, - ) - } + scriptPageChain(client, 10) + + items, err := FetchAll[fakeItem](context.Background(), client, "https://prices.example/pagea", DefaultPageTimeout, 3) + require.Error(t, err) + assert.Contains(t, err.Error(), "3-page cap") + assert.Contains(t, err.Error(), "3 items read") + assert.Nil(t, items, "a truncated result must not be returned alongside the error") + assert.Len(t, client.calls, 3, "walker must not fetch beyond maxPages") +} + +// TestFetchAll_ExactlyMaxPagesSucceeds pins the boundary: a chain of +// exactly maxPages pages whose last page has an empty NextPageLink is +// complete and must not be reported as truncated. +func TestFetchAll_ExactlyMaxPagesSucceeds(t *testing.T) { + client := newFakeHTTPClient() + scriptPageChain(client, 3) - // Cap at 3 — walker should read pages a, b, c only. items, err := FetchAll[fakeItem](context.Background(), client, "https://prices.example/pagea", DefaultPageTimeout, 3) require.NoError(t, err) require.Len(t, items, 3) assert.Equal(t, "a", items[0].Name) assert.Equal(t, "c", items[2].Name) - assert.Len(t, client.calls, 3, "walker must not fetch beyond maxPages") + assert.Len(t, client.calls, 3) } // TestFetchAll_PerPageTimeout proves the per-page timeout is applied diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index fb25aa1f7..1f5d6ada6 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -567,7 +567,7 @@ func (c *CacheClient) getRedisPricing(ctx context.Context, sku, region string, t } // fetchAzurePricing fetches pricing data from Azure Retail Prices API, -// following NextPageLink until exhausted or the shared safety cap fires. +// following NextPageLink until exhausted; hitting the shared page cap is an error. // Delegates pagination to pricing.FetchAll. func (c *CacheClient) fetchAzurePricing(ctx context.Context, serviceName, sku, region string) (*AzureRetailPrice, error) { filter := fmt.Sprintf("serviceName eq '%s' and armRegionName eq '%s' and contains(armSkuName, '%s')", diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index c74398d00..6d4badcd6 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -728,8 +728,8 @@ func (c *ComputeClient) getVMPricing(ctx context.Context, vmSize, region string, } // fetchAzurePricing fetches pricing data from Azure Retail Prices API, -// following NextPageLink until exhausted (or the shared safety cap is -// hit). Delegates the pagination walk to pricing.FetchAll so every +// following NextPageLink until exhausted (hitting the shared page cap +// is an error). Delegates the pagination walk to pricing.FetchAll so every // service client shares the same per-page timeout, seen-URL guard, and // max-pages cap — see providers/azure/internal/pricing for those // invariants. diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index 5c20f3d35..66008c2f9 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -567,7 +567,7 @@ func (c *CosmosDBClient) getCosmosPricing(ctx context.Context, sku, region strin } // fetchAzurePricing fetches pricing data from Azure Retail Prices API, -// following NextPageLink until exhausted or the shared safety cap fires. +// following NextPageLink until exhausted; hitting the shared page cap is an error. // Delegates pagination to pricing.FetchAll. func (c *CosmosDBClient) fetchAzurePricing(ctx context.Context, filter string) (*AzureRetailPrice, error) { baseURL := "https://prices.azure.com/api/retail/prices" diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index d52191351..13f94e3d8 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -586,7 +586,7 @@ func (c *DatabaseClient) getSQLPricing(ctx context.Context, sku, region string, } // fetchAzurePricing fetches pricing data from Azure Retail Prices API, -// following NextPageLink until exhausted or the shared safety cap fires. +// following NextPageLink until exhausted; hitting the shared page cap is an error. // Delegates pagination to pricing.FetchAll — see // providers/azure/internal/pricing for the per-page timeout, max-pages // cap, and seen-URL guard invariants.