Reject spans that create a circular loop

src/Aspire.Dashboard/Otlp/Model/OtlpSpan.cs

@@ -42,7 +42,20 @@ public class OtlpSpan
     public TimeSpan Duration => EndTime - StartTime;
 
     public IEnumerable<OtlpSpan> GetChildSpans() => Trace.Spans.Where(s => s.ParentSpanId == SpanId);
-    public OtlpSpan? GetParentSpan() => string.IsNullOrEmpty(ParentSpanId) ? null : Trace.Spans.Where(s => s.SpanId == ParentSpanId).FirstOrDefault();
+    public OtlpSpan? GetParentSpan()
+    {
+        if (string.IsNullOrEmpty(ParentSpanId))
+        {
+            return null;
+        }
+
+        if (Trace.Spans.TryGetValue(ParentSpanId, out var span))
+        {
+            return span;
+        }
+
+        return null;
+    }
 
     public OtlpSpan(OtlpApplicationView applicationView, OtlpTrace trace, OtlpScope scope)
     {

src/Aspire.Dashboard/Otlp/Model/OtlpSpanCollection.cs

@@ -0,0 +1,27 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Collections.ObjectModel;
+
+namespace Aspire.Dashboard.Otlp.Model;
+
+public sealed class OtlpSpanCollection : KeyedCollection<string, OtlpSpan>
+{
+    // Chosen to balance memory usage overhead of dictionary vs work to find spans by ID.
+    private const int DictionaryCreationThreshold = 128;
+
+    public OtlpSpanCollection() : base(StringComparers.OtlpSpanId, DictionaryCreationThreshold)
+    {
+    }
+
+    protected override string GetKeyForItem(OtlpSpan item)
+    {
+        return item.SpanId;
+    }
+
+    public new List<OtlpSpan>.Enumerator GetEnumerator()
+    {
+        // Avoid allocating an enumerator when iterated with foreach.
+        return ((List<OtlpSpan>)this.Items).GetEnumerator();
+    }
+}

src/Aspire.Dashboard/Otlp/Model/OtlpTrace.cs

@@ -34,7 +34,7 @@ public TimeSpan Duration
         }
     }
 
-    public List<OtlpSpan> Spans { get; } = new List<OtlpSpan>();
+    public OtlpSpanCollection Spans { get; } = new OtlpSpanCollection();
 
     public int CalculateDepth(OtlpSpan span)
     {
@@ -52,6 +52,11 @@ public int CalculateDepth(OtlpSpan span)
 
     public void AddSpan(OtlpSpan span)
     {
+        if (Spans.Contains(span.SpanId))
+        {
+            throw new InvalidOperationException($"Duplicate span id '{span.SpanId}' detected.");
+        }
+
         var added = false;
         for (var i = Spans.Count - 1; i >= 0; i--)
         {
@@ -65,12 +70,12 @@ public void AddSpan(OtlpSpan span)
         if (!added)
         {
             Spans.Insert(0, span);
+        }
 
-            // If there isn't a root span then the first span is used as the trace name.
-            if (_rootSpan == null && !string.IsNullOrEmpty(span.ParentSpanId))
-            {
-                FullName = BuildFullName(span);
-            }
+        if (HasCircularReference(span))
+        {
+            Spans.Remove(span);
+            throw new InvalidOperationException($"Circular loop detected for span '{span.SpanId}' with parent '{span.ParentSpanId}'.");
         }
 
         if (string.IsNullOrEmpty(span.ParentSpanId))
@@ -87,6 +92,11 @@ public void AddSpan(OtlpSpan span)
                 }
             }
         }
+        else if (_rootSpan == null && span == Spans[0])
+        {
+            // If there isn't a root span then the first span is used as the trace name.
+            FullName = BuildFullName(span);
+        }
 
         AssertSpanOrder();
 
@@ -96,6 +106,31 @@ static string BuildFullName(OtlpSpan existingSpan)
         }
     }
 
