fix: concurrent map writes in Manager.LookupSdk (#610)

* Initial plan

* Fix concurrent map writes in Manager with RWMutex

Co-authored-by: aooohan <40265686+aooohan@users.noreply.github.com>

* Add test simulating activate.go concurrent SDK lookups

Co-authored-by: aooohan <40265686+aooohan@users.noreply.github.com>

* Address code review feedback: fix cache key consistency and Close() locking

Co-authored-by: aooohan <40265686+aooohan@users.noreply.github.com>

* Add comments documenting zero-value mutex initialization

Co-authored-by: aooohan <40265686+aooohan@users.noreply.github.com>

* Improve consistency: use normalizedName variable in LoadAllSdk

Co-authored-by: aooohan <40265686+aooohan@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: aooohan <40265686+aooohan@users.noreply.github.com>

Author: Copilot

internal/manager.go

@@ -29,6 +29,7 @@ import (
 	"sort"
 	"strconv"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/pterm/pterm"
@@ -67,20 +68,29 @@ type Arg struct {
 type Manager struct {
 	RuntimeEnvContext *env.RuntimeEnvContext // runtime environment context
 	openSdks          map[string]sdk.Sdk
+	mu                sync.RWMutex // protects openSdks map
 }
 
 // LookupSdk lookup sdk by name
 // Loads SDK plugins and generates env keys based on the tool versions in the chain
 func (m *Manager) LookupSdk(name string) (sdk.Sdk, error) {
 	logger.Debugf("Looking up SDK: %s\n", name)
 
-	if s, ok := m.openSdks[name]; ok {
+	// Normalize the name for cache lookup
+	normalizedName := strings.ToLower(name)
+	
+	// Check cache with read lock
+	m.mu.RLock()
+	s, ok := m.openSdks[normalizedName]
+	m.mu.RUnlock()
+	
+	if ok {
 		logger.Debugf("SDK %s found in cache\n", name)
 		return s, nil
 	}
 
 	// Query plugin directly from shared root
-	pluginPath := filepath.Join(m.RuntimeEnvContext.PathMeta.Shared.Plugins, strings.ToLower(name))
+	pluginPath := filepath.Join(m.RuntimeEnvContext.PathMeta.Shared.Plugins, normalizedName)
 	logger.Debugf("Checking plugin path: %s\n", pluginPath)
 
 	if !util.FileExists(pluginPath) {
@@ -95,7 +105,11 @@ func (m *Manager) LookupSdk(name string) (sdk.Sdk, error) {
 		return nil, err
 	}
 
-	m.openSdks[strings.ToLower(name)] = s
+	// Add to cache with write lock
+	m.mu.Lock()
+	m.openSdks[normalizedName] = s
+	m.mu.Unlock()
+	
 	logger.Debugf("SDK %s loaded and cached successfully\n", name)
 	return s, nil
 }
@@ -182,14 +196,20 @@ func (m *Manager) LoadAllSdk() ([]sdk.Sdk, error) {
 
 	for _, f := range files {
 		sdkName := f.Name()
+		normalizedName := strings.ToLower(sdkName)
 		path := filepath.Join(dir, sdkName)
 
 		if f.IsDir() {
 			logger.Debugf("Loading SDK: %s from %s\n", sdkName, path)
 			s, err := sdk.NewSdk(m.RuntimeEnvContext, path)
 			if err == nil {
 				sdkSlice = append(sdkSlice, s)
-				m.openSdks[strings.ToLower(sdkName)] = s
+				
+				// Add to cache with write lock
+				m.mu.Lock()
+				m.openSdks[normalizedName] = s
+				m.mu.Unlock()
+				
 				logger.Debugf("SDK %s loaded successfully\n", sdkName)
 			} else {
 				logger.Debugf("Failed to load SDK %s: %v\n", sdkName, err)
@@ -209,6 +229,11 @@ func (m *Manager) LoadAllSdk() ([]sdk.Sdk, error) {
 }
 
 func (m *Manager) Close() {
+	// Use write lock to ensure no other operations are in progress
+	// while we're closing SDK handlers
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	
 	for _, handler := range m.openSdks {
 		handler.Close()
 	}
@@ -796,6 +821,7 @@ func NewSdkManager() (*Manager, error) {
 			RuntimeVersion:    RuntimeVersion,
 		},
 		openSdks: make(map[string]sdk.Sdk),
+		// mu is intentionally zero-initialized (Go's zero-value mutex is ready to use)
 	}, nil
 }
 

internal/manager_test.go

@@ -0,0 +1,285 @@
+/*
+ *    Copyright 2026 Han Li and contributors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+package internal
+
+import (
+	"os"
+	"path/filepath"
+	"sync"
+	"testing"
+
+	"github.com/version-fox/vfox/internal/config"
+	"github.com/version-fox/vfox/internal/env"
+	"github.com/version-fox/vfox/internal/pathmeta"
+	"github.com/version-fox/vfox/internal/sdk"
+)
+
+// Note: In these tests, Manager instances are created without explicitly
+// initializing the mu field. This is intentional and valid in Go, as the
+// zero-value of sync.RWMutex is ready to use.
+
+// TestLookupSdk_ConcurrentAccess tests that concurrent calls to LookupSdk
+// do not cause race conditions or panics
+func TestLookupSdk_ConcurrentAccess(t *testing.T) {
+	// Create a temporary directory structure for testing
+	tmpDir := t.TempDir()
+	userHome := filepath.Join(tmpDir, "user_home")
+	vfoxHome := filepath.Join(tmpDir, "vfox_home")
+	currentDir := tmpDir
+
+	// Create necessary directories
+	os.MkdirAll(userHome, 0755)
+	os.MkdirAll(vfoxHome, 0755)
+
+	// Create PathMeta
+	meta, err := pathmeta.NewPathMeta(userHome, vfoxHome, currentDir, 12345)
+	if err != nil {
+		t.Fatalf("Failed to create PathMeta: %v", err)
+	}
+
+	// Create plugins directory
+	pluginsDir := meta.Shared.Plugins
+	os.MkdirAll(pluginsDir, 0755)
+
+	// Create Manager
+	manager := &Manager{
+		RuntimeEnvContext: &env.RuntimeEnvContext{
+			UserConfig:        &config.Config{},
+			CurrentWorkingDir: currentDir,
+			PathMeta:          meta,
+			RuntimeVersion:    "test",
+		},
+		openSdks: make(map[string]sdk.Sdk),
+	}
+
+	// Test concurrent access with non-existent SDK
+	// This should not cause race conditions or panics
+	const numGoroutines = 10
+	var wg sync.WaitGroup
+	wg.Add(numGoroutines)
+
+	for i := 0; i < numGoroutines; i++ {
+		go func(id int) {
+			defer wg.Done()
+			// Try to lookup a non-existent SDK
+			_, err := manager.LookupSdk("nonexistent")
+			// We expect an error since the SDK doesn't exist
+			if err == nil {
+				t.Errorf("Expected error for non-existent SDK, got nil")
+			}
+		}(i)
+	}
+
+	wg.Wait()
+}
+
+// TestLookupSdk_ConcurrentCacheHits tests that concurrent cache hits
+// do not cause race conditions
+func TestLookupSdk_ConcurrentCacheHits(t *testing.T) {
+	// Create a temporary directory structure for testing
+	tmpDir := t.TempDir()
+	userHome := filepath.Join(tmpDir, "user_home")
+	vfoxHome := filepath.Join(tmpDir, "vfox_home")
+	currentDir := tmpDir
+
+	// Create necessary directories
+	os.MkdirAll(userHome, 0755)
+	os.MkdirAll(vfoxHome, 0755)
+
+	// Create PathMeta
+	meta, err := pathmeta.NewPathMeta(userHome, vfoxHome, currentDir, 12345)
+	if err != nil {
+		t.Fatalf("Failed to create PathMeta: %v", err)
+	}
+
+	// Create Manager
+	manager := &Manager{
+		RuntimeEnvContext: &env.RuntimeEnvContext{
+			UserConfig:        &config.Config{},
+			CurrentWorkingDir: currentDir,
+			PathMeta:          meta,
+			RuntimeVersion:    "test",
+		},
+		openSdks: make(map[string]sdk.Sdk),
+	}
+
+	// Pre-populate the cache with a nil SDK (for testing purposes)
+	// In real usage, this would be a valid SDK object
+	// Use proper locking to avoid race conditions in the test itself
+	manager.mu.Lock()
+	manager.openSdks["test"] = nil
+	manager.mu.Unlock()
+
+	// Test concurrent cache reads
+	const numGoroutines = 20
+	var wg sync.WaitGroup
+	wg.Add(numGoroutines)
+
+	for i := 0; i < numGoroutines; i++ {
+		go func(id int) {
+			defer wg.Done()
+			// Try to lookup the cached SDK
+			_, err := manager.LookupSdk("test")
+			if err != nil {
+				// Error is expected since we stored nil
+				// But we should not panic or race
+			}
+		}(i)
+	}
+
+	wg.Wait()
+}
+
+// TestLoadAllSdk_NoRaceCondition verifies that LoadAllSdk
+// can safely populate the cache
+func TestLoadAllSdk_NoRaceCondition(t *testing.T) {
+	// Create a temporary directory structure for testing
+	tmpDir := t.TempDir()
+	userHome := filepath.Join(tmpDir, "user_home")
+	vfoxHome := filepath.Join(tmpDir, "vfox_home")
+	currentDir := tmpDir
+
+	// Create necessary directories
+	os.MkdirAll(userHome, 0755)
+	os.MkdirAll(vfoxHome, 0755)
+
+	// Create PathMeta
+	meta, err := pathmeta.NewPathMeta(userHome, vfoxHome, currentDir, 12345)
+	if err != nil {
+		t.Fatalf("Failed to create PathMeta: %v", err)
+	}
+
+	// Create plugins directory
+	pluginsDir := meta.Shared.Plugins
+	os.MkdirAll(pluginsDir, 0755)
+
+	// Create Manager
+	manager := &Manager{
+		RuntimeEnvContext: &env.RuntimeEnvContext{
+			UserConfig:        &config.Config{},
+			CurrentWorkingDir: currentDir,
+			PathMeta:          meta,
+			RuntimeVersion:    "test",
+		},
+		openSdks: make(map[string]sdk.Sdk),
+	}
+
+	// Call LoadAllSdk (will return empty list but shouldn't panic)
+	sdks, err := manager.LoadAllSdk()
+	if err != nil {
+		t.Fatalf("LoadAllSdk failed: %v", err)
+	}
+
+	// Should return empty list for empty plugins directory
+	if len(sdks) != 0 {
+		t.Errorf("Expected 0 SDKs, got %d", len(sdks))
+	}
+}
+
+// TestClose_NoRaceCondition verifies that Close
+// can safely iterate over the cache
+func TestClose_NoRaceCondition(t *testing.T) {
+	// Create a temporary directory structure for testing
+	tmpDir := t.TempDir()
+	userHome := filepath.Join(tmpDir, "user_home")
+	vfoxHome := filepath.Join(tmpDir, "vfox_home")
+	currentDir := tmpDir
+
+	// Create necessary directories
+	os.MkdirAll(userHome, 0755)
+	os.MkdirAll(vfoxHome, 0755)
+
+	// Create PathMeta
+	meta, err := pathmeta.NewPathMeta(userHome, vfoxHome, currentDir, 12345)
+	if err != nil {
+		t.Fatalf("Failed to create PathMeta: %v", err)
+	}
+
+	// Create Manager
+	manager := &Manager{
+		RuntimeEnvContext: &env.RuntimeEnvContext{
+			UserConfig:        &config.Config{},
+			CurrentWorkingDir: currentDir,
+			PathMeta:          meta,
+			RuntimeVersion:    "test",
+		},
+		openSdks: make(map[string]sdk.Sdk),
+	}
+
+	// Call Close (should not panic even with empty cache)
+	manager.Close()
+}
+
+// TestLookupSdk_SimulateActivateScenario simulates the exact scenario from activate.go
+// where multiple goroutines lookup different SDKs concurrently
+func TestLookupSdk_SimulateActivateScenario(t *testing.T) {
+	// Create a temporary directory structure for testing
+	tmpDir := t.TempDir()
+	userHome := filepath.Join(tmpDir, "user_home")
+	vfoxHome := filepath.Join(tmpDir, "vfox_home")
+	currentDir := tmpDir
+
+	// Create necessary directories
+	os.MkdirAll(userHome, 0755)
+	os.MkdirAll(vfoxHome, 0755)
+
+	// Create PathMeta
+	meta, err := pathmeta.NewPathMeta(userHome, vfoxHome, currentDir, 12345)
+	if err != nil {
+		t.Fatalf("Failed to create PathMeta: %v", err)
+	}
+
+	// Create plugins directory
+	pluginsDir := meta.Shared.Plugins
+	os.MkdirAll(pluginsDir, 0755)
+
+	// Create Manager
+	manager := &Manager{
+		RuntimeEnvContext: &env.RuntimeEnvContext{
+			UserConfig:        &config.Config{},
+			CurrentWorkingDir: currentDir,
+			PathMeta:          meta,
+			RuntimeVersion:    "test",
+		},
+		openSdks: make(map[string]sdk.Sdk),
+	}
+
+	// Simulate the activate.go scenario with multiple SDKs being looked up concurrently
+	// This is what was causing the concurrent map writes error
+	sdkNames := []string{"nodejs", "python", "java", "go", "ruby", "rust"}
+	
+	var wg sync.WaitGroup
+	wg.Add(len(sdkNames))
+
+	for _, sdkName := range sdkNames {
+		sdkName := sdkName // Capture loop variable
+		go func() {
+			defer wg.Done()
+			// Each goroutine tries to lookup a different SDK
+			// This simulates the errgroup in activate.go
+			_, err := manager.LookupSdk(sdkName)
+			// We expect errors since these SDKs don't exist
+			// But we should not panic or race
+			if err == nil {
+				t.Errorf("Expected error for non-existent SDK %s, got nil", sdkName)
+			}
+		}()
+	}
+
+	wg.Wait()
+}
+