Skip to content

fix(s3): retry refused connections and unresolvable hosts #170

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
HarshMN2345 wants to merge 1 commit into from
Closed

fix(s3): retry refused connections and unresolvable hosts #170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/Storage/Device/S3/RetryStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;
use Utopia\Client\Decorator\Retry\Strategy;
use Utopia\Client\Exception\ConnectionException;
use Utopia\Client\Exception\DnsException;

/**
* Retry strategy for transient S3 rate-limiting errors (e.g. SlowDown,
Expand All @@ -19,6 +21,10 @@
* error code is not retried, while unparseable 429/503 responses fall back to
* status-code detection.
*
* A refused connection or an unresolvable host is retried too: the request
* never reached the service, so replaying it cannot duplicate an effect. Other
* transport failures are left alone, since the request may have been applied.
*
* Waits use exponential backoff with full jitter so a fleet throttled at the
* same moment does not retry in lockstep.
* @see \Utopia\Tests\Storage\Device\S3\RetryStrategyTest
Expand Down Expand Up @@ -54,11 +60,15 @@ public function __construct(

public function delay(RequestInterface $request, int $attempt, ?ResponseInterface $response, ?ClientExceptionInterface $error): ?float
{
if ($attempt > $this->retries || ! $response instanceof ResponseInterface) {
if ($attempt > $this->retries) {
return null;
}

if (! $this->isTransient($response)) {
$retryable = $response instanceof ResponseInterface
? $this->isTransient($response)

@greptile-apps greptile-apps Bot Aug 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Retries include delivered requests

When a mutating S3 request fails during sending or response reception, the client reports several such failures as ConnectionException, and this branch retries the request even though the service may already have applied it. Replaying a non-idempotent request such as multipart-upload creation can create a second orphaned upload with associated storage costs.

Knowledge Base Used: S3 transport resilience

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/Storage/Device/S3/RetryStrategy.php
Line: 68

Comment:
**Retries include delivered requests**

When a mutating S3 request fails during sending or response reception, the client reports several such failures as `ConnectionException`, and this branch retries the request even though the service may already have applied it. Replaying a non-idempotent request such as multipart-upload creation can create a second orphaned upload with associated storage costs.

**Knowledge Base Used:** [S3 transport resilience](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/storage/-/docs/s3-transport-resilience.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Codex

: $error instanceof ConnectionException || $error instanceof DnsException;

if (! $retryable) {
return null;
}

Expand Down
27 changes: 27 additions & 0 deletions tests/Storage/Device/S3/RetryStrategyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
namespace Utopia\Tests\Storage\Device\S3;

use PHPUnit\Framework\TestCase;
use Utopia\Client\Exception\ConnectionException;
use Utopia\Client\Exception\DnsException;
use Utopia\Client\Exception\NetworkException;
use Utopia\Psr7\Request;
use Utopia\Psr7\Response;
Expand Down Expand Up @@ -79,13 +81,38 @@ public function testRetriesAreCapped(): void
$this->assertNull($strategy->delay($this->request(), 3, $response, null));
}

/** A reset connection may have been applied server-side, so it stays non-retryable. */
public function testTransportErrorsAreNotRetried(): void
{
$error = new NetworkException($this->request(), 'Connection reset');

$this->assertNull(new RetryStrategy()->delay($this->request(), 1, null, $error));
}

public function testRefusedConnectionIsRetried(): void
{
$error = new ConnectionException($this->request(), 'Could not connect to server');
$strategy = new RetryStrategy(delay: 0.5, randomizer: static fn(): float => 1.0);

$this->assertEqualsWithDelta(0.5, $strategy->delay($this->request(), 1, null, $error), PHP_FLOAT_EPSILON);
}

public function testUnresolvableHostIsRetried(): void
{
$error = new DnsException($this->request(), 'Could not resolve host');
$strategy = new RetryStrategy(delay: 0.5, randomizer: static fn(): float => 1.0);

$this->assertEqualsWithDelta(0.5, $strategy->delay($this->request(), 1, null, $error), PHP_FLOAT_EPSILON);
}

public function testConnectionRetriesAreCapped(): void
{
$error = new ConnectionException($this->request(), 'Could not connect to server');
$strategy = new RetryStrategy(retries: 2, delay: 0.5, randomizer: static fn(): float => 1.0);

$this->assertNull($strategy->delay($this->request(), 3, null, $error));
}

public function testReadingTheBodyLeavesItReadable(): void
{
$body = '<?xml version="1.0" encoding="UTF-8"?><Error><Code>SlowDown</Code></Error>';
Expand Down