Skip to content

fix: handle definition cache delete races safely#479

Open
jinhoe-kim-imweb-me wants to merge 2 commits intoezyang:masterfrom
jinhoe-kim-imweb-me:fix/safe-unlink-definition-cache
Open

fix: handle definition cache delete races safely#479
jinhoe-kim-imweb-me wants to merge 2 commits intoezyang:masterfrom
jinhoe-kim-imweb-me:fix/safe-unlink-definition-cache

Conversation

@jinhoe-kim-imweb-me
Copy link

@jinhoe-kim-imweb-me jinhoe-kim-imweb-me commented Mar 11, 2026

Summary

  • route remove(), flush(), and cleanup() through a shared safeUnlink() helper
  • treat files already deleted by another process as a successful cleanup
  • keep emitting a warning when the file still exists after unlink() fails

Background

Fixes #442.

cleanup() already checks file_exists() before calling unlink(), but that still leaves a small TOCTOU window when multiple processes clean the same serializer cache directory at the same time. remove() and flush() still call unlink() directly as well.

This change keeps the existing warning signal for real delete failures while avoiding noisy warnings when another process has already removed the cache file.

Tests

  • php tests/index.php --file=HTMLPurifier/DefinitionCache/SerializerTest.php --txt

@jinhoe-kim-imweb-me jinhoe-kim-imweb-me changed the title Handle definition cache delete races safely fix: handle definition cache delete races safely Mar 11, 2026
@jinhoe-kim-imweb-me
Copy link
Author

FYI: after the follow-up test change, the SerializerTest-specific PHP 8.5 deprecation from ReflectionMethod::setAccessible() is gone.

The remaining PHP 8.5 failures in CI appear to come from existing upstream deprecations outside this change, for example:

  • library/HTMLPurifier/URISchemeRegistry.php (Using null as an array offset is deprecated)
  • library/HTMLPurifier/Injector/RemoveSpansWithoutAttributes.php (SplObjectStorage::attach()/contains()/detach() deprecations)

So the current red PHP 8.5 job does not look specific to this PR's serializer changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deleting a file that does not exist on the server

1 participant