Merge pull request 'The node-engine reloads the service manager when a unit file or drop-in it writes changes (group fix/shutdown-bound, issue 1126)' (#164) from fix/shutdown-bound into main

internal/apply/apply.go

@@ -1223,6 +1223,19 @@ func applyScoped(
 		}
 	}
 
+	// A file written or removed in a unit directory is read by the service manager only after a reload, whether or not
+	// a service reflects it: a drop-in on reboot.target is reflected by no service, and without the reload the next
+	// reboot still runs the target as it was loaded (issue 1126). Once per apply.
+	if at := unitFileMoved(report.Outcomes); at != "" {
+		if u, ok := sys.(unitReloader); ok {
+			if err := u.ReloadUnits(ctx, run); err != nil {
+				failures = append(failures, &Error{Resource: at, Err: fmt.Errorf("reloading the service manager's units: %w", err)})
+			} else {
+				log(fmt.Sprintf("  reloaded the service manager's units: %s changed", at))
+			}
+		}
+	}
+
 	// Only the mesh says what is assigned: a carried bundle's silence undeclares no identity and no folder.
 	if origin == store.OriginDeclared && scope.isNode() {
 		// And an identity, then a library group, no longer declared goes, once what ran as it went above (ADR 0328
@@ -1997,6 +2010,34 @@ type serviceReloader interface {
 	ReloadService(ctx context.Context, run system.Runner, unit string) error
 }
 
+// UnitDirs are the system service manager's unit directories: a file the apply writes or removes in one of them, or in a
+// drop-in directory below one, is read by the manager only after a reload (issue 1126). Replaceable in tests.
+var UnitDirs = []string{"/etc/systemd/system", "/run/systemd/system", "/usr/local/lib/systemd/system"}
+
+// unitFileMoved is the first file this apply wrote or removed in a unit directory or a drop-in directory below one,
+// or "" when it moved none.
+func unitFileMoved(outcomes []Outcome) string {
+	for _, o := range outcomes {
+		// Only what changed the file on disk, as MovesRoot reads it: skipped, held, kept, forgotten and waiting come
+		// back on every apply and change nothing.
+		if declaration.Type(o.Type) != declaration.TypeFile {
+			continue
+		}
+		switch o.Action {
+		case "created", "updated", "corrected", "removed":
+		default:
+			continue
+		}
+		at := filepath.Clean(o.Target)
+		for _, dir := range UnitDirs {
+			if rel, err := filepath.Rel(dir, at); err == nil && rel != "." && !strings.HasPrefix(rel, "..") {
+				return at
+			}
+		}
+	}
+	return ""
+}
+
 // unitReloader is a service manager that caches unit files and must be told to read them again.
 type unitReloader interface {
 	ReloadUnits(ctx context.Context, run system.Runner) error

internal/apply/apply_test.go

@@ -1261,6 +1261,52 @@ func TestAServiceIsRestartedWhenWhatItReflectsChanges(t *testing.T) {
 		if reloaded < 0 || reloaded > stop {
 			t.Errorf("the service was restarted without the unit files being read again first; commands were %v", commands)
 		}
+
+		// A drop-in in a unit directory that no service reflects — the power module's bound on reboot.target
+		// (issue 1126) — is read by the service manager only after a reload: one per apply, and no restart.
+		// Unchanged, it reloads nothing.
+		units := filepath.Join(dir, "system")
+		saved := UnitDirs
+		UnitDirs = []string{units}
+		t.Cleanup(func() { UnitDirs = saved })
+		dropIn := filepath.Join(units, "reboot.target.d", "mesh-power.conf")
+		withDropIn := parse(t, fmt.Sprintf(`{"declaration":1,"resources":[
+		  {"id":"conf","type":"file","path":%q,"content":"second\n","mode":"0644"},
+		  {"id":"svc","type":"service","unit":"thing.service","state":"running","restart-on":["conf"]},
+		  {"id":"units","type":"directory","path":%q,"mode":"0755"},
+		  {"id":"drop-ins","type":"directory","path":%q,"mode":"0755"},
+		  {"id":"bound","type":"file","path":%q,"content":"[Unit]\nJobTimeoutSec=10min\n","mode":"0644"},
+		  {"id":"bound-too","type":"file","path":%q,"content":"[Unit]\nJobTimeoutSec=10min\n","mode":"0644"}
+		]}`, path, units, filepath.Dir(dropIn), dropIn, filepath.Join(filepath.Dir(dropIn), "other.conf")))
+		for _, row := range []struct {
+			name    string
+			reloads int
+		}{{"a drop-in written", 1}, {"the same drop-in again", 0}} {
+			commands = nil
+			if _, state, err = Apply(context.Background(), archHost(t), withDropIn, state,
+				store.OriginCarried, run, nil, nil); err != nil {
+				t.Fatal(err)
+			}
+			reloads := 0
+			for _, c := range commands {
+				if strings.Contains(c, "daemon-reload") {
+					reloads++
+				}
+				if strings.Contains(c, "stop thing.service") {
+					t.Errorf("%s: a drop-in no service reflects restarted one: %v", row.name, commands)
+				}
+			}
+			if reloads != row.reloads {
+				t.Errorf("%s: %d reloads, want %d; commands were %v", row.name, reloads, row.reloads, commands)
+			}
+		}
+		// A drop-in whose module is skipped behind its step, or held as found, comes back so on every apply: it moved
+		// nothing, and reloads nothing.
+		for _, action := range []string{"skipped", "held"} {
+			if at := unitFileMoved([]Outcome{{ID: "bound", Type: "file", Target: dropIn, Action: action}}); at != "" {
+				t.Errorf("a drop-in %s reloads the service manager (%s)", action, at)
+			}
+		}
 	}
 }