From 16cebc1163b472c75f7a4f38ce728fc7ddde9564 Mon Sep 17 00:00:00 2001 From: Sean O'Brien Date: Wed, 22 Jul 2026 15:36:15 -0400 Subject: [PATCH] bugfix: new retries --- .changes/nextrelease/bugfix-retries.json | 7 ++ src/Middleware.php | 10 +- src/Retry/ConfigurationProvider.php | 44 ++++++--- src/Retry/RateLimiter.php | 23 +++++ src/Retry/RetryHelperTrait.php | 8 ++ src/Retry/V3/RetryMiddleware.php | 55 +++++++---- src/S3/AmbiguousSuccessParser.php | 6 +- src/Signature/SignatureV4.php | 4 + tests/Retry/RateLimiterTest.php | 70 +++++++++++++ tests/Retry/V3/RetryMiddlewareTest.php | 120 +++++++++++++++++------ 10 files changed, 283 insertions(+), 64 deletions(-) create mode 100644 .changes/nextrelease/bugfix-retries.json diff --git a/.changes/nextrelease/bugfix-retries.json b/.changes/nextrelease/bugfix-retries.json new file mode 100644 index 0000000000..1fa68fc787 --- /dev/null +++ b/.changes/nextrelease/bugfix-retries.json @@ -0,0 +1,7 @@ +[ + { + "type": "bugfix", + "category": "", + "description": "`Aws\\Retry` - Under the `AWS_NEW_RETRIES_2026` opt-in, emit the SEP-required retry headers `amz-sdk-invocation-id` and `amz-sdk-request` (formatted as `attempt=; max=`) instead of the legacy `aws-sdk-invocation-id` / `aws-sdk-retry`. Both legacy and new header names are excluded from SigV4 signing so intermediaries can add or modify them without invalidating signatures." + } +] diff --git a/src/Middleware.php b/src/Middleware.php index 13a19d6b92..cd7387eb3a 100644 --- a/src/Middleware.php +++ b/src/Middleware.php @@ -259,13 +259,17 @@ public static function retry( */ public static function invocationId() { - return function (callable $handler) { + $headerName = \Aws\Retry\V3\OptIn::isEnabled() + ? 'amz-sdk-invocation-id' + : 'aws-sdk-invocation-id'; + + return function (callable $handler) use ($headerName) { return function ( CommandInterface $command, RequestInterface $request - ) use ($handler){ + ) use ($handler, $headerName){ return $handler($command, $request->withHeader( - 'aws-sdk-invocation-id', + $headerName, md5(uniqid(gethostname(), true)) )); }; diff --git a/src/Retry/ConfigurationProvider.php b/src/Retry/ConfigurationProvider.php index 5194dcdfd8..036dabf5f6 100644 --- a/src/Retry/ConfigurationProvider.php +++ b/src/Retry/ConfigurationProvider.php @@ -106,12 +106,22 @@ public static function defaultProvider(array $config = []) public static function env() { return function () { - // Use config from environment variables, if available - $mode = getenv(self::ENV_MODE); - $maxAttempts = getenv(self::ENV_MAX_ATTEMPTS) - ? getenv(self::ENV_MAX_ATTEMPTS) - : self::DEFAULT_MAX_ATTEMPTS; - if (!empty($mode)) { + $modeEnv = getenv(self::ENV_MODE); + $maxAttemptsEnv = getenv(self::ENV_MAX_ATTEMPTS); + + if (!empty($modeEnv) || !empty($maxAttemptsEnv)) { + $mode = !empty($modeEnv) + ? $modeEnv + : self::getDefaultMode(); + $maxAttempts = !empty($maxAttemptsEnv) + ? $maxAttemptsEnv + : self::DEFAULT_MAX_ATTEMPTS; + + if (empty($modeEnv) && !OptIn::isEnabled()) { + return self::reject('Could not find environment variable' + . ' config in ' . self::ENV_MODE); + } + return Promise\Create::promiseFor( new Configuration($mode, $maxAttempts) ); @@ -136,10 +146,6 @@ public static function fallback() }; } - /** - * Returns the default retry mode. Reflects the AWS_NEW_RETRIES_2026 - * opt-in: 'standard' when the env flag is set, 'legacy' otherwise. - */ public static function getDefaultMode(): string { return OptIn::isEnabled() ? 'standard' : self::DEFAULT_MODE; @@ -175,18 +181,30 @@ public static function ini( if (!isset($data[$profile])) { return self::reject("'$profile' not found in config file"); } - if (!isset($data[$profile][self::INI_MODE])) { + + $hasMode = isset($data[$profile][self::INI_MODE]); + $hasMaxAttempts = isset($data[$profile][self::INI_MAX_ATTEMPTS]); + + if (!$hasMode && !$hasMaxAttempts) { + return self::reject("Required retry config values + not present in INI profile '{$profile}' ({$filename})"); + } + + if (!$hasMode && !OptIn::isEnabled()) { return self::reject("Required retry config values not present in INI profile '{$profile}' ({$filename})"); } - $maxAttempts = isset($data[$profile][self::INI_MAX_ATTEMPTS]) + $mode = $hasMode + ? $data[$profile][self::INI_MODE] + : self::getDefaultMode(); + $maxAttempts = $hasMaxAttempts ? $data[$profile][self::INI_MAX_ATTEMPTS] : self::DEFAULT_MAX_ATTEMPTS; return Promise\Create::promiseFor( new Configuration( - $data[$profile][self::INI_MODE], + $mode, $maxAttempts ) ); diff --git a/src/Retry/RateLimiter.php b/src/Retry/RateLimiter.php index b58cc5929f..f65ae5f569 100644 --- a/src/Retry/RateLimiter.php +++ b/src/Retry/RateLimiter.php @@ -67,6 +67,29 @@ public function getSendToken() $this->acquireToken(1); } + /** + * @return int Non-blocking delay hint in milliseconds; 0 if a token is + * available immediately. A token is debited on each call. + */ + public function getSendDelayMs(): int + { + if (!$this->enabled) { + return 0; + } + + $this->refillTokenBucket(); + $delayMs = 0; + if (1 > $this->currentCapacity) { + $needed = 1 - $this->currentCapacity; + $fillRate = max($this->fillRate, $this->minFillRate); + $delayMs = (int) ceil(1000 * $needed / $fillRate); + } + + $this->currentCapacity = max(0, $this->currentCapacity - 1); + + return $delayMs; + } + public function updateSendingRate($isThrottled) { $this->updateMeasuredRate(); diff --git a/src/Retry/RetryHelperTrait.php b/src/Retry/RetryHelperTrait.php index a7edb72b59..37a1b73cc2 100644 --- a/src/Retry/RetryHelperTrait.php +++ b/src/Retry/RetryHelperTrait.php @@ -11,6 +11,14 @@ private function addRetryHeader($request, $retries, $delayBy) return $request->withHeader('aws-sdk-retry', "{$retries}/{$delayBy}"); } + private function addRetryHeaderV3($request, int $attempt, int $maxAttempts) + { + return $request->withHeader( + 'amz-sdk-request', + "attempt={$attempt}; max={$maxAttempts}" + ); + } + private function updateStats($retries, $delay, array &$stats) { diff --git a/src/Retry/V3/RetryMiddleware.php b/src/Retry/V3/RetryMiddleware.php index e009a9fb55..15cccb5d14 100644 --- a/src/Retry/V3/RetryMiddleware.php +++ b/src/Retry/V3/RetryMiddleware.php @@ -51,6 +51,7 @@ class RetryMiddleware private static array $standardTransientErrors = [ 'RequestTimeout' => true, 'RequestTimeoutException' => true, + 'InternalError' => true, ]; private static array $standardTransientStatusCodes = [ @@ -90,10 +91,7 @@ public static function wrap(ConfigurationInterface $config, array $options): \Cl */ public static function createDefaultDecider(array $options = []): \Closure { - $retryCurlErrors = []; - if (extension_loaded('curl')) { - $retryCurlErrors[CURLE_RECV_ERROR] = true; - } + $retryCurlErrors = self::defaultRetryCurlErrors(); return function ( int $attempts, @@ -128,10 +126,7 @@ public function __construct( ? ($options['delayer'])(...) : null; - $this->retryCurlErrors = []; - if (extension_loaded('curl')) { - $this->retryCurlErrors[CURLE_RECV_ERROR] = true; - } + $this->retryCurlErrors = self::defaultRetryCurlErrors(); if (!empty($options['curl_errors']) && is_array($options['curl_errors'])) { foreach ($options['curl_errors'] as $code) { $this->retryCurlErrors[$code] = true; @@ -143,6 +138,25 @@ public function __construct( } } + private static function defaultRetryCurlErrors(): array + { + $errors = []; + if (!extension_loaded('curl')) { + return $errors; + } + foreach ([ + 'CURLE_RECV_ERROR', + 'CURLE_PARTIAL_FILE', + 'CURLE_GOT_NOTHING', + 'CURLE_HTTP2_STREAM', + ] as $name) { + if (defined($name)) { + $errors[constant($name)] = true; + } + } + return $errors; + } + public function __invoke(CommandInterface $cmd, RequestInterface $req): PromiseInterface { $handler = $this->nextHandler; @@ -152,7 +166,10 @@ public function __invoke(CommandInterface $cmd, RequestInterface $req): PromiseI $requestStats = []; $capacityUsed = null; - $req = $this->addRetryHeader($req, 0, 0); + $maxAttemptsForHeader = ($cmd['@retries'] !== null) + ? $cmd['@retries'] + 1 + : $this->maxAttempts; + $req = $this->addRetryHeaderV3($req, 1, $maxAttemptsForHeader); $callback = function ($value) use ( $handler, @@ -247,10 +264,8 @@ public function __invoke(CommandInterface $cmd, RequestInterface $req): PromiseI $acquired = $this->quotaManager->acquireRetryQuota($isThrottling); if ($acquired === false) { - // Long-polling: sleep and surface the error rather than retry. if (LongPolling::isLongPolling($this->service, $cmd->getName())) { $cmd['@http']['delay'] = $delayByMs; - usleep((int) ($delayByMs * 1000)); } if ($isError) { @@ -271,21 +286,27 @@ public function __invoke(CommandInterface $cmd, RequestInterface $req): PromiseI $attempts++; $cmd['@http']['delay'] = $delayByMs; + if ($this->mode === 'adaptive') { + $rlDelayMs = $this->rateLimiter->getSendDelayMs(); + if ($rlDelayMs > 0) { + $cmd['@http']['delay'] = ($cmd['@http']['delay'] ?? 0) + $rlDelayMs; + } + } + if ($this->collectStats) { $this->updateStats($attempts - 1, $delayByMs, $requestStats); } - $req = $this->addRetryHeader($req, $attempts - 1, $delayByMs); - - if ($this->mode === 'adaptive') { - $this->rateLimiter->getSendToken(); - } + $req = $this->addRetryHeaderV3($req, $attempts, $maxAttempts); return $handler($cmd, $req)->then($callback, $callback); }; if ($this->mode === 'adaptive') { - $this->rateLimiter->getSendToken(); + $rlDelayMs = $this->rateLimiter->getSendDelayMs(); + if ($rlDelayMs > 0) { + $cmd['@http']['delay'] = ($cmd['@http']['delay'] ?? 0) + $rlDelayMs; + } } return $handler($cmd, $req)->then($callback, $callback); diff --git a/src/S3/AmbiguousSuccessParser.php b/src/S3/AmbiguousSuccessParser.php index 18d3a174dc..3858f54e33 100644 --- a/src/S3/AmbiguousSuccessParser.php +++ b/src/S3/AmbiguousSuccessParser.php @@ -60,7 +60,11 @@ public function __invoke( throw new $this->exceptionClass( $parsed['message'], $command, - ['connection_error' => true] + [ + 'connection_error' => true, + 'code' => $parsed['code'], + 'message' => $parsed['message'], + ] ); } } diff --git a/src/Signature/SignatureV4.php b/src/Signature/SignatureV4.php index 150d1ffcb1..dc5f930b8a 100644 --- a/src/Signature/SignatureV4.php +++ b/src/Signature/SignatureV4.php @@ -68,6 +68,8 @@ protected function getHeaderBlacklist() 'x-amzn-trace-id' => true, 'aws-sdk-invocation-id' => true, 'aws-sdk-retry' => true, + 'amz-sdk-invocation-id' => true, + 'amz-sdk-request' => true, ]; } @@ -496,6 +498,8 @@ private function removeIllegalV4aHeaders(&$request) self::AMZ_CONTENT_SHA256_HEADER, 'aws-sdk-invocation-id', 'aws-sdk-retry', + 'amz-sdk-invocation-id', + 'amz-sdk-request', 'x-amz-region-set', 'transfer-encoding', ]; diff --git a/tests/Retry/RateLimiterTest.php b/tests/Retry/RateLimiterTest.php index b7fdf7226a..e46ac34b04 100644 --- a/tests/Retry/RateLimiterTest.php +++ b/tests/Retry/RateLimiterTest.php @@ -230,4 +230,74 @@ public function testUpdatesClientSendingRatesCorrectly() ); } } + + public function testGetSendDelayMsReturnsZeroWhenDisabled() + { + $rateLimiter = new RateLimiter(); + $this->assertSame(0, $rateLimiter->getSendDelayMs()); + $this->assertSame(0, $rateLimiter->getSendDelayMs()); + } + + public function testGetSendDelayMsReturnsZeroWhenTokensAvailable() + { + $now = 0.0; + $rateLimiter = new RateLimiter([ + 'time_provider' => function () use (&$now) { + return $now; + }, + ]); + + // Enable the bucket via the public API; with an initial measuredTxRate + // of 0, this settles at fillRate = minFillRate (0.5) and + // maxCapacity = minCapacity (1), with currentCapacity drained to 0. + $rateLimiter->updateSendingRate(true); + + // Advance the clock enough to refill one token (>= 1 / 0.5 = 2 s). + $now = 3.0; + + $this->assertSame(0, $rateLimiter->getSendDelayMs()); + // A second call at the same virtual time drains the bucket back to + // empty, so it must now return a non-zero delay — proving the debit + // from the first call landed. + $this->assertGreaterThan(0, $rateLimiter->getSendDelayMs()); + } + + public function testGetSendDelayMsReflectsFillRateDeficit() + { + $now = 0.0; + $rateLimiter = new RateLimiter([ + 'time_provider' => function () use (&$now) { + return $now; + }, + ]); + + // Enabling the bucket at t=0 with measuredTxRate=0 leaves it at + // fillRate=0.5 and currentCapacity=0. + $rateLimiter->updateSendingRate(true); + + // fillRate = 0.5 tokens/s → 2000 ms to accrue 1 token. + $this->assertSame(2000, $rateLimiter->getSendDelayMs()); + } + + public function testGetSendDelayMsDoesNotBlock() + { + $now = 0.0; + $rateLimiter = new RateLimiter([ + 'time_provider' => function () use (&$now) { + return $now; + }, + ]); + $rateLimiter->updateSendingRate(true); + + $start = microtime(true); + $delayMs = $rateLimiter->getSendDelayMs(); + $elapsed = microtime(true) - $start; + + $this->assertGreaterThan(0, $delayMs, 'Expected a non-zero delay hint'); + $this->assertLessThan( + 0.05, + $elapsed, + 'getSendDelayMs() blocked the PHP thread instead of returning a delay hint' + ); + } } diff --git a/tests/Retry/V3/RetryMiddlewareTest.php b/tests/Retry/V3/RetryMiddlewareTest.php index 77dc3fd45c..4421135d99 100644 --- a/tests/Retry/V3/RetryMiddlewareTest.php +++ b/tests/Retry/V3/RetryMiddlewareTest.php @@ -640,25 +640,9 @@ public function testRetriesForAdapativeMode() ] ); - $time = microtime(true); - $attempt = 0; - $expectedTimes = [0, 0, 0, 1.9, 3.8, 5.7]; - - // Errors within MockHandler closure get caught silently - $errors = []; - - $assertFunction = function() use (&$time, &$attempt, &$errors, $expectedTimes) { - try { - $this->assertLessThanOrEqual( - 0.5, - abs($expectedTimes[$attempt] - (microtime(true) - $time)) - ); - } catch (\Exception $e) { - $errors[] = $e; - } - - $time = microtime(true); - $attempt++; + $observedDelays = []; + $recorder = function () use (&$observedDelays, $command) { + $observedDelays[] = $command['@http']['delay'] ?? null; }; $mock = new MockHandler( @@ -670,12 +654,13 @@ public function testRetriesForAdapativeMode() $throttlingErrorShapeException, $result200, ], - $assertFunction, - $assertFunction + $recorder, + $recorder ); $times = [0, 0]; - foreach ($expectedTimes as $index => $expected) { + $expectedAttempts = 6; + for ($index = 0; $index < $expectedAttempts; $index++) { for ($i = 0; $i < 5; $i++) { $times[] = 0.1 * ($index + 1); } @@ -696,24 +681,42 @@ public function testRetriesForAdapativeMode() return $times[$i]; } ]), - 'throttling_error_codes' => ['CustomThrottlingException'] + 'throttling_error_codes' => ['CustomThrottlingException'], + 'exponential_base' => 1, ] ); $wrapped($command, $request)->wait(); - // Throw first silently caught error if any - if (!empty($errors)) { - throw $errors[0]; - } + $this->assertCount($expectedAttempts, $observedDelays); + $this->assertCount(0, $mock, 'Not all responses were consumed'); - $this->assertCount($attempt, $expectedTimes); + $this->assertSame( + null, + $observedDelays[0] === 0 ? null : $observedDelays[0], + 'First attempt should not carry a retry backoff' + ); + + // Non-throttling exceptions: base_delay * 2^attemptIndex ms. + $this->assertGreaterThanOrEqual(50, $observedDelays[1]); + $this->assertLessThanOrEqual(50, $observedDelays[1]); + $this->assertGreaterThanOrEqual(100, $observedDelays[2]); + $this->assertLessThanOrEqual(100, $observedDelays[2]); + + // Throttling: base 1000 ms with exp-backoff, plus rate-limiter contribution. + $this->assertGreaterThanOrEqual(4000, $observedDelays[3]); + $this->assertGreaterThanOrEqual(8000, $observedDelays[4]); + $this->assertGreaterThanOrEqual(16000, $observedDelays[5]); } public function testAddRetryHeader() { - $nextHandler = function (CommandInterface $command, RequestInterface $request) { - $this->assertTrue($request->hasHeader('aws-sdk-retry')); + $observedHeaders = []; + $nextHandler = function (CommandInterface $command, RequestInterface $request) + use (&$observedHeaders) + { + $this->assertTrue($request->hasHeader('amz-sdk-request')); + $observedHeaders[] = $request->getHeaderLine('amz-sdk-request'); return new RejectedPromise( new AwsException('e', $command, ['connection_error' => true]) ); @@ -730,6 +733,10 @@ public function testAddRetryHeader() $retryMW(new Command('SomeCommand'), new Request('GET', ''))->wait(); $this->fail(); } catch (AwsException $e) { } + + $this->assertSame('attempt=1; max=5', $observedHeaders[0]); + $this->assertSame('attempt=2; max=5', $observedHeaders[1]); + $this->assertSame('attempt=5; max=5', end($observedHeaders)); } public function testDeciderRetriesWhenStatusCodeMatches() @@ -795,6 +802,59 @@ public function testDeciderRetriesWhenCurlErrorCodeMatches() $this->assertTrue($decider(0, $command, $err)); } + public function testDeciderRetriesForPartialReadCurlErrors() + { + if (!extension_loaded('curl')) { + $this->markTestSkipped('Test skipped on no cURL extension'); + } + $decider = RetryMiddleware::createDefaultDecider(); + $command = new Command('foo'); + $request = new Request('GET', 'http://www.example.com'); + + foreach ([ + 'CURLE_PARTIAL_FILE', + 'CURLE_GOT_NOTHING', + 'CURLE_RECV_ERROR', + 'CURLE_HTTP2_STREAM', + ] as $constName) { + if (!defined($constName)) { + continue; + } + $previous = new RequestException( + 'test', + $request, + null, + null, + ['errno' => constant($constName)] + ); + $err = new AwsException( + 'e', + $command, + ['connection_error' => false], + $previous + ); + $this->assertTrue( + $decider(0, $command, $err), + "Decider should retry on cURL errno {$constName}" + ); + } + } + + public function testDeciderRetriesInternalErrorCode() + { + $decider = RetryMiddleware::createDefaultDecider(); + $command = new Command('foo'); + $err = new AwsException( + 'We encountered an internal error. Please try again.', + $command, + [ + 'response' => new Response(200), + 'code' => 'InternalError', + ] + ); + $this->assertTrue($decider(0, $command, $err)); + } + public function testDeciderRetriesForCustomCurlErrors() { if (!extension_loaded('curl')) {