Use atomic file writes for query cache (#2716)

Author: spawnia

.github/workflows/format.yml

@@ -14,9 +14,9 @@ jobs:
 
       - uses: shivammathur/setup-php@v2
         with:
+          php-version: 8.3
           coverage: none
           extensions: mbstring
-          php-version: 8.3
 
       - run: composer install --no-interaction --no-progress --no-suggest
 
@@ -51,9 +51,9 @@ jobs:
 
       - uses: shivammathur/setup-php@v2
         with:
+          php-version: 8.3
           coverage: none
           extensions: mbstring
-          php-version: 8.3
 
       - run: composer install --no-interaction --no-progress --no-suggest
 

.github/workflows/validate.yml

@@ -55,9 +55,9 @@ jobs:
 
       - uses: shivammathur/setup-php@v2
         with:
+          php-version: ${{ matrix.php-version }}
           coverage: none
           extensions: ${{ env.REQUIRED_PHP_EXTENSIONS }}
-          php-version: ${{ matrix.php-version }}
 
       - name: "Cache composer dependencies"
         uses: actions/cache@v3
@@ -140,9 +140,10 @@ jobs:
 
       - uses: shivammathur/setup-php@v2
         with:
+          php-version: ${{ matrix.php-version }}
           coverage: none
           extensions: ${{ env.REQUIRED_PHP_EXTENSIONS }}
-          php-version: ${{ matrix.php-version }}
+          ini-values: zend.assertions=1
 
       - name: "Cache composer dependencies"
         uses: actions/cache@v3
@@ -196,9 +197,10 @@ jobs:
 
       - uses: shivammathur/setup-php@v2
         with:
+          php-version: ${{ matrix.php-version }}
           coverage: pcov
           extensions: ${{ env.REQUIRED_PHP_EXTENSIONS }}
-          php-version: ${{ matrix.php-version }}
+          ini-values: zend.assertions=1
 
       - name: "Cache composer dependencies"
         uses: actions/cache@v3
@@ -228,8 +230,9 @@ jobs:
 
       - uses: shivammathur/setup-php@v2
         with:
-          extensions: ${{ env.REQUIRED_PHP_EXTENSIONS }}
           php-version: ${{ matrix.php-version }}
+          coverage: none
+          extensions: ${{ env.REQUIRED_PHP_EXTENSIONS }}
 
       - name: "Cache composer dependencies"
         uses: actions/cache@v3

CHANGELOG.md

@@ -9,6 +9,12 @@ You can find and compare releases at the [GitHub release page](https://github.co
 
 ## Unreleased
 
+## v6.63.1
+
+### Fixed
+
+- Use atomic file writes for query cache https://github.com/nuwave/lighthouse/pull/2716
+
 ## v6.63.0
 
 ### Added

src/Cache/CacheDirective.php

@@ -67,13 +67,13 @@ public function handleField(FieldValue $fieldValue): void
                 : $this->cacheRepository;
 
             $cacheKey = $this->cacheKeyAndTags->key(
-                $context->user(),
-                $isPrivate,
-                $parentName,
-                $rootID,
-                $fieldName,
-                $args,
-                $path,
+                user: $context->user(),
+                isPrivate: $isPrivate,
+                parentName: $parentName,
+                id: $rootID,
+                fieldName: $fieldName,
+                args: $args,
+                path: $path,
             );
 
             // We found a matching value in the cache, so we can return early without actually running the query.

src/Cache/QueryCache.php

@@ -9,6 +9,7 @@
 use Illuminate\Contracts\Cache\Factory as CacheFactory;
 use Illuminate\Contracts\Cache\Repository as CacheRepository;
 use Illuminate\Filesystem\Filesystem;
+use Nuwave\Lighthouse\Support\Utils;
 
 class QueryCache
 {
@@ -30,7 +31,7 @@ public function __construct(
 
         $this->enable = (bool) $config['enable'];
         $this->mode = $config['mode'] ?? 'store';
-        $this->opcachePath = $config['opcache_path'] ?? base_path('bootstrap/cache');
+        $this->opcachePath = rtrim($config['opcache_path'] ?? base_path('bootstrap/cache'), '/');
         $this->store = $config['store'] ?? null;
         $this->ttl = $config['ttl'] ?? null;
     }
@@ -86,43 +87,43 @@ protected function fromStoreOrParse(string $hash, \Closure $parse): DocumentNode
     /** @param  \Closure(): DocumentNode  $parse */
     protected function fromOPcacheOrParse(string $hash, \Closure $parse): DocumentNode
     {
-        $filePath = $this->opcacheFilePath($hash);
+        $path = $this->opcacheFilePath($hash);
 
-        if ($this->filesystem->exists($filePath)) {
-            return $this->requireOPcacheFile($filePath);
+        if ($this->filesystem->exists($path)) {
+            return $this->requireOPcacheFile($path);
         }
 
         $query = $parse();
 
         $contents = static::opcacheFileContents($query);
-        $this->filesystem->put(path: $filePath, contents: $contents, lock: true);
+        Utils::atomicPut(filesystem: $this->filesystem, path: $path, contents: $contents);
 
         return $query;
     }
 
     /** @param  \Closure(): DocumentNode  $parse */
     protected function fromHybridOrParse(string $hash, \Closure $parse): DocumentNode
     {
-        $filePath = $this->opcacheFilePath($hash);
+        $path = $this->opcacheFilePath($hash);
 
-        if ($this->filesystem->exists($filePath)) {
-            return $this->requireOPcacheFile($filePath);
+        if ($this->filesystem->exists($path)) {
+            return $this->requireOPcacheFile($path);
         }
 
         $store = $this->makeCacheStore();
 
         $contents = $store->get(key: "lighthouse:query:{$hash}");
         if (is_string($contents)) {
-            $this->filesystem->put(path: $filePath, contents: $contents, lock: true);
+            Utils::atomicPut(filesystem: $this->filesystem, path: $path, contents: $contents);
 
-            return $this->requireOPcacheFile($filePath);
+            return $this->requireOPcacheFile($path);
         }
 
         $query = $parse();
 
         $contents = static::opcacheFileContents($query);
         $store->put(key: "lighthouse:query:{$hash}", value: $contents, ttl: $this->ttl);
-        $this->filesystem->put(path: $filePath, contents: $contents, lock: true);
+        Utils::atomicPut(filesystem: $this->filesystem, path: $path, contents: $contents);
 
         return $query;
     }
@@ -144,10 +145,10 @@ public static function opcacheFileContents(DocumentNode $query): string
         return "<?php return {$queryArrayString};";
     }
 
-    protected function requireOPcacheFile(string $filePath): DocumentNode
+    protected function requireOPcacheFile(string $path): DocumentNode
     {
-        $astArray = require $filePath;
-        assert(is_array($astArray), 'The cache file is expected to return an array.');
+        $astArray = require $path;
+        assert(is_array($astArray), "The query cache file at {$path} is expected to return an array.");
 
         $astInstance = AST::fromArray($astArray);
         assert($astInstance instanceof DocumentNode, 'The AST array is expected to convert to a DocumentNode.');

src/Console/PrintSchemaCommand.php

@@ -84,7 +84,8 @@ public function handle(ASTCache $cache, FilesystemManager $filesystemManager, Sc
                 return;
             }
 
-            $filesystemManager->disk($disk)->put($filename, $schemaString);
+            $filesystem = $filesystemManager->disk($disk);
+            $filesystem->put(path: $filename, contents: $schemaString);
             $this->info("Wrote schema to disk ({$disk}) as {$filename}.");
         } else {
             $this->info($schemaString);

src/Exceptions/InvalidSchemaCacheContentsException.php

@@ -319,15 +319,15 @@ public function loadPersistedQuery(string $sha256hash): DocumentNode
             || ! $queryCacheConfig['enable']
         ) {
             // https://github.com/apollographql/apollo-server/blob/37a5c862261806817a1d71852c4e1d9cdb59eab2/packages/apollo-server-errors/src/index.ts#L240-L248
-            throw new Error('PersistedQueryNotSupported', null, null, [], null, null, ['code' => 'PERSISTED_QUERY_NOT_SUPPORTED']);
+            throw new Error(message: 'PersistedQueryNotSupported', extensions: ['code' => 'PERSISTED_QUERY_NOT_SUPPORTED']);
         }
 
         $cacheFactory = Container::getInstance()->make(CacheFactory::class);
         $store = $cacheFactory->store($queryCacheConfig['store']);
 
         return $store->get("lighthouse:query:{$sha256hash}")
             // https://github.com/apollographql/apollo-server/blob/37a5c862261806817a1d71852c4e1d9cdb59eab2/packages/apollo-server-errors/src/index.ts#L230-L239
-            ?? throw new Error('PersistedQueryNotFound', null, null, [], null, null, ['code' => 'PERSISTED_QUERY_NOT_FOUND']);
+            ?? throw new Error(message: 'PersistedQueryNotFound', extensions: ['code' => 'PERSISTED_QUERY_NOT_FOUND']);
     }
 
     /** @return ErrorsHandler */

src/GraphQL.php

@@ -4,7 +4,7 @@
 
 use Illuminate\Contracts\Config\Repository as ConfigRepository;
 use Illuminate\Filesystem\Filesystem;
-use Nuwave\Lighthouse\Exceptions\InvalidSchemaCacheContentsException;
+use Nuwave\Lighthouse\Support\Utils;
 
 /**
  * @phpstan-type CacheConfig array{
@@ -48,13 +48,7 @@ public function set(DocumentAST $documentAST): void
         );
         $contents = /** @lang PHP */ "<?php return {$variable};";
 
-        // Since the schema cache can be very large, we write it to a temporary file first.
-        // This avoids issues with the filesystem not being able to write large files atomically.
-        // Then, we move the temporary file to the final location which is an atomic operation.
-        $path = $this->path();
-        $partialPath = "{$path}.partial";
-        $this->filesystem->put(path: $partialPath, contents: $contents, lock: true);
-        $this->filesystem->move(path: $partialPath, target: $path);
+        Utils::atomicPut(filesystem: $this->filesystem, path: $this->path(), contents: $contents);
     }
 
     public function clear(): void
@@ -67,14 +61,12 @@ public function fromCacheOrBuild(callable $build): DocumentAST
     {
         $path = $this->path();
         if ($this->filesystem->exists($path)) {
-            $ast = require $path;
-            if (! is_array($ast)) {
-                throw new InvalidSchemaCacheContentsException($path, $ast);
-            }
+            $astArray = require $path;
+            assert(is_array($astArray), "The schema cache file at {$path} is expected to return an array.");
 
-            /** @var SerializableDocumentAST $ast */
+            /** @var SerializableDocumentAST $astArray */
 
-            return DocumentAST::fromArray($ast);
+            return DocumentAST::fromArray($astArray);
         }
 
         $documentAST = $build();

src/Schema/AST/ASTCache.php

@@ -3,6 +3,7 @@
 namespace Nuwave\Lighthouse\Support;
 
 use Illuminate\Container\Container;
+use Illuminate\Filesystem\Filesystem;
 use Illuminate\Support\Str;
 use Nuwave\Lighthouse\Exceptions\DefinitionException;
 
@@ -200,4 +201,22 @@ public static function toEnumValueName(string $name): string
             ? $name
             : "_{$name}";
     }
+
+    /**
+     * Write the contents to a file atomically.
+     *
+     * This is done by writing to a temporary file first and then renaming it.
+     *
+     * It circumvents the following issues:
+     * - large files not being written completely before being read
+     * - partial writes from other processes while we expect to read what we wrote
+     */
+    public static function atomicPut(Filesystem $filesystem, string $path, string $contents): void
+    {
+        $randomSuffix = bin2hex(random_bytes(6));
+        $tempPath = "{$path}.{$randomSuffix}";
+
+        $filesystem->put(path: $tempPath, contents: $contents);
+        $filesystem->move(path: $tempPath, target: $path);
+    }
 }