fix(contexts): refresh registry against disk on each list call (SKY-834 bug 52)

internal/k8s/client.go

@@ -10,6 +10,7 @@ import (
 	"sort"
 	"strings"
 	"sync"
+	"time"
 
 	"k8s.io/client-go/discovery"
 	"k8s.io/client-go/dynamic"
@@ -43,7 +44,14 @@ var (
 	contextRegistry map[string]contextEntry
 	// perFileConfigs caches each file's parsed api.Config so GetAvailableContexts
 	// doesn't re-read N files on every call. Keyed by absolute file path.
-	perFileConfigs    map[string]*clientcmdapi.Config
+	perFileConfigs map[string]*clientcmdapi.Config
+	// perFileMtimes lets refreshContextRegistry detect rewritten or
+	// removed kubeconfig files between calls. Without this the
+	// registry is built once at startup and never refreshes, so
+	// destroyed clusters / removed contexts linger in the dropdown
+	// (they only error out when the user tries to switch to them).
+	// Same lifecycle / lock as perFileConfigs.
+	perFileMtimes     map[string]time.Time
 	contextName       string
 	clusterName       string
 	contextNamespace  string // Default namespace from kubeconfig context
@@ -692,19 +700,39 @@ func GetAvailableContexts() ([]ContextInfo, error) {
 		}, nil
 	}
 
-	clientMu.RLock()
-	registry := contextRegistry
-	fileConfigs := perFileConfigs
+	// Reconcile registry against disk before reading. This is the
+	// only refresh point in multi-file (isolated-load) mode — without
+	// it, kubeconfigs that were rewritten or deleted on disk after
+	// startup keep showing up in the dropdown until the user
+	// restarts Radar (the "junk clusters" complaint).
+	//
+	// CRITICAL: refresh, snapshot, AND iterate must all happen under
+	// the same lock. The previous shape released the lock and then
+	// iterated, but `registry` and `fileConfigs` still pointed at the
+	// LIVE maps that a second concurrent caller could be mutating
+	// via refreshContextRegistry — Go's "fatal error: concurrent map
+	// read and map write". Hold the write lock through the snapshot,
+	// then iterate over a private []ContextInfo we've already built.
+	clientMu.Lock()
+	if contextRegistry != nil {
+		// Lazy init: MergeAndSwitchContext promotes single-file mode
+		// to isolated-load mode without touching perFileMtimes, so a
+		// CAPI promotion can leave it nil. Seeding it here is safe
+		// because we always hold the write lock.
+		if perFileMtimes == nil {
+			perFileMtimes = make(map[string]time.Time, len(perFileConfigs))
+		}
+		refreshContextRegistry(contextRegistry, perFileConfigs, perFileMtimes)
+	}
 	currentCtx := contextName
-	clientMu.RUnlock()
 
-	if registry != nil {
+	if contextRegistry != nil {
 		// Isolated-load mode: enumerate every registered context, pulling
 		// cluster/user/namespace from the file it originally lives in.
 		// No merge happens — shared names across files stay distinct.
-		contexts := make([]ContextInfo, 0, len(registry))
-		for qName, entry := range registry {
-			cfg, ok := fileConfigs[entry.SourceFile]
+		contexts := make([]ContextInfo, 0, len(contextRegistry))
+		for qName, entry := range contextRegistry {
+			cfg, ok := perFileConfigs[entry.SourceFile]
 			if !ok {
 				continue
 			}
@@ -720,8 +748,10 @@ func GetAvailableContexts() ([]ContextInfo, error) {
 				IsCurrent: qName == currentCtx,
 			})
 		}
+		clientMu.Unlock()
 		return contexts, nil
 	}
+	clientMu.Unlock()
 
 	// Single-file fallback: load the one file and enumerate its contexts.
 	kubeconfig := kubeconfigPath
@@ -948,6 +978,7 @@ func MergeAndSwitchContext(kubeconfigData []byte, contextName string) (string, s
 	// remove the temp file and leave the globals untouched — no half-state.
 	var newRegistry map[string]contextEntry
 	var newFileConfigs map[string]*clientcmdapi.Config
+	var newFileMtimes map[string]time.Time
 	var newPaths []string
 
 	if contextRegistry == nil {
@@ -964,6 +995,18 @@ func MergeAndSwitchContext(kubeconfigData []byte, contextName string) (string, s
 		}
 		newRegistry = registry
 		newFileConfigs = fileConfigs
+		// Seed the mtime cache for the same set of files. Without
+		// this, the next refresh would write to a nil map
+		// (perFileMtimes is package-level and stays nil through the
+		// promotion). Refresh's nil-map guard would also catch this,
+		// but seeding here keeps the invariant "perFileMtimes is
+		// non-nil whenever contextRegistry is non-nil".
+		newFileMtimes = make(map[string]time.Time, len(seedPaths))
+		for _, p := range seedPaths {
+			if info, err := os.Stat(p); err == nil {
+				newFileMtimes[p] = info.ModTime()
+			}
+		}
 		newPaths = seedPaths
 	} else {
 		cfg, err := clientcmd.LoadFromFile(tmpPath)
@@ -982,6 +1025,13 @@ func MergeAndSwitchContext(kubeconfigData []byte, contextName string) (string, s
 			newFileConfigs[k] = v
 		}
 		newFileConfigs[tmpPath] = cfg
+		newFileMtimes = make(map[string]time.Time, len(perFileMtimes)+1)
+		for k, v := range perFileMtimes {
+			newFileMtimes[k] = v
+		}
+		if info, err := os.Stat(tmpPath); err == nil {
+			newFileMtimes[tmpPath] = info.ModTime()
+		}
 		newPaths = append(append([]string(nil), kubeconfigPaths...), tmpPath)
 		for name := range cfg.Contexts {
 			qName := qualifyContextName(newRegistry, name, tmpPath)
@@ -1001,6 +1051,7 @@ func MergeAndSwitchContext(kubeconfigData []byte, contextName string) (string, s
 	// Commit. All globals updated atomically under the single Lock held above.
 	contextRegistry = newRegistry
 	perFileConfigs = newFileConfigs
+	perFileMtimes = newFileMtimes
 	kubeconfigPaths = newPaths
 	capiKubeconfigs[contextName] = tmpPath
 

internal/k8s/context_registry.go

@@ -3,9 +3,11 @@ package k8s
 import (
 	"fmt"
 	"log"
+	"os"
 	"path/filepath"
 	"sort"
 	"strings"
+	"time"
 
 	"k8s.io/client-go/tools/clientcmd"
 	clientcmdapi "k8s.io/client-go/tools/clientcmd/api"
@@ -38,6 +40,12 @@ func setupIsolatedLoad(paths []string) (
 	}
 	contextRegistry = registry
 	perFileConfigs = fileConfigs
+	perFileMtimes = make(map[string]time.Time, len(paths))
+	for _, p := range paths {
+		if info, err := os.Stat(p); err == nil {
+			perFileMtimes[p] = info.ModTime()
+		}
+	}
 	contextName = qName
 	return &clientcmd.ClientConfigLoadingRules{ExplicitPath: entry.SourceFile},
 		&clientcmd.ConfigOverrides{CurrentContext: entry.InFileName},
@@ -166,6 +174,118 @@ func pickInitialContext(
 	return "", contextEntry{}, false
 }
 
+// refreshContextRegistry reconciles the in-memory contextRegistry +
+// perFileConfigs against what's actually on disk RIGHT NOW. Returns
+// whether anything changed (so the caller can decide whether to log
+// or invalidate downstream caches).
+//
+// The original registry is built ONCE in setupIsolatedLoad and was
+// never refreshed in multi-file mode. That left the cluster
+// dropdown showing entries for:
+//
+//   - kubeconfig files the user removed from a watched directory,
+//   - kubeconfig files rewritten by `kubectl config delete-context`,
+//   - kubeconfig files written then deleted by CAPI when a managed
+//     cluster was destroyed.
+//
+// All three look like "junk clusters" to the user (the entry is
+// there but selecting it errors out). This helper is the surgical
+// fix: same per-file isolation guarantees as buildContextRegistry,
+// just incremental — it ONLY touches files whose mtime moved or
+// whose path no longer exists on disk.
+//
+// Concurrency: caller MUST hold clientMu (write lock). The helper
+// rebuilds shared map values, so it's not safe to interleave with
+// GetAvailableContexts readers.
+func refreshContextRegistry(
+	registry map[string]contextEntry,
+	fileConfigs map[string]*clientcmdapi.Config,
+	fileMtimes map[string]time.Time,
+) bool {
+	if registry == nil {
+		return false
+	}
+	changed := false
+	// Group registry entries by source file so we can decide
+	// per-file: keep, re-parse, or drop everything pointing at it.
+	byFile := make(map[string][]string)
+	for qName, entry := range registry {
+		byFile[entry.SourceFile] = append(byFile[entry.SourceFile], qName)
+	}
+	for path, qNames := range byFile {
+		info, statErr := os.Stat(path)
+		if statErr != nil {
+			// File is gone (or unreadable). Drop every registry
+			// entry pointing at it AND its cached config. This
+			// is the CAPI-cluster-destroyed and
+			// "user removed file from kubeconfig dir" cases.
+			for _, qName := range qNames {
+				delete(registry, qName)
+			}
+			delete(fileConfigs, path)
+			delete(fileMtimes, path)
+			changed = true
+			continue
+		}
+		mtime := info.ModTime()
+		if cached, ok := fileMtimes[path]; ok && cached.Equal(mtime) {
+			// Unchanged — keep the cached parse + entries.
+			continue
+		}
+		// File is new to us OR has been rewritten on disk. Re-parse
+		// and rebuild ONLY this file's entries.
+		cfg, err := clientcmd.LoadFromFile(path)
+		if err != nil {
+			// Couldn't parse the rewritten file. Don't drop the
+			// existing entries — the user may be mid-edit, and
+			// silently pruning the dropdown while they save would
+			// be more confusing than a stale entry. Log and skip.
+			log.Printf("[k8s-init] refresh: skipping kubeconfig %q (parse failed): %v",
+				filepath.Base(path), err)
+			errorlog.Record("k8s-init", "warning",
+				"refresh: kubeconfig %q failed to load: %s",
+				filepath.Base(path), scrubPathError(err))
+			continue
+		}
+		fileConfigs[path] = cfg
+		fileMtimes[path] = mtime
+		// Replace this file's entries in the registry. Names that
+		// are no longer in the file get dropped; new ones are added.
+		liveNames := make(map[string]struct{}, len(cfg.Contexts))
+		for name := range cfg.Contexts {
+			liveNames[name] = struct{}{}
+		}
+		for _, qName := range qNames {
+			if _, alive := liveNames[registry[qName].InFileName]; !alive {
+				delete(registry, qName)
+				changed = true
+			}
+		}
+		// Add any contexts that are new in this file. We deliberately
+		// re-use qualifyContextName to keep the cross-file collision
+		// behaviour consistent with the initial build.
+		for name := range cfg.Contexts {
+			already := false
+			for _, qName := range qNames {
+				if e, ok := registry[qName]; ok && e.SourceFile == path && e.InFileName == name {
+					already = true
+					break
+				}
+			}
+			if already {
+				continue
+			}
+			qName := qualifyContextName(registry, name, path)
+			registry[qName] = contextEntry{
+				SourceFile: path,
+				InFileName: name,
+			}
+			changed = true
+		}
+	}
+	return changed
+}
+
 // aggregateExecPluginCommands walks every context across every per-file
 // config and returns the unique sorted set of exec-plugin basenames plus
 // the list of AuthInfos that reference exec blocks with empty Commands.