Address code review feedback: status codes consistency, race condition, null handling, and tests

Co-authored-by: Thavarshan <10804999+Thavarshan@users.noreply.github.com>

Author: Copilot

src/Fetch/Cache/CacheControl.php

@@ -99,9 +99,10 @@ public function shouldCache(ResponseInterface $response, bool $isSharedCache = f
             return false;
         }
 
-        // Check response status code
+        // Check response status code - using RFC 7234 recommended cacheable status codes
+        // This list matches the default cache_status_codes in ManagesCache trait
         $status = $response->getStatusCode();
-        $cacheableStatuses = [200, 203, 204, 206, 300, 301, 404, 405, 410, 414, 501];
+        $cacheableStatuses = [200, 203, 204, 206, 300, 301, 404, 410];
 
         if (! in_array($status, $cacheableStatuses, true)) {
             return false;

src/Fetch/Cache/FileCache.php

@@ -53,11 +53,9 @@ public function get(string $key): ?CachedResponse
     {
         $path = $this->getPath($key);
 
-        if (! file_exists($path)) {
-            return null;
-        }
-
-        $contents = file_get_contents($path);
+        // Directly attempt to read the file - file_get_contents returns false if file doesn't exist
+        // This avoids a race condition where file could be deleted between exists check and read
+        $contents = @file_get_contents($path);
         if ($contents === false) {
             return null;
         }
@@ -108,29 +106,35 @@ public function set(string $key, CachedResponse $response, ?int $ttl = null): vo
         if ($ttl !== null) {
             if ($ttl === 0) {
                 // No expiration
-                $response = CachedResponse::fromArray(
+                $updatedResponse = CachedResponse::fromArray(
                     array_merge($response->toArray(), [
                         'expires_at' => null,
                     ])
                 );
+                if ($updatedResponse !== null) {
+                    $response = $updatedResponse;
+                }
             } else {
-                $response = CachedResponse::fromArray(
+                $updatedResponse = CachedResponse::fromArray(
                     array_merge($response->toArray(), [
                         'expires_at' => time() + $ttl,
                     ])
                 );
+                if ($updatedResponse !== null) {
+                    $response = $updatedResponse;
+                }
             }
         } elseif ($response->getExpiresAt() === null && $this->defaultTtl > 0) {
-            $response = CachedResponse::fromArray(
+            $updatedResponse = CachedResponse::fromArray(
                 array_merge($response->toArray(), [
                     'expires_at' => time() + $this->defaultTtl,
                 ])
             );
+            if ($updatedResponse !== null) {
+                $response = $updatedResponse;
+            }
         }
 
-        // PHPStan cannot infer that $response is non-null here after the conditionals above
-        // but we know $response is always a valid CachedResponse at this point
-        // @phpstan-ignore-next-line
         $encoded = json_encode($response->toArray());
         if ($encoded === false) {
             throw new RuntimeException('Failed to encode cache data as JSON');

src/Fetch/Concerns/ManagesCache.php

@@ -14,6 +14,9 @@
 
 /**
  * Trait for managing HTTP caching.
+ *
+ * Note: Caching is only supported for synchronous requests. Asynchronous requests
+ * bypass the cache entirely and do not check or store cached responses.
  */
 trait ManagesCache
 {

tests/Unit/ClientHandlerCacheTest.php

@@ -81,7 +81,7 @@ public function test_caches_get_requests(): void
         $this->assertEquals('HIT', $response2->getHeaderLine('X-Cache-Status'));
     }
 
-    public function test_cache_miss_adds_x_cache_status_header(): void
+    public function test_cache_miss_returns_successful_response(): void
     {
         $responses = [
             new GuzzleResponse(200, ['Content-Type' => 'application/json'], '{"data":"test"}'),
@@ -93,8 +93,9 @@ public function test_cache_miss_adds_x_cache_status_header(): void
 
         $response = $handler->get('/users');
 
-        // After request is cached, the response won't have the header until fetched from cache
+        // First request is a cache miss, response should be successful
         $this->assertEquals(200, $response->getStatusCode());
+        $this->assertEquals('{"data":"test"}', $response->body());
     }
 
     public function test_does_not_cache_post_requests_by_default(): void
@@ -286,7 +287,7 @@ public function test_cache_respects_max_age_from_response(): void
         $this->assertEquals('{"data":"cached"}', $response->body());
     }
 
-    public function test_cache_disabled_when_respect_headers_false_and_no_store(): void
+    public function test_cache_respects_no_store_when_respect_headers_enabled(): void
     {
         $responses = [
             new GuzzleResponse(200, [
@@ -298,7 +299,7 @@ public function test_cache_disabled_when_respect_headers_false_and_no_store(): v
 
         $handler = $this->create_handler_with_mock_responses($responses);
         $handler->baseUri('https://api.example.com');
-        // Even with respect_cache_headers=false, no-store should be respected
+        // With respect_cache_headers=true (default), no-store should be respected
         $handler->withCache(null, ['respect_cache_headers' => true]);
 
         $response1 = $handler->get('/users');
@@ -308,4 +309,33 @@ public function test_cache_disabled_when_respect_headers_false_and_no_store(): v
         $response2 = $handler->get('/users');
         $this->assertEquals('{"data":"second"}', $response2->body());
     }
+
+    public function test_cache_requires_revalidation_with_no_cache_directive(): void
+    {
+        // no-cache means the response may be cached but must be revalidated before use
+        // This is different from no-store which forbids caching entirely
+        $responses = [
+            new GuzzleResponse(200, [
+                'Content-Type' => 'application/json',
+                'Cache-Control' => 'no-cache',
+                'ETag' => '"abc123"',
+            ], '{"data":"first"}'),
+            // Second request should include conditional headers and may get 304
+            new GuzzleResponse(304, [], ''),
+        ];
+
+        $handler = $this->create_handler_with_mock_responses($responses);
+        $handler->baseUri('https://api.example.com');
+        $handler->withCache(null, ['respect_cache_headers' => true]);
+
+        // First request - gets response with no-cache
+        $response1 = $handler->get('/users');
+        $this->assertEquals('{"data":"first"}', $response1->body());
+        $this->assertEquals(200, $response1->getStatusCode());
+
+        // Second request - should revalidate and use cached response if 304
+        $response2 = $handler->get('/users');
+        // Response should still be the cached one after revalidation
+        $this->assertEquals(200, $response2->getStatusCode());
+    }
 }