Skip to content

fix: a corrupt cache is rebuilt rather than fatal - #881

Merged
blaipr merged 1 commit into
mainfrom
fix/a-corrupt-cache-is-rebuilt-not-fatal
Aug 26, 2026
Merged

fix: a corrupt cache is rebuilt rather than fatal#881
blaipr merged 1 commit into
mainfrom
fix/a-corrupt-cache-is-rebuilt-not-fatal

Conversation

@blaipr

@blaipr blaipr commented Aug 26, 2026

Copy link
Copy Markdown
Member

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.

`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.
@blaipr
blaipr merged commit 076f065 into main Aug 26, 2026
8 checks passed
@blaipr
blaipr deleted the fix/a-corrupt-cache-is-rebuilt-not-fatal branch August 26, 2026 18:26
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.

1 participant