fix: Guard against race condition in FileStore::createCacheDir - #466
Open
Sarsharses wants to merge 1 commit into
Open
fix: Guard against race condition in FileStore::createCacheDir#466Sarsharses wants to merge 1 commit into
Sarsharses wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Under concurrent uploads,
FileStore::createCacheDir()can crash withmkdir(): File exists. On PHP setups that promote warnings to exceptions (e.g. Laravel's error handler), this surfaces as a fatalErrorExceptionon the first upload after the cache directory is absent.Root cause
The check-then-create is not atomic:
Two concurrent requests can both pass
file_exists(), then both callmkdir(); the loser gets aFile existswarning.Note: this is a different race from the cache-file data race fixed previously (#366, #368). Those hardened reads/writes of the cache file, whereas this fixes creation of the cache directory itself.
Fix
@mkdir(); the directory ends up existing either way.is_dir()(the correct predicate, sincefile_exists()is also true for a file at that path).0777, true) so nested cache paths work; default mode is unchanged.Testing
Added
it_creates_nested_cache_dir_recursively, which fails onmainand passes with the fix. Full suite stays green.A pure concurrency race can't be reproduced deterministically in a single-threaded test; the added test covers the recursive-creation half, and the warning suppression is the standard idiom used by Symfony/Laravel
mkdirhelpers.