From 87c9d9f7e5b143bfa43bc29cafc2f8d048a5747e Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 8 Oct 2026 17:39:07 +0200 Subject: [PATCH 1/2] 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). --- modules/systemd/cmd/systemd-tools/client.go | 8 +- modules/systemd/cmd/systemd-tools/main.go | 30 ++- modules/systemd/cmd/systemd-tools/unlink.go | 145 ++++++++++++++ .../systemd/cmd/systemd-tools/unlink_test.go | 177 ++++++++++++++++++ modules/systemd/cmd/systemd-tools/wanted.go | 17 +- .../systemd/cmd/systemd-tools/wanted_test.go | 12 ++ modules/systemd/module.json | 15 +- 7 files changed, 395 insertions(+), 9 deletions(-) create mode 100644 modules/systemd/cmd/systemd-tools/unlink.go create mode 100644 modules/systemd/cmd/systemd-tools/unlink_test.go diff --git a/modules/systemd/cmd/systemd-tools/client.go b/modules/systemd/cmd/systemd-tools/client.go index 9b5af653..2d21da76 100644 --- a/modules/systemd/cmd/systemd-tools/client.go +++ b/modules/systemd/cmd/systemd-tools/client.go @@ -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`) diff --git a/modules/systemd/cmd/systemd-tools/main.go b/modules/systemd/cmd/systemd-tools/main.go index 02cc47a6..06b4eb80 100644 --- a/modules/systemd/cmd/systemd-tools/main.go +++ b/modules/systemd/cmd/systemd-tools/main.go @@ -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() { diff --git a/modules/systemd/cmd/systemd-tools/unlink.go b/modules/systemd/cmd/systemd-tools/unlink.go new file mode 100644 index 00000000..1a7aafc4 --- /dev/null +++ b/modules/systemd/cmd/systemd-tools/unlink.go @@ -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 ) 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) +} diff --git a/modules/systemd/cmd/systemd-tools/unlink_test.go b/modules/systemd/cmd/systemd-tools/unlink_test.go new file mode 100644 index 00000000..6ebe3d66 --- /dev/null +++ b/modules/systemd/cmd/systemd-tools/unlink_test.go @@ -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) + } +} diff --git a/modules/systemd/cmd/systemd-tools/wanted.go b/modules/systemd/cmd/systemd-tools/wanted.go index 4fa67f80..a2612572 100644 --- a/modules/systemd/cmd/systemd-tools/wanted.go +++ b/modules/systemd/cmd/systemd-tools/wanted.go @@ -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 { diff --git a/modules/systemd/cmd/systemd-tools/wanted_test.go b/modules/systemd/cmd/systemd-tools/wanted_test.go index c8d0cb8f..5013d68a 100644 --- a/modules/systemd/cmd/systemd-tools/wanted_test.go +++ b/modules/systemd/cmd/systemd-tools/wanted_test.go @@ -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) } diff --git a/modules/systemd/module.json b/modules/systemd/module.json index f48d983f..a5ed503b 100644 --- a/modules/systemd/module.json +++ b/modules/systemd/module.json @@ -20,7 +20,8 @@ "journal", "failed", "reset-failed", - "wanted-by" + "wanted-by", + "unlink-dangling" ] } ], @@ -35,7 +36,10 @@ "binary": "systemd-tools", "loads": [ "systemd-tools" - ] + ], + "env": { + "MESH_SYSTEMD_STATE": "${dir:state}" + } } ] }, @@ -44,6 +48,13 @@ "id": "package", "type": "package", "package": "systemd" + }, + { + "id": "state", + "type": "directory", + "mode": "0700", + "owner": "${machine:account}", + "place": "." } ] } From 1b368f9aaf59d609bea1897f4d562728c949be34 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 8 Oct 2026 17:55:56 +0200 Subject: [PATCH 2/2] Record a removal's outcome, publish it, and read one directory once The record kept only the intent, so a failed unlink read as removed; each link now gets a synced intent and outcome line, and the outcome is the module's link.removal event so it is not only a file on one machine. The put-back hint no longer advises a symlink, an aliased directory is read once, a machine-wide removal from user scope is said, and the state directory names no owner a machine without an account cannot resolve. --- modules/systemd/cmd/systemd-tools/client.go | 5 + modules/systemd/cmd/systemd-tools/main.go | 7 +- modules/systemd/cmd/systemd-tools/unlink.go | 114 ++++++++++-- .../systemd/cmd/systemd-tools/unlink_test.go | 162 +++++++++++++++++- modules/systemd/cmd/systemd-tools/wanted.go | 23 ++- modules/systemd/module.json | 4 +- 6 files changed, 286 insertions(+), 29 deletions(-) diff --git a/modules/systemd/cmd/systemd-tools/client.go b/modules/systemd/cmd/systemd-tools/client.go index 2d21da76..c7c837e3 100644 --- a/modules/systemd/cmd/systemd-tools/client.go +++ b/modules/systemd/cmd/systemd-tools/client.go @@ -147,6 +147,11 @@ type Manager struct { // State is the module's state directory, where each removed link is recorded. State string Now func() time.Time + // Emit publishes one of this module's events (unlink.go). + Emit func(eventType string, body any) error + // Resolve is a directory's path with every link in it followed, so one directory reached by two + // names is searched once (on Arch /etc/xdg/systemd/user leads to /etc/systemd/user). + Resolve func(path string) (string, error) } var userBus = regexp.MustCompile(`(?i)Failed to connect to (user scope )?bus`) diff --git a/modules/systemd/cmd/systemd-tools/main.go b/modules/systemd/cmd/systemd-tools/main.go index 06b4eb80..4b9f4280 100644 --- a/modules/systemd/cmd/systemd-tools/main.go +++ b/modules/systemd/cmd/systemd-tools/main.go @@ -10,6 +10,7 @@ import ( "fmt" "os" "os/user" + "path/filepath" "strconv" "strings" @@ -153,7 +154,8 @@ func tools(m *Manager) []stdio.Tool { "`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.", + "and said; a user-scope call that removes a machine-wide link says so. Each outcome is also published " + + "as the module's link.removal event. `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) { @@ -208,7 +210,8 @@ func fromEnv() *Manager { } return &Manager{Account: account, UID: uid, User: name, Run: execRunner, Read: readFile, Env: os.Environ(), Home: home, ReadDir: readDir, Lstat: lstatEntry, Remove: os.Remove, - State: strings.TrimSpace(os.Getenv("MESH_SYSTEMD_STATE"))} + State: strings.TrimSpace(os.Getenv("MESH_SYSTEMD_STATE")), Resolve: filepath.EvalSymlinks, + Emit: func(eventType string, body any) error { return stdio.Emit(eventType, body) }} } func main() { diff --git a/modules/systemd/cmd/systemd-tools/unlink.go b/modules/systemd/cmd/systemd-tools/unlink.go index 1a7aafc4..462bcf29 100644 --- a/modules/systemd/cmd/systemd-tools/unlink.go +++ b/modules/systemd/cmd/systemd-tools/unlink.go @@ -12,10 +12,21 @@ package main // 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. +// **The record never claims what did not happen.** Each link gets two lines in the module's state, each +// synced to disk before the next step: the intent (its path, where it pointed, what wanted the unit, the +// person's reason) before it is removed, and the outcome — removed, or not removed and why — after. A line +// that is an intent with no outcome after it is a removal interrupted, said as such. The outcome is also +// published on the bus as this module's `link.removal` event, so the record is not only a file on one +// machine; it carries no secret, only the names and the reason. +// +// The record's directory is the module's `state`, which the manifest gives no owner on purpose: a +// manifest cannot say ${machine:account} on a machine that has no operator account. The controller makes a +// directory a bundle's words name the account's where the machine has one (novox/hq ADR 0192), and there +// the runtime runs as root and the directory stays root's — either way it is the tools' own. +// +// 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. A user-scope call that removes a machine-wide link (one in +// /etc/systemd/user, read by every account's manager) says so. import ( "encoding/json" @@ -29,15 +40,29 @@ import ( // 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. +// RemovalEvent is the event each attempted removal is published as (module.json's emits). +const RemovalEvent = "link.removal" + +// Unlinked is one line of the record: an intent, or the outcome that follows it. type Unlinked struct { - At string `json:"at"` - Scope string `json:"scope"` - Unit string `json:"unit"` - Path string `json:"path"` + At string `json:"at"` + Step string `json:"step"` // "intent" or "outcome" + Scope string `json:"scope"` + Unit string `json:"unit"` + Path string `json:"path"` + // Target is where the link pointed: what enabling the unit makes again, once the unit exists. Target string `json:"target"` Of string `json:"of"` Why string `json:"why"` + // Outcome is "removed" or "not removed: ", on an outcome line only. + Outcome string `json:"outcome,omitempty"` + // MachineWide is whether the link was in a directory every account's manager reads. + MachineWide bool `json:"machine_wide,omitempty"` +} + +// machineWide is whether a user-scope link sits where every account's manager reads it. +func (m *Manager) machineWide(scope Scope, path string) bool { + return scope == User && !(m.Home != "" && strings.HasPrefix(path, filepath.Clean(m.Home)+string(filepath.Separator))) } // UnlinkDangling removes the dangling enable links named for one unit, on the person's word (why). @@ -51,8 +76,9 @@ func (m *Manager) UnlinkDangling(scope Scope, unit, why string) (map[string]any, 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{} + links, unread, _ := m.links(scope, unit) + removed, notRemoved, left := []Unlinked{}, []Unlinked{}, []map[string]string{} + unrecorded, unpublished := []string{}, []string{} for _, l := range links { reason := "" switch { @@ -75,23 +101,58 @@ func (m *Manager) UnlinkDangling(scope Scope, unit, why string) (map[string]any, 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} + rec := Unlinked{At: m.now().UTC().Format(time.RFC3339), Step: "intent", Scope: string(scope), Unit: unit, + Path: l.Path, Target: l.Target, Of: l.Of, Why: why, MachineWide: m.machineWide(scope, l.Path)} 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) + return nil, fmt.Errorf("the record of %s could not be kept, so it was not removed (removed before it: %d): %w", l.Path, len(removed), err) } + rec.Step, rec.Outcome = "outcome", "removed" 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)) + rec.Outcome = "not removed: " + err.Error() + } + rec.At = m.now().UTC().Format(time.RFC3339) + if err := m.record(rec); err != nil { + unrecorded = append(unrecorded, l.Path+": "+err.Error()) + } + if err := m.emit(rec); err != nil { + unpublished = append(unpublished, l.Path+": "+err.Error()) + } + if rec.Outcome == "removed" { + removed = append(removed, rec) + } else { + notRemoved = append(notRemoved, rec) } - removed = append(removed, rec) } answer := map[string]any{"unit": unit, "scope": string(scope), "removed": removed, "left": left, - "record": filepath.Join(m.State, UnlinkedRecord)} + "record": filepath.Join(m.State, UnlinkedRecord), "ok": len(notRemoved) == 0 && len(unrecorded) == 0} + if len(notRemoved) > 0 { + answer["not_removed"] = notRemoved + } + if len(unrecorded) > 0 { + answer["unrecorded"] = unrecorded + answer["record_note"] = "these outcomes are not in the record, which ends at their intent: the answer here is what happened" + } + if len(unpublished) > 0 { + answer["unpublished"] = unpublished + } if len(unread) > 0 { answer["unread"] = unread } + var wide []string + for _, r := range removed { + if r.MachineWide { + wide = append(wide, r.Path) + } + } + if len(wide) > 0 { + answer["machine_wide"] = wide + answer["machine_wide_note"] = "these links were in the machine's configuration for every account, not only the operator account's own: " + + "no account's manager asks for " + unit + " through them any more" + } 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" + if len(notRemoved) == 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. @@ -100,7 +161,8 @@ func (m *Manager) UnlinkDangling(scope Scope, unit, why string) (map[string]any, } 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 ) if it was wanted" + answer["next"] = "`reset-failed` clears the unit's failed record. A removed link comes back by enabling the unit through " + + "node-service-manager.enable, once its unit file exists again; the record says what each link was" return answer, nil } @@ -111,6 +173,7 @@ func (m *Manager) now() time.Time { return time.Now() } +// record appends one line to the record and syncs it to disk before answering. func (m *Manager) record(rec Unlinked) error { line, err := json.Marshal(rec) if err != nil { @@ -127,9 +190,22 @@ func (m *Manager) record(rec Unlinked) error { f.Close() return err } + if err := f.Sync(); err != nil { + f.Close() + return err + } return f.Close() } +// emit publishes an outcome as this module's event: the names and the reason, nothing else. +func (m *Manager) emit(rec Unlinked) error { + if m.Emit == nil { + return fmt.Errorf("this process publishes no events") + } + return m.Emit(RemovalEvent, map[string]any{"unit": rec.Unit, "scope": rec.Scope, "path": rec.Path, + "target": rec.Target, "of": rec.Of, "why": rec.Why, "outcome": rec.Outcome, "machine_wide": rec.MachineWide, "at": rec.At}) +} + // 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 { diff --git a/modules/systemd/cmd/systemd-tools/unlink_test.go b/modules/systemd/cmd/systemd-tools/unlink_test.go index 6ebe3d66..697e3993 100644 --- a/modules/systemd/cmd/systemd-tools/unlink_test.go +++ b/modules/systemd/cmd/systemd-tools/unlink_test.go @@ -6,6 +6,8 @@ package main import ( "bufio" "encoding/json" + "errors" + "io/fs" "os" "path/filepath" "reflect" @@ -37,9 +39,22 @@ func unlinker(t *testing.T, fsys tree, calls *[]call, removed *[]string) *Manage 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 } + m.Emit = func(eventType string, body any) error { + emitted = append(emitted, event{eventType, body.(map[string]any)}) + return nil + } + emitted = nil return m } +type event struct { + kind string + body map[string]any +} + +// emitted is what the last unlinker published. +var emitted []event + func records(t *testing.T, m *Manager) []Unlinked { f, err := os.Open(filepath.Join(m.State, UnlinkedRecord)) if err != nil { @@ -83,12 +98,36 @@ func TestUnlinkDanglingRemovesOnlyTheDanglingLinksNamedForTheUnitInOwnConfigurat if !sudo || !reload { t.Fatalf("%+v", calls) } + // Two lines a link, intent then outcome, in the order they were removed. 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" { + steps := []string{} + for _, r := range got { + steps = append(steps, r.Step+" "+r.Outcome+" "+r.Path) + } + wide := "/etc/systemd/user/graphical-session.target.wants/greenclip.service" + if !reflect.DeepEqual(steps, []string{"intent " + wide, "outcome removed " + wide, "intent " + own, "outcome removed " + own}) { + t.Fatalf("%q", steps) + } + if r := got[3]; r.Target != "/usr/lib/systemd/user/greenclip.service" || r.Of != "default.target" || r.MachineWide || + r.Why != "greenclip was replaced by clipmenu" || r.At != "2026-10-08T17:30:00Z" || r.Scope != "user" || !got[1].MachineWide { t.Fatalf("%+v", got) } + // Each outcome published as the module's event, with the names and the reason and nothing else. + if len(emitted) != 2 || emitted[1].kind != RemovalEvent || emitted[1].body["path"] != own || + emitted[1].body["outcome"] != "removed" || emitted[1].body["why"] != "greenclip was replaced by clipmenu" || + emitted[1].body["unit"] != "greenclip.service" || emitted[1].body["target"] != "/usr/lib/systemd/user/greenclip.service" { + t.Fatalf("%+v", emitted) + } + // A user-scope call that removed a machine-wide link says so. + if w, _ := answer["machine_wide"].([]string); !reflect.DeepEqual(w, []string{wide}) || answer["machine_wide_note"] == nil { + t.Fatalf("%v", answer) + } + if next, _ := answer["next"].(string); strings.Contains(next, "ln -s") || !strings.Contains(next, "node-service-manager.enable") { + t.Fatalf("%q", next) + } + if answer["ok"] != true { + t.Fatalf("%v", answer) + } left := answer["left"].([]map[string]string) why := map[string]string{} for _, l := range left { @@ -175,3 +214,120 @@ func TestUnlinkDanglingInTheSystemScopeGoesThroughSudo(t *testing.T) { t.Fatalf("%v", answer) } } + +// **The record never claims a removal that failed** (the re-review of #138): the intent is on disk before the +// link is touched, and the outcome after it says it was not removed and why — in the record, the event and +// the answer alike. +func TestAnUnlinkThatFailsAfterTheIntentIsRecordedAsNotRemoved(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: "/gone/x.service", Dangles: true}}, + } + var calls []call + var removed []string + m := unlinker(t, fsys, &calls, &removed) + m.Remove = func(p string) error { + // The intent is already on disk when the removal is tried. + if got := records(t, m); len(got) != 1 || got[0].Step != "intent" { + t.Fatalf("removed before the intent was kept: %+v", got) + } + return fs.ErrPermission + } + answer, err := m.UnlinkDangling(User, "x.service", "test") + if err != nil { + t.Fatal(err) + } + got := records(t, m) + if len(got) != 2 || got[1].Step != "outcome" || !strings.HasPrefix(got[1].Outcome, "not removed: ") || !strings.Contains(got[1].Outcome, "permission denied") { + t.Fatalf("%+v", got) + } + if answer["ok"] != false || len(answer["removed"].([]Unlinked)) != 0 || len(answer["not_removed"].([]Unlinked)) != 1 { + t.Fatalf("%v", answer) + } + if len(emitted) != 1 || !strings.HasPrefix(emitted[0].body["outcome"].(string), "not removed: ") { + t.Fatalf("%+v", emitted) + } + for _, c := range calls { + if contains(c.args, "daemon-reload") { + t.Fatal("reloaded with nothing removed") + } + } +} + +// An event that cannot be published is said, and the removal and its record stand. +func TestAnUnpublishedOutcomeIsSaid(t *testing.T) { + var removed []string + m := unlinker(t, leftovers(), nil, &removed) + m.Emit = func(string, any) error { return errors.New("no bus") } + answer, err := m.UnlinkDangling(User, "greenclip.service", "test") + if err != nil { + t.Fatal(err) + } + if u, _ := answer["unpublished"].([]string); len(u) != 2 || len(removed) != 1 { + t.Fatalf("%v %v", answer, removed) + } +} + +// A dependency directory that is itself a link is never removed through, even into the searched +// directories: such a link is listed by wanted-by, and left by unlink-dangling. +func TestUnlinkDanglingRefusesALinkReachedThroughALinkedDependencyDirectory(t *testing.T) { + user := home + "/.config/systemd/user" + fsys := tree{ + user: {{Name: "default.target.wants", Link: "/etc/systemd/user/shared.wants"}}, + "/etc/systemd/user/shared.wants": {{Name: "x.service", Link: "/gone/x.service", Dangles: true}}, + } + var calls []call + var removed []string + m := unlinker(t, fsys, &calls, &removed) + answer, err := m.UnlinkDangling(User, "x.service", "test") + if err != nil { + t.Fatal(err) + } + left := answer["left"].([]map[string]string) + if len(removed) != 0 || len(calls) != 0 || len(left) != 1 || !strings.Contains(left[0]["why_left"], "reached through a linked directory") { + t.Fatalf("%v %v %v", answer, removed, calls) + } +} + +// One directory reached by two names (Arch's /etc/xdg/systemd/user leads to /etc/systemd/user) is read +// once: its link is listed once and removed once. +func TestADirectoryReachedByTwoNamesIsReadOnce(t *testing.T) { + fsys := tree{ + "/etc/systemd/user": {{Name: "default.target.wants", Dir: true}}, + "/etc/systemd/user/default.target.wants": {{Name: "x.service", Link: "/gone/x.service", Dangles: true}}, + "/etc/xdg/systemd/user": {{Name: "default.target.wants", Dir: true}}, + "/etc/xdg/systemd/user/default.target.wants": {{Name: "x.service", Link: "/gone/x.service", Dangles: true}}, + } + var calls []call + var removed []string + m := unlinker(t, fsys, &calls, &removed) + m.Resolve = func(p string) (string, error) { + if p == "/etc/xdg/systemd/user" { + return "/etc/systemd/user", nil + } + return p, nil + } + answer, err := m.WantedBy(User, "x.service") + if err != nil { + t.Fatal(err) + } + if links := answer["links"].([]EnableLink); len(links) != 1 || links[0].Path != "/etc/systemd/user/default.target.wants/x.service" { + t.Fatalf("%+v", links) + } + if same, _ := answer["same_directory"].([]string); len(same) != 1 || answer["unread"] != nil { + t.Fatalf("%v", answer) + } + if _, err := m.UnlinkDangling(User, "x.service", "test"); err != nil { + t.Fatal(err) + } + unlinks := 0 + for _, c := range calls { + if c.cmd == "sudo" && contains(c.args, "unlink") { + unlinks++ + } + } + if unlinks != 1 { + t.Fatalf("removed %d times: %+v", unlinks, calls) + } +} diff --git a/modules/systemd/cmd/systemd-tools/wanted.go b/modules/systemd/cmd/systemd-tools/wanted.go index a2612572..ac2d7913 100644 --- a/modules/systemd/cmd/systemd-tools/wanted.go +++ b/modules/systemd/cmd/systemd-tools/wanted.go @@ -127,10 +127,22 @@ func within(path string, dirs []ConfigDir) bool { // empty; one that does not exist is the ordinary case and is not. **A dependency directory that is itself a // link is followed only when it leads into the searched directories**: elsewhere it is said, and not read, // so a link cannot point the search, or a removal after it, at any directory on the machine. -func (m *Manager) links(scope Scope, unit string) ([]EnableLink, []string) { - found, unread := []EnableLink{}, []string{} +func (m *Manager) links(scope Scope, unit string) ([]EnableLink, []string, []string) { + found, unread, same := []EnableLink{}, []string{}, []string{} dirs := m.configDirs(scope) + seen := map[string]string{} for _, dir := range dirs { + // One directory reached by two names is read once, under the first: the same link is not listed + // twice, nor removed through the second name. + if m.Resolve != nil { + if real, err := m.Resolve(dir.Path); err == nil { + if first, twice := seen[real]; twice { + same = append(same, dir.Path+" is "+first+", read under that name") + continue + } + seen[real] = dir.Path + } + } entries, err := m.ReadDir(dir.Path) if err != nil { if !errors.Is(err, fs.ErrNotExist) { @@ -179,7 +191,7 @@ func (m *Manager) links(scope Scope, unit string) ([]EnableLink, []string) { } } sort.Slice(found, func(i, j int) bool { return found[i].Path < found[j].Path }) - return found, unread + return found, unread, same } // WantedBy is what wants, requires or triggers one unit: the manager's reverse dependencies and the enable @@ -236,8 +248,11 @@ func (m *Manager) WantedBy(scope Scope, unit string) (map[string]any, error) { } answer["manager"] = manager - links, unread := m.links(scope, unit) + links, unread, same := m.links(scope, unit) answer["links"] = links + if len(same) > 0 { + answer["same_directory"] = same + } answer["searched"] = m.configDirs(scope) if len(unread) > 0 { answer["unread"] = unread diff --git a/modules/systemd/module.json b/modules/systemd/module.json index a5ed503b..11ee4042 100644 --- a/modules/systemd/module.json +++ b/modules/systemd/module.json @@ -25,6 +25,9 @@ ] } ], + "emits": [ + "link.removal" + ], "build": { "artifacts": [ { @@ -53,7 +56,6 @@ "id": "state", "type": "directory", "mode": "0700", - "owner": "${machine:account}", "place": "." } ]