Skip to content

Commit 9a50e7b

Browse files
fix: resolve streaming responseId duplication and ownership gaps
- AIChatService: remove responseId yield from StreamingResponseCompletedUpdate; StreamingResponseCreatedUpdate already emits the ID early at the start of each API call leg, making the CompletedUpdate emit redundant and causing duplicate SSE events (one per leg) when tool calls are involved - AIChatService: drop now-unnecessary per-iteration string? responseId local variable and the dead esponseId = functionResult.responseId assignment inside the tool-call foreach loop - ChatController: remove esponseIdRecorded one-shot flag; instead record ownership for every non-null responseId yielded — one per response leg (initial + each tool-call continuation) — so tool-call continuation IDs are properly bound to the authenticated user before being forwarded to the client - ChatController: add null-guard for userId after FindFirstValue(); [Authorize] + ASP.NET Identity guarantees the claim is present but the explicit check returns 401 early rather than silently propagating a null into validation - ResponseIdValidationServiceTests: extract CreateCache() helper with SizeLimit = 10_000 matching production config, so SetSize(1) per entry is actually exercised in tests rather than silently ignored; add new RecordResponseId_SizeLimitEnforced_EntryCountedInCache test
1 parent f40cce1 commit 9a50e7b

3 files changed

Lines changed: 43 additions & 27 deletions

File tree

