Serve unlink-dangling, so a leftover enable link goes through the seat

systemctl disable leaves a link whose unit file is gone, and the manager keeps asking for the unit. The verb removes only such links in the account's or the machine's own configuration, on the person's word, and records each with its reason before removing it (hq issue 332).
2026-10-08 17:53:53 +02:00
parent e740683ca4
commit 87c9d9f7e5
7 changed files with 395 additions and 9 deletions
@@ -98,7 +98,7 @@ func execRunner(cmd string, args []string, env []string) Ran {
 const MeshUnitHeader = "# Generated by the mesh."

 // acts are the verbs that change the system manager's state, which polkit keeps from a non-root account.
-var acts = map[string]bool{"start": true, "stop": true, "restart": true, "enable": true, "disable": true, "reset-failed": true}
+var acts = map[string]bool{"start": true, "stop": true, "restart": true, "enable": true, "disable": true, "reset-failed": true, "daemon-reload": true}

 // escalated is the command as it is run: as given when this process is root, or in the user scope, or
 // when the call is a systemctl read; else through sudo without a prompt — an act on the system manager,
@@ -141,6 +141,12 @@ type Manager struct {
 	Home string
 	// ReadDir lists a directory, to find the enable links naming a unit (wanted.go).
 	ReadDir func(dir string) ([]Entry, error)
+	// Lstat reads one path again just before it is removed, and Remove removes one the account owns (unlink.go).
+	Lstat  func(path string) (Entry, error)
+	Remove func(path string) error
+	// State is the module's state directory, where each removed link is recorded.
+	State string
+	Now   func() time.Time
 }

 var userBus = regexp.MustCompile(`(?i)Failed to connect to (user scope )?bus`)
@@ -1,5 +1,5 @@
-// systemd's tools: the node-service-manager seat's eleven verbs — the units on this machine in both scopes,
-// read and acted on by name, their journal, what has failed, what wants a unit, and a failed record cleared
+// systemd's tools: the node-service-manager seat's twelve verbs — the units on this machine in both scopes,
+// read and acted on by name, their journal, what has failed, what wants a unit, a dangling enable link removed and a failed record cleared
 // (novox/hq ADR 0177, issue 332). The node tools runtime launches this bundle as a process of its own and serves what it serves (ADR 0188, ADR 0193);
 // it runs as the operator account, so acts on the system manager, and reads of its journal, escalate with
 // sudo -n, and the user scope is the account's own manager (client.go). The host applies units; this answers
@@ -145,6 +145,29 @@ func tools(m *Manager) []stdio.Tool {
 				}
 				return m.WantedBy(scope, unit)
 			}},
