Skip to content

Commit 670b0f5

Browse files
CopilotBenjaminMichaelisgithub-code-quality[bot]
authored
fix: ChatController naming consistency and streaming cancellation/error handling (#1089)
- [x] Inspect PR feedback and failing CI for `/home/runner/work/EssentialCSharp.Web/EssentialCSharp.Web/EssentialCSharp.Web/Controllers/ChatController.cs` - [x] Update `StreamMessage` cancellation/error handling to address the active PR comments and fix the compile error - [x] Rebuild the solution and run relevant tests to verify the change - [ ] Run final validation and summarize results <!-- START COPILOT CODING AGENT SUFFIX --> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> Please apply the following diffs and create a pull request. Once the PR is ready, give it a title based on the messages of the fixes being applied. [{"message":"The field `_AiChatService` uses inconsistent casing ('Ai' instead of 'AI'). It should be renamed to `_AIChatService` to match the type name `AIChatService` and maintain consistency with other fields like `_Logger`.","fixFiles":[{"filePath":"EssentialCSharp.Web/Controllers/ChatController.cs","diff":"diff --git a/EssentialCSharp.Web/Controllers/ChatController.cs b/EssentialCSharp.Web/Controllers/ChatController.cs\n--- a/EssentialCSharp.Web/Controllers/ChatController.cs\n+++ b/EssentialCSharp.Web/Controllers/ChatController.cs\n@@ -15,13 +15,13 @@\n [IgnoreAntiforgeryToken]\n public partial class ChatController : ControllerBase\n {\n- private readonly AIChatService _AiChatService;\n+ private readonly AIChatService _AIChatService;\n private readonly ResponseIdValidationService _ResponseIdValidationService;\n private readonly ILogger<ChatController> _Logger;\n \n public ChatController(ILogger<ChatController> logger, AIChatService aiChatService, ResponseIdValidationService responseIdValidationService)\n {\n- _AiChatService = aiChatService;\n+ _AIChatService = aiChatService;\n _ResponseIdValidationService = responseIdValidationService;\n _Logger = logger;\n }\n@@ -46,7 +41,7 @@\n \n try\n {\n- var (response, responseId) = await _AiChatService.GetChatCompletion(\n+ var (response, responseId) = await _AIChatService.GetChatCompletion(\n prompt: request.Message,\n previousResponseId: previousResponseId,\n enableContextualSearch: request.EnableContextualSearch,\n"}]},{"message":"The method uses `CancellationToken.None` instead of the provided `cancellationToken` parameter when writing error responses. This prevents graceful cancellation of the write operation if the request is aborted. Replace `CancellationToken.None` with `cancellationToken` throughout the StreamMessage method (lines 78, 86, 97, 144, 151, 152, 167, 174, 175) to respect cancellation requests.","fixFiles":[{"filePath":"EssentialCSharp.Web/Controllers/ChatController.cs","diff":"diff --git a/EssentialCSharp.Web/Controllers/ChatController.cs b/EssentialCSharp.Web/Controllers/ChatController.cs\n--- a/EssentialCSharp.Web/Controllers/ChatController.cs\n+++ b/EssentialCSharp.Web/Controllers/ChatController.cs\n@@ -75,7 +75,7 @@\n if (string.IsNullOrEmpty(request.Message))\n {\n Response.StatusCode = 400;\n- await Response.WriteAsJsonAsync(new { error = \"Message cannot be empty.\" }, CancellationToken.None);\n+ await Response.WriteAsJsonAsync(new { error = \"Message cannot be empty.\" }, cancellationToken);\n return;\n }\n \n@@ -83,7 +83,7 @@\n if (string.IsNullOrEmpty(userId))\n {\n Response.StatusCode = 401;\n- await Response.WriteAsJsonAsync(new { error = \"Unauthorized.\" }, CancellationToken.None);\n+ await Response.WriteAsJsonAsync(new { error = \"Unauthorized.\" }, cancellationToken);\n return;\n }\n \n@@ -94,7 +94,7 @@\n if (!_ResponseIdValidationService.ValidateResponseId(userId, previousResponseId))\n {\n Response.StatusCode = 400;\n- await Response.WriteAsJsonAsync(new { error = \"Invalid conversation context.\" }, CancellationToken.None);\n+ await Response.WriteAsJsonAsync(new { error = \"Invalid conversation context.\" }, cancellationToken);\n return;\n }\n \n"}]},{"message":"The when clause checks both `cancellationToken.IsCancellationRequested` and `HttpContext.RequestAborted.IsCancellationRequested`, but this filter will not catch `OperationCanceledException` thrown when either token is cancelled without the corresponding property being set at the exact moment the filter evaluates. Remove the when clause entirely or simplify to catch all `OperationCanceledException` instances, as these are expected during cancellation.","fixFiles":[{"filePath":"EssentialCSharp.Web/Controllers/ChatController.cs","diff":"diff --git a/EssentialCSharp.Web/Controllers/ChatController.cs b/EssentialCSharp.Web/Controllers/ChatController.cs\n--- a/EssentialCSharp.Web/Controllers/ChatController.cs\n+++ b/EssentialCSharp.Web/Controllers/ChatController.cs\n@@ -133,7 +133,7 @@\n await Response.WriteAsync(\"data: [DONE]\\n\\n\", cancellationToken);\n await Response.Body.FlushAsync(cancellationToken);\n }\n- catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested || HttpContext.RequestAborted.IsCancellationRequested)\n+ catch (OperationCanceledException)\n {\n LogChatStreamCancelled(_Logger, User.Identity?.Name);\n }\n"}]},{"message":"The exception handling f... </details> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: BenjaminMichaelis <22186029+BenjaminMichaelis@users.noreply.github.com> Co-authored-by: Benjamin Michaelis <git@relay.benjamin.michaelis.net> Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
1 parent 44487d8 commit 670b0f5

4 files changed

Lines changed: 162 additions & 42 deletions

File tree

‎EssentialCSharp.Web/Controllers/ChatController.cs‎

Lines changed: 80 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using System.IO;
12
using System.Security.Claims;
23
using System.Text.Json;
34
using EssentialCSharp.Chat.Common.Services;
@@ -15,13 +16,13 @@ namespace EssentialCSharp.Web.Controllers;
1516
[IgnoreAntiforgeryToken]
1617
public partial class ChatController : ControllerBase
1718
{
18-
private readonly AIChatService _AiChatService;
19+
private readonly AIChatService _AIChatService;
1920
private readonly ResponseIdValidationService _ResponseIdValidationService;
2021
private readonly ILogger<ChatController> _Logger;
2122

2223
public ChatController(ILogger<ChatController> logger, AIChatService aiChatService, ResponseIdValidationService responseIdValidationService)
2324
{
24-
_AiChatService = aiChatService;
25+
_AIChatService = aiChatService;
2526
_ResponseIdValidationService = responseIdValidationService;
2627
_Logger = logger;
2728
}
@@ -46,7 +47,7 @@ public async Task<IActionResult> SendMessage([FromBody] ChatMessageRequest reque
4647

4748
try
4849
{
49-
var (response, responseId) = await _AiChatService.GetChatCompletion(
50+
var (response, responseId) = await _AIChatService.GetChatCompletion(
5051
prompt: request.Message,
5152
previousResponseId: previousResponseId,
5253
enableContextualSearch: request.EnableContextualSearch,
@@ -75,15 +76,15 @@ public async Task StreamMessage([FromBody] ChatMessageRequest request, Cancellat
7576
if (string.IsNullOrEmpty(request.Message))
7677
{
7778
Response.StatusCode = 400;
78-
await Response.WriteAsJsonAsync(new { error = "Message cannot be empty." }, CancellationToken.None);
79+
await Response.WriteAsJsonAsync(new { error = "Message cannot be empty." }, cancellationToken);
7980
return;
8081
}
8182

8283
var userId = User.FindFirstValue(ClaimTypes.NameIdentifier);
8384
if (string.IsNullOrEmpty(userId))
8485
{
8586
Response.StatusCode = 401;
86-
await Response.WriteAsJsonAsync(new { error = "Unauthorized." }, CancellationToken.None);
87+
await Response.WriteAsJsonAsync(new { error = "Unauthorized." }, cancellationToken);
8788
return;
8889
}
8990

@@ -94,7 +95,7 @@ public async Task StreamMessage([FromBody] ChatMessageRequest request, Cancellat
9495
if (!_ResponseIdValidationService.ValidateResponseId(userId, previousResponseId))
9596
{
9697
Response.StatusCode = 400;
97-
await Response.WriteAsJsonAsync(new { error = "Invalid conversation context." }, CancellationToken.None);
98+
await Response.WriteAsJsonAsync(new { error = "Invalid conversation context." }, cancellationToken);
9899
return;
99100
}
100101

@@ -104,7 +105,7 @@ public async Task StreamMessage([FromBody] ChatMessageRequest request, Cancellat
104105

105106
try
106107
{
107-
await foreach (var (text, responseId) in _AiChatService.GetChatCompletionStream(
108+
await foreach (var (text, responseId) in _AIChatService.GetChatCompletionStream(
108109
prompt: request.Message,
109110
previousResponseId: previousResponseId,
110111
enableContextualSearch: request.EnableContextualSearch,
@@ -133,53 +134,102 @@ public async Task StreamMessage([FromBody] ChatMessageRequest request, Cancellat
133134
await Response.WriteAsync("data: [DONE]\n\n", cancellationToken);
134135
await Response.Body.FlushAsync(cancellationToken);
135136
}
136-
catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested || HttpContext.RequestAborted.IsCancellationRequested)
137+
catch (OperationCanceledException)
137138
{
138-
LogChatStreamCancelled(_Logger, User.Identity?.Name);
139-
}
140-
catch (ConversationContextLimitExceededException) when (!Response.HasStarted)
141-
{
142-
Response.StatusCode = 400;
143-
Response.ContentType = "application/json";
144-
await Response.WriteAsJsonAsync(new { error = "This conversation has grown too long. Please start a new one.", errorCode = "context_limit_exceeded" }, CancellationToken.None);
139+
if (cancellationToken.IsCancellationRequested || HttpContext.RequestAborted.IsCancellationRequested)
140+
{
141+
LogChatStreamCancelled(_Logger, User.Identity?.Name);
142+
return;
143+
}
144+
145+
throw;
145146
}
146147
catch (ConversationContextLimitExceededException ex)
147148
{
148-
LogChatStreamErrorMidStream(_Logger, ex, User.Identity?.Name);
149-
try
149+
if (!Response.HasStarted)
150150
{
151-
await Response.WriteAsync("data: {\"type\":\"error\",\"message\":\"This conversation has grown too long. Please start a new one.\",\"errorCode\":\"context_limit_exceeded\"}\n\n", CancellationToken.None);
152-
await Response.Body.FlushAsync(CancellationToken.None);
151+
if (cancellationToken.IsCancellationRequested || HttpContext.RequestAborted.IsCancellationRequested)
152+
return;
153+
154+
Response.StatusCode = 400;
155+
Response.ContentType = "application/json";
156+
try
157+
{
158+
var writeCancellationToken =
159+
cancellationToken.IsCancellationRequested || HttpContext.RequestAborted.IsCancellationRequested
160+
? CancellationToken.None
161+
: cancellationToken;
162+
await Response.WriteAsJsonAsync(new { error = "This conversation has grown too long. Please start a new one.", errorCode = "context_limit_exceeded" }, writeCancellationToken);
163+
}
164+
catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested || HttpContext.RequestAborted.IsCancellationRequested)
165+
{
166+
// Best-effort write during an aborted request — no response body can be delivered.
167+
}
168+
catch (IOException) when (HttpContext.RequestAborted.IsCancellationRequested)
169+
{
170+
// Expected client disconnect while attempting a best-effort error response write.
171+
}
172+
catch (ObjectDisposedException) when (HttpContext.RequestAborted.IsCancellationRequested)
173+
{
174+
// Response stream can already be disposed after an abrupt client disconnect.
175+
}
153176
}
154-
catch (Exception)
177+
else
155178
{
156-
// Best-effort write to an already-streaming response. Kestrel can throw
157-
// IOException (connection reset), OperationCanceledException, or
158-
// ObjectDisposedException on abrupt client disconnect — swallow all to
159-
// avoid masking the original exception.
179+
LogChatStreamErrorMidStream(_Logger, ex, User.Identity?.Name);
180+
try
181+
{
182+
await Response.WriteAsync("data: {\"type\":\"error\",\"message\":\"This conversation has grown too long. Please start a new one.\",\"errorCode\":\"context_limit_exceeded\"}\n\n", cancellationToken);
183+
await Response.Body.FlushAsync(cancellationToken);
184+
}
185+
catch (Exception writeException) when (writeException is IOException or OperationCanceledException or ObjectDisposedException)
186+
{
187+
// Best-effort write to an already-streaming response. Kestrel can throw
188+
// IOException (connection reset), OperationCanceledException, or
189+
// ObjectDisposedException on abrupt client disconnect — swallow expected
190+
// transport/disconnect exceptions to avoid masking the original exception.
191+
}
160192
}
161193
}
162194
catch (Exception ex) when (!Response.HasStarted)
163195
{
164196
LogChatStreamErrorBeforeResponseStarted(_Logger, ex, User.Identity?.Name);
197+
if (cancellationToken.IsCancellationRequested || HttpContext.RequestAborted.IsCancellationRequested)
198+
return;
199+
165200
Response.StatusCode = 500;
166201
Response.ContentType = "application/json";
167-
await Response.WriteAsJsonAsync(new { error = "Chat service unavailable" }, CancellationToken.None);
202+
try
203+
{
204+
await Response.WriteAsJsonAsync(new { error = "Chat service unavailable" }, cancellationToken);
205+
}
206+
catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested || HttpContext.RequestAborted.IsCancellationRequested)
207+
{
208+
// Best-effort write during an aborted request — no response body can be delivered.
209+
}
210+
catch (IOException) when (HttpContext.RequestAborted.IsCancellationRequested)
211+
{
212+
// Expected client disconnect while attempting a best-effort error response write.
213+
}
214+
catch (ObjectDisposedException) when (HttpContext.RequestAborted.IsCancellationRequested)
215+
{
216+
// Response stream can already be disposed after an abrupt client disconnect.
217+
}
168218
}
169219
catch (Exception ex)
170220
{
171221
LogChatStreamErrorMidStream(_Logger, ex, User.Identity?.Name);
172222
try
173223
{
174-
await Response.WriteAsync("data: {\"type\":\"error\",\"message\":\"Stream interrupted\"}\n\n", CancellationToken.None);
175-
await Response.Body.FlushAsync(CancellationToken.None);
224+
await Response.WriteAsync("data: {\"type\":\"error\",\"message\":\"Stream interrupted\"}\n\n", cancellationToken);
225+
await Response.Body.FlushAsync(cancellationToken);
176226
}
177-
catch (Exception)
227+
catch (Exception writeException) when (writeException is IOException or OperationCanceledException or ObjectDisposedException)
178228
{
179229
// Best-effort write to an already-streaming response. Kestrel can throw
180230
// IOException (connection reset), OperationCanceledException, or
181-
// ObjectDisposedException on abrupt client disconnect — swallow all to
182-
// avoid masking the original exception.
231+
// ObjectDisposedException on abrupt client disconnect — swallow expected
232+
// transport/disconnect exceptions to avoid masking the original exception.
183233
}
184234
}
185235
}

‎EssentialCSharp.Web/Extensions/IServiceCollectionExtensions.cs‎

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
1-
using EssentialCSharp.Web.Services;
1+
using System.Net;
2+
using System.Net.Sockets;
3+
using EssentialCSharp.Web.Services;
4+
using Microsoft.AspNetCore.HttpOverrides;
25

36
namespace EssentialCSharp.Web.Extensions;
47

@@ -13,4 +16,77 @@ public static void AddCaptchaService(this IServiceCollection services, IConfigur
1316
c.BaseAddress = new Uri("https://api.hcaptcha.com");
1417
});
1518
}
19+
20+
public static void AddTrustedForwardedHeaders(this IServiceCollection services, IConfiguration configuration, IHostEnvironment environment)
21+
{
22+
services.Configure<ForwardedHeadersOptions>(options =>
23+
{
24+
options.ForwardedHeaders =
25+
ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto;
26+
options.ForwardLimit = 1;
27+
28+
var trustedProxyCidrs = configuration
29+
.GetSection("ForwardedHeaders:TrustedProxyCidrs")
30+
.Get<string[]>() ?? [];
31+
var trustedProxies = configuration
32+
.GetSection("ForwardedHeaders:TrustedProxies")
33+
.Get<string[]>() ?? [];
34+
35+
if (trustedProxyCidrs.Length == 0 && trustedProxies.Length == 0)
36+
{
37+
if (!environment.IsDevelopment())
38+
{
39+
throw new InvalidOperationException(
40+
"Forwarded headers are enabled but no trusted proxies are configured. " +
41+
"Set ForwardedHeaders:TrustedProxyCidrs or ForwardedHeaders:TrustedProxies.");
42+
}
43+
return;
44+
}
45+
46+
options.KnownIPNetworks.Clear();
47+
options.KnownProxies.Clear();
48+
49+
foreach (var cidr in trustedProxyCidrs)
50+
{
51+
if (!TryParseCidr(cidr, out var network))
52+
throw new InvalidOperationException($"Invalid ForwardedHeaders:TrustedProxyCidrs entry '{cidr}'. Use CIDR notation, e.g. '10.0.0.0/8'.");
53+
54+
options.KnownIPNetworks.Add(network);
55+
}
56+
57+
foreach (var proxy in trustedProxies)
58+
{
59+
if (!IPAddress.TryParse(proxy, out var proxyAddress))
60+
throw new InvalidOperationException($"Invalid ForwardedHeaders:TrustedProxies entry '{proxy}'. Use a valid IP address.");
61+
62+
options.KnownProxies.Add(proxyAddress);
63+
}
64+
});
65+
}
66+
67+
private static bool TryParseCidr(string cidr, out System.Net.IPNetwork network)
68+
{
69+
network = default!;
70+
if (string.IsNullOrWhiteSpace(cidr))
71+
return false;
72+
73+
string[] parts = cidr.Split('/', 2, StringSplitOptions.TrimEntries);
74+
if (parts.Length != 2
75+
|| !IPAddress.TryParse(parts[0], out var networkAddress)
76+
|| !int.TryParse(parts[1], out var prefixLength))
77+
return false;
78+
79+
int maxPrefixLength = networkAddress.AddressFamily switch
80+
{
81+
AddressFamily.InterNetwork => 32,
82+
AddressFamily.InterNetworkV6 => 128,
83+
_ => -1
84+
};
85+
86+
if (maxPrefixLength < 0 || prefixLength < 0 || prefixLength > maxPrefixLength)
87+
return false;
88+
89+
network = new System.Net.IPNetwork(networkAddress, prefixLength);
90+
return true;
91+
}
1692
}

‎EssentialCSharp.Web/Program.cs‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -119,17 +119,7 @@ private static void Main(string[] args)
119119

120120

121121

122-
builder.Services.Configure<ForwardedHeadersOptions>(options =>
123-
{
124-
options.ForwardedHeaders =
125-
ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto;
126-
127-
// Only loopback proxies are allowed by default.
128-
// Clear that restriction because forwarders are enabled by explicit
129-
// configuration.
130-
options.KnownIPNetworks.Clear();
131-
options.KnownProxies.Clear();
132-
});
122+
builder.Services.AddTrustedForwardedHeaders(builder.Configuration, builder.Environment);
133123

134124
ConfigurationManager configuration = builder.Configuration;
135125
string connectionString = builder.Configuration.GetConnectionString("EssentialCSharpWebContextConnection") ?? throw new InvalidOperationException("Connection string 'EssentialCSharpWebContextConnection' not found.");

‎EssentialCSharp.Web/appsettings.json‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,10 @@
99
}
1010
},
1111
"AllowedHosts": "*",
12+
"ForwardedHeaders": {
13+
"TrustedProxyCidrs": [],
14+
"TrustedProxies": []
15+
},
1216
"HCaptcha": {
1317
"SecretKey": "0x0000000000000000000000000000000000000000",
1418
"SiteKey": "10000000-ffff-ffff-ffff-000000000001"

0 commit comments

Comments
 (0)