Re-throw RedisCache::increment errors instead of returning 0

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
 

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;

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());
     }
 }