diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c595a8..e59f501 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - **`BanManager` registry has a TTL and prunes expired entries on save** — Previously the per-rule ban registry (the audit-view list backing `listBans()` / `listRulesWithBans()`) was written without a TTL and grew monotonically across ban churn. Each save now applies a TTL equal to the longest-surviving entry's remaining lifetime, and `saveRegistry()` prunes already-expired entries before writing so the registry tracks live bans only. The registry is best-effort under concurrent `ban()` calls for the same rule (the primary ban cache key set by `ban()` is the source of truth and is not affected). +- **`RedisCache::increment()` re-throws on Redis errors instead of returning `0`** — Previously any `\Throwable` from the underlying Predis client (connection refused, AUTH failure, Lua script error, network blip) was swallowed and `increment()` returned `0`, which every throttle / fail2ban / allow2ban rule then interpreted as "no hits this window". The method now re-throws the original `\Throwable` after emitting an `E_USER_WARNING` for diagnostic visibility, so `Middleware::process()` applies the configured `Config::failOpen` policy uniformly for Redis errors. Integrators relying on the prior fail-open behaviour can keep `Config::failOpen` at its current default of `true`; those wanting the failure to surface as a 5xx can set it to `false`. ## 0.4.0 - 2026-05-19 diff --git a/src/Store/RedisCache.php b/src/Store/RedisCache.php index 7b4b6ec..5cd25ce 100644 --- a/src/Store/RedisCache.php +++ b/src/Store/RedisCache.php @@ -127,6 +127,16 @@ public function has(string $key): bool /** * Atomic fixed-window increment aligned to the end of the current window. * Returns the new counter value. + * + * Errors from the underlying Redis client (connection refused, AUTH failure, + * Lua script error, etc.) are re-thrown so that + * {@see \Flowd\Phirewall\Middleware::process()} can apply the configured + * fail-open / fail-closed policy. A diagnostic `E_USER_WARNING` is emitted + * via {@see trigger_error()} before the throw so the failure remains + * observable even when callers later suppress the exception. + * + * @throws \Throwable Re-thrown from the Predis client on infrastructure + * errors. Concrete type depends on the Predis backend. */ public function increment(string $key, int $period): int { @@ -149,16 +159,22 @@ public function increment(string $key, int $period): int try { $counter = $this->client->eval($script, 1, $namespacedKey, (string)$windowEnd); } catch (\Throwable $throwable) { + // Diagnostic visibility: emit a warning so operators see infrastructure + // failures even when an upstream catch converts the exception. Some + // frameworks install error handlers that re-throw on E_USER_WARNING; + // the inner try/catch swallows that upgrade so the original Redis + // exception is the one that ultimately surfaces. try { trigger_error( sprintf('RedisCache::increment() failed for key "%s": %s', $key, $throwable->getMessage()), E_USER_WARNING, ); } catch (\Throwable) { - // Preserve fail-open behavior even when warnings are converted to exceptions. + // Diagnostic warning was upgraded to an exception by a hostile + // error handler; ignore and surface the underlying Redis error. } - return 0; + throw $throwable; } return is_scalar($counter) ? (int) $counter : 0; diff --git a/tests/Unit/Store/RedisCacheTest.php b/tests/Unit/Store/RedisCacheTest.php index ceab081..0c681b2 100644 --- a/tests/Unit/Store/RedisCacheTest.php +++ b/tests/Unit/Store/RedisCacheTest.php @@ -11,7 +11,7 @@ #[CoversClass(RedisCache::class)] final class RedisCacheTest extends TestCase { - public function testIncrementReturnsZeroAndTriggersWarningWhenEvalThrows(): void + public function testIncrementRethrowsAndTriggersWarningWhenEvalThrows(): void { if (!interface_exists(\Predis\ClientInterface::class)) { $this->markTestSkipped('Predis is not installed.'); @@ -34,21 +34,27 @@ public function testIncrementReturnsZeroAndTriggersWarningWhenEvalThrows(): void return false; }); + $caught = null; + try { - $result = $cache->increment('my-counter', 60); + $cache->increment('my-counter', 60); + } catch (\Throwable $throwable) { + $caught = $throwable; } finally { restore_error_handler(); } - $this->assertSame(0, $result, 'increment() must fail open and return 0 on Redis error'); - $this->assertCount(1, $capturedWarnings, 'Exactly one E_USER_WARNING should be triggered'); + $this->assertInstanceOf(\RuntimeException::class, $caught, 'increment() must re-throw the underlying Redis error'); + $this->assertSame('Connection refused', $caught->getMessage()); + + $this->assertCount(1, $capturedWarnings, 'Exactly one E_USER_WARNING should be emitted before the re-throw'); $this->assertSame(E_USER_WARNING, $capturedWarnings[0]['errno']); $this->assertStringContainsString('RedisCache::increment()', $capturedWarnings[0]['errstr']); $this->assertStringContainsString('my-counter', $capturedWarnings[0]['errstr']); $this->assertStringContainsString('Connection refused', $capturedWarnings[0]['errstr']); } - public function testIncrementFailsOpenWhenErrorHandlerThrows(): void + public function testIncrementRethrowsEvenWhenErrorHandlerRejectsTheWarning(): void { if (!interface_exists(\Predis\ClientInterface::class)) { $this->markTestSkipped('Predis is not installed.'); @@ -61,17 +67,24 @@ public function testIncrementFailsOpenWhenErrorHandlerThrows(): void $cache = new RedisCache($client); - // Simulate a framework error handler that converts warnings to exceptions + // Simulate a framework error handler that converts warnings to exceptions. + // The inner try/catch around trigger_error() in increment() must + // swallow that upgrade so the original Redis exception still surfaces. set_error_handler(static function (int $errno, string $errstr): bool { throw new \ErrorException($errstr, 0, $errno); }); + $caught = null; + try { - $result = $cache->increment('my-counter', 60); + $cache->increment('my-counter', 60); + } catch (\Throwable $throwable) { + $caught = $throwable; } finally { restore_error_handler(); } - $this->assertSame(0, $result, 'increment() must fail open even when error handler throws'); + $this->assertInstanceOf(\RuntimeException::class, $caught, 'increment() must surface the underlying Redis error even if the error handler is hostile'); + $this->assertSame('Connection refused', $caught->getMessage()); } }