From 5128137ef67520e9736f8fd3fda0756667cab951 Mon Sep 17 00:00:00 2001 From: Denis Hamon Date: Fri, 21 Aug 2026 14:34:10 +0200 Subject: [PATCH] docs(http): pin the index alignment of FetchObjectsParallel, which looks like a bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The result of FetchObjectsParallel is index-aligned with its ids, and stays aligned when ignoreErrors is set: an id whose fetch failed keeps its slot, holding the zero value of T. A slice with nil holes in it reads like an oversight, so this was queued as a defect to fix in the helper — one change for the ~30 call sites that share it. Measured before changing anything, and the measurement says do not: - Seven call sites in internal/services/browser pair objects[i] with a name held in a parallel slice (allNames[i], allRegionNames[i]). Compacting the result would attach one region's details to another region's name — silently, and far worse than a hole. - Of the 30 call sites, none is actually broken by the padding. Two pass ignoreErrors false, so a failure returns an error and no holes exist. Twenty pass true and skip nil explicitly, several through FetchExpandedArray, which filters. The remaining eight are typed []map[string]any, where the hole is a nil slice that ranges as empty. So the contract is documented where it lives, and a test pins it: it fails if anyone makes the result dense. Verified in both directions — the test passes as is and fails against a compacting version of the helper, which is the change this comment exists to prevent. Signed-off-by: Denis Hamon Co-Authored-By: Claude Opus 5 (1M context) --- internal/http/client.go | 19 +++++++++++++++++++ internal/http/client_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/internal/http/client.go b/internal/http/client.go index 6c5222839..00bcb8238 100644 --- a/internal/http/client.go +++ b/internal/http/client.go @@ -66,6 +66,25 @@ func InitClientWithProfile(cfg *ini.File, profileOverride string) { } } +// FetchObjectsParallel fetches one object per id, ten requests at a time. +// +// The result is INDEX-ALIGNED with ids, and stays aligned when ignoreErrors is +// set: an id whose fetch failed keeps its slot, holding the zero value of T. That +// is the contract, not an oversight, and compacting the result would be a silent +// breaking change — seven call sites in internal/services/browser pair +// objects[i] with a name they hold in a parallel slice, so dropping a hole there +// would attach one region's details to another region's name. +// +// What that leaves each caller to do depends on T, and measured across the 30 +// call sites all three cases are already handled: +// +// - ignoreErrors false: a failure returns an error and no holes exist (2 sites). +// - T is a map or a struct: the hole is nil and has to be skipped explicitly +// (20 sites do, several of them through FetchExpandedArray, which filters). +// - T is a slice: the hole is a nil slice, which ranges as empty (8 sites). +// +// So it is safe to leave alone, and worth saying so: the padding looks like a bug +// until you know who depends on it. func FetchObjectsParallel[T any](path string, ids []any, ignoreErrors bool) ([]T, error) { var ( parallelRequests = 10 diff --git a/internal/http/client_test.go b/internal/http/client_test.go index b48f1aef4..467207090 100644 --- a/internal/http/client_test.go +++ b/internal/http/client_test.go @@ -65,3 +65,36 @@ func TestFetchObjectsParallel_IgnoredErrorLogging(t *testing.T) { td.Cmp(t, strings.Contains(run(true), "error fetching"), true, "ignored errors must be logged when debug is enabled") } + +// The index alignment of FetchObjectsParallel is a contract that seven call sites +// in internal/services/browser depend on: they pair objects[i] with a name held +// in a parallel slice, so a result compacted to drop failures would attach one +// region's details to another region's name. +// +// The padding reads like a bug — a slice with nil holes in it — which is exactly +// why it needs a test. This one fails if anyone makes the result dense. +func TestFetchObjectsParallelKeepsFailedItemsInPlace(t *testing.T) { + httpmock.Activate(t) + client, err := ovh.NewClient("ovh-eu", "k", "s", "c") + td.Require(t).CmpNoError(err) + saved := Client + Client = client + t.Cleanup(func() { Client = saved }) + + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/1.0/auth/time", + httpmock.NewStringResponder(200, "0")) + for _, id := range []string{"a", "c"} { + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/1.0/thing/"+id, + httpmock.NewStringResponder(200, `{"name":"`+id+`"}`)) + } + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/1.0/thing/b", + httpmock.NewStringResponder(500, `{"message":"nope"}`)) + + objects, err := FetchObjectsParallel[map[string]any]("/thing/%s", []any{"a", "b", "c"}, true) + + td.Require(t).CmpNoError(err, "ignoreErrors means the batch still succeeds") + td.Require(t).Cmp(len(objects), 3, "one slot per id, however many failed") + td.Cmp(t, objects[0]["name"], "a") + td.CmpNil(t, objects[1], "the failed id keeps its slot, holding the zero value") + td.Cmp(t, objects[2]["name"], "c", "and the ids after it are not shifted up") +}