Skip to content

Add circuit breaker support to RedisPool #117

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

Open
premtsd-code wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add circuit breaker support to RedisPool #117

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
1 change: 1 addition & 0 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
"ext-curl": "*",
"ext-redis": "*",
"utopia-php/database": "^6.0.0",
"utopia-php/circuit-breaker": "0.3.*",
"utopia-php/pools": "1.*",
"appwrite/appwrite": "^26.0"
},
Expand Down
2 changes: 1 addition & 1 deletion composer.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

38 changes: 29 additions & 9 deletions src/Abuse/Adapters/TimeLimit/RedisPool.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use RuntimeException;
use Throwable;
use Utopia\Abuse\Adapters\TimeLimit;
use Utopia\CircuitBreaker\CircuitBreaker;
use Utopia\Pools\Pool as UtopiaPool;

class RedisPool extends TimeLimit
Expand All @@ -21,7 +22,8 @@ public function __construct(
string $key,
int $limit,
int $seconds,
protected UtopiaPool $pool
protected UtopiaPool $pool,
protected ?CircuitBreaker $breaker = null
) {
$this->key = $key;
$this->ttl = $seconds;
Expand All @@ -30,6 +32,24 @@ public function __construct(
$this->limit = $limit;
}

/**
* @template T
* @param callable(\Redis|\RedisCluster): T $operation
* @param T $fallback
* @return T
*/
private function guard(callable $operation, mixed $fallback): mixed
{
if ($this->breaker === null) {
return $this->pool->use($operation);
}

return $this->breaker->call(
open: fn (): mixed => $fallback,
close: fn (): mixed => $this->pool->use($operation),
);
}

protected function count(string $key, int $timestamp): int
{
if (0 == $this->limit) {
Expand All @@ -41,11 +61,11 @@ protected function count(string $key, int $timestamp): int
}

/** @var int $count */
$count = $this->pool->use(function (\Redis|\RedisCluster $redis) use ($key, $timestamp): int {
$count = $this->guard(function (\Redis|\RedisCluster $redis) use ($key, $timestamp): int {
$count = $redis->get(Redis::NAMESPACE . '__' . $key . '__' . $timestamp);

return \is_numeric($count) ? (int) $count : 0;
});
}, 0);

$this->count = $count;

Expand All @@ -61,7 +81,7 @@ protected function hit(string $key, int $timestamp): void
$ttl = $this->ttl;
$key = Redis::NAMESPACE . '__' . $key . '__' . $timestamp;

$this->pool->use(function (\Redis|\RedisCluster $redis) use ($key, $ttl): void {
$this->guard(function (\Redis|\RedisCluster $redis) use ($key, $ttl): void {
$redis->multi();
try {
$redis->incr($key);
Expand All @@ -76,7 +96,7 @@ protected function hit(string $key, int $timestamp): void
$this->discard($redis);
throw new RuntimeException('Redis transaction failed.');
}
});
}, null);

$this->count = ($this->count ?? 0) + 1;
Comment on lines 84 to 101

@greptile-apps greptile-apps Bot Jul 7, 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.

P2 Count incremented unconditionally when circuit-open write is skipped

When the circuit is open, guard() returns null without writing to Redis, but $this->count = ($this->count ?? 0) + 1 still runs on line 101. Within the same request this inflates the cached count to 1 (matching the limit), which would make a second check() call on the same adapter instance report abuse (count >= limit) even though nothing was recorded in Redis. This is a latent inconsistency: if callers reuse the same adapter for more than one check per request cycle, the second call will always report abuse when the circuit is open, regardless of actual Redis state.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/Abuse/Adapters/TimeLimit/RedisPool.php
Line: 84-101

Comment:
**Count incremented unconditionally when circuit-open write is skipped**

When the circuit is open, `guard()` returns `null` without writing to Redis, but `$this->count = ($this->count ?? 0) + 1` still runs on line 101. Within the same request this inflates the cached count to 1 (matching the limit), which would make a second `check()` call on the same adapter instance report abuse (`count >= limit`) even though nothing was recorded in Redis. This is a latent inconsistency: if callers reuse the same adapter for more than one check per request cycle, the second call will always report abuse when the circuit is open, regardless of actual Redis state.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code Fix in Codex

}
Expand All @@ -86,7 +106,7 @@ protected function set(string $key, int $timestamp, int $value): void
$ttl = $this->ttl;
$key = Redis::NAMESPACE . '__' . $key . '__' . $timestamp;

$this->pool->use(function (\Redis|\RedisCluster $redis) use ($key, $ttl, $value): void {
$this->guard(function (\Redis|\RedisCluster $redis) use ($key, $ttl, $value): void {
$redis->multi();
try {
$redis->set($key, (string) $value);
Expand All @@ -101,7 +121,7 @@ protected function set(string $key, int $timestamp, int $value): void
$this->discard($redis);
throw new RuntimeException('Redis transaction failed.');
}
});
}, null);

$this->count = $value;
}
Expand All @@ -121,7 +141,7 @@ public function getLogs(?int $offset = null, ?int $limit = 25): array
$limit = $limit ?? 25;

