fix(comment): bound errored-project rendering to prevent comment-generation hang (#21)

* fix(comment): bound errored-project rendering to prevent comment-generation hang

formatErroredProject built its output with `s += ...` while splitting the
diagnostic message on ": " and applying a per-piece growing indent via
strings.Repeat. A large critical diagnostic (e.g. a multi-MB Terraform HCL
parse error, which also contains many ": " separators) turned this into an
O(n^2) string build, spinning a single goroutine on-CPU indefinitely. This
runs inside GenerateComment, before renderWithTruncation, so the 64KB comment
cap never kicked in.

Fixes:
- formatErroredProject uses strings.Builder, caps each diagnostic message to
  4KB before splitting, caps indent depth, and caps the number of critical
  diagnostics rendered per project (10) with a summarised remainder.
- processProjectCostDetails caps the combined errored-projects section (32KB),
  since the whole section is built before truncation.

Adds a regression test that hangs on the old implementation and completes
instantly with the fix.

* fix(comment): use fmt.Fprintf to satisfy staticcheck QF1012

Author: liamg

pkg/vcs/comment/cost_details.go

@@ -87,10 +87,26 @@ func (data *Data) processProjectCostDetails(inputs *Inputs) {
 	}
 
 	if len(erroredProjects) > 0 {
-		for _, project := range erroredProjects {
-			parts = append(parts, projectTitle(project))
-			parts = append(parts, formatErroredProject(project))
-			parts = append(parts, "\n"+separator)
+		var erroredBytes int
+		var omitted int
+		for i, project := range erroredProjects {
+			// Defence in depth: even with each message individually capped, a run
+			// with many errored projects could produce a huge section that the
+			// truncation pass then has to churn through. Stop once the section
+			// exceeds its budget and note how many projects were left out.
+			if erroredBytes > maxErroredProjectsSectionBytes {
+				omitted = len(erroredProjects) - i
+				break
+			}
+			title := projectTitle(project)
+			body := formatErroredProject(project)
+			tail := "\n" + separator
+			erroredBytes += len(title) + len(body) + len(tail)
+			parts = append(parts, title, body, tail)
+		}
+		if omitted > 0 {
+			parts = append(parts, fmt.Sprintf("… and %d more project(s) with errors (output truncated).\n", omitted))
+			parts = append(parts, separator)
 		}
 	}
 
@@ -413,23 +429,65 @@ func formatResourceCounts(counts map[string]int) string {
 	return msg
 }
 
+const (
+	// maxErroredDiagnosticBytes caps a single critical diagnostic message before
+	// it is rendered. Diagnostic errors (e.g. a Terraform HCL parse failure) can
+	// be many megabytes; without a cap, formatErroredProject's per-piece
+	// rendering degrades to O(n^2) and comment generation stalls indefinitely.
+	maxErroredDiagnosticBytes = 4096
+
+	// maxErroredIndentDepth caps the progressive indent applied to each ": "
+	// separated piece, so a message containing a large number of separators
+	// can't amplify into an enormous string.
+	maxErroredIndentDepth = 8
+
+	// maxErroredProjectsSectionBytes caps the combined size of the errored
+	// projects section. The whole section is built before renderWithTruncation
+	// runs, so this bounds the work regardless of how many projects errored.
+	maxErroredProjectsSectionBytes = 32 * 1024
+
+	// maxErroredDiagnosticsPerProject caps how many critical diagnostics are
+	// rendered for a single project; the rest are summarised as a count.
+	maxErroredDiagnosticsPerProject = 10
+)
+
 func formatErroredProject(pr ProjectResult) string {
-	s := "Errors:\n"
+	var b strings.Builder
+	b.WriteString("Errors:\n")
+	var shown int
+	var omitted int
 	for _, diag := range pr.Diagnostics {
 		if !diag.Critical {
 			continue
 		}
-		pieces := strings.Split(diag.FormatMessage(), ": ")
+		if shown >= maxErroredDiagnosticsPerProject {
+			omitted++
+			continue
+		}
+		shown++
+		msg := diag.FormatMessage()
+		if len(msg) > maxErroredDiagnosticBytes {
+			msg = prefixWithin(msg, maxErroredDiagnosticBytes, SizeUnitBytes) + "…"
+		}
+		pieces := strings.Split(msg, ": ")
 		for x, piece := range pieces {
-			s += strings.Repeat(" ", x+1) + piece
+			indent := x + 1
+			if indent > maxErroredIndentDepth {
+				indent = maxErroredIndentDepth
+			}
+			b.WriteString(strings.Repeat(" ", indent))
+			b.WriteString(piece)
 			if x == len(pieces)-1 {
-				s += "\n"
+				b.WriteString("\n")
 			} else {
-				s += ":\n"
+				b.WriteString(":\n")
 			}
 		}
 	}
-	return s
+	if omitted > 0 {
+		fmt.Fprintf(&b, " … and %d more error(s).\n", omitted)
+	}
+	return b.String()
 }
 
 func sortBreakdownResources(resources []BreakdownResource) {

pkg/vcs/comment/errored_project_test.go

@@ -0,0 +1,64 @@
+package comment
+
+import (
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/infracost/go-proto/pkg/diagnostic"
+	parserpb "github.com/infracost/proto/gen/go/infracost/parser"
+)
+
+// TestFormatErroredProject_Pathological guards against the O(n^2) blow-up that
+// caused comment generation to hang indefinitely: a single critical diagnostic
+// with a very large error string containing many ": " separators. Before the
+// fix this took effectively forever; it must now complete near-instantly and
+// stay bounded in size.
+func TestFormatErroredProject_Pathological(t *testing.T) {
+	huge := strings.Repeat("a: ", 2_000_000) // ~6MB, ~2M ": " separators
+
+	pr := ProjectResult{
+		Name: "big-error",
+		Diagnostics: []*diagnostic.Diagnostic{
+			{Critical: true, Type: parserpb.DiagnosticType_DIAGNOSTIC_TYPE_HCL_PARSE_ERROR, Error: huge},
+		},
+	}
+
+	done := make(chan string, 1)
+	go func() { done <- formatErroredProject(pr) }()
+
+	select {
+	case out := <-done:
+		// The single message is capped before rendering, so output stays a small
+		// bounded multiple of the cap (per-piece indent + ":\n" markers add a
+		// constant factor) rather than exploding with the ~6MB input.
+		if maxBytes := maxErroredDiagnosticBytes * 6; len(out) > maxBytes {
+			t.Fatalf("output not bounded: got %d bytes, want <= %d", len(out), maxBytes)
+		}
+	case <-time.After(5 * time.Second):
+		t.Fatal("formatErroredProject did not complete in time (quadratic blow-up regression)")
+	}
+}
+
+// TestFormatErroredProject_LimitsErrorCount verifies that only the first
+// maxErroredDiagnosticsPerProject critical diagnostics are rendered and the
+// remainder are summarised as a count.
+func TestFormatErroredProject_LimitsErrorCount(t *testing.T) {
+	var diags []*diagnostic.Diagnostic
+	for i := 0; i < maxErroredDiagnosticsPerProject+5; i++ {
+		diags = append(diags, &diagnostic.Diagnostic{
+			Critical: true,
+			Type:     parserpb.DiagnosticType_DIAGNOSTIC_TYPE_HCL_PARSE_ERROR,
+			Error:    "boom",
+		})
+	}
+	pr := ProjectResult{Name: "many-errors", Diagnostics: diags}
+
+	out := formatErroredProject(pr)
+	if !strings.Contains(out, "and 5 more error(s)") {
+		t.Fatalf("expected omitted-count summary, got:\n%s", out)
+	}
+	if got := strings.Count(out, "HCL parse error"); got != maxErroredDiagnosticsPerProject {
+		t.Fatalf("expected %d rendered diagnostics, got %d", maxErroredDiagnosticsPerProject, got)
+	}
+}