fix: resolve race conditions and cache inconsistencies from post-optimization review

go.mod

@@ -6,6 +6,7 @@ require (
 	github.com/fatih/color v1.18.0
 	github.com/olekukonko/tablewriter v1.1.3
 	github.com/spf13/cobra v1.10.2
+	github.com/stretchr/testify v1.10.0
 	go.uber.org/zap v1.27.1
 	gopkg.in/yaml.v3 v3.0.1
 )
@@ -14,6 +15,7 @@ require (
 	github.com/clipperhouse/displaywidth v0.7.0 // indirect
 	github.com/clipperhouse/stringish v0.1.1 // indirect
 	github.com/clipperhouse/uax29/v2 v2.3.1 // indirect
+	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/inconshreveable/mousetrap v1.1.0 // indirect
 	github.com/mattn/go-colorable v0.1.14 // indirect
 	github.com/mattn/go-isatty v0.0.20 // indirect
@@ -22,8 +24,8 @@ require (
 	github.com/olekukonko/cat v0.0.0-20250911104152-50322a0618f6 // indirect
 	github.com/olekukonko/errors v1.2.0 // indirect
 	github.com/olekukonko/ll v0.1.4-0.20260115111900-9e59c2286df0 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/spf13/pflag v1.0.10 // indirect
-	github.com/stretchr/testify v1.10.0 // indirect
 	go.uber.org/multierr v1.11.0 // indirect
 	golang.org/x/sys v0.40.0 // indirect
 	gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f // indirect

internal/brew/unified_cache.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 	"sync"
 	"time"
 
@@ -102,13 +103,8 @@ func ResetCache() {
 
 // IsInstalled checks if a package is installed.
 func (c *UnifiedCache) IsInstalled(name string) bool {
-	c.mu.RLock()
-	needsRefresh := c.needsRefreshLocked()
-	c.mu.RUnlock()
-
-	if needsRefresh {
-		_ = c.Refresh(context.Background(), false)
-	}
+	// Refresh if needed (internally checks TTL and holds appropriate locks)
+	_ = c.Refresh(context.Background(), false)
 
 	c.mu.RLock()
 	defer c.mu.RUnlock()
@@ -174,6 +170,7 @@ func (c *UnifiedCache) Refresh(ctx context.Context, force bool) error {
 	// Step 1: Get installed packages (brew list)
 	installed, err := c.fetchInstalledPackages()
 	if err != nil {
+		// Keep existing cache on error - better stale than empty
 		return fmt.Errorf("failed to fetch installed packages: %w", err)
 	}
 
@@ -342,6 +339,7 @@ func (c *UnifiedCache) MarkUpgraded(name, newVersion string) error {
 	defer c.mu.Unlock()
 
 	state := c.Packages[name]
+	state.Installed = true
 	state.InstalledVersion = newVersion
 	state.LatestVersion = newVersion
 	state.Outdated = false
@@ -472,22 +470,32 @@ func (c *UnifiedCache) saveToDiskLocked() error {
 // Helper functions
 
 func splitLines(s string) []string {
+	if s == "" {
+		return []string{}
+	}
+
 	result := make([]string, 0)
-	current := ""
+	var line strings.Builder
+	line.Grow(64) // Pre-allocate reasonable line size
+
 	for _, r := range s {
 		if r == '\n' {
-			trimmed := trimSpace(current)
+			trimmed := trimSpace(line.String())
 			if trimmed != "" {
 				result = append(result, trimmed)
 			}
-			current = ""
+			line.Reset()
 		} else {
-			current += string(r)
+			line.WriteRune(r)
 		}
 	}
-	trimmed := trimSpace(current)
-	if trimmed != "" {
-		result = append(result, trimmed)
+
+	// Handle last line if no trailing newline
+	if line.Len() > 0 {
+		trimmed := trimSpace(line.String())
+		if trimmed != "" {
+			result = append(result, trimmed)
+		}
 	}
 
 	return result

internal/brew/unified_cache_test.go

@@ -0,0 +1,237 @@
+package brew
+
+import (
+	"context"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/MrSnakeDoc/keg/internal/runner"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// testRunner implements runner.CommandRunner for testing
+type testRunner struct {
+	listOutput     string
+	listErr        error
+	outdatedOutput string
+	outdatedErr    error
+	infoOutput     string
+	infoErr        error
+}
+
+func (m *testRunner) Run(ctx context.Context, timeout time.Duration, mode runner.Mode, cmd string, args ...string) ([]byte, error) {
+	if cmd == "brew" && len(args) > 0 {
+		switch args[0] {
+		case "list":
+			return []byte(m.listOutput), m.listErr
+		case "outdated":
+			return []byte(m.outdatedOutput), m.outdatedErr
+		case "info":
+			return []byte(m.infoOutput), m.infoErr
+		}
+	}
+	return []byte{}, nil
+}
+
+func TestUnifiedCache_IsInstalled(t *testing.T) {
+	r := &testRunner{
+		listOutput: "wget\ncurl\njq\n",
+	}
+
+	cache := &UnifiedCache{
+		Packages: map[string]PackageState{
+			"wget": {Installed: true, FetchedAt: time.Now()},
+			"curl": {Installed: true, FetchedAt: time.Now()},
+		},
+		LastUpdated: time.Now(),
+		TTL:         1 * time.Hour,
+		runner:      r,
+	}
+
+	assert.True(t, cache.IsInstalled("wget"))
+	assert.True(t, cache.IsInstalled("curl"))
+	assert.False(t, cache.IsInstalled("nonexistent"))
+}
+
+func TestUnifiedCache_GetInstalledSet(t *testing.T) {
+	cache := &UnifiedCache{
+		Packages: map[string]PackageState{
+			"wget":   {Installed: true, FetchedAt: time.Now()},
+			"curl":   {Installed: true, FetchedAt: time.Now()},
+			"notins": {Installed: false, FetchedAt: time.Now()},
+		},
+	}
+
+	installed := cache.GetInstalledSet()
+	assert.Len(t, installed, 2)
+	assert.Contains(t, installed, "wget")
+	assert.Contains(t, installed, "curl")
+	assert.NotContains(t, installed, "notins")
+}
+
+func TestUnifiedCache_GetOutdatedMap(t *testing.T) {
+	cache := &UnifiedCache{
+		Packages: map[string]PackageState{
+			"wget": {
+				Installed:        true,
+				Outdated:         true,
+				InstalledVersion: "1.0.0",
+				LatestVersion:    "1.1.0",
+				FetchedAt:        time.Now(),
+			},
+			"curl": {
+				Installed: true,
+				Outdated:  false,
+				FetchedAt: time.Now(),
+			},
+			"jq": {
+				Installed:        false,
+				Outdated:         true,
+				InstalledVersion: "1.5.0",
+				LatestVersion:    "1.6.0",
+				FetchedAt:        time.Now(),
+			},
+		},
+	}
+
+	outdated := cache.GetOutdatedMap()
+	assert.Len(t, outdated, 1, "only installed+outdated packages should be returned")
+	assert.Contains(t, outdated, "wget")
+	assert.Equal(t, "1.0.0", outdated["wget"].InstalledVersion)
+	assert.Equal(t, "1.1.0", outdated["wget"].LatestVersion)
+}
+
+func TestUnifiedCache_MarkInstalled(t *testing.T) {
+	cache := &UnifiedCache{
+		Packages:  make(map[string]PackageState),
+		cachePath: filepath.Join(t.TempDir(), "cache.json"),
+	}
+
+	err := cache.MarkInstalled("wget", "1.0.0")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	assert.True(t, cache.Packages["wget"].Installed)
+	assert.Equal(t, "1.0.0", cache.Packages["wget"].InstalledVersion)
+	assert.Equal(t, "1.0.0", cache.Packages["wget"].LatestVersion)
+	assert.False(t, cache.Packages["wget"].Outdated)
+}
+
+func TestUnifiedCache_MarkUpgraded(t *testing.T) {
+	cache := &UnifiedCache{
+		Packages: map[string]PackageState{
+			"wget": {
+				Installed:        true,
+				Outdated:         true,
+				InstalledVersion: "1.0.0",
+				LatestVersion:    "1.1.0",
+			},
+		},
+		cachePath: filepath.Join(t.TempDir(), "cache.json"),
+	}
+
+	err := cache.MarkUpgraded("wget", "1.1.0")
+	require.NoError(t, err)
+
+	assert.True(t, cache.Packages["wget"].Installed)
+	assert.Equal(t, "1.1.0", cache.Packages["wget"].InstalledVersion)
+	assert.Equal(t, "1.1.0", cache.Packages["wget"].LatestVersion)
+	assert.False(t, cache.Packages["wget"].Outdated)
+}
+
+func TestUnifiedCache_MarkUpgraded_SetsInstalledFlag(t *testing.T) {
+	cache := &UnifiedCache{
+		Packages:  make(map[string]PackageState),
+		cachePath: filepath.Join(t.TempDir(), "cache.json"),
+	}
+
+	// Upgrade a package that doesn't exist in cache yet
+	err := cache.MarkUpgraded("newpkg", "2.0.0")
+	require.NoError(t, err)
+
+	// Should be marked as installed
+	assert.True(t, cache.Packages["newpkg"].Installed, "upgraded package should be marked as installed")
+	assert.Equal(t, "2.0.0", cache.Packages["newpkg"].InstalledVersion)
+}
+
+func TestUnifiedCache_MarkUninstalled(t *testing.T) {
+	cache := &UnifiedCache{
+		Packages: map[string]PackageState{
+			"wget": {
+				Installed:        true,
+				InstalledVersion: "1.0.0",
+			},
+			"curl": {
+				Installed: true,
+			},
+		},
+		cachePath: filepath.Join(t.TempDir(), "cache.json"),
+	}
+
+	err := cache.MarkUninstalled("wget")
+	require.NoError(t, err)
+
+	// Package should be completely removed from cache
+	assert.NotContains(t, cache.Packages, "wget", "uninstalled package should be deleted from cache")
+	// Other packages should remain
+	assert.Contains(t, cache.Packages, "curl")
+}
+
+func TestUnifiedCache_RefreshVersions_EmptyList(t *testing.T) {
+	cache := &UnifiedCache{
+		Packages: make(map[string]PackageState),
+		runner:   &testRunner{},
+	}
+
+	err := cache.RefreshVersions(context.Background(), []string{})
+	require.NoError(t, err)
+}
+
+func TestSplitLines_HandlesVariousInputs(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected []string
+	}{
+		{
+			name:     "empty string",
+			input:    "",
+			expected: []string{},
+		},
+		{
+			name:     "single line",
+			input:    "wget",
+			expected: []string{"wget"},
+		},
+		{
+			name:     "multiple lines",
+			input:    "wget\ncurl\njq",
+			expected: []string{"wget", "curl", "jq"},
+		},
+		{
+			name:     "lines with spaces",
+			input:    "wget\n  curl  \njq",
+			expected: []string{"wget", "curl", "jq"},
+		},
+		{
+			name:     "empty lines",
+			input:    "wget\n\ncurl\n\n",
+			expected: []string{"wget", "curl"},
+		},
+		{
+			name:     "windows line endings",
+			input:    "wget\r\ncurl\r\njq",
+			expected: []string{"wget", "curl", "jq"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := splitLines(tt.input)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}

internal/core/core.go

@@ -103,9 +103,7 @@ func NewBase(config *models.Config, r runner.CommandRunner) *Base {
 
 	cache, err := brew.GetCache(r)
 	if err != nil {
-		logger.Debug("failed to initialize unified cache: %v", err)
-		// Fallback to empty cache
-		cache = &brew.UnifiedCache{}
+		logger.Fatal("failed to initialize unified cache: %v", err)
 	}
 
 	return &Base{