fix: retry transient errors on api autocomplete status GET#115
fix: retry transient errors on api autocomplete status GET#115vtushar06 wants to merge 1 commit into
Conversation
Signed-off-by: Tushar Verma <tusharmyself06@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the OpenCost API client path used by the autocomplete probe/validation by ensuring /allocation/.../autocomplete status GETs go through the same retry-capable HTTP client path as other API calls, and adds deterministic unit coverage for the retry behavior.
Changes:
- Route
GetAutocompleteStatusthrough a newhttpGetWithRetryhelper instead ofhttp.Get. - Add
httpGetWithRetryto retry GET transport errors with the existingMAX_RETRIESand a configurable backoff (test-shortenable). - Add deterministic unit tests for retry success and retry exhaustion via an injected
RoundTripper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/api/api.go | Adds httpGetWithRetry and a configurable retry backoff used by the autocomplete status path. |
| pkg/api/autocomplete.go | Switches GetAutocompleteStatus from raw http.Get to the retrying helper using sharedHTTPClient. |
| pkg/api/retry_test.go | Adds deterministic unit tests covering retry success and retry exhaustion for httpGetWithRetry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@ameijer went through the run-tests failure, none of it is from my change. 2 are pre-existing demo backend issues unrelated to networking (asset type list out of sync, and node UIDs not matching across metric snapshots), the 3rd is a real conn-reset but on the MCP client not the api client I touched here, can do a follow up for that one if useful. build/vet/unit tests are all green on my end, still approved so good to merge whenever |
Description
Follow-up to #106 (thanks @ameijer for catching that the retries weren't firing). The connection resets in that run were on the OpenCost API client path, not the Prometheus client I hardened in #106.
API.GETalready retries transport errors, butGetAutocompleteStatus(used by the autocomplete probe and validation tests) used a rawhttp.Getthat bypassed that retry, so a connection reset there failed the run with no retry and no log, e.g.:This routes that GET through a small
httpGetWithRetryhelper that retries transient transport errors with the sameMAX_RETRIESand backoff asAPI.GET, and logs each retry so they show up in the run output. As a bonus it now uses the shared client's timeout instead of the barehttp.Get.Related
Follow-up to #106.
Testing
pkg/api/retry_test.go(deterministic, injectedRoundTripper): retries transient errors then succeeds, and returns the error afterMAX_RETRIES.go build ./...,go vet ./pkg/api/, and thepkg/apitests pass locally.One thing out of scope: the other failure in that run (
TestAllocationAutocompletePrometheusGroundTruth, 74.2% coverage) is a data-coverage assertion, not a network issue, so it's not addressed here.cc @ameijer