From 2c8bbcf64665aade83b8649751e6a379ec6c8fbf Mon Sep 17 00:00:00 2001
From: jedarden <github@jedarden.com>
Date: Sun, 24 May 2026 09:47:54 -0400
Subject: [PATCH] fix(fleet): remove duplicate route registration to prevent
 chi panic

- Remove duplicate node-specific routes (role, label, locate, delete) from
  FleetHandler.RegisterRoutes to avoid chi panic on duplicate registration
- Keep only unique FleetHandler routes: /api/fleet/health, /api/fleet/history,
  /api/fleet/optimise, /api/fleet/simulate
- Add startup smoke test TestRouteRegistrationNoPanic to verify both Handler
  and FleetHandler can be registered on same router without panic

main.go registers both fleet.NewHandler and fleet.NewFleetHandler on the
same router, which previously caused chi to panic due to duplicate routes:
  POST /api/nodes/{mac}/role
  PATCH /api/nodes/{mac}/label
  POST /api/nodes/{mac}/locate
  DELETE /api/nodes/{mac}

The Handler has comprehensive node/room/mode endpoints while FleetHandler
focuses on health/optimization/simulation, so duplicates are removed from
FleetHandler.

Closes: bf-3o15x
---
 mothership/internal/fleet/fleethandler.go | 17 ++------
 mothership/internal/fleet/handler_test.go | 52 +++++++++++++++++++++++
 2 files changed, 56 insertions(+), 13 deletions(-)

diff --git a/mothership/internal/fleet/fleethandler.go b/mothership/internal/fleet/fleethandler.go
index 85ff81d..e1baa5a 100644
--- a/mothership/internal/fleet/fleethandler.go
+++ b/mothership/internal/fleet/fleethandler.go
@@ -34,29 +34,20 @@ func (h *FleetHandler) SetUnpairedProvider(p UnpairedProvider) {
 	h.unpairedProvider = p
 }
 
-// RegisterRoutes mounts fleet endpoints on r.
+// RegisterRoutes mounts fleet health/optimisation endpoints on r.
 //
-//	GET  /api/fleet           — all provisioned nodes with full details
 //	GET  /api/fleet/health   — current fleet health status
 //	GET  /api/fleet/history  — recent optimisation history
 //	POST /api/fleet/optimise — trigger manual re-optimisation
 //	GET  /api/fleet/simulate — simulate node removal impact
-//	PATCH /api/nodes/{mac}/label — update node label
-//	POST /api/nodes/{mac}/locate — send identify command
-//	POST /api/nodes/{mac}/role — assign new role
-//	DELETE /api/nodes/{mac} — remove from fleet
+//
+// Node-specific routes (role, label, locate, delete) are handled by fleet.Handler
+// to avoid duplicate route registration.
 func (h *FleetHandler) RegisterRoutes(r chi.Router) {
-	r.Get("/api/fleet", h.getFleet)
 	r.Get("/api/fleet/health", h.getFleetHealth)
 	r.Get("/api/fleet/history", h.getFleetHistory)
 	r.Post("/api/fleet/optimise", h.triggerOptimise)
 	r.Get("/api/fleet/simulate", h.simulateNodeRemoval)
-
-	// Node-specific routes
-	r.Patch("/api/nodes/{mac}/label", h.setNodeLabel)
-	r.Post("/api/nodes/{mac}/locate", h.locateNode)
-	r.Post("/api/nodes/{mac}/role", h.setNodeRole)
-	r.Delete("/api/nodes/{mac}", h.removeNode)
 }
 
 // fleetHealthResponse is the wire format for /api/fleet/health
diff --git a/mothership/internal/fleet/handler_test.go b/mothership/internal/fleet/handler_test.go
index 50f11c8..b17946f 100644
--- a/mothership/internal/fleet/handler_test.go
+++ b/mothership/internal/fleet/handler_test.go
@@ -2510,3 +2510,55 @@ func TestHandlerDisableEnableRoundTrip(t *testing.T) {
 		t.Errorf("Expected role_before_disable to be cleared after enable, got %s", savedRole)
 	}
 }
+
+// TestRouteRegistrationNoPanic verifies that registering both Handler and FleetHandler
+// on the same router does not cause chi to panic with duplicate route registration.
+// This is a startup smoke test to catch accidental route conflicts early.
+func TestRouteRegistrationNoPanic(t *testing.T) {
+	reg := newTestRegistry(t)
+	mgr := NewManager(reg)
+	selfHealMgr := NewSelfHealManager(reg, NewRoleOptimiser(DefaultOptimisationConfig()), DefaultSelfHealConfig())
+
+	// Create both handlers as main.go does
+	h := NewHandler(mgr)
+	fleetHealthHandler := NewFleetHandler(selfHealMgr, reg)
+
+	// This should not panic - if there are duplicate routes, chi will panic here
+	r := chi.NewRouter()
+	h.RegisterRoutes(r)
+	fleetHealthHandler.RegisterRoutes(r)
+
+	// Verify expected routes are registered
+	// From Handler: /api/nodes, /api/fleet (list), /api/nodes/{mac}/role, etc.
+	// From FleetHandler: /api/fleet/health, /api/fleet/history, /api/fleet/optimise, /api/fleet/simulate
+	// The key is no panic on duplicate routes like POST /api/nodes/{mac}/role
+
+	// Try to walk the routes to ensure they're registered
+	routes := chi.Routes(r)
+	if len(routes) == 0 {
+		t.Fatal("No routes registered")
+	}
+
+	// Verify some expected routes exist
+	expectedRoutes := []string{
+		"/api/nodes",
+		"/api/fleet",
+		"/api/fleet/health",
+		"/api/fleet/history",
+		"/api/fleet/optimise",
+		"/api/fleet/simulate",
+	}
+
+	for _, expected := range expectedRoutes {
+		found := false
+		for _, route := range routes {
+			if route.Pattern == expected {
+				found = true
+				break
+			}
+		}
+		if !found {
+			t.Errorf("Expected route %s not found", expected)
+		}
+	}
+}