[Caching] Use atomic rename() on non-Windows in FileCacheStorage to avoid partially written cache reads on parallel run

[samsonasik]

Fixes rectorphp/rector#9876

[samsonasik]

@dragosprotung could you try manually verify if this works ? Thank you.

[TomasVotruba]

This seems very costly on every cache call.

[TomasVotruba]

For inspiration: https://github.com/phpstan/phpstan-src/blob/b293f3c1c6c55d63efeb0634f131770f9b1c7e0f/src/Cache/FileCacheStorage.php#L88-L96

[samsonasik]

@TomasVotruba we originally follow that, and cause windows issue, see original issue and PR that fix rename issue:

• Could not process a file due to System error: Cannot rename rector#8432

that was resolved by using copy

• [Windows] Utilize Nette\Utils\FileSystem instead of Symfony\Component\Filesystem\Filesystem to write file #5514

as back to rename() may cause old bug show again on windows.

[TomasVotruba]

Not sure it was the sole Windows causing issue, that would have been spotted in our CI.
What is the OS causing the issue here?

[samsonasik]

see still open php issue on rename

• rename fails on Windows PHP 8.1 if the target file is being executed php/php-src#7910

we still support php 7.4 on scoped build.

[dragosprotung]

@samsonasik using rename works

[samsonasik]

@TomasVotruba I've updated to php-src include issue:

• rename fails on Windows PHP 8.1 if the target file is being executed php/php-src#7910

in the comment and make less diff so easier to review.

Should be ready now 👍

[TomasVotruba]

This still executes a condition on every call. Very costly.

Instead, we should trigger fallback only in case of write failure.
Something like:

$copySuccess = @\copy($tmpPath, $filePath);

if ($copySuccess) {
    return;
}

// try again here

[dragosprotung]

Copy does not fail, it's just that in parallel run, the cache file is written by different processes at the same time and it gets corrupted.
copy() truncates the destination and streams into it, so it is not atomic.

[TomasVotruba]

We'll need a failing reproducer in our CI, so we avoid changing this back and forth.

[samsonasik]

@TomasVotruba as per @dragosprotung above, this can't one function too early call and fallback later. It not run 2 function, it just verify which OS it run via directory_separator, if it windows, use copy, otherwise, use rename.

The @unlink is original code ensure nothing left behind after copied.

[samsonasik]

@TomasVotruba It can't be proven here, since the issue is on php < 8.1 on windows.

[TomasVotruba]

@samsonasik We can setup any version in CI, even Windows

[samsonasik]

this repo is on php 8.2, the test require php <8.1, so it needs merge first with e2e on scoped build?

[samsonasik]

Even on scoped build, I am not sure if CI can setup UAC protected locations windows setup.

[samsonasik]

@dragosprotung I am thinking if rename can run early, then copy later if rename failure, since rename can fail, so copy can be fallback....

[samsonasik]

@TomasVotruba I've updated to rename() early, copy later if rename() failure on affected php < 8.1 versions.

[dragosprotung]

@samsonasik i think this is the best approach, since copy is needed only in a very specific situation

[samsonasik]

@TomasVotruba this one should be ready now 👍

[staabm]

you might consider dropping your workarounds and use FileSystem::writeAtomic instead

nette/utils@cc0326d#diff-0e93d4e4e78af9e32c960e4a2b36d8339bb2c76fa8b84ba8fe7df03cd9d542fe

[samsonasik]

is that cover windows issue? The windows needs copy instead of rename.

[staabm]

a automated test should tell

[samsonasik]

As I noted in previous comment, that' need php < 8.1, and that's only after scoped build deployed, and even on scoped build, I am not sure if CI can setup UAC protected locations windows setup.

[TomasVotruba]

@staabm Thanks for spotting this. Knowing you as a Windows expert here, I'll give it a go 👍
#8520

[dragosprotung]

@TomasVotruba anything blocking to merge this ?

[TomasVotruba]

@dragosprotung Reproducer tests that is failing now, so we avoid blind back and forth commits.

[dragosprotung]

Not sure how this can be added in a test since is a race condition. I run rector on a 32 threads machine and i get it all the time. if i switch to 2 cores, not so often.
@samsonasik is there something we can do ?

[samsonasik]

@dragosprotung @TomasVotruba I am not sure about reproduce it on CI, as it require setup UAC protected locations windows.

The only way is to check manually on that as far as I can see.

[calebdw]

We hit this on Linux, which I think reframes the reproducer discussion a bit.

@TomasVotruba: Not sure it was the sole Windows causing issue, that would have been spotted in our CI. What is the OS causing the issue here?

