Repository navigation
fix: Exceptions in hosted services causing runtime issues - #171
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe changes refactor hosted services to use primary constructors and convert async operations from async void to async Task. The base AbstractHostedService class is modified with a protected constructor and structured logging. The DoWork method becomes async with error handling and awaits the OnRun method. Child services (CleanupHostedService, ReminderHostedService, RepeatHostedService, SnoozeHostedService) adopt primary constructors and update OnRun signatures to return Task. RepeatHostedService includes significant logic refactoring for card processing, pending repeat filtering, and navigation population. A duplicate logger injection in UserSettings.razor is removed. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR addresses runtime issues caused by unhandled exceptions in hosted services by changing the OnRun method signature from async void to async Task and adding proper exception handling. The changes improve error handling and code quality across multiple hosted services.
Key Changes:
- Modified
OnRunmethod fromasync voidtoasync Taskin all hosted services to enable proper exception handling - Added try-catch block in
AbstractHostedService.DoWorkto catch and log exceptions from hosted service executions - Refactored logging statements to use structured logging instead of string interpolation
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Ticky.Web/Components/Pages/UserSettings.razor | Removed duplicate logger injection |
| Ticky.Internal/Services/Hosted/SnoozeHostedService.cs | Changed OnRun to async Task, updated to primary constructor, improved logging |
| Ticky.Internal/Services/Hosted/RepeatHostedService.cs | Changed OnRun to async Task, updated to primary constructor, optimized query execution |
| Ticky.Internal/Services/Hosted/ReminderHostedService.cs | Changed OnRun to async Task, updated to primary constructor |
| Ticky.Internal/Services/Hosted/CleanupHostedService.cs | Changed OnRun to async Task, updated to primary constructor |
| Ticky.Internal/Services/Hosted/AbstractHostedService.cs | Made constructor protected, changed OnRun signature to Task, added exception handling, improved logging |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Ticky.Internal/Services/Hosted/AbstractHostedService.cs (1)
28-31: Scope created for Logger resolution is never disposed.The scope created to resolve
ILogger<T>is not disposed, which may leak resources (e.g., scoped services resolved alongside the logger). Consider disposing it immediately after obtaining the logger.Proposed fix
- Logger = ServiceScopeFactory - .CreateScope() - .ServiceProvider.GetRequiredService<ILogger<T>>()!; + using (var scope = ServiceScopeFactory.CreateScope()) + { + Logger = scope.ServiceProvider.GetRequiredService<ILogger<T>>()!; + }Ticky.Internal/Services/Hosted/ReminderHostedService.cs (1)
44-44: Use structured logging instead of string interpolation.This file uses string interpolation for logging (lines 44, 53, 93, 103) while the base class and
SnoozeHostedServiceuse structured logging with placeholders. For consistency and to leverage log aggregation/filtering by parameter, prefer structured logging.Proposed fix for line 44
- Logger.LogError($"Failed to send reminder to {email}. Error: {ex}"); + Logger.LogError(ex, "Failed to send reminder to {Email}.", email);Similar changes should be applied to lines 53, 93, and 103.
Ticky.Internal/Services/Hosted/CleanupHostedService.cs (1)
40-42: Use structured logging instead of string interpolation for consistency.This service uses string interpolation for logging (lines 40-42, 75-77, 104-106) while the base class and
SnoozeHostedServiceuse structured logging. Consider updating for consistency.Proposed fix for lines 40-42
Logger.LogInformation( - $"{codesForDeletion.Count} codes have been deleted alongside {deletedAccounts} unconfirmed accounts." + "{CodesCount} codes have been deleted alongside {DeletedAccounts} unconfirmed accounts.", + codesForDeletion.Count, + deletedAccounts );Ticky.Internal/Services/Hosted/SnoozeHostedService.cs (1)
12-12: Redundant logger resolution.The base class
AbstractHostedService<T>already provides aLoggerproperty. Resolving another logger from the scope is unnecessary and creates inconsistency with other hosted services that use the inheritedLogger.Proposed fix
using var scope = ServiceScopeFactory.CreateScope(); var db = scope.ServiceProvider.GetRequiredService<DataContext>()!; - var logger = scope.ServiceProvider.GetRequiredService<ILogger<SnoozeHostedService>>()!; ... - logger.LogInformation("{ExpiredSnoozesCount} cards unsnoozed.", expiredSnoozes.Count); + Logger.LogInformation("{ExpiredSnoozesCount} cards unsnoozed.", expiredSnoozes.Count);Ticky.Internal/Services/Hosted/RepeatHostedService.cs (1)
78-79: Shared collection references for many-to-many relationships with tracked entities.Directly assigning
card.Assigneesandcard.LabelstonewCardcauses both entities to reference the same collection instances. Since both are many-to-many relationships (Card-User and Card-Label), this creates shared state between tracked entities and may lead to unexpected EF Core behavior when saving. The code explicitly creates new collection instances for Attachments, Reminders, and Subtasks in the same method but omits this for Assignees and Labels.Use
.ToList()to create independent collection instances:var newCard = new Card { ... - Assignees = card.Assignees, - Labels = card.Labels + Assignees = card.Assignees.ToList(), + Labels = card.Labels.ToList() };
♻️ Duplicate comments (1)
Ticky.Internal/Services/Hosted/RepeatHostedService.cs (1)
191-191: Structured logging correctly applied.The logging now uses structured format with placeholders, addressing the pattern established in this PR.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
Ticky.Internal/Services/Hosted/AbstractHostedService.csTicky.Internal/Services/Hosted/CleanupHostedService.csTicky.Internal/Services/Hosted/ReminderHostedService.csTicky.Internal/Services/Hosted/RepeatHostedService.csTicky.Internal/Services/Hosted/SnoozeHostedService.csTicky.Web/Components/Pages/UserSettings.razor
💤 Files with no reviewable changes (1)
- Ticky.Web/Components/Pages/UserSettings.razor
🧰 Additional context used
🧬 Code graph analysis (4)
Ticky.Internal/Services/Hosted/ReminderHostedService.cs (2)
Ticky.Internal/Services/Hosted/AbstractHostedService.cs (5)
AbstractHostedService(5-109)AbstractHostedService(14-31)Task(33-42)Task(44-54)Task(100-103)Ticky.Base/Constants.cs (2)
Constants(3-117)Limits(57-67)
Ticky.Internal/Services/Hosted/CleanupHostedService.cs (4)
Ticky.Internal/Services/Hosted/AbstractHostedService.cs (5)
AbstractHostedService(5-109)AbstractHostedService(14-31)Task(33-42)Task(44-54)Task(100-103)Ticky.Internal/Services/Hosted/RepeatHostedService.cs (1)
Task(8-193)Ticky.Internal/Services/Hosted/ReminderHostedService.cs (1)
Task(8-105)Ticky.Internal/Services/Hosted/SnoozeHostedService.cs (1)
Task(8-28)
Ticky.Internal/Services/Hosted/SnoozeHostedService.cs (3)
Ticky.Internal/Services/Hosted/AbstractHostedService.cs (5)
AbstractHostedService(5-109)AbstractHostedService(14-31)Task(33-42)Task(44-54)Task(100-103)Ticky.Base/Constants.cs (1)
Limits(57-67)Ticky.Internal/Services/Hosted/ReminderHostedService.cs (1)
Task(8-105)
Ticky.Internal/Services/Hosted/RepeatHostedService.cs (3)
Ticky.Internal/Services/Hosted/AbstractHostedService.cs (5)
AbstractHostedService(5-109)AbstractHostedService(14-31)Task(33-42)Task(44-54)Task(100-103)Ticky.Base/Constants.cs (2)
Constants(3-117)Limits(57-67)Ticky.Base/Entities/Card.cs (1)
DateTime(34-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (6)
Ticky.Internal/Services/Hosted/AbstractHostedService.cs (2)
56-75: Good improvement:async voidwith proper exception handling.Converting
DoWorktoasync voidwith a try-catch wrapper is the correct pattern for timer callbacks. This ensures exceptions inOnRunare logged rather than silently crashing the application or tearing down the process. Theawait OnRun()properly propagates the async flow.
100-103: Good change:OnRunnow returnsTask.This allows derived classes to properly implement async operations without the pitfalls of
async void.Ticky.Internal/Services/Hosted/ReminderHostedService.cs (1)
3-8: Good adoption of primary constructor and async Task pattern.The service correctly uses the primary constructor to inject dependencies and pass timing parameters to the base class. The
OnRunsignature properly returnsTaskto align with the base class contract.Ticky.Internal/Services/Hosted/CleanupHostedService.cs (1)
3-6: Good adoption of primary constructor and async Task pattern.The service correctly uses the primary constructor and returns
TaskfromOnRun, aligning with the base class changes.Ticky.Internal/Services/Hosted/SnoozeHostedService.cs (1)
3-8: Good adoption of primary constructor and async Task pattern.The service correctly uses the primary constructor pattern and returns
TaskfromOnRun. The structured logging at line 26 is correctly implemented.Ticky.Internal/Services/Hosted/RepeatHostedService.cs (1)
3-8: Good adoption of primary constructor and async Task pattern.The service correctly implements the primary constructor and async
Taskreturn type forOnRun.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.