Merge pull request 'A one-shot at rest satisfies a stopped declaration, so the kernel's boot cleanup applies (issue 1126)' (#162) from fix/ace-tun-cleanup into main

internal/apply/apply.go

@@ -2002,6 +2002,35 @@ type unitReloader interface {
 	ReloadUnits(ctx context.Context, run system.Runner) error
 }
 
+// restReader is a service manager that can say whether a one-shot has nothing running now
+// (novox/hq issue 1126).
+type restReader interface {
+	ServiceAtRest(ctx context.Context, run system.Runner, unit string) (bool, error)
+}
+
+// declaredState is a unit's state as a declaration of want reads it. **A one-shot at rest is
+// stopped to a declaration that it be stopped** (novox/hq issue 1126): a one-shot that does not
+// linger reads "running" once it ran and worked — that is what a declaration that it run asks — but
+// it has no process between its runs, so it is also as stopped as it can be, and stopping it changes
+// nothing. Read only as "running", a boot-time cleanup declared stopped and enabled failed every
+// apply after the boot that ran it: "was asked to be stopped and is running".
+func declaredState(ctx context.Context, sys system.System, run Runner, unit, want string) (string, error) {
+	state, err := sys.ServiceState(ctx, run, unit)
+	if err != nil || want != "stopped" || state != "running" {
+		return state, err
+	}
+	if r, ok := sys.(restReader); ok {
+		rest, err := r.ServiceAtRest(ctx, run, unit)
+		if err != nil {
+			return "", err
+		}
+		if rest {
+			return "stopped", nil
+		}
+	}
+	return state, nil
+}
+
 // serviceSettle is how long the host waits before looking at a unit a second time. A test sets it
 // to nothing; on a machine it is the window in which a daemon that refuses its configuration dies.
 var serviceSettle = 2 * time.Second
