From 10393059d03610272f3c72ddcff04a4555769eac Mon Sep 17 00:00:00 2001 From: shortcuts Date: Fri, 25 Jul 2025 10:36:35 +0200 Subject: [PATCH 1/5] fix(clients): retry strategy on error --- scripts/cts/runCts.ts | 2 + scripts/cts/testServer/error.ts | 59 +++++++++++++++++++ scripts/cts/testServer/index.ts | 4 ++ .../replaceAllObjectsWithTransformation.ts | 1 - tests/CTS/client/search/api.json | 39 +++++++++++- 5 files changed, 103 insertions(+), 2 deletions(-) create mode 100644 scripts/cts/testServer/error.ts diff --git a/scripts/cts/runCts.ts b/scripts/cts/runCts.ts index a38921927cc..28577b6602f 100644 --- a/scripts/cts/runCts.ts +++ b/scripts/cts/runCts.ts @@ -8,6 +8,7 @@ import type { Language } from '../types.ts'; import { assertValidAccountCopyIndex } from './testServer/accountCopyIndex.ts'; import { printBenchmarkReport } from './testServer/benchmark.ts'; import { assertChunkWrapperValid } from './testServer/chunkWrapper.ts'; +import { assertValidErrors } from './testServer/error.ts'; import { startTestServer } from './testServer/index.ts'; import { assertPushMockValid } from './testServer/pushMock.ts'; import { assertValidReplaceAllObjects } from './testServer/replaceAllObjects.ts'; @@ -152,6 +153,7 @@ export async function runCts( const skip = (lang: Language): number => (languages.includes(lang) ? 1 : 0); const only = skip; + assertValidErrors(languages.length); assertValidTimeouts(languages.length); assertChunkWrapperValid(languages.length - skip('dart')); assertValidReplaceAllObjects(languages.length - skip('dart')); diff --git a/scripts/cts/testServer/error.ts b/scripts/cts/testServer/error.ts new file mode 100644 index 00000000000..c3a54a9f910 --- /dev/null +++ b/scripts/cts/testServer/error.ts @@ -0,0 +1,59 @@ +import type { Server } from 'http'; + +import { expect } from 'chai'; +import type express from 'express'; + +import { setupServer } from './index.ts'; + +const errorState: Record = {}; + +export function assertValidErrors(expectedCount: number): void { + // assert that the retry strategy uses the correct timings, by checking the time between each request, and how long each request took before being timed out + // there should be no delay between requests, only an increase in error. + if (Object.keys(errorState).length !== expectedCount) { + throw new Error(`Expected ${expectedCount} error(s)`); + } + + for (const [lang, state] of Object.entries(errorState)) { + let numberOfTestSuites = 1; + + // python has sync and async tests + if (lang === 'python') { + numberOfTestSuites = 2; + } + + expect(state.errorCount).to.equal(Number(numberOfTestSuites) * 3); + } +} + +function addRoutes(app: express.Express): void { + app.post('/1/test/error/:lang', (req, res) => { + const lang = req.params.lang; + if (!errorState[lang]) { + errorState[lang] = { + errorCount: 0, + }; + } + + errorState[lang].errorCount++; + + if (errorState[lang].errorCount < 3) { + res.status(500).json({ message: 'error test server response' }); + return; + } + + res.status(200).json({ status: 'ok' }); + }); +} + +export function errorServer(): Promise { + return setupServer('error', 6671, addRoutes); +} + +export function errorServerRetriedOnce(): Promise { + return setupServer('errorRetriedOnce', 6672, addRoutes); +} + +export function errorServerRetriedTwice(): Promise { + return setupServer('errorRetriedTwice', 6673, addRoutes); +} diff --git a/scripts/cts/testServer/index.ts b/scripts/cts/testServer/index.ts index 448c69d630d..f4fdde578ea 100644 --- a/scripts/cts/testServer/index.ts +++ b/scripts/cts/testServer/index.ts @@ -12,6 +12,7 @@ import { algoliaMockServer } from './algoliaMock.ts'; import { apiKeyServer } from './apiKey.ts'; import { benchmarkServer } from './benchmark.ts'; import { chunkWrapperServer } from './chunkWrapper.ts'; +import { errorServer, errorServerRetriedOnce, errorServerRetriedTwice } from './error.ts'; import { gzipServer } from './gzip.ts'; import { pushMockServer } from './pushMock.ts'; import { replaceAllObjectsServer } from './replaceAllObjects.ts'; @@ -27,6 +28,9 @@ export async function startTestServer(suites: Record): Promise if (suites.client) { toStart.push( timeoutServer(), + errorServer(), + errorServerRetriedOnce(), + errorServerRetriedTwice(), gzipServer(), timeoutServerBis(), accountCopyIndexServer(), diff --git a/scripts/cts/testServer/replaceAllObjectsWithTransformation.ts b/scripts/cts/testServer/replaceAllObjectsWithTransformation.ts index 09d70ba9692..1bcb2e88079 100644 --- a/scripts/cts/testServer/replaceAllObjectsWithTransformation.ts +++ b/scripts/cts/testServer/replaceAllObjectsWithTransformation.ts @@ -88,7 +88,6 @@ function addRoutes(app: Express): void { case 'move': { const lang = req.body.destination.replace('cts_e2e_replace_all_objects_with_transformation_', ''); expect(raowtState).to.include.keys(lang); - console.log(raowtState[lang]); expect(raowtState[lang]).to.deep.equal({ copyCount: 2, pushCount: 4, diff --git a/tests/CTS/client/search/api.json b/tests/CTS/client/search/api.json index b57dca7daa0..0ece918c75f 100644 --- a/tests/CTS/client/search/api.json +++ b/tests/CTS/client/search/api.json @@ -109,7 +109,7 @@ ] }, { - "testName": "tests the retry strategy error", + "testName": "tests the retry strategy on timeout", "autoCreateClient": false, "steps": [ { @@ -148,6 +148,43 @@ } ] }, + { + "testName": "tests the retry strategy on 5xx", + "autoCreateClient": false, + "steps": [ + { + "type": "createClient", + "parameters": { + "appId": "test-app-id", + "apiKey": "test-api-key", + "customHosts": [ + { + "port": 6671 + }, + { + "port": 6672 + }, + { + "port": 6673 + } + ] + } + }, + { + "type": "method", + "method": "customPost", + "parameters": { + "path": "1/test/error/${{language}}" + }, + "expected": { + "type": "response", + "match": { + "status": "ok" + } + } + } + ] + }, { "testName": "test the compression strategy", "autoCreateClient": false, From c47cbe76ba0da427eb61aeb0bdd852213dbbebc7 Mon Sep 17 00:00:00 2001 From: Antoine GILLES Date: Fri, 25 Jul 2025 11:18:26 +0200 Subject: [PATCH 2/5] fix(go/transport) Fix retry strategy because of req.Body read multiple times. --- .../algolia/transport/transport.go | 38 ++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/clients/algoliasearch-client-go/algolia/transport/transport.go b/clients/algoliasearch-client-go/algolia/transport/transport.go index 018a9ff252e..534ac6b076c 100644 --- a/clients/algoliasearch-client-go/algolia/transport/transport.go +++ b/clients/algoliasearch-client-go/algolia/transport/transport.go @@ -44,6 +44,27 @@ func New(cfg Configuration) *Transport { return transport } +func prepareRetryableRequest(req *http.Request) (*http.Request, error) { + // Read the original body + if req.Body == nil { + return req, nil // Nothing to do if there's no body + } + + bodyBytes, err := io.ReadAll(req.Body) + if err != nil { + return nil, err + } + _ = req.Body.Close() // close the original body + + // Set up GetBody to recreate the body for retries + req.Body = io.NopCloser(bytes.NewReader(bodyBytes)) + req.GetBody = func() (io.ReadCloser, error) { + return io.NopCloser(bytes.NewReader(bodyBytes)), nil + } + + return req, nil +} + func (t *Transport) Request(ctx context.Context, req *http.Request, k call.Kind, c RequestConfiguration) (*http.Response, []byte, error) { var intermediateNetworkErrors []error @@ -52,7 +73,10 @@ func (t *Transport) Request(ctx context.Context, req *http.Request, k call.Kind, req.Header.Add("Content-Encoding", "gzip") } - for _, h := range t.retryStrategy.GetTryableHosts(k) { + // Prepare the request to be retryable. + req, _ = prepareRetryableRequest(req) + + for i, h := range t.retryStrategy.GetTryableHosts(k) { // Handle per-request timeout by using a context with timeout. // Note that because we are in a loop, the cancel() callback cannot be // deferred. Instead, we call it precisely after the end of each loop or @@ -62,8 +86,17 @@ func (t *Transport) Request(ctx context.Context, req *http.Request, k call.Kind, var ( ctxTimeout time.Duration connectTimeout time.Duration + err error ) + // Reassign a fresh body for the retry + if i > 0 && req.GetBody != nil { + req.Body, err = req.GetBody() + if err != nil { + break + } + } + switch { case k == call.Read && c.ReadTimeout != nil: ctxTimeout = *c.ReadTimeout @@ -113,6 +146,9 @@ func (t *Transport) Request(ctx context.Context, req *http.Request, k call.Kind, default: if err != nil { intermediateNetworkErrors = append(intermediateNetworkErrors, err) + } else if res != nil { + msg := fmt.Sprintf("cannot perform request:\n\tStatusCode=%d\n\tmethod=%s\n\turl=%s\n\t", res.StatusCode, req.Method, req.URL) + intermediateNetworkErrors = append(intermediateNetworkErrors, errors.New(msg)) } if res != nil && res.Body != nil { if err = res.Body.Close(); err != nil { From 84f0d1dde1662032fb550a0ebe1c07ffd2ec8141 Mon Sep 17 00:00:00 2001 From: shortcuts Date: Fri, 25 Jul 2025 11:26:36 +0200 Subject: [PATCH 3/5] fix: python assertion --- scripts/cts/testServer/error.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/scripts/cts/testServer/error.ts b/scripts/cts/testServer/error.ts index c3a54a9f910..aec810a2fbb 100644 --- a/scripts/cts/testServer/error.ts +++ b/scripts/cts/testServer/error.ts @@ -5,7 +5,7 @@ import type express from 'express'; import { setupServer } from './index.ts'; -const errorState: Record = {}; +const errorState: Record = {}; export function assertValidErrors(expectedCount: number): void { // assert that the retry strategy uses the correct timings, by checking the time between each request, and how long each request took before being timed out @@ -15,14 +15,14 @@ export function assertValidErrors(expectedCount: number): void { } for (const [lang, state] of Object.entries(errorState)) { - let numberOfTestSuites = 1; - // python has sync and async tests if (lang === 'python') { - numberOfTestSuites = 2; + expect(state.errorCount).to.equal(state.maxError * 2); + + return; } - expect(state.errorCount).to.equal(Number(numberOfTestSuites) * 3); + expect(state.errorCount).to.equal(state.maxError); } } @@ -32,12 +32,13 @@ function addRoutes(app: express.Express): void { if (!errorState[lang]) { errorState[lang] = { errorCount: 0, + maxError: 3, }; } errorState[lang].errorCount++; - if (errorState[lang].errorCount < 3) { + if (errorState[lang].errorCount % errorState[lang].maxError !== 0) { res.status(500).json({ message: 'error test server response' }); return; } From e6ca33c0cdd0e5253e8963bbf4bbfb24c54ba177 Mon Sep 17 00:00:00 2001 From: shortcuts Date: Fri, 25 Jul 2025 11:38:22 +0200 Subject: [PATCH 4/5] fix: go --- clients/algoliasearch-client-go/algolia/transport/transport.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clients/algoliasearch-client-go/algolia/transport/transport.go b/clients/algoliasearch-client-go/algolia/transport/transport.go index 534ac6b076c..b91e33ed4f3 100644 --- a/clients/algoliasearch-client-go/algolia/transport/transport.go +++ b/clients/algoliasearch-client-go/algolia/transport/transport.go @@ -52,7 +52,7 @@ func prepareRetryableRequest(req *http.Request) (*http.Request, error) { bodyBytes, err := io.ReadAll(req.Body) if err != nil { - return nil, err + return nil, fmt.Errorf("cannot read body: %v", err) } _ = req.Body.Close() // close the original body From f9321b5bc5089a132a1a8908055e1b1c9612cbc4 Mon Sep 17 00:00:00 2001 From: shortcuts Date: Fri, 25 Jul 2025 12:29:40 +0200 Subject: [PATCH 5/5] chore: err --- .../algoliasearch-client-go/algolia/transport/transport.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/clients/algoliasearch-client-go/algolia/transport/transport.go b/clients/algoliasearch-client-go/algolia/transport/transport.go index b91e33ed4f3..fc8e77069d6 100644 --- a/clients/algoliasearch-client-go/algolia/transport/transport.go +++ b/clients/algoliasearch-client-go/algolia/transport/transport.go @@ -74,7 +74,10 @@ func (t *Transport) Request(ctx context.Context, req *http.Request, k call.Kind, } // Prepare the request to be retryable. - req, _ = prepareRetryableRequest(req) + req, err := prepareRetryableRequest(req) + if err != nil { + return nil, nil, err + } for i, h := range t.retryStrategy.GetTryableHosts(k) { // Handle per-request timeout by using a context with timeout.