Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
|
Warning Review limit reached
More reviews will be available in 25 minutes and 35 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds stricter validation and race-condition handling to timer persistence, batch-loads state data to improve active-timer filtering, introduces schedule-based weather alert auto-notification deferral with decision-driven logic, hardens controller input validation, and corrects semaphore handling in the RabbitMQ connection pool. ChangesCheck-In Timer Robustness & Unit Type Filtering
Weather Alert Scheduling & Auto-Message Decisions
RabbitMQ Connection Semaphore Correction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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)
31-77:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Missing double-checked locking causes race condition.
After acquiring the semaphore on line 33, the code does not re-check whether
_connectionis still null. If two threads both observe_connection == nullon line 31 before either acquires the semaphore, the second thread will overwrite the first thread's connection and potentially leak resources.🔒 Proposed fix: Add second null check inside semaphore
if (_connection == null) { await _semaphore.WaitAsync(); try { + // Double-checked locking: recheck null after acquiring semaphore to prevent race + if (_connection == null) + { _factory = new ConnectionFactory() { HostName = ServiceBusConfig.RabbitHostname, UserName = ServiceBusConfig.RabbitUsername, Password = ServiceBusConfig.RabbbitPassword }; _connection = await _factory.CreateConnectionAsync(clientName); + } } catch (Exception ex) { Logging.LogException(ex); if (!String.IsNullOrWhiteSpace(ServiceBusConfig.RabbitHostname2)) { try { + if (_connection == null) + { _factory = new ConnectionFactory() { HostName = ServiceBusConfig.RabbitHostname2, UserName = ServiceBusConfig.RabbitUsername, Password = ServiceBusConfig.RabbbitPassword }; _connection = await _factory.CreateConnectionAsync(clientName); + } } catch (Exception ex2) { Logging.LogException(ex2); if (!String.IsNullOrWhiteSpace(ServiceBusConfig.RabbitHostname3)) { try { + if (_connection == null) + { _factory = new ConnectionFactory() { HostName = ServiceBusConfig.RabbitHostname3, UserName = ServiceBusConfig.RabbitUsername, Password = ServiceBusConfig.RabbbitPassword }; _connection = await _factory.CreateConnectionAsync(clientName); + } } catch (Exception ex3) { Logging.LogException(ex3); throw; } } } } } finally { // Single release: the semaphore is acquired once above, so release it exactly once here. // The outer finally covers every path (primary success, host2/host3 fallback, and rethrow); // a second release in the fallback branch previously threw SemaphoreFullException. _semaphore.Release(); }🤖 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 31 - 77, After acquiring _semaphore in the connection creation block, perform a second null-check on _connection and skip creating/overwriting it if another thread already set it; specifically, in the try block after await _semaphore.WaitAsync(), add if (_connection == null) { ...existing create/fallback logic... } so ConnectionFactory/await _factory.CreateConnectionAsync(clientName) only run when still necessary, preserving the single _semaphore.Release() in the finally and preventing races that overwrite/ leak connections.
🧹 Nitpick comments (3)
Core/Resgrid.Services/WeatherAlertService.cs (1)
704-708: 💤 Low valueOvernight window branch may be unreachable given API validation.
The API validation in
WeatherAlertsController.SaveSettingsrejects schedules whereEndHour <= StartHourfor enabled entries, which prevents overnight windows (e.g., StartHour=22, EndHour=6) from being configured. This makes the overnight branch at lines 704-708 unreachable for newly saved schedules.If overnight windows are intentionally unsupported, consider removing this branch to avoid confusion. If they should be supported in the future, the API validation at
WeatherAlertsController.cs:380-382would need to be updated to allowStartHour > EndHourfor overnight scenarios.As-is, this code is defensive and handles pre-existing data gracefully, so not blocking.
🤖 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 `@Core/Resgrid.Services/WeatherAlertService.cs` around lines 704 - 708, The overnight-window branch in WeatherAlertService (the inWindow calculation that handles StartHour > EndHour) is effectively unreachable because WeatherAlertsController.SaveSettings currently rejects enabled schedules with EndHour <= StartHour; either remove the overnight branch to clean up dead code (replace the conditional with the straightforward inWindow = currentHour >= entry.StartHour && currentHour < entry.EndHour logic in the method containing the inWindow calculation) or, if overnight schedules should be supported, update WeatherAlertsController.SaveSettings validation (the checks around lines referenced in that method) to allow StartHour > EndHour for enabled entries and add/update unit tests to cover overnight scenarios.Core/Resgrid.Services/CheckInTimerService.cs (2)
145-150: ⚖️ Poor tradeoffConsider a direct query instead of loading all overrides.
The method loads all department overrides via
GetByDepartmentIdAsyncand filters in-memory twice (lines 145-150 for initial check, lines 193-198 for race recovery). For departments with many overrides, a direct repository query likeGetByDepartmentAndTargetAsync(as used for configs at line 55) would be more efficient.If adding such a method is non-trivial, this is acceptable for now given typical override counts are likely low.
Also applies to: 193-198
🤖 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 `@Core/Resgrid.Services/CheckInTimerService.cs` around lines 145 - 150, Replace the in-memory filtering of department overrides with a direct repository query: instead of calling GetByDepartmentIdAsync and then using departmentOverrides?.FirstOrDefault(...) to produce existingForTarget, add/use a repository method like GetByDepartmentAndTargetAsync (same pattern used for configs) that accepts DepartmentId, CallTypeId, CallPriority, TimerTargetType and UnitTypeId and returns the matching override; update both the initial check (where existingForTarget is computed) and the race-recovery check (the later block that repeats the filtering) to call this new method so we avoid loading all overrides into memory.
91-109: 💤 Low valueAdd logging when recovering from insert race condition.
The race condition handling logic is correct, but when the exception path is taken, there's no logging to aid debugging. Per coding guidelines, use
Resgrid.Framework.Loggingto capture exceptions.catch (Exception) when (isInsert) { + Resgrid.Framework.Logging.LogInfo($"CheckInTimerConfig insert race for DepartmentId={config.DepartmentId}, TargetType={config.TimerTargetType}, UnitTypeId={config.UnitTypeId}; adopting winner row"); // Two concurrent saves can both pass the lookup above and race the unique // target index; the loser lands here — adopt the winner's row and update it🤖 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 `@Core/Resgrid.Services/CheckInTimerService.cs` around lines 91 - 109, The catch block in CheckInTimerService that handles the insert race (the catch (Exception) when (isInsert) around the _configRepository.SaveOrUpdateAsync call) should log the caught exception and context before attempting recovery; change the handler to catch (Exception ex) when (isInsert) and use Resgrid.Framework.Logging to write an error including the exception and identifying details (config.DepartmentId, config.TimerTargetType, config.UnitTypeId and that we are recovering by calling GetByDepartmentAndTargetAsync and re-saving via SaveOrUpdateAsync) so failures and race recoveries are visible in logs, then continue with the existing winner-adoption logic.Source: Coding guidelines
🤖 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 `@Core/Resgrid.Services/CheckInTimerService.cs`:
- Around line 547-557: The ValidateTimerValues method currently checks enums and
minimums but not that the warning window is shorter than the total duration;
update ValidateTimerValues (method name) to validate that
warningThresholdMinutes < durationMinutes and throw an InvalidOperationException
(same exception type used) with a clear message like "Check-in timer warning
threshold must be less than the duration." so misconfigurations where
warningThresholdMinutes >= durationMinutes are rejected.
In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs`:
- Around line 374-386: The SaveSettings validation currently rejects overnight
schedules by returning BadRequest when entry.EndHour <= entry.StartHour; update
WeatherAlertsController.SaveSettings to allow overnight windows (where EndHour <
StartHour) but still reject empty windows (EndHour == StartHour) for enabled
entries: keep the existing range checks for entry.StartHour (0-23) and
entry.EndHour (0-24), and change the enabled-window check to only return
BadRequest when entry.Enabled && entry.EndHour == entry.StartHour. Reference
input.AutoMessageSchedule and the entry.StartHour/entry.EndHour/entry.Enabled
fields when making the change.
In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs`:
- Around line 2072-2079: The DepartmentController method that calls
_checkInTimerService.SaveTimerOverrideAsync currently only catches
InvalidOperationException; to match CheckInTimersController.SaveTimerOverride
add an additional catch for UnauthorizedAccessException and return
Unauthorized() (or BadRequest with the exception message if project convention
differs) so unauthorized errors are handled consistently—update the try/catch
around the call to _checkInTimerService.SaveTimerOverrideAsync in the
DepartmentController method to include a catch (UnauthorizedAccessException ex)
and map it to the appropriate HTTP response.
- Around line 2016-2023: The DepartmentController method calling
_checkInTimerService.SaveTimerConfigAsync currently only catches
InvalidOperationException and returns BadRequest; add a catch for
UnauthorizedAccessException (same handling as in
CheckInTimersController.SaveTimerConfig) and return NotFound() to handle
service-layer authorization failures (e.g., when configId belongs to another
department) and avoid returning a 500.
In `@Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml`:
- Line 69: The help text in Settings.cshtml incorrectly says "the end hour must
be greater than the start hour"; update that copy to reflect that overnight
windows are supported by WeatherAlertService.GetAutoMessageDecision (i.e.,
delivery runs from start hour up to but not including end hour, and if StartHour
> EndHour it wraps overnight — e.g., 18–6 for 6pm–6am; use 0–24 for always).
Change the sentence to remove the restriction and add a brief example mentioning
the overnight wrap to match the service logic.
---
Outside diff comments:
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitConnection.cs`:
- Around line 31-77: After acquiring _semaphore in the connection creation
block, perform a second null-check on _connection and skip creating/overwriting
it if another thread already set it; specifically, in the try block after await
_semaphore.WaitAsync(), add if (_connection == null) { ...existing
create/fallback logic... } so ConnectionFactory/await
_factory.CreateConnectionAsync(clientName) only run when still necessary,
preserving the single _semaphore.Release() in the finally and preventing races
that overwrite/ leak connections.
---
Nitpick comments:
In `@Core/Resgrid.Services/CheckInTimerService.cs`:
- Around line 145-150: Replace the in-memory filtering of department overrides
with a direct repository query: instead of calling GetByDepartmentIdAsync and
then using departmentOverrides?.FirstOrDefault(...) to produce
existingForTarget, add/use a repository method like
GetByDepartmentAndTargetAsync (same pattern used for configs) that accepts
DepartmentId, CallTypeId, CallPriority, TimerTargetType and UnitTypeId and
returns the matching override; update both the initial check (where
existingForTarget is computed) and the race-recovery check (the later block that
repeats the filtering) to call this new method so we avoid loading all overrides
into memory.
- Around line 91-109: The catch block in CheckInTimerService that handles the
insert race (the catch (Exception) when (isInsert) around the
_configRepository.SaveOrUpdateAsync call) should log the caught exception and
context before attempting recovery; change the handler to catch (Exception ex)
when (isInsert) and use Resgrid.Framework.Logging to write an error including
the exception and identifying details (config.DepartmentId,
config.TimerTargetType, config.UnitTypeId and that we are recovering by calling
GetByDepartmentAndTargetAsync and re-saving via SaveOrUpdateAsync) so failures
and race recoveries are visible in logs, then continue with the existing
winner-adoption logic.
In `@Core/Resgrid.Services/WeatherAlertService.cs`:
- Around line 704-708: The overnight-window branch in WeatherAlertService (the
inWindow calculation that handles StartHour > EndHour) is effectively
unreachable because WeatherAlertsController.SaveSettings currently rejects
enabled schedules with EndHour <= StartHour; either remove the overnight branch
to clean up dead code (replace the conditional with the straightforward inWindow
= currentHour >= entry.StartHour && currentHour < entry.EndHour logic in the
method containing the inWindow calculation) or, if overnight schedules should be
supported, update WeatherAlertsController.SaveSettings validation (the checks
around lines referenced in that method) to allow StartHour > EndHour for enabled
entries and add/update unit tests to cover overnight scenarios.
🪄 Autofix (Beta)
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
Run ID: 341c02bb-c150-46db-a118-0215be107614
⛔ Files ignored due to path filters (1)
Tests/Resgrid.Tests/Services/CheckInTimerServiceTests.csis excluded by!**/Tests/**
📒 Files selected for processing (9)
Core/Resgrid.Services/CheckInTimerService.csCore/Resgrid.Services/WeatherAlertService.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitConnection.csWeb/Resgrid.Web.Services/Controllers/v4/CheckInTimersController.csWeb/Resgrid.Web.Services/Controllers/v4/UnitStatusController.csWeb/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.csWeb/Resgrid.Web/Areas/User/Controllers/DepartmentController.csWeb/Resgrid.Web/Areas/User/Controllers/DispatchController.csWeb/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // Validate the unit id up front: this runs before the try/catch and ModelState check below, so a | ||
| // non-numeric id would otherwise throw an unhandled FormatException (HTTP 500) instead of a 400. | ||
| if (!int.TryParse(stateInput.Id, out var unitId)) |
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
New Features
Improvements