Pass -- before every unit, and give a unit back only when it is exactly as the mesh held it

Third review of mesh-catalog #147.

A unit's name can now come from a setting (a template's instance), so every systemctl call that names a
unit passes `--` before it, and a name can never be read as an option.

On removal, a unit is started or enabled again only when it is exactly the stopped and disabled the mesh
held it to. A masked unit, or one somebody started or enabled since, is left as it is, and the report
says why. A restore that fails (the start refused, or the unit not running after it) puts back what this
attempt changed and is reported as waiting, so no apply is made fatal by it and the next one tries again.

The tests' fake service managers read systemctl's arguments in the shape they were written for
(oldShape); internal/system's own test holds the `--`.

internal/apply/apply.go

@@ -2909,52 +2909,71 @@ func giveUnitBack(ctx context.Context, sys system.System, a store.Applied, run R
 	return "restored", detail, nil
 }
 
-// startUnitBack gives back a unit the mesh's declaration stopped or disabled, found running or enabled:
-// enabled first, then started, each read back, and the report says what was done (novox/hq ADR 0269).
+// startUnitBack gives back a unit the mesh's declaration stopped or disabled, found running or enabled
+// (novox/hq ADR 0269).
+//
+// **Only a unit exactly as the mesh held it.** A masked unit is somebody's word that it must not run, and
+// one started or enabled since the mesh held it is somebody's doing: both are left as they are, and the
+// report says why. **A restore that fails waits**: the start refused, or the unit not running after it,
+// puts back what this attempt changed — so the next apply finds the unit as the mesh held it and tries
+// again — and is reported as waiting, never as an error that would make an apply fatal.
 func startUnitBack(ctx context.Context, sys system.System, a store.Applied, run Runner, start, enable bool) (string, string, error) {
-	state, err := sys.ServiceState(ctx, run, a.Target)
-	if err != nil {
-		if strings.Contains(err.Error(), "does not exist on this machine") {
+	state, serr := sys.ServiceState(ctx, run, a.Target)
+	boot, berr := sys.ServiceBoot(ctx, run, a.Target)
+	for _, err := range []error{serr, berr} {
+		switch {
+		case err == nil:
+		case strings.Contains(err.Error(), "does not exist on this machine"):
 			return "forgotten", "the unit no longer exists", nil
+		case strings.Contains(err.Error(), "masked"):
+			return "forgotten", a.Target + " is masked, which is somebody's word that it must not run; left as it is", nil
+		default:
+			return "", "", fmt.Errorf("%w: %s could not be read to give it back (%v); tried again by the next apply",
+				errRemovalWaits, a.Target, err)
 		}
-		return "", "", err
 	}
-	var did, already []string
+	if state != a.Declared.State || (a.Declared.Boot != "" && boot != a.Declared.Boot) {
+		return "forgotten", fmt.Sprintf("it is %s and %s at boot, not the %s and %s the mesh held it to: somebody "+
+			"changed it since, so it is left as it is", state, boot, a.Declared.State, orUnset(a.Declared.Boot)), nil
+	}
+	var did []string
 	if enable {
-		if boot, err := sys.ServiceBoot(ctx, run, a.Target); err == nil && boot == "enabled" {
-			already = append(already, "enabled at boot")
-		} else {
-			if err := sys.SetServiceBoot(ctx, run, a.Target, "enabled"); err != nil {
-				return "", "", fmt.Errorf("enabling %s again, which the mesh disabled and found enabled: %w", a.Target, err)
-			}
-			did = append(did, "enabled at boot")
+		if err := sys.SetServiceBoot(ctx, run, a.Target, "enabled"); err != nil {
+			return "", "", fmt.Errorf("%w: enabling %s again failed (%v); tried again by the next apply", errRemovalWaits, a.Target, err)
 		}
+		did = append(did, "enabled at boot")
 	}
 	if start {
-		if state == "running" {
-			already = append(already, "running")
-		} else {
-			if err := sys.SetServiceState(ctx, run, a.Target, "running"); err != nil {
-				return "", "", fmt.Errorf("starting %s again, which the mesh stopped and found running: %w", a.Target, err)
-			}
-			after, err := sys.ServiceState(ctx, run, a.Target)
-			if err != nil {
-				return "", "", err
-			}
-			if after != "running" {
-				return "", "", fmt.Errorf("%s was started again, as the host found it, and is %s", a.Target, after)
-			}
-			did = append(did, "started")
+		why := ""
+		if err := sys.SetServiceState(ctx, run, a.Target, "running"); err != nil {
+			why = "its start failed: " + err.Error()
+		} else if after, err := sys.ServiceState(ctx, run, a.Target); err != nil {
+			why = "it could not be read after its start: " + err.Error()
+		} else if after != "running" {
+			why = "it is " + after + " after its start, not running"
 		}
+		if why != "" {
+			undo := ""
+			if enable {
+				if err := sys.SetServiceBoot(ctx, run, a.Target, "disabled"); err != nil {
+					undo = "; it stays enabled at boot, its disabling again failed: " + err.Error()
+				} else {
+					undo = "; disabled at boot again, as the mesh held it, until then"
+				}
+			}
+			return "", "", fmt.Errorf("%w: giving %s back running, as the host found it: %s%s; tried again by the next apply",
+				errRemovalWaits, a.Target, why, undo)
+		}
+		did = append(did, "started")
 	}
-	if len(did) == 0 {
-		return "forgotten", "already as the host found it (" + strings.Join(already, ", ") + ")", nil
+	return "restored", strings.Join(did, " and ") + " again, as the host found it before the mesh's declaration stopped it", nil
+}
+
+func orUnset(s string) string {
+	if s == "" {
+		return "as it was at boot"
 	}
-	detail := strings.Join(did, " and ") + " again, as the host found it before the mesh's declaration stopped it"
-	if len(already) > 0 {
-		detail += "; already " + strings.Join(already, ", ")
-	}
-	return "restored", detail, nil
+	return s
 }
 
 // foundAs is what a service's unit was before the mesh touched it, as far as this host can know:

internal/apply/apply_test.go

@@ -313,6 +313,7 @@ func TestAServiceIsReadBackNotAssumed(t *testing.T) {
 	// running. A unit that starts and immediately dies satisfies the command.
 	started := false
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if args[0] == "show" {
 			if started {
 				return "LoadState=loaded\nActiveState=failed\n", nil // started, then died
@@ -337,6 +338,7 @@ func TestAServiceIsReadBackNotAssumed(t *testing.T) {
 
 func TestAnUnknownServiceStateIsRefusedNotGuessed(t *testing.T) {
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		return "LoadState=loaded\nActiveState=reticent\n", nil
 	}
 	d := parse(t, `{"declaration":1,"resources":[
@@ -371,6 +373,7 @@ func TestADroppedServiceIsGivenBackTheStateItWasFoundIn(t *testing.T) {
 		t.Run(c.name, func(t *testing.T) {
 			var commands []string
 			run := func(ctx context.Context, name string, args ...string) (string, error) {
+				args = oldShape(name, args)
 				commands = append(commands, strings.Join(args, " "))
 				if args[0] == "show" {
 					return "LoadState=loaded\nActiveState=active\n", nil
@@ -413,6 +416,7 @@ func TestAServiceRecordsTheStateItWasFoundInOnceAndCarriesIt(t *testing.T) {
 	active := "inactive"
 	enabled := "disabled"
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "show":
 			return "LoadState=loaded\nActiveState=" + active + "\n", nil
@@ -464,6 +468,7 @@ func TestAUnitThatDoesNotExistIsNotStopped(t *testing.T) {
 	// Absence read as satisfaction — 04-ISSUES/007 wearing a different hat, and the mirror of
 	// the degraded-init bug the capability detector had.
 	absent := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		return "LoadState=not-found\nActiveState=inactive\n", nil
 	}
 	d := parse(t, `{"declaration":1,"resources":[
@@ -486,6 +491,7 @@ func TestAMaskedUnitIsRefused(t *testing.T) {
 	// Masked means someone deliberately made it unstartable. Applying over that would undo a
 	// decision the host did not make and cannot see the reason for.
 	masked := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		return "LoadState=masked\nActiveState=inactive\n", nil
 	}
 	d := parse(t, `{"declaration":1,"resources":[
@@ -506,6 +512,7 @@ func TestForgettingAUnitThatIsGoneDoesNotStrandTheNode(t *testing.T) {
 	// already true.
 	var stopped bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if args[0] == "show" {
 			return "LoadState=not-found\nActiveState=inactive\n", nil
 		}
@@ -557,6 +564,7 @@ func TestABrokenPackageDatabaseIsNotReadAsNotInstalled(t *testing.T) {
 	// answer would silently reinstall on a machine whose package manager is broken, or report
 	// "installed nothing" as success. The apply must fail instead.
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		return "", errors.New("pacman: error: could not lock database")
 	}
 	d := parseTrusted(t, `{"declaration":1,"resources":[
@@ -575,6 +583,7 @@ func TestABrokenPackageDatabaseIsNotReadAsNotInstalled(t *testing.T) {
 func TestAnInstalledPackageIsNotReinstalled(t *testing.T) {
 	var installed bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if args[0] == "-S" {
 			installed = true
 		}
@@ -602,6 +611,7 @@ func TestAPackageIsNeverUninstalled(t *testing.T) {
 	// machine may have had it before the mesh ever saw it. Undeclaring is not "remove it".
 	var uninstalled bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if len(args) > 0 && (args[0] == "-R" || args[0] == "-Rs") {
 			uninstalled = true
 		}
@@ -635,6 +645,7 @@ func TestAnActionThatIsAlreadyTrueDoesNotRun(t *testing.T) {
 	// database is, so "is it already there" is a question only the declaration can ask.
 	var ran bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name == "create-db" {
 			ran = true
 		}
@@ -661,6 +672,7 @@ func TestAnActionThatSucceedsAndDoesNothingFails(t *testing.T) {
 	// this repository's most expensive failure shape. Here the command "succeeds" every time
 	// and verify never passes.
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name == "has-db" {
 			return "", errors.New("no such database")
 		}
@@ -687,6 +699,7 @@ func TestAnActionRunsInsideTheContainerItNames(t *testing.T) {
 	// there is any mesh to ask.
 	var sawExec bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name == "docker" && args[0] == "exec" && args[1] == "store" {
 			sawExec = true
 			return "", nil
@@ -710,6 +723,7 @@ func TestAContainerThatExitsImmediatelyFailsTheApply(t *testing.T) {
 	// still running. A container whose entrypoint dies satisfies the command exactly as one
 	// that came up does — which is the read-back rule, in the place it matters most.
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch {
 		case args[0] == "info":
 			return "27.0\n", nil
@@ -748,6 +762,7 @@ func TestAContainerWhoseDeclarationChangedIsReplaced(t *testing.T) {
 
 	var removed, created bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
@@ -786,6 +801,7 @@ func TestAContainerThatMatchesIsLeftAlone(t *testing.T) {
 
 	var touched bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
@@ -831,6 +847,7 @@ func TestAContainerIsRecreatedWhenARestartOnResourceChanged(t *testing.T) {
 
 	var removed, created bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
@@ -886,6 +903,7 @@ func TestRestartOnFiresOnlyForResourcesThatChanged(t *testing.T) {
 func systemctlStub(t *testing.T, load, active, enabled string, verbs *[]string) Runner {
 	t.Helper()
 	return func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "show":
 			return "LoadState=" + load + "\nActiveState=" + active + "\n", nil
@@ -1043,6 +1061,7 @@ func TestTheRuntimeProbeIsPerRuntime(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			var probedWith string
 			run := func(ctx context.Context, name string, args ...string) (string, error) {
+				args = oldShape(name, args)
 				if name != tc.present {
 					return "", errors.New("not installed")
 				}
@@ -1071,6 +1090,7 @@ func TestAContainerUsesTheRuntimeTheMachineHas(t *testing.T) {
 	// machine already has (novox/hq research 012), so hardcoding one contradicts it.
 	var calledWith []string
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name == "docker" {
 			return "", errors.New("not installed")
 		}
@@ -1107,6 +1127,7 @@ func TestNoRuntimeIsSaidPlainly(t *testing.T) {
 	// Naming what was tried, because "docker: command not found" on a machine that deliberately
 	// runs podman sends the reader looking for the wrong thing.
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		return "", errors.New("not installed")
 	}
 	d := parseTrusted(t, `{"declaration":1,"resources":[
@@ -1250,6 +1271,7 @@ func TestAServiceIsNotRestartedByAChangeItDoesNotName(t *testing.T) {
 // before and after one.
 func recordingServices(commands *[]string) Runner {
 	return func(_ context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		line := name + " " + strings.Join(args, " ")
 		*commands = append(*commands, line)
 		switch {
@@ -1398,6 +1420,7 @@ func TestAFailedActionStopsWhatFollows(t *testing.T) {
 func TestAContainerIsGivenTheMeshsNames(t *testing.T) {
 	var ran []string
 	run := func(_ context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name != "docker" {
 			return "", errors.New("not installed")
 		}
@@ -1433,6 +1456,7 @@ func TestAContainerIsGivenTheMeshsNames(t *testing.T) {
 func TestAContainerGivenNoNamesIsRunAsBefore(t *testing.T) {
 	var ran []string
 	run := func(_ context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name != "docker" {
 			return "", errors.New("not installed")
 		}
@@ -1477,6 +1501,7 @@ func TestResourcesAreAppliedInTheOrderTheyWereDeclared(t *testing.T) {
 	var order []string
 	done := map[string]bool{}
 	run := func(_ context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		order = append(order, name+" "+strings.Join(args, " "))
 		// `verify` is the idempotency check, so it has to fail before the step and pass after —
 		// a stub that always succeeds means every action is already done and nothing ever runs,
@@ -1627,6 +1652,7 @@ func TestAContainerStaleFromAnEarlierApplyIsReplaced(t *testing.T) {
 
 	var removed, created bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil

internal/apply/file_handover_test.go

@@ -43,6 +43,7 @@ func watching(t *testing.T, path string) Runner {
 	var commands []string
 	services := recordingServices(&commands)
 	return func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if _, err := os.Stat(path); err != nil {
 			t.Errorf("the file was not there while %s %s ran: %v", name, strings.Join(args, " "), err)
 		}

internal/apply/former_archive_test.go

@@ -15,6 +15,7 @@ import (
 // starts (ADR 0141).
 func TestTheHostsOwnFormerArchiveIsLeftInPlaceNotFatal(t *testing.T) {
 	run := func(_ context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name == "docker" && args[0] == "info" {
 			return "29.0.0\n", nil
 		}

internal/apply/found_test.go

@@ -59,6 +59,7 @@ func TestAUnitWhoseFileTheMeshWroteIsStoppedWhateverItsRecordSays(t *testing.T)
 	m := unitsMachine(map[string]*fakeUnit{"mesh-guard.service": {active: "active", enabled: "enabled"}})
 	fileThereAtStop := false
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name == "systemctl" && args[0] == "stop" {
 			_, err := os.Stat(unitFile)
 			fileThereAtStop = err == nil

internal/apply/giveback_test.go

@@ -101,3 +101,70 @@ func TestThePlanSaysAStoppedUnitIsStartedAgain(t *testing.T) {
 		t.Errorf("the plan says %q", said)
 	}
 }
+
+// Third review: on removal a unit is started again only when it is exactly as the mesh held it, stopped and
+// disabled. A masked unit, or one somebody started or enabled since, is left alone, and the report says why.
+func TestAMaskedOrChangedUnitIsLeftAloneOnRemoval(t *testing.T) {
+	for _, c := range []struct {
+		name, active, enabled, says string
+	}{
+		{"masked", "inactive", "masked", "masked"},
+		{"enabled since", "inactive", "enabled", "not the stopped and disabled"},
+		{"started since", "active", "disabled", "not the stopped and disabled"},
+	} {
+		m := unitsMachine(map[string]*fakeUnit{"w.timer": {active: "active", enabled: "enabled"}})
+		_, state, err := applyOn(t, `{"declaration":1,"resources":[
+		  {"id":"w","type":"service","unit":"w.timer","state":"stopped","boot":"disabled"}]}`, store.State{}, m)
+		if err != nil {
+			t.Fatal(err)
+		}
+		m.units["w.timer"].active, m.units["w.timer"].enabled = c.active, c.enabled
+		report, _, err := applyOn(t, nothingButA(t), state, m)
+		if err != nil {
+			t.Fatalf("%s: %v", c.name, err)
+		}
+		if u := m.units["w.timer"]; u.active != c.active || u.enabled != c.enabled {
+			t.Errorf("%s: changed to %s and %s", c.name, u.active, u.enabled)
+		}
+		said := ""
+		for _, o := range report.Outcomes {
+			if o.ID == "w" {
+				said = o.Action + ": " + o.Detail
+			}
+		}
+		if !strings.HasPrefix(said, "forgotten: ") || !strings.Contains(said, c.says) {
+			t.Errorf("%s: the report says %q", c.name, said)
+		}
+	}
+}
+
+// A restore that fails — the start refused, or the unit not running after it — waits: the record stays,
+// the report says so, and no apply is made fatal by it.
+func TestAFailedRestoreWaitsAndIsNeverFatal(t *testing.T) {
+	m := unitsMachine(map[string]*fakeUnit{"w.timer": {active: "active", enabled: "enabled"}})
+	_, state, err := applyOn(t, `{"declaration":1,"resources":[
+	  {"id":"w","type":"service","unit":"w.timer","state":"stopped","boot":"disabled"}]}`, store.State{}, m)
+	if err != nil {
+		t.Fatal(err)
+	}
+	m.units["w.timer"].wontStart = true
+	for i := 0; i < 2; i++ {
+		report, next, err := applyOn(t, nothingButA(t), state, m)
+		if err != nil {
+			t.Fatalf("apply %d failed: %v", i+1, err)
+		}
+		said := ""
+		for _, o := range report.Outcomes {
+			if o.ID == "w" {
+				said = o.Action + ": " + o.Detail
+			}
+		}
+		if !strings.Contains(said, "wait") || !strings.Contains(said, "not running") {
+			t.Errorf("apply %d: the report says %q", i+1, said)
+		}
+		if _, kept := next.Find("w"); !kept {
+			t.Errorf("apply %d: the record went, so nothing would try again", i+1)
+		}
+		state = next
+	}
+}

internal/apply/groups_test.go

@@ -35,6 +35,8 @@ func newGroupDB(in []string, exists ...string) *groupDB {
 }
 
 func (g *groupDB) run(_ context.Context, name string, args ...string) (string, error) {
+
+	args = oldShape(name, args)
 	g.asked = append(g.asked, name+" "+strings.Join(args, " "))
 	switch {
 	case name == "getent" && args[0] == "passwd":

internal/apply/handover_test.go

@@ -30,6 +30,8 @@ type aMachine struct {
 }
 
 func (m *aMachine) run(ctx context.Context, name string, args ...string) (string, error) {
+
+	args = oldShape(name, args)
 	line := name + " " + strings.Join(args, " ")
 	m.commands = append(m.commands, line)
 	switch {
@@ -245,6 +247,7 @@ func TestAStepThatFailsGatesItsModule(t *testing.T) {
 	body, digest := anArchive(t, map[string]string{"mesh-controller": "#!/bin/sh\n"})
 	src := serving(t, body)
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name == "systemctl" && len(args) > 1 && args[0] == "start" {
 			return "", errors.New("Job for mesh-controller-prepare.service failed")
 		}

internal/apply/hold_test.go

@@ -107,7 +107,28 @@ type fakeContainer struct {
 	networks, mounts, ports []string
 }
 
+// oldShape is a systemctl call as these fakes were written to read it: the verb, the unit, then the
+// options. The host passes `--` before every unit (third review of mesh-catalog #147), which
+// internal/system's own test holds; the fakes read the unit where they always did.
+func oldShape(name string, args []string) []string {
+	if name != "systemctl" || len(args) < 2 || args[len(args)-2] != "--" {
+		return args
+	}
+	v := 0
+	for v < len(args) && strings.HasPrefix(args[v], "-") {
+		v++
+	}
+	if v >= len(args)-2 {
+		return args
+	}
+	out := append([]string{}, args[:v+1]...)
+	out = append(out, args[len(args)-1])
+	return append(out, args[v+1:len(args)-2]...)
+}
+
 func (m *machine) run(_ context.Context, name string, args ...string) (string, error) {
+
+	args = oldShape(name, args)
 	m.asked = append(m.asked, name+" "+strings.Join(args, " "))
 	if name == "systemctl" {
 		return m.systemctl(args)
@@ -907,6 +928,8 @@ func TestMountingTheMachinesOwnPlumbingIsNotFoundData(t *testing.T) {
 type unreachable struct{ asked []string }
 
 func (u *unreachable) run(_ context.Context, name string, args ...string) (string, error) {
+
+	args = oldShape(name, args)
 	u.asked = append(u.asked, name+" "+strings.Join(args, " "))
 	if name == "docker" && args[0] == "info" {
 		return "27.0\n", nil
@@ -958,6 +981,7 @@ func TestAHeldContainerStaysHeldWhenTheRuntimeCannotAnswer(t *testing.T) {
 func TestAVolumeTheRuntimeCannotBeAskedAboutStopsTheContainer(t *testing.T) {
 	dir := t.TempDir()
 	run := func(_ context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch {
 		case name == "docker" && args[0] == "info":
 			return "27.0\n", nil

internal/apply/inspect_scope_test.go

@@ -19,6 +19,7 @@ import (
 // not catch a regression back to it — this one refuses to, on purpose.
 func dockerLikeByKind(containers map[string]bool) func(context.Context, string, ...string) (string, error) {
 	return func(_ context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name != "docker" {
 			return "", errors.New("unexpected program: " + name)
 		}

internal/apply/left_out_test.go

@@ -16,6 +16,7 @@ func TestAContainerJoinsTheNetworksItKeeps(t *testing.T) {
 	var ran []string
 	connectFails := false
 	run := func(_ context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name != "docker" {
 			return "", errors.New("not installed")
 		}
@@ -81,6 +82,7 @@ func TestALeftOutModuleIsNeitherRemovedNorForgotten(t *testing.T) {
 	var removed []string
 	gone := map[string]bool{}
 	run := func(_ context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name != "docker" {
 			return "", nil
 		}
@@ -139,6 +141,7 @@ func TestALeftOutModuleIsNeitherRemovedNorForgotten(t *testing.T) {
 func TestACapabilityReachesTheRuntimeAndTheSpec(t *testing.T) {
 	var ran []string
 	run := func(_ context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name != "docker" {
 			return "", errors.New("not installed")
 		}
@@ -180,6 +183,7 @@ func TestAPackageDeclaredAbsentIsRemovedWhenPresentAndLeftWhenNot(t *testing.T)
 	installed := true
 	var ran []string
 	run := func(_ context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		ran = append(ran, name+" "+strings.Join(args, " "))
 		if name != "pacman" {
 			return "", nil

internal/apply/opening_test.go

@@ -42,6 +42,8 @@ func (u *ufwMachine) iptables(args []string) (string, error) {
 }
 
 func (u *ufwMachine) run(_ context.Context, name string, args ...string) (string, error) {
+
+	args = oldShape(name, args)
 	u.asked = append(u.asked, name+" "+strings.Join(args, " "))
 	switch name {
 	case "nft":
@@ -374,6 +376,7 @@ func TestReturningToAdoptedLoadsTheGuardBeforeRemovingTheFilter(t *testing.T) {
 		}
 		guardUpAtStop := false
 		run := func(_ context.Context, name string, args ...string) (string, error) {
+			args = oldShape(name, args)
 			if name != "systemctl" {
 				return "", &exec.Error{Name: name, Err: exec.ErrNotFound}
 			}
@@ -471,6 +474,7 @@ func TestAGuardThatFailsLeavesTheDerivedFilterInForce(t *testing.T) {
 		`","content":"table inet mesh_guard {}\n"}`
 	stopped := false
 	run := func(_ context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name != "systemctl" {
 			return "", &exec.Error{Name: name, Err: exec.ErrNotFound}
 		}

internal/apply/placement_guard_test.go

@@ -58,6 +58,7 @@ func TestNoAccessIsAtTheMachinesOwn(t *testing.T) {
 func TestNoContainerMountsTheMachinesOwn(t *testing.T) {
 	ran := false
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch {
 		case args[0] == "info":
 			return "27.0\n", nil

internal/apply/placement_homes_test.go

@@ -46,6 +46,7 @@ func TestBelowAHomeOnlyThatAccountsDirectoryIsPlaced(t *testing.T) {
 	// A container mounting a refused directory is refused with it, and not run.
 	ran := false
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil

internal/apply/plan_test.go

@@ -103,6 +103,8 @@ type both struct {
 }
 
 func (b *both) run(ctx context.Context, name string, args ...string) (string, error) {
+
+	args = oldShape(name, args)
 	switch name {
 	case "nft", "ufw", "iptables", "ip6tables", "firewall-cmd":
 		return b.u.run(ctx, name, args...)

internal/apply/process_step_test.go

@@ -29,6 +29,7 @@ func TestARunOnceProcessRunsAsItsOneshotUnit(t *testing.T) {
 	var commands []string
 	fail := false
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		commands = append(commands, name+" "+strings.Join(args, " "))
 		if fail && name == "systemctl" && len(args) > 0 && args[0] == "start" {
 			return "", errors.New("exit status 1")

internal/apply/process_test.go

@@ -187,6 +187,7 @@ func TestAnUndeclaredProcessIsRemovedWithItsUnitAndBundle(t *testing.T) {
 	}
 	var commands []string
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		commands = append(commands, strings.Join(args, " "))
 		return "", nil
 	}
@@ -269,6 +270,7 @@ func TestAProcessAppliedAgainIsUnchangedAndNotRestarted(t *testing.T) {
 	body, digest := anArchive(t, map[string]string{"main.js": "console.log(1)\n"})
 	var commands []string
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		commands = append(commands, name+" "+strings.Join(args, " "))
 		return "", nil
 	}

internal/apply/removal_order_test.go

@@ -35,6 +35,7 @@ func TestAnUnassignedModulesProcessIsStoppedAfterTheConfigurationItReads(t *test
 
 	var configAtStop []bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name == "systemctl" && strings.Join(args, " ") == "disable --now nfs-server-exports.service" {
 			_, err := os.Stat(config)
 			configAtStop = append(configAtStop, err == nil)
@@ -96,6 +97,7 @@ func TestAProcessRecordedAfterItsConfigurationIsStoppedFirst(t *testing.T) {
 
 	var configAtStop []bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name == "systemctl" && strings.Join(args, " ") == "disable --now m-exports.service" {
 			_, err := os.Stat(config)
 			configAtStop = append(configAtStop, err == nil)

internal/apply/restart_order_test.go

@@ -34,6 +34,7 @@ func TestAServiceIsRestartedOnlyOnceEveryFileItNamesIsWritten(t *testing.T) {
 	var commands []string
 	services := recordingServices(&commands)
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if strings.Contains(strings.Join(args, " "), "start resolver.service") {
 			if _, err := os.Stat(zones); err != nil {
 				commands = append(commands, name+" "+strings.Join(args, " "))

internal/apply/runonce_test.go

@@ -32,6 +32,7 @@ func TestARunOnceStepIsRunToCompletionNotLeftRunning(t *testing.T) {
 	// container exits) nor --restart (a step that is restarted is not a step).
 	var runArgs []string
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
@@ -78,6 +79,7 @@ func TestARunOnceStepThatExitsNonZeroFailsTheApply(t *testing.T) {
 	// Run in the foreground, a non-zero exit is an error the runtime hands back. That must fail
 	// the apply, not be swallowed — a seed that did not happen leaves the machine unready.
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
@@ -110,6 +112,7 @@ func TestAFailedRunOnceStepGatesWhatFollows(t *testing.T) {
 	// mirror of a failed action stopping what follows.
 	var startedNames []string
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
@@ -159,6 +162,7 @@ func TestARunOnceStepAlreadyCompletedIsNotReRun(t *testing.T) {
 
 	var ran bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
@@ -194,6 +198,7 @@ func TestARunOnceStepReRunsWhenItsDeclarationChanged(t *testing.T) {
 
 	var ran bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
@@ -236,6 +241,7 @@ func TestARunOnceStepRunsAgainWhenWhatItReadsChanged(t *testing.T) {
 
 	var ran bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
@@ -284,6 +290,7 @@ func TestAContainerNamingARunOnceStepIsRecreatedWhenItRan(t *testing.T) {
 
 	var removed, created bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
@@ -326,6 +333,7 @@ func TestAFailedStepGatesItsModuleAndNotTheMachine(t *testing.T) {
 	// unreachable must not stop every module declared after it.
 	var startedNames []string
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil

internal/apply/schedule_test.go

@@ -44,6 +44,8 @@ type recordingRun struct {
 }
 
 func (r *recordingRun) run(ctx context.Context, name string, args ...string) (string, error) {
+
+	args = oldShape(name, args)
 	r.mu.Lock()
 	defer r.mu.Unlock()
 	r.all = append(r.all, append([]string{name}, args...))
@@ -91,6 +93,7 @@ func TestInstallingAScheduleDoesNotRunItAndReportsCurrent(t *testing.T) {
 	var fired bool  // a `docker run` — the container was started
 	var pulled bool // an image reported present was pulled anyway
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil // a container runtime answers the apply's probe
@@ -154,6 +157,7 @@ func TestInstallingAScheduleEnsuresItsImageIsPresentWithoutStartingIt(t *testing
 	var pulledImage string
 	var fired bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
@@ -205,6 +209,7 @@ func TestAScheduledStepDoesNotGateWhatFollows(t *testing.T) {
 	var startedWeb bool
 	specs := map[string]string{} // name -> spec label, so read-back sees a running container
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
@@ -295,6 +300,8 @@ func TestAFailedRunIsRecordedAndDoesNotFailAnything(t *testing.T) {
 	}
 
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+
+		args = oldShape(name, args)
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
@@ -343,6 +350,7 @@ func TestASlowRunSkipsTheNextDueRunRatherThanStacking(t *testing.T) {
 	var enters int
 	var mu sync.Mutex
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if args[0] == "info" {
 			return "27.0\n", nil
 		}

internal/apply/stateless_test.go

@@ -17,6 +17,7 @@ import (
 // unitIn is a service manager whose one unit is active or not, recording what it is asked.
 func unitIn(active bool, commands *[]string) Runner {
 	return func(_ context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		line := name + " " + strings.Join(args, " ")
 		*commands = append(*commands, line)
 		switch {

internal/apply/stayed_running_test.go

@@ -20,6 +20,7 @@ func TestAServiceThatDiesJustAfterItsRestartIsNotReportedRestarted(t *testing.T)
 	// refuses what it was just given.
 	started, looks := false, 0
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if args[0] == "show" {
 			state := "active"
 			if started {
@@ -56,6 +57,7 @@ func TestAServiceThatDiesJustAfterItsRestartIsNotReportedRestarted(t *testing.T)
 func TestAUnitStillStartingIsNotAFailure(t *testing.T) {
 	serviceSettle = 0
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if args[0] == "show" {
 			return "LoadState=loaded\nActiveState=activating\n", nil
 		}
@@ -74,6 +76,7 @@ func TestAServiceAskedToStopIsNotWaitedOn(t *testing.T) {
 	serviceSettle = 0
 	shows := 0
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if args[0] == "show" {
 			shows++
 			if shows == 1 {

internal/apply/user_test.go

@@ -22,6 +22,8 @@ type logins struct {
 }
 
 func (l *logins) run(_ context.Context, name string, args ...string) (string, error) {
+
+	args = oldShape(name, args)
 	l.asked = append(l.asked, name+" "+strings.Join(args, " "))
 	who := args[len(args)-1]
 	switch name {

internal/apply/userscope_test.go

@@ -59,6 +59,8 @@ func (a *account) did(prefix string) bool {
 }
 
 func (a *account) run(_ context.Context, name string, args ...string) (string, error) {
+
+	args = oldShape(name, args)
 	line := name + " " + strings.Join(args, " ")
 	a.asked = append(a.asked, line)
 	switch name {
@@ -256,6 +258,7 @@ func TestAManagerThatStopsDuringTheApplyIsWaitedFor(t *testing.T) {
 	a.busDown = true
 	calls := 0
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name == "systemctl" && len(args) == 2 && args[0] == "is-active" {
 			calls++
 			if calls > 1 {

internal/apply/vocabulary_test.go

@@ -259,6 +259,7 @@ func TestANetworkIsCreatedAndThenRemovedWhenNoLongerDeclared(t *testing.T) {
 	var calls []string
 	there := map[string]bool{}
 	run := func(_ context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		calls = append(calls, name+" "+strings.Join(args, " "))
 		if name != "docker" || len(args) < 2 || args[0] != "network" {
 			return "", nil // the runtime probe
@@ -302,6 +303,7 @@ func TestANetworkIsCreatedAndThenRemovedWhenNoLongerDeclared(t *testing.T) {
 func TestANetworkAlreadyThereIsNotRebuilt(t *testing.T) {
 	var created int
 	run := func(_ context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		if name == "docker" && len(args) > 1 && args[0] == "network" && args[1] == "create" {
 			created++
 		}
@@ -375,6 +377,7 @@ func TestASealedValueIsPutIntoTheFileThatNamesIt(t *testing.T) {
 func TestAContainerIsGivenItsEnvironmentFiles(t *testing.T) {
 	var ran []string
 	run := func(_ context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		ran = append(ran, name+" "+strings.Join(args, " "))
 		if len(args) > 0 && args[0] == "container" {
 			return "", fmt.Errorf("no such container")
@@ -410,6 +413,7 @@ func TestAContainerIsGivenItsEnvironmentFiles(t *testing.T) {
 func TestAContainerIsGivenItsResolverAndItsAddress(t *testing.T) {
 	var ran []string
 	run := func(_ context.Context, name string, args ...string) (string, error) {
+		args = oldShape(name, args)
 		ran = append(ran, name+" "+strings.Join(args, " "))
 		if len(args) > 0 && args[0] == "container" {
 			return "", fmt.Errorf("no such container")

internal/system/arch.go

@@ -187,9 +187,9 @@ func describeAge(when, now time.Time) string {
 // so LoadState is what is read — and it is the thing an interface spanning systemd and OpenRC
 // would have had to drop.
 func (arch) ServiceState(ctx context.Context, run Runner, unit string) (string, error) {
-	out, asked := run(ctx, "systemctl", "show", unit,
+	out, asked := run(ctx, "systemctl", "show",
 		"--property=LoadState", "--property=ActiveState", "--property=Type",
-		"--property=RemainAfterExit", "--property=ExecMainStatus")
+		"--property=RemainAfterExit", "--property=ExecMainStatus", "--", unit)
 
 	var load, active, kind, remains, exited string
 	for _, line := range strings.Split(out, "\n") {
@@ -266,7 +266,7 @@ func (arch) SetServiceState(ctx context.Context, run Runner, unit, state string)
 	if state == "stopped" {
 		verb = "stop"
 	}
-	_, err := run(ctx, "systemctl", verb, unit)
+	_, err := run(ctx, "systemctl", verb, "--", unit)
 	return err
 }
 
@@ -276,7 +276,7 @@ func (arch) SetServiceState(ctx context.Context, run Runner, unit, state string)
 // install section and CANNOT be enabled. Reading it as "disabled" would have the host try, fail,
 // and blame the wrong thing — the same shape as reading a missing unit as "stopped".
 func (arch) ServiceBoot(ctx context.Context, run Runner, unit string) (string, error) {
-	out, _ := run(ctx, "systemctl", "is-enabled", unit)
+	out, _ := run(ctx, "systemctl", "is-enabled", "--", unit)
 	switch state := strings.TrimSpace(out); state {
 	case "enabled", "enabled-runtime", "alias":
 		return "enabled", nil
@@ -302,7 +302,7 @@ func (arch) SetServiceBoot(ctx context.Context, run Runner, unit, boot string) e
 	if boot == "disabled" {
 		verb = "disable"
 	}
-	_, err := run(ctx, "systemctl", verb, unit)
+	_, err := run(ctx, "systemctl", verb, "--", unit)
 	return err
 }
 
@@ -354,7 +354,7 @@ func (arch) RemoveUserFromGroup(ctx context.Context, run Runner, name, group str
 // is how the host tells a unit an administrator installed, under /etc or /run, from one a package
 // ships under /usr (novox/hq ADR 0103). Empty, with no error, for a unit that loads from nowhere.
 func (arch) ServiceUnitFile(ctx context.Context, run Runner, unit string) (string, error) {
-	out, err := run(ctx, "systemctl", "show", unit, "--property=FragmentPath")
+	out, err := run(ctx, "systemctl", "show", "--property=FragmentPath", "--", unit)
 	if err != nil {
 		return "", fmt.Errorf("the service manager did not say where %s comes from: %w", unit, err)
 	}
@@ -376,7 +376,7 @@ func (arch) ReloadUnits(ctx context.Context, run Runner) error {
 
 // ReloadService tells a running unit to read its configuration again, without stopping it.
 func (arch) ReloadService(ctx context.Context, run Runner, unit string) error {
-	_, err := run(ctx, "systemctl", "reload", unit)
+	_, err := run(ctx, "systemctl", "reload", "--", unit)
 	return err
 }
 

internal/system/dashdash_test.go

@@ -0,0 +1,37 @@
+package system
+
+import (
+	"context"
+	"strings"
+	"testing"
+)
+
+// Third review of mesh-catalog #147: a unit's name reaches systemctl as an argument, and since a setting
+// may now name a template's instance, a name beginning with '-' must never be read as an option. Every
+// call that names a unit passes `--` before it.
+func TestEverySystemctlCallPassesDashDashBeforeTheUnit(t *testing.T) {
+	var asked [][]string
+	run := func(_ context.Context, name string, args ...string) (string, error) {
+		if name == "systemctl" {
+			asked = append(asked, args)
+		}
+		return "LoadState=loaded\nActiveState=active\nType=simple\nFragmentPath=/x\n", nil
+	}
+	ctx := context.Background()
+	a := arch{}
+	unit := "-x@y.timer"
+	a.ServiceState(ctx, run, unit)
+	a.SetServiceState(ctx, run, unit, "running")
+	a.SetServiceState(ctx, run, unit, "stopped")
+	a.ServiceBoot(ctx, run, unit)
+	a.SetServiceBoot(ctx, run, unit, "enabled")
+	a.SetServiceBoot(ctx, run, unit, "disabled")
+	if len(asked) != 6 {
+		t.Fatalf("asked %d times: %v", len(asked), asked)
+	}
+	for _, args := range asked {
+		if len(args) < 2 || args[len(args)-1] != unit || args[len(args)-2] != "--" {
+			t.Errorf("systemctl %s", strings.Join(args, " "))
+		}
+	}
+}