Skip to content

[Cache] Temporary files not cleaned from filesystem #52092

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
cedric-anne opened this issue Oct 17, 2023 · 3 comments
Closed

[Cache] Temporary files not cleaned from filesystem #52092

cedric-anne opened this issue Oct 17, 2023 · 3 comments
Labels
Bug Cache Status: Needs Review

Comments

@cedric-anne
Copy link
Contributor

cedric-anne commented Oct 17, 2023 •
edited
Loading

Symfony version(s) affected

5.x, 6.x

Description

On an open source PHP application I work on, we sometimes have issues reported by administrators related to unexpected filesystem saturation. The impacted cache namespace is supposed to contains only a dozen of items, but the directory contains 250k files located at its root. These files are temporary files generated when a cache entry is updated, and are supposed to be removed automatically.

The problem is that the target file is exists, contains stale information, but is not writable.

First, let's resume what is done by the Filesystem apadter when a cache entry is written:

  1. a temporary file is created at the root of the cache directory;
  2. content is written inside this file;
  3. this file is renamed to its target path, using the PHP rename() function.

Related code:

symfony/src/Symfony/Component/Cache/Traits/FilesystemCommonTrait.php

Lines 88 to 114 in d69ab75

private function write(string $file, string $data, int $expiresAt = null): bool
{
set_error_handler(__CLASS__.'::throwError');
try {
$tmp = $this->directory.$this->tmpSuffix ??= str_replace('/', '-', base64_encode(random_bytes(6)));
try {
$h = fopen($tmp, 'x');
} catch (\ErrorException $e) {
if (!str_contains($e->getMessage(), 'File exists')) {
throw $e;
}
$tmp = $this->directory.$this->tmpSuffix = str_replace('/', '-', base64_encode(random_bytes(6)));
$h = fopen($tmp, 'x');
}
fwrite($h, $data);
fclose($h);
if (null !== $expiresAt) {
touch($tmp, $expiresAt ?: time() + 31556952); // 1 year in seconds
}
return rename($tmp, $file);
} finally {
restore_error_handler();
}
}

Now, let's see what the rename() function do:

  1. the file is copied to its target path;
  2. the ownership of the target file is updated;
  3. the mod of the target file is updated;
  4. the source file is deleted.

If any of the 3 first steps fails, the unlink is not done.

Related code: https://github.com/php/php-src/blob/221b4fe246e62c5d59f617ee4cbe6fd4c614fb5a/main/streams/plain_wrapper.c#L1272-L1371

How to reproduce

  1. Create a cache entry with a short lifetime (1s).
  2. Make the corresponding file read-only on the filesystem.
  3. Update the cache entry.

-> The temporary file created on the cache directory root is not deleted.

Possible Solution

On Symfony 4.4, a cleaning operation was always done after the rename, see #39059. I think it could be simple to put back this cleaning operation, to prevent filesystem saturation.

Additional Context

No response
@cedric-anne
Copy link
Contributor Author

My bad, one of the target file was existing but was not writable. I thought an exception would be triggered, but it is in fact catched internally by Symfony.

I will update the main description.

@nicolas-grekas
Copy link
Member

Could you please send a PR if you have a fix in mind?

@cedric-anne
Copy link
Contributor Author

Could you please send a PR if you have a fix in mind?

I will do.

@cedric-anne cedric-anne mentioned this issue Oct 17, 2023
Merged
nicolas-grekas added a commit that referenced this issue Oct 17, 2023
@nicolas-grekas
bug #52105 [Cache] Remove temporary cache item file on rename() fai…
8c02677 
…lure (cedric-anne)

This PR was squashed before being merged into the 5.4 branch.

Discussion
----------

[Cache] Remove temporary cache item file on `rename()` failure

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #52092
| License       | MIT

The filesystem cache adapter creates a temporary file to store cache item data, then moves it to its target path using `rename()` function. If rename fails, for instance target path is not writable, the temporary file will remains.

To prevent filesystem saturation, the temporary files should be deleted if this operation is not done by the `rename()` function itself.

Commits
-------

af15423 [Cache] Remove temporary cache item file on `rename()` failure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Cache Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@nicolas-grekas @carsonbot @cedric-anne