admin: forward 500 parity + log unexpected source errors (Codex P2 ×2)

Two related findings on the leader-side AdminForward dispatcher.

- Operation-specific 500 codes: forwardErrorResponse mapped any
  unmapped failure to {"error":"internal", ...}, but the HTTP
  handler's writeTablesError emits dynamo_create_failed /
  dynamo_delete_failed. A forwarded write that hit the same
  backend/storage error therefore returned a different code than
  a leader-direct write — clients branching on those codes saw
  divergent behaviour. Fix: forwardErrorResponse now takes an
  `op` argument ("create" or "delete") so the fallthrough emits
  dynamo_<op>_failed, matching leader-direct parity exactly.

- Log unexpected source errors: handleCreate / handleDelete on
  the forward path returned 500 with "see leader logs" in the
  body but never logged anything for non-sentinel source
  failures, leaving operators with no breadcrumb for forwarded
  500s. Add logUnexpectedSourceError that emits an
  admin_forward_<op>_failed line at LevelError carrying the
  table, forwarded_from, and raw error. Sentinels (forbidden,
  not-found, already-exists, validation) skip the log — they're
  routine client-side outcomes, and logging at LevelError would
  drown the operational signal (same policy as the HTTP path's
  writeTablesError).

Tests:
- TestForwardServer_CreateTable_GenericErrorReturns500 now also
  asserts the response body contains dynamo_create_failed.
- New TestForwardServer_DeleteTable_GenericErrorUsesOperationSpecificCode
  pins the delete-side code parity.
- New TestForwardServer_LogsUnexpectedSourceError captures the
  slog output and confirms the table, forwarded_from, and raw
  error all appear.
- New TestForwardServer_DoesNotLogStructuredSourceErrors sweeps
  the four sentinel cases and confirms the LevelError handler
  never emits anything.

Author: bootjp

internal/admin/forward_server.go

