fix: surface faults on the AMI-read and service-ownership paths (#679)

DescribeImages skipped any ami-* prefix whose config.json could not be
fetched, read, or parsed, logging each at DEBUG. At the daemon's default
INFO level the result was an empty Images list and a completion line
reading count=0, indistinguishable from a healthy empty registry.

Promote the three fault branches plus the body-close failure to WARN,
leave NoSuchKey at DEBUG since a prefix with no config is a legitimate
non-AMI directory, and report prefixes examined alongside images
returned so a mismatch is visible without raising the log level.

SetServiceOwnership chowned per-service directories to service users
without propagating a missing user, so spx admin init reported success
while leaving /etc/spinifex/northstar root-owned 0600. ChownRecursive
and SetServiceOwnership now return errors, failures are aggregated
across every service rather than stopping at the first, and
finalizeNodeSetup prints the unapplied paths instead of a success mark.

cmd/spinifex/cmd/admin.go

@@ -2799,7 +2799,11 @@ func finalizeNodeSetup(dataDir, certPath, adminAccessKey, adminSecretKey, region
 	admin.CreateServiceDirectories(dataDir)
 
 	if os.Getuid() == 0 {
-		admin.SetServiceOwnership()
+		if err := admin.SetServiceOwnership(); err != nil {
+			fmt.Fprintf(os.Stderr, "Warning: service ownership not fully applied: %v\n", err)
+		} else {
+			fmt.Println("✅ Service ownership set")
+		}
 	}
 }
 

spinifex/admin/admin.go

@@ -7,6 +7,7 @@ import (
 	"crypto/x509/pkix"
 	"encoding/base64"
 	"encoding/pem"
+	"errors"
 	"fmt"
 	"io/fs"
 	"log/slog"
@@ -351,22 +352,20 @@ func FileExists(path string) bool {
 }
 
 // ChownRecursive changes ownership of path and its contents to username.
-// Best-effort: errors are logged but do not halt the operation.
-func ChownRecursive(path, username string) {
+// Failing to resolve the user is an error, since it leaves path untouched.
+// Per-entry walk failures below path stay best-effort and are only logged.
+func ChownRecursive(path, username string) error {
 	u, err := user.Lookup(username)
 	if err != nil {
-		slog.Warn("ChownRecursive: user lookup failed, skipping", "user", username, "path", path, "err", err)
-		return
+		return fmt.Errorf("chown %s: user %q not found: %w", path, username, err)
 	}
 	uid, err := strconv.Atoi(u.Uid)
 	if err != nil {
-		slog.Warn("ChownRecursive: invalid UID, skipping", "user", username, "uid", u.Uid, "err", err)
-		return
+		return fmt.Errorf("chown %s: invalid UID %q for user %q: %w", path, u.Uid, username, err)
 	}
 	gid, err := strconv.Atoi(u.Gid)
 	if err != nil {
-		slog.Warn("ChownRecursive: invalid GID, skipping", "user", username, "gid", u.Gid, "err", err)
-		return
+		return fmt.Errorf("chown %s: invalid GID %q for user %q: %w", path, u.Gid, username, err)
 	}
 
 	// Use os.Root to scope filesystem operations and avoid symlink TOCTOU races.
@@ -380,7 +379,7 @@ func ChownRecursive(path, username string) {
 		if chownErr := os.Lchown(path, uid, gid); chownErr != nil {
 			slog.Warn("chown failed", "path", path, "err", chownErr)
 		}
-		return
+		return nil
 	}
 	defer root.Close()
 
@@ -395,43 +394,61 @@ func ChownRecursive(path, username string) {
 		}
 		return nil
 	})
+	return nil
+}
+
+// servicePaths maps each per-service data/config directory to the system user
+// that must own it. Kept separate from SetServiceOwnership so the aggregation
+// logic can be tested against a small map rather than these absolute paths.
+var servicePaths = map[string]string{
+	"/etc/spinifex/nats":           "spinifex-nats",
+	"/var/lib/spinifex/nats":       "spinifex-nats",
+	"/etc/spinifex/predastore":     "spinifex-storage",
+	"/var/lib/spinifex/predastore": "spinifex-storage",
+	"/etc/spinifex/northstar":      "spinifex-northstar",
+	"/var/lib/spinifex/northstar":  "spinifex-northstar",
+	"/etc/spinifex/viperblock":     "spinifex-viperblock",
+	"/var/lib/spinifex/spinifex":   "spinifex-daemon",
+	"/var/lib/spinifex/viperblock": "spinifex-viperblock",
+	"/var/lib/spinifex/vpcd":       "spinifex-vpcd",
+	"/var/lib/spinifex/awsgw":      "spinifex-gw",
+}
+
+// chownServicePaths applies ChownRecursive to each path in services that
+// exists on disk, collecting every failure instead of stopping at the
+// first so one missing service user doesn't mask problems with the rest.
+func chownServicePaths(services map[string]string) error {
+	var errs []error
+	for path, u := range services {
+		if _, err := os.Stat(path); err != nil {
+			continue
+		}
+		if err := ChownRecursive(path, u); err != nil {
+			errs = append(errs, err)
+		}
+	}
+	return errors.Join(errs...)
 }
 
 // SetServiceOwnership sets per-service ownership on data/config directories
-// and shared config files to root:spinifex with correct modes.
-func SetServiceOwnership() {
+// and shared config files to root:spinifex with correct modes. Returns an error
+// naming every service path whose ownership could not be applied.
+func SetServiceOwnership() error {
 	grp, err := user.LookupGroup("spinifex")
 	if err != nil {
 		slog.Error("SetServiceOwnership: spinifex group not found, skipping all ownership changes", "err", err)
 		fmt.Fprintln(os.Stderr, "WARNING: spinifex group not found — service ownership not set. Run setup.sh first or create the group manually.")
-		return
+		return fmt.Errorf("spinifex group not found: %w", err)
 	}
 	gid, err := strconv.Atoi(grp.Gid)
 	if err != nil {
 		slog.Error("SetServiceOwnership: invalid spinifex group GID", "gid", grp.Gid, "err", err)
 		fmt.Fprintln(os.Stderr, "WARNING: invalid spinifex group GID — service ownership not set.")
-		return
+		return fmt.Errorf("invalid spinifex group GID %q: %w", grp.Gid, err)
 	}
 
 	// Per-service directory trees
-	for path, u := range map[string]string{
-		"/etc/spinifex/nats":           "spinifex-nats",
-		"/var/lib/spinifex/nats":       "spinifex-nats",
-		"/etc/spinifex/predastore":     "spinifex-storage",
-		"/var/lib/spinifex/predastore": "spinifex-storage",
-		"/etc/spinifex/northstar":      "spinifex-northstar",
-		"/var/lib/spinifex/northstar":  "spinifex-northstar",
-		"/etc/spinifex/viperblock":     "spinifex-viperblock",
-		"/var/lib/spinifex/spinifex":   "spinifex-daemon",
-		"/var/lib/spinifex/viperblock": "spinifex-viperblock",
-		"/var/lib/spinifex/vpcd":       "spinifex-vpcd",
-		"/var/lib/spinifex/awsgw":      "spinifex-gw",
-	} {
-		if _, err := os.Stat(path); err != nil {
-			continue
-		}
-		ChownRecursive(path, u)
-	}
+	ownershipErr := chownServicePaths(servicePaths)
 
 	// Shared data directories — root:spinifex 0770 so daemon + admin CLI can write
 	for _, dir := range []string{
@@ -474,6 +491,8 @@ func SetServiceOwnership() {
 			slog.Warn("SetServiceOwnership: chmod failed", "path", path, "err", err)
 		}
 	}
+
+	return ownershipErr
 }
 
 // UpdateAWSINIFile updates or creates an AWS INI file section with the given key-value pairs.
@@ -915,9 +934,13 @@ func SetupAWSCredentials(accessKey, secretKey, region, certPath, bindIP string)
 		return err
 	}
 
-	// Fix ownership so the sudo invoking user can read the files
+	// Fix ownership so the sudo invoking user can read the files. This is
+	// cosmetic convenience, not a service-critical permission, so a failure
+	// here is logged and never propagated to fail credential setup.
 	if os.Getuid() == 0 && sudoUser != "" {
-		ChownRecursive(awsDir, sudoUser)
+		if err := ChownRecursive(awsDir, sudoUser); err != nil {
+			slog.Warn("failed to fix AWS config ownership for sudo user", "user", sudoUser, "path", awsDir, "err", err)
+		}
 	}
 
 	fmt.Printf("   Profile: %s\n", profileName)

spinifex/admin/admin_test.go

@@ -990,17 +990,20 @@ secretkey = "{{.SecretKey}}"
 }
 
 func TestChownRecursive_InvalidUser(t *testing.T) {
-	// ChownRecursive should silently return on invalid username
+	// ChownRecursive must report a non-nil error naming the user when the
+	// user cannot be resolved, rather than silently doing nothing.
 	tmpDir := t.TempDir()
 	testFile := filepath.Join(tmpDir, "test.txt")
 	os.WriteFile(testFile, []byte("test"), 0644)
 
-	// Should not panic or error with a non-existent user
-	ChownRecursive(tmpDir, "nonexistent-user-that-does-not-exist-12345")
+	const badUser = "nonexistent-user-that-does-not-exist-12345"
+	err := ChownRecursive(tmpDir, badUser)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), badUser)
 
 	// File should still exist and be readable
