fix: an interrupted upgrade resumes where it stopped

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

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,23 +143,48 @@ public function upgrade(string $version, ConfigDataInterface $configData): void
     }
 
     /**
-     * @return iterable<array{string, UpgradeHandlerService}>
+     * 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<string, class-string<UpgradeHandlerService>[]>
      * @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<UpgradeVersion> $attribute */
                 foreach ($reflection->getAttributes(UpgradeVersion::class) as $attribute) {
                     $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);
         }

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