[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.