/** @var array<string, mixed> $result */
$result = $this->pool->use(function (\Redis|\RedisCluster $redis) use ($offset, $limit): array {
$result = $this->guard(function (\Redis|\RedisCluster $redis) use ($offset, $limit): array {
if ($redis instanceof \RedisCluster) {
return $this->getRedisClusterLogs($redis, $offset, $limit);
}
Expand Down Expand Up @@ -150,7 +170,7 @@ public function getLogs(?int $offset = null, ?int $limit = 25): array
}

return $logs;
});
}, []);

return $result;
}
Expand Down
37 changes: 37 additions & 0 deletions tests/Abuse/RedisPoolCircuitBreakerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<?php

namespace Utopia\Tests;

use PHPUnit\Framework\TestCase;
use Utopia\Abuse\Abuse;
use Utopia\Abuse\Adapters\TimeLimit\RedisPool;
use Utopia\CircuitBreaker\CircuitBreaker;
use Utopia\Pools\Adapter\Stack;
use Utopia\Pools\Pool;

class RedisPoolCircuitBreakerTest extends TestCase
{
public function testCircuitBreakerFailsOpenWhenPoolUnavailable(): void
{
$pool = new Pool(new Stack(), 'abuse-redis-unavailable', 1, function (): \Redis {
throw new \Exception('Redis unavailable');
});
$pool
->setReconnectAttempts(1)
->setRetryAttempts(1);

/** @var Pool<\Redis> $pool */
$adapter = new RedisPool(
'redis-pool-unavailable',
1,
60,
$pool,
new CircuitBreaker()
);

$abuse = new Abuse($adapter);

Comment on lines +29 to +33

@greptile-apps greptile-apps Bot Jul 7, 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 Test uses a breaker that won't open in time

new CircuitBreaker() defaults to threshold: 3, meaning the circuit stays CLOSED for the first two failures and re-throws the exception on each. $abuse->check() triggers exactly one guard() call (via count()), so the pool throws once, the circuit records one failure (1 < 3), and the exception propagates through count() → check() → the test instead of returning false. The assertion is never reached.

The test should construct the breaker with threshold: 1 so the circuit opens on the very first failed pool call and the fallback (0) is returned immediately.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: tests/Abuse/RedisPoolCircuitBreakerTest.php
Line: 29-33

Comment:
**Test uses a breaker that won't open in time**

`new CircuitBreaker()` defaults to `threshold: 3`, meaning the circuit stays CLOSED for the first two failures and re-throws the exception on each. `$abuse->check()` triggers exactly one `guard()` call (via `count()`), so the pool throws once, the circuit records one failure (1 < 3), and the exception propagates through `count()` → `check()` → the test instead of returning `false`. The assertion is never reached.

The test should construct the breaker with `threshold: 1` so the circuit opens on the very first failed pool call and the fallback (`0`) is returned immediately.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code Fix in Codex

$this->assertFalse($abuse->check());
$this->assertSame([], $adapter->getLogs());
}
}
Loading