fix(migrations): a migration that ships without a version bump reaches nobody - #3451
Merged
Merged
Conversation
…s nobody Nextcloud reads an app's migration directory only when appinfo/info.xml's <version> is greater than the installed_version it recorded. Equal versions mean `occ upgrade` answers "No upgrade required.", exits 0, and opens no migration file at all. Nothing is logged, nothing fails, and the feature that needed the table is absent with no error anywhere. Measured on a throwaway NC 34.0.3 rig running openregister 2.0.15-unstable.20260905134511 from its release tarball: a migration added with <version> left alone did not run and was not recorded; changing nothing but <version> and re-running `occ upgrade` ran it. The code was byte-identical across the two runs. This is not hypothetical. development carried four migrations added since <version> last moved on 2026-09-03, one of them the run-lock table #3444 depends on, and an instance updated to that code got none of them. scripts/check-migration-version-bump.php fails when a branch adds a file under lib/Migration/ without moving <version> past its value at the merge base. It runs in Merge Hygiene on every push and PR, in composer check:strict, and as a warning from .githooks/pre-commit. Run against the real history it reds on exactly those four files. It exits 2, not 0, when it cannot resolve the base ref: a check that cannot see the base has no verdict, and a silent pass is the failure this removes. occ migrations:status cannot be used for this and is documented as such rather than worked around. It is Nextcloud's command and three of its five counting fields are wrong: Pending Migrations reads None for this app always, because core filters the list on \$migration->name() and SimpleMigrationStep::name() returns '' for all 204 of ours; New Migrations and Executed Unavailable both call array_keys() on a list and so diff version strings against 0..n. The only honest pair is Executed against Available, which is what read 204 of 205 on the rig while the line below it said nothing was pending. Refs #3444
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| test-l10n-parity | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 174/174 | |||
| npm | ✅ | ✅ 543/543 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-05 14:42 UTC
Download the full PDF report from the workflow artifacts.
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.
A migration that ships without a version bump reaches nobody
Found while updating a live instance.
developmenthad picked up new migrationsbut the app version had not moved, so
occ upgraderan none of them — includingthe one that creates the run-lock table #3444 depends on. The feature was simply
absent, with no error anywhere.
Reproduced, not inherited
Throwaway NC 34.0.3 rig on a free port, openregister
2.0.15-unstable.20260905134511installed from its release tarball, 204migrations,
installed_versionequal to<version>.Version1Date20260906090000.php(creates a table), leave<version>alone,occ upgradeNo upgrade required.exit 0information_schema.tables)oc_migrations?occ app:update openregisterup-to-date or no updates could be found, still no table<version>(2.0.15-unstable… → 2.0.16),occ upgradeUpdated <openregister> to 2.0.16The code was byte-identical across the failing and passing runs. One line of XML
is the entire gate.
And the instrument agreed with the failure:
Which half is the defect
The version discipline. A PR that adds a migration must move
<version>inthe same change, and until now nothing said so or checked it. Measured on the
real history:
<version>last moved on 2026-09-03 (85e8321), anddevelopmenthas added four migrations since, none of which could reach an instance
already on that version.
Waiting for the release bump PR is not enough, because between releases
developmentis what people deploy — and those bump PRs queue up (#3450 is openright now).
The fix fits the pattern rather than inventing a third mechanism. sc#396's answer
— derive the gate value from the content it gates — does not transfer here: this
value is a published App Store semver, not an internal signature, so it cannot be
computed. dossiq#1786's answer applies instead: a deliberate bump, in the same
change, now with something mechanical behind it.
The reporting is Nextcloud's, so it is documented rather than worked around.
Three of the five counting fields in
occ migrations:statusare wrong, and allthree defects are in core:
Pending Migrations: None—MigrationService::describeMigrationStep()builds its list under
if ($migration->name()), andSimpleMigrationStep::name()returns''by default. None of our 204migrations override it, so this field reads
Nonefor openregister always,whatever the database says. It is not a wrong answer about this branch; it is a
field that has never once been able to answer.
New Migrations/Executed Unavailable Migrations—StatusCommand::getMigrationsInfos()callsarray_keys()ongetAvailableVersions(), which returns a list. Both fields therefore diffversion strings against the integers
0..nand report the full count on everyrun. That is why the rig showed 204 and 205 with nothing wrong.
describeMigrationStep('lastest')— the alias is misspelled in core. Harmlessonly by luck:
sortMigrations()falls through tostrnatcmp, andVersion…sorts below
lastest.The only honest pair in that output is
ExecutedagainstAvailable. 204 of205 means one migration has not run, whatever the line below it says. That is now
written down in
lib/Migration/NAMING.md, along with the recovery — bump andocc upgrade, orocc migrations:migrate openregister(verified on the rig: itran the pending step and created the table). Both
migrations:*commands needdebug: truein config.php oroccanswersCommand "migrations:status" is not defined.The mechanical check
scripts/check-migration-version-bump.phpfails when a branch adds a file underlib/Migration/without moving<version>past its value at the merge base.Committed, staged and untracked additions all count, and
--no-renamesisdeliberate: renaming an applied migration makes Nextcloud re-run it, which needs
a bump and a human.
It runs in Merge Hygiene (every push and PR, milliseconds), in
composer check:strict, and as a warning from.githooks/pre-commit.It exits 2, not 0, when it cannot resolve the base ref or parse
info.xml.A check that cannot see the base has no verdict to give, and a silent pass is
precisely the failure this removes.
Run against the real history that caused the incident (
--base=85e8321) it redsand names all four files.
Tests, proven red
tests/Unit/Migration/MigrationVersionBumpCheckTest.php— 8 cases, each over apurpose-built git repository, so a green is the check looking and finding
nothing rather than the branch happening to have no migrations: the defect, the
fix, a prerelease bump (
2.0.14-unstable.…→2.0.15-unstable.…), a controlthat edits other files and passes, a lowered version, an untracked migration,
an unresolvable base ref, and a non-repository.
Proven red by mutation, not by assertion:
The sweep
Same shape elsewhere — the fleet gating work on a version string a change can
forget to move:
by a content hash now, not a version (
ImportHandler~L2147, Added property close-after-click to every NcActionButton #426). That isthe sc#396 answer, already landed.
ImportHandler(L844, L998, L1873, L2872, L3491) compare versions with nocontent escape — the dossiq#1786 shape.
pass a
REGISTER_VERSIONPHP constant to that gate. Three of the nine(descriptor, version) pairs disagree with the descriptor's own
info.version:ImportCredentialBrokerRegister(const1.0.0, JSON1.4.0),and both
ImportDsarRegistersentries (const1.2.0/1.1.0, JSON1.0.0).The constant is what gates; the JSON version is read by
RegisterDescriptorService::inventory()for the admin panel, so the panel andthe importer can disagree about which version an instance is on. Filed
separately rather than folded in here — deciding which of those numbers is
canonical is a design ruling, not a gate.
Verified locally (CI is bottlenecked), by exit code
composer lintcomposer check:migration-versioncomposer phpcscomposer psalmcomposer phpstancomposer test:unitphpmd lib/Migration--scope-to-diff(v1.15.1)composer test:unitexits 1 on this host for an environmental reason, not afailure: PHPUnit's runner warning
No code coverage driver availableunderfailOnWarning. Reproduced on an untouched pre-existing test file(
tests/Unit/SearchControllerTest.php, exit 1, same single warning, 0 failures).CI has a coverage driver.
Not run locally: the Integration, Database and Service suites (they boot a
Nextcloud server) and Playwright.
Refs #3444
🤖 Generated with Claude Code