@@ -150,7 +150,8 @@ func (s *ForwardServer) handleCreate(ctx context.Context, principal AuthPrincipa
 	}
 	summary, err := s.source.AdminCreateTable(ctx, principal, body)
 	if err != nil {
-		return forwardErrorResponse(err), nil
+		s.logUnexpectedSourceError(ctx, "create_table", body.TableName, req.GetForwardedFrom(), err)
+		return forwardErrorResponse("create", err), nil
 	}
 	s.logger.LogAttrs(ctx, slog.LevelInfo, "admin_audit",
 		slog.String("actor", principal.AccessKey),
@@ -203,7 +204,8 @@ func (s *ForwardServer) handleDelete(ctx context.Context, principal AuthPrincipa
 		return rejectForward(http.StatusBadRequest, "invalid_body", "delete payload name must not contain '/'")
 	}
 	if err := s.source.AdminDeleteTable(ctx, principal, body.Name); err != nil {
-		return forwardErrorResponse(err), nil
+		s.logUnexpectedSourceError(ctx, "delete_table", body.Name, req.GetForwardedFrom(), err)
+		return forwardErrorResponse("delete", err), nil
 	}
 	s.logger.LogAttrs(ctx, slog.LevelInfo, "admin_audit",
 		slog.String("actor", principal.AccessKey),
@@ -220,7 +222,14 @@ func (s *ForwardServer) handleDelete(ctx context.Context, principal AuthPrincipa
 // is the leader-side counterpart of writeTablesError: every status /
 // JSON code the HTTP handler chooses is mirrored here so a forwarded
 // call is indistinguishable to the SPA from a leader-direct call.
-func forwardErrorResponse(err error) *pb.AdminForwardResponse {
+//
+// op is "create" or "delete" so the unmapped 500 fallthrough emits
+// dynamo_create_failed / dynamo_delete_failed — the same
+// operation-specific codes the leader-direct HTTP path produces in
+// writeTablesError. Without this, forwarded write failures showed
+// up to clients as a generic "internal" code, breaking parity with
+// the leader-direct path (Codex P2 on PR #635).
+func forwardErrorResponse(op string, err error) *pb.AdminForwardResponse {
 	switch {
 	case errors.Is(err, ErrTablesForbidden):
 		return mustForwardJSON(http.StatusForbidden, errorBody{Error: "forbidden", Message: "this endpoint requires a full-access role"})
@@ -238,7 +247,45 @@ func forwardErrorResponse(err error) *pb.AdminForwardResponse {
 	if errors.As(err, &verr) {
 		return mustForwardJSON(http.StatusBadRequest, errorBody{Error: "invalid_request", Message: verr.Error()})
 	}
-	return mustForwardJSON(http.StatusInternalServerError, errorBody{Error: "internal", Message: "internal error; see leader logs"})
+	return mustForwardJSON(http.StatusInternalServerError, errorBody{
+		Error:   "dynamo_" + op + "_failed",
+		Message: "failed to " + op + " table; see leader logs",
+	})
+}
+
+// logUnexpectedSourceError emits an error log for non-sentinel
+// source failures so operators have a breadcrumb when forwarded
+// writes 500. Sentinel errors that map to specific HTTP statuses
+// (forbidden, not-found, validation, ...) are deliberately
+// silent: those are routine client-side failures, not server
+// regressions, and logging them at LevelError would drown the
+// operational signal. The HTTP path's writeTablesError applies
+// the same policy (Codex P2 on PR #635 flagged the silent path).
+func (s *ForwardServer) logUnexpectedSourceError(ctx context.Context, op, table, forwardedFrom string, err error) {
+	if isStructuredSourceError(err) {
+		return
+	}
+	s.logger.LogAttrs(ctx, slog.LevelError, "admin_forward_"+op+"_failed",
+		slog.String("table", table),
+		slog.String("forwarded_from", forwardedFrom),
+		slog.String("error", err.Error()),
+	)
+}
+
+// isStructuredSourceError reports whether err is one of the
+// admin-package sentinels or a ValidationError — i.e., a known
+// failure mode the handler maps to a non-500 status. These are
+// expected and not log-worthy.
+func isStructuredSourceError(err error) bool {
+	switch {
+	case errors.Is(err, ErrTablesForbidden),
+		errors.Is(err, ErrTablesNotLeader),
+		errors.Is(err, ErrTablesNotFound),
+		errors.Is(err, ErrTablesAlreadyExists):
+		return true
+	}
+	var verr *ValidationError
+	return errors.As(err, &verr)
 }
 
 // errorBody is the shared JSON shape for both the HTTP handler's

internal/admin/forward_server_test.go

@@ -1,8 +1,10 @@
 package admin
 
 import (
+	"bytes"
 	"context"
 	"errors"
+	"log/slog"
 	"net/http"
 	"testing"
 
@@ -285,4 +287,87 @@ func TestForwardServer_CreateTable_GenericErrorReturns500(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, int32(http.StatusInternalServerError), resp.GetStatusCode())
 	require.NotContains(t, string(resp.GetPayload()), "ZQ-993")
+	// Error code parity with leader-direct path: must use
+	// dynamo_create_failed, not the previous generic "internal"
+	// (Codex P2 on PR #635).
+	require.Contains(t, string(resp.GetPayload()), `"error":"dynamo_create_failed"`)
+}
+
+// TestForwardServer_DeleteTable_GenericErrorUsesOperationSpecificCode
+// is the delete-side counterpart: 500 fallthrough must use
+// dynamo_delete_failed so client retry/branching logic that
+// already handles the leader-direct path works unchanged.
+func TestForwardServer_DeleteTable_GenericErrorUsesOperationSpecificCode(t *testing.T) {
+	src := &stubTablesSource{
+		tables:    map[string]*DynamoTableSummary{"users": {Name: "users"}},
+		deleteErr: errors.New("storage backing sentinel DEL-1"),
+	}
+	srv := newForwardServerForTest(src, fullPrincipalRoleStore())
+	resp, err := srv.Forward(context.Background(), &pb.AdminForwardRequest{
+		Principal: &pb.AdminPrincipal{AccessKey: "AKIA_FULL", Role: "full"},
+		Operation: pb.AdminOperation_ADMIN_OP_DELETE_TABLE,
+		Payload:   []byte(`{"name":"users"}`),
+	})
+	require.NoError(t, err)
+	require.Equal(t, int32(http.StatusInternalServerError), resp.GetStatusCode())
+	require.Contains(t, string(resp.GetPayload()), `"error":"dynamo_delete_failed"`)
+	require.NotContains(t, string(resp.GetPayload()), "DEL-1")
+}
+
+// TestForwardServer_LogsUnexpectedSourceError confirms the leader
+// emits an error log line for non-sentinel source failures so
+// operators can investigate forwarded write 500s without
+// reverse-engineering the response body. The HTTP path's
+// writeTablesError already logs; without the symmetric emit on
+// the forward path, forwarded failures were operationally invisible
+// (Codex P2 on PR #635).
+func TestForwardServer_LogsUnexpectedSourceError(t *testing.T) {
+	var buf bytes.Buffer
+	logger := slog.New(slog.NewJSONHandler(&buf, &slog.HandlerOptions{Level: slog.LevelError}))
+	src := &stubTablesSource{createErr: errors.New("storage sentinel SENT-42")}
+	srv := NewForwardServer(src, fullPrincipalRoleStore(), logger)
+
+	_, err := srv.Forward(context.Background(), &pb.AdminForwardRequest{
+		Principal:     &pb.AdminPrincipal{AccessKey: "AKIA_FULL", Role: "full"},
+		Operation:     pb.AdminOperation_ADMIN_OP_CREATE_TABLE,
+		Payload:       mustJSON(t, CreateTableRequest{TableName: "u", PartitionKey: CreateTableAttribute{Name: "id", Type: "S"}}),
+		ForwardedFrom: "follower-9",
+	})
+	require.NoError(t, err)
+	logged := buf.String()
+	require.Contains(t, logged, "admin_forward_create_table_failed")
+	require.Contains(t, logged, "SENT-42")
+	require.Contains(t, logged, "follower-9")
+}
+
+// TestForwardServer_DoesNotLogStructuredSourceErrors confirms the
+// "expected" sentinels (forbidden, not-found, already-exists,
+// validation) do NOT emit error-level logs. They are routine
+// client-side outcomes, not server regressions, so logging them
+// at LevelError would drown the operational signal.
+func TestForwardServer_DoesNotLogStructuredSourceErrors(t *testing.T) {
+	cases := []struct {
+		name string
+		err  error
+	}{
+		{"forbidden", ErrTablesForbidden},
+		{"not_found", ErrTablesNotFound},
+		{"already_exists", ErrTablesAlreadyExists},
+		{"validation", &ValidationError{Message: "field foo invalid"}},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			var buf bytes.Buffer
+			logger := slog.New(slog.NewJSONHandler(&buf, &slog.HandlerOptions{Level: slog.LevelError}))
+			src := &stubTablesSource{createErr: tc.err}
+			srv := NewForwardServer(src, fullPrincipalRoleStore(), logger)
+			_, _ = srv.Forward(context.Background(), &pb.AdminForwardRequest{
+				Principal: &pb.AdminPrincipal{AccessKey: "AKIA_FULL", Role: "full"},
+				Operation: pb.AdminOperation_ADMIN_OP_CREATE_TABLE,
+				Payload:   mustJSON(t, CreateTableRequest{TableName: "u", PartitionKey: CreateTableAttribute{Name: "id", Type: "S"}}),
+			})
+			require.NotContains(t, buf.String(), "admin_forward_create_table_failed",
+				"sentinel error %q must not produce an error log", tc.name)
+		})
+	}
 }