From 7a862c5b70d2ec9c23fda3a740a02d93de39f000 Mon Sep 17 00:00:00 2001 From: blaipr Date: Tue, 8 Sep 2026 01:45:08 +0200 Subject: [PATCH] fix: an interrupted upgrade resumes where it stopped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upgrade::upgrade() decided what still needed running from appVersion, and wrote that only after every handler had succeeded — while progress was really being stamped per file, with UpgradeDatabase::apply() setting databaseVersion for each migration as it completed. 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 permanently — the exact failure that file's own atomicity fix exists to prevent, reopened at the orchestrator — and UpgradeConfigText would decode text that is already decoded, which its header says must happen exactly once. Nothing in the upgrade calls set_time_limit(0), though BackupFile, XmlExport, AccountMasterPassword and Import all do, so max_execution_time alone reaches it. appVersion now advances as each version finishes. That forces versions to be applied oldest first — a resume point that goes backwards is worse than one that never moves — which also closes a latent fragility: the order used to be whichever way the handlers were registered and their attributes declared. Handlers are grouped by version because two can share one. Two things the restructure could have lost and does not: handlers are still resolved lazily, so one an earlier failure means we never reach is never constructed; and a container failure is still converted to the application's own ServiceException. --- CLAUDE.md | 15 ++++ src/Domain/Upgrade/Services/Upgrade.php | 72 +++++++++++++--- .../Domain/Upgrade/Services/UpgradeTest.php | 83 ++++++++++++++++++- 3 files changed, 159 insertions(+), 11 deletions(-) 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