Conversation
… fails The ee-custom-container-migrations step had no undo, so when a later step (e.g. the image migration after a failed pull) failed, the container migrations of that run stayed applied and recorded while the old images kept running. For v4.13.0 this leaves the auth migration's _wildcard.X files on the old nginx-proxy template, which applies them to sibling sites. The step now reverts the container migrations completed by the same invocation, newest first: it calls their down() and deletes their migrations rows so the next attempt runs them again. Migrations recorded by earlier runs are never touched. A down() that throws is logged and doesn't stop the others, and the undo runs at most once. On a fresh install nothing is reverted, as before.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The rollback behavior is sound; only a non-blocking misleading debug message remains.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds rollback support for container migrations when later upgrade steps fail.
Changes:
- Tracks migrations completed during the current run.
- Reverts them in reverse order and removes their database records.
| File | Description |
|---|---|
php/EE/Runner.php |
Registers the container migration rollback callback. |
php/EE/Migration/CustomContainerMigrations.php |
Tracks and reverts newly executed migrations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| foreach ( Migration::where( 'migration', $name ) as $row ) { | ||
| $row->delete(); | ||
| } | ||
| EE::debug( "Reverted: $name" ); |
When a container migration's down() threw during the undo, the log still said "Reverted: X" after the warning. It now says "Reverted" only when down() succeeded, and "Removed the migrations row of X" otherwise.
…ration succeeds Adds an ee-after-docker-image-migrations step right after ee-docker-image-migrations that runs EE::do_hook( 'after_docker_image_migration' ). The updated global containers, e.g. the new nginx-proxy, run from that point, so packages can apply changes that must wait for them. auth-command uses it to apply the _wildcard.* auth files it staged while the old nginx-proxy template ran. The hook also fires when no image changed, e.g. on nightly re-runs. The step has no undo: if a later step fails, the container migrations are reverted as before, which undoes what their hooks applied.
…s run The container migrations' undo also ran when a step after ee-docker-image-migrations failed, e.g. ee-update-docker-compose. That step has no undo, so the new images, the recreated containers and their DB tags stayed, and reverting the container migrations put their old state onto the upgraded containers. For the auth migration, this restored the old-template auth files on the new nginx-proxy, so subsites and aliases were unprotected until a successful re-run. A new ee-keep-container-migrations step right after the image migration clears the list of container migrations to revert. A later failure, including one in an after_docker_image_migration listener, now keeps them and their migrations rows. A failure up to and including the image migration still reverts them as before.
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.
Problem
Runner::migrate()runs the container migrations (ee-custom-container-migrations) before the image migration, but that step has no undo. When a later step fails, for exampleUnable to pull easyengine/nginx-proxy, the rollback reverts nothing. The container migrations of that run stay applied and recorded inmigrations,options.versionstays old, and the old images keep running with the new files. The next attempt never runs those migrations again.In v4.13.0 this is what made the auth-command migration unsafe. Its
_wildcard.Xhtpasswd/ACL files stayed on the old nginx-proxy template, which applies_wildcard.Xto every subdomain of X that has no file of its own. A sibling site such asshop.<multisite>then asked for the multisite's password until an upgrade finally succeeded.Packages also had no point in the upgrade at which they could apply changes that need the new global containers, e.g. the new nginx-proxy template.
Fix
CustomContainerMigrationsrecords the container migrations completed by the current invocation.revert_executed_migrations()is registered as the undo ofee-custom-container-migrations. When the image migration (or an earlier step) fails, it callsdown()on those migrations, newest first, and deletes theirmigrationsrows so the next attempt runs them again. Migrations recorded by earlier runs are never touched.down()that throws is logged as a warning and doesn't stop the others. Its row is still deleted, since migrations are idempotent and must re-run on the retry. The log then saysRemoved the migrations row of X;Reverted: Xonly appears whendown()succeeded.options.version): nothing is reverted, as before.ee-keep-container-migrations, right afteree-docker-image-migrations, clears that list. The image migration has no undo: the new images, the recreated containers and their DB tags stay. Reverting the container migrations after it would put their old state onto the upgraded containers (for auth-command: old-template auth files on the new proxy, so subsites and aliases lose their protection). A failure in a later step (ee-update-docker-compose, cron config, logrotate, or anafter_docker_image_migrationlistener) now keeps them and their rows. The retry doesn't re-run them.after_docker_image_migration, fired by the stepee-after-docker-image-migrationsright afteree-keep-container-migrations. The updated global containers run from that point. It also fires when no image changed (e.g. nightly re-runs). fix(migration): don't apply wildcard auth files on the old nginx-proxy template auth-command#58 uses it to apply the_wildcard.*auth files it staged while the old proxy template ran.Pipeline:
ee-db-migrations→ee-custom-container-migrations(undo: revert) →ee-docker-image-migrations→ee-keep-container-migrations→ee-after-docker-image-migrations→ee-update-docker-compose→ee-update-cron-config→ee-setup-logrotate.Every container migration new in v4.13.0 was checked (
git diff v4.12.0..develop -- migrationsin core and each bumped package's tag range). Only auth-command's20260924120000_auth-command_regenerate_site_auth_files.phpis new, and it gets a realdown()in EasyEngine/auth-command#58.Not covered: steps that call
EE::error()(process exit) instead of throwing still skip every undo (pre-existing). A failed pull throws, so it is covered.Tests
Isolated harness (throwaway sqlite, fake packages, the real
RevertableStepProcessor/CustomContainerMigrations/models; PHP 7.4 and 8.5, 34/34). It covers:down()runs newest first for this run's migrations only, their rows are deleted, and a throwingdown()is logged as "row removed" while the next one is still reverted; a second undo is a no-op; the retry re-runs them;Runner::migrate().Live upgrade from 4.12.0, with a phar of develop + this PR + fix(migration): don't apply wildcard auth files on the old nginx-proxy template auth-command#58 + fix(migration): reload site nginx after its containers are recreated #1937 built the way CI builds the nightly. The host had a WP subdomain multisite with site auth and a whitelist, a sibling
shop.<multisite>without auth, a site withaliasand*.Xaliases plus auth and a whitelist, WP--cache, PHP with a DB, php 8.1, php 7.4 and html sites. A probe curled the public IP every 0.5 s without credentials, and container events were forced every ~5 s during every run (adocker run --rmand adocker restartof an unrelated container).shop.never 401 (0/88), auth migration reverted: htpasswd/ and ACL files byte-identical to before (content, mode, owner, mtime), row removed, version stays 4.12.0shop.never 401 (0/113), subsites and aliases protected after promotion, content identical, 0No route to hostee-update-docker-composefails after the image migrationeekilled mid-pull, then retryshop.never 401 (0/72 while held with events); the retry completes the upgrade.backupunchangedThe TESTING-NOTES §1 auth matrix passed after the upgrade, and so did create/delete of every site type.
Behat (
@migration, on the earlier head): a probe container migration and a blocked registry. The probe'sdown()runs, its row is removed and the version stays old; after unblocking, the probe runs again and the version is bumped. CI: https://github.com/mrrobot47/easyengine/actions/runs/36123058489.php -lon PHP 7.4 and 8.5; no new PHPCS violations.Release
This should land in v4.13.0, together with auth-command v1.3.1 (EasyEngine/auth-command#58, a patch tag). Bump core's auth-command pin to v1.3.1 before core v4.13.0 is tagged. It combines cleanly with #1937.