Add MessageMerger edge cases and test coverage documentation

Agent-Logs-Url: https://github.com/microsoft/agent-framework/sessions/789cdb87-c90d-4681-b8e0-9b6b63762856 Co-authored-by: lokitoth <6936551+lokitoth@users.noreply.github.com>
2026-06-16 21:04:09 +08:00 · 2026-05-13 19:18:49 +00:00
parent 23312ee8b1
commit b684a441c0
1 changed files with 213 additions and 0 deletions
@@ -0,0 +1,213 @@
+# MessageMerger Edge Cases and Test Coverage
+
+This document identifies edge cases in `MessageMerger` and recommends additional test coverage.
+
+## Overview
+
+`MessageMerger` (`dotnet/src/Microsoft.Agents.AI.Hosting.AzureFunctions/MessageMerger.cs`) handles merging streaming `AgentResponseUpdate` messages into a final `AgentResponse`. It groups updates by `ResponseId`, then by `MessageId`, sorts messages by `CreatedAt` timestamp, and produces a consolidated response.
+
+---
+
+## Edge Cases Identified
+
+| # | Edge Case | Risk | Current Test Coverage |
+|---|-----------|------|----------------------|
+| 1 | **Non-transitive timestamp comparison** — mixed timestamped/untimestamped updates with 3+ messages can produce inconsistent sort order | High | ❌ Not covered |
+| 2 | **Cross-ResponseId ordering** — messages from different ResponseIds are emitted in first-seen order, not chronological | Medium | ❌ Not covered |
+| 3 | **ResponseId=null updates always last** — dangling updates appended after all response-scoped messages regardless of arrival time | Medium | ❌ Not covered |
+| 4 | **MessageId=null updates within a response** — keyed messages always precede message-id-less updates | Low | ❌ Not covered |
+| 5 | **CreatedAt overwritten** — all messages in a merged response get same `CreatedAt`, erasing original timestamps | Low | Partial (test asserts message timestamps) |
+| 6 | **Dangling metadata lost** — `FinishReason`, `Usage`, `AgentId` from ResponseId=null updates not merged | Medium | ❌ Not covered |
+| 7 | **Unused `createdTimes` collection** — final response uses `UtcNow`, collected times are unused | Low (code smell) | N/A |
+
+---
+
+## Edge Case Details
+
+### 1. Non-transitive Timestamp Comparison (High Risk)
+
+**Problem:** The `OrderBy` lambda uses a comparison that isn't transitive when some messages have `CreatedAt` and others don't:
+
+```csharp
+.OrderBy(kvp => kvp.Value.CreatedAt ?? m.CreatedAt)
+```
+
+With three messages:
+- A: `CreatedAt = 10`, idx=0
+- B: `CreatedAt = null`, idx=1
+- C: `CreatedAt = 5`, idx=2
+
+The comparison produces: A < B (10 < 10=false, so equal), B < C (10 < 5=false), but A > C (10 > 5). This violates transitivity and can cause non-deterministic sort results.
+
+**Recommendation:** Use insertion order as fallback for null timestamps, or store original index.
+
+---
+
+### 2. Cross-ResponseId Ordering (Medium Risk)
+
+**Problem:** When multiple `ResponseId`s arrive interleaved, messages are emitted in response-first-seen order:
+
+```
+Updates: R1-msg1 → R2-msg1 → R1-msg2 → R2-msg2
+Output:  [R1-msg1, R1-msg2], [R2-msg1, R2-msg2]
+```
+
+This may not match chronological arrival order.
+
+---
+
+### 3. ResponseId=null Updates Always Last (Medium Risk)
+
+**Problem:** Updates with `ResponseId = null` are grouped as "dangling" and always appended last, regardless of when they arrived:
+
+```csharp
+if (update.ResponseId is null)
+{
+    danglingUpdates.Add(update);
+    continue;
+}
+```
+
+This means metadata-only updates sent early in the stream appear at the end.
+
+---
+
+### 4. MessageId=null Updates Within a Response (Low Risk)
+
+**Problem:** Within a `ResponseId` group, updates with `MessageId = null` are stored in a separate list and appended after keyed messages:
+
+```csharp
+if (update.MessageId is null)
+{
+    grouping.Value.noIds.Add(update);
+}
+```
+
+This is likely intentional but not documented or tested.
+
+---
+
+### 5. CreatedAt Overwritten (Low Risk)
+
+**Problem:** All messages in the final response receive the same `CreatedAt` timestamp:
+
+```csharp
+message.CreatedAt = m.CreatedAt ?? DateTimeOffset.UtcNow;
+```
+
+Individual message timestamps are overwritten with the response-level timestamp or current time.
+
+---
+
+### 6. Dangling Metadata Lost (Medium Risk)
+
+**Problem:** When `ResponseId = null` updates contain `FinishReason`, `Usage`, or `AgentId`, these values are not merged into the final response:
+
+```csharp
+// Dangling updates become orphan messages, not merged with response metadata
+foreach (var orphan in danglingUpdates)
+{
+    orphanMessages.Add(CreateAgentMessage(orphan));
+}
+```
+
+---
+
+### 7. Unused `createdTimes` Collection (Low Risk - Code Smell)
+
+**Problem:** The code collects `CreatedAt` values into `createdTimes` list but never uses them:
+
+```csharp
+List<DateTimeOffset> createdTimes = [];
+// ... later ...
+if (update.CreatedAt.HasValue) createdTimes.Add(update.CreatedAt.Value);
+// createdTimes is never used
+```
+
+---
+
+## Recommended Additional Tests
+
+### High Priority
+
+1. **Non-transitive sorting with mixed timestamps**
+   ```csharp
+   [Fact]
+   public void MergeMessages_MixedTimestamps_ProducesStableOrder()
+   {
+       // Arrange: 3 messages - A (CreatedAt=10), B (null), C (CreatedAt=5)
+       // Act: Merge
+       // Assert: Order is deterministic (either chronological or insertion order)
+   }
+   ```
+
+2. **Function call/result sequencing with 3+ messages**
+   ```csharp
+   [Fact]
+   public void MergeMessages_FunctionCallAndResultsWithMixedTimestamps_PreservesLogicalOrder()
+   {
+       // Arrange: FunctionCall (null), FunctionResult (T1), Assistant (null)
+       // Act: Merge
+       // Assert: FunctionCall precedes FunctionResult precedes Assistant
+   }
+   ```
+
+### Medium Priority
+
+3. **Multiple ResponseIds interleaved**
+   ```csharp
+   [Fact]
+   public void MergeMessages_InterleavedResponseIds_GroupsByResponseId()
+   {
+       // Arrange: R1-msg1, R2-msg1, R1-msg2, R2-msg2
+       // Act: Merge
+       // Assert: Messages grouped by ResponseId, verify order
+   }
+   ```
+
+4. **Dangling updates with FinishReason/Usage**
+   ```csharp
+   [Fact]
+   public void MergeMessages_DanglingUpdatesWithMetadata_MetadataPropagates()
+   {
+       // Arrange: ResponseId=null update with FinishReason=Stop, Usage=(10,20,30)
+       // Act: Merge
+       // Assert: Final response contains FinishReason and Usage
+   }
+   ```
+
+5. **ResponseId=null timing**
+   ```csharp
+   [Fact]
+   public void MergeMessages_DanglingUpdatesFirst_AppearsAfterKeyedMessages()
+   {
+       // Arrange: null-response update, then keyed update
+       // Act: Merge
+       // Assert: Keyed messages appear before dangling
+   }
+   ```
+
+### Low Priority
+
+6. **MessageId=null updates ordering**
+   ```csharp
+   [Fact]
+   public void MergeMessages_MixedMessageIds_KeyedBeforeUnkeyed()
+   {
+       // Arrange: Mix of keyed and unkeyed updates within same ResponseId
+       // Act: Merge
+       // Assert: Keyed messages precede unkeyed in arrival order
+   }
+   ```
+
+---
+
+## Summary
+
+| Priority | Test Count | Risk Addressed |
+|----------|------------|----------------|
+| High | 2 | Non-deterministic sorting, Function sequencing |
+| Medium | 3 | ResponseId grouping, Metadata propagation, Timing |
+| Low | 1 | MessageId ordering |
+
+**Recommendation:** Add at minimum the 2 high-priority tests and 2 medium-priority tests (interleaved ResponseIds, dangling metadata) to ensure `MessageMerger` behaves correctly in production streaming scenarios.