Conversation
This comment has been minimized.
This comment has been minimized.
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [ProducesResponseType(StatusCodes.Status401Unauthorized)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| public async Task<ActionResult<ChatAttachmentUploadedResult>> UploadAttachment(string channelId, string messageId, [FromForm] IFormFile file, CancellationToken cancellationToken) | ||
| public async Task<ActionResult<ChatAttachmentUploadedResult>> UploadAttachment(string channelId, string messageId, IFormFile file, CancellationToken cancellationToken) |
|
Warning Review limit reached
Next review available in: 32 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changes add AES-256-GCM encryption with legacy CBC decryption, RabbitMQ channel and connection recovery, connection monitoring with restart limits, and targeted moderation, attachment, and client-side validation fixes. ChangesRabbitMQ recovery
Authenticated encryption
Application validation and error handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EventWorker
participant RabbitInboundEventProvider
participant RabbitConnection
participant RabbitOutboundQueueProvider
EventWorker->>RabbitInboundEventProvider: Start
EventWorker->>RabbitInboundEventProvider: IsConnected
RabbitInboundEventProvider->>RabbitConnection: VerifyAndCreateClients
RabbitOutboundQueueProvider->>RabbitConnection: ForceResetAsync on channel allocation failure
RabbitOutboundQueueProvider->>RabbitOutboundQueueProvider: retry publishing once
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| /// numbers are exhausted (ChannelAllocationException) — which the IsOpen guards can never | ||
| /// detect. Raises ConnectionReset so cached declaration state is cleared. | ||
| /// </summary> | ||
| public static async Task ForceResetAsync() |
There was a problem hiding this comment.
Race condition in VerifyAndCreateClients: the _connection == null check (line 31) lacks a re-check inside the semaphore block, allowing concurrent send threads recovering from ChannelAllocationException to simultaneously create new IConnection instances and exhaust the broker's connection limit. Adding an inner _connection == null re-check inside the critical section ensures only the first thread creates the connection.
if (_connection == null)
{
await _semaphore.WaitAsync();
try
{
if (_connection == null) // re-check inside the lock: a prior holder may have already created it
{
_factory = new ConnectionFactory() { ... };
_connection = await _factory.CreateConnectionAsync(clientName);
}
}Prompt for LLM
File Providers/Resgrid.Providers.Bus.Rabbit/RabbitConnection.cs:
Line 248:
Race condition in `VerifyAndCreateClients`: the `_connection == null` check (line 31) lacks a re-check inside the semaphore block, allowing concurrent send threads recovering from `ChannelAllocationException` to simultaneously create new `IConnection` instances and exhaust the broker's connection limit. Adding an inner `_connection == null` re-check inside the critical section ensures only the first thread creates the connection.
Suggested Code:
if (_connection == null)
{
await _semaphore.WaitAsync();
try
{
if (_connection == null) // re-check inside the lock: a prior holder may have already created it
{
_factory = new ConnectionFactory() { ... };
_connection = await _factory.CreateConnectionAsync(clientName);
}
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| catch (Exception ex) | ||
| { | ||
| Logging.LogException(ex); |
There was a problem hiding this comment.
Insufficient error context: the catch block logs only the raw exception without identifying the failing channel or the disposal operation. Pass a descriptive message and structured fields, such as Logging.LogException(ex, "DisposeChannelsAsync", new { ChannelType = channel?.GetType().Name }), to capture essential diagnostic data.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs:
Line 670:
Insufficient error context: the catch block logs only the raw exception without identifying the failing channel or the disposal operation. Pass a descriptive message and structured fields, such as `Logging.LogException(ex, "DisposeChannelsAsync", new { ChannelType = channel?.GetType().Name })`, to capture essential diagnostic data.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (connection != null) | ||
| return await PublishAsync(queueName, message, durable, expiration, requirePublisherConfirmation); | ||
| } | ||
| catch (RabbitMQ.Client.Exceptions.ChannelAllocationException ex) |
There was a problem hiding this comment.
Code duplication: the entire ChannelAllocationException recovery block is duplicated verbatim in SendMessagesWithConfirmation at line 260. Extract the log-reset-retry pattern into a reusable named function like RetryAfterResetAsync(Func<Task<bool>> retryOp, Exception ex) and call it from both catch blocks.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Providers/Resgrid.Providers.Bus.Rabbit/RabbitOutboundQueueProvider.cs:
Line 156:
Code duplication: the entire `ChannelAllocationException` recovery block is duplicated verbatim in `SendMessagesWithConfirmation` at line 260. Extract the log-reset-retry pattern into a reusable named function like `RetryAfterResetAsync(Func<Task<bool>> retryOp, Exception ex)` and call it from both catch blocks.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| } | ||
| else if ($('#Type').select2('data').text === 'Internal') { | ||
| else if (typeof currentType !== 'undefined' && currentType === 'Internal') { |
There was a problem hiding this comment.
Magic string vulnerability: the literal 'Internal' is error-prone because typos or casing changes will not be caught at compile time. Define a constants object or enum in a shared module and compare against ListType.Internal.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/dlist/resgrid.dlists.editlist.js:
Line 66:
Magic string vulnerability: the literal `'Internal'` is error-prone because typos or casing changes will not be caught at compile time. Define a constants object or enum in a shared module and compare against `ListType.Internal`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| try | ||
| { | ||
| _logger.LogWarning($"{Name}: Queue consumers disconnected; restarting queue monitoring."); | ||
| await queue.Start("QueueProcessor-CQRS"); |
There was a problem hiding this comment.
Duplicated magic string: repeating the "QueueProcessor-CQRS" queue name risks drift if it is renamed and obscures the canonical value location. Define a single constant in a shared location and use it for both the initial Start and restart calls.
Kody rule violation: Centralize string constants
Prompt for LLM
File Workers/Resgrid.Workers.Console/Tasks/QueuesProcessorTask.cs:
Line 85:
Duplicated magic string: repeating the `"QueueProcessor-CQRS"` queue name risks drift if it is renamed and obscures the canonical value location. Define a single constant in a shared location and use it for both the initial Start and restart calls.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| disconnectedChecks++; | ||
|
|
||
| if (disconnectedChecks >= 20 && (DateTime.UtcNow - lastRestartAttemptUtc) >= TimeSpan.FromSeconds(60)) |
There was a problem hiding this comment.
Magic numbers in condition: inlined literals 20 and 60 encode operational thresholds that are prone to inconsistent modifications and misinterpretation. Hoist these values to named constants like DisconnectThresholdChecks and RestartCooldown to clarify their operational meaning.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Workers/Resgrid.Workers.Console/Tasks/QueuesProcessorTask.cs:
Line 78:
Magic numbers in condition: inlined literals `20` and `60` encode operational thresholds that are prone to inconsistent modifications and misinterpretation. Hoist these values to named constants like `DisconnectThresholdChecks` and `RestartCooldown` to clarify their operational meaning.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Web/Resgrid.Web.Eventing/Worker.cs (1)
88-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute the watchdog messages through
Resgrid.Framework.Logging.These two new messages report a silent-outage condition and its recovery.
Console.WriteLineoutput is not captured by the framework logging pipeline, so the signal is lost in production. The coding guidelines require theResgrid.Framework.Loggingstatic methods for logging.♻️ Proposed change
- Console.WriteLine("Eventing Worker: Rabbit consumer disconnected; restarting event monitoring."); + Resgrid.Framework.Logging.LogError("Eventing Worker: Rabbit consumer disconnected; restarting event monitoring."); await StartProviderAsync(); if (_rabbitInboundEventProvider.IsConnected()) { disconnectedChecks = 0; - Console.WriteLine("Eventing Worker: Event monitoring restarted."); + Resgrid.Framework.Logging.LogInfo("Eventing Worker: Event monitoring restarted."); }As per coding guidelines: "Use
Resgrid.Framework.Loggingstatic methods for logging:LogException(),LogError(),LogInfo(),LogDebug()".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Eventing/Worker.cs` around lines 88 - 94, Replace the two watchdog Console.WriteLine calls in the Rabbit consumer restart flow with the appropriate Resgrid.Framework.Logging static method, using LogInfo for the disconnect/restart and recovery messages while preserving their existing text and control flow.Source: Coding guidelines
Workers/Resgrid.Workers.Console/Tasks/QueuesProcessorTask.cs (1)
66-74: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReplace
Thread.Sleepwithawait Task.Delayin this async loop.
ProcessAsyncis async, and the loop now runs for the whole life of the task.Thread.Sleep(500)blocks a thread pool thread continuously and it does not observe_cancellationToken, so shutdown waits up to 500 ms per iteration.♻️ Proposed change
while (!_cancellationToken.IsCancellationRequested) { - Thread.Sleep(500); + try + { + await Task.Delay(500, _cancellationToken); + } + catch (OperationCanceledException) + { + break; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Workers/Resgrid.Workers.Console/Tasks/QueuesProcessorTask.cs` around lines 66 - 74, In ProcessAsync, replace the blocking Thread.Sleep(500) inside the cancellation loop with an awaited Task.Delay that uses _cancellationToken, preserving the 500 ms interval and allowing prompt cancellation during shutdown.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitConnection.cs`:
- Around line 248-277: Update VerifyAndCreateClients to re-check _connection
after acquiring _semaphore and return the existing connection when another
caller has already initialized it. Only create and assign a new
ConnectionFactory and connection while the semaphore is held and _connection
remains null, preventing concurrent reset callers from leaking duplicate
connections.
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs`:
- Around line 44-50: Update Start and its StartMonitoring startup flow so any
exception during channel creation or consumer registration disposes all newly
created channels via DisposeChannelsAsync before propagating the failure. Ensure
the failed startup leaves the provider’s connection state reporting false,
preventing partially initialized channels from being treated as healthy.
---
Nitpick comments:
In `@Web/Resgrid.Web.Eventing/Worker.cs`:
- Around line 88-94: Replace the two watchdog Console.WriteLine calls in the
Rabbit consumer restart flow with the appropriate Resgrid.Framework.Logging
static method, using LogInfo for the disconnect/restart and recovery messages
while preserving their existing text and control flow.
In `@Workers/Resgrid.Workers.Console/Tasks/QueuesProcessorTask.cs`:
- Around line 66-74: In ProcessAsync, replace the blocking Thread.Sleep(500)
inside the cancellation loop with an awaited Task.Delay that uses
_cancellationToken, preserving the 500 ms interval and allowing prompt
cancellation during shutdown.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4757be48-3c47-4a8d-aaae-bfdf0a8a7bf1
⛔ Files ignored due to path filters (2)
Tests/Resgrid.Tests/Bootstrapper.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/EncryptionServiceTests.csis excluded by!**/Tests/**
📒 Files selected for processing (14)
Core/Resgrid.Model/Providers/IRabbitInboundEventProvider.csCore/Resgrid.Services/EncryptionService.csCore/Resgrid.Services/ModerationService.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitConnection.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitInboundEventProvider.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitOutboundQueueProvider.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitTopicProvider.csWeb/Resgrid.Web.Eventing/Worker.csWeb/Resgrid.Web.Services/Controllers/v4/ChatController.csWeb/Resgrid.Web/wwwroot/js/app/internal/dlist/resgrid.dlists.editlist.jsWeb/Resgrid.Web/wwwroot/js/app/internal/dlist/resgrid.dlists.newlist.jsWeb/Resgrid.Web/wwwroot/js/app/internal/resgrid.user.jsWorkers/Resgrid.Workers.Console/Tasks/QueuesProcessorTask.cs
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Providers/Resgrid.Providers.Bus.Rabbit/RabbitConnection.cs (1)
259-273: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSerialize every
_connectionmutation through_semaphore, not only the reset path.
ForceResetAsyncestablishes_semaphoreas the owner of_connectionand_factory. Two other paths in this file still write those fields without holding the semaphore:
- Lines 23-29 in
VerifyAndCreateClientsdispose the connection and null both fields.- Lines 297-307 in
CreateConnectiondo the same.This allows a lost write. Thread A enters
ForceResetAsync, capturesconnection, and nulls the fields. Thread B is already past Line 23 with the old reference and is awaitingDisposeAsync(). Thread C then creates a fresh connection throughVerifyAndCreateClients. Thread B resumes and executes_connection = nullat Line 26, which discards the live connection that thread C created. That connection is never disposed and every later caller rebuilds another one. The same interleaving also double-disposes the oldIConnection.Move the stale-connection detection into a semaphore-protected helper and reuse it from all three call sites.
🔒️ Proposed fix: route the stale-connection teardown through the same lock
+ /// <summary> + /// Clears the shared connection under the semaphore when the supplied reference is still the + /// current one. Returns the connection to dispose, or null when another caller already replaced it. + /// </summary> + private static async Task<IConnection> ClearIfCurrentAsync(IConnection expected) + { + await _semaphore.WaitAsync(); + + try + { + if (!ReferenceEquals(_connection, expected)) + return null; + + _connection = null; + _factory = null; + } + finally + { + _semaphore.Release(); + } + + return expected; + } + public static async Task ForceResetAsync() { - IConnection connection; - - await _semaphore.WaitAsync(); - try - { - connection = _connection; - _connection = null; - _factory = null; - } - finally - { - _semaphore.Release(); - } + var connection = await ClearIfCurrentAsync(_connection);Then apply the helper at the two lock-free sites (outside the selected range):
// VerifyAndCreateClients, replacing lines 23-29 var stale = _connection; if (stale != null && !stale.IsOpen) { var cleared = await ClearIfCurrentAsync(stale); if (cleared != null) { RaiseConnectionReset(); try { await cleared.DisposeAsync(); } catch (Exception ex) { Logging.LogException(ex); } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitConnection.cs` around lines 259 - 273, Serialize all mutations of _connection and _factory through _semaphore by adding a ClearIfCurrentAsync helper that locks, verifies the supplied connection is still the current reference, clears both fields, and returns the cleared connection only when successful. Replace the lock-free stale-connection teardown in VerifyAndCreateClients and CreateConnection with this helper, preserving reset notification and disposal only for the returned connection; update ForceResetAsync to reuse the same protected clearing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs`:
- Around line 86-95: Update the startup catch around StartMonitoring to
specifically handle ChannelAllocationException, dispose the provider channels,
call RabbitConnection.ForceResetAsync(), then rethrow so watchdog retries create
a fresh connection; preserve existing cleanup and rethrow behavior for other
exceptions, and add the RabbitMQ.Client.Exceptions import if needed.
---
Outside diff comments:
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitConnection.cs`:
- Around line 259-273: Serialize all mutations of _connection and _factory
through _semaphore by adding a ClearIfCurrentAsync helper that locks, verifies
the supplied connection is still the current reference, clears both fields, and
returns the cleared connection only when successful. Replace the lock-free
stale-connection teardown in VerifyAndCreateClients and CreateConnection with
this helper, preserving reset notification and disposal only for the returned
connection; update ForceResetAsync to reuse the same protected clearing
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f1408bb0-6c72-4565-9c24-42be81633dc0
📒 Files selected for processing (4)
Providers/Resgrid.Providers.Bus.Rabbit/RabbitConnection.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitInboundEventProvider.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.csWorkers/Resgrid.Workers.Console/Tasks/QueuesProcessorTask.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundEventProvider.cs
- Workers/Resgrid.Workers.Console/Tasks/QueuesProcessorTask.cs
| try | ||
| { | ||
| _factory = new ConnectionFactory() { HostName = ServiceBusConfig.RabbitHostname2, UserName = ServiceBusConfig.RabbitUsername, Password = ServiceBusConfig.RabbbitPassword }; | ||
| _factory = new ConnectionFactory() { HostName = ServiceBusConfig.RabbitHostname, UserName = ServiceBusConfig.RabbitUsername, Password = ServiceBusConfig.RabbbitPassword }; |
There was a problem hiding this comment.
Duplicated logic across the connection-factory creation and CreateConnectionAsync sequence (lines 46-47, 57-58, 68-69) complicates maintenance if factory options change. Extract a helper such as static async Task<bool> TryConnectAsync(string hostname, string clientName) to build the factory and attempt connections.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Providers/Resgrid.Providers.Bus.Rabbit/RabbitConnection.cs:
Line 46:
Duplicated logic across the connection-factory creation and `CreateConnectionAsync` sequence (lines 46-47, 57-58, 68-69) complicates maintenance if factory options change. Extract a helper such as `static async Task<bool> TryConnectAsync(string hostname, string clientName)` to build the factory and attempt connections.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // IsConnected() would report healthy and the host watchdog would never rebuild. | ||
| // Tear the channel down (nulled field makes IsConnected() false) and rethrow for | ||
| // the caller's retry path. | ||
| await DisposeChannelAsync(); |
There was a problem hiding this comment.
Unhandled exception in await DisposeChannelAsync() inside the catch block masks the original StartMonitoring exception and prevents the rethrow from executing. Wrap the cleanup in its own try/catch to handle secondary errors, then throw; the original exception.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundEventProvider.cs:
Line 59:
Unhandled exception in `await DisposeChannelAsync()` inside the catch block masks the original `StartMonitoring` exception and prevents the rethrow from executing. Wrap the cleanup in its own try/catch to handle secondary errors, then `throw;` the original exception.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // A failed initial start (broker briefly down at boot, partial startup torn down by | ||
| // the provider) must not kill this task before the watchdog loop below exists — | ||
| // the loop sees IsConnected() == false and retries. | ||
| Resgrid.Framework.Logging.LogException(ex); |
There was a problem hiding this comment.
Insufficient logging context in Resgrid.Framework.Logging.LogException(ex) catch blocks prevents proper failure correlation. Include structured fields for the operation name and queue identifier using an equivalent overload, such as Resgrid.Framework.Logging.LogException("QueueProcessor start failed", ex, new { Queue = "QueueProcessor-CQRS", Operation = "queue.Start" }).
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Workers/Resgrid.Workers.Console/Tasks/QueuesProcessorTask.cs:
Line 65:
Insufficient logging context in `Resgrid.Framework.Logging.LogException(ex)` catch blocks prevents proper failure correlation. Include structured fields for the operation name and queue identifier using an equivalent overload, such as `Resgrid.Framework.Logging.LogException("QueueProcessor start failed", ex, new { Queue = "QueueProcessor-CQRS", Operation = "queue.Start" })`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
| // watchdog's retry would get the same connection back forever — reset it so the | ||
| // next Start builds a fresh one. | ||
| if (ex is RabbitMQ.Client.Exceptions.ChannelAllocationException) | ||
| await RabbitConnection.ForceResetAsync(); |
There was a problem hiding this comment.
Unhandled exception vulnerability exists in the catch block because RabbitConnection.ForceResetAsync() lacks its own try/catch. If ForceResetAsync throws, the exception escapes the handler and crashes the caller; wrap the await in a nested try/catch and log any secondary failure before returning false.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundEventProvider.cs:
Line 106:
Unhandled exception vulnerability exists in the catch block because `RabbitConnection.ForceResetAsync()` lacks its own try/catch. If `ForceResetAsync` throws, the exception escapes the handler and crashes the caller; wrap the await in a nested try/catch and log any secondary failure before returning false.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // watchdog's retry would get the same connection back forever — reset it so the | ||
| // next Start builds a fresh one. | ||
| if (ex is RabbitMQ.Client.Exceptions.ChannelAllocationException) | ||
| await RabbitConnection.ForceResetAsync(); |
There was a problem hiding this comment.
Unhandled exception risk in ForceResetAsync exposes the caller to unhandled network I/O failures propagating from the catch block with no context. Wrap the reset call in a dedicated try/catch, log the secondary exception with context (clientName, operation 'ForceReset'), and continue to return false.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundEventProvider.cs:
Line 106:
Unhandled exception risk in `ForceResetAsync` exposes the caller to unhandled network I/O failures propagating from the catch block with no context. Wrap the reset call in a dedicated try/catch, log the secondary exception with context (clientName, operation 'ForceReset'), and continue to return false.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
3 participants
Description
This PR addresses RabbitMQ connection reliability issues, chat functionality, and several web UI bugs.
RabbitMQ Resilience Improvements
Channel exhaustion recovery: The shared RabbitMQ connection can report as open while its channel numbers are exhausted (
ChannelAllocationException), making all outbound sends fail silently until a process restart. Outbound and topic providers now detect this exception, force-reset the connection, and retry the publish on a fresh connection.Consumer watchdog: Inbound event consumers (SignalR eventing worker and CQRS queue processor) die silently when the shared connection is replaced or automatic recovery fails, with nothing restarting them. A watchdog loop now monitors
IsConnected()every 500ms and rebuilds consumers after ~10 seconds of continuous disconnect, retrying at most once per minute to avoid spinning during a broker outage.Duplicate event prevention:
Start()calls in both inbound providers now dispose any existing channels before creating new ones. This prevents old consumers from being resurrected by late automatic-recovery events alongside new consumers, which would double-deliver or double-process messages.Chat Fix
Removed the
[FromForm]attribute from theUploadAttachmentendpoint's file parameter to fix attachment upload binding.Web UI Fixes
switchInputsfunction that toggled between internal/external email fields, and corrected validation logic that relied on reading Select2 state (edit list) or incorrectly allowed blank addresses for new lists.nameattribute. Validation is now configured to ignore contenteditable elements globally.Summary by CodeRabbit
Security
Reliability
Bug Fixes