+		// **A dangling enable link removed on the person's word** (novox/hq issue 332): `disable` leaves it
+		// once the unit's file is gone. Recorded before it is removed (unlink.go).
+		{Name: seat + ".unlink-dangling",
+			Description: "Remove the dangling enable links named for one unit — links in a *.wants, *.requires or *.upholds " +
+				"directory of the account's or the machine's own configuration whose target does not exist, which " +
+				"`disable` leaves once the unit's file is gone — on the person's word: `why` is required. Each link is " +
+				"recorded first (its path, its target, the person's reason) in the module's state, then removed, and " +
+				"the manager reloaded. A link whose target exists, a package's, a generator's or the runtime's is left, " +
+				"and said. `wanted-by` shows what it would remove.",
+			Input: map[string]any{"scope": scopeArg, "unit": unitArgS,
+				"why": str("the person's reason for removing the links, kept in the record")},
+			Run: func(a map[string]any) (any, error) {
+				scope, err := scopeOf(a)
+				if err != nil {
+					return nil, err
+				}
+				unit, err := unitOf(a)
+				if err != nil {
+					return nil, err
+				}
+				why, _ := a["why"].(string)
+				return m.UnlinkDangling(scope, unit, why)
+			}},
 		// Was the module's own systemd_failed; on the seat since the operator's direction of 2026-10-07, so
 		// whatever holds the role answers it and every machine is asked the same way.
 		{Name: seat + ".failed",
@@ -184,7 +207,8 @@ func fromEnv() *Manager {
 		home, _ = os.UserHomeDir()
 	}
 	return &Manager{Account: account, UID: uid, User: name, Run: execRunner, Read: readFile, Env: os.Environ(),
-		Home: home, ReadDir: readDir}
+		Home: home, ReadDir: readDir, Lstat: lstatEntry, Remove: os.Remove,
+		State: strings.TrimSpace(os.Getenv("MESH_SYSTEMD_STATE"))}
 }

 func main() {
@@ -0,0 +1,145 @@
+package main
+
+// Removing a dangling enable link (novox/hq issue 332), on a person's word. `systemctl disable` does not do it
+// once the unit's file is gone: it reports the unit file as not existing and leaves the link, and the manager
+// goes on asking for the unit at every boot or login.
+//
+// It removes as little as can be named, and nothing else:
+//
+//   - only a symbolic link, named for the unit, in a *.wants, *.requires or *.upholds directory under the
+//     scope's searched directories (wanted.go), whose target does not exist;
+//   - only in the account's or the machine's own configuration — never a package's, a generator's or the
+//     runtime's, which put theirs back — and never through a dependency directory that is itself a link;
+//   - each link read again just before it goes, and left when it changed since it was found.
+//
+// A record of each (its path, where it pointed, what wanted the unit) and the person's reason is written to
+// the module's state before the link is removed, so nothing is removed that is not recorded, and the answer
+// says how to put it back. The account's own link is removed by this process, which is that account; any
+// other through sudo -n, as every act on the machine's manager is.
+
+import (
+	"encoding/json"
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+	"time"
+)
+
+// UnlinkedRecord is the file in the module's state each removal is recorded in, one JSON object a line.
+const UnlinkedRecord = "unlinked-links.jsonl"
+
+// Unlinked is one removal, as recorded.
+type Unlinked struct {
+	At     string `json:"at"`
+	Scope  string `json:"scope"`
+	Unit   string `json:"unit"`
+	Path   string `json:"path"`
+	Target string `json:"target"`
+	Of     string `json:"of"`
+	Why    string `json:"why"`
+}
+
+// UnlinkDangling removes the dangling enable links named for one unit, on the person's word (why).
+func (m *Manager) UnlinkDangling(scope Scope, unit, why string) (map[string]any, error) {
+	if err := unitArg(unit); err != nil {
+		return nil, err
+	}
+	if why = strings.TrimSpace(why); why == "" {
+		return nil, fmt.Errorf("why is required: removing a link is the person's act, and their reason is kept with the record")
+	}
+	if m.State == "" {
+		return nil, fmt.Errorf("this module has no state directory to keep the record in, so nothing is removed")
+	}
+	links, unread := m.links(scope, unit)
+	removed, left := []Unlinked{}, []map[string]string{}
+	for _, l := range links {
+		reason := ""
+		switch {
+		case !l.Link:
+			reason = "a file, not a link"
+		case !l.Dangles:
+			reason = "its target exists: `disable` is the verb for a unit that is there"
+		case filepath.Base(l.Path) != unit:
+			reason = "named for another unit, pointing at this one's file"
+		case !l.removable:
+			reason = "in " + l.Whose + " directory, which puts it back, or reached through a linked directory"
+		}
+		if reason == "" {
+			now, err := m.Lstat(l.Path)
+			if err != nil || now.Link != l.Target || !now.Dangles {
+				reason = "it changed since it was read"
+			}
+		}
+		if reason != "" {
+			left = append(left, map[string]string{"path": l.Path, "why_left": reason})
+			continue
+		}
+		rec := Unlinked{At: m.now().UTC().Format(time.RFC3339), Scope: string(scope), Unit: unit, Path: l.Path,
+			Target: l.Target, Of: l.Of, Why: why}
+		if err := m.record(rec); err != nil {
+			return nil, fmt.Errorf("the record of %s could not be kept, so it was not removed: %w", l.Path, err)
+		}
+		if err := m.unlink(l.Path); err != nil {
+			return nil, fmt.Errorf("%s was recorded and not removed: %w (removed before it: %d)", l.Path, err, len(removed))
+		}
+		removed = append(removed, rec)
+	}
+	answer := map[string]any{"unit": unit, "scope": string(scope), "removed": removed, "left": left,
+		"record": filepath.Join(m.State, UnlinkedRecord)}
+	if len(unread) > 0 {
+		answer["unread"] = unread
+	}
+	if len(removed) == 0 {
+		answer["note"] = "no dangling link named for " + unit + " was in the account's or the machine's own configuration: nothing was removed"
+		return answer, nil
+	}
+	// The manager keeps the dependency it loaded from the link until it reloads.
+	if _, err := m.call(scope, "systemctl", "daemon-reload"); err != nil {
+		answer["reload"] = "the manager was not reloaded, so it still lists the dependency until it is: " + err.Error()
+	} else {
+		answer["reload"] = "the manager reloaded"
+	}
+	answer["next"] = "`reset-failed` clears the unit's failed record; each link can be made again from the record (ln -s <target> <path>) if it was wanted"
+	return answer, nil
+}
+
+func (m *Manager) now() time.Time {
+	if m.Now != nil {
+		return m.Now()
+	}
+	return time.Now()
+}
+
+func (m *Manager) record(rec Unlinked) error {
+	line, err := json.Marshal(rec)
+	if err != nil {
+		return err
+	}
+	if err := os.MkdirAll(m.State, 0o700); err != nil {
+		return err
+	}
+	f, err := os.OpenFile(filepath.Join(m.State, UnlinkedRecord), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600)
+	if err != nil {
+		return err
+	}
+	if _, err := f.Write(append(line, '\n')); err != nil {
+		f.Close()
+		return err
+	}
+	return f.Close()
+}
+
+// unlink removes one link: the account's own by this process, which is the account; another as root,
+// through sudo -n. unlink(1) takes one path and removes it without following it.
+func (m *Manager) unlink(path string) error {
+	own := m.Home != "" && strings.HasPrefix(path, filepath.Clean(m.Home)+string(filepath.Separator))
+	if own || m.UID == 0 {
+		return m.Remove(path)
+	}
+	r := m.Run("sudo", []string{"-n", "unlink", path}, nil)
+	if r.Status == 0 && r.Error == "" {
+		return nil
+	}
+	return m.failure("unlink", "sudo", System, r)
+}
@@ -0,0 +1,177 @@
+package main
+
+// Removing a dangling enable link on the person's word (novox/hq issue 332), over the fake listing of
+// wanted_test.go: what is removed and what is left, the record kept before each removal, and who removes.
+
+import (
+	"bufio"
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"reflect"
+	"strings"
+	"testing"
+	"time"
+)
+
+// leftovers is a machine with one removable dangling link and every kind that must be left.
+func leftovers() tree {
+	user := home + "/.config/systemd/user"
+	return tree{
+		user: {{Name: "default.target.wants", Dir: true}},
+		user + "/default.target.wants": {
+			{Name: "greenclip.service", Link: "/usr/lib/systemd/user/greenclip.service", Dangles: true}, // removed
+			{Name: "clip.service", Link: "/usr/lib/systemd/user/greenclip.service", Dangles: true},      // named for another unit
+		},
+		"/etc/systemd/user": {{Name: "graphical-session.target.wants", Dir: true}},
+		"/etc/systemd/user/graphical-session.target.wants": {
+			{Name: "greenclip.service", Link: "/usr/lib/systemd/user/greenclip.service", Dangles: true}, // removed, as root
+		},
+		"/usr/lib/systemd/user":                      {{Name: "default.target.wants", Dir: true}},
+		"/usr/lib/systemd/user/default.target.wants": {{Name: "greenclip.service", Link: "../greenclip.service", Dangles: true}}, // a package's
+	}
+}
+
+func unlinker(t *testing.T, fsys tree, calls *[]call, removed *[]string) *Manager {
+	m := withTree(operator(fake(func(call) Ran { return Ran{} }, calls)), fsys)
+	m.State = t.TempDir()
+	m.Now = func() time.Time { return time.Date(2026, 10, 8, 17, 30, 0, 0, time.UTC) }
+	m.Remove = func(p string) error { *removed = append(*removed, p); return nil }
+	return m
+}
+
+func records(t *testing.T, m *Manager) []Unlinked {
+	f, err := os.Open(filepath.Join(m.State, UnlinkedRecord))
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer f.Close()
+	var out []Unlinked
+	s := bufio.NewScanner(f)
+	for s.Scan() {
+		var r Unlinked
+		if err := json.Unmarshal(s.Bytes(), &r); err != nil {
+			t.Fatal(err)
+		}
+		out = append(out, r)
+	}
+	return out
+}
+
+func TestUnlinkDanglingRemovesOnlyTheDanglingLinksNamedForTheUnitInOwnConfiguration(t *testing.T) {
+	var calls []call
+	var removed []string
+	m := unlinker(t, leftovers(), &calls, &removed)
+	answer, err := m.UnlinkDangling(User, "greenclip.service", "greenclip was replaced by clipmenu")
+	if err != nil {
+		t.Fatal(err)
+	}
+	own := home + "/.config/systemd/user/default.target.wants/greenclip.service"
+	if !reflect.DeepEqual(removed, []string{own}) {
+		t.Fatalf("removed by the account: %v", removed)
+	}
+	// The machine's own, outside the home, as root through sudo -n unlink; then the user manager reloaded.
+	var sudo, reload bool
+	for _, c := range calls {
+		if c.cmd == "sudo" && reflect.DeepEqual(c.args, []string{"-n", "unlink", "/etc/systemd/user/graphical-session.target.wants/greenclip.service"}) {
+			sudo = true
+		}
+		if c.cmd == "systemctl" && reflect.DeepEqual(c.args, []string{"--user", "daemon-reload"}) {
+			reload = sudo // after the removals
+		}
+	}
+	if !sudo || !reload {
+		t.Fatalf("%+v", calls)
+	}
+	got := records(t, m)
+	if len(got) != 2 || got[0].Path != "/etc/systemd/user/graphical-session.target.wants/greenclip.service" || got[1].Path != own ||
+		got[1].Target != "/usr/lib/systemd/user/greenclip.service" || got[1].Of != "default.target" ||
+		got[1].Why != "greenclip was replaced by clipmenu" || got[1].At != "2026-10-08T17:30:00Z" || got[1].Scope != "user" {
+		t.Fatalf("%+v", got)
+	}
+	left := answer["left"].([]map[string]string)
+	why := map[string]string{}
+	for _, l := range left {
+		why[l["path"]] = l["why_left"]
+	}
+	if !strings.Contains(why[home+"/.config/systemd/user/default.target.wants/clip.service"], "named for another unit") ||
+		!strings.Contains(why["/usr/lib/systemd/user/default.target.wants/greenclip.service"], "a package's") {
+		t.Fatalf("%v", why)
+	}
+}
+
+func TestUnlinkDanglingLeavesALinkWhoseTargetExistsOrThatChanged(t *testing.T) {
+	user := home + "/.config/systemd/user"
+	fsys := tree{
+		user:                           {{Name: "default.target.wants", Dir: true}},
+		user + "/default.target.wants": {{Name: "x.service", Link: "/usr/lib/systemd/user/x.service"}},
+	}
+	var removed []string
+	m := unlinker(t, fsys, nil, &removed)
+	answer, err := m.UnlinkDangling(User, "x.service", "test")
+	if err != nil || len(removed) != 0 || !strings.Contains(answer["left"].([]map[string]string)[0]["why_left"], "`disable`") {
+		t.Fatalf("%v %v %v", answer, err, removed)
+	}
+	// Found dangling, then pointed elsewhere before it is removed.
+	fsys[user+"/default.target.wants"] = []Entry{{Name: "x.service", Link: "/gone/x.service", Dangles: true}}
+	m = unlinker(t, fsys, nil, &removed)
+	m.Lstat = func(string) (Entry, error) {
+		return Entry{Name: "x.service", Link: "/usr/lib/systemd/user/x.service"}, nil
+	}
+	answer, err = m.UnlinkDangling(User, "x.service", "test")
+	if err != nil || len(removed) != 0 || answer["left"].([]map[string]string)[0]["why_left"] != "it changed since it was read" {
+		t.Fatalf("%v %v %v", answer, err, removed)
+	}
+	if _, err := os.Stat(filepath.Join(m.State, UnlinkedRecord)); !os.IsNotExist(err) {
+		t.Fatal("a record was kept of a link that was not removed")
+	}
+}
+
+func TestUnlinkDanglingNeedsAReasonAndAPlaceForTheRecord(t *testing.T) {
+	var removed []string
+	m := unlinker(t, leftovers(), nil, &removed)
+	if _, err := m.UnlinkDangling(User, "greenclip.service", "  "); err == nil || !strings.Contains(err.Error(), "why is required") {
+		t.Fatalf("%v", err)
+	}
+	m.State = ""
+	if _, err := m.UnlinkDangling(User, "greenclip.service", "x"); err == nil {
+		t.Fatal("removed with nowhere to record")
+	}
+	// A record that cannot be written: nothing is removed.
+	file := filepath.Join(t.TempDir(), "a-file")
+	if err := os.WriteFile(file, nil, 0o600); err != nil {
+		t.Fatal(err)
+	}
+	m.State = file
+	if _, err := m.UnlinkDangling(User, "greenclip.service", "x"); err == nil || !strings.Contains(err.Error(), "not removed") || len(removed) != 0 {
+		t.Fatalf("%v %v", err, removed)
+	}
+}
+
+func TestUnlinkDanglingInTheSystemScopeGoesThroughSudo(t *testing.T) {
+	fsys := tree{
+		"/etc/systemd/system":                          {{Name: "multi-user.target.wants", Dir: true}},
+		"/etc/systemd/system/multi-user.target.wants":  {{Name: "old.service", Link: "/usr/lib/systemd/system/old.service", Dangles: true}},
+		"/run/systemd/generator":                       {{Name: "local-fs.target.wants", Dir: true}},
+		"/run/systemd/generator/local-fs.target.wants": {{Name: "old.service", Link: "/gone", Dangles: true}},
+	}
+	var calls []call
+	var removed []string
+	m := unlinker(t, fsys, &calls, &removed)
+	answer, err := m.UnlinkDangling(System, "old.service", "the package is gone")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(removed) != 0 {
+		t.Fatalf("removed without sudo: %v", removed)
+	}
+	if got := append([]string{calls[0].cmd}, calls[0].args...); !reflect.DeepEqual(got, []string{"sudo", "-n", "unlink", "/etc/systemd/system/multi-user.target.wants/old.service"}) {
+		t.Fatalf("%v", got)
+	}
+	if got := append([]string{calls[1].cmd}, calls[1].args...); !reflect.DeepEqual(got, []string{"sudo", "-n", "systemctl", "daemon-reload"}) {
+		t.Fatalf("%v", got)
+	}
+	if len(answer["removed"].([]Unlinked)) != 1 || len(answer["left"].([]map[string]string)) != 1 {
+		t.Fatalf("%v", answer)
+	}
+}
@@ -12,7 +12,8 @@ package main
 //     is the case a failed record of a unit whose load state is not-found comes from, and the answer says
 //     each link's target and whether it dangles.
 //
-// Nothing here changes anything. Clearing the record is `reset-failed`'s.
+// Nothing here changes anything. Removing a dangling link is unlink.go's; clearing the record is
+// `reset-failed`'s.

 import (
 	"errors"
@@ -275,8 +276,9 @@ func findings(scope Scope, unit, loadState string, dependents int, links []Enabl
 		out = append(out, "the unit's file is not found, and "+plural(len(links), "enable link")+" still name it")
 	}
 	if removable > 0 {
-		out = append(out, "`disable` does not remove a link whose unit file is gone, and no verb of this seat removes one yet "+
-			"(novox/hq issue 332); once it is gone, `reset-failed` clears the unit's failed record")
+		out = append(out, "`disable` does not remove a link whose unit file is gone; `unlink-dangling` in this scope removes "+
+			"the dangling links named for the unit in the account's or the machine's own configuration, keeping a record of each; "+
+			"then `reset-failed` clears the unit's failed record")
 	} else if dangling > 0 {
 		out = append(out, "the dangling links are in a package's, a generator's or the runtime's directory, which puts them back: they are not the person's to remove")
 	}
@@ -326,6 +328,15 @@ func readDir(dir string) ([]Entry, error) {
 	return out, nil
 }

+// lstatEntry is one path as readDir would list it, read again just before it is acted on.
+func lstatEntry(path string) (Entry, error) {
+	info, err := os.Lstat(path)
+	if err != nil {
+		return Entry{}, err
+	}
+	return entryOf(path, filepath.Base(path), info.Mode().Type()), nil
+}
+
 func entryOf(full, name string, mode fs.FileMode) Entry {
 	entry := Entry{Name: name, Dir: mode.IsDir()}
 	if mode&fs.ModeSymlink == 0 {
@@ -31,6 +31,14 @@ const home = "/home/operator"

 func withTree(m *Manager, t tree) *Manager {
 	m.Home, m.ReadDir = home, t.readDir
+	m.Lstat = func(path string) (Entry, error) {
+		for _, e := range t[filepath.Dir(path)] {
+			if e.Name == filepath.Base(path) {
+				return e, nil
+			}
+		}
+		return Entry{}, fs.ErrNotExist
+	}
 	return m
 }

@@ -263,6 +271,7 @@ func TestAGlobIsRefusedAsAUnitByEveryVerb(t *testing.T) {
 	}
 	var calls []call
 	m := withTree(operator(fake(func(call) Ran { return Ran{} }, &calls)), tree{})
+	m.State = t.TempDir()
 	for _, tool := range tools(m) {
 		verb := strings.TrimPrefix(tool.Name, seat+".")
 		if verb == "units" || verb == "failed" {
@@ -418,6 +427,9 @@ func TestReadDirListsByLstat(t *testing.T) {
 	if !reflect.DeepEqual(entries, want) {
 		t.Fatalf("%+v", entries)
 	}
+	if e, err := lstatEntry(filepath.Join(dir, "x.service")); err != nil || e.Link != "" || e.Dangles || e.Dir {
+		t.Fatalf("%+v %v", e, err)
+	}
 	if _, err := readDir(filepath.Join(dir, "absent")); !errors.Is(err, fs.ErrNotExist) {
 		t.Fatalf("a missing directory: %v", err)
 	}