🐛 os: fix services.list reporting wrong running state for systemd services

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
 		}
 	}
 

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)
 }