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 */