-	_, err := os.ReadFile(testFile)
-	assert.NoError(t, err)
+	_, readErr := os.ReadFile(testFile)
+	assert.NoError(t, readErr)
 }
 
 func TestChownRecursive_CurrentUser(t *testing.T) {
@@ -1016,7 +1019,7 @@ func TestChownRecursive_CurrentUser(t *testing.T) {
 		t.Skip("USER env not set")
 	}
 
-	ChownRecursive(tmpDir, currentUser)
+	assert.NoError(t, ChownRecursive(tmpDir, currentUser))
 
 	// Verify files are still accessible
 	_, err := os.ReadFile(testFile)
@@ -1024,12 +1027,48 @@ func TestChownRecursive_CurrentUser(t *testing.T) {
 }
 
 func TestChownRecursive_NonExistentPath(t *testing.T) {
-	// Should not panic on a path that doesn't exist
+	// Should not panic on a path that doesn't exist; OpenRoot fails and the
+	// fallback Lchown fails too, but both are logged, not returned — the
+	// user itself resolved fine, so this is not one of the abort cases.
 	currentUser := os.Getenv("USER")
 	if currentUser == "" {
 		t.Skip("USER env not set")
 	}
-	ChownRecursive("/tmp/nonexistent-path-12345", currentUser)
+	assert.NoError(t, ChownRecursive("/tmp/nonexistent-path-12345", currentUser))
+}
+
+func TestChownServicePaths_MissingUser(t *testing.T) {
+	// chownServicePaths is the map-walking core extracted from
+	// SetServiceOwnership so it can be exercised against a small map
+	// instead of the hardcoded /etc/spinifex and /var/lib/spinifex paths,
+	// which are not writable/creatable without root in CI.
+	tmpDir := t.TempDir()
+	require.NoError(t, os.MkdirAll(tmpDir, 0755))
+
+	const badUser = "nonexistent-user-that-does-not-exist-12345"
+	err := chownServicePaths(map[string]string{tmpDir: badUser})
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), badUser)
+	assert.Contains(t, err.Error(), tmpDir)
+}
+
+func TestChownServicePaths_SkipsMissingPath(t *testing.T) {
+	// A path that doesn't exist on disk is never attempted, so it can't
+	// produce a "user not found" failure even for a bogus user.
+	err := chownServicePaths(map[string]string{
+		"/nonexistent-path-12345": "nonexistent-user-that-does-not-exist-12345",
+	})
+	assert.NoError(t, err)
+}
+
+func TestSetServiceOwnership_RequiresRoot(t *testing.T) {
+	// SetServiceOwnership operates on hardcoded /etc/spinifex and
+	// /var/lib/spinifex paths and chowns to the spinifex group, both of
+	// which require root. The per-service failure-aggregation logic itself
+	// is covered without root via TestChownServicePaths_MissingUser above.
+	if os.Geteuid() != 0 {
+		t.Skip("SetServiceOwnership requires root")
+	}
 }
 
 // --- SetGPUPassthrough ---

