From 49edba95de302956e99e4d4273bbfc717730628a Mon Sep 17 00:00:00 2001 From: Jay Mundrawala Date: Thu, 12 Mar 2026 15:42:55 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20os:=20fix=20services.list=20repo?= =?UTF-8?q?rting=20wrong=20running=20state=20for=20systemd=20services?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit services.list reported running=false for services that were actually running (e.g., systemd-journald). The root cause was in the batched "systemctl show" approach used by List(): template units (name@.service) cause systemctl show to fail with no stdout output and no blank-line separator. Since ParseServiceSystemDShow uses blank lines to delimit records, the service before a template and the service after it get merged into one record, silently losing the first service's data. Fix: Replace the batched "systemctl show" calls with a single "systemctl list-units --type service --all" command, which directly reports ActiveState for all loaded services. This uses the already- defined SYSTEMD_LIST_UNITS_REGEX and eliminates the template unit issue entirely. Also hardens ParseServiceSystemDShow with defense-in-depth: if a new Id= is encountered while building a record (missing separator), flush the current record first. Co-Authored-By: Claude Opus 4.6 --- providers/os/resources/services/systemd.go | 97 +++++++++---- .../os/resources/services/systemd_test.go | 137 +++++++++++++++--- 2 files changed, 186 insertions(+), 48 deletions(-) diff --git a/providers/os/resources/services/systemd.go b/providers/os/resources/services/systemd.go index 8af4c38873..4830b6385c 100644 --- a/providers/os/resources/services/systemd.go +++ b/providers/os/resources/services/systemd.go @@ -21,14 +21,13 @@ import ( ) var ( - SYSTEMD_LIST_UNITS_REGEX = regexp.MustCompile(`(?m)^(?:[^\S\n]{2}|●[^\S\n]|)(\S+)(?:[^\S\n])+(loaded|not-found|masked)(?:[^\S\n])+(\S+)(?:[^\S\n])+(\S+)(?:[^\S\n])+(.+)$`) + SYSTEMD_LIST_UNITS_REGEX = regexp.MustCompile(`(?m)^(?:[^\S\n]{2}|●[^\S\n]|)(\S+)(?:[^\S\n])+(\S+)(?:[^\S\n])+(\S+)(?:[^\S\n])+(\S+)(?:[^\S\n])+(.+)$`) serviceNameRegex = regexp.MustCompile(`(.*)\.(service|target|socket)$`) errIgnored = errors.New("ignored") ) const ( systemdShowProperties = "Id,LoadState,ActiveState,UnitFileState,Description" - systemdShowChunkSize = 100 ) func ResolveSystemdServiceManager(conn shared.Connection) OSServiceManager { @@ -82,6 +81,36 @@ func ParseServiceSystemDUnitFiles(input io.Reader) ([]*Service, error) { return services, nil } +// ParseSystemdListUnits parses the output of "systemctl list-units --type service --all" +// and returns a map of normalized service name to Service. This provides running state +// directly from systemd without the fragility of batched "systemctl show" calls. +func ParseSystemdListUnits(input io.Reader) (map[string]*Service, error) { + content, err := io.ReadAll(input) + if err != nil { + return nil, fmt.Errorf("error reading systemctl list-units output: %v", err) + } + + services := map[string]*Service{} + matches := SYSTEMD_LIST_UNITS_REGEX.FindAllStringSubmatch(string(content), -1) + for _, match := range matches { + unitName := match[1] + if !strings.HasSuffix(unitName, ".service") { + continue + } + + name := normalizeSystemdServiceName(unitName) + services[name] = &Service{ + Name: name, + Description: strings.TrimSpace(match[5]), + Running: match[3] == "active", + Installed: match[2] != "not-found", + Type: "systemd", + } + } + + return services, nil +} + func applySystemdUnitFileState(service *Service, unitFileState string) { service.Enabled = unitFileState == "enabled" || unitFileState == "enabled-runtime" service.Masked = strings.HasPrefix(unitFileState, "masked") @@ -144,6 +173,16 @@ func ParseServiceSystemDShow(input io.Reader) (map[string]*Service, error) { if !ok { return nil, fmt.Errorf("unexpected output from systemctl show: %q", line) } + + // Defense in depth: if we encounter a new Id= while already building a + // record, flush the current one first. This handles cases where systemctl + // omits the blank-line separator (e.g., when a template unit fails). + if key == "Id" && record["Id"] != "" { + if err := flushRecord(); err != nil { + return nil, err + } + } + record[key] = value } @@ -199,8 +238,13 @@ func (s *SystemDServiceManager) Get(name string) (*Service, error) { return service, nil } -// List returns a slice of Service structs representing the state of all services +// List returns a slice of Service structs representing the state of all services. +// It uses list-unit-files for the complete set (including unloaded services) and +// list-units for running state. This avoids batched "systemctl show" which fails +// on template units (name@.service), causing missing blank-line separators that +// merge adjacent records and silently lose service data. func (s *SystemDServiceManager) List() ([]*Service, error) { + // Step 1: Get all service unit files (provides Enabled/Masked/Static/Installed) cmdList, err := s.conn.RunCommand("systemctl list-unit-files --type service --all") if err != nil { return nil, err @@ -211,36 +255,29 @@ func (s *SystemDServiceManager) List() ([]*Service, error) { return nil, err } - for start := 0; start < len(services); start += systemdShowChunkSize { - end := start + systemdShowChunkSize - if end > len(services) { - end = len(services) - } + // Step 2: Get running state from list-units (provides Running/Description) + cmdUnits, err := s.conn.RunCommand("systemctl list-units --type service --all") + if err != nil { + return nil, err + } - units := make([]string, 0, end-start) - for _, service := range services[start:end] { - units = append(units, ensureSystemdServiceUnit(service.Name)) - } + unitStates, err := ParseSystemdListUnits(cmdUnits.Stdout) + if err != nil { + return nil, err + } - shownServices, err := s.showUnits(units) - if err != nil { - return nil, err + // Step 3: Merge - update services from step 1 with running state from step 2. + // Services in list-unit-files but not in list-units are unloaded/templates; + // Running=false is correct for them (already the default). + for _, service := range services { + unitState, ok := unitStates[service.Name] + if !ok { + continue } - - for _, service := range services[start:end] { - shownService, ok := shownServices[service.Name] - if !ok { - continue - } - - service.Description = shownService.Description - service.Running = shownService.Running - if !shownService.Installed { - service.Installed = false - } - service.Enabled = shownService.Enabled - service.Masked = shownService.Masked - service.Static = shownService.Static + service.Description = unitState.Description + service.Running = unitState.Running + if !unitState.Installed { + service.Installed = false } } diff --git a/providers/os/resources/services/systemd_test.go b/providers/os/resources/services/systemd_test.go index a6e863d4b0..231908a96b 100644 --- a/providers/os/resources/services/systemd_test.go +++ b/providers/os/resources/services/systemd_test.go @@ -308,9 +308,97 @@ func TestSystemDServiceManagerGetReturnsNotFound(t *testing.T) { assert.Equal(t, []string{showCmd}, conn.commands) } -func TestSystemDServiceManagerListUsesBatchedShow(t *testing.T) { - const listCmd = "systemctl list-unit-files --type service --all" - const showCmd = "systemctl show --property=Id,LoadState,ActiveState,UnitFileState,Description alpha.service beta.service" +func TestParseSystemdListUnits(t *testing.T) { + input := strings.NewReader(strings.Join([]string{ + " UNIT LOAD ACTIVE SUB DESCRIPTION", + " accounts-daemon.service loaded active running Accounts Service", + " acpid.service loaded inactive dead ACPI Events Daemon", + " apparmor.service loaded active exited Load AppArmor profiles", + "● auditd.service not-found inactive dead auditd.service", + " cron.service loaded active running Regular background program processing daemon", + " ssh.service loaded inactive dead OpenBSD Secure Shell server", + " dbus.target loaded active active D-Bus", + "", + "LOAD = Reflects whether the unit definition was properly loaded.", + "ACTIVE = The high-level unit activation state, i.e. generalization of SUB.", + "SUB = The low-level unit activation state, values depend on unit type.", + "", + "7 loaded units listed.", + "", + }, "\n")) + + services, err := ParseSystemdListUnits(input) + require.NoError(t, err) + // 6 .service units (dbus.target is excluded) + require.Len(t, services, 6) + + // Active running service + assert.Equal(t, "accounts-daemon", services["accounts-daemon"].Name) + assert.True(t, services["accounts-daemon"].Running) + assert.True(t, services["accounts-daemon"].Installed) + assert.Equal(t, "Accounts Service", services["accounts-daemon"].Description) + + // Inactive (dead) service + assert.False(t, services["acpid"].Running) + assert.True(t, services["acpid"].Installed) + + // Active but exited (oneshot that ran successfully) + assert.True(t, services["apparmor"].Running) + assert.Equal(t, "Load AppArmor profiles", services["apparmor"].Description) + + // Not-found unit (preceded by bullet) + assert.False(t, services["auditd"].Running) + assert.False(t, services["auditd"].Installed) + + // SSH - inactive + assert.False(t, services["ssh"].Running) + assert.Equal(t, "OpenBSD Secure Shell server", services["ssh"].Description) + + // Non-.service unit should be excluded + assert.Nil(t, services["dbus"]) +} + +func TestParseServiceSystemDShowMergedRecords(t *testing.T) { + // Simulates what happens when systemctl show skips a template unit and + // doesn't output a blank-line separator between adjacent records. + services, err := ParseServiceSystemDShow(strings.NewReader(strings.Join([]string{ + "Id=svc-before.service", + "Description=Before Template", + "LoadState=loaded", + "ActiveState=active", + "UnitFileState=enabled", + // No blank line here — template was skipped by systemctl show + "Id=svc-after.service", + "Description=After Template", + "LoadState=loaded", + "ActiveState=inactive", + "UnitFileState=disabled", + "", + }, "\n"))) + require.NoError(t, err) + require.Len(t, services, 2) + + assert.Equal(t, &Service{ + Name: "svc-before", + Description: "Before Template", + Installed: true, + Running: true, + Enabled: true, + Type: "systemd", + }, services["svc-before"]) + assert.Equal(t, &Service{ + Name: "svc-after", + Description: "After Template", + Installed: true, + Running: false, + Enabled: false, + Type: "systemd", + }, services["svc-after"]) +} + +func TestSystemDServiceManagerListUsesListUnits(t *testing.T) { + const listFilesCmd = "systemctl list-unit-files --type service --all" + const listUnitsCmd = "systemctl list-units --type service --all" mockConn, err := mock.New(0, &inventory.Asset{ Platform: &inventory.Platform{ @@ -319,28 +407,28 @@ func TestSystemDServiceManagerListUsesBatchedShow(t *testing.T) { }, }, mock.WithData(&mock.TomlData{ Commands: map[string]*mock.Command{ - listCmd: { + listFilesCmd: { Stdout: strings.Join([]string{ "UNIT FILE STATE PRESET", "alpha.service enabled enabled", "beta.service static enabled", - "2 unit files listed.", + "gamma.service disabled enabled", + "template@.service static enabled", + "4 unit files listed.", "", }, "\n"), }, - showCmd: { + listUnitsCmd: { Stdout: strings.Join([]string{ - "Id=alpha.service", - "Description=Alpha Service", - "LoadState=loaded", - "ActiveState=active", - "UnitFileState=enabled", + " UNIT LOAD ACTIVE SUB DESCRIPTION", + " alpha.service loaded active running Alpha Service", + " beta.service loaded inactive dead Beta Service", "", - "Id=beta.service", - "Description=Beta Service", - "LoadState=loaded", - "ActiveState=inactive", - "UnitFileState=static", + "LOAD = ...", + "ACTIVE = ...", + "SUB = ...", + "", + "2 loaded units listed.", "", }, "\n"), }, @@ -353,18 +441,31 @@ func TestSystemDServiceManagerListUsesBatchedShow(t *testing.T) { services, err := mgr.List() require.NoError(t, err) - require.Len(t, services, 2) - assert.Equal(t, []string{listCmd, showCmd}, conn.commands) + require.Len(t, services, 4) + // Exactly 2 commands: list-unit-files + list-units + assert.Equal(t, []string{listFilesCmd, listUnitsCmd}, conn.commands) servicesMap := map[string]*Service{} for _, service := range services { servicesMap[service.Name] = service } + // alpha: in both list-unit-files (enabled) and list-units (active) assert.Equal(t, "Alpha Service", servicesMap["alpha"].Description) assert.True(t, servicesMap["alpha"].Running) assert.True(t, servicesMap["alpha"].Enabled) + + // beta: in both, but inactive assert.Equal(t, "Beta Service", servicesMap["beta"].Description) assert.False(t, servicesMap["beta"].Running) assert.True(t, servicesMap["beta"].Static) + + // gamma: only in list-unit-files (not loaded), Running stays false + assert.False(t, servicesMap["gamma"].Running) + assert.False(t, servicesMap["gamma"].Enabled) + assert.Equal(t, "", servicesMap["gamma"].Description) + + // template@: only in list-unit-files, correctly not running + assert.False(t, servicesMap["template@"].Running) + assert.True(t, servicesMap["template@"].Static) }