From efbb7bc8b877461cc28d3da8c5b68954d19e915a Mon Sep 17 00:00:00 2001
From: Mohammed Al Sahaf <msaa1990@gmail.com>
Date: Sun, 30 Aug 2026 18:22:20 +0300
Subject: [PATCH] caddyfile: fix importGraph self-loop and stale-edge bugs

- willCycle now reports a cycle when from == to, so addEdge rejects
  self-loops (a self-importing file was previously accepted).
- removeNode now drops the removed node's outgoing edges and any
  incoming edges pointing at it, keeping the adjacency map consistent.

Also tightens the two tests that previously only logged the buggy
behavior into real assertions.

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
---
 caddyconfig/caddyfile/importgraph.go      |  7 +++++
 caddyconfig/caddyfile/importgraph_test.go | 32 ++++++-----------------
 2 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/caddyconfig/caddyfile/importgraph.go b/caddyconfig/caddyfile/importgraph.go
index ca859299d..b574ccbde 100644
--- a/caddyconfig/caddyfile/importgraph.go
+++ b/caddyconfig/caddyfile/importgraph.go
@@ -44,6 +44,10 @@ func (i *importGraph) addNodes(names []string) {
 
 func (i *importGraph) removeNode(name string) {
 	delete(i.nodes, name)
+	delete(i.edges, name)
+	for k, targets := range i.edges {
+		i.edges[k] = slices.DeleteFunc(targets, func(t string) bool { return t == name })
+	}
 }
 
 func (i *importGraph) removeNodes(names []string) {
@@ -96,6 +100,9 @@ func (i *importGraph) areConnected(from, to string) bool {
 }
 
 func (i *importGraph) willCycle(from, to string) bool {
+	if from == to {
+		return true
+	}
 	collector := make(map[string]bool)
 
 	var visit func(string)
diff --git a/caddyconfig/caddyfile/importgraph_test.go b/caddyconfig/caddyfile/importgraph_test.go
index 4535354a3..e02adfdb4 100644
--- a/caddyconfig/caddyfile/importgraph_test.go
+++ b/caddyconfig/caddyfile/importgraph_test.go
@@ -177,16 +177,8 @@ func TestImportGraphSelfLoop(t *testing.T) {
 	g := &importGraph{}
 	g.addNode("a")
 
-	// BUG: Self-loops are not detected by willCycle(). The function checks if
-	// adding edge from→to would create a cycle by traversing edges from "to"
-	// to see if "from" is reachable. But for a self-loop (from==to), the edge
-	// doesn't exist yet, so the DFS finds nothing and returns false.
-	// A self-importing file would NOT be caught by this cycle detection.
-	err := g.addEdge("a", "a")
-	if err != nil {
-		t.Log("Self-loop was correctly detected (bug may have been fixed)")
-	} else {
-		t.Log("BUG CONFIRMED: addEdge('a', 'a') did not detect self-loop cycle")
+	if err := g.addEdge("a", "a"); err == nil {
+		t.Error("expected error for self-loop cycle a -> a")
 	}
 }
 
@@ -232,9 +224,7 @@ func TestImportGraphAddEdgesWithCycle(t *testing.T) {
 	}
 }
 
-func TestImportGraphRemoveNodeEdgeLeakBug(t *testing.T) {
-	// This test documents a known bug: removeNode doesn't clean up edges.
-	// Edges FROM the removed node remain in the adjacency list.
+func TestImportGraphRemoveNodeCleansEdges(t *testing.T) {
 	g := &importGraph{}
 	g.addNodes([]string{"a", "b", "c"})
 	_ = g.addEdge("a", "b")
@@ -242,20 +232,14 @@ func TestImportGraphRemoveNodeEdgeLeakBug(t *testing.T) {
 
 	g.removeNode("b")
 
-	// Bug: "b" is removed from nodes, but edges from "b" are still in the adjacency list.
-	// This means the graph is now inconsistent.
-	// The node doesn't exist...
 	if g.exists("b") {
 		t.Error("node 'b' should not exist after removeNode")
 	}
-
-	// ...but edges from "b" may still be present in the edges map (this is a bug).
-	// We test this to document the behavior.
-	if g.edges != nil {
-		if targets, ok := g.edges["b"]; ok && len(targets) > 0 {
-			t.Log("BUG CONFIRMED: removeNode does not clean up outgoing edges. " +
-				"Edges from removed node 'b' still exist in adjacency list.")
-		}
+	if targets, ok := g.edges["b"]; ok && len(targets) > 0 {
+		t.Errorf("outgoing edges from removed node 'b' should be cleared, got %v", targets)
+	}
+	if g.areConnected("a", "b") {
+		t.Error("incoming edge 'a' -> 'b' should be removed when 'b' is removed")
 	}
 }