diff --git a/CLAUDE.md b/CLAUDE.md index d71fffb0e..4f33a67ab 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -553,6 +553,21 @@ you have no relationship with at all — `AccountSearchItem::isShowRequest()` is ACL. **Ask what a feature is for before deciding which check it was missing**; a guard copied from a sibling can be the wrong guard. +**A resume point that only moves when everything succeeded.** The upgrade decided what still +needed running from `appVersion`, written once after every handler had finished, while progress was +really being stamped per file in `databaseVersion`. So an interruption between two versions left a +database already migrated and a resume point that had not moved, and the retry re-ran a migration +that had already been applied — `40024210101.sql` drops a column that is no longer there and fails +for good, and `UpgradeConfigText` would decode text that is already decoded, which its own header +says must happen exactly once. Nothing in the upgrade calls `set_time_limit(0)`, though every other +long write path does, so `max_execution_time` alone reaches it. + +**When one field records progress and another decides what to do next, they have to be the same +field.** The fix advances `appVersion` as each version completes, which forces the versions to be +applied in ascending order — a resume point that goes backwards is worse than one that never moves +— and that in turn closed a latent fragility: the order used to be whichever way the handlers were +registered and their attributes declared. + **A guard on the read but not on the write.** `Notification` has the rule written down and named — `checkUserAccess()`, admins may reach any notification and regular users only their own, answering "not found" so ids cannot be enumerated by the difference. It was called from `getById()` and diff --git a/src/Domain/Upgrade/Services/Upgrade.php b/src/Domain/Upgrade/Services/Upgrade.php index 75ffd425a..07fc116d9 100644 --- a/src/Domain/Upgrade/Services/Upgrade.php +++ b/src/Domain/Upgrade/Services/Upgrade.php @@ -85,15 +85,42 @@ public function upgrade(string $version, ConfigDataInterface $configData): void ) ); - foreach ($this->getTargetUpgradeHandlers($version) as [$targetVersion, $upgradeHandler]) { - if (!$upgradeHandler->apply($targetVersion, $configData)) { - throw UpgradeException::critical( - __u('Error while applying the update'), - __u('Please, check the event log for more details') - ); + foreach ($this->getTargetUpgradeHandlersByVersion($version) as $targetVersion => $upgradeHandlers) { + foreach ($upgradeHandlers as $upgradeHandlerClass) { + try { + // Resolved here rather than while grouping, so a handler we never reach — + // because an earlier one failed — is never constructed. The conversion is + // what the grouping's own catch used to provide for this call. + $upgradeHandler = $this->container->get($upgradeHandlerClass); + } catch (Throwable $e) { + throw ServiceException::from($e); + } + + if (!$upgradeHandler->apply($targetVersion, $configData)) { + throw UpgradeException::critical( + __u('Error while applying the update'), + __u('Please, check the event log for more details') + ); + } + + logger('Upgrade: ' . $upgradeHandler::class); } - logger('Upgrade: ' . $upgradeHandler::class); + // The resume point, advanced as each version finishes rather than only once at the end. + // + // What still needs running is derived from `appVersion`, and that used to be written + // after *every* handler had succeeded, while progress was really being stamped per file + // in `databaseVersion`. So an interruption between two versions — an OOM kill, a + // stopped container, or simply `max_execution_time`, which nothing here raises although + // every other long write path calls `set_time_limit(0)` — left a database already + // migrated and a resume point that had not moved. The retry then re-ran a migration + // that had already been applied: `40024210101.sql` drops a column that is no longer + // there and fails for good, and `UpgradeConfigText` would decode text that is already + // decoded, which its own header says must happen exactly once. + // + // Writing it inside the loop is safe because the generator was built from the original + // version and is not re-evaluated; only a later run sees the advanced value. + $configData->setAppVersion($targetVersion); $this->config->save($configData); } @@ -116,12 +143,25 @@ public function upgrade(string $version, ConfigDataInterface $configData): void } /** - * @return iterable + * Every handler still to run, grouped by the version it belongs to, oldest version first. + * + * Grouped because two handlers can declare the same version — `UpgradeDatabase` and + * `UpgradeConfigText` both carry `400.24240101` — and the resume point may only advance once + * both have run. Sorted because it is a resume point: applying a lower version after a higher + * one would move it backwards, and a migration must in any case not run before one that + * precedes it. The order used to be whatever order the handlers were registered and their + * attributes declared in, which happens to ascend today and is nothing the code required. + * + * @param string $version + * + * @return array[]> * @throws ServiceException */ - private function getTargetUpgradeHandlers(string $version): iterable + private function getTargetUpgradeHandlersByVersion(string $version): array { try { + $byVersion = []; + foreach ($this->upgradeHandlers as $class) { $reflection = new ReflectionClass($class); /** @var ReflectionAttribute $attribute */ @@ -129,10 +169,22 @@ private function getTargetUpgradeHandlers(string $version): iterable $instance = $attribute->newInstance(); if (Version::checkVersion($version, $instance->version)) { - yield [$instance->version, $this->container->get($class)]; + // The class, not the instance: a handler that a failure upstream means we + // never reach should not be constructed either. + $byVersion[$instance->version][] = $class; } } } + + uksort( + $byVersion, + static fn(string $left, string $right): int => version_compare( + (string)Version::normalizeVersionForCompare($left), + (string)Version::normalizeVersionForCompare($right) + ) + ); + + return $byVersion; } catch (Throwable $e) { throw ServiceException::from($e); } diff --git a/tests/Unit/Domain/Upgrade/Services/UpgradeTest.php b/tests/Unit/Domain/Upgrade/Services/UpgradeTest.php index e4a5b6b98..d6f51e50c 100644 --- a/tests/Unit/Domain/Upgrade/Services/UpgradeTest.php +++ b/tests/Unit/Domain/Upgrade/Services/UpgradeTest.php @@ -157,6 +157,84 @@ public function testUpgradeWithHandler() $this->upgrade->upgrade('400.00000000', $configData); } + /** + * The resume point moves as each version finishes, not once at the end. + * + * What still needs running is derived from `appVersion`, and it used to be written only after + * every handler had succeeded — while progress was really being stamped per file in + * `databaseVersion`. An interruption between two versions therefore left a database already + * migrated and a resume point that had not moved, and the retry re-ran a migration that had + * already been applied: `40024210101.sql` drops a column that is no longer there and fails for + * good, and `UpgradeConfigText` would decode text that is already decoded, which its own + * header says must happen exactly once. + * + * The second version failing is what makes this say anything: the first has completed, so its + * version must be on record before the failure, and the run must not go on to claim the + * application is fully upgraded. + * + * @throws Exception + * @throws ServiceException + * @throws FileException + * @throws InvalidClassException + */ + public function testAnInterruptedUpgradeRecordsTheVersionsThatFinished() + { + $configData = $this->createMock(ConfigDataInterface::class); + + $handler = $this->createMock(UpgradeHandlerService::class); + $handler->method('apply')->willReturnCallback( + static fn(string $version): bool => $version === '400.00000001' + ); + + $this->container->method('get')->willReturn($handler); + + // The version that finished, and nothing else — in particular not the application version, + // which would tell the next run there is nothing left to do. + $configData->expects($this->once())->method('setAppVersion')->with('400.00000001'); + $this->config->expects($this->once())->method('save'); + + $this->upgrade->registerUpgradeHandler(UpgradeHandlerStub::class); + + $this->expectException(UpgradeException::class); + + $this->upgrade->upgrade('400.00000000', $configData); + } + + /** + * And two versions that both finish are recorded in order, oldest first, before the run stamps + * the application version it reached. + * + * The order is not incidental: the value written after each version is a resume point, so + * applying a lower version after a higher one would move it backwards. It used to be whatever + * order the handlers were registered and their attributes declared in. + * + * @throws Exception + * @throws ServiceException + * @throws FileException + * @throws InvalidClassException + */ + public function testTheVersionsAreAppliedOldestFirst() + { + $configData = $this->createStub(ConfigDataInterface::class); + + $applied = []; + $handler = $this->createMock(UpgradeHandlerService::class); + $handler->method('apply')->willReturnCallback( + static function (string $version) use (&$applied): bool { + $applied[] = $version; + + return true; + } + ); + + $this->container->method('get')->willReturn($handler); + + $this->upgrade->registerUpgradeHandler(UpgradeHandlerStub::class); + $this->upgrade->upgrade('400.00000000', $configData); + + self::assertSame(['400.00000001', '400.00000002'], $applied); + } + /** * @throws Exception * @throws ServiceException @@ -168,9 +246,12 @@ public function testUpgradeWithHandlerWithFailedApply() { $configData = $this->createStub(ConfigDataInterface::class); $handler = $this->createMock(UpgradeHandlerService::class); + // The oldest version outstanding, not whichever the stub happens to declare first: the + // handlers now run in ascending order, because the resume point written after each one + // must not go backwards. $handler->expects($this->once()) ->method('apply') - ->with('400.00000002', $configData) + ->with('400.00000001', $configData) ->willReturn(false); $this->container