diff --git a/modules/systemd/cmd/systemd-tools/client.go b/modules/systemd/cmd/systemd-tools/client.go index 9b5af653..c7c837e3 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,17 @@ 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 + // 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 02cc47a6..4b9f4280 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 @@ -10,6 +10,7 @@ import ( "fmt" "os" "os/user" + "path/filepath" "strconv" "strings" @@ -145,6 +146,30 @@ 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; 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) { + 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 +209,9 @@ 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")), 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 new file mode 100644 index 00000000..462bcf29 --- /dev/null +++ b/modules/systemd/cmd/systemd-tools/unlink.go @@ -0,0 +1,221 @@ +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. +// +// **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" + "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" + +// 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"` + 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). +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, notRemoved, left := []Unlinked{}, []Unlinked{}, []map[string]string{} + unrecorded, unpublished := []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), 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 (removed before it: %d): %w", l.Path, len(removed), err) + } + rec.Step, rec.Outcome = "outcome", "removed" + if err := m.unlink(l.Path); err != nil { + 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) + } + } + answer := map[string]any{"unit": unit, "scope": string(scope), "removed": removed, "left": left, + "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 { + 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. + 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. 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 +} + +func (m *Manager) now() time.Time { + if m.Now != nil { + return m.Now() + } + 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 { + 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 + } + 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 { + 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..697e3993 --- /dev/null +++ b/modules/systemd/cmd/systemd-tools/unlink_test.go @@ -0,0 +1,333 @@ +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" + "errors" + "io/fs" + "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 } + 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 { + 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) + } + // Two lines a link, intent then outcome, in the order they were removed. + got := records(t, m) + 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 { + 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) + } +} + +// **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 4fa67f80..ac2d7913 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" @@ -126,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) { @@ -178,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 @@ -235,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 @@ -275,8 +291,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 +343,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..11ee4042 100644 --- a/modules/systemd/module.json +++ b/modules/systemd/module.json @@ -20,10 +20,14 @@ "journal", "failed", "reset-failed", - "wanted-by" + "wanted-by", + "unlink-dangling" ] } ], + "emits": [ + "link.removal" + ], "build": { "artifacts": [ { @@ -35,7 +39,10 @@ "binary": "systemd-tools", "loads": [ "systemd-tools" - ] + ], + "env": { + "MESH_SYSTEMD_STATE": "${dir:state}" + } } ] }, @@ -44,6 +51,12 @@ "id": "package", "type": "package", "package": "systemd" + }, + { + "id": "state", + "type": "directory", + "mode": "0700", + "place": "." } ] }