fix(cache): fixed lock exception

smarcet · smarcet · commit 4879a4a0d88c · 2026-04-06T21:14:15.000-03:00
diff --git a/app/Http/Middleware/CacheMiddleware.php b/app/Http/Middleware/CacheMiddleware.php
@@ -13,6 +13,7 @@
  **/
 
 use Closure;
+use Illuminate\Contracts\Cache\LockTimeoutException;
 use Illuminate\Http\JsonResponse;
 use Illuminate\Support\Facades\Cache;
 use Illuminate\Support\Facades\Log;
@@ -112,11 +113,7 @@ public function handle($request, Closure $next, $cache_lifetime, $cache_region =
 
         if ($encoded !== null) {
             Log::debug("CacheMiddleware: cache HIT", $logCtx);
-
-            $data = $this->decode($encoded);
-            if ($data === null) $data = is_array($encoded) ? $encoded : [];
-
-            $response = new JsonResponse($data, 200, ['Content-Type' => 'application/json']);
+            $response = $this->buildCachedResponse($encoded);
             $wasHit = true;
         } else {
             // Phase 2: cache miss — acquire lock so only one request executes the handler
@@ -125,65 +122,65 @@ public function handle($request, Closure $next, $cache_lifetime, $cache_region =
             $wasHit = false;
 
             try {
-                if ($lock->block(self::LOCK_WAIT)) {
-                    // Won the lock — double-check: another request may have populated the cache
-                    $encoded = $cache->get($key);
-
-                    if ($encoded !== null) {
-                        Log::debug("CacheMiddleware: cache HIT (after lock)", $logCtx);
-
-                        $data = $this->decode($encoded);
-                        if ($data === null) $data = is_array($encoded) ? $encoded : [];
-
-                        $response = new JsonResponse($data, 200, ['Content-Type' => 'application/json']);
-                        $wasHit = true;
-                    } else {
-                        Log::debug("CacheMiddleware: cache MISS (executing handler)", $logCtx);
-
-                        $resp = $next($request);
+                $lock->block(self::LOCK_WAIT);
 
-                        // Only cache 200 JSON responses; let everything else pass through as-is
-                        if ($resp instanceof JsonResponse && $resp->getStatusCode() === 200) {
-                            $cache->put($key, $this->encode($resp->getData(true)), $cache_lifetime);
-                        } else {
-                            return $resp;
-                        }
+                // Won the lock — double-check: another request may have populated the cache
+                $encoded = $cache->get($key);
 
-                        $response = $resp;
-                    }
+                if ($encoded !== null) {
+                    Log::debug("CacheMiddleware: cache HIT (after lock)", $logCtx);
+                    $response = $this->buildCachedResponse($encoded);
+                    $wasHit = true;
                 } else {
-                    // Could not acquire lock within LOCK_WAIT seconds — fall through without lock
-                    Log::warning("CacheMiddleware: lock timeout, executing handler without lock", $logCtx);
-
-                    $resp = $next($request);
-
-                    if ($resp instanceof JsonResponse && $resp->getStatusCode() === 200) {
-                        $cache->put($key, $this->encode($resp->getData(true)), $cache_lifetime);
-                    } else {
-                        return $resp;
-                    }
-
-                    $response = $resp;
+                    Log::debug("CacheMiddleware: cache MISS (executing handler)", $logCtx);
+                    $response = $this->executeAndCache($next, $request, $cache, $key, $cache_lifetime);
                 }
+            } catch (LockTimeoutException $e) {
+                Log::warning("CacheMiddleware: lock timeout, executing handler without lock", $logCtx);
+                $response = $this->executeAndCache($next, $request, $cache, $key, $cache_lifetime);
             } finally {
                 $lock->release();
             }
         }
 
-        // Mark for revalidation so ETag middleware can return 304 when unchanged
-        $response->setPublic();
-        $response->setMaxAge(0);
-        $response->headers->addCacheControlDirective('must-revalidate', true);
-        $response->headers->addCacheControlDirective('proxy-revalidate', true);
-        $response->headers->add([
-            'X-Cache-Result' => $wasHit ? 'HIT' : 'MISS',
-        ]);
+        // Cache headers only for cacheable (200 JSON) responses
+        if ($response instanceof JsonResponse && $response->getStatusCode() === 200) {
+            $response->setPublic();
+            $response->setMaxAge(0);
+            $response->headers->addCacheControlDirective('must-revalidate', true);
+            $response->headers->addCacheControlDirective('proxy-revalidate', true);
+            $response->headers->add([
+                'X-Cache-Result' => $wasHit ? 'HIT' : 'MISS',
+            ]);
+        }
 
         Log::debug("CacheMiddleware: returning response", $logCtx);
 
         return $response;
     }
 
+    /**
+     * Decode a cached value and wrap it in a JsonResponse.
+     */
+    private function buildCachedResponse($encoded): JsonResponse
+    {
+        $data = $this->decode($encoded);
+        if ($data === null) $data = is_array($encoded) ? $encoded : [];
+        return new JsonResponse($data, 200, ['Content-Type' => 'application/json']);
+    }
+
+    /**
+     * Execute the next handler and cache the response if it is a 200 JSON response.
+     */
+    private function executeAndCache(Closure $next, $request, $cache, string $key, int $cache_lifetime)
+    {
+        $resp = $next($request);
+        if ($resp instanceof JsonResponse && $resp->getStatusCode() === 200) {
+            $cache->put($key, $this->encode($resp->getData(true)), $cache_lifetime);
+        }
+        return $resp;
+    }
+
     /**
      * Build a cache key based on path + sorted query params
      */
diff --git a/tests/Unit/CacheMiddlewareTest.php b/tests/Unit/CacheMiddlewareTest.php
@@ -0,0 +1,105 @@
+<?php namespace Tests\Unit;
+/**
+ * Copyright 2026 OpenStack Foundation
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+
+use App\Http\Middleware\CacheMiddleware;
+use Illuminate\Contracts\Cache\LockTimeoutException;
+use Illuminate\Http\JsonResponse;
+use Illuminate\Http\Request;
+use Illuminate\Support\Facades\Cache;
+use Illuminate\Support\Facades\Log;
+use models\oauth2\IResourceServerContext;
+use Tests\TestCase;
+
+/**
+ * Class CacheMiddlewareTest
+ * Tests for CacheMiddleware lock timeout handling
+ */
+final class CacheMiddlewareTest extends TestCase
+{
+    /**
+     * Test that LockTimeoutException is caught and handled gracefully
+     * When block() throws LockTimeoutException, the middleware should:
+     * 1. Log a WARNING (not propagate the exception as ERROR)
+     * 2. Execute the handler without the lock
+     * 3. Return a valid response
+     *
+     * @return void
+     */
+    public function test_lock_timeout_exception_handled_gracefully()
+    {
+        // Arrange: Create a GET request
+        $request = Request::create('/api/v1/summits/123', 'GET');
+
+        // Mock the resource server context (no user for this test)
+        $context = $this->createMock(IResourceServerContext::class);
+        $context->method('getCurrentUser')->willReturn(null);
+
+        // Create the middleware instance
+        $middleware = new CacheMiddleware($context);
+
+        // Mock the next handler to return a JSON response
+        $expectedData = ['data' => 'test_response'];
+        $next = function ($req) use ($expectedData) {
+            return new JsonResponse($expectedData, 200);
+        };
+
+        // Mock Cache facade behavior
+        $mockCache = $this->createMock(\Illuminate\Contracts\Cache\Repository::class);
+        $mockLock = $this->createMock(\Illuminate\Contracts\Cache\Lock::class);
+
+        // First get() returns null (cache miss)
+        $mockCache->expects($this->once())
+            ->method('get')
+            ->willReturn(null);
+
+        // Lock is created
+        $mockCache->expects($this->once())
+            ->method('lock')
+            ->willReturn($mockLock);
+
+        // block() throws LockTimeoutException (simulating timeout)
+        $mockLock->expects($this->once())
+            ->method('block')
+            ->willThrowException(new LockTimeoutException());
+
+        // Lock release is attempted in finally block
+        $mockLock->expects($this->once())
+            ->method('release');
+
+        // Cache put should be called (caching the 200 response)
+        $mockCache->expects($this->once())
+            ->method('put');
+
+        Cache::shouldReceive('store')
+            ->once()
+            ->andReturn($mockCache);
+
+        // Expect WARNING log (not ERROR)
+        Log::shouldReceive('warning')
+            ->once()
+            ->with('CacheMiddleware: lock timeout, executing handler without lock', $this->anything());
+
+        Log::shouldReceive('debug')
+            ->zeroOrMoreTimes();
+
+        // Act: Handle the request through the middleware
+        $response = $middleware->handle($request, $next, 60);
+
+        // Assert: Response is valid, not a 500 error
+        $this->assertInstanceOf(JsonResponse::class, $response);
+        $this->assertEquals(200, $response->getStatusCode());
+        $this->assertEquals($expectedData, $response->getData(true));
+        $this->assertEquals('MISS', $response->headers->get('X-Cache-Result'));
+    }
+}
