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,16 +700,48 @@ func GetAvailableContexts() ([]ContextInfo, error) {
 		}, nil
 	}
 
-	clientMu.RLock()
+	// 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).
+	//
+	// refreshContextRegistry returns NEW maps when anything changes,
+	// so we publish them atomically under the write lock. Snapshot
+	// readers (SwitchContext, WriteKubeconfigForCurrentContext) take
+	// bare references under RLock and use them after the unlock — that
+	// pattern is only safe as long as the maps they captured are never
+	// mutated. Returning fresh maps preserves that invariant.
+	clientMu.Lock()
+	if contextRegistry != nil {
+		// Lazy init: a future code path that promotes single-file mode
+		// to isolated-load without touching perFileMtimes would leave
+		// it nil. Seeding it here is safe because we always hold the
+		// write lock and refresh's nil guard catches it too.
+		if perFileMtimes == nil {
+			perFileMtimes = make(map[string]time.Time, len(perFileConfigs))
+		}
+		newRegistry, newFileConfigs, newFileMtimes, changed := refreshContextRegistry(
+			contextRegistry, perFileConfigs, perFileMtimes,
+		)
+		if changed {
+			contextRegistry = newRegistry
+			perFileConfigs = newFileConfigs
+			perFileMtimes = newFileMtimes
+		}
+	}
 	registry := contextRegistry
 	fileConfigs := perFileConfigs
 	currentCtx := contextName
-	clientMu.RUnlock()
+	clientMu.Unlock()
 
 	if registry != 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.
+		// Iterating outside the lock is safe because refresh publishes
+		// fresh maps on change rather than mutating in place, so the
+		// snapshot we captured is frozen.
 		contexts := make([]ContextInfo, 0, len(registry))
 		for qName, entry := range registry {
 			cfg, ok := fileConfigs[entry.SourceFile]
@@ -913,9 +953,17 @@ func MergeAndSwitchContext(kubeconfigData []byte, contextName string) (string, s
 	if existingPath, ok := capiKubeconfigs[contextName]; ok {
 		if err := clientcmd.WriteToFile(*newConfig, existingPath); err == nil {
 			// Refresh the cached parsed config so subsequent GetAvailableContexts
-			// calls reflect any changes in the incoming YAML.
+			// calls reflect any changes in the incoming YAML. Also bump the
+			// cached mtime so the next refresh doesn't see a stale value
+			// (the WriteToFile above just changed the file's mtime) and
+			// uselessly re-parse a file we've already re-parsed here.
 			if parsed, perr := clientcmd.LoadFromFile(existingPath); perr == nil {
 				perFileConfigs[existingPath] = parsed
+				if perFileMtimes != nil {
+					if info, serr := os.Stat(existingPath); serr == nil {
+						perFileMtimes[existingPath] = info.ModTime()
+					}
+				}
 			}
 			qName := findQualifiedNameForPath(contextRegistry, existingPath, contextName)
 			if qName == "" {
@@ -948,6 +996,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 +1013,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 +1043,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 +1069,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,173 @@ func pickInitialContext(
 	return "", contextEntry{}, false
 }
 
+// refreshContextRegistry reconciles the in-memory contextRegistry +
+// perFileConfigs against what's actually on disk RIGHT NOW. Returns
+// new map values (registry, fileConfigs, fileMtimes) plus a `changed`
+// flag. When `changed` is false the returned maps are guaranteed to
+// equal the inputs and callers can keep the originals.
+//
+// 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: returns NEW maps instead of mutating in place so
+// callers (GetAvailableContexts under Lock, plus snapshot-style
+// readers like SwitchContext under RLock) can atomically swap the
+// package globals. The maps the inputs point at are never mutated,
+// preserving the post-init "publish once, never modify" invariant
+// that the snapshot-then-read pattern relies on.
+func refreshContextRegistry(
+	registry map[string]contextEntry,
+	fileConfigs map[string]*clientcmdapi.Config,
+	fileMtimes map[string]time.Time,
+) (
+	map[string]contextEntry,
+	map[string]*clientcmdapi.Config,
+	map[string]time.Time,
+	bool,
+) {
+	if registry == nil {
+		return registry, fileConfigs, fileMtimes, false
+	}
+	// Defensive nil-check on fileMtimes: callers
+	// (GetAvailableContexts) already lazy-init this, but the helper
+	// is exported and a future caller that forgets would otherwise
+	// panic on the first `fileMtimes[path] = mtime` write below.
+	if fileMtimes == nil {
+		return registry, fileConfigs, fileMtimes, false
+	}
+	// Group registry entries by source file so we can decide
+	// per-file: keep, re-parse, or drop everything pointing at it.
+	// Seed byFile from BOTH the registry AND fileMtimes — if a
+	// previous refresh dropped every context for a file (e.g. user
+	// removed all kubectl-config-delete-context'd from a single
+	// file), the file's path stays in fileMtimes but no longer
+	// appears in registry. Without seeding from fileMtimes, that
+	// file would never be re-stat'd and any newly-added contexts
+	// to it would be invisible until the user restarted Radar.
+	byFile := make(map[string][]string)
+	for qName, entry := range registry {
+		byFile[entry.SourceFile] = append(byFile[entry.SourceFile], qName)
+	}
+	for path := range fileMtimes {
+		if _, ok := byFile[path]; !ok {
+			byFile[path] = nil
+		}
+	}
+	// Walk each file once, deciding whether to keep / drop / re-parse.
+	// We start from a lazy "no changes" state and only allocate fresh
+	// maps when something actually changes. This keeps the steady-state
+	// cost (most calls find nothing to do) at a few stat()s.
+	newRegistry := registry
+	newFileConfigs := fileConfigs
+	newFileMtimes := fileMtimes
+	changed := false
+	cloneOnce := func() {
+		if changed {
+			return
+		}
+		changed = true
+		nr := make(map[string]contextEntry, len(registry))
+		for k, v := range registry {
+			nr[k] = v
+		}
+		nfc := make(map[string]*clientcmdapi.Config, len(fileConfigs))
+		for k, v := range fileConfigs {
+			nfc[k] = v
+		}
+		nm := make(map[string]time.Time, len(fileMtimes))
+		for k, v := range fileMtimes {
+			nm[k] = v
+		}
+		newRegistry = nr
+		newFileConfigs = nfc
+		newFileMtimes = nm
+	}
+	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.
+			cloneOnce()
+			for _, qName := range qNames {
+				delete(newRegistry, qName)
+			}
+			delete(newFileConfigs, path)
+			delete(newFileMtimes, path)
+			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
+		}
+		cloneOnce()
+		newFileConfigs[path] = cfg
+		newFileMtimes[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[newRegistry[qName].InFileName]; !alive {
+				delete(newRegistry, qName)
+			}
+		}
+		// 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 := newRegistry[qName]; ok && e.SourceFile == path && e.InFileName == name {
+					already = true
+					break
+				}
+			}
+			if already {
+				continue
+			}
+			qName := qualifyContextName(newRegistry, name, path)
+			newRegistry[qName] = contextEntry{
+				SourceFile: path,
+				InFileName: name,
+			}
+		}
+	}
+	return newRegistry, newFileConfigs, newFileMtimes, 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.