Merge pull request #8930 from dokku/8929-0-38-26-regression-drainlegacyenvfile-silently-overwrites-current-config-with-the-stale-legacy-env-file

fix: do not import an already-migrated ENV file

docs/appendices/0.38.0-migration-guide.md

@@ -15,7 +15,7 @@
     }
     ```
 
-- The path on disk to both the global `ENV` file and app `ENV` files have been moved. Users should reference environment variables via the provided plugin triggers rather than directly sourcing the ENV files. Existing ENV files are merged into the new location and removed once they have been drained. **Changed in 0.38.26:** removal previously happened on the subsequent Dokku install for app ENV files, and never happened at all for the global ENV file. An ENV file that reappears at the old path afterwards is treated as a hand-edit: its contents are merged into the new location and the keys are named in a warning rather than being discarded.
+- The path on disk to both the global `ENV` file and app `ENV` files have been moved. Users should reference environment variables via the provided plugin triggers rather than directly sourcing the ENV files. Existing ENV files are merged into the new location and removed once they have been drained. **Changed in 0.38.26:** removal previously happened on the subsequent Dokku install for app ENV files, and never happened at all for the global ENV file. **Changed in 0.38.27:** an ENV file found at the old path after its migration has been recorded is never merged into the new location, because the new location holds every change made since. Such a file is removed when its values agree with the current config, and otherwise moved aside to `ENV.migrated` with the keys it disagrees on named in a warning, so its values can still be applied by hand with `dokku config:set`. That copy is left for review and should be deleted afterwards: Dokku no longer reads it, and it holds everything it was not allowed to import - including keys that were unset on purpose, which for a revoked secret means a copy lingering on disk.
 - During a fresh apt install, the upstream nginx default vhost files (`/etc/nginx/sites-enabled/default`, `/etc/nginx/sites-available/default`, and `/etc/nginx/conf.d/default.conf`) are renamed to `${path}.dokku-disabled` (not deleted) to avoid a `duplicate default server for 0.0.0.0:80` error. Operators with local customizations can recover them by inspecting the `.dokku-disabled` siblings. Upgrade-in-place installs do not touch any existing nginx files.
 - Fresh apt installs now ship a catch-all default site at `/etc/nginx/conf.d/00-default-vhost.conf` that rejects requests with unknown Host headers using `ssl_reject_handshake on` (HTTPS) and `return 444` (HTTP). This replaces the manual workaround previously documented in the nginx docs. The behavior can be opted out at install time via the `dokku/install_default_site` debconf prompt. On nginx older than 1.19.4 (e.g., Debian Bullseye's nginx 1.18.0), the postinst installs an HTTP-only variant of the catch-all that omits the SSL listener and `ssl_reject_handshake`, since that directive is unsupported on those versions. See the [Default site documentation](/docs/networking/proxies/nginx.md#default-site).
 - The `docker-local` scheduler now sends `SIGTERM` to old containers immediately after a successful deploy, rather than waiting `wait-to-retire` seconds before signaling. This matches Heroku's graceful-shutdown contract and lets applications begin draining in-flight work as soon as proxy traffic switches. The `wait-to-retire` grace period and `stop-timeout-seconds` hard-stop continue to apply as before. See the [zero downtime deploys documentation](/docs/deployment/zero-downtime-deploys.md#wait-to-retire) for more details.
@@ -29,6 +29,7 @@
 - **New in 0.38.25:** Values supplied through docker options, `dokku run`'s `-e`/`--env` flag, and `--ttl-seconds` are no longer evaluated by the shell when assembling a container's arguments; they are now tokenized and passed through verbatim. This closes a command-injection vector where a `$(...)` or backtick expression in one of these values executed on the host as the `dokku` user during build, deploy, or run. As a result, shell metacharacters such as `$(...)`, backticks, `$VAR`, and globs in these values are treated literally instead of being expanded, and `--ttl-seconds` must now be a plain integer. Existing Traefik docker-options labels (those whose label key begins with `traefik.`) whose backticks were stored with a stray backslash are repaired automatically the first time `dokku` runs after the upgrade, so they become valid on the next deploy.
 - **New in 0.38.26:** Docker options drained out of the pre-0.38.0 `DOCKER_OPTIONS_<PHASE>` files were copied verbatim rather than being re-serialized the way `docker-options:add` stores them, so `docker-options:remove` compared the canonical string it builds against a value that could never match it and exited successfully without removing anything. Removal now matches stored options by shell word instead of by exact string, so those entries can be removed with the value as originally written. Stored options are also rewritten into the canonical form the first time `dokku` runs after the upgrade: values whose shell metacharacters were left unquoted are quoted, and an entry that carried several flags on one line becomes one entry per flag, which additionally fixes `ps:report` reading a restart policy off such a line and the `k3s` scheduler translating only the first of several `--cap-add`/`--sysctl` flags. Options added through `docker-options:add` are already canonical and are left untouched.
 - **New in 0.38.26:** Deprecated `DOKKU_*` config variables belonging to plugins whose install step runs before the `config` plugin's were not migrated to their plugin property on the upgrade run. Install steps fire in alphabetical order, so `apps`, `builder`, and `checks` read an app's environment before the `config` plugin had relocated the `ENV` file, found nothing, and moved nothing - without reporting anything. `dokku config:show` kept listing the variable while the plugin behaved as though it were unset, which for `DOKKU_CHECKS_SKIPPED` meant a process type silently regained a health check it was meant to skip. The relocation is now performed before any deprecated variable is read, regardless of install order. Affected installs recover on their next `dokku plugin:install --core`, which the upgrade already runs; the variables listed below can also be re-applied by hand using their replacement command.
+- **New in 0.38.27:** The 0.38.26 upgrade merged the leftover pre-0.38.0 `ENV` file back over the current config. Releases 0.38.0 through 0.38.25 recorded the migration and left that file on disk on purpose, and 0.38.26 read it as a hand-edit, so the environment as it stood at the 0.38.x upgrade won over every `dokku config:set` and `dokku config:unset` made since - including reinstating variables and secrets that had been deliberately unset. Nothing appeared broken, because running containers keep the environment they were created with; the rewound values would have reached containers on the next deploy. The 0.38.26 run consumed the leftover file, so this cannot happen a second time, but it also cannot be undone automatically. To check an app that was upgraded to 0.38.26 and has a container still running from before that upgrade, compare each key in `dokku config:show <app>` against `docker exec <container> printenv <key>` - `GIT_REV` is expected to differ after a deploy, anything else that differs was rewound. Repair with `dokku config:set --no-restart <app> KEY=VALUE` for values that regressed and `dokku config:unset --no-restart <app> KEY` for variables that came back.
 - The storage plugin now treats persistent volumes as named, scheduler-aware first-class resources via `storage:create`, `storage:mount`, `storage:set`, and `storage:destroy`. The legacy `storage:mount <app> <host>:<container>` colon form continues to work on docker-local apps but is deprecated; on k3s apps it is rejected. Existing colon-form mounts are migrated automatically the first time the new storage plugin runs (during the install trigger) - they appear as `legacy-<hash>` entries in `storage:list-entries`. The migration is idempotent and tied to a per-app flag file at `$DOKKU_LIB_ROOT/config/storage/.migrated/<app>`; deleting that file forces a re-scan on the next install. The `storage:ensure-directory` command keeps working but now emits a deprecation warning - prefer `storage:create <name> [<path>]` (the path defaults to the same `$DOKKU_LIB_ROOT/data/storage/<name>` location). Storage entry names must now be DNS-1123 labels of 45 characters or less so they can be used verbatim as Helm release and Kubernetes resource names; underscores and uppercase characters that the older `ensure-directory` validator accepted are rejected for new names. The migration synthesizer always uses lowercase hex hashes so existing data is never locked out.
 
 ### TLS handshake behavior change

docs/configuration/environment-variables.md

@@ -151,4 +151,4 @@ The following property is recorded internally by the config plugin and is not ex
 
 | Property | Description | Source |
 |---|---|---|
-| `env-migrated` | Migration sentinel that records that the per-app or global `ENV` file has been drained out of its pre-0.38.0 location into the config path. The old file is removed as soon as it has been drained, so one that reappears afterwards was written by hand rather than through `dokku config:*`; its contents are merged in and named in a warning on the next migration pass | `plugins/config/migrate.go` writes `"true"` once the file has been drained |
+| `env-migrated` | Migration sentinel that records that the per-app or global `ENV` file has been drained out of its pre-0.38.0 location into the config path. The drain happens once: a file found at the old path once this is recorded is never imported, since the config path holds every change made since. Such a file is removed when it agrees with the current config, and otherwise moved aside to `ENV.migrated` with the keys it disagrees on named in a warning. The preserved copy is left for review and should be deleted afterwards, since it holds keys that were unset on purpose | `plugins/config/migrate.go` writes `"true"` once the file has been drained |

docs/development/plugin-triggers.md

@@ -583,7 +583,7 @@ set -eo pipefail; [[ $DOKKU_TRACE ]] && set -x
 
 ### `config-migrate-env`
 
-- Description: Drains the pre-0.38 `$DOKKU_ROOT/ENV` and `$DOKKU_ROOT/<app>/ENV` files into the config property path, removing each file once it has been drained. Idempotent, and safe to call from an install trigger that runs before the config plugin's own.
+- Description: Drains the pre-0.38 `$DOKKU_ROOT/ENV` and `$DOKKU_ROOT/<app>/ENV` files into the config property path, removing each file once it has been drained. A file found at the old path after its migration has been recorded is never imported: the config path holds every change made since, so the file is removed when it agrees with the current config and otherwise moved aside to `ENV.migrated`. Idempotent, and safe to call from an install trigger that runs before the config plugin's own.
 - Invoked by: `common` when migrating deprecated config vars to plugin properties, checks plugin
 - Arguments: none
 - Example:

plugins/common/properties.go

@@ -706,9 +706,15 @@ func MigrateConfigToProperties(pluginName string, entries []MigrateConfigEntry)
 // A failure here is not fatal: the migration is retried on the next install, so
 // an unavailable trigger degrades to the previous behavior rather than aborting
 // every plugin's install.
+//
+// The trigger's stderr is streamed rather than captured. This is the first call
+// to reach the migration on an upgrade, so anything it has to report about the
+// files it found is only ever said once, and a captured warning would be
+// discarded with the response.
 func migrateLegacyEnvFiles() {
 	if _, err := CallPlugnTrigger(PlugnTriggerInput{
-		Trigger: "config-migrate-env",
+		Trigger:      "config-migrate-env",
+		StreamStderr: true,
 	}); err != nil {
 		LogWarn(fmt.Sprintf("Unable to migrate legacy env files: %s", err.Error()))
 	}

plugins/config/migrate.go

@@ -86,9 +86,11 @@ func migrateAppEnv(appName string) error {
 // removed once the merged environment has been written successfully, so a parse
 // or write failure leaves the original in place.
 //
-// A legacy file that turns up after the migration has already been recorded was
-// written by hand rather than through `dokku config:*`, so its contents are
-// merged in and named in a warning rather than being discarded silently.
+// The merge happens once and only once. A legacy file that is still there when
+// the migration is already on record is handed to preserveStaleEnvFile instead:
+// releases 0.38.0 through 0.38.25 recorded the migration and left the file
+// behind on purpose, so importing it would replay the environment as it stood
+// at that upgrade over every config:set and config:unset made since.
 func drainLegacyEnvFile(name string, oldEnvFile string, env *Env) error {
 	migrated := common.PropertyGetDefault("config", name, envMigratedProperty, "") == "true"
 
@@ -99,15 +101,15 @@ func drainLegacyEnvFile(name string, oldEnvFile string, env *Env) error {
 		return writeEnvMigrated(name)
 	}
 
+	if migrated {
+		return preserveStaleEnvFile(name, oldEnvFile, env)
+	}
+
 	oldEnv, err := loadFromFile(name, oldEnvFile)
 	if err != nil {
 		return fmt.Errorf("Unable to load old environment: %s", err.Error())
 	}
 
-	if migrated && oldEnv.Len() > 0 {
-		common.LogWarn(fmt.Sprintf("Importing %s written outside of dokku config: %s", oldEnvFile, strings.Join(oldEnv.Keys(), " ")))
-	}
-
 	env.Merge(oldEnv)
 	if err := env.Write(); err != nil {
 		return fmt.Errorf("Unable to write environment: %s", err.Error())
@@ -131,6 +133,56 @@ func drainLegacyEnvFile(name string, oldEnvFile string, env *Env) error {
 	return nil
 }
 
+// preserveStaleEnvFile clears a legacy ENV file that outlived its own migration
+// out of the way without importing any of it. A file whose values all match the
+// current environment has nothing left to give and is removed outright;
+// otherwise it is moved alongside itself with a .migrated suffix and the keys it
+// disagrees on are named, so an operator who did write it by hand can still
+// recover the values through `dokku config:set`.
+//
+// The preserved copy is the operator's to delete. It holds everything the file
+// was not allowed to import, which includes keys that were unset on purpose, so
+// leaving a revoked secret sitting in a file dokku no longer reads is not what
+// anyone wants once its contents have been reviewed.
+func preserveStaleEnvFile(name string, oldEnvFile string, env *Env) error {
+	staleEnv, err := loadFromFile(name, oldEnvFile)
+	if err != nil {
+		return fmt.Errorf("Unable to load stale environment: %s", err.Error())
+	}
+
+	diverged := divergedKeys(staleEnv, env)
+	if len(diverged) == 0 {
+		if err := os.Remove(oldEnvFile); err != nil {
+			return fmt.Errorf("Unable to remove stale file %s: %s", oldEnvFile, err.Error())
+		}
+		return nil
+	}
+
+	preservedFile := oldEnvFile + ".migrated"
+	common.LogWarn(fmt.Sprintf("Not importing already-migrated ENV file %s: the current config takes precedence for %s", oldEnvFile, strings.Join(diverged, " ")))
+	common.LogWarn(fmt.Sprintf("Preserved at %s: re-apply anything still needed with dokku config:set, then delete it - dokku does not read it and it may hold values that were unset on purpose", preservedFile))
+
+	if err := os.Rename(oldEnvFile, preservedFile); err != nil {
+		return fmt.Errorf("Unable to preserve stale file %s: %s", oldEnvFile, err.Error())
+	}
+
+	return nil
+}
+
+// divergedKeys returns the keys stale holds that env either does not have at all
+// or holds a different value for.
+func divergedKeys(stale *Env, env *Env) []string {
+	diverged := []string{}
+	for _, key := range stale.Keys() {
+		staleValue, _ := stale.Get(key)
+		if value, ok := env.Get(key); !ok || value != staleValue {
+			diverged = append(diverged, key)
+		}
+	}
+
+	return diverged
+}
+
 func writeEnvMigrated(name string) error {
 	if err := common.PropertyWrite("config", name, envMigratedProperty, "true"); err != nil {
 		return fmt.Errorf("Unable to set %s property: %s", envMigratedProperty, err.Error())

plugins/config/migrate_test.go

@@ -49,6 +49,24 @@ func writeLegacyAppEnv(t *testing.T, dokkuRoot, appName, contents string) string
 	return path
 }
 
+// expectPreservedKey asserts the stale ENV file moved aside at path still holds
+// the given key, so the values it was not allowed to import remain recoverable.
+func expectPreservedKey(t *testing.T, path, key, want string) {
+	t.Helper()
+
+	preserved, err := loadFromFile("preserved", path)
+	if err != nil {
+		t.Fatalf("loadFromFile(%s): %v", path, err)
+	}
+	got, ok := preserved.Get(key)
+	if !ok {
+		t.Fatalf("expected %s to hold %s", path, key)
+	}
+	if got != want {
+		t.Errorf("%s in %s = %q, want %q", key, path, got, want)
+	}
+}
+
 func expectEnvValue(t *testing.T, appName, key, want string) {
 	t.Helper()
 	got, ok := Get(appName, key)
@@ -102,11 +120,10 @@ func TestMigrateEnvFiles_DrainsAndRemovesGlobalFile(t *testing.T) {
 	}
 }
 
-// TestMigrateEnvFiles_ImportsHandEditedFile covers a legacy file that turns up
-// after the migration was already recorded, which means it was written by hand
-// rather than through `dokku config:*`. Its contents are merged in rather than
-// discarded.
-func TestMigrateEnvFiles_ImportsHandEditedFile(t *testing.T) {
+// TestMigrateEnvFiles_PreservesStaleFileWithoutImporting covers a legacy file
+// that is still there once the migration has been recorded. Nothing it holds is
+// imported, and it is moved aside so the values remain recoverable.
+func TestMigrateEnvFiles_PreservesStaleFileWithoutImporting(t *testing.T) {
 	dokkuRoot, _ := setupMigrateEnv(t)
 
 	writeLegacyAppEnv(t, dokkuRoot, "alpha", "export FIRST=one\n")
@@ -114,21 +131,129 @@ func TestMigrateEnvFiles_ImportsHandEditedFile(t *testing.T) {
 		t.Fatalf("first MigrateEnvFiles: %v", err)
 	}
 
-	legacy := writeLegacyAppEnv(t, dokkuRoot, "alpha", "export SECOND=two\n")
+	legacy := writeLegacyAppEnv(t, dokkuRoot, "alpha", "export FIRST=one\nexport SECOND=two\n")
 	if err := MigrateEnvFiles(); err != nil {
 		t.Fatalf("second MigrateEnvFiles: %v", err)
 	}
 
 	expectEnvValue(t, "alpha", "FIRST", "one")
-	expectEnvValue(t, "alpha", "SECOND", "two")
+	if _, ok := Get("alpha", "SECOND"); ok {
+		t.Errorf("did not expect SECOND to be imported from a stale legacy file")
+	}
+
 	if _, err := os.Stat(legacy); !os.IsNotExist(err) {
-		t.Errorf("expected %s to be removed, got err=%v", legacy, err)
+		t.Errorf("expected %s to be cleared out, got err=%v", legacy, err)
+	}
+	expectPreservedKey(t, legacy+".migrated", "SECOND", "two")
+}
+
+// TestMigrateEnvFiles_KeepsConfigSetAfterMigration covers the 0.38.26 regression
+// reported in #8929: releases 0.38.0 through 0.38.25 recorded the migration and
+// left the legacy file in place, so draining it a second time replayed the
+// environment as it stood at that upgrade over every config:set made since.
+func TestMigrateEnvFiles_KeepsConfigSetAfterMigration(t *testing.T) {
+	dokkuRoot, _ := setupMigrateEnv(t)
+
+	writeLegacyAppEnv(t, dokkuRoot, "alpha", "export DATABASE_URL=value-A\n")
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("first MigrateEnvFiles: %v", err)
+	}
+
+	if err := SetMany("alpha", map[string]string{"DATABASE_URL": "value-B"}, false, false); err != nil {
+		t.Fatalf("SetMany: %v", err)
+	}
+
+	legacy := writeLegacyAppEnv(t, dokkuRoot, "alpha", "export DATABASE_URL=value-A\n")
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("second MigrateEnvFiles: %v", err)
+	}
+
+	expectEnvValue(t, "alpha", "DATABASE_URL", "value-B")
+	expectPreservedKey(t, legacy+".migrated", "DATABASE_URL", "value-A")
+}
+
+// TestMigrateEnvFiles_DoesNotResurrectUnsetKeys covers the other half of #8929: a
+// key unset after the migration was recorded came back when the legacy file was
+// drained again, which for a rotated secret meant putting it back into service.
+func TestMigrateEnvFiles_DoesNotResurrectUnsetKeys(t *testing.T) {
+	dokkuRoot, _ := setupMigrateEnv(t)
+
+	writeLegacyAppEnv(t, dokkuRoot, "alpha", "export DOKKU_PROXY_PORT=80\n")
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("first MigrateEnvFiles: %v", err)
+	}
+
+	if err := UnsetMany("alpha", []string{"DOKKU_PROXY_PORT"}, false); err != nil {
+		t.Fatalf("UnsetMany: %v", err)
+	}
+
+	writeLegacyAppEnv(t, dokkuRoot, "alpha", "export DOKKU_PROXY_PORT=80\n")
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("second MigrateEnvFiles: %v", err)
+	}
+
+	if _, ok := Get("alpha", "DOKKU_PROXY_PORT"); ok {
+		t.Errorf("did not expect DOKKU_PROXY_PORT to be resurrected from a stale legacy file")
 	}
 }
 
-// TestMigrateEnvFiles_LegacyValueWins documents the merge precedence the
-// hand-edit path depends on: the legacy file overwrites the value already held
-// at the config path.
+// TestMigrateEnvFiles_RemovesStaleFileMatchingCurrentConfig covers the common
+// upgrade: the leftover file still agrees with the current config, so there is
+// nothing to preserve and no reason to say anything about it.
+func TestMigrateEnvFiles_RemovesStaleFileMatchingCurrentConfig(t *testing.T) {
+	dokkuRoot, _ := setupMigrateEnv(t)
+
+	writeLegacyAppEnv(t, dokkuRoot, "alpha", "export MY_VAR=value\n")
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("first MigrateEnvFiles: %v", err)
+	}
+
+	legacy := writeLegacyAppEnv(t, dokkuRoot, "alpha", "export MY_VAR=value\n")
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("second MigrateEnvFiles: %v", err)
+	}
+
+	expectEnvValue(t, "alpha", "MY_VAR", "value")
+	if _, err := os.Stat(legacy); !os.IsNotExist(err) {
+		t.Errorf("expected %s to be removed, got err=%v", legacy, err)
+	}
+	if _, err := os.Stat(legacy + ".migrated"); !os.IsNotExist(err) {
+		t.Errorf("did not expect %s.migrated to be left behind, got err=%v", legacy, err)
+	}
+}
+
+// TestMigrateEnvFiles_PreservesStaleGlobalFile covers the global file, which
+// releases 0.38.0 through 0.38.25 never removed at all, so every host upgraded
+// through that range still has one.
+func TestMigrateEnvFiles_PreservesStaleGlobalFile(t *testing.T) {
+	dokkuRoot, _ := setupMigrateEnv(t)
+
+	legacy := filepath.Join(dokkuRoot, "ENV")
+	if err := os.WriteFile(legacy, []byte("export DOKKU_WAIT_TO_RETIRE=30\n"), 0600); err != nil {
+		t.Fatalf("WriteFile: %v", err)
+	}
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("first MigrateEnvFiles: %v", err)
+	}
+
+	if err := SetMany("--global", map[string]string{"DOKKU_WAIT_TO_RETIRE": "60"}, false, false); err != nil {
+		t.Fatalf("SetMany: %v", err)
+	}
+
+	if err := os.WriteFile(legacy, []byte("export DOKKU_WAIT_TO_RETIRE=30\n"), 0600); err != nil {
+		t.Fatalf("WriteFile: %v", err)
+	}
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("second MigrateEnvFiles: %v", err)
+	}
+
+	expectEnvValue(t, "--global", "DOKKU_WAIT_TO_RETIRE", "60")
+	expectPreservedKey(t, legacy+".migrated", "DOKKU_WAIT_TO_RETIRE", "30")
+}
+
+// TestMigrateEnvFiles_LegacyValueWins documents the precedence of the one drain
+// that does import: before the migration is recorded the legacy file is the
+// source of truth, so it overwrites the value already held at the config path.
 func TestMigrateEnvFiles_LegacyValueWins(t *testing.T) {
 	dokkuRoot, _ := setupMigrateEnv(t)
 

tests/unit/config.bats

@@ -623,32 +623,97 @@ teardown() {
   assert_failure
 }
 
-@test "(config) config-migrate-env imports an ENV file written after migration" {
-  # drain once so the migration is on record, which makes the ENV file staged
-  # below one that could only have been written by hand
+@test "(config) config-migrate-env preserves a stale ENV file without importing it" {
+  # drain once so the migration is on record, which is the state releases 0.38.0
+  # through 0.38.25 left behind alongside the ENV file staged below
   run_plugn_trigger config-migrate-env
   echo "output: $output"
   echo "status: $status"
   assert_success
 
-  run /bin/bash -c "echo 'export HAND_EDITED=yes' | sudo tee $DOKKU_ROOT/$TEST_APP/ENV && sudo chown dokku:dokku $DOKKU_ROOT/$TEST_APP/ENV"
+  run /bin/bash -c "dokku config:set --no-restart $TEST_APP KEEP=new"
   echo "output: $output"
   echo "status: $status"
   assert_success
 
+  stage_stale_env "KEEP=old" "RESURRECTED=yes"
+
   run_plugn_trigger config-migrate-env
   echo "output: $output"
   echo "status: $status"
   assert_success
-  assert_output_contains "$DOKKU_ROOT/$TEST_APP/ENV"
+  assert_output_contains "the current config takes precedence for KEEP RESURRECTED"
+  assert_output_contains "$DOKKU_ROOT/$TEST_APP/ENV.migrated"
+  assert_output_contains "then delete it"
 
-  run /bin/bash -c "dokku config:get $TEST_APP HAND_EDITED"
+  run /bin/bash -c "dokku config:get $TEST_APP KEEP"
   echo "output: $output"
   echo "status: $status"
   assert_success
-  assert_output "yes"
+  assert_output "new"
+
+  run /bin/bash -c "dokku config:get $TEST_APP RESURRECTED"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
 
   run /bin/bash -c "test -f $DOKKU_ROOT/$TEST_APP/ENV"
   echo "status: $status"
   assert_failure
+
+  run /bin/bash -c "sudo cat $DOKKU_ROOT/$TEST_APP/ENV.migrated"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output_contains "RESURRECTED"
+
+  run /bin/bash -c "sudo rm -f $DOKKU_ROOT/$TEST_APP/ENV.migrated"
+  echo "status: $status"
+  assert_success
+}
+
+@test "(config) install reports a stale ENV file it did not import" {
+  # the apps plugin installs before config and drains through a captured
+  # trigger, so the report only reaches the operator if that stderr is streamed
+  run_plugn_trigger config-migrate-env
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku config:set --no-restart $TEST_APP KEEP=new"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  stage_stale_env "KEEP=old"
+
+  run /bin/bash -c "dokku plugin:install --core"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output_contains "$DOKKU_ROOT/$TEST_APP/ENV.migrated"
+
+  run /bin/bash -c "dokku config:get $TEST_APP KEEP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output "new"
+
+  run /bin/bash -c "sudo rm -f $DOKKU_ROOT/$TEST_APP/ENV.migrated"
+  echo "status: $status"
+  assert_success
+}
+
+stage_stale_env() {
+  declare desc="writes a legacy ENV file at the pre-0.38 path for the test app"
+
+  local contents=""
+  for pair in "$@"; do
+    contents+="export $pair\n"
+  done
+
+  run /bin/bash -c "printf '$contents' | sudo tee $DOKKU_ROOT/$TEST_APP/ENV && sudo chown dokku:dokku $DOKKU_ROOT/$TEST_APP/ENV"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
 }