feat(main): keyviz wiring follow-up (--keyvizHistoryColumns + Phase-2 read TODO) (#651)

## Summary

Follow-up to PR #647, which was merged at the round-1 commit before
round-2 review fixes propagated. This PR carries the orphaned round-2
changes:

- New `--keyvizHistoryColumns` flag (defaults to
`keyviz.DefaultHistoryColumns = 1440`, i.e. 24h at 60s) so operators can
shorten the ring buffer for high-cardinality clusters without
rebuilding.
- `startKeyVizFlusher` early-returns when the sampler is nil instead of
spawning a goroutine that just parks on `ctx.Done` — the goroutine was
harmless but had no signal.
- TODO on `observeMutation` documenting the Phase-2 read-sampling
milestone (design §5.1, §10) so future readers don't think the missing
read path is a regression. Until that wiring lands the matrix's
`Reads`/`ReadBytes` series stay zero.

These items came out of Claude bot's round-2 review of #647 but landed
after the merge button was pressed.

## Test plan

- [x] `go build .`, `go vet .`, `golangci-lint run ./...` clean.
- [x] `go test -race -count=1 -run
'TestBuildKeyVizSampler|TestSeedKeyVizRoutes|TestStartKeyVizFlusher' .`
clean.

Author: bootjp

keyviz/sampler.go

@@ -82,6 +82,15 @@ const (
 	DefaultMaxMemberRoutesPerSlot = 256
 )
 
+// MaxHistoryColumns is the upper bound on opts.HistoryColumns. The
+// ring buffer pre-allocates a slice of capacity HistoryColumns at
+// construction; misconfiguration (e.g. an operator typo of
+// 100_000_000) would otherwise reserve gigabytes up front. 100 000
+// columns at the default 60s Step is ~70 days of history — longer
+// retention is the Phase 3 persistence path's job, not the in-memory
+// ring's.
+const MaxHistoryColumns = 100_000
+
 // MemSamplerOptions configures NewMemSampler. Zero values fall back to
 // the Default* constants above; passing a struct literal with only the
 // fields you care about is the expected call style.
@@ -308,6 +317,9 @@ func NewMemSampler(opts MemSamplerOptions) *MemSampler {
 	if opts.HistoryColumns <= 0 {
 		opts.HistoryColumns = DefaultHistoryColumns
 	}
+	if opts.HistoryColumns > MaxHistoryColumns {
+		opts.HistoryColumns = MaxHistoryColumns
+	}
 	if opts.MaxTrackedRoutes <= 0 {
 		opts.MaxTrackedRoutes = DefaultMaxTrackedRoutes
 	}
@@ -798,6 +810,17 @@ func (s *MemSampler) Step() time.Duration {
 	return s.opts.Step
 }
 
+// HistoryColumns returns the configured ring-buffer length after
+// applying defaults and the MaxHistoryColumns clamp. Wiring tests use
+// this to verify --keyvizHistoryColumns is forwarded end-to-end
+// without exposing the internal opts struct.
+func (s *MemSampler) HistoryColumns() int {
+	if s == nil {
+		return DefaultHistoryColumns
+	}
+	return s.opts.HistoryColumns
+}
+
 // appendDrainedRow swaps the slot's counters to zero and appends a
 // MatrixRow when any counter was non-zero. Idle slots are skipped.
 // Metadata is read under the slot's metaMu so a concurrent

keyviz/sampler_test.go

@@ -869,6 +869,31 @@ func TestNonPositiveOptionsFallBackToDefaults(t *testing.T) {
 	}
 }
 
+// TestHistoryColumnsClampedAboveMax pins the upper bound on
+// HistoryColumns. The ring buffer pre-allocates a slice of capacity
+// HistoryColumns at construction; an operator typo (e.g.
+// 100_000_000) would otherwise reserve gigabytes up front. Confirm
+// that values above MaxHistoryColumns are silently clamped to
+// MaxHistoryColumns instead of trusted as-is.
+func TestHistoryColumnsClampedAboveMax(t *testing.T) {
+	t.Parallel()
+	s, _ := newTestSampler(t, MemSamplerOptions{
+		Step:           time.Second,
+		HistoryColumns: MaxHistoryColumns * 10,
+	})
+	if s.opts.HistoryColumns != MaxHistoryColumns {
+		t.Fatalf("HistoryColumns clamp = %d, want %d", s.opts.HistoryColumns, MaxHistoryColumns)
+	}
+	// At the cap exactly: preserved.
+	s2, _ := newTestSampler(t, MemSamplerOptions{
+		Step:           time.Second,
+		HistoryColumns: MaxHistoryColumns,
+	})
+	if s2.opts.HistoryColumns != MaxHistoryColumns {
+		t.Fatalf("HistoryColumns at cap = %d, want %d", s2.opts.HistoryColumns, MaxHistoryColumns)
+	}
+}
+
 // TestStepAccessor pins the Step() accessor contract: returns the
 // configured Step (after defaulting) for a constructed sampler, and
 // returns DefaultStep for a typed nil so callers wiring RunFlusher

kv/sharded_coordinator.go

@@ -980,6 +980,13 @@ func (c *ShardedCoordinator) txnLogs(reqs *OperationGroup[OP]) ([]*pb.Request, e
 // pre-commit, so a mutation that subsequently fails its Raft
 // proposal is still recorded — the heatmap reflects offered load,
 // not just committed writes (intentional for traffic visualisation).
+//
+// TODO(keyviz Phase 2): the design (§5.1, §10) calls for read
+// sampling on the node that serves the read (LeaseRead /
+// LinearizableRead / follower reads). Until that wiring lands the
+// matrix's Reads / ReadBytes series stay zero. Tracked as a Phase-2
+// milestone in docs/admin_ui_key_visualizer_design.md — not a
+// regression for the writes-first slice this method covers.
 func (c *ShardedCoordinator) observeMutation(routeID uint64, mut *pb.Mutation) {
 	if c.sampler == nil {
 		return

main.go

@@ -135,6 +135,7 @@ var (
 	keyvizStep                   = flag.Duration("keyvizStep", keyviz.DefaultStep, "Flush interval / matrix-column resolution for the keyviz sampler")
 	keyvizMaxTrackedRoutes       = flag.Int("keyvizMaxTrackedRoutes", keyviz.DefaultMaxTrackedRoutes, "Maximum routes tracked individually before excess routes coarsen into virtual buckets")
 	keyvizMaxMemberRoutesPerSlot = flag.Int("keyvizMaxMemberRoutesPerSlot", keyviz.DefaultMaxMemberRoutesPerSlot, "Maximum members listed on a virtual bucket; excess routes still drive the bucket counters")
+	keyvizHistoryColumns         = flag.Int("keyvizHistoryColumns", keyviz.DefaultHistoryColumns, "Maximum matrix columns retained in the keyviz ring buffer (each column = one Step)")
 )
 
 const adminTokenMaxBytes = 4 << 10
@@ -1376,6 +1377,7 @@ func buildKeyVizSampler() *keyviz.MemSampler {
 	}
 	return keyviz.NewMemSampler(keyviz.MemSamplerOptions{
 		Step:                   *keyvizStep,
+		HistoryColumns:         *keyvizHistoryColumns,
 		MaxTrackedRoutes:       *keyvizMaxTrackedRoutes,
 		MaxMemberRoutesPerSlot: *keyvizMaxMemberRoutesPerSlot,
 	})
@@ -1411,9 +1413,13 @@ func seedKeyVizRoutes(s *keyviz.MemSampler, engine *distribution.Engine) {
 // startKeyVizFlusher launches RunFlusher in the supplied errgroup
 // and harvests the in-progress step with a final Flush after the
 // goroutine returns, so a graceful shutdown does not lose the most
-// recent partial column. Nil-safe: a disabled sampler reduces the
-// goroutine to ctx-wait + a no-op Flush.
+// recent partial column. Skip the goroutine entirely when the
+// sampler is disabled — RunFlusher would just park on ctx.Done with
+// no work to do, which is a free goroutine but adds no signal.
 func startKeyVizFlusher(ctx context.Context, eg *errgroup.Group, s *keyviz.MemSampler) {
+	if s == nil {
+		return
+	}
 	eg.Go(func() error {
 		keyviz.RunFlusher(ctx, s, s.Step())
 		s.Flush()

main_keyviz_test.go

@@ -14,16 +14,20 @@ import (
 // TestBuildKeyVizSamplerHonorsEnabledFlag pins the on/off contract:
 // --keyvizEnabled=false returns nil (so coordinator/admin server take
 // the disabled paths), and --keyvizEnabled=true with explicit options
-// returns a configured sampler.
+// returns a configured sampler whose options match every flag.
 func TestBuildKeyVizSamplerHonorsEnabledFlag(t *testing.T) {
 	t.Parallel()
-	withFlags(t, false, time.Second, 5, 7, func() {
+	withFlags(t, false, time.Second, 5, 7, 16, func() {
 		require.Nil(t, buildKeyVizSampler())
 	})
-	withFlags(t, true, 250*time.Millisecond, 5, 7, func() {
+	withFlags(t, true, 250*time.Millisecond, 5, 7, 16, func() {
 		s := buildKeyVizSampler()
 		require.NotNil(t, s)
 		require.Equal(t, 250*time.Millisecond, s.Step())
+		// Pin the --keyvizHistoryColumns forwarding so a future
+		// refactor that drops the flag from buildKeyVizSampler is
+		// caught here, not at runtime.
+		require.Equal(t, 16, s.HistoryColumns())
 	})
 }
 
@@ -95,23 +99,26 @@ func withFlags(
 	t *testing.T,
 	enabled bool,
 	step time.Duration,
-	maxTracked, maxMembers int,
+	maxTracked, maxMembers, historyColumns int,
 	fn func(),
 ) {
 	t.Helper()
 	prevEnabled := *keyvizEnabled
 	prevStep := *keyvizStep
 	prevMaxTracked := *keyvizMaxTrackedRoutes
 	prevMaxMembers := *keyvizMaxMemberRoutesPerSlot
+	prevHistoryColumns := *keyvizHistoryColumns
 	*keyvizEnabled = enabled
 	*keyvizStep = step
 	*keyvizMaxTrackedRoutes = maxTracked
 	*keyvizMaxMemberRoutesPerSlot = maxMembers
+	*keyvizHistoryColumns = historyColumns
 	defer func() {
 		*keyvizEnabled = prevEnabled
 		*keyvizStep = prevStep
 		*keyvizMaxTrackedRoutes = prevMaxTracked
 		*keyvizMaxMemberRoutesPerSlot = prevMaxMembers
+		*keyvizHistoryColumns = prevHistoryColumns
 	}()
 	fn()
 }