@@ -2154,7 +2183,7 @@ func applyServiceIn(ctx context.Context, sys system.System, r *declaration.Servi
 	// own stopping from anybody else's (novox/hq ADR 0269).
 	out.declared = &store.FoundUnit{Unit: r.Unit, State: r.State, Boot: r.Boot}
 
-	before, err := sys.ServiceState(ctx, run, r.Unit)
+	before, err := declaredState(ctx, sys, run, r.Unit, r.State)
 	if err != nil {
 		return out, err
 	}
@@ -2165,7 +2194,7 @@ func applyServiceIn(ctx context.Context, sys system.System, r *declaration.Servi
 
 		// Read back. A service manager accepting a command says the transaction was accepted,
 		// not that the unit is running — one that starts and immediately dies satisfies it.
-		after, err := sys.ServiceState(ctx, run, r.Unit)
+		after, err := declaredState(ctx, sys, run, r.Unit, r.State)
 		if err == nil && r.State == "running" {
 			after, err = stayedRunning(ctx, sys, run, r.Unit)
 		}

internal/apply/apply_test.go

@@ -350,6 +350,42 @@ func TestAnUnknownServiceStateIsRefusedNotGuessed(t *testing.T) {
 	}
 }
 
+// A boot-time one-shot a package ships, declared stopped and enabled at boot, is satisfied while
+// nothing of it runs, whatever its last run did (novox/hq issue 1126). The kernel module's
+// linux-modules-cleanup.service failed on ace with "was asked to be stopped and is running": an
+// inactive one-shot with ExecMainStatus=0 reads "running", and stopping it changes nothing.
+func TestABootOneShotDeclaredStoppedIsSatisfiedAtRest(t *testing.T) {
+	for _, c := range []struct{ name, status string }{
+		{"exited 0: never ran, ran at boot, or unloaded", "0"},
+	} {
+		t.Run(c.name, func(t *testing.T) {
+			var asked []string
+			run := func(ctx context.Context, name string, args ...string) (string, error) {
+				args = oldShape(name, args)
+				switch args[0] {
+				case "show":
+					return "LoadState=loaded\nActiveState=inactive\nType=oneshot\nRemainAfterExit=no\n" +
+						"ExecMainStatus=" + c.status + "\n", nil
+				case "is-enabled":
+					return "enabled\n", nil
+				}
+				asked = append(asked, strings.Join(args, " "))
+				return "", nil
+			}
+			d := parse(t, `{"declaration":1,"resources":[
+			  {"id":"cleanup","type":"service","unit":"linux-modules-cleanup.service","state":"stopped","boot":"enabled"}
+			]}`)
+			_, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, run, nil, nil)
+			if err != nil {
+				t.Fatalf("a one-shot at rest declared stopped failed: %v", err)
+			}
+			if len(asked) > 0 {
+				t.Errorf("the service manager was asked %q for a unit already as declared", asked)
+			}
+		})
+	}
+}
+
 func TestADroppedServiceIsGivenBackTheStateItWasFoundIn(t *testing.T) {
 	// novox/hq ADR 0118: undeclaring removes what the mesh made, gives back what it changed, and
 	// leaves what was the machine's. A service resource never installs a unit — so undeclaring it

internal/system/arch.go

@@ -187,38 +187,11 @@ 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",
+	u, err := showUnit(ctx, run, unit)
-		"--property=LoadState", "--property=ActiveState", "--property=Type",
+	if err != nil {
-		"--property=RemainAfterExit", "--property=ExecMainStatus", "--", unit)
+		return "", err
-
-	var load, active, kind, remains, exited string
-	for _, line := range strings.Split(out, "\n") {
-		key, value, found := strings.Cut(strings.TrimSpace(line), "=")
-		if !found {
-			continue
 	}
-		switch key {
+	switch u.load {
-		case "LoadState":
-			load = value
-		case "ActiveState":
-			active = value
-		case "Type":
-			kind = value
-		case "RemainAfterExit":
-			remains = value
-		case "ExecMainStatus":
-			exited = value
-		}
-	}
-
-	switch load {
-	case "":
-		// With why, where it said why: an account's manager that could not be reached says so
-		// here, and nowhere else (novox/hq ADR 0177).
-		if asked != nil {
-			return "", fmt.Errorf("the service manager said nothing about %s: %w", unit, asked)
-		}
-		return "", fmt.Errorf("the service manager said nothing about %s", unit)
 	case "not-found":
 		return "", fmt.Errorf(
 			"%s does not exist on this machine. A declaration naming a unit that is not "+
@@ -227,7 +200,7 @@ func (arch) ServiceState(ctx context.Context, run Runner, unit string) (string,
 	case "masked":
 		return "", fmt.Errorf("%s is masked, so its state cannot be declared", unit)
 	case "error", "bad-setting":
-		return "", fmt.Errorf("%s is installed but its unit file cannot be loaded (%s)", unit, load)
+		return "", fmt.Errorf("%s is installed but its unit file cannot be loaded (%s)", unit, u.load)
 	}
 
 	// **A one-shot that finished is not stopped.** A unit whose whole job is to apply something
@@ -243,24 +216,88 @@ func (arch) ServiceState(ctx context.Context, run Runner, unit string) (string,
 	// wrong with.
 	//
 	// So for that shape, what "running" means is "it ran, and it worked".
-	if kind == "oneshot" && remains != "yes" && active == "inactive" {
+	//
-		if exited == "0" || exited == "" {
+	// It cannot be told from one that never ran: systemd says ExecMainStatus=0 and a start timestamp
+	// of 0 for a one-shot that never ran, and also for one that ran and was unloaded because nothing
+	// referenced it, then loaded again. So a one-shot declared running that must be seen to have run
+	// lingers (RemainAfterExit) or is enabled at boot; one declared stopped is judged at rest by the
+	// apply (ServiceAtRest, novox/hq issue 1126).
+	if u.oneShotAtRest() {
+		if u.exited == "0" || u.exited == "" {
 			return "running", nil
 		}
 		return "stopped", nil
 	}
 
-	switch active {
+	switch u.active {
 	case "active", "activating", "reloading":
 		return "running", nil
 	case "inactive", "failed", "deactivating":
 		return "stopped", nil
 	default:
 		return "", fmt.Errorf(
-			"the service manager reports %s as %q, which is neither running nor stopped", unit, active)
+			"the service manager reports %s as %q, which is neither running nor stopped", unit, u.active)
 	}
 }
 
+// ServiceAtRest is whether unit is a one-shot that does not linger and has nothing running now
+// (novox/hq issue 1126). Such a unit has no state in which a process is up between its runs, so a
+// declaration that it be stopped is satisfied whatever its last run did: there is nothing to stop,
+// and asking the manager to stop it changes nothing. A boot-time cleanup a package ships is declared
+// this way — enabled at boot, never started by an apply.
+func (arch) ServiceAtRest(ctx context.Context, run Runner, unit string) (bool, error) {
+	u, err := showUnit(ctx, run, unit)
+	if err != nil {
+		return false, err
+	}
+	return u.oneShotAtRest(), nil
+}
+
+// shownUnit is what `systemctl show` says of a unit, as far as its state is read.
+type shownUnit struct {
+	load, active, kind, remains, exited string
+}
+
+// oneShotAtRest is a one-shot that does not linger and is not running now.
+func (u shownUnit) oneShotAtRest() bool {
+	return u.kind == "oneshot" && u.remains != "yes" && u.active == "inactive"
+}
+
+// showUnit reads a unit's state properties. A manager that said nothing is an error, with why
+// where it said why: an account's manager that could not be reached says so here, and nowhere
+// else (novox/hq ADR 0177).
+func showUnit(ctx context.Context, run Runner, unit string) (shownUnit, error) {
+	out, asked := run(ctx, "systemctl", "show",
+		"--property=LoadState", "--property=ActiveState", "--property=Type",
+		"--property=RemainAfterExit", "--property=ExecMainStatus", "--", unit)
+	var u shownUnit
+	for _, line := range strings.Split(out, "\n") {
+		key, value, found := strings.Cut(strings.TrimSpace(line), "=")
+		if !found {
+			continue
+		}
+		switch key {
+		case "LoadState":
+			u.load = value
+		case "ActiveState":
+			u.active = value
+		case "Type":
+			u.kind = value
+		case "RemainAfterExit":
+			u.remains = value
+		case "ExecMainStatus":
+			u.exited = value
+		}
+	}
+	if u.load == "" {
+		if asked != nil {
+			return u, fmt.Errorf("the service manager said nothing about %s: %w", unit, asked)
+		}
+		return u, fmt.Errorf("the service manager said nothing about %s", unit)
+	}
+	return u, nil
+}
+
 func (arch) SetServiceState(ctx context.Context, run Runner, unit, state string) error {
 	verb := "start"
 	if state == "stopped" {

internal/system/arch_test.go

@@ -5,47 +5,51 @@ import (
 	"testing"
 )
 
-// A one-shot unit that did its work and exited is satisfied, not stopped.
+// How a one-shot unit's state is read: what it did, not whether a process is up.
 //
-// **This made the firewall permanently unsatisfiable.** Its unit loads a rule set and exits, so it
+// **A one-shot that did its work and exited is satisfied, not stopped.** Its unit loads a rule set
-// is inactive the instant it succeeds — the host started it, it worked, the host read back
+// and exits, so it is inactive the instant it succeeds; reading that as stopped made the firewall
-// "stopped" and reported the machine as not doing what it was told. On every apply, for ever, with
+// permanently unsatisfiable, on every apply, with the rules correctly in place the whole time.
-// the rules correctly in place the whole time.
+//
-func TestAOneShotThatFinishedIsRunning(t *testing.T) {
+// **One that failed is still stopped**, or the host would report success for work that did not
+// happen — the opposite mistake and the worse one.
+//
+// **At rest** (novox/hq issue 1126, the kernel's boot-time cleanup on ace) is whether a one-shot that does not linger has nothing running now: a declaration
+// that it be stopped is then satisfied, whatever it last did, because there is nothing to stop.
+func TestAOneShotIsReadByWhatItDid(t *testing.T) {
+	for _, c := range []struct {
+		name, show, want string
+		atRest           bool
+	}{
+		{"finished", "ActiveState=inactive\nType=oneshot\nRemainAfterExit=no\nExecMainStatus=0\n",
+			"running", true},
+		{"failed", "ActiveState=inactive\nType=oneshot\nRemainAfterExit=no\nExecMainStatus=1\n",
+			"stopped", true},
+		{"running now", "ActiveState=activating\nType=oneshot\nRemainAfterExit=no\nExecMainStatus=0\n",
+			"running", false},
+		{"lingers", "ActiveState=active\nType=oneshot\nRemainAfterExit=yes\nExecMainStatus=0\n",
+			"running", false},
+		{"a daemon, stopped", "ActiveState=inactive\nType=simple\nRemainAfterExit=no\nExecMainStatus=0\n",
+			"stopped", false},
+	} {
+		t.Run(c.name, func(t *testing.T) {
 			run := func(_ context.Context, _ string, _ ...string) (string, error) {
-		return "LoadState=loaded\nActiveState=inactive\nType=oneshot\nRemainAfterExit=no\nExecMainStatus=0\n", nil
+				return "LoadState=loaded\n" + c.show, nil
 			}
-	state, err := arch{}.ServiceState(context.Background(), run, "nftables.service")
+			state, err := arch{}.ServiceState(context.Background(), run, "thing.service")
 			if err != nil {
-		t.Fatalf("a one-shot that succeeded was an error: %v", err)
+				t.Fatalf("read as an error: %v", err)
 			}
-	if state != "running" {
+			if state != c.want {
-		t.Fatalf("a one-shot that did its work reads as %q, so it can never be satisfied", state)
+				t.Errorf("reads as %q, want %q", state, c.want)
 			}
-}
+			rest, err := arch{}.ServiceAtRest(context.Background(), run, "thing.service")
-
-// And one that failed is still stopped, or the host would report success for work that did not
-// happen — which is the opposite mistake and the worse one.
-func TestAOneShotThatFailedIsStopped(t *testing.T) {
-	run := func(_ context.Context, _ string, _ ...string) (string, error) {
-		return "LoadState=loaded\nActiveState=inactive\nType=oneshot\nRemainAfterExit=no\nExecMainStatus=1\n", nil
-	}
-	state, err := arch{}.ServiceState(context.Background(), run, "nftables.service")
 			if err != nil {
-		t.Fatalf("unexpected error: %v", err)
+				t.Fatalf("at rest read as an error: %v", err)
 			}
-	if state != "stopped" {
+			if rest != c.atRest {
-		t.Fatalf("a one-shot that failed reads as %q, so a broken firewall reports success", state)
+				t.Errorf("at rest is %v, want %v", rest, c.atRest)
 			}
-}
+		})
-
-// A unit that lingers deliberately is unaffected: it says so, and its active state is the answer.
-func TestAOneShotThatRemainsIsJudgedByItsActiveState(t *testing.T) {
-	run := func(_ context.Context, _ string, _ ...string) (string, error) {
-		return "LoadState=loaded\nActiveState=active\nType=oneshot\nRemainAfterExit=yes\nExecMainStatus=0\n", nil
-	}
-	state, _ := arch{}.ServiceState(context.Background(), run, "thing.service")
-	if state != "running" {
-		t.Fatalf("a lingering one-shot reads as %q", state)
 	}
 }