fix: a corrupt cache is rebuilt rather than fatal - #881
Merged
Conversation
`FileCache::save()` truncates the file in place and writes, holding an advisory lock; `readToString()` takes no lock at all. `flock()` only binds processes that call it, so a reader can land between another request's `ftruncate(0)` and its completed `fwrite()` and get a partial file. `unserialize()` refuses it, and `Serde` turns that into a plain `SPException`. `Actions::loadCache()` caught `FileException` — which is `SPException`'s *child*, so the catch never matched. The exception escaped the constructor, and `Acl` depends on `ActionsInterface`, so essentially every request broke. The corrupting write refreshes the file's mtime, so the 24-hour expiry check could not heal it either: it took deleting `var/cache/actions.cache` by hand. `MimeTypes::loadCache()` had no catch at all, and it is what the file upload and the config manager read. Both rebuild from the YAML now instead of failing. The catch is around the *load* alone, not the whole method. Wrapping the rebuild as well — which is what I wrote first — means a rebuild that fails on its own terms gets retried once and then swallowed, and it broke the existing test that expects an unreadable YAML file to report rather than be papered over. Checked by narrowing each catch back to `FileException`: both new tests fail with the `SPException` escaping, which is the production symptom.
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.
FileCache::save()truncates the file in place and writes, holding an advisory lock;readToString()takes no lock at all.flock()only binds processes that call it, so areader can land between another request's
ftruncate(0)and its completedfwrite()andget a partial file.
unserialize()refuses it, andSerdeturns that into a plainSPException.Actions::loadCache()caughtFileException— which isSPException's child, so thecatch never matched. The exception escaped the constructor, and
Acldepends onActionsInterface, so essentially every request broke. The corrupting write refreshes thefile's mtime, so the 24-hour expiry check could not heal it either: it took deleting
var/cache/actions.cacheby hand.MimeTypes::loadCache()had no catch at all, and it iswhat the file upload and the config manager read.
Both rebuild from the YAML now instead of failing.
The catch is around the load alone, not the whole method. Wrapping the rebuild as well —
which is what I wrote first — means a rebuild that fails on its own terms gets retried once
and then swallowed, and it broke the existing test that expects an unreadable YAML file to
report rather than be papered over.
Checked by narrowing each catch back to
FileException: both new tests fail with theSPExceptionescaping, which is the production symptom.