From 442f374ca2db95dd00a8b56778a0ff837a6abf1b Mon Sep 17 00:00:00 2001 From: blaipr Date: Wed, 26 Aug 2026 20:18:49 +0200 Subject: [PATCH] fix: a corrupt cache is rebuilt rather than fatal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- src/Infrastructure/Acl/Actions.php | 34 +++++++++++++--- src/Infrastructure/MimeTypes.php | 16 +++++++- tests/Unit/Infrastructure/Acl/ActionsTest.php | 39 +++++++++++++++++++ tests/Unit/Infrastructure/MimeTypesTest.php | 30 ++++++++++++++ 4 files changed, 112 insertions(+), 7 deletions(-) diff --git a/src/Infrastructure/Acl/Actions.php b/src/Infrastructure/Acl/Actions.php index 40f3db5a8..9c5c6525b 100644 --- a/src/Infrastructure/Acl/Actions.php +++ b/src/Infrastructure/Acl/Actions.php @@ -32,6 +32,7 @@ use SP\Domain\Storage\Ports\FileCacheService; use SP\Infrastructure\Storage\Ports\YamlFileStorageService; use SP\Domain\Core\Exceptions\FileException; +use SP\Domain\Core\Exceptions\SPException; use function SP\__u; use function SP\logger; @@ -73,18 +74,39 @@ private function loadCache(): void || $this->fileCache->isExpiredDate($this->yamlFileStorage->getFileTime()) ) { $this->mapAndSave(); - } else { - // Action[]: an array of objects, which loadWith() cannot express — it answers - // with one object of the class it was given. Naming the class here keeps the - // cache from building anything else. - $this->actions = $this->fileCache->load(null, Action::class); - logger('Loaded actions cache', 'INFO'); + return; } } catch (FileException $e) { processException($e); $this->mapAndSave(); + + return; + } + + try { + // Action[]: an array of objects, which loadWith() cannot express — it answers + // with one object of the class it was given. Naming the class here keeps the + // cache from building anything else. + $this->actions = $this->fileCache->load(null, Action::class); + + logger('Loaded actions cache', 'INFO'); + } catch (SPException $e) { + // SPException, not FileException. A cache file that cannot be opened raises the + // latter, but one that is readable and *corrupt* — truncated by a reader landing + // between another request's ftruncate() and its write — comes back from Serde as a + // plain SPException, and FileException is its child, so catching the child never + // caught it. It escaped the constructor, and since Acl depends on this class, + // essentially every request broke. The corrupting write refreshes the file's mtime, + // so the expiry check above could not heal it either: it took deleting the file by + // hand. + // + // Around the load alone, so that a rebuild which fails on its own terms still + // reports rather than being retried once and swallowed. + processException($e); + + $this->mapAndSave(); } } diff --git a/src/Infrastructure/MimeTypes.php b/src/Infrastructure/MimeTypes.php index 169493065..ad7c60f07 100644 --- a/src/Infrastructure/MimeTypes.php +++ b/src/Infrastructure/MimeTypes.php @@ -31,6 +31,7 @@ use SP\Domain\Storage\Ports\FileCacheService; use SP\Infrastructure\Storage\Ports\YamlFileStorageService; use SP\Domain\Core\Exceptions\FileException; +use SP\Domain\Core\Exceptions\SPException; use function SP\logger; use function SP\processException; @@ -76,13 +77,26 @@ private function loadCache(): void || $this->fileCache->isExpiredDate($this->yamlFileStorageService->getFileTime()) ) { $this->mapAndSave(); - } else { + + return; + } + + try { // MimeType[]: an array of objects, so the class is named. Without it every entry // came back as __PHP_Incomplete_Class and failed far from here, where a closure in // ConfigManager\IndexController takes a MimeType. $this->mimeTypes = $this->fileCache->load(null, MimeType::class); logger('Loaded MIME types cache', 'INFO'); + } catch (SPException $e) { + // Same reason as Actions::loadCache(), and there was no catch here at all: a readable + // but corrupt cache — a reader landing mid-write — comes back from Serde as a plain + // SPException and took this down too. MimeTypes is what the file upload and the + // config manager read. Around the load alone, so a rebuild that fails on its own + // terms still reports. + processException($e); + + $this->mapAndSave(); } } diff --git a/tests/Unit/Infrastructure/Acl/ActionsTest.php b/tests/Unit/Infrastructure/Acl/ActionsTest.php index 7f520e6a5..fc7f97e76 100644 --- a/tests/Unit/Infrastructure/Acl/ActionsTest.php +++ b/tests/Unit/Infrastructure/Acl/ActionsTest.php @@ -183,6 +183,45 @@ public function testResetAndNotExpired() self::assertEquals($action, $out); } + /** + * A cache file that is readable but corrupt is rebuilt, not fatal. + * + * FileCache::save() truncates in place and writes, holding an advisory lock; readToString() + * takes no lock at all. So a reader can land between the truncate and the write and get a + * partial file — which unserialize() refuses, and Serde turns into a plain SPException. + * + * Only FileException was caught, and FileException is SPException's *child*, so that catch + * never matched: the exception escaped the constructor. Acl depends on this class, so it took + * essentially every request down — and the corrupting write refreshes the mtime, so the + * expiry check could not heal it. It took deleting the file by hand. + * + * @throws ActionNotFoundException + * @throws FileException + * @throws Exception + */ + public function testResetWithACorruptCacheRebuildsIt() + { + $this->fileCache + ->expects(self::once()) + ->method('isExpired') + ->with(Actions::CACHE_EXPIRE) + ->willReturn(false); + + // What Serde answers for a truncated file, which is not a FileException. + $this->fileCache + ->expects(self::once()) + ->method('load') + ->willThrowException(SPException::error('Couldn\'t deserialize the data')); + + $actionsMapped = $this->checkLoadAndSave(); + + $this->actions->reset(); + + $action = current($actionsMapped); + + self::assertEquals($action, $this->actions->getActionById($action->getId())); + } + /** * @throws ActionNotFoundException * @throws FileException diff --git a/tests/Unit/Infrastructure/MimeTypesTest.php b/tests/Unit/Infrastructure/MimeTypesTest.php index 0d443736e..a259cf8ca 100644 --- a/tests/Unit/Infrastructure/MimeTypesTest.php +++ b/tests/Unit/Infrastructure/MimeTypesTest.php @@ -37,6 +37,7 @@ use SP\Domain\Storage\Ports\XmlFileStorageService; use SP\Infrastructure\Storage\Ports\YamlFileStorageService; use SP\Domain\Core\Exceptions\FileException; +use SP\Domain\Core\Exceptions\SPException; use SP\Tests\Support\UnitaryTestCase; /** @@ -70,6 +71,35 @@ public function testGetMimeTypes() } } + /** + * A cache file that is readable but corrupt is rebuilt, not fatal. + * + * There was no catch here at all — Actions::loadCache() at least caught FileException, which + * still did not match what Serde raises for a truncated file. MimeTypes is what the file + * upload and the config manager read, so the same torn write took those down. + * + * @throws FileException + */ + public function testACorruptCacheIsRebuilt() + { + $this->fileCache->method('exists')->willReturn(true); + $this->fileCache->method('isExpired')->willReturn(false); + + // What Serde answers for a truncated file, which is not a FileException. + $this->fileCache + ->expects(self::once()) + ->method('load') + ->willThrowException(SPException::error('Couldn\'t deserialize the data')); + + // The rebuild reads the YAML and writes the cache again. + $this->checkBuildCache(); + + $mimeTypes = new MimeTypes($this->fileCache, $this->yamlFileStorage); + + // Rebuilt rather than escaping the constructor. + self::assertCount(10, $mimeTypes->getMimeTypes()); + } + /** * @throws FileException */