Skip to content

fix: an interrupted upgrade resumes where it stopped - #917

Merged
blaipr merged 1 commit into
mainfrom
fix/an-interrupted-upgrade-resumes-where-it-stopped
Sep 7, 2026
Merged

fix: an interrupted upgrade resumes where it stopped#917
blaipr merged 1 commit into
mainfrom
fix/an-interrupted-upgrade-resumes-where-it-stopped

Conversation

@blaipr

@blaipr blaipr commented Sep 7, 2026

Copy link
Copy Markdown
Member

Upgrade::upgrade() decides what still needs running from appVersion:

foreach ($this->getTargetUpgradeHandlers($version) as [$targetVersion, $upgradeHandler]) { … }

$configData->setAppVersion(Version::getVersionStringNormalized());   // ← only here

…and that line runs only after every handler has succeeded. Progress, meanwhile, is really being
stamped per file: UpgradeDatabase::apply() sets databaseVersion for each migration as it
completes, and the loop saves the config after each handler.

So an interruption between two versions leaves a database that is already migrated and a resume
point that has not moved. The retry re-derives the same starting point and re-runs a migration that
has already been applied:

  • 40024210101.sql drops a column that is no longer there — it fails, permanently, and the upgrade
    can then be neither finished nor repeated. That is the exact failure mode that file's own
    atomicity fix exists to prevent, reopened one level up at the orchestrator.
  • UpgradeConfigText would decode text that is already decoded. Its own header records that it is
    not idempotent and no decode of stored text could be.

The trigger does not need bad luck: docker/php/syspass.ini sets max_execution_time = 120, and
nothing in Upgrade, UpgradeDatabase or UpgradeController calls set_time_limit(0) — although
BackupFile, XmlExport, AccountMasterPassword and Import all do. 40024240101.sql runs eight
leading-wildcard LIKE '%&%' scans over Account and AccountHistory among others.

The change

appVersion is written as each version finishes, so a retry resumes where the work actually
stopped.

That forces one other thing: versions must be applied oldest first. A resume point that goes
backwards is worse than one that never moves, and a migration must not run before one that precedes
it anyway. The order used to be whichever way the handlers happened to be registered and their
attributes declared — ascending today by luck, and nothing required it. Handlers are grouped by
version because two can share one (UpgradeDatabase and UpgradeConfigText both carry
400.24240101) and the resume point may only advance once both have run.

Writing it inside the loop is safe: the grouped list is built from the original version before the
loop starts, so only a later run sees the advanced value.

Two things kept that the restructure could have lost

  • Handlers are still resolved lazily. Building the group map with container->get() would
    construct handlers that an earlier failure means we never reach; the existing test noticed
    immediately (get() … not expected to be called more than once). The map holds class names.
  • A container failure is still the application's own exception. get() used to sit inside the
    generator's try, which converts Throwable to ServiceException; moving it into the loop lost
    that until the same conversion was put back. testUpgradeWithException is what caught it.

Tests

  • an interrupted run records the version that finished, and not the application version, which
    would tell the next run there is nothing left to do;
  • two versions that both finish are applied oldest first.

Both fail against the old implementation. testUpgradeWithHandlerWithFailedApply changed its
expectation from 400.00000002 to 400.00000001 — it was encoding the stub's declaration order,
which is the fragility this removes.

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.
@blaipr
blaipr merged commit 99e96d8 into main Sep 7, 2026
8 checks passed
@blaipr
blaipr deleted the fix/an-interrupted-upgrade-resumes-where-it-stopped branch September 7, 2026 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant