main: gate AdminForward on adminEnabled + close conn cache (review)

Four findings from Claude / Codex / CodeRabbit on PR #648:

1) **P1 / Major**: AdminForward gRPC registration was gated only on
   adminForwardServerDeps.readyForRegistration() (tables + roles both
   non-nil), neither of which referenced *adminEnabled. A node started
   with -adminEnabled=false but with admin access keys still configured
   would have registered the leader-side gRPC AdminForward service and
   accepted forwarded admin writes — re-exposing the table-mutation
   surface a follower-direct admin call could reach. Fix: build
   roleStore + connCache only when *adminEnabled. With admin disabled,
   forwardDeps.roles stays nil and registerAdminForwardServer's
   readyForRegistration gate skips the gRPC bind entirely.

2) **Resource leak (Claude Issue 1, CodeRabbit Minor)**: connCache was
   created in startServers() but never closed. Every address dialled
   accumulates an open HTTP/2 connection that is never released.
   Fix: shutdown goroutine that drains the cache via Close() on
   in.ctx.Done(), wired through the existing errgroup.

3) **Lint hygiene (Claude Issue 2)**: //nolint:nilnil suppresses a
   linter that is not enabled in .golangci.yaml AND violates
   CLAUDE.md's "Avoid //nolint — refactor instead" rule. The directive
   was pure dead weight. Fix: drop the directive; the doc comment on
   the function and the inline comment already explain the
   nil-means-disabled contract.

4) **Coverage gap (Claude Issue 3)**: buildAdminLeaderForwarder's
   nil-coordinate / nil-cache short-circuit was untested. A future
   refactor that drops the guard would silently pass a nil
   collaborator into buildLeaderForwarder, which would either crash
   on the nil deref or build a forwarder that panics on the first
   request. Fix: TestBuildAdminLeaderForwarder_NilGateReturnsNoForwarder
   covers all three nil combinations + the empty-nodeID error path,
   plus a happy-path companion.

Author: bootjp

main.go

