Skip to content

BRD-1239: add connect timeout and idempotent-only retry to the HTTP clients - #81

Open
rokaszygmantas wants to merge 1 commit into
mainfrom
brd-1239-outbound-http-policy
Open

BRD-1239: add connect timeout and idempotent-only retry to the HTTP clients#81
rokaszygmantas wants to merge 1 commit into
mainfrom
brd-1239-outbound-http-policy

Conversation

@rokaszygmantas

@rokaszygmantas rokaszygmantas commented Sep 3, 2026

Copy link
Copy Markdown
Member

Ticket: BRD-1239

Summary

  • Every HTTP client now sets a connect timeout (default 10s) next to the existing read timeout, and SyncConfigV2 finally exposes both, so brad-app can size bulk calls above the 30s default.
  • Idempotent calls (GET, PUT, DELETE, PATCH, bulk-operations, V1 sync/delete-products, normalize) retry transport errors, 5xx and 429 with jittered exponential backoff, 3 attempts by default. Configuration POSTs never retry.
  • Why: nightly bulk syncs were cancelled mid-flight at the 03:00 peak (BRD-1205) and a single transient 502 cost a full 60s queue-level retry in brad-app.
  • The cURL call moved behind a small Transport interface so the retry loop is unit-tested with a scripted fake; TransportException extends ApiException keeps existing catch blocks working.

Proof

No UI surface; browser QA does not apply to a PHP library.

vendor/bin/phpunit --no-progress   Tests: 1671, Assertions: 5564 (35 new), OK
vendor/bin/phpstan analyse         [OK] No errors
vendor/bin/phpcs src tests         exit 0

Real-network smoke against a non-routable IP with connectTimeout: 2, RetryPolicy::none():

TransportException after 2.00s: cURL error: Connection timed out after 2002 milliseconds (status=0, body=NULL)

Acceptance criteria evidence

  • AC-1 Every transport sets CURLOPT_CONNECTTIMEOUT from config, default 10s
    • HttpClientRetryTest::testRequestCarriesTimeoutsHeadersAndBody, SyncV2SdkTransportTest::testDefaultsAreThirtySecondReadAndTenSecondConnect, AdminSdkTransportTest::testAdminHeaderAndTimeoutsReachTheTransport; Magento client via ConnectTimeoutConfigTest::testMagentoConfigHasConnectTimeout plus the smoke run above
  • AC-2 SyncConfig, SyncConfigV2, MagentoConfig accept optional connectTimeout/timeout; SyncV2Sdk forwards them; existing callers compile unchanged
    • ConnectTimeoutConfigTest (defaults, positional callers, validation), SyncV2SdkTransportTest::testTimeoutsAndAuthReachTheTransport, full existing suite green
  • AC-3 Idempotent calls retry on connect error, 5xx and 429 with jittered backoff (1s to 8s cap, 3 attempts); bulk POSTs idempotent, config POSTs never
    • HttpClientRetryTest::testRetriesIdempotentCallOn502ThenSucceeds, ::testRetriesIdempotentCallOn429, ::testRetriesIdempotentCallOnTransportError, ::testRetriesPostFlaggedIdempotent, ::testDoesNotRetryNonIdempotentPostOn503, SyncV2SdkTransportTest::testBulkOperationsIsRetriedOn503, ::testConfigurationRefreshIsNotRetriedOn503
  • AC-4 4xx other than 429 never retries
    • HttpClientRetryTest::testDoesNotRetryOn400, RetryPolicyTest::testRetryableStatuses
  • AC-5 After the final attempt the thrown exception carries the last status code and body; transport errors carry the cURL error string
    • HttpClientRetryTest::testThrowsLastResponseWhenAttemptsExhausted, ::testThrowsLastTransportErrorWhenAttemptsExhausted
  • AC-6 Unit tests use a fake transport, do not sleep or hit the network, and check backoff stays within jitter bounds
    • tests/Client/Support/FakeTransport.php, FakeSleeper.php; HttpClientRetryTest::testBackoffDelaysStayWithinJitterBounds, RetryPolicyTest::testDelayGrowsExponentiallyWithEqualJitterAndCap

AC-7 to AC-13 belong to the brad-app half of the ticket and ship in a separate PR after this one is tagged.

Details
  • Idempotency is an explicit flag on HttpClient::post(), not a path rule. The facade marks bulk-operations, V1 sync/delete-products and normalize; everything else that POSTs stays single-attempt. This is the one place a future endpoint has to make a conscious choice.
  • Equal jitter ([d/2, d]) rather than full jitter, so the minimum wait still grows and tests can assert bounds.
  • No Retry-After parsing. brad-search does not emit it today; add when it does.
  • MagentoGraphQLClient gets the connect timeout only, no retry. It talks to merchant stores, and retry policy for store APIs is BRD-1206's concern.
  • curl_close() dropped in the new CurlTransport: no-op since PHP 8.0, deprecated in 8.5. Left untouched in the Magento client.
  • Semver: additive, source-compatible. Suggested tag v4.16.0.
  • Pre-existing suite noise (1 PHPUnit warning about coverage driver, 27 PHPUnit deprecations) is unchanged by this PR.

Confidence: 4/5 - every SDK criterion has a named test and the real cURL path was smoke-tested, but the backoff timing only ran against the fake sleeper, never a real stalled brad-search.

Bulk syncs were cancelled mid-flight during the nightly peak and a single
transient 502 cost the caller a full queue-level retry. The transports set
only CURLOPT_TIMEOUT (30s) and had no retry at all.

- Move the cURL call behind a Transport interface with an HttpRequest that
  carries both timeouts; CurlTransport sets CURLOPT_CONNECTTIMEOUT.
- HttpClient retries transport errors, 5xx and 429 for idempotent calls
  using RetryPolicy (3 attempts, 1s doubling to 8s, equal jitter). POST
  is non-idempotent unless the facade passes idempotent: true; only
  bulk-operations, V1 sync/delete-products and normalize do.
- SyncConfig, SyncConfigV2 and MagentoConfig gain connectTimeout (10s);
  SyncConfigV2 also gains timeout and retryPolicy and forwards them.
- AdminHttpClient wraps HttpClient so admin calls share the policy.
- TransportException extends ApiException (status 0) for no-response
  failures, so existing catch blocks keep working.
- SyncV2Sdk and AdminSdk accept an optional Transport for tests.

Tests script responses through a fake transport and sleeper; no network,
no real sleeps.
@rokaszygmantas

Copy link
Copy Markdown
Member Author

Consumer half: Invertus/brad-app#565 pins this branch at 3d5c55e as 4.16.0 until the tag exists. Suggested tag after merge: v4.16.0.

@rokaszygmantas
rokaszygmantas marked this pull request as ready for review September 3, 2026 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant