fix: an interrupted upgrade resumes where it stopped - #917
Merged
Conversation
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
deleted the
fix/an-interrupted-upgrade-resumes-where-it-stopped
branch
September 7, 2026 23:53
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upgrade::upgrade()decides what still needs running fromappVersion:…and that line runs only after every handler has succeeded. Progress, meanwhile, is really being
stamped per file:
UpgradeDatabase::apply()setsdatabaseVersionfor each migration as itcompletes, 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.sqldrops a column that is no longer there — it fails, permanently, and the upgradecan 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.
UpgradeConfigTextwould decode text that is already decoded. Its own header records that it isnot idempotent and no decode of stored text could be.
The trigger does not need bad luck:
docker/php/syspass.inisetsmax_execution_time = 120, andnothing in
Upgrade,UpgradeDatabaseorUpgradeControllercallsset_time_limit(0)— althoughBackupFile,XmlExport,AccountMasterPasswordandImportall do.40024240101.sqlruns eightleading-wildcard
LIKE '%&%'scans overAccountandAccountHistoryamong others.The change
appVersionis written as each version finishes, so a retry resumes where the work actuallystopped.
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 (
UpgradeDatabaseandUpgradeConfigTextboth carry400.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
container->get()wouldconstruct 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.get()used to sit inside thegenerator's
try, which convertsThrowabletoServiceException; moving it into the loop lostthat until the same conversion was put back.
testUpgradeWithExceptionis what caught it.Tests
would tell the next run there is nothing left to do;
Both fail against the old implementation.
testUpgradeWithHandlerWithFailedApplychanged itsexpectation from
400.00000002to400.00000001— it was encoding the stub's declaration order,which is the fragility this removes.