EssentialCSharp.Chat.Shared/Services/AIChatService.cs

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -190,14 +190,12 @@ private static string SanitizeForXmlContext(string? input) =>
190190
{
191191
await foreach (var update in streamingUpdates.WithCancellation(cancellationToken))
192192
{
193-
string? responseId;
194193
#pragma warning disable OPENAI001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
195194
if (update is StreamingResponseCreatedUpdate created)
196195
{
197196
// Emit the response ID early so the controller can record ownership
198197
// before the stream completes — handles client disconnects mid-stream.
199-
responseId = created.Response.Id;
200-
yield return (string.Empty, responseId: responseId);
198+
yield return (string.Empty, responseId: created.Response.Id);
201199
}
202200
else if (update is StreamingResponseOutputItemDoneUpdate itemDone)
203201
{
@@ -207,13 +205,11 @@ private static string SanitizeForXmlContext(string? input) =>
207205
if (toolCallDepth >= 10)
208206
throw new InvalidOperationException("Maximum tool call depth exceeded.");
209207

210-
// Execute the function call and stream its response
208+
// Execute the function call and stream its response.
209+
// Each nested ProcessStreamingUpdatesAsync emits its own StreamingResponseCreatedUpdate
210+
// (which already yields its responseId early), so no extra tracking is needed here.
211211
await foreach (var functionResult in ExecuteFunctionCallAsync(functionCallItem, responseOptions, mcpClient, toolCallDepth + 1, endUserId, cancellationToken))
212212
{
213-
if (functionResult.responseId != null)
214-
{
215-
responseId = functionResult.responseId;
216-
}
217213
yield return functionResult;
218214
}
219215
}
@@ -222,9 +218,10 @@ private static string SanitizeForXmlContext(string? input) =>
222218
{
223219
yield return (deltaUpdate.Delta.ToString(), null);
224220
}
225-
else if (update is StreamingResponseCompletedUpdate completedUpdate)
221+
else if (update is StreamingResponseCompletedUpdate)
226222
{
227-
yield return (string.Empty, responseId: completedUpdate.Response.Id); // Signal completion with response ID
223+
// ResponseId was already emitted from StreamingResponseCreatedUpdate at the start
224+
// of this response leg — no need to emit again from the completion event.
228225
}
229226
#pragma warning restore OPENAI001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
230227
}

EssentialCSharp.Web.Tests/ResponseIdValidationServiceTests.cs

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,17 @@ namespace EssentialCSharp.Web.Tests;
55

66
public class ResponseIdValidationServiceTests
77
{
8+
// Match production SizeLimit so SetSize(1) is exercised in tests, not silently ignored.
9+
private static MemoryCache CreateCache() => new(new MemoryCacheOptions { SizeLimit = 10_000 });
10+
811
private static ResponseIdValidationService CreateService(MemoryCache cache) => new(cache);
912

1013
[Test]
1114
[Arguments(null)]
1215
[Arguments("")]
1316
public async Task ValidateResponseId_BlankResponseId_AllowsNewConversation(string? responseId)
1417
{
15-
using var cache = new MemoryCache(new MemoryCacheOptions());
18+
using var cache = CreateCache();
1619
var service = CreateService(cache);
1720

1821
bool result = service.ValidateResponseId("user1", responseId);
@@ -25,7 +28,7 @@ public async Task ValidateResponseId_BlankResponseId_AllowsNewConversation(strin
2528
[Arguments("")]
2629
public async Task ValidateResponseId_BlankUserId_Rejects(string? userId)
2730
{
28-
using var cache = new MemoryCache(new MemoryCacheOptions());
31+
using var cache = CreateCache();
2932
var service = CreateService(cache);
3033

3134
bool result = service.ValidateResponseId(userId, "resp_123");
@@ -36,7 +39,7 @@ public async Task ValidateResponseId_BlankUserId_Rejects(string? userId)
3639
[Test]
3740
public async Task ValidateResponseId_CacheMiss_AllowsGracefulDegradation()
3841
{
39-
using var cache = new MemoryCache(new MemoryCacheOptions());
42+
using var cache = CreateCache();
4043
var service = CreateService(cache);
4144
// No RecordResponseId call — simulate server restart / different instance
4245

@@ -48,7 +51,7 @@ public async Task ValidateResponseId_CacheMiss_AllowsGracefulDegradation()
4851
[Test]
4952
public async Task ValidateResponseId_RecordedByOwner_Validates()
5053
{
51-
using var cache = new MemoryCache(new MemoryCacheOptions());
54+
using var cache = CreateCache();
5255
var service = CreateService(cache);
5356
service.RecordResponseId("user1", "resp_abc");
5457

@@ -60,7 +63,7 @@ public async Task ValidateResponseId_RecordedByOwner_Validates()
6063
[Test]
6164
public async Task ValidateResponseId_RecordedByDifferentUser_Rejects()
6265
{
63-
using var cache = new MemoryCache(new MemoryCacheOptions());
66+
using var cache = CreateCache();
6467
var service = CreateService(cache);
6568
service.RecordResponseId("user1", "resp_abc");
6669

@@ -72,7 +75,7 @@ public async Task ValidateResponseId_RecordedByDifferentUser_Rejects()
7275
[Test]
7376
public async Task RecordResponseId_NullInputs_DoesNotThrow()
7477
{
75-
using var cache = new MemoryCache(new MemoryCacheOptions());
78+
using var cache = CreateCache();
7679
var service = CreateService(cache);
7780

7881
service.RecordResponseId(null, "resp_abc");
@@ -87,7 +90,7 @@ public async Task RecordResponseId_NullInputs_DoesNotThrow()
8790
[Test]
8891
public async Task ValidateResponseId_MultipleResponseIds_EachValidatedIndependently()
8992
{
90-
using var cache = new MemoryCache(new MemoryCacheOptions());
93+
using var cache = CreateCache();
9194
var service = CreateService(cache);
9295
service.RecordResponseId("user1", "resp_001");
9396
service.RecordResponseId("user1", "resp_002");
@@ -101,7 +104,7 @@ public async Task ValidateResponseId_MultipleResponseIds_EachValidatedIndependen
101104
[Test]
102105
public async Task ValidateResponseId_TwoUsers_IsolatedFromEachOther()
103106
{
104-
using var cache = new MemoryCache(new MemoryCacheOptions());
107+
using var cache = CreateCache();
105108
var service = CreateService(cache);
106109
service.RecordResponseId("user1", "resp_A");
107110
service.RecordResponseId("user2", "resp_B");
@@ -111,4 +114,17 @@ public async Task ValidateResponseId_TwoUsers_IsolatedFromEachOther()
111114
await Assert.That(service.ValidateResponseId("user2", "resp_A")).IsFalse();
112115
await Assert.That(service.ValidateResponseId("user1", "resp_B")).IsFalse();
113116
}
117+
118+
[Test]
119+
public async Task RecordResponseId_SizeLimitEnforced_EntryCountedInCache()
120+
{
121+
using var cache = CreateCache();
122+
var service = CreateService(cache);
123+
124+
// Record an entry — with SizeLimit set, SetSize(1) should count toward the cache size.
125+
service.RecordResponseId("user1", "resp_size_test");
126+
127+
// Verify it was recorded (i.e., not silently evicted due to misconfiguration).
128+
await Assert.That(service.ValidateResponseId("user1", "resp_size_test")).IsTrue();
129+
}
114130
}

EssentialCSharp.Web/Controllers/ChatController.cs

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ public async Task<IActionResult> SendMessage([FromBody] ChatMessageRequest reque
3333
return BadRequest(new { error = "Message cannot be empty." });
3434

3535
var userId = User.FindFirstValue(ClaimTypes.NameIdentifier);
36+
if (string.IsNullOrEmpty(userId))
37+
return Unauthorized();
3638

3739
var previousResponseId = string.IsNullOrWhiteSpace(request.PreviousResponseId)
3840
? null
@@ -70,6 +72,12 @@ public async Task StreamMessage([FromBody] ChatMessageRequest request, Cancellat
7072
}
7173

7274
var userId = User.FindFirstValue(ClaimTypes.NameIdentifier);
75+
if (string.IsNullOrEmpty(userId))
76+
{
77+
Response.StatusCode = 401;
78+
await Response.WriteAsJsonAsync(new { error = "Unauthorized." }, CancellationToken.None);
79+
return;
80+
}
7381

7482
var previousResponseId = string.IsNullOrWhiteSpace(request.PreviousResponseId)
7583
? null
@@ -88,8 +96,6 @@ public async Task StreamMessage([FromBody] ChatMessageRequest request, Cancellat
8896

8997
try
9098
{
91-
bool responseIdRecorded = false;
92-
9399
await foreach (var (text, responseId) in _AiChatService.GetChatCompletionStream(
94100
prompt: request.Message,
95101
previousResponseId: previousResponseId,
@@ -106,13 +112,10 @@ public async Task StreamMessage([FromBody] ChatMessageRequest request, Cancellat
106112

107113
if (!string.IsNullOrEmpty(responseId))
108114
{
109-
// Record ownership as soon as the response ID is first observed —
110-
// if the client disconnects mid-stream the ownership is still cached.
111-
if (!responseIdRecorded)
112-
{
113-
_ResponseIdValidationService.RecordResponseId(userId, responseId);
114-
responseIdRecorded = true;
115-
}
115+
// Record ownership for every responseId emitted — one per API call leg
116+
// (initial request + each tool-call continuation). This ensures all leg IDs
117+
// are bound to the authenticated user before being forwarded to the client.
118+
_ResponseIdValidationService.RecordResponseId(userId, responseId);
116119
var eventData = JsonSerializer.Serialize(new { type = "responseId", data = responseId });
117120
await Response.WriteAsync($"data: {eventData}\n\n", cancellationToken);
118121
await Response.Body.FlushAsync(cancellationToken);

0 commit comments

Comments
 (0)