+    private static bool HasCircularReference(OtlpSpan span)
+    {
+        // Can't have a circular reference if the span has no parent.
+        if (string.IsNullOrEmpty(span.ParentSpanId))
+        {
+            return false;
+        }
+
+        // Walk up span ancestors to check there is no loop.
+        var stack = new OtlpSpanCollection { span };
+        var currentSpan = span;
+        while (currentSpan.GetParentSpan() is { } parentSpan)
+        {
+            if (stack.Contains(parentSpan))
+            {
+                return true;
+            }
+
+            stack.Add(parentSpan);
+            currentSpan = parentSpan;
+        }
+
+        return false;
+    }
+
     [Conditional("DEBUG")]
     private void AssertSpanOrder()
     {

src/Shared/StringComparers.cs

@@ -23,6 +23,7 @@ internal static class StringComparers
     public static StringComparer GridColumn => StringComparer.Ordinal;
     public static StringComparer OtlpAttribute => StringComparer.Ordinal;
     public static StringComparer OtlpFieldValue => StringComparer.OrdinalIgnoreCase;
+    public static StringComparer OtlpSpanId => StringComparer.Ordinal;
 }
 
 internal static class StringComparisons
@@ -43,4 +44,5 @@ internal static class StringComparisons
     public static StringComparison GridColumn => StringComparison.Ordinal;
     public static StringComparison OtlpAttribute => StringComparison.Ordinal;
     public static StringComparison OtlpFieldValue => StringComparison.OrdinalIgnoreCase;
+    public static StringComparison OtlpSpanId => StringComparison.Ordinal;
 }

tests/Aspire.Dashboard.Tests/TelemetryRepositoryTests/TraceTests.cs

@@ -8,6 +8,8 @@
 using Aspire.Dashboard.Otlp.Storage;
 using Google.Protobuf;
 using Google.Protobuf.Collections;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging.Testing;
 using OpenTelemetry.Proto.Common.V1;
 using OpenTelemetry.Proto.Trace.V1;
 using Xunit;
@@ -90,6 +92,172 @@ public void AddTraces()
             });
     }
 
+    [Fact]
+    public void AddTraces_SelfParent_Reject()
+    {
+        // Arrange
+        var testSink = new TestSink();
+        var factory = LoggerFactory.Create(b => b.AddProvider(new TestLoggerProvider(testSink)));
+
+        var repository = CreateRepository(loggerFactory: factory);
+
+        // Act
+        var addContext = new AddContext();
+        repository.AddTraces(addContext, new RepeatedField<ResourceSpans>()
+        {
+            new ResourceSpans
+            {
+                Resource = CreateResource(),
+                ScopeSpans =
+                {
+                    new ScopeSpans
+                    {
+                        Scope = CreateScope(),
+                        Spans =
+                        {
+                            CreateSpan(traceId: "1", spanId: "1-1", startTime: s_testTime.AddMinutes(1), endTime: s_testTime.AddMinutes(10), parentSpanId: "1-1")
+                        }
+                    }
+                }
+            }
+        });
+
+        // Assert
+        Assert.Equal(1, addContext.FailureCount);
+
+        var applications = repository.GetApplications();
+        Assert.Collection(applications,
+            app =>
+            {
+                Assert.Equal("TestService", app.ApplicationName);
+                Assert.Equal("TestId", app.InstanceId);
+            });
+
+        var traces = repository.GetTraces(new GetTracesRequest
+        {
+            ApplicationKey = applications[0].ApplicationKey,
+            FilterText = string.Empty,
+            StartIndex = 0,
+            Count = 10,
+            Filters = []
+        });
+        Assert.Empty(traces.PagedResult.Items);
+
+        var write = Assert.Single(testSink.Writes);
+        Assert.Equal("Error adding span.", write.Message);
+        Assert.Equal("Circular loop detected for span '312d31' with parent '312d31'.", write.Exception!.Message);
+    }
+
+    [Fact]
+    public void AddTraces_MultipleSpansLoop_Reject()
+    {
+        // Arrange
+        var repository = CreateRepository();
+
+        // Act
+        var addContext = new AddContext();
+        repository.AddTraces(addContext, new RepeatedField<ResourceSpans>()
+        {
+            new ResourceSpans
+            {
+                Resource = CreateResource(),
+                ScopeSpans =
+                {
+                    new ScopeSpans
+                    {
+                        Scope = CreateScope(),
+                        Spans =
+                        {
+                            CreateSpan(traceId: "1", spanId: "1-1", startTime: s_testTime.AddMinutes(1), endTime: s_testTime.AddMinutes(10), parentSpanId: "1-3"),
+                            CreateSpan(traceId: "1", spanId: "1-2", startTime: s_testTime.AddMinutes(5), endTime: s_testTime.AddMinutes(10), parentSpanId: "1-1"),
+                            CreateSpan(traceId: "1", spanId: "1-3", startTime: s_testTime.AddMinutes(5), endTime: s_testTime.AddMinutes(10), parentSpanId: "1-2")
+                        }
+                    }
+                }
+            }
+        });
+
+        // Assert
+        Assert.Equal(1, addContext.FailureCount);
+
+        var applications = repository.GetApplications();
+        Assert.Collection(applications,
+            app =>
+            {
+                Assert.Equal("TestService", app.ApplicationName);
+                Assert.Equal("TestId", app.InstanceId);
+            });
+
+        var traces = repository.GetTraces(new GetTracesRequest
+        {
+            ApplicationKey = applications[0].ApplicationKey,
+            FilterText = string.Empty,
+            StartIndex = 0,
+            Count = 10,
+            Filters = []
+        });
+        Assert.Collection(traces.PagedResult.Items,
+            trace =>
+            {
+                Assert.Equal(2, trace.Spans.Count);
+            });
+    }
+
+    [Fact]
+    public void AddTraces_DuplicateTraceIds_Reject()
+    {
+        // Arrange
+        var repository = CreateRepository();
+
+        // Act
+        var addContext = new AddContext();
+        repository.AddTraces(addContext, new RepeatedField<ResourceSpans>()
+        {
+            new ResourceSpans
+            {
+                Resource = CreateResource(),
+                ScopeSpans =
+                {
+                    new ScopeSpans
+                    {
+                        Scope = CreateScope(),
+                        Spans =
+                        {
+                            CreateSpan(traceId: "1", spanId: "1-1", startTime: s_testTime.AddMinutes(1), endTime: s_testTime.AddMinutes(10)),
+                            CreateSpan(traceId: "1", spanId: "1-1", startTime: s_testTime.AddMinutes(5), endTime: s_testTime.AddMinutes(10)),
+                            CreateSpan(traceId: "1", spanId: "1-2", startTime: s_testTime.AddMinutes(1), endTime: s_testTime.AddMinutes(10), parentSpanId: "1-1"),
+                        }
+                    }
+                }
+            }
+        });
+
+        // Assert
+        Assert.Equal(1, addContext.FailureCount);
+
+        var applications = repository.GetApplications();
+        Assert.Collection(applications,
+            app =>
+            {
+                Assert.Equal("TestService", app.ApplicationName);
+                Assert.Equal("TestId", app.InstanceId);
+            });
+
+        var traces = repository.GetTraces(new GetTracesRequest
+        {
+            ApplicationKey = applications[0].ApplicationKey,
+            FilterText = string.Empty,
+            StartIndex = 0,
+            Count = 10,
+            Filters = []
+        });
+        Assert.Collection(traces.PagedResult.Items,
+            trace =>
+            {
+                Assert.Equal(2, trace.Spans.Count);
+            });
+    }
+
     [Fact]
     public void AddTraces_Scope_Multiple()
     {