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": "." } ]