spinifex/handlers/ec2/image/service_impl.go

@@ -135,6 +135,11 @@ func (s *ImageServiceImpl) DescribeImages(ctx context.Context, input *ec2.Descri
 	var images []*ec2.Image
 	encryptedAtRest := s.clusterEncryptionEnabled()
 
+	// Counting prefixes seen against those dropped for an unreadable config.json
+	// makes a mismatch with len(images) visible at INFO, so an empty result is
+	// distinguishable from one where every config failed to load.
+	var prefixesExamined, unreadableConfigs int
+
 	// Iterate over CommonPrefixes to find ami-* directories
 	for _, prefix := range result.CommonPrefixes {
 		if prefix.Prefix == nil {
@@ -148,6 +153,7 @@ func (s *ImageServiceImpl) DescribeImages(ctx context.Context, input *ec2.Descri
 		if !strings.HasPrefix(prefixStr, "ami-") {
 			continue
 		}
+		prefixesExamined++
 
 		// Early skip: if image-id filter is set, check the prefix (ami-xxx/)
 		// against filter values before fetching config from S3.
@@ -168,19 +174,25 @@ func (s *ImageServiceImpl) DescribeImages(ctx context.Context, input *ec2.Descri
 		})
 		if err != nil {
 			if objectstore.IsNoSuchKeyError(err) {
+				// No config.json means this is a legitimate non-AMI directory,
+				// not a fault, so it stays at DEBUG and isn't counted below.
 				slog.DebugContext(ctx, "Config file not found", "key", configKey)
 			} else {
-				slog.DebugContext(ctx, "Failed to get config file", "key", configKey, "err", err)
+				unreadableConfigs++
+				slog.WarnContext(ctx, "Failed to get config file", "key", configKey, "err", err)
 			}
 			continue
 		}
 
 		body, err := io.ReadAll(getResult.Body)
-		if err := getResult.Body.Close(); err != nil {
-			slog.DebugContext(ctx, "Failed to close config body", "key", configKey, "err", err)
+		// A close failure alone doesn't drop the prefix (read below may still
+		// succeed), so it's only warned about, not counted as unreadable.
+		if closeErr := getResult.Body.Close(); closeErr != nil {
+			slog.WarnContext(ctx, "Failed to close config body", "key", configKey, "err", closeErr)
 		}
 		if err != nil {
-			slog.DebugContext(ctx, "Failed to read config body", "key", configKey, "err", err)
+			unreadableConfigs++
+			slog.WarnContext(ctx, "Failed to read config body", "key", configKey, "err", err)
 			continue
 		}
 
@@ -191,7 +203,8 @@ func (s *ImageServiceImpl) DescribeImages(ctx context.Context, input *ec2.Descri
 			VolumeConfig viperblock.VolumeConfig `json:"VolumeConfig"`
 		}
 		if err := json.Unmarshal(viperblock.StateBody(body), &vbConfig); err != nil {
-			slog.DebugContext(ctx, "Failed to unmarshal config", "key", configKey, "err", err)
+			unreadableConfigs++
+			slog.WarnContext(ctx, "Failed to unmarshal config", "key", configKey, "err", err)
 			continue
 		}
 
@@ -315,7 +328,8 @@ func (s *ImageServiceImpl) DescribeImages(ctx context.Context, input *ec2.Descri
 		}
 	}
 
-	slog.InfoContext(ctx, "DescribeImages completed", "count", len(images))
+	slog.InfoContext(ctx, "DescribeImages completed", "count", len(images),
+		"prefixesExamined", prefixesExamined, "unreadableConfigs", unreadableConfigs)
 
 	return &ec2.DescribeImagesOutput{
 		Images: images,

spinifex/handlers/ec2/image/service_impl_test.go

@@ -3,7 +3,9 @@ package handlers_ec2_image
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"io"
+	"log/slog"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -735,6 +737,90 @@ func TestDescribeImages_InvalidConfigJSON(t *testing.T) {
 	assert.Empty(t, result.Images)
 }
 
+// TestDescribeImages_ConfigFaultCounting covers mulga-7r6pm: a config.json that
+// exists but can't be fetched/read/parsed must be promoted to WARN and counted
+// as unreadable so it stays visible at INFO, while a genuinely absent
+// config.json (a non-AMI directory) stays silent at DEBUG and uncounted, and a
+// healthy prefix is unaffected either way.
+func TestDescribeImages_ConfigFaultCounting(t *testing.T) {
+	tests := []struct {
+		name           string
+		setup          func(t *testing.T, store *objectstore.MemoryObjectStore)
+		wantImageCount int
+		wantUnreadable int
+		wantWarnLogged bool
+	}{
+		{
+			name: "malformed config.json is counted as unreadable and does not vanish silently",
+			setup: func(t *testing.T, store *objectstore.MemoryObjectStore) {
+				_, err := store.PutObject(context.Background(), &awss3.PutObjectInput{
+					Bucket: aws.String(testBucket),
+					Key:    aws.String("ami-malformed1/config.json"),
+					Body:   strings.NewReader("{not valid json"),
+				})
+				require.NoError(t, err)
+			},
+			wantImageCount: 0,
+			wantUnreadable: 1,
+			wantWarnLogged: true,
+		},
+		{
+			name: "missing config.json (NoSuchKey) is not counted as a fault",
+			setup: func(t *testing.T, store *objectstore.MemoryObjectStore) {
+				// A placeholder object under the prefix makes ListObjectsV2 surface
+				// "ami-nokey1/" as a CommonPrefix with no config.json underneath.
+				_, err := store.PutObject(context.Background(), &awss3.PutObjectInput{
+					Bucket: aws.String(testBucket),
+					Key:    aws.String("ami-nokey1/placeholder.txt"),
+					Body:   strings.NewReader("x"),
+				})
+				require.NoError(t, err)
+			},
+			wantImageCount: 0,
+			wantUnreadable: 0,
+			wantWarnLogged: false,
+		},
+		{
+			name: "healthy prefix still yields its image",
+			setup: func(t *testing.T, store *objectstore.MemoryObjectStore) {
+				createTestAMIConfigWithOwner(t, store, "ami-healthy1", "healthy-ami", testAccountID)
+			},
+			wantImageCount: 1,
+			wantUnreadable: 0,
+			wantWarnLogged: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var buf strings.Builder
+			prev := slog.Default()
+			slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelInfo})))
+			t.Cleanup(func() { slog.SetDefault(prev) })
+
+			svc, store := setupTestImageService(t)
+			tt.setup(t, store)
+
+			out, err := svc.DescribeImages(context.Background(), &ec2.DescribeImagesInput{}, testAccountID)
+			require.NoError(t, err)
+			require.NotNil(t, out)
+			assert.Len(t, out.Images, tt.wantImageCount)
+
+			logs := buf.String()
+			assert.Contains(t, logs, "prefixesExamined=1", "the single ami-* prefix must be counted as examined")
+			assert.Contains(t, logs, fmt.Sprintf("unreadableConfigs=%d", tt.wantUnreadable))
+
+			if tt.wantWarnLogged {
+				assert.Contains(t, logs, "level=WARN",
+					"a config fetch/read/parse fault must be visible at WARN without raising the daemon's default level")
+			} else {
+				assert.NotContains(t, logs, "level=WARN",
+					"a legitimate skip (missing config or healthy prefix) must not be logged as a fault")
+			}
+		})
+	}
+}
+
 func TestDescribeImages_EmptyImageIDSkipped(t *testing.T) {
 	svc, store := setupTestImageService(t)