fix(dashboard): truncate all caller-controlled span fields, wire max-spans-per-trace, and close the untested retention gaps

Extends the truncation fix to http.path, http.host, and the span Name
(all three were still unbounded and the path was stored twice per span).
Wires TraceMaxSpansPerTrace through Config/MemoryProfile into the trace
store constructor, so the per-trace cap is finally reachable outside
tests. Adds an integration test that drives the real TracingMiddleware
and inspects the stored span, and a test pinning down that trace
retention keeps evicting even while a saturated trace makes every
insert on it fail.

Author: juicycleff

extensions/dashboard/collector/trace_store_test.go

@@ -290,6 +290,84 @@ func TestTraceStore_TTLGateOpensOnAccessAndLapses(t *testing.T) {
 	}
 }
 
+// TestTraceStore_EvictsExpiredTraceEvenWhileAnotherTraceIsSaturated pins down
+// the guarantee documented on the ts.evict() call inside AddSpan's
+// cap-exceeded branch: retention must not become conditional on a successful
+// insert. Without that call, a store whose only traffic is one saturated
+// trace ("hot") would never call evict() again — every further AddSpan on
+// "hot" takes the early-return cap-rejection branch — so an unrelated expired
+// trace ("old") would sit in memory forever instead of aging out.
+//
+// "old" is seeded directly into the store's internals (this test lives in
+// package collector, so it can) rather than through AddSpan, for two reasons.
+// First, the span() helper always stamps StartTime with time.Now(), so an
+// already-expired span has to be built by hand. Second, and more subtly, if
+// "old" were seeded before "hot" existed, the unconditional ts.evict() call
+// at the end of AddSpan's normal (non-cap) path — the one that runs on every
+// successful insert, not only the cap branch — would reclaim it the moment
+// "hot"'s first (still normal-path) span landed, long before "hot" ever
+// saturates. So "hot" is saturated first, while "old" does not exist yet,
+// and "old" is only seeded afterward, bypassing AddSpan entirely so its
+// insertion triggers no evict() call of its own. It is placed ahead of
+// "hot" in eviction order, because evict() walks oldest-first and stops at
+// the first non-expired trace — "hot" is fresh, so if it came first, evict()
+// would never even look at "old".
+func TestTraceStore_EvictsExpiredTraceEvenWhileAnotherTraceIsSaturated(t *testing.T) {
+	const retention = 200 * time.Millisecond
+	const spanCap = 3
+
+	ts := NewTraceStore(10, retention, WithMaxSpansPerTrace(spanCap))
+
+	// Saturate "hot" first, before "old" exists, so nothing here can
+	// accidentally evict "old".
+	for i := 0; i < spanCap; i++ {
+		ts.AddSpan(span("hot", "span"))
+	}
+
+	ts.mu.RLock()
+	hotCount := len(ts.traces["hot"])
+	ts.mu.RUnlock()
+	if hotCount != spanCap {
+		t.Fatalf("trace \"hot\" has %d spans after saturating, want %d", hotCount, spanCap)
+	}
+
+	expiredStart := time.Now().Add(-time.Hour)
+	oldSpan := &SpanView{
+		SpanID:     "span",
+		TraceID:    "old",
+		Name:       "GET /x",
+		Kind:       SpanKindServer,
+		Status:     SpanStatusOK,
+		StartTime:  expiredStart,
+		EndTime:    expiredStart.Add(time.Millisecond),
+		Duration:   time.Millisecond,
+		Attributes: map[string]string{},
+		Events:     []SpanEventView{},
+	}
+	ts.mu.Lock()
+	ts.traces["old"] = []*SpanView{oldSpan}
+	ts.order = append([]string{"old"}, ts.order...)
+	ts.mu.Unlock()
+
+	// From here, every AddSpan on "hot" takes the cap-rejection branch
+	// exclusively (its span count is already at the cap and no insert can
+	// ever succeed again). That branch's evict() call is the only thing
+	// that can still reclaim "old".
+	for i := 0; i < 20; i++ {
+		ts.AddSpan(span("hot", "span"))
+
+		ts.mu.RLock()
+		_, stillPresent := ts.traces["old"]
+		ts.mu.RUnlock()
+
+		if !stillPresent {
+			return // "old" was reclaimed: the guarantee holds.
+		}
+	}
+
+	t.Error("trace \"old\" was never evicted while \"hot\" stayed saturated at its cap")
+}
+
 // MarkAccessed runs on the request path alongside AddSpan. It must not race.
 func TestTraceStore_MarkAccessedIsRaceFree(t *testing.T) {
 	ts := NewTraceStore(100, time.Hour)

extensions/dashboard/config.go

@@ -58,6 +58,9 @@ type Config struct {
 	// Tracing
 	TraceMaxCount  int           `json:"trace_max_count"  yaml:"trace_max_count"`
 	TraceRetention time.Duration `json:"trace_retention"  yaml:"trace_retention"`
+	// TraceMaxSpansPerTrace caps how many spans one trace retains. A single
+	// long-lived trace, such as a websocket, would otherwise grow unbounded.
+	TraceMaxSpansPerTrace int `json:"trace_max_spans_per_trace" yaml:"trace_max_spans_per_trace"`
 	// TraceIdleTTL is how long after the last dashboard request spans keep being
 	// retained. A negative duration disables the gate, retaining always (the
 	// pre-gate behaviour). Zero is not a reliable way to disable it: under a
@@ -126,9 +129,10 @@ func DefaultConfig() Config {
 		HistoryDuration: 30 * time.Minute,
 		MaxDataPoints:   120,
 
-		TraceMaxCount:  200,
-		TraceRetention: 30 * time.Minute,
-		TraceIdleTTL:   5 * time.Minute,
+		TraceMaxCount:         200,
+		TraceRetention:        30 * time.Minute,
+		TraceMaxSpansPerTrace: 200,
+		TraceIdleTTL:          5 * time.Minute,
 
 		ProxyTimeout: 10 * time.Second,
 		CacheMaxSize: 100,
@@ -279,6 +283,11 @@ func WithTraceRetention(duration time.Duration) ConfigOption {
 	return func(c *Config) { c.TraceRetention = duration }
 }
 
+// WithTraceMaxSpansPerTrace sets how many spans a single trace retains.
+func WithTraceMaxSpansPerTrace(n int) ConfigOption {
+	return func(c *Config) { c.TraceMaxSpansPerTrace = n }
+}
+
 // WithTraceIdleTTL sets how long after the last dashboard request traces keep
 // being collected. Pass a negative duration to collect always, disabling the
 // gate. Passing zero is not reliable for this: under a ConfigManager, config
@@ -391,20 +400,23 @@ func WithMemoryProfile(profile MemoryProfile) ConfigOption {
 			c.HistoryDuration = 15 * time.Minute
 			c.TraceMaxCount = 50
 			c.TraceRetention = 10 * time.Minute
+			c.TraceMaxSpansPerTrace = 50
 			c.TraceIdleTTL = 1 * time.Minute
 			c.CacheMaxSize = 50
 		case MemoryProfileHigh:
 			c.MaxDataPoints = 500
 			c.HistoryDuration = 1 * time.Hour
 			c.TraceMaxCount = 1000
 			c.TraceRetention = 1 * time.Hour
+			c.TraceMaxSpansPerTrace = 500
 			c.TraceIdleTTL = 15 * time.Minute
 			c.CacheMaxSize = 500
 		default: // medium — same as DefaultConfig
 			c.MaxDataPoints = 120
 			c.HistoryDuration = 30 * time.Minute
 			c.TraceMaxCount = 200
 			c.TraceRetention = 30 * time.Minute
+			c.TraceMaxSpansPerTrace = 200
 			c.TraceIdleTTL = 5 * time.Minute
 			c.CacheMaxSize = 100
 		}

extensions/dashboard/extension.go

@@ -213,7 +213,7 @@ func (e *Extension) Register(app forge.App) error {
 	e.collector.SetCacheTTL(e.config.RefreshInterval)
 
 	// Initialize trace store for dashboard tracing UI
-	e.traceStore = collector.NewTraceStore(e.config.TraceMaxCount, e.config.TraceRetention)
+	e.traceStore = collector.NewTraceStore(e.config.TraceMaxCount, e.config.TraceRetention, collector.WithMaxSpansPerTrace(e.config.TraceMaxSpansPerTrace))
 
 	// Initialize SSE broker (if real-time is enabled)
 	if e.config.EnableRealtime {

extensions/dashboard/tracing_middleware.go

@@ -109,11 +109,14 @@ func TracingMiddleware(store *collector.TraceStore, basePath string) forge.Middl
 				status = collector.SpanStatusError
 			}
 
-			// Build attributes.
+			// Build attributes. http.path and http.host are caller-controlled
+			// and unbounded (Go accepts a request line up to MaxHeaderBytes+4096,
+			// about 1MB by default), so they are truncated at the point of
+			// storage just like the query and user-agent attributes below.
 			attrs := map[string]string{
 				"http.method": req.Method,
-				"http.path":   path,
-				"http.host":   req.Host,
+				"http.path":   truncateAttr(req.URL.Path, maxAttrValueLen),
+				"http.host":   truncateAttr(req.Host, maxAttrValueLen),
 				"protocol":    protocol,
 			}
 			if req.URL.RawQuery != "" {
@@ -129,7 +132,7 @@ func TracingMiddleware(store *collector.TraceStore, basePath string) forge.Middl
 			span := &collector.SpanView{
 				SpanID:     spanID,
 				TraceID:    traceID,
-				Name:       req.Method + " " + path,
+				Name:       truncateAttr(req.Method+" "+path, maxAttrValueLen),
 				Kind:       collector.SpanKindServer,
 				Status:     status,
 				StartTime:  start,

extensions/dashboard/tracing_middleware_test.go

@@ -143,3 +143,78 @@ func TestTracingMiddleware_StampsAccessOnDashboardRequests(t *testing.T) {
 		})
 	}
 }
+
+// TestTracingMiddleware_TruncatesStoredAttributes drives the real
+// TracingMiddleware end to end and inspects what actually landed in the
+// TraceStore. Unit tests on truncateAttr alone cannot see this: the reviewer
+// deleted every truncateAttr call site inside TracingMiddleware and the
+// truncateAttr unit tests, plus the rest of the suite, stayed green — nothing
+// exercised the middleware's own attribute construction. This test also
+// covers the http.path, http.host, and span Name fields, which were never
+// truncated at all: all three are caller-controlled (a request line can run
+// to about 1MB by default) and the path was being stored twice per span,
+// unbounded, for the whole retention window.
+func TestTracingMiddleware_TruncatesStoredAttributes(t *testing.T) {
+	const basePath = "/dashboard"
+
+	store := collector.NewTraceStore(10, time.Hour)
+	// No ingest gate installed: a store with no gate retains everything. If a
+	// gate were installed here and left closed, AddSpan would silently discard
+	// the span and every assertion below would pass vacuously.
+
+	noopNext := func(ctx forge.Context) error { return nil }
+
+	longQuery := strings.Repeat("q", 5000)
+	longUA := strings.Repeat("a", 5000)
+	longPath := "/" + strings.Repeat("p", 5000)
+	longHost := strings.Repeat("h", 5000) + ".example.com"
+
+	req := httptest.NewRequest("GET", longPath+"?"+longQuery, nil)
+	req.Host = longHost
+	req.Header.Set("User-Agent", longUA)
+	w := httptest.NewRecorder()
+	ctx := forge_http.NewContext(w, req, nil)
+
+	middleware := TracingMiddleware(store, basePath)
+	handler := middleware(noopNext)
+
+	if err := handler(ctx); err != nil {
+		t.Fatalf("unexpected error handling request: %v", err)
+	}
+
+	// The middleware picks its own trace ID from time.Now(), so recover it by
+	// listing the store rather than guessing it.
+	summaries, _ := store.ListTraces(collector.TraceFilter{})
+	if len(summaries) != 1 {
+		t.Fatalf("store has %d traces after one traced request, want 1", len(summaries))
+	}
+	traceID := summaries[0].TraceID
+
+	detail := store.GetTrace(traceID)
+	if detail == nil {
+		t.Fatalf("GetTrace(%q) returned nil", traceID)
+	}
+	if len(detail.Spans) != 1 {
+		t.Fatalf("trace has %d spans, want 1", len(detail.Spans))
+	}
+
+	span := detail.Spans[0]
+
+	if len(span.Name) > maxAttrValueLen {
+		t.Errorf("stored span Name is %d bytes, want at most %d", len(span.Name), maxAttrValueLen)
+	}
+
+	for key, val := range span.Attributes {
+		if len(val) > maxAttrValueLen {
+			t.Errorf("stored attribute %q is %d bytes, want at most %d", key, len(val), maxAttrValueLen)
+		}
+	}
+
+	// Spot-check the two attributes the earlier review found unbounded.
+	if got := len(span.Attributes["http.path"]); got > maxAttrValueLen {
+		t.Errorf("http.path attribute is %d bytes, want at most %d", got, maxAttrValueLen)
+	}
+	if got := len(span.Attributes["http.host"]); got > maxAttrValueLen {
+		t.Errorf("http.host attribute is %d bytes, want at most %d", got, maxAttrValueLen)
+	}
+}