fix(admin): default-group-scoped leader probe + RFC-compliant 405

Address findings from PR #689 round-1 review:

- [Codex P1] Scope newAdminLeaderProbe to the **default Raft group**
  via coordinate.IsLeader / coordinate.VerifyLeader instead of the
  prior "any local runtime" loop. Admin write paths
  (kv/sharded_coordinator.go, adapter/sqs_admin.go) all key off the
  default group, so a node leading only a non-default group could
  return 200 here while still rejecting admin writes -- breaking the
  endpoint's stated load-balancer contract. Routing through the
  coordinator keeps the healthz answer aligned with what the
  AdminForward proxy and SQS admin write path treat as authoritative.

  newAdminLeaderProbe(runtimes []*raftGroupRuntime) becomes
  newAdminLeaderProbe(coordinate kv.Coordinator). The change drops
  the per-runtime engine.VerifyLeader fan-out and the
  adminLeaderProbeTimeout constant -- coordinate.VerifyLeader is the
  same ReadIndex round-trip lease reads use, with the engine's own
  bounded deadline, so an outer 2 s timeout is no longer needed.

- [Claude / Gemini] Add the RFC 7231 §6.5.5 Allow: GET, HEAD header
  to every router-served 405 response (healthz, healthz/leader,
  asset, SPA). Pulled the body shape into a writeMethodNotAllowed
  helper so the four call sites stay uniform; load balancers and
  synthetic-monitor tools now see the same Allow header the S3 and
  DynamoDB /healthz/leader handlers already advertise. New
  TestRouter_405_AllowHeader sweeps every 405-emitting path so a
  future handler that bypasses the helper would regress this test.

- [Claude / Gemini] Drop the redundant rt.leader == nil check inside
  serveLeaderHealth. dispatch() already short-circuits the nil case
  to rt.notFound before this handler is ever invoked; the in-body
  check was dead code that misled readers about the precondition.
  Documented the precondition explicitly on the function.

- [Gemini] Rewrite the misleading "ordering before /admin/healthz"
  comment in classify(). Both healthz paths are exact-match equality
  checks; their relative order does not matter for correctness. The
  load-bearing rule is that BOTH must run before the SPA prefix
  branch -- now spelled out that way.

Self-review (CLAUDE.md 5 lenses):
1. Data loss -- None. Read-only handlers; no Raft proposals touched.
2. Concurrency -- The probe now serialises through the coordinator's
   default-group view, which is the same path lease reads use.
   Behaviour during leadership change is unchanged: VerifyLeader's
   ReadIndex returns an error, the probe returns false, the LB
   takes the node out of rotation.
3. Performance -- Cheaper than the prior per-runtime loop in the
   common single-group case (one IsLeader / one VerifyLeader vs N
   per-runtime Status checks plus M VerifyLeader calls).
4. Data consistency -- coordinate.VerifyLeader is the same ReadIndex
   round-trip the lease-read path uses; the load balancer can now
   key off the same liveness signal admin writes do.
5. Test coverage -- new TestRouter_405_AllowHeader. Existing six
   /admin/healthz/leader tests still cover happy / 503 / HEAD /
   POST / nil-probe / SPA-no-swallow. The redundant-nil-guard
   removal is covered by TestRouter_HealthzLeader_NilProbeReturns404
   (still passes via dispatch()'s nil check).

Body-text parity nit: the PR description's "identical responses to
S3 and DynamoDB" is technically true vs DynamoDB but not S3 (S3
omits the trailing newline; this admin path matches DynamoDB). Not
worth a behaviour change -- LBs key off status codes, not body
strings -- but worth noting in case a future PR aligns the three.

Author: bootjp

internal/admin/router.go