@@ -654,18 +654,40 @@ func startServers(in serversInput) error {
 	if err != nil {
 		return err
 	}
-	// roleStore is parsed from flags up-front so the leader-side gRPC
-	// AdminForward registration in startRaftServers (which runs from
-	// runner.start() below, before startAdminFromFlags has a chance
-	// to parse the admin config) can re-use the same access-key map
-	// the HTTP path will eventually populate.
-	roleStore := roleStoreFromFlags(parseCSV(*adminFullAccessKeys), parseCSV(*adminReadOnlyAccessKeys))
-	// connCache is shared between the follower-side LeaderForwarder
-	// (built inside startAdminFromFlags) and any future bridge that
-	// dials the leader's gRPC ports. Keeping a single instance per
-	// process means the two paths re-use TLS / HTTP/2 connections
-	// rather than each maintaining a parallel pool.
-	connCache := &kv.GRPCConnCache{}
+	// roleStore + connCache are gated on *adminEnabled. With admin
+	// disabled, building either is wasted work AND a security
+	// regression risk: a non-empty -adminFullAccessKeys flag would
+	// otherwise still flip forwardDeps.readyForRegistration() to
+	// true, registering the leader-side gRPC AdminForward service
+	// and re-exposing the table-write surface a follower-direct
+	// admin call could reach (Codex P1, CodeRabbit Major on #648).
+	// The HTTP admin listener already short-circuits in
+	// startAdminFromFlags when *adminEnabled is false; the gRPC path
+	// must do the same.
+	var (
+		roleStore admin.RoleStore
+		connCache *kv.GRPCConnCache
+	)
+	if *adminEnabled {
+		roleStore = roleStoreFromFlags(parseCSV(*adminFullAccessKeys), parseCSV(*adminReadOnlyAccessKeys))
+		// connCache is shared between the follower-side LeaderForwarder
+		// (built inside startAdminFromFlags) and any future bridge that
+		// dials the leader's gRPC ports. Keeping a single instance per
+		// process means the two paths re-use TLS / HTTP/2 connections
+		// rather than each maintaining a parallel pool. The shutdown
+		// goroutine drains the cache on context cancellation so the
+		// accumulated HTTP/2 connections are not leaked when the
+		// process exits gracefully (Claude review on #648).
+		connCache = &kv.GRPCConnCache{}
+		cache := connCache
+		in.eg.Go(func() error {
+			<-in.ctx.Done()
+			if err := cache.Close(); err != nil {
+				return errors.Wrap(err, "close admin gRPC connection cache")
+			}
+			return nil
+		})
+	}
 	runner := runtimeServerRunner{
 		ctx:             in.ctx,
 		lc:              in.lc,

main_admin.go

@@ -134,7 +134,13 @@ func startAdminFromFlags(
 // only builds; that's handled higher up by ServerDeps.Tables == nil.
 func buildAdminLeaderForwarder(coordinate kv.Coordinator, connCache *kv.GRPCConnCache, nodeID string) (admin.LeaderForwarder, error) {
 	if coordinate == nil || connCache == nil {
-		return nil, nil //nolint:nilnil // explicit "no forwarder" signal — the handler falls back to 503 + Retry-After:1.
+		// Returning (nil, nil) is the explicit "no forwarder" signal
+		// — the handler falls back to 503 + Retry-After:1 on
+		// ErrTablesNotLeader. The function-level doc comment above
+		// describes this contract; the nilnil linter is not enabled
+		// in .golangci.yaml so no suppression directive is needed
+		// (Claude review on #648).
+		return nil, nil
 	}
 	if nodeID == "" {
 		// admin.NewGRPCForwardClient enforces this too; surfacing

main_admin_forward_test.go

@@ -124,6 +124,59 @@ func TestAdminForwardServerDeps_ReadyForRegistration(t *testing.T) {
 	}.readyForRegistration())
 }
 
+func TestBuildAdminLeaderForwarder_NilGateReturnsNoForwarder(t *testing.T) {
+	// buildAdminLeaderForwarder is the wrapper in main_admin.go that
+	// short-circuits to (nil, nil) when either coordinate or
+	// connCache is nil — the explicit "no forwarder" path for
+	// single-node / leader-only deployments. A future refactor that
+	// drops the guard would silently pass a nil collaborator into
+	// buildLeaderForwarder, which would either crash on the nil
+	// resolver / cache deref or build a forwarder that panics on
+	// the first request. Locking this down keeps the contract intact
+	// (Claude review on #648).
+	cases := []struct {
+		name      string
+		coord     kv.Coordinator
+		cache     *kv.GRPCConnCache
+		nodeID    string
+		wantNil   bool
+		wantError string
+	}{
+		{name: "nil coordinator", cache: &kv.GRPCConnCache{}, nodeID: "n1", wantNil: true},
+		{name: "nil conn cache", coord: &kv.Coordinate{}, nodeID: "n1", wantNil: true},
+		{name: "both nil", nodeID: "n1", wantNil: true},
+		{
+			name:      "complete deps but empty node id",
+			coord:     &kv.Coordinate{},
+			cache:     &kv.GRPCConnCache{},
+			wantError: "--raftId is required",
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			fwd, err := buildAdminLeaderForwarder(tc.coord, tc.cache, tc.nodeID)
+			if tc.wantError != "" {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tc.wantError)
+				require.Nil(t, fwd)
+				return
+			}
+			require.NoError(t, err)
+			if tc.wantNil {
+				require.Nil(t, fwd)
+			} else {
+				require.NotNil(t, fwd)
+			}
+		})
+	}
+}
+
+func TestBuildAdminLeaderForwarder_HappyPathReturnsForwarder(t *testing.T) {
+	fwd, err := buildAdminLeaderForwarder(&kv.Coordinate{}, &kv.GRPCConnCache{}, "n1")
+	require.NoError(t, err)
+	require.NotNil(t, fwd)
+}
+
 // dummyTablesSource is the smallest concrete admin.TablesSource for
 // the readyForRegistration gate test — no method body needs to
 // execute, so every method just panics. Using a real implementation