From adf3c366328bcf961718c2118908e87f2471e849 Mon Sep 17 00:00:00 2001
From: Tai Groot <tai@taigrr.com>
Date: Thu, 5 Mar 2026 12:26:12 -0500
Subject: [PATCH] fix(helpers): return ErrValueNotSet for nonexistent units in
 GetNumRestarts (#9)

systemd returns NRestarts=0 for units with LoadState=not-found, making
it indistinguishable from a genuinely loaded unit with zero restarts.
GetNumRestarts now checks LoadState when NRestarts is 0 and returns
ErrValueNotSet for units that don't exist, matching GetMemoryUsage
behavior.

Also adds unit tests for filterErr (all stderr error mapping cases)
and HasValidUnitSuffix (all valid unit types + negative cases).

Updates syncthing test expectation from ErrValueNotSet to nil since
loaded-but-inactive units legitimately have NRestarts=0.
---
 filtererr_test.go | 119 ++++++++++++++++++++++++++++++++++++++++++++++
 helpers.go        |  18 ++++++-
 helpers_test.go   |   4 +-
 3 files changed, 138 insertions(+), 3 deletions(-)
 create mode 100644 filtererr_test.go

diff --git a/filtererr_test.go b/filtererr_test.go
new file mode 100644
index 0000000..e7689d2
--- /dev/null
+++ b/filtererr_test.go
@@ -0,0 +1,119 @@
+package systemctl
+
+import (
+	"errors"
+	"testing"
+)
+
+func TestFilterErr(t *testing.T) {
+	tests := []struct {
+		name   string
+		stderr string
+		want   error
+	}{
+		{
+			name:   "empty stderr",
+			stderr: "",
+			want:   nil,
+		},
+		{
+			name:   "unit does not exist",
+			stderr: "Unit foo.service does not exist, proceeding anyway.",
+			want:   ErrDoesNotExist,
+		},
+		{
+			name:   "unit not found",
+			stderr: "Unit foo.service not found.",
+			want:   ErrDoesNotExist,
+		},
+		{
+			name:   "unit not loaded",
+			stderr: "Unit foo.service not loaded.",
+			want:   ErrUnitNotLoaded,
+		},
+		{
+			name:   "no such file or directory",
+			stderr: "No such file or directory",
+			want:   ErrDoesNotExist,
+		},
+		{
+			name:   "interactive authentication required",
+			stderr: "Interactive authentication required.",
+			want:   ErrInsufficientPermissions,
+		},
+		{
+			name:   "access denied",
+			stderr: "Access denied",
+			want:   ErrInsufficientPermissions,
+		},
+		{
+			name:   "dbus session bus address",
+			stderr: "Failed to connect to bus: $DBUS_SESSION_BUS_ADDRESS not set",
+			want:   ErrBusFailure,
+		},
+		{
+			name:   "unit is masked",
+			stderr: "Unit foo.service is masked.",
+			want:   ErrMasked,
+		},
+		{
+			name:   "generic failed",
+			stderr: "Failed to do something unknown",
+			want:   ErrUnspecified,
+		},
+		{
+			name:   "unrecognized warning",
+			stderr: "Warning: something benign happened",
+			want:   nil,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := filterErr(tt.stderr)
+			if tt.want == nil {
+				if got != nil {
+					t.Errorf("filterErr(%q) = %v, want nil", tt.stderr, got)
+				}
+				return
+			}
+			if !errors.Is(got, tt.want) {
+				t.Errorf("filterErr(%q) = %v, want error wrapping %v", tt.stderr, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestHasValidUnitSuffix(t *testing.T) {
+	tests := []struct {
+		unit string
+		want bool
+	}{
+		{"nginx.service", true},
+		{"sshd.socket", true},
+		{"backup.timer", true},
+		{"dev-sda1.device", true},
+		{"home.mount", true},
+		{"dev-sda1.swap", true},
+		{"user.slice", true},
+		{"multi-user.target", true},
+		{"session-1.scope", true},
+		{"foo.automount", true},
+		{"backup.path", true},
+		{"foo.snapshot", true},
+		{"nginx", false},
+		{"", false},
+		{"foo.bar", false},
+		{"foo.services", false},
+		{".service", true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.unit, func(t *testing.T) {
+			got := HasValidUnitSuffix(tt.unit)
+			if got != tt.want {
+				t.Errorf("HasValidUnitSuffix(%q) = %v, want %v", tt.unit, got, tt.want)
+			}
+		})
+	}
+}
diff --git a/helpers.go b/helpers.go
index ad62fcf..51285f7 100644
--- a/helpers.go
+++ b/helpers.go
@@ -32,7 +32,23 @@ func GetNumRestarts(ctx context.Context, unit string, opts Options) (int, error)
 	if err != nil {
 		return -1, err
 	}
-	return strconv.Atoi(value)
+	if value == "[not set]" {
+		return -1, ErrValueNotSet
+	}
+	restarts, err := strconv.Atoi(value)
+	if err != nil {
+		return -1, err
+	}
+	// systemd returns NRestarts=0 for both genuinely zero-restart units and
+	// nonexistent/unloaded units. Disambiguate by checking LoadState: if the
+	// unit isn't loaded, the value is meaningless.
+	if restarts == 0 {
+		loadState, loadErr := Show(ctx, unit, properties.LoadState, opts)
+		if loadErr == nil && loadState == "not-found" {
+			return -1, ErrValueNotSet
+		}
+	}
+	return restarts, nil
 }
 
 // Get current memory in bytes (`systemctl show [unit] --property MemoryCurrent`) as an int
diff --git a/helpers_test.go b/helpers_test.go
index 9b91c9d..a468d8c 100644
--- a/helpers_test.go
+++ b/helpers_test.go
@@ -105,8 +105,8 @@ func TestGetNumRestarts(t *testing.T) {
 
 		// try nonexistant unit in user mode as user
 		{"nonexistant", ErrValueNotSet, Options{UserMode: false}, true},
-		// try existing unit in user mode as user
-		{"syncthing", ErrValueNotSet, Options{UserMode: true}, true},
+		// try existing unit in user mode as user (loaded, so NRestarts=0 is valid)
+		{"syncthing", nil, Options{UserMode: true}, true},
 		// try existing unit in system mode as user
 		{"nginx", nil, Options{UserMode: false}, true},