fix(apps): report stale env keys from every active revision

ActiveAppSpec stopped at the first active revision whose app env
differed from desired, so keys stale only in another service's
revision were missing from apply and diff previews. Every distinct
active revision is now folded into the effective env, so the diff
lists each stale key once.

commit: d53efe22f3a2d4126d20ef25e6866833a68b1f8e
author: brice · 5d ago · 2026-10-17 04:59 UTC
parent: d0f5c6c
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +63 -18

M internal/domain/app_spec.go +25 -18


@@ -1198,9 +1198,9 @@ func diffEnvKeys(prefix string, desired, effective map[string]string) []string {
1198	1198		// ActiveAppSpec rebuilds the effective AppSpec view of ACTIVE for diffing.
1199	1199		// ACTIVE stores per-service specs but not the app-level [env], so
1200	1200		// revisionEnv returns the app env of the revision a service was activated
1201		-	// from (nil when that revision is gone). When services disagree, the first
1202		-	// one (by name) whose env differs from desiredEnv wins, so a diff never
1203		-	// hides a service still running a stale app env.
	1201	+	// from (nil when that revision is gone). When services run from different
	1202	+	// revisions, every key that is stale in any of them is reported, so a diff
	1203	+	// never hides a service still running a stale app env.
1204	1204		func ActiveAppSpec(
1205	1205		active AppActive,
1206	1206		desiredEnv map[string]string,

@@ -1213,6 +1213,13 @@ func ActiveAppSpec(
1213	1213		svc.Name = name
1214	1214		spec.Services = append(spec.Services, svc)
1215	1215		}
	1216	+	if len(names) == 0 {
	1217	+	return spec, nil
	1218	+	}
	1219	+	spec.Env = maps.Clone(desiredEnv)
	1220	+	if spec.Env == nil {
	1221	+	spec.Env = map[string]string{}
	1222	+	}
1216	1223		checked := map[string]struct{}{}
1217	1224		for _, name := range names {
1218	1225		revision := active.Services[name].EffectiveRevision

@@ -1227,28 +1234,28 @@ func ActiveAppSpec(
1227	1234		return AppSpec{}, err
1228	1235		}
1229	1236		}
1230		-	spec.Env = maps.Clone(env)
1231		-	if spec.Env == nil {
1232		-	spec.Env = map[string]string{}
1233		-	}
1234		-	if !equalStringMaps(env, desiredEnv) {
1235		-	break
1236		-	}
	1237	+	mergeStaleEnv(spec.Env, desiredEnv, env)
1237	1238		}
1238	1239		return spec, nil
1239	1240		}
1240	1241
1241		-	// equalStringMaps compares string maps.
1242		-	func equalStringMaps(a, b map[string]string) bool {
1243		-	if len(a) != len(b) {
1244		-	return false
	1242	+	// mergeStaleEnv folds one running env into effective, which starts as a
	1243	+	// copy of desired. Every key whose running value differs from desired is
	1244	+	// overwritten (or deleted when not running), so it stays different from
	1245	+	// desired once any revision marks it stale; matching keys are untouched.
	1246	+	func mergeStaleEnv(effective, desired, running map[string]string) {
	1247	+	for key, value := range desired {
	1248	+	if runningValue, ok := running[key]; !ok {
	1249	+	delete(effective, key)
	1250	+	} else if runningValue != value {
	1251	+	effective[key] = runningValue
	1252	+	}
1245	1253		}
1246		-	for k, v := range a {
1247		-	if b[k] != v {
1248		-	return false
	1254	+	for key, value := range running {
	1255	+	if _, ok := desired[key]; !ok {
	1256	+	effective[key] = value
1249	1257		}
1250	1258		}
1251		-	return true
1252	1259		}
1253	1260
1254	1261		// equalVolumes compares volume slices by value.

 // ActiveAppSpec rebuilds the effective AppSpec view of ACTIVE for diffing.
 // ACTIVE stores per-service specs but not the app-level [env], so
 // revisionEnv returns the app env of the revision a service was activated
-// from (nil when that revision is gone). When services disagree, the first
+// from (nil when that revision is gone). When services run from different
-// one (by name) whose env differs from desiredEnv wins, so a diff never
+// revisions, every key that is stale in any of them is reported, so a diff
-// hides a service still running a stale app env.
+// never hides a service still running a stale app env.
 func ActiveAppSpec(
 	active AppActive,
 	desiredEnv map[string]string,
 		svc.Name = name
 		spec.Services = append(spec.Services, svc)
 	}
+	if len(names) == 0 {
+		return spec, nil
+	}
+	spec.Env = maps.Clone(desiredEnv)
+	if spec.Env == nil {
+		spec.Env = map[string]string{}
+	}
 	checked := map[string]struct{}{}
 	for _, name := range names {
 		revision := active.Services[name].EffectiveRevision
 				return AppSpec{}, err
 			}
 		}
-		spec.Env = maps.Clone(env)
+		mergeStaleEnv(spec.Env, desiredEnv, env)
-		if spec.Env == nil {
-			spec.Env = map[string]string{}
-		}
-		if !equalStringMaps(env, desiredEnv) {
-			break
-		}
 	}
 	return spec, nil
 }
-// equalStringMaps compares string maps.
+// mergeStaleEnv folds one running env into effective, which starts as a
-func equalStringMaps(a, b map[string]string) bool {
+// copy of desired. Every key whose running value differs from desired is
-	if len(a) != len(b) {
+// overwritten (or deleted when not running), so it stays different from
-		return false
+// desired once any revision marks it stale; matching keys are untouched.
+func mergeStaleEnv(effective, desired, running map[string]string) {
+	for key, value := range desired {
+		if runningValue, ok := running[key]; !ok {
+			delete(effective, key)
+		} else if runningValue != value {
+			effective[key] = runningValue
+		}
 	}
-	for k, v := range a {
+	for key, value := range running {
-		if b[k] != v {
+		if _, ok := desired[key]; !ok {
-			return false
+			effective[key] = value
 		}
 	}
-	return true
 }
 // equalVolumes compares volume slices by value.

M internal/domain/app_spec_test.go +38 -0


@@ -258,3 +258,41 @@ func TestActiveAppSpec_UsesActivatedRevisionEnv(t *testing.T) {
258	258		_, err = domain.ActiveAppSpec(active, nil, func(string) (map[string]string, error) { return nil, errors.New("boom") })
259	259		require.Error(t, err)
260	260		}
	261	+
	262	+	func TestActiveAppSpec_ReportsStaleKeysFromEveryRevision(t *testing.T) {
	263	+	web := validSpec().Services[0]
	264	+	worker := web
	265	+	worker.Name = "worker"
	266	+	active := domain.AppActive{
	267	+	App: "blog",
	268	+	Services: map[string]domain.AppEffectiveService{
	269	+	"web": {EffectiveRevision: "rev-1", Spec: web},
	270	+	"worker": {EffectiveRevision: "rev-2", Spec: worker},
	271	+	},
	272	+	}
	273	+	envs := map[string]map[string]string{
	274	+	"rev-1": {"A": "old", "B": "new", "C": "same"},
	275	+	"rev-2": {"A": "new", "C": "same", "D": "gone"},
	276	+	}
	277	+	desired := domain.AppSpec{Name: "blog", Env: map[string]string{"A": "new", "B": "new", "C": "same"}}
	278	+	desired.Services = []domain.AppService{web, worker}
	279	+
	280	+	effective, err := domain.ActiveAppSpec(active, desired.Env, func(revision string) (map[string]string, error) {
	281	+	return envs[revision], nil
	282	+	})
	283	+	require.NoError(t, err)
	284	+
	285	+	diff := domain.DiffAppSpec(desired, effective)
	286	+	assert.Equal(t, []string{"env/A", "env/B", "env/D"}, diff.Changed)
	287	+	assert.Equal(t, map[string]string{"A": "new", "B": "new", "C": "same"}, desired.Env, "desired env must not be mutated")
	288	+	}
	289	+
	290	+	func TestActiveAppSpec_EmptyActiveHasEmptyEnv(t *testing.T) {
	291	+	spec, err := domain.ActiveAppSpec(domain.AppActive{App: "blog"}, map[string]string{"A": "x"}, func(string) (map[string]string, error) {
	292	+	t.Fatal("no revision should be loaded")
	293	+	return nil, nil
	294	+	})
	295	+	require.NoError(t, err)
	296	+	assert.Empty(t, spec.Env)
	297	+	assert.NotNil(t, spec.Env)
	298	+	}


@@ -258,3 +258,41 @@ func TestActiveAppSpec_UsesActivatedRevisionEnv(t *testing.T) {
258	_, err = domain.ActiveAppSpec(active, nil, func(string) (map[string]string, error) { return nil, errors.New("boom") })	258		_, err = domain.ActiveAppSpec(active, nil, func(string) (map[string]string, error) { return nil, errors.New("boom") })
259	require.Error(t, err)	259		require.Error(t, err)
260	}	260		}
		261	+
		262	+	func TestActiveAppSpec_ReportsStaleKeysFromEveryRevision(t *testing.T) {
		263	+	web := validSpec().Services[0]
		264	+	worker := web
		265	+	worker.Name = "worker"
		266	+	active := domain.AppActive{
		267	+	App: "blog",
		268	+	Services: map[string]domain.AppEffectiveService{
		269	+	"web": {EffectiveRevision: "rev-1", Spec: web},
		270	+	"worker": {EffectiveRevision: "rev-2", Spec: worker},
		271	+	},
		272	+	}
		273	+	envs := map[string]map[string]string{
		274	+	"rev-1": {"A": "old", "B": "new", "C": "same"},
		275	+	"rev-2": {"A": "new", "C": "same", "D": "gone"},
		276	+	}
		277	+	desired := domain.AppSpec{Name: "blog", Env: map[string]string{"A": "new", "B": "new", "C": "same"}}
		278	+	desired.Services = []domain.AppService{web, worker}
		279	+
		280	+	effective, err := domain.ActiveAppSpec(active, desired.Env, func(revision string) (map[string]string, error) {
		281	+	return envs[revision], nil
		282	+	})
		283	+	require.NoError(t, err)
		284	+
		285	+	diff := domain.DiffAppSpec(desired, effective)
		286	+	assert.Equal(t, []string{"env/A", "env/B", "env/D"}, diff.Changed)
		287	+	assert.Equal(t, map[string]string{"A": "new", "B": "new", "C": "same"}, desired.Env, "desired env must not be mutated")
		288	+	}
		289	+
		290	+	func TestActiveAppSpec_EmptyActiveHasEmptyEnv(t *testing.T) {
		291	+	spec, err := domain.ActiveAppSpec(domain.AppActive{App: "blog"}, map[string]string{"A": "x"}, func(string) (map[string]string, error) {
		292	+	t.Fatal("no revision should be loaded")
		293	+	return nil, nil
		294	+	})
		295	+	require.NoError(t, err)
		296	+	assert.Empty(t, spec.Env)
		297	+	assert.NotNil(t, spec.Env)
		298	+	}