[Caching] Use FileSystem::writeAtomic() in FileCacheStorage to avoid partial cache reads on parallel run (#8520)

Author: TomasVotruba

.github/workflows/file_cache_storage_test.yaml

@@ -0,0 +1,39 @@
+name: File Cache Storage Test
+
+on:
+    pull_request: null
+    push:
+     branches:
+        - main
+
+
+
+env:
+    # see https://github.com/composer/composer/issues/9368#issuecomment-718112361
+    COMPOSER_ROOT_VERSION: "dev-main"
+
+jobs:
+    tests:
+        strategy:
+            fail-fast: false
+            matrix:
+                os: [ubuntu-latest, windows-latest]
+                php-versions: ['8.4']
+
+        runs-on: ${{ matrix.os }}
+        timeout-minutes: 3
+
+        name: File Cache Storage ${{ matrix.php-versions }} (${{ matrix.os }})
+        steps:
+            -   uses: actions/checkout@v5
+
+            -
+                uses: shivammathur/setup-php@v2
+                with:
+                    php-version: ${{ matrix.php-versions }}
+                    coverage: none
+                    ini-values: zend.assertions=1
+
+            -   uses: "ramsey/composer-install@v4"
+
+            -   run: vendor/bin/phpunit tests/Caching/ValueObject/Storage/FileCacheStorageTest.php --colors

composer.json

@@ -20,7 +20,7 @@
         "doctrine/inflector": "^2.1",
         "entropy/entropy": "^0.4.12",
         "fidry/cpu-core-counter": "^1.1",
-        "nette/utils": "^4.1.4",
+        "nette/utils": "^4.1.5",
         "nikic/php-parser": "^5.9",
         "ondram/ci-detector": "^4.2",
         "phpstan/phpdoc-parser": "^2.3.3",

src/Caching/ValueObject/Storage/FileCacheStorage.php

@@ -6,7 +6,6 @@
 
 use FilesystemIterator;
 use Nette\Utils\FileSystem;
-use Nette\Utils\Random;
 use Rector\Caching\Contract\ValueObject\Storage\CacheStorageInterface;
 use Rector\Caching\ValueObject\CacheFilePaths;
 use Rector\Caching\ValueObject\CacheItem;
@@ -55,7 +54,6 @@ public function save(string $key, string $variableKey, mixed $data): void
 
         $filePath = $cacheFilePaths->getFilePath();
 
-        $tmpPath = \sprintf('%s/%s.tmp', $this->directory, Random::generate());
         $errorBefore = \error_get_last();
         $exported = @\var_export(new CacheItem($variableKey, $data), true);
         $errorAfter = \error_get_last();
@@ -68,18 +66,9 @@ public function save(string $key, string $variableKey, mixed $data): void
             ));
         }
 
-        // for performance reasons we don't use SmartFileSystem
-        FileSystem::write($tmpPath, \sprintf("<?php declare(strict_types = 1);\n\nreturn %s;", $exported), null);
-        $copySuccess = @\copy($tmpPath, $filePath);
-        @\unlink($tmpPath);
-
-        if ($copySuccess) {
-            return;
-        }
-
-        if (\DIRECTORY_SEPARATOR === '/' || ! \file_exists($filePath)) {
-            throw new CachingException(\sprintf('Could not write data to cache file %s.', $filePath));
-        }
+        // atomic write via temp file + rename(), so a parallel reader never sees a partially written cache file
+        // handles the Windows rename() edge case too, see https://github.com/rectorphp/rector/issues/9876
+        FileSystem::writeAtomic($filePath, \sprintf("<?php declare(strict_types = 1);\n\nreturn %s;", $exported), null);
     }
 
     public function clean(string $key): void

tests/Caching/ValueObject/Storage/FileCacheStorageTest.php

@@ -49,6 +49,35 @@ public function testClean(): void
         $this->assertDirectoryDoesNotExist(__DIR__ . '/Source/0e');
     }
 
+    public function testSaveLeavesConcurrentReaderOnCompleteFile(): void
+    {
+        if (\DIRECTORY_SEPARATOR === '\\') {
+            // Windows blocks rename() over a file open in another handle, so the atomic-replace-under-open-reader
+            // scenario this asserts is POSIX-only; on Windows writeAtomic() retries the transient lock instead
+            $this->markTestSkipped('Atomic replace under an open reader is POSIX-only');
+        }
+
+        $filePath = __DIR__ . '/Source/0e/76/0e76658526655756207688271159624026011393.php';
+
+        $this->fileCacheStorage->save('aaK1STfY', 'TEST', 'first');
+        $contentsBeforeSave = (string) file_get_contents($filePath);
+
+        // every parallel worker require()s this path while booting; open it as such a worker would,
+        // then save over it mid-read - an atomic write must leave the reader on the file it opened
+        $readerHandle = fopen($filePath, 'r');
+        $this->assertNotFalse($readerHandle);
+
+        $this->fileCacheStorage->save('aaK1STfY', 'TEST', 'second');
+
+        $contentsSeenByReader = stream_get_contents($readerHandle);
+        fclose($readerHandle);
+
+        $this->assertSame($contentsBeforeSave, $contentsSeenByReader);
+        $this->assertSame('second', $this->fileCacheStorage->load('aaK1STfY', 'TEST'));
+
+        $this->fileCacheStorage->clean('aaK1STfY');
+    }
+
     public function provideConfigFilePath(): string
     {
         return __DIR__ . '/config.php';