@@ -150,9 +150,11 @@ func (rt *Router) classify(p string) routeKind {
 	if k, ok := classifyAssets(p); ok {
 		return k
 	}
-	// /admin/healthz/leader must be checked before /admin/healthz so
-	// the longer path does not fall through the equality check and
-	// resolve to the SPA fallback.
+	// Both /admin/healthz and /admin/healthz/leader are exact-match
+	// equality checks — relative ordering between them does not affect
+	// correctness, but BOTH must run before the SPA prefix branch
+	// below; otherwise the catch-all "anything under /admin/" resolves
+	// these paths to index.html.
 	if p == pathHealthzLeader {
 		return routeHealthzLeader
 	}
@@ -214,9 +216,27 @@ func (rt *Router) dispatch(k routeKind) http.Handler {
 	return rt.notFound
 }
 
+// allowGetHead is the canonical Allow value for read-only handlers
+// (healthz / SPA / static assets). RFC 7231 §6.5.5 requires every 405
+// to advertise the supported methods; load balancers and synthetic-
+// monitor tools key off this header to discover the right verbs
+// without scraping the body. Mirrors the value the S3 and DynamoDB
+// /healthz/leader handlers already set
+// (adapter/s3.go:2404, adapter/dynamodb.go:399).
+const allowGetHead = "GET, HEAD"
+
+// writeMethodNotAllowed centralises the read-only 405 response shape
+// for router-served paths. The Allow header is set BEFORE
+// writeJSONError because writeJSONError calls w.WriteHeader, after
+// which header mutations would silently no-op.
+func writeMethodNotAllowed(w http.ResponseWriter) {
+	w.Header().Set("Allow", allowGetHead)
+	writeJSONError(w, http.StatusMethodNotAllowed, "method_not_allowed", "only GET or HEAD supported")
+}
+
 func (rt *Router) serveHealth(w http.ResponseWriter, r *http.Request) {
 	if r.Method != http.MethodGet && r.Method != http.MethodHead {
-		writeJSONError(w, http.StatusMethodNotAllowed, "method_not_allowed", "only GET or HEAD supported")
+		writeMethodNotAllowed(w)
 		return
 	}
 	w.Header().Set("Content-Type", "text/plain; charset=utf-8")
@@ -233,13 +253,17 @@ func (rt *Router) serveHealth(w http.ResponseWriter, r *http.Request) {
 // /healthz/leader endpoints (adapter/s3.go:serveS3LeaderHealthz,
 // adapter/dynamodb.go:serveDynamoLeaderHealthz) so an upstream load
 // balancer that probes any of the three sees identical semantics.
+//
+// Precondition: rt.leader is non-nil. dispatch() short-circuits the
+// nil case to rt.notFound before this handler is ever invoked, so a
+// belt-and-braces nil check inside this body would be dead code.
 func (rt *Router) serveLeaderHealth(w http.ResponseWriter, r *http.Request) {
 	if r.Method != http.MethodGet && r.Method != http.MethodHead {
-		writeJSONError(w, http.StatusMethodNotAllowed, "method_not_allowed", "only GET or HEAD supported")
+		writeMethodNotAllowed(w)
 		return
 	}
 	status, body := http.StatusOK, "ok\n"
-	if rt.leader == nil || !rt.leader.IsVerifiedLeader() {
+	if !rt.leader.IsVerifiedLeader() {
 		status, body = http.StatusServiceUnavailable, "not leader\n"
 	}
 	w.Header().Set("Content-Type", "text/plain; charset=utf-8")
@@ -258,7 +282,7 @@ func (rt *Router) serveAsset(w http.ResponseWriter, r *http.Request) {
 	// the file body for a write request) or surface as a confusing
 	// 404 — neither matches the API contract for assets.
 	if r.Method != http.MethodGet && r.Method != http.MethodHead {
-		writeJSONError(w, http.StatusMethodNotAllowed, "method_not_allowed", "only GET or HEAD supported")
+		writeMethodNotAllowed(w)
 		return
 	}
 	if rt.static == nil {
@@ -335,7 +359,7 @@ func (rt *Router) serveSPA(w http.ResponseWriter, r *http.Request) {
 	// /admin/something returned a JSON 404 with a nil static and a
 	// JSON 405 with a populated static — same URL, different answer.
 	if r.Method != http.MethodGet && r.Method != http.MethodHead {
-		writeJSONError(w, http.StatusMethodNotAllowed, "method_not_allowed", "only GET or HEAD supported")
+		writeMethodNotAllowed(w)
 		return
 	}
 	if rt.static == nil {

internal/admin/router_test.go

@@ -119,12 +119,47 @@ func TestRouter_HealthzLeader_HeadOmitsBody(t *testing.T) {
 
 // TestRouter_HealthzLeader_RejectsPost guards the method allowlist:
 // only GET / HEAD are accepted. Mirrors TestRouter_HealthzRejectsPost.
+// Also asserts the Allow: GET, HEAD header required by RFC 7231
+// §6.5.5 — load balancers and synthetic-monitor tools key off this
+// header to discover supported verbs.
 func TestRouter_HealthzLeader_RejectsPost(t *testing.T) {
 	r := NewRouterWithLeaderProbe(nil, nil, LeaderProbeFunc(func() bool { return true }))
 	req := httptest.NewRequest(http.MethodPost, "/admin/healthz/leader", strings.NewReader(""))
 	rec := httptest.NewRecorder()
 	r.ServeHTTP(rec, req)
 	require.Equal(t, http.StatusMethodNotAllowed, rec.Code)
+	require.Equal(t, "GET, HEAD", rec.Header().Get("Allow"))
+}
+
+// TestRouter_405_AllowHeader sweeps every router-served path that
+// rejects non-GET/HEAD verbs and asserts each emits the
+// RFC-required Allow header. Locks down the writeMethodNotAllowed
+// invariant — a future handler that bypasses the helper would
+// regress this test rather than silently shipping a non-compliant
+// 405.
+func TestRouter_405_AllowHeader(t *testing.T) {
+	t.Parallel()
+	cases := []struct {
+		name string
+		path string
+	}{
+		{"healthz", "/admin/healthz"},
+		{"healthz_leader", "/admin/healthz/leader"},
+		{"asset", "/admin/assets/app.js"},
+		{"spa", "/admin/somewhere"},
+	}
+	r := NewRouterWithLeaderProbe(nil, newTestStatic(), LeaderProbeFunc(func() bool { return true }))
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			req := httptest.NewRequest(http.MethodPost, tc.path, strings.NewReader(""))
+			rec := httptest.NewRecorder()
+			r.ServeHTTP(rec, req)
+			require.Equal(t, http.StatusMethodNotAllowed, rec.Code)
+			require.Equal(t, "GET, HEAD", rec.Header().Get("Allow"),
+				"path %s missing RFC 7231 Allow header on 405", tc.path)
+		})
+	}
 }
 
 // TestRouter_HealthzLeader_NilProbeReturns404 locks down the

main_admin.go

@@ -135,55 +135,46 @@ func startAdminFromFlags(
 	if err != nil {
 		return errors.Wrap(err, "build admin leader forwarder")
 	}
-	leaderProbe := newAdminLeaderProbe(runtimes)
+	leaderProbe := newAdminLeaderProbe(coordinate)
 	_, err = startAdminServer(ctx, lc, eg, cfg, staticCreds, clusterSrc, tablesSrc, bucketsSrc, queuesSrc, forwarder, leaderProbe, keyvizSampler, keyvizFanoutCfg, buildVersion())
 	return err
 }
 
-// adminLeaderProbeTimeout caps the per-runtime VerifyLeader ReadIndex
-// round-trip the /admin/healthz/leader handler triggers. Keep it short:
-// a load balancer that probes every few seconds wants a fast "yes / no"
-// even when the cluster is mid-election; surfacing 503 on a stalled
-// ReadIndex is the right answer for routing.
-const adminLeaderProbeTimeout = 2 * time.Second
-
 // newAdminLeaderProbe builds the LeaderProbe consumed by
 // /admin/healthz/leader. It mirrors the verified-leader pattern S3 and
 // DynamoDB use on their own /healthz/leader endpoints
 // (adapter/s3.go:isVerifiedS3Leader,
-// adapter/dynamodb.go:isVerifiedDynamoLeader): a cheap Status check
+// adapter/dynamodb.go:isVerifiedDynamoLeader): a cheap IsLeader check
 // short-circuits non-leaders, and only nodes claiming leadership pay
 // the ReadIndex round-trip that confirms the claim is still valid.
 //
-// "Leader of any local Raft group" is treated as "yes" so a multi-group
-// deployment that distributes leadership across nodes still surfaces a
-// useful answer to a load balancer that wants to skip the
-// AdminForward proxy hop. In the common single-group / co-located
-// deployment, this collapses to "is this node THE leader".
+// Crucially the probe is scoped to the **default Raft group** (via
+// coordinate.IsLeader / coordinate.VerifyLeader) — the same group the
+// admin write paths key off (kv/sharded_coordinator.go), which the
+// AdminForward proxy and the SQS admin write path
+// (adapter/sqs_admin.go) both treat as authoritative. An earlier
+// design returned true on any local-group leadership; in a multi-group
+// deployment that could surface 200 on a node leading only a non-
+// default group while admin writes there still 503'd or forwarded.
+// Codex P1 on PR #689 caught this; using the coordinator keeps the
+// healthz contract aligned with the actual admin-write leader.
 //
-// Returns nil for an empty runtimes slice so the router answers
+// Returns nil when no coordinator is wired so the router answers
 // /admin/healthz/leader with the standard JSON 404 (matches the
 // "feature off" pattern Tables / Buckets / Queues already use).
-func newAdminLeaderProbe(runtimes []*raftGroupRuntime) admin.LeaderProbe {
-	if len(runtimes) == 0 {
+func newAdminLeaderProbe(coordinate kv.Coordinator) admin.LeaderProbe {
+	if coordinate == nil {
 		return nil
 	}
 	return admin.LeaderProbeFunc(func() bool {
-		for _, rt := range runtimes {
-			if rt == nil || rt.engine == nil {
-				continue
-			}
-			if rt.engine.Status().State != raftengine.StateLeader {
-				continue
-			}
-			ctx, cancel := context.WithTimeout(context.Background(), adminLeaderProbeTimeout)
-			err := rt.engine.VerifyLeader(ctx)
-			cancel()
-			if err == nil {
-				return true
-			}
+		if !coordinate.IsLeader() {
+			return false
 		}
-		return false
+		// VerifyLeader is the same ReadIndex round-trip lease reads
+		// use; under the hood it carries an engine-bounded deadline,
+		// so a stalled cluster surfaces 503 here on its own without
+		// the probe needing an outer timeout.
+		return coordinate.VerifyLeader() == nil
 	})
 }