Linux, PHP 8.5, withParallel(maxNumberOfProcess: 8), rector/rector dev-main @ 0766827. Nothing to do with php/php-src#7910 — that's the Windows bug that motivated the original switch to copy(), and it's separate from what breaks here:

{"fatal_errors":["syntax error, unexpected string content \"a735e9b56\""]}
 [ERROR] Could not process some files, due to: "Child process error".

a735e9b5608a... is a rule hash from inside Rector's own cache, truncated mid-literal — a worker required a half-written cache file.

It's always the configuration_hash entry, and that's not luck. RectorContainerFactory::createFromBootstrapConfigs() calls ChangedFilesDetector::setFirstResolvedConfigFileInfo(), which both load()s (require) and unconditionally save()s that one path — in every process, including all 8 workers. So N workers stampede a single file during boot, while copy() truncates the destination and streams into it. Per-file hash entries aren't contended the same way, which is why this is the only entry that ever tears.

This is what PHPStan does

@TomasVotruba: For inspiration: https://github.com/phpstan/phpstan-src/blob/b293f3c1c6c55d63efeb0634f131770f9b1c7e0f/src/Cache/FileCacheStorage.php#L88-L96

Worth underlining how closely this PR already lands on that. PHPStan 2.2.14 FileCacheStorage::save(), today:

FileWriter::write($tmpPath, "<?php declare(strict_types = 1);\n\n" . '// ' . $key . "\nreturn " . $exported . ';');
$renameSuccess = @rename($tmpPath, $path);
if ($renameSuccess) {
    return;
}
@unlink($tmpPath);
if (DIRECTORY_SEPARATOR === '/' || !is_file($path)) {
    throw new InvalidArgumentException(sprintf('Could not write data to cache file %s.', $path));
}

Same temp file, same rename() first, same early return, same DIRECTORY_SEPARATOR === '/' || ! is_file() guard. This PR is that code plus a copy() fallback for the affected Windows versions — which is strictly more conservative than the reference.

That's not a coincidence of style: the class docblock still says "Inspired by https://github.com/phpstan/phpstan-src/.../src/Cache/FileCacheStorage.php", and #498 was literally titled "Sync FileCacheStorage with changes from phpstan-src". rename() is what was here until #5514 swapped it for copy() to dodge the Windows bug, and that swap is what reintroduced the tearing. PHPStan runs parallel over the same cache layout and doesn't hit this, because it never gave up the atomic replace.

On the reproducer

@TomasVotruba: We'll need a failing reproducer in our CI, so we avoid changing this back and forth.

Here's a test that fails on copy() and passes on this PR's rename(), with no threads and no timing:

public function testSaveLeavesAConcurrentReaderOnACompleteFile(): void
{
    $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 its container; open it the
    // way such a worker would, then have another worker save over it mid-read
    $readerHandle = fopen($filePath, 'r');
    $this->assertNotFalse($readerHandle);

    $this->fileCacheStorage->save('aaK1STfY', 'TEST', 'second');

    $contentsSeenByReader = stream_get_contents($readerHandle);
    fclose($readerHandle);

    // an atomic replace leaves the reader on the whole file it opened; writing through the
    // published inode instead exposes the reader to however much of the new file has landed
    $this->assertSame($contentsBeforeSave, $contentsSeenByReader);

    $this->assertSame('second', $this->fileCacheStorage->load('aaK1STfY', 'TEST'));

    $this->fileCacheStorage->clean('aaK1STfY');
}

On current main:

1) Rector\Tests\Caching\ValueObject\Storage\FileCacheStorageTest::testSaveLeavesAConcurrentReaderOnACompleteFile
Failed asserting that two strings are identical.
--- Expected
+++ Actual
 return \Rector\Caching\ValueObject\CacheItem::__set_state(array(
    'variableKey' => 'TEST',
-   'data' => 'first',
+   'data' => 'second',
 ));

The handle was opened before the save and never re-opened, yet it reads 'second' — the writer mutated the file underneath a live reader. On this branch it reads 'first', because rename() leaves that reader on the inode it opened.

To be straight about what this does and doesn't prove: it is not a reproduction of the crash. It pins the precondition — a writer mutating a file under an open reader. It doesn't catch a torn file, because with a small payload copy() finishes before the read, so you get complete-but-wrong content rather than a ParseError. Producing an actual torn read needs the interleaving, and I don't think that can be made deterministic. What it does give you is a guard that fails the moment someone puts copy() back, which I took to be the point of asking.

Happy to send it as a PR against partial-cache if that's easier than applying it by hand.

@dragosprotung — this matches your read exactly, FWIW, including the 32-threads-vs-2-cores frequency.

[TomasVotruba]

Resolved in #8520
Let's see how it goes :)