[FEATURE] Background jobs#745
Conversation
…kers, and persistence
Reviewer's GuideThis PR introduces a complete lightweight background job processing framework—supporting in-memory and EF Core persistence, DI-scoped handlers, scheduling, retry/backoff, and monitoring—alongside a robust HttpClient wrapper with configurable retry policies, pluggable serialization, and seamless DI integration. Sequence diagram for job scheduling and execution (background job framework)sequenceDiagram
participant User as actor User
participant API as API/Service
participant JobDispatcher
participant JobStore
participant BackgroundWorker
participant JobHandler
User->>API: Request to enqueue job
API->>JobDispatcher: EnqueueAsync(handlerName, payload)
alt Persistent store
JobDispatcher->>JobStore: SaveAsync(job)
else In-memory
JobDispatcher->>BackgroundJobQueue: Enqueue(job)
end
BackgroundWorker->>JobStore: FetchNextAsync()
BackgroundWorker->>JobHandler: HandleAsync(payload)
alt Success
BackgroundWorker->>JobStore: MarkCompletedAsync(jobId)
else Failure with retry
BackgroundWorker->>JobStore: MarkRetryAsync(jobId, nextAttempt, attemptCount)
else Failure, no retry
BackgroundWorker->>JobStore: MarkFailedAsync(jobId, error)
end
Class diagram for the new background job frameworkclassDiagram
class JobEntity {
+Guid Id
+string HandlerName
+string Payload
+JobPriority Priority
+JobStatus Status
+DateTimeOffset? ScheduledAt
+int Attempt
+int MaxAttempt
+string? LastError
+DateTimeOffset CreatedAt
+DateTimeOffset UpdatedAt
+int RetryDelaySeconds
}
class IBackgroundJob {
+Task<JobResult> ExecuteAsync(CancellationToken)
}
class IBackgroundJobHandler~TData~ {
+Task<JobResult> HandleAsync(TData, CancellationToken)
}
class IJobStore {
+Task SaveAsync(JobEntity, CancellationToken)
+Task<JobEntity?> FetchNextAsync(CancellationToken)
+Task MarkCompletedAsync(Guid, CancellationToken)
+Task MarkFailedAsync(Guid, string, CancellationToken)
+Task MarkRetryAsync(Guid, DateTimeOffset?, int, CancellationToken)
}
class IJobScheduler {
+Task ScheduleAsync(string, object, TimeSpan, int, JobPriority)
+Task EnqueueAsync(string, object, int, JobPriority)
}
class JobHandlerRegistry {
+void Register<THandler, TData>(string)
+bool TryGet(string, out Type?, out Type?)
}
class BackgroundWorker {
+Task ExecuteAsync(CancellationToken)
}
class BackgroundJobQueue {
+void Enqueue(JobEntity)
+Task<JobEntity?> DequeueAsync(CancellationToken)
}
class JobDispatcher {
+Task<Guid> EnqueueAsync(string, object, int?, JobPriority)
+Task ScheduleAsync(string, object, TimeSpan, int?, JobPriority)
}
class InMemoryJobStore {
+Task SaveAsync(JobEntity, CancellationToken)
+Task<JobEntity?> FetchNextAsync(CancellationToken)
+Task MarkCompletedAsync(Guid, CancellationToken)
+Task MarkFailedAsync(Guid, string, CancellationToken)
+Task MarkRetryAsync(Guid, DateTimeOffset?, int, CancellationToken)
}
class EfCoreJobStore {
+Task SaveAsync(JobEntity, CancellationToken)
+Task<JobEntity?> FetchNextAsync(CancellationToken)
+Task MarkCompletedAsync(Guid, CancellationToken)
+Task MarkFailedAsync(Guid, string, CancellationToken)
+Task MarkRetryAsync(Guid, DateTimeOffset?, int, CancellationToken)
}
class JobDbContext {
+DbSet<JobEntity> Jobs
}
class JobOptions {
+int WorkerCount
+int DefaultMaxAttempt
+TimeSpan PollingInterval
+int MaxBackoffSeconds
}
class JobResult {
+bool Success
+bool Retry
+string? ErrorMessage
+static JobResult Ok()
+static JobResult Fail(string, bool)
}
class JobMetrics {
+void Increment(string)
+long Get(string)
+IDictionary<string, long> Snapshot()
}
class MonitoringMiddleware {
+Task InvokeAsync(HttpContext)
}
JobDbContext --> JobEntity
InMemoryJobStore --|> IJobStore
EfCoreJobStore --|> IJobStore
DelayedJobScheduler --|> IJobScheduler
JobHandlerRegistry --|> IJobHandlerRegistry
BackgroundWorker --> BackgroundJobQueue
BackgroundWorker --> IJobStore
BackgroundWorker --> JobHandlerRegistry
BackgroundWorker --> JobOptions
BackgroundWorker --> JobDispatcher
JobDispatcher --> BackgroundJobQueue
JobDispatcher --> IJobStore
MonitoringMiddleware --> JobMetrics
BackgroundJobQueue --> JobEntity
IBackgroundJobHandler <|.. JobHandlerRegistry
IBackgroundJob <|.. BackgroundWorker
JobEntity --> JobPriority
JobEntity --> JobStatus
Class diagram for the new HttpClient wrapperclassDiagram
class IHttpClientWrapper {
+Task<HttpResponse<T>> GetAsync<T>(string, HttpRequestOptions?, CancellationToken)
+Task<HttpResponse<TResponse>> PostAsync<TRequest, TResponse>(string, TRequest?, HttpRequestOptions?, CancellationToken)
+Task<HttpResponse<TResponse>> PutAsync<TRequest, TResponse>(string, TRequest?, HttpRequestOptions?, CancellationToken)
+Task<HttpResponse<object?>> DeleteAsync(string, HttpRequestOptions?, CancellationToken)
+HttpResponse<T> Get<T>(string, HttpRequestOptions?)
+HttpResponse<TResponse> Post<TRequest, TResponse>(string, TRequest?, HttpRequestOptions?)
+HttpResponse<TResponse> Put<TRequest, TResponse>(string, TRequest?, HttpRequestOptions?)
+HttpResponse<object?> Delete(string, HttpRequestOptions?)
}
class HttpClientWrapper {
+Task<HttpResponse<T>> GetAsync<T>(...)
+Task<HttpResponse<TResponse>> PostAsync<TRequest, TResponse>(...)
+Task<HttpResponse<TResponse>> PutAsync<TRequest, TResponse>(...)
+Task<HttpResponse<object?>> DeleteAsync(...)
+HttpResponse<T> Get<T>(...)
+HttpResponse<TResponse> Post<TRequest, TResponse>(...)
+HttpResponse<TResponse> Put<TRequest, TResponse>(...)
+HttpResponse<object?> Delete(...)
}
class HttpRequestOptions {
+string? BaseAddress
+int TimeoutSeconds
+int RetryCount
+int RetryBaseDelayMs
+int MaxRetryDelayMs
+bool ThrowOnError
+IDictionary<string, string>? DefaultRequestHeaders
+string? NamedHttpClient
+bool UseJsonContentType
+IJsonSerializer? Serializer
}
class HttpResponse~T~ {
+bool IsSuccess
+HttpStatusCode? StatusCode
+T? Data
+string? RawContent
+IList<string>? Errors
+IDictionary<string, IEnumerable<string>>? Headers
+TimeSpan Duration
+static HttpResponse<T> Success(...)
+static HttpResponse<T> Failure(...)
}
class IJsonSerializer {
+string Serialize<T>(T)
+T? Deserialize<T>(string)
+object? Deserialize(string, Type)
}
class SystemTextJsonSerializer {
+string Serialize<T>(T)
+T? Deserialize<T>(string)
+object? Deserialize(string, Type)
}
class HttpClientException {
+HttpStatusCode? StatusCode
+string? ResponseContent
}
HttpClientWrapper --|> IHttpClientWrapper
SystemTextJsonSerializer --|> IJsonSerializer
HttpClientWrapper --> HttpRequestOptions
HttpClientWrapper --> IJsonSerializer
HttpClientWrapper --> HttpResponse~T~
HttpClientWrapper --> HttpClientException
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
WalkthroughUpdates centralized dependency pins in ChangesPackage Versions
Estimated code review effort: 1 (Trivial) | ~2 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![gstraccini[bot]](https://avatars.githubusercontent.com/in/480132?s=60&v=4){kind=small}
gstraccini
Bot
added
the
🚦 awaiting triage
penify-dev
Bot
added
enhancement
PR Review 🔍
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `Src/CrispyWaffle.BackgroundJobs/Core/BackgroundWorker.cs:78` </location>
<code_context>
+ if (job == null) continue;
+
+ // If job has ScheduledAt in future, re-enqueue / re-schedule.
+ if (job.ScheduledAt.HasValue && job.ScheduledAt.Value > DateTimeOffset.UtcNow)
+ {
+ // For persistence, mark back to pending with same ScheduledAt; for in-memory, just re-enqueue.
+ if (_store != null)
+ {
+ // mark as Pending again (store.FetchNextAsync should have returned only due jobs, this is defensive)
+ await _store.MarkRetryAsync(job.Id, job.ScheduledAt, job.Attempt, stoppingToken);
+ }
+ else
+ {
+ _queue!.Enqueue(job);
+ }
+
+ continue;
+ }
+
</code_context>
<issue_to_address>
Jobs scheduled in the future are immediately re-enqueued, which may cause tight polling.
The worker loop does not wait before rechecking jobs scheduled for the future, leading to unnecessary CPU usage. Consider implementing a delay based on the job's scheduled time to reduce tight polling.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// If job has ScheduledAt in future, re-enqueue / re-schedule.
if (job.ScheduledAt.HasValue && job.ScheduledAt.Value > DateTimeOffset.UtcNow)
{
// For persistence, mark back to pending with same ScheduledAt; for in-memory, just re-enqueue.
if (_store != null)
{
// mark as Pending again (store.FetchNextAsync should have returned only due jobs, this is defensive)
await _store.MarkRetryAsync(job.Id, job.ScheduledAt, job.Attempt, stoppingToken);
}
else
{
_queue!.Enqueue(job);
}
continue;
}
=======
// If job has ScheduledAt in future, delay until due, then re-enqueue / re-schedule.
if (job.ScheduledAt.HasValue && job.ScheduledAt.Value > DateTimeOffset.UtcNow)
{
var delay = job.ScheduledAt.Value - DateTimeOffset.UtcNow;
if (delay > TimeSpan.Zero)
{
await Task.Delay(delay, stoppingToken);
}
// For persistence, mark back to pending with same ScheduledAt; for in-memory, just re-enqueue.
if (_store != null)
{
// mark as Pending again (store.FetchNextAsync should have returned only due jobs, this is defensive)
await _store.MarkRetryAsync(job.Id, job.ScheduledAt, job.Attempt, stoppingToken);
}
else
{
_queue!.Enqueue(job);
}
continue;
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `Src/CrispyWaffle.BackgroundJobs/Core/BackgroundWorker.cs:221` </location>
<code_context>
+ }
+ }
+
+ private static int ComputeBackoffSeconds(int attempt)
+ {
+ // exponential backoff with cap
+ var seconds = (int)Math.Pow(2, Math.Min(attempt, 10));
+ return Math.Min(seconds, 300);
+ }
+
</code_context>
<issue_to_address>
Backoff calculation is capped at 300 seconds, but this is hardcoded.
If JobOptions.MaxBackoffSeconds is meant to be configurable, use it here instead of the hardcoded 300 to support flexible retry strategies.
Suggested implementation:
```csharp
job.Attempt++;
job.ScheduledAt = DateTimeOffset.UtcNow.AddSeconds(ComputeBackoffSeconds(job.Attempt, _options.MaxBackoffSeconds));
_queue!.Enqueue(job);
```
```csharp
private static int ComputeBackoffSeconds(int attempt, int maxBackoffSeconds)
{
// exponential backoff with cap
var seconds = (int)Math.Pow(2, Math.Min(attempt, 10));
return Math.Min(seconds, maxBackoffSeconds);
}
```
</issue_to_address>
### Comment 3
<location> `Src/CrispyWaffle.HttpClient/HttpClientWrapper.cs:35` </location>
<code_context>
+ private System.Net.Http.HttpClient CreateClient(HttpRequestOptions? perRequest)
</code_context>
<issue_to_address>
Headers are merged and set on HttpClient, which may persist across requests.
Setting headers on DefaultRequestHeaders can cause them to persist across requests when reusing the client. To avoid this, set headers on HttpRequestMessage for per-request isolation.
</issue_to_address>
### Comment 4
<location> `Src/CrispyWaffle.HttpClient/RetryPolicy.cs:85` </location>
<code_context>
+ return false;
+ }
+
+ private static bool IsTransientException(Exception ex)
+ {
+ // HttpRequestException and TaskCanceled (timeout) are considered transient
+ return ex is HttpRequestException || ex is OperationCanceledException || ex is TimeoutException;
+ }
+
</code_context>
<issue_to_address>
OperationCanceledException is treated as transient, which may mask cancellation.
Check the cancellation token before retrying to ensure user-initiated cancellations are not retried as transient errors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // If job has ScheduledAt in future, re-enqueue / re-schedule. | ||
| if (job.ScheduledAt.HasValue && job.ScheduledAt.Value > DateTimeOffset.UtcNow) | ||
| { | ||
| // For persistence, mark back to pending with same ScheduledAt; for in-memory, just re-enqueue. | ||
| if (_store != null) | ||
| { | ||
| // mark as Pending again (store.FetchNextAsync should have returned only due jobs, this is defensive) | ||
| await _store.MarkRetryAsync(job.Id, job.ScheduledAt, job.Attempt, stoppingToken); | ||
| } | ||
| else | ||
| { | ||
| _queue!.Enqueue(job); | ||
| } | ||
|
|
||
| continue; | ||
| } |
There was a problem hiding this comment.
suggestion (performance): Jobs scheduled in the future are immediately re-enqueued, which may cause tight polling.
The worker loop does not wait before rechecking jobs scheduled for the future, leading to unnecessary CPU usage. Consider implementing a delay based on the job's scheduled time to reduce tight polling.
| // If job has ScheduledAt in future, re-enqueue / re-schedule. | |
| if (job.ScheduledAt.HasValue && job.ScheduledAt.Value > DateTimeOffset.UtcNow) | |
| { | |
| // For persistence, mark back to pending with same ScheduledAt; for in-memory, just re-enqueue. | |
| if (_store != null) | |
| { | |
| // mark as Pending again (store.FetchNextAsync should have returned only due jobs, this is defensive) | |
| await _store.MarkRetryAsync(job.Id, job.ScheduledAt, job.Attempt, stoppingToken); | |
| } | |
| else | |
| { | |
| _queue!.Enqueue(job); | |
| } | |
| continue; | |
| } | |
| // If job has ScheduledAt in future, delay until due, then re-enqueue / re-schedule. | |
| if (job.ScheduledAt.HasValue && job.ScheduledAt.Value > DateTimeOffset.UtcNow) | |
| { | |
| var delay = job.ScheduledAt.Value - DateTimeOffset.UtcNow; | |
| if (delay > TimeSpan.Zero) | |
| { | |
| await Task.Delay(delay, stoppingToken); | |
| } | |
| // For persistence, mark back to pending with same ScheduledAt; for in-memory, just re-enqueue. | |
| if (_store != null) | |
| { | |
| // mark as Pending again (store.FetchNextAsync should have returned only due jobs, this is defensive) | |
| await _store.MarkRetryAsync(job.Id, job.ScheduledAt, job.Attempt, stoppingToken); | |
| } | |
| else | |
| { | |
| _queue!.Enqueue(job); | |
| } | |
| continue; | |
| } |
| private static int ComputeBackoffSeconds(int attempt) | ||
| { | ||
| // exponential backoff with cap | ||
| var seconds = (int)Math.Pow(2, Math.Min(attempt, 10)); | ||
| return Math.Min(seconds, 300); |
There was a problem hiding this comment.
suggestion: Backoff calculation is capped at 300 seconds, but this is hardcoded.
If JobOptions.MaxBackoffSeconds is meant to be configurable, use it here instead of the hardcoded 300 to support flexible retry strategies.
Suggested implementation:
job.Attempt++;
job.ScheduledAt = DateTimeOffset.UtcNow.AddSeconds(ComputeBackoffSeconds(job.Attempt, _options.MaxBackoffSeconds));
_queue!.Enqueue(job); private static int ComputeBackoffSeconds(int attempt, int maxBackoffSeconds)
{
// exponential backoff with cap
var seconds = (int)Math.Pow(2, Math.Min(attempt, 10));
return Math.Min(seconds, maxBackoffSeconds);
}| private System.Net.Http.HttpClient CreateClient(HttpRequestOptions? perRequest) | ||
| { | ||
| var name = perRequest?.NamedHttpClient ?? _defaults.NamedHttpClient; | ||
| System.Net.Http.HttpClient client = string.IsNullOrEmpty(name) | ||
| ? _httpClientFactory.CreateClient() | ||
| : _httpClientFactory.CreateClient(name); | ||
|
|
||
| var baseAddr = perRequest?.BaseAddress ?? _defaults.BaseAddress; | ||
| if (!string.IsNullOrEmpty(baseAddr) && client.BaseAddress == null) | ||
| { |
There was a problem hiding this comment.
issue (bug_risk): Headers are merged and set on HttpClient, which may persist across requests.
Setting headers on DefaultRequestHeaders can cause them to persist across requests when reusing the client. To avoid this, set headers on HttpRequestMessage for per-request isolation.
| private static bool IsTransientException(Exception ex) | ||
| { | ||
| // HttpRequestException and TaskCanceled (timeout) are considered transient | ||
| return ex is HttpRequestException || ex is OperationCanceledException || ex is TimeoutException; |
There was a problem hiding this comment.
issue (bug_risk): OperationCanceledException is treated as transient, which may mask cancellation.
Check the cancellation token before retrying to ensure user-initiated cancellations are not retried as transient errors.
PR Code Suggestions ✨
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
♻️ Duplicate comments (4)
Src/CrispyWaffle.HttpClient/RetryPolicy.cs (2)
62-70: Respect cancellation: don’t treat OperationCanceledException as transient when the token is canceled.Retrying after user-requested cancellation is incorrect and may mask cancellations. Also, include a dedicated catch for cancellation.
Apply this diff:
- catch (Exception ex) when (IsTransientException(ex)) + // If the CancellationToken was canceled, propagate immediately + catch (OperationCanceledException oce) when (ct.IsCancellationRequested) + { + return (null, oce); + } + // Treat timeouts/cancelled tasks (not due to ct) and HttpRequestException as transient + catch (OperationCanceledException oce) when (!ct.IsCancellationRequested) + { + lastEx = oce; + if (attempt == retryCount) break; + var delay = ComputeDelay(attempt, baseDelayMs, maxDelayMs, rnd); + await Task.Delay(delay, ct).ConfigureAwait(false); + continue; + } + catch (Exception ex) when (IsTransientException(ex)) { lastEx = ex; if (attempt == retryCount) break; var delay = ComputeDelay(attempt, baseDelayMs, maxDelayMs, rnd); await Task.Delay(delay, ct).ConfigureAwait(false); continue; }And narrow IsTransientException accordingly (see next comment).
85-89: Narrow transient exceptions to avoid conflating cancellations.Move OperationCanceledException handling to explicit catch blocks and keep only true transient types here.
Apply this diff:
- private static bool IsTransientException(Exception ex) + private static bool IsTransientException(Exception ex) { - // HttpRequestException and TaskCanceled (timeout) are considered transient - return ex is HttpRequestException || ex is OperationCanceledException || ex is TimeoutException; + // HttpRequestException and Timeout are considered transient + return ex is HttpRequestException || ex is TimeoutException; }Src/CrispyWaffle.BackgroundJobs/Core/BackgroundWorker.cs (1)
77-92: Re-check “future scheduled job” handling (duplicate of earlier feedback)Re-enqueueing a not-due job without delay can cause unnecessary churn. The retry path fix above addresses the main spinner; consider applying a short
Task.Delayuntil due here as well, or simply continue processing after an awaited delay.Src/CrispyWaffle.HttpClient/HttpClientWrapper.cs (1)
35-87: Avoid mutating shared HttpClient (BaseAddress, Timeout, DefaultRequestHeaders) per requestClients from IHttpClientFactory are pooled and reused. Setting BaseAddress, Timeout, or DefaultRequestHeaders here causes cross-request side effects and header “bleed” between callers.
Refactor CreateClient to only select the named client and leave configuration to named-client setup; move per-request concerns to the request message:
- private System.Net.Http.HttpClient CreateClient(HttpRequestOptions? perRequest) - { - var name = perRequest?.NamedHttpClient ?? _defaults.NamedHttpClient; - System.Net.Http.HttpClient client = string.IsNullOrEmpty(name) - ? _httpClientFactory.CreateClient() - : _httpClientFactory.CreateClient(name); - - var baseAddr = perRequest?.BaseAddress ?? _defaults.BaseAddress; - if (!string.IsNullOrEmpty(baseAddr) && client.BaseAddress == null) - { - client.BaseAddress = new Uri(baseAddr); - } - - var timeoutSeconds = perRequest?.TimeoutSeconds ?? _defaults.TimeoutSeconds; - client.Timeout = TimeSpan.FromSeconds(Math.Max(1, timeoutSeconds)); - - // Merge default headers (global) and per-request headers - var headers = new Dictionary<string, string>(); - if (_defaults.DefaultRequestHeaders != null) - { - foreach (var kv in _defaults.DefaultRequestHeaders) - headers[kv.Key] = kv.Value; - } - if (perRequest?.DefaultRequestHeaders != null) - { - foreach (var kv in perRequest.DefaultRequestHeaders) - headers[kv.Key] = kv.Value; - } - - // Set headers (be careful to not duplicate) - foreach (var kv in headers) - { - if (!client.DefaultRequestHeaders.Contains(kv.Key)) - { - try - { - client.DefaultRequestHeaders.Add(kv.Key, kv.Value); - } - catch (Exception ex) - { - _logger?.LogWarning(ex, "Failed to set default header {Header}", kv.Key); - } - } - } - - // Ensure Accept header - if (client.DefaultRequestHeaders.Accept.Count == 0) - { - client.DefaultRequestHeaders.Accept.Add(new System.Net.Http.Headers.MediaTypeWithQualityHeaderValue("application/json")); - } - - return client; - } + private System.Net.Http.HttpClient CreateClient(HttpRequestOptions? perRequest) + { + var name = perRequest?.NamedHttpClient ?? _defaults.NamedHttpClient; + return string.IsNullOrEmpty(name) + ? _httpClientFactory.CreateClient() + : _httpClientFactory.CreateClient(name); + }Follow-up changes for per-request BaseAddress/Timeout/Headers are proposed below.
🧹 Nitpick comments (51)
Src/CrispyWaffle.BackgroundJobs/Scheduling/CronJobScheduler.cs (5)
7-10: Clarify XML docs to match the current behavior (interval-based stub, not CRON).The summary mentions CRON expressions, but the implementation is an every-minute loop. Adjust the docs to avoid misleading consumers.
- /// CronJobScheduler is a lightweight helper that triggers an action on a CRON expression. - /// Note: to support CRON syntax you'd typically add a dependency such as NCrontab or Cronos. - /// This is a stub showing how to wire a hosted service cron runner. + /// CronJobService is a lightweight base that runs ExecuteOnce on a fixed interval (default: 1 minute). + /// To support real CRON expressions, consider a library such as NCrontab or Cronos. + /// This is a stub demonstrating how to wire a hosted-service "cron-like" runner.
25-34: Graceful cancellation: don’t log errors on shutdown; include job name in logs.If ExecuteOnce honors cancellation and throws OperationCanceledException, we shouldn’t log it as an error. Also, include the job name (derived type) in logs for better observability.
- try - { - await ExecuteOnce(stoppingToken); - } - catch (Exception ex) - { - _logger.LogError(ex, "Cron job execution error"); - } + try + { + await ExecuteOnce(stoppingToken); + } + catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested) + { + // Graceful shutdown + _logger.LogInformation("Cron job {Job} was cancelled", GetType().Name); + break; + } + catch (Exception ex) + { + _logger.LogError(ex, "Cron job {Job} execution error"); + }
11-19: Make the run interval configurable (avoid hard-coding one minute).Give derived jobs flexibility to choose their cadence. Expose a virtual Interval property with a sensible default and use it in the delay.
public abstract class CronJobService : BackgroundService { - private readonly ILogger _logger; + private readonly ILogger<CronJobService> _logger; protected CronJobService(ILogger logger) { _logger = logger; } + + /// <summary> + /// The interval between ExecuteOnce runs. Override to customize cadence. + /// </summary> + protected virtual TimeSpan Interval => TimeSpan.FromMinutes(1);And update the delay below:
- await Task.Delay(TimeSpan.FromMinutes(1), stoppingToken); + await Task.Delay(Interval, stoppingToken);
20-36: Optional: Use PeriodicTimer for more precise, drift-resistant intervals.PeriodicTimer avoids accumulating drift that can happen with Task.Delay after work completes and simplifies cancellation.
protected override async Task ExecuteAsync(CancellationToken stoppingToken) { using var timer = new PeriodicTimer(Interval); while (await timer.WaitForNextTickAsync(stoppingToken)) { try { await ExecuteOnce(stoppingToken); } catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested) { _logger.LogInformation("Cron job {Job} was cancelled", GetType().Name); break; } catch (Exception ex) { _logger.LogError(ex, "Cron job {Job} execution error"); } } }Note: Requires
using System.Threading;and targeting .NET 6+.
4-4: Nit: file/class naming consistency.The file is CronJobScheduler.cs but the class is CronJobService. Consider renaming the file to match the type for consistency with common C# conventions.
Src/CrispyWaffle.HttpClient/CrispyWaffle.HttpClient.csproj (2)
3-7: Consider enabling XML docs for the package (if you plan to publish).If this library is intended for NuGet distribution, generating XML docs helps consumers with IntelliSense and API docs.
Apply this diff:
<PropertyGroup> <TargetFramework>net8.0</TargetFramework> <ImplicitUsings>enable</ImplicitUsings> <Nullable>enable</Nullable> + <GenerateDocumentationFile>true</GenerateDocumentationFile> </PropertyGroup>
10-10: If you plan to use policy-based retries, add Microsoft.Extensions.Http.Polly.Given the PR mentions retry/backoff for the HTTP client wrapper, verify whether you’re relying purely on custom logic or want to leverage Polly handlers via IHttpClientFactory. If the latter, add the Polly package.
Apply this diff:
<ItemGroup> <PackageReference Include="Microsoft.Extensions.Http" /> + <PackageReference Include="Microsoft.Extensions.Http.Polly" /> </ItemGroup>Src/CrispyWaffle.BackgroundJobs/Abstractions/JobPriority.cs (2)
3-8: Default enum value 0 maps to High; confirm this is intentional.In C#, enum default is 0. With High = 0, any uninitialized JobPriority will be High. The PR summary indicates Normal as the default when scheduling. If there’s any code path that doesn’t explicitly set Priority (e.g., constructing JobEntity directly), jobs could be inadvertently enqueued as High priority.
Two options:
- Keep the current values and ensure all JobEntity instances default the property to Normal explicitly.
- Or, make Normal = 0 so the default is Normal (but then adjust queue semantics accordingly).
If you keep the current enum values, ensure JobEntity sets a default:
// In JobEntity.cs public JobPriority Priority { get; set; } = JobPriority.Normal;
3-8: Add XML docs to clarify priority ordering.Documenting that lower numeric values represent higher priority will avoid confusion when reading or maintaining the code.
Apply this diff:
namespace CrispyWaffle.BackgroundJobs.Abstractions { public enum JobPriority { - High = 0, - Normal = 1, - Low = 2 + /// <summary> + /// Highest priority. Lower numeric value means higher priority in the queue. + /// </summary> + High = 0, + /// <summary> + /// Default priority for most jobs. + /// </summary> + Normal = 1, + /// <summary> + /// Lowest priority. + /// </summary> + Low = 2 } }Src/CrispyWaffle.BackgroundJobs/Exceptions/JobFailedException.cs (1)
3-6: Provide standard exception constructors and seal the type.Adding the default and inner-exception constructors improves ergonomics and preserves exception causality. Sealing avoids unintended inheritance for a domain-specific exception.
Apply this diff:
namespace CrispyWaffle.BackgroundJobs.Exceptions { - public class JobFailedException : Exception + /// <summary> + /// Represents a failure in a background job execution pipeline. + /// </summary> + public sealed class JobFailedException : Exception { - public JobFailedException(string message) : base(message) { } + /// <summary> + /// Initializes a new instance of the <see cref="JobFailedException"/> class. + /// </summary> + public JobFailedException() { } + + /// <summary> + /// Initializes a new instance of the <see cref="JobFailedException"/> class with a specified error message. + /// </summary> + public JobFailedException(string message) : base(message) { } + + /// <summary> + /// Initializes a new instance of the <see cref="JobFailedException"/> class with a specified error message and a reference to the inner exception that is the cause of this exception. + /// </summary> + public JobFailedException(string message, Exception? innerException) : base(message, innerException) { } } }Src/CrispyWaffle.BackgroundJobs/CrispyWaffle.BackgroundJobs.csproj (1)
10-18: Trim dependencies if possible (prefer abstractions, avoid duplicates).
- You reference both Microsoft.Extensions.DependencyInjection and Microsoft.Extensions.DependencyInjection.Abstractions. If you only need IServiceCollection and define your own extension methods without calling framework extension methods (like AddHostedService/AddOptions), Abstractions alone is sufficient. Otherwise, keep the full package.
- For logging, consider depending on Microsoft.Extensions.Logging.Abstractions to reduce transitive dependencies if you only use ILogger.
Option A (keep as-is if using DI/hosting extension methods):
- No change.
Option B (if not needed, swap to abstractions-only):
<ItemGroup> <PackageReference Include="Microsoft.AspNetCore.Hosting.Abstractions" /> <PackageReference Include="Microsoft.EntityFrameworkCore" /> - <PackageReference Include="Microsoft.Extensions.DependencyInjection" /> <PackageReference Include="Microsoft.EntityFrameworkCore.Relational" /> <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" /> <PackageReference Include="Microsoft.Extensions.Hosting.Abstractions" /> - <PackageReference Include="Microsoft.Extensions.Logging" /> + <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" /> <PackageReference Include="Microsoft.Extensions.Options" /> </ItemGroup>Src/CrispyWaffle.BackgroundJobs/Core/JobOptions.cs (3)
20-23: Unify time-based options as TimeSpan (avoid raw seconds).MaxBackoffSeconds as an int diverges from PollingInterval (TimeSpan). Using TimeSpan for both improves clarity and prevents unit mismatch.
Apply this diff:
/// <summary> - /// Maximum exponential backoff seconds for retry. + /// Maximum exponential backoff duration for retry. /// </summary> - public int MaxBackoffSeconds { get; set; } = 300; // 5 minutes + public TimeSpan MaxBackoff { get; set; } = TimeSpan.FromMinutes(5);Follow-up: update call sites to use JobOptions.MaxBackoff instead of MaxBackoffSeconds.
3-24: Add options validation to guard against misconfiguration.Guard rails like WorkerCount >= 1, DefaultMaxAttempt >= 0, PollingInterval > 0, and MaxBackoff >= PollingInterval (optional) help avoid runtime surprises.
You can add a validator in a separate file and register it via services.AddOptions().Validate(...) or IValidateOptions:
// e.g., Src/CrispyWaffle.BackgroundJobs/Core/JobOptionsValidation.cs using Microsoft.Extensions.Options; namespace CrispyWaffle.BackgroundJobs.Core; public sealed class JobOptionsValidation : IValidateOptions<JobOptions> { public ValidateOptionsResult Validate(string? name, JobOptions options) { if (options.WorkerCount < 1) return ValidateOptionsResult.Fail("WorkerCount must be >= 1."); if (options.DefaultMaxAttempt < 0) return ValidateOptionsResult.Fail("DefaultMaxAttempt must be >= 0."); if (options.PollingInterval <= TimeSpan.Zero) return ValidateOptionsResult.Fail("PollingInterval must be > 0."); // If you adopt TimeSpan MaxBackoff: // if (options.MaxBackoff <= TimeSpan.Zero) // return ValidateOptionsResult.Fail("MaxBackoff must be > 0."); return ValidateOptionsResult.Success; } }And register in DI:
services.AddOptions<JobOptions>(); services.AddSingleton<IValidateOptions<JobOptions>, JobOptionsValidation>();
8-8: Double-check default WorkerCount in containerized environments.Environment.ProcessorCount reports host cores, not CPU limits in containers. That can oversubscribe threads. Consider documenting this and encouraging configuration override, or cap with a sane maximum for defaults.
Src/CrispyWaffle.BackgroundJobs/Core/JobResult.cs (1)
3-7: Optional: add a dedicated factory for retryable failures to improve call-site readabilityAvoiding a boolean flag at call sites makes intent clearer.
Apply this diff to add a convenience factory:
public sealed record JobResult(bool Success, bool Retry = false, string? ErrorMessage = null) { public static JobResult Ok() => new(true); public static JobResult Fail(string message, bool retry = false) => new(false, retry, message); + public static JobResult RetryableFail(string message) => new(false, true, message); }Src/CrispyWaffle.BackgroundJobs/Abstractions/IBackgroundJobHandler.cs (1)
9-12: Consider contravariance and non-null constraint on TDataThis improves flexibility (allowing handlers for base types) and documents nullability intent of payloads.
Apply this diff:
- public interface IBackgroundJobHandler<TData> + public interface IBackgroundJobHandler<in TData> + where TData : notnull { Task<JobResult> HandleAsync(TData data, CancellationToken cancellationToken); }Src/CrispyWaffle.BackgroundJobs/Abstractions/IBackgroundJob.cs (1)
9-12: Optional: consider ValueTask if many implementations complete synchronouslyIf common implementations are fast/synchronous, ValueTask can reduce allocations on hot paths. If most are truly async (I/O), keep Task.
Src/CrispyWaffle.HttpClient/Exceptions/HttpClientException.cs (1)
6-16: Base on HttpRequestException and seal the typeDeriving from HttpRequestException makes it easier for callers to handle all HTTP-related failures uniformly. Sealing prevents accidental subclassing of an exception type.
Apply this diff:
- public class HttpClientException : Exception + public sealed class HttpClientException : System.Net.Http.HttpRequestException { public HttpStatusCode? StatusCode { get; } public string? ResponseContent { get; } public HttpClientException(string message, HttpStatusCode? statusCode = null, string? responseContent = null, Exception? inner = null) - : base(message, inner) + : base(message, inner) { StatusCode = statusCode; ResponseContent = responseContent; } }Src/CrispyWaffle.BackgroundJobs/Monitoring/JobMetrics.cs (2)
9-16: Validate keys and return a read-only snapshot to harden the APIAdds guardrails and expresses immutability to consumers. Minor change with low risk if usages rely on enumeration only.
Apply this diff:
- public void Increment(string key) + public void Increment(string key) { - _counters.AddOrUpdate(key, 1, (_, v) => v + 1); + if (string.IsNullOrWhiteSpace(key)) + { + throw new System.ArgumentException("Key cannot be null or whitespace.", nameof(key)); + } + _counters.AddOrUpdate(key, 1, (_, v) => v + 1); } @@ - public System.Collections.Generic.IDictionary<string, long> Snapshot() => new System.Collections.Generic.Dictionary<string, long>(_counters); + public System.Collections.Generic.IReadOnlyDictionary<string, long> Snapshot() => + new System.Collections.Generic.Dictionary<string, long>(_counters);
7-16: Watch out for unbounded key cardinality in productionIf keys include dynamic values (e.g., job IDs), the dictionary can grow unbounded. Prefer a curated, finite key set (e.g., by job type/status) or add eviction/TTL if needed.
Src/CrispyWaffle.BackgroundJobs/Monitoring/MonitoringMiddleware.cs (3)
11-15: Guard against null dependencies in middleware constructorAdd argument validation to fail fast if DI wiring is incorrect, preventing later null reference exceptions.
- public MonitoringMiddleware(RequestDelegate next, JobMetrics metrics) - { - _next = next; - _metrics = metrics; - } + public MonitoringMiddleware(RequestDelegate next, JobMetrics metrics) + { + _next = next ?? throw new ArgumentNullException(nameof(next)); + _metrics = metrics ?? throw new ArgumentNullException(nameof(metrics)); + }
19-23: Limit endpoint to GET and honor request abort tokenConstrain the metrics endpoint to GET requests and pass the RequestAborted token to WriteAsync for cooperative cancellation.
- if (context.Request.Path.StartsWithSegments("/jobs/metrics")) + if (HttpMethods.IsGet(context.Request.Method) && + context.Request.Path.StartsWithSegments("/jobs/metrics")) { context.Response.ContentType = "application/json"; - await context.Response.WriteAsync(JsonSerializer.Serialize(_metrics.Snapshot())); + await context.Response.WriteAsync( + JsonSerializer.Serialize(_metrics.Snapshot()), + context.RequestAborted); return; }
17-27: Consider securing or making the metrics endpoint configurableExposing /jobs/metrics unprotected can leak operational data in production. Consider one or more:
- Make the path configurable via options (e.g., MonitoringOptions.Path).
- Allow injection of an auth predicate or require an authorization policy.
- Optionally disable by default and require explicit opt-in.
I can sketch an options class and the minimal wiring (IOptions) if you’d like.
Src/CrispyWaffle.HttpClient/HttpResponse{T}.cs (2)
10-18: Prefer immutability: seal the type and switch to init-only settersSealing and using init-only setters make the response immutable after construction, preventing accidental mutation by consumers.
- public class HttpResponse<T> + public sealed class HttpResponse<T> { - public bool IsSuccess { get; set; } - public HttpStatusCode? StatusCode { get; set; } - public T? Data { get; set; } - public string? RawContent { get; set; } - public IList<string>? Errors { get; set; } - public IDictionary<string, IEnumerable<string>>? Headers { get; set; } - public TimeSpan Duration { get; set; } + public bool IsSuccess { get; init; } + public HttpStatusCode? StatusCode { get; init; } + public T? Data { get; init; } + public string? RawContent { get; init; } + public IList<string>? Errors { get; init; } + public IDictionary<string, IEnumerable<string>>? Headers { get; init; } + public TimeSpan Duration { get; init; } }
1-1: Nit: consider renaming the fileUsing braces in filenames (HttpResponse{T}.cs) can be awkward with tooling. Consider HttpResponse.T.cs or HttpResponse`1.cs.
Src/CrispyWaffle.HttpClient/Serialization/IJsonSerializer.cs (1)
5-10: Interface shape looks solid; aligns with the provided SystemTextJsonSerializer implementationThe members cover the required use cases for the HttpClient wrapper. No functional concerns.
If you want to improve discoverability, consider adding XML docs to the interface methods (Serialize/Deserialize) describing null-handling semantics and expected behavior for empty inputs.
Src/CrispyWaffle.BackgroundJobs/Abstractions/IJobScheduler.cs (1)
1-9: Add missing usings; consider cancellation tokens for scheduler APIs
- The interface uses TimeSpan and Task but lacks explicit imports. Add them to avoid relying on ImplicitUsings.
- Optional: Expose CancellationToken in both methods; scheduling and enqueueing often touch external stores and should be cancelable. Since this is a new interface, adding the token now prevents an immediate breaking change later.
Minimal fix (explicit usings):
-namespace CrispyWaffle.BackgroundJobs.Abstractions +using System; +using System.Threading.Tasks; + +namespace CrispyWaffle.BackgroundJobs.Abstractions { public interface IJobScheduler { Task ScheduleAsync(string handlerName, object payload, TimeSpan delay, int maxAttempts = 3, JobPriority priority = JobPriority.Normal); Task EnqueueAsync(string handlerName, object payload, int maxAttempts = 3, JobPriority priority = JobPriority.Normal); } }Optional API enhancement (cancellable operations):
- Task ScheduleAsync(string handlerName, object payload, TimeSpan delay, int maxAttempts = 3, JobPriority priority = JobPriority.Normal); + Task ScheduleAsync(string handlerName, object payload, TimeSpan delay, int maxAttempts = 3, JobPriority priority = JobPriority.Normal, System.Threading.CancellationToken cancellationToken = default); - Task EnqueueAsync(string handlerName, object payload, int maxAttempts = 3, JobPriority priority = JobPriority.Normal); + Task EnqueueAsync(string handlerName, object payload, int maxAttempts = 3, JobPriority priority = JobPriority.Normal, System.Threading.CancellationToken cancellationToken = default);If you choose the cancellable signatures, remember to update DelayedJobScheduler (and any other IJobScheduler implementations) accordingly.
Src/CrispyWaffle.BackgroundJobs/Core/BackgroundJobQueue.cs (2)
15-16: Channel created as unbounded: set SingleReader if only one consumer readsIf BackgroundWorker is the only consumer, use SingleReader = true for minor perf gains.
- private readonly Channel<JobEntity> _signal = Channel.CreateUnbounded<JobEntity>(new UnboundedChannelOptions { SingleReader = false, SingleWriter = false }); + private readonly Channel<JobEntity> _signal = Channel.CreateUnbounded<JobEntity>( + new UnboundedChannelOptions { SingleReader = true, SingleWriter = false });If you expect multiple concurrent consumers, keep SingleReader = false.
23-25: Use TryWrite on unbounded channel to avoid unnecessary async machineryFor unbounded channels, TryWrite is preferable and avoids allocating/awaiting a ValueTask. Current code discards the ValueTask anyway.
- // Always write to signal channel to notify consumers. - _ = _signal.Writer.WriteAsync(job); + // Always write to signal channel to notify consumers. + _signal.Writer.TryWrite(job);Src/CrispyWaffle.HttpClient/ServiceCollectionExtensions.cs (2)
37-43: Use TryAddSingleton to respect prior registrations (extensibility)Switch to TryAddSingleton so apps can override the serializer or wrapper before calling this extension. This is a common DI best practice for library-provided defaults.
- // Register serializer as a singleton for thread-safety. - // This assumes the serializer implementation is stateless or internally thread-safe. - services.AddSingleton<Serialization.IJsonSerializer, Serialization.SystemTextJsonSerializer>(); + // Register serializer as a singleton for thread-safety (allow override). + // This assumes the serializer implementation is stateless or internally thread-safe. + services.TryAddSingleton<Serialization.IJsonSerializer, Serialization.SystemTextJsonSerializer>(); - // Register the HttpClient wrapper. - services.AddSingleton<IHttpClientWrapper, HttpClientWrapper>(); + // Register the HttpClient wrapper (allow override). + services.TryAddSingleton<IHttpClientWrapper, HttpClientWrapper>();You’ll need the Extensions namespace import:
using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions;
26-35: Options registration looks good; minor nitUsing Configure with a no-op delegate is fine. Alternatively, you could call AddOptions() and only Configure when a delegate is provided, but it’s functionally equivalent.
No change required.
Src/CrispyWaffle.BackgroundJobs/Persistence/JobDbContext.cs (1)
1-1: Remove unused and suspicious import (System.Reflection.Emit).This import is not used and is out of place in a DbContext file.
Apply this diff:
-using System.Reflection.Emit; using CrispyWaffle.BackgroundJobs.Abstractions; using Microsoft.EntityFrameworkCore;Src/CrispyWaffle.BackgroundJobs/Core/JobHandlerRegistry.cs (1)
7-13: Use case-insensitive keys and prevent accidental re-registration.Handler names are logical identifiers often treated case-insensitively. Also, silently overwriting an existing mapping can hide misconfiguration.
Apply this diff:
- private readonly ConcurrentDictionary<string, (Type handlerType, Type dataType)> _map = new(); + private readonly ConcurrentDictionary<string, (Type handlerType, Type dataType)> _map = + new(System.StringComparer.OrdinalIgnoreCase); public void Register<THandler, TData>(string handlerName) where THandler : class, CrispyWaffle.BackgroundJobs.Abstractions.IBackgroundJobHandler<TData> { if (string.IsNullOrWhiteSpace(handlerName)) throw new ArgumentException("handlerName required", nameof(handlerName)); - _map[handlerName] = (typeof(THandler), typeof(TData)); + if (!_map.TryAdd(handlerName, (typeof(THandler), typeof(TData)))) + { + throw new InvalidOperationException($"Handler '{handlerName}' is already registered."); + } }Src/CrispyWaffle.HttpClient/RetryPolicy.cs (2)
104-114: Standardize comments to English for consistency.Non-English comments can confuse readers and tooling. Suggest updating these lines.
Apply this diff:
- // 1. Chuẩn HTTP (HttpResponseHeaders) + // 1. Standard HTTP header (HttpResponseHeaders)
114-126: Standardize comments to English for consistency.Same here; keep comments consistent across the codebase.
Apply this diff:
- // 2. Fallback - parse thủ công từ mọi header (cả ContentHeaders) + // 2. Fallback - manual parse from any header (including ContentHeaders)Src/CrispyWaffle.BackgroundJobs/Core/JobDispatcher.cs (2)
21-43: Validate inputs and normalizemaxAttempts
- Ensure
handlerNameis not null/empty.- Normalize
maxAttemptsto a positive integer to avoid non-sensical configurations.Apply this diff:
- public async Task<Guid> EnqueueAsync(string handlerName, object payload, int? maxAttempts = null, JobPriority priority = JobPriority.Normal) + public async Task<Guid> EnqueueAsync(string handlerName, object payload, int? maxAttempts = null, JobPriority priority = JobPriority.Normal) { + if (string.IsNullOrWhiteSpace(handlerName)) + throw new ArgumentException("Handler name is required.", nameof(handlerName)); + var effectiveMaxAttempts = (maxAttempts ?? _options.DefaultMaxAttempt); + if (effectiveMaxAttempts <= 0) effectiveMaxAttempts = _options.DefaultMaxAttempt; var job = new JobEntity { HandlerName = handlerName, Payload = JsonSerializer.Serialize(payload), Priority = priority, - MaxAttempt = maxAttempts ?? _options.DefaultMaxAttempt, + MaxAttempt = effectiveMaxAttempts, Status = JobStatus.Pending, CreatedAt = DateTimeOffset.UtcNow };- public async Task ScheduleAsync(string handlerName, object payload, TimeSpan delay, int? maxAttempts = null, JobPriority priority = JobPriority.Normal) + public async Task ScheduleAsync(string handlerName, object payload, TimeSpan delay, int? maxAttempts = null, JobPriority priority = JobPriority.Normal) { + if (string.IsNullOrWhiteSpace(handlerName)) + throw new ArgumentException("Handler name is required.", nameof(handlerName)); + var effectiveMaxAttempts = (maxAttempts ?? _options.DefaultMaxAttempt); + if (effectiveMaxAttempts <= 0) effectiveMaxAttempts = _options.DefaultMaxAttempt; var job = new JobEntity { HandlerName = handlerName, Payload = JsonSerializer.Serialize(payload), Priority = priority, - MaxAttempt = maxAttempts ?? _options.DefaultMaxAttempt, + MaxAttempt = effectiveMaxAttempts, Status = JobStatus.Pending, ScheduledAt = DateTimeOffset.UtcNow.Add(delay), CreatedAt = DateTimeOffset.UtcNow };Also applies to: 45-75
64-73: Fire-and-forget delay swallows exceptions; consider logging and cancellation integrationThe in-memory scheduling path suppresses all exceptions. Consider injecting
ILogger<JobDispatcher>and logging failures, and optionally wiring aCancellationToken(e.g., viaIHostApplicationLifetime) for graceful shutdown.Would you like a patch that adds
ILogger<JobDispatcher>and logs the suppressed exceptions?Src/CrispyWaffle.BackgroundJobs/Core/BackgroundWorker.cs (4)
221-226: Backoff cap is hardcoded; use configurableJobOptions.MaxBackoffSecondsMake the backoff cap configurable per options instead of a constant 300 seconds.
Apply this diff:
- private static int ComputeBackoffSeconds(int attempt) + private int ComputeBackoffSeconds(int attempt) { // exponential backoff with cap var seconds = (int)Math.Pow(2, Math.Min(attempt, 10)); - return Math.Min(seconds, 300); + return Math.Min(seconds, _options.MaxBackoffSeconds); }Note: this changes the method to instance; call sites remain the same.
146-151: Treat deserialization errors as non-retriable failuresA malformed payload will currently throw and be retried indefinitely until
MaxAttemptis hit. Consider marking as failed onJsonException(bad input) instead of retrying.Apply this diff:
- if (!string.IsNullOrWhiteSpace(job.Payload)) - { - data = JsonSerializer.Deserialize(job.Payload, dataType ?? typeof(object)); - } + if (!string.IsNullOrWhiteSpace(job.Payload)) + { + try + { + data = JsonSerializer.Deserialize(job.Payload, dataType ?? typeof(object)); + } + catch (JsonException jex) + { + var msg = $"Payload deserialization failed for handler '{job.HandlerName}': {jex.Message}"; + _logger.LogError(jex, msg); + if (_store != null) + { + await _store.MarkFailedAsync(job.Id, msg, cancellationToken); + } + return false; + } + }Also applies to: 198-209
228-276: Broaden handler invocation to supportIBackgroundJob.ExecuteAsync(CancellationToken)Current logic only looks for
HandleAsync. If you intend to support bothIBackgroundJobHandler<TData>andIBackgroundJob, add a fallback toExecuteAsync(CancellationToken)and support (CancellationToken) only signatures.Proposed replacement for method discovery:
- var method = handler.GetType().GetMethod("HandleAsync"); - if (method == null) throw new InvalidOperationException("Handler does not implement HandleAsync"); + var t = handler.GetType(); + var method = t.GetMethod("HandleAsync") + ?? t.GetMethod("ExecuteAsync"); + if (method == null) throw new InvalidOperationException("Handler does not implement HandleAsync/ExecuteAsync");And augment parameter binding to handle the
(CancellationToken)-only case:- if (parameters.Length == 2) + if (parameters.Length == 2) { invokeArg1 = data; invokeArg2 = cancellationToken; } - else if (parameters.Length == 1) + else if (parameters.Length == 1) { - invokeArg1 = data ?? cancellationToken; - if (invokeArg1 is CancellationToken) - { - invokeArg2 = null; - } + // either (TData) or (CancellationToken) + if (parameters[0].ParameterType == typeof(CancellationToken)) + { + invokeArg1 = cancellationToken; + } + else + { + invokeArg1 = data; + } }
40-132: Break down complex methods to improve readability and testability
WorkerLoopAsyncandProcessJobAsyncare doing a lot. Consider extracting:
FetchOrWaitAsync(store/queue selection, polling delay)HandleScheduledFutureAsyncHandleFailureAsync(retry/exhaust)ResolveAndInvokeAsync
This will also ease unit testing.Also applies to: 133-220
Src/CrispyWaffle.BackgroundJobs/Persistence/EfCoreJobStore.cs (1)
28-33: Add supporting indexes to reduce scan costFetching next job by
(Status, ScheduledAt, Priority, CreatedAt)benefits from indexes. Add composite indexes inJobDbContext:For example (in JobDbContext OnModelCreating):
modelBuilder.Entity<JobEntity>() .HasIndex(j => new { j.Status, j.ScheduledAt, j.Priority, j.CreatedAt }) .HasDatabaseName("IX_Jobs_NextDue"); modelBuilder.Entity<JobEntity>() .HasIndex(j => j.Status) .HasDatabaseName("IX_Jobs_Status");Src/CrispyWaffle.HttpClient/HttpRequestOptions.cs (1)
10-10: Typo: “overriden” → “overridden”Apply this diff:
- /// Options used as defaults (via IOptions) and can be overriden per-request. + /// Options used as defaults (via IOptions) and can be overridden per-request.Src/CrispyWaffle.BackgroundJobs/Persistence/InMemoryJobStore.cs (1)
38-47: Optional: Avoid unbounded memory growth in the in-memory storeCompleted/failed jobs are retained indefinitely. For long-running processes this can leak memory.
Options:
- Remove terminal jobs on completion/failure:
@@ - public Task MarkCompletedAsync(Guid jobId, CancellationToken cancellationToken = default) + public Task MarkCompletedAsync(Guid jobId, CancellationToken cancellationToken = default) { - if (_storage.TryGetValue(jobId, out var job)) - { - job.Status = JobStatus.Completed; - job.UpdatedAt = DateTimeOffset.UtcNow; - _storage[jobId] = job; - } + _storage.TryRemove(jobId, out _); return Task.CompletedTask; } @@ - public Task MarkFailedAsync(Guid jobId, string error, CancellationToken cancellationToken = default) + public Task MarkFailedAsync(Guid jobId, string error, CancellationToken cancellationToken = default) { - if (_storage.TryGetValue(jobId, out var job)) - { - job.Status = JobStatus.Failed; - job.LastError = error; - job.UpdatedAt = DateTimeOffset.UtcNow; - _storage[jobId] = job; - } + if (_storage.TryGetValue(jobId, out var job)) + { + job.Status = JobStatus.Failed; + job.LastError = error; + job.UpdatedAt = DateTimeOffset.UtcNow; + _storage[jobId] = job; + } return Task.CompletedTask; }
- Or implement a simple TTL/background cleanup for terminal jobs. I can sketch this if you prefer to keep terminal state for monitoring.
Also applies to: 49-59, 61-72
Src/CrispyWaffle.BackgroundJobs/Extensions/ServiceCollectionExtensions.cs (3)
14-16: XML doc/parameter naming mismatchDoc says “pass useEfCore=true” but the parameter is named useEfCoreStore.
-/// Register core background job services. By default uses InMemoryJobStore. To use EF Core, call AddDbContext<JobDbContext> before this and pass useEfCore=true. +/// Register core background job services. By default uses InMemoryJobStore. To use EF Core, call AddDbContext<JobDbContext> before this and pass useEfCoreStore=true.
21-25: Consider using the Options pattern for JobOptionsRegistering a concrete JobOptions singleton works, but using IOptions integrates better with configuration binding and validation.
If interested, I can provide a minimal refactor to:
- services.Configure(configureOptions ?? (_ => {}));
- Inject IOptions where needed.
77-83: Reflection-based Register() is brittleRelying on a generic method name via reflection couples to a concrete registry implementation. Prefer a non-generic API on IJobHandlerRegistry, e.g., Register(string handlerName, Type handlerType, Type dataType).
I can add this method to the interface and registry to remove reflection here.
Src/CrispyWaffle.BackgroundJobs/Abstractions/JobEntity.cs (1)
11-46: Looks solid; consider adding a concurrency token for EF storeEntity defaults and fields cover the lifecycle well.
If EF Core is used for claiming work with optimistic concurrency, adding a RowVersion (byte[] timestamp) and mapping it as a concurrency token strengthens “claim” semantics in EfCoreJobStore. Optional:
public byte[]? RowVersion { get; set; }Then configure .IsRowVersion() in JobDbContext.
Src/CrispyWaffle.HttpClient/IHttpClientWrapper.cs (2)
18-23: Sync wrappers can deadlock; consider isolating or removing from the primary interfaceBlocking on async with GetAwaiter().GetResult() can deadlock under certain sync contexts (ASP.NET classic, UI). You already warn in comments, but exposing them on the primary interface invites misuse.
Options:
- Drop sync methods from the interface and keep only async.
- Move sync methods to an extension class or separate interface to discourage routine use.
13-16: Optional: Add typed DeleteAsync for symmetryYou support typed Get/Post/Put but Delete returns object?. A typed variant is often useful.
Example signature:
Task<HttpResponse<T>> DeleteAsync<T>(string url, HttpRequestOptions? options = null, CancellationToken cancellationToken = default);I can wire this through HttpClientWrapper if desired.
Src/CrispyWaffle.HttpClient/HttpClientWrapper.cs (1)
123-260: Method complexity is high; consider extracting helpersSendAsync handles URI composition, request construction, retries, content reading, header aggregation, deserialization, and error shaping. Extracting a few helpers will improve maintainability and testability.
Candidates:
- BuildRequestMessage(method, url, body, effectiveOptions, serializer)
- ReadRawContentAsync(HttpResponseMessage, logger, throwOnError)
- DeserializeOrFailure(raw, serializer, headers, status, duration, throwOnError)
| else | ||
| { | ||
| job.ScheduledAt = nextAttempt; | ||
| _queue!.Enqueue(job); | ||
| } |
There was a problem hiding this comment.
In-memory retry re-enqueues immediately → tight loop CPU spin
When using the in-memory queue, failed jobs are re-enqueued immediately with a future ScheduledAt, causing the worker to pick them up again right away and spin. Schedule with a delay instead of immediate enqueue.
Apply this diff:
- if (_store != null)
- {
- await _store.MarkRetryAsync(job.Id, nextAttempt, job.Attempt, cancellationToken);
- }
- else
- {
- job.ScheduledAt = nextAttempt;
- _queue!.Enqueue(job);
- }
+ if (_store != null)
+ {
+ await _store.MarkRetryAsync(job.Id, nextAttempt, job.Attempt, cancellationToken);
+ }
+ else
+ {
+ job.ScheduledAt = nextAttempt;
+ _ = Task.Run(async () =>
+ {
+ try
+ {
+ var delay = nextAttempt - DateTimeOffset.UtcNow;
+ if (delay > TimeSpan.Zero) await Task.Delay(delay, cancellationToken);
+ _queue!.Enqueue(job);
+ }
+ catch (OperationCanceledException) { /* shutting down */ }
+ catch { /* suppressed */ }
+ });
+ }- else
- {
- job.Attempt++;
- job.ScheduledAt = DateTimeOffset.UtcNow.AddSeconds(ComputeBackoffSeconds(job.Attempt));
- _queue!.Enqueue(job);
- }
+ else
+ {
+ job.Attempt++;
+ var nextAttempt = DateTimeOffset.UtcNow.AddSeconds(ComputeBackoffSeconds(job.Attempt));
+ job.ScheduledAt = nextAttempt;
+ _ = Task.Run(async () =>
+ {
+ try
+ {
+ var delay = nextAttempt - DateTimeOffset.UtcNow;
+ if (delay > TimeSpan.Zero) await Task.Delay(delay, cancellationToken);
+ _queue!.Enqueue(job);
+ }
+ catch (OperationCanceledException) { /* shutting down */ }
+ catch { /* suppressed */ }
+ });
+ }Also applies to: 211-215
🤖 Prompt for AI Agents
In Src/CrispyWaffle.BackgroundJobs/Core/BackgroundWorker.cs around lines 184-188
and 211-215, failed jobs are being re-enqueued immediately into the in-memory
queue even when their ScheduledAt is set in the future, causing the worker to
pick them up repeatedly and spin the CPU; update the logic to schedule the job
with a delay instead of immediate Enqueue by using a delayed scheduling
mechanism (e.g., compute delay = job.ScheduledAt - DateTime.UtcNow and call a
queue method that accepts a delay or use Task.Delay to enqueue after that delay)
and ensure null checks on _queue remain, so jobs with future ScheduledAt are not
available to consumers until the delay elapses.
| public JobDispatcher(IServiceProvider provider, BackgroundJobQueue? queue, IJobStore? store, JobOptions options) | ||
| { | ||
| _provider = provider; | ||
| _queue = queue; | ||
| _store = store; | ||
| _options = options; | ||
| } |
There was a problem hiding this comment.
Guard against missing queue/store to prevent silently dropped jobs
If both _store and _queue are null, jobs are effectively lost (no persistence and no enqueue). Enforce a guard in the constructor.
Apply this diff:
public JobDispatcher(IServiceProvider provider, BackgroundJobQueue? queue, IJobStore? store, JobOptions options)
{
_provider = provider;
_queue = queue;
_store = store;
_options = options;
+
+ if (_queue is null && _store is null)
+ {
+ throw new InvalidOperationException("JobDispatcher requires either IJobStore or BackgroundJobQueue to be registered.");
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public JobDispatcher(IServiceProvider provider, BackgroundJobQueue? queue, IJobStore? store, JobOptions options) | |
| { | |
| _provider = provider; | |
| _queue = queue; | |
| _store = store; | |
| _options = options; | |
| } | |
| public JobDispatcher(IServiceProvider provider, BackgroundJobQueue? queue, IJobStore? store, JobOptions options) | |
| { | |
| _provider = provider; | |
| _queue = queue; | |
| _store = store; | |
| _options = options; | |
| if (_queue is null && _store is null) | |
| { | |
| throw new InvalidOperationException("JobDispatcher requires either IJobStore or BackgroundJobQueue to be registered."); | |
| } | |
| } |
🤖 Prompt for AI Agents
In Src/CrispyWaffle.BackgroundJobs/Core/JobDispatcher.cs around lines 13 to 19,
the constructor currently allows both _queue and _store to be null which would
silently drop jobs; add a guard that validates at least one of queue or store is
provided and throw an ArgumentException (or ArgumentNullException) if both are
null. Update the constructor to check if queue == null && store == null and
throw a clear exception message like "Either queue or store must be provided" so
the failure is explicit and prevents silent job loss.
| // StartupFilter ensures the registry is populated after DI container is built | ||
| private class JobHandlerStartupFilter : Microsoft.AspNetCore.Hosting.IStartupFilter | ||
| { | ||
| private readonly IServiceProvider _sp; | ||
| private readonly string _handlerName; | ||
| private readonly Type _handlerType; | ||
| private readonly Type _dataType; | ||
|
|
||
| public JobHandlerStartupFilter(IServiceProvider sp, string handlerName, Type handlerType, Type dataType) | ||
| { | ||
| _sp = sp; _handlerName = handlerName; _handlerType = handlerType; _dataType = dataType; | ||
| } | ||
|
|
||
| public Action<Microsoft.AspNetCore.Builder.IApplicationBuilder> Configure(Action<Microsoft.AspNetCore.Builder.IApplicationBuilder> next) | ||
| { | ||
| return app => | ||
| { | ||
| var registry = _sp.GetRequiredService<IJobHandlerRegistry>(); | ||
| var registerMethod = registry.GetType().GetMethod("Register")?.MakeGenericMethod(_handlerType, _dataType); | ||
| // If Register<THandler, TData>(string) exists we call via reflection | ||
| if (registerMethod != null) | ||
| { | ||
| registerMethod.Invoke(registry, new object[] { _handlerName }); | ||
| } | ||
|
|
||
| next(app); | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid ASP.NET-only IStartupFilter for handler registration; support Generic Host
IStartupFilter runs only in ASP.NET Core pipelines. Consumers using Generic Host (e.g., WorkerService without a web app) won’t trigger this, and handlers won’t be registered.
Safer alternative: register via an IHostedService that runs at startup, resolves IJobHandlerRegistry, and performs the registration. This works in both web and worker hosts.
I can replace JobHandlerStartupFilter with a small HostedService (JobHandlerRegistrationHostedService<THandler,TData>) and wire it in AddJobHandler. Would you like me to generate that patch?
🏁 Script executed:
#!/bin/bash
set -e
echo "Searching for JobHandlerStartupFilter usage..."
rg -n "JobHandlerStartupFilter" -C3
echo
echo "Searching for AddJobHandler extension..."
rg -n "AddJobHandler" -C3Length of output: 3714
Use IHostedService instead of IStartupFilter for handler registration to support Generic Host
IStartupFilter only fires in the ASP.NET Core request pipeline—any app running on Generic Host (e.g. a WorkerService) will never invoke it, so your job handlers won’t be registered. To fix this:
• Remove the IStartupFilter registration in AddJobHandler<THandler,TData>.
• Introduce a generic IHostedService (e.g. JobHandlerRegistrationHostedService<THandler,TData>) that, in StartAsync, resolves IJobHandlerRegistry and calls its Register<THandler,TData>(handlerName).
• In AddJobHandler, register that hosted service:
services.AddSingleton<IHostedService>(sp =>
new JobHandlerRegistrationHostedService<THandler, TData>(
sp.GetRequiredService<IJobHandlerRegistry>(),
handlerName));This approach works in both Web and Worker hosts. Let me know if you’d like a sample implementation.
🤖 Prompt for AI Agents
In Src/CrispyWaffle.BackgroundJobs/Extensions/ServiceCollectionExtensions.cs
around lines 60 to 88, the current JobHandlerStartupFilter implementing
IStartupFilter won't run for Generic Host apps (e.g. WorkerService) so handlers
may never be registered; replace this approach by removing the IStartupFilter
registration and adding a generic IHostedService (e.g.
JobHandlerRegistrationHostedService<THandler,TData>) that takes an
IJobHandlerRegistry and handlerName and calls
registry.Register<THandler,TData>(handlerName) in StartAsync, then in
AddJobHandler<THandler,TData> register that hosted service as a singleton
IHostedService via a factory (resolving IJobHandlerRegistry and passing
handlerName) so registration runs on both Web and Worker hosts.
| // The action will create a NEW HttpRequestMessage each time it's invoked (important for retries). | ||
| Func<CancellationToken, Task<HttpResponseMessage>> action = async ct => | ||
| { | ||
| // Create a new request per attempt - ensures headers/content are fresh for retries. | ||
| using var req = new HttpRequestMessage(method, url); | ||
|
|
||
| if (body != null) | ||
| { | ||
| // Serialize the body (synchronous serializer expected here). | ||
| string payload = serializer.Serialize(body); | ||
| req.Content = new StringContent(payload, Encoding.UTF8, effectiveOptions.UseJsonContentType ? "application/json" : "text/plain"); | ||
| } | ||
|
|
||
| // Apply per-request (merged) headers to the request message. | ||
| if (effectiveOptions.DefaultRequestHeaders != null) | ||
| { | ||
| foreach (var kv in effectiveOptions.DefaultRequestHeaders) | ||
| { | ||
| if (!req.Headers.Contains(kv.Key)) | ||
| req.Headers.TryAddWithoutValidation(kv.Key, kv.Value); | ||
| } | ||
| } | ||
|
|
||
| // Send with ResponseHeadersRead to start streaming the body (we will read it explicitly afterward). | ||
| // This avoids buffering the entire response in some handlers. | ||
| return await client.SendAsync(req, HttpCompletionOption.ResponseHeadersRead, ct).ConfigureAwait(false); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Build request URI from per-request BaseAddress and set Accept/header values on the request, not the client
Compose the request URI using BaseAddress when provided, and ensure Accept header is set per request to avoid pollution.
- // Create a new request per attempt - ensures headers/content are fresh for retries.
- using var req = new HttpRequestMessage(method, url);
+ // Build request URI from optional BaseAddress + relative path
+ var requestUri = BuildRequestUri(url, effectiveOptions.BaseAddress);
+ // Create a new request per attempt - ensures headers/content are fresh for retries.
+ using var req = new HttpRequestMessage(method, requestUri);
@@
- if (effectiveOptions.DefaultRequestHeaders != null)
+ if (effectiveOptions.DefaultRequestHeaders != null)
{
foreach (var kv in effectiveOptions.DefaultRequestHeaders)
{
if (!req.Headers.Contains(kv.Key))
req.Headers.TryAddWithoutValidation(kv.Key, kv.Value);
}
}
+
+ // Ensure an Accept header per request (avoid mutating HttpClient.DefaultRequestHeaders)
+ if (req.Headers.Accept == null || req.Headers.Accept.Count == 0)
+ {
+ req.Headers.Accept.Add(new System.Net.Http.Headers.MediaTypeWithQualityHeaderValue("application/json"));
+ }Add this helper in the Helpers region (outside this range):
private static Uri BuildRequestUri(string url, string? baseAddress)
{
if (Uri.TryCreate(url, UriKind.Absolute, out var absolute))
return absolute;
if (!string.IsNullOrWhiteSpace(baseAddress) && Uri.TryCreate(baseAddress, UriKind.Absolute, out var baseUri))
return new Uri(baseUri, url);
// fall back: try as absolute; will throw if invalid
return new Uri(url, UriKind.RelativeOrAbsolute);
}🤖 Prompt for AI Agents
In Src/CrispyWaffle.HttpClient/HttpClientWrapper.cs around lines 136 to 162, the
request is created directly from the url and headers like Accept may be added to
the shared HttpClient; instead, use a per-request URI built from the optional
BaseAddress and apply headers (including Accept) to the HttpRequestMessage so
you don't mutate client defaults. Replace new HttpRequestMessage(method, url)
with new HttpRequestMessage(method, BuildRequestUri(url,
effectiveOptions.BaseAddress)) and set Accept and any other per-request headers
on req.Headers (not on client.DefaultRequestHeaders), preserving existing logic
for DefaultRequestHeaders merging; also add the provided BuildRequestUri helper
method in the Helpers region (outside this block) to resolve absolute or
base-relative URIs.
| // Execute the action with retry policy. The policy is expected to return the final response (or null + exception). | ||
| var (resp, ex) = await RetryPolicy.ExecuteAsync( | ||
| action, | ||
| effectiveOptions.RetryCount, | ||
| effectiveOptions.RetryBaseDelayMs, | ||
| effectiveOptions.MaxRetryDelayMs, | ||
| cancellationToken).ConfigureAwait(false); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a per-request timeout via a linked CancellationTokenSource instead of mutating HttpClient.Timeout
This preserves isolation between callers and avoids cross-request side effects.
- var (resp, ex) = await RetryPolicy.ExecuteAsync(
+ using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
+ timeoutCts.CancelAfter(TimeSpan.FromSeconds(Math.Max(1, effectiveOptions.TimeoutSeconds)));
+
+ var (resp, ex) = await RetryPolicy.ExecuteAsync(
action,
effectiveOptions.RetryCount,
effectiveOptions.RetryBaseDelayMs,
effectiveOptions.MaxRetryDelayMs,
- cancellationToken).ConfigureAwait(false);
+ timeoutCts.Token).ConfigureAwait(false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Execute the action with retry policy. The policy is expected to return the final response (or null + exception). | |
| var (resp, ex) = await RetryPolicy.ExecuteAsync( | |
| action, | |
| effectiveOptions.RetryCount, | |
| effectiveOptions.RetryBaseDelayMs, | |
| effectiveOptions.MaxRetryDelayMs, | |
| cancellationToken).ConfigureAwait(false); | |
| // Execute the action with retry policy. The policy is expected to return the final response (or null + exception). | |
| using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); | |
| timeoutCts.CancelAfter(TimeSpan.FromSeconds(Math.Max(1, effectiveOptions.TimeoutSeconds))); | |
| var (resp, ex) = await RetryPolicy.ExecuteAsync( | |
| action, | |
| effectiveOptions.RetryCount, | |
| effectiveOptions.RetryBaseDelayMs, | |
| effectiveOptions.MaxRetryDelayMs, | |
| timeoutCts.Token).ConfigureAwait(false); |
🤖 Prompt for AI Agents
In Src/CrispyWaffle.HttpClient/HttpClientWrapper.cs around lines 164 to 171, do
not change HttpClient.Timeout; instead create a linked CancellationTokenSource
using the incoming cancellationToken and the per-request timeout from
effectiveOptions (e.g. TimeoutMs), then pass linkedCts.Token into
RetryPolicy.ExecuteAsync (and/or into the delegated action) so the timeout
cancels only this request; ensure you Dispose the linked CTS (use using or
try/finally) and propagate OperationCanceledException as before.
| /// <summary>Gets or sets a value indicating whether if true, methods throw HttpClientException on final failure instead of returning unsuccessful HttpResponse<T>.</summary> | ||
| public bool ThrowOnError { get; set; } = false; | ||
|
|
||
| /// <summary>Gets or sets default request headers to add to each request if present.</summary> | ||
| public IDictionary<string, string>? DefaultRequestHeaders { get; set; } | ||
|
|
||
| /// <summary>Gets or sets optional specific named client to create from IHttpClientFactory. If null, factory.CreateClient() is used.</summary> | ||
| public string? NamedHttpClient { get; set; } | ||
|
|
||
| /// <summary>Gets or sets a value indicating whether if true, sends content type application/json when body present.</summary> | ||
| public bool UseJsonContentType { get; set; } = true; | ||
|
|
||
| /// <summary>Gets or sets optional serializer override per-request. If null will use DI resolved one.</summary> | ||
| public IJsonSerializer? Serializer { get; set; } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bool overrides cannot express “unset”; they always override defaults
ThrowOnError and UseJsonContentType are plain bools. In the merge logic, a provided per-request options instance will always override defaults (even if the caller didn’t intend to change these). Make them nullable and only override when set.
Apply this diff:
- public bool ThrowOnError { get; set; } = false;
+ public bool? ThrowOnError { get; set; }- public bool UseJsonContentType { get; set; } = true;
+ public bool? UseJsonContentType { get; set; }And update the merge logic in HttpClientWrapper (for reference, outside this file):
ThrowOnError = perRequest.ThrowOnError ?? defaults.ThrowOnError,
UseJsonContentType = perRequest.UseJsonContentType ?? defaults.UseJsonContentType,🤖 Prompt for AI Agents
In Src/CrispyWaffle.HttpClient/HttpRequestOptions.cs around lines 29-43, change
the two plain bool properties ThrowOnError and UseJsonContentType to nullable
booleans (bool?) and remove their current default initializers so they can
express "unset"; then update the merge logic in HttpClientWrapper to use
null-coalescing (e.g., ThrowOnError = perRequest.ThrowOnError ??
defaults.ThrowOnError and UseJsonContentType = perRequest.UseJsonContentType ??
defaults.UseJsonContentType) so per-request options only override when
explicitly set.
| private static int ComputeDelay(int attempt, int baseDelayMs, int maxDelayMs, Random rnd) | ||
| { | ||
| // exponential backoff with jitter | ||
| // formula: min(maxDelay, base * 2^attempt) +/- jitter up to 20% | ||
| var exponential = baseDelayMs * (1 << Math.Min(attempt, 10)); // cap shift | ||
| var capped = Math.Min(exponential, maxDelayMs); | ||
| var jitter = (int)(capped * 0.2 * (rnd.NextDouble() - 0.5)); // ±10% | ||
| var result = Math.Max(0, capped + jitter); | ||
| return result; |
There was a problem hiding this comment.
Fix integer overflow in exponential backoff and align jitter with comment.
Using int math can overflow for larger attempts/base delays. Compute in long and clamp. Also make jitter ±20% as the comment states.
Apply this diff:
- private static int ComputeDelay(int attempt, int baseDelayMs, int maxDelayMs, Random rnd)
+ private static int ComputeDelay(int attempt, int baseDelayMs, int maxDelayMs, Random rnd)
{
// exponential backoff with jitter
- // formula: min(maxDelay, base * 2^attempt) +/- jitter up to 20%
- var exponential = baseDelayMs * (1 << Math.Min(attempt, 10)); // cap shift
- var capped = Math.Min(exponential, maxDelayMs);
- var jitter = (int)(capped * 0.2 * (rnd.NextDouble() - 0.5)); // ±10%
- var result = Math.Max(0, capped + jitter);
- return result;
+ // formula: min(maxDelay, base * 2^attempt) +/- jitter up to 20%
+ var shift = 1L << Math.Min(attempt, 10); // cap shift
+ var exponential = (long)baseDelayMs * shift;
+ var capped = (int)Math.Min(exponential, (long)maxDelayMs);
+ var jitter = (int)(capped * 0.2 * (rnd.NextDouble() * 2 - 1)); // ±20%
+ return Math.Max(0, capped + jitter);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static int ComputeDelay(int attempt, int baseDelayMs, int maxDelayMs, Random rnd) | |
| { | |
| // exponential backoff with jitter | |
| // formula: min(maxDelay, base * 2^attempt) +/- jitter up to 20% | |
| var exponential = baseDelayMs * (1 << Math.Min(attempt, 10)); // cap shift | |
| var capped = Math.Min(exponential, maxDelayMs); | |
| var jitter = (int)(capped * 0.2 * (rnd.NextDouble() - 0.5)); // ±10% | |
| var result = Math.Max(0, capped + jitter); | |
| return result; | |
| private static int ComputeDelay(int attempt, int baseDelayMs, int maxDelayMs, Random rnd) | |
| { | |
| // exponential backoff with jitter | |
| // formula: min(maxDelay, base * 2^attempt) +/- jitter up to 20% | |
| var shift = 1L << Math.Min(attempt, 10); // cap shift | |
| var exponential = (long)baseDelayMs * shift; | |
| var capped = (int)Math.Min(exponential, (long)maxDelayMs); | |
| var jitter = (int)(capped * 0.2 * (rnd.NextDouble() * 2 - 1)); // ±20% | |
| return Math.Max(0, capped + jitter); | |
| } |
🤖 Prompt for AI Agents
In Src/CrispyWaffle.HttpClient/RetryPolicy.cs around lines 91–99, the
exponential backoff uses int math which can overflow and the jitter calculation
implements ±10% despite the comment saying ±20%; change the math to use long for
the exponential calculation (e.g., use 1L << Math.Min(attempt, 10) and multiply
baseDelayMs as long), clamp the long result against maxDelayMs (also treated as
long), compute jitter as ±20% of the capped value using a symmetric random
factor (e.g., (rnd.NextDouble()*2 - 1) * 0.2 * capped), convert the final
clamped value safely back to int ensuring it stays within 0..int.MaxValue and
not above maxDelayMs, and return that int.
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Overly Generic Exception Name ▹ view | |
|
Missing Context in Exception ▹ view | |
 |
Insufficient Exception Context ▹ view | |
 |
Invalid State Combinations Possible ▹ view | |
|
Interface merges distinct responsibilities ▹ view | |
 |
Unsafe JSON Deserialization ▹ view | |
 |
Undefined null handling in serialization ▹ view | |
|
Unnecessary Namespace Qualification in Constraint ▹ view | |
|
Nullable Types in Registry Lookup ▹ view | |
|
Verbose namespace qualification ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| Src/CrispyWaffle.BackgroundJobs/Abstractions/JobPriority.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Exceptions/JobFailedException.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Abstractions/JobStatus.cs | ✅ |
| Src/CrispyWaffle.HttpClient/Serialization/IJsonSerializer.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Core/JobResult.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Core/IJobHandlerRegistry.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Abstractions/IJobScheduler.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Abstractions/IBackgroundJobHandler.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Abstractions/IBackgroundJob.cs | ✅ |
| Src/CrispyWaffle.HttpClient/Exceptions/HttpClientException.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Monitoring/JobMetrics.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Core/JobOptions.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Monitoring/MonitoringMiddleware.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Abstractions/IJobStore.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Core/JobHandlerRegistry.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Persistence/JobDbContext.cs | ✅ |
| Src/CrispyWaffle.HttpClient/Serialization/SystemTextJsonSerializer.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Scheduling/DelayedJobScheduler.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Scheduling/CronJobScheduler.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Abstractions/JobEntity.cs | ✅ |
| Src/CrispyWaffle.HttpClient/IHttpClientWrapper.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Core/BackgroundJobQueue.cs | ✅ |
| Src/CrispyWaffle.HttpClient/HttpResponse{T}.cs | ✅ |
| Src/CrispyWaffle.HttpClient/ServiceCollectionExtensions.cs | ✅ |
| Src/CrispyWaffle.HttpClient/HttpRequestOptions.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Core/JobDispatcher.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Persistence/InMemoryJobStore.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Persistence/EfCoreJobStore.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Extensions/ServiceCollectionExtensions.cs | ✅ |
| Src/CrispyWaffle.HttpClient/RetryPolicy.cs | ✅ |
| Src/CrispyWaffle.BackgroundJobs/Core/BackgroundWorker.cs | ✅ |
| Src/CrispyWaffle.HttpClient/HttpClientWrapper.cs | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
|
Hi @ducdaiii 👋, Thank you so much for your pull request! 🙌 I appreciate the time and effort you put into this contribution. Thanks again for your valuable contribution! 🚀 |
guibranco
removed
the
🚦 awaiting triage
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
Caution Review the following alerts detected in dependencies. According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. Learn more about Socket for GitHub.
|
|
@gstraccini csharpier |
|
Running CSharpier on this branch! 🔧 |
|
✅ Build CrispyWaffle 10.0.1546 completed (commit 03be3cc68d by @guibranco) |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sonarcsharp (reported by Codacy) found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 13768420 | Triggered | Generic Password | b1fde96 | .github/workflows/release.yml | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
@gstraccini csharpier |
|
Running CSharpier on this branch! 🔧 |
|
❌ CSharpier failed! |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Directory.Packages.props (2)
22-22: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Microsoft.Extensions.Hosting.Abstractionsstill pinned to legacy 2.2.0.A prior review requested bumping this to 9.0.8 to match the rest of the Microsoft.Extensions.* pins added in this same change (DependencyInjection, EntityFrameworkCore, Options, Logging are all 9.0.8). The current diff still leaves it at 2.2.0, which is inconsistent with the newly added pins on adjacent lines.
♻️ Suggested fix
- <PackageVersion Include="Microsoft.Extensions.Hosting.Abstractions" Version="2.2.0" /> + <PackageVersion Include="Microsoft.Extensions.Hosting.Abstractions" Version="9.0.8" />🤖 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 `@Directory.Packages.props` at line 22, The Microsoft.Extensions.Hosting.Abstractions package version is still pinned to the legacy 2.2.0 value and should be aligned with the other Microsoft.Extensions.* package pins in Directory.Packages.props. Update the PackageVersion entry for Microsoft.Extensions.Hosting.Abstractions to the same 9.0.8 version used by the adjacent DependencyInjection, EntityFrameworkCore, Options, and Logging entries.
17-17: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winLegacy 2.2.0 package still present despite prior "addressed" note.
A previous review flagged
Microsoft.AspNetCore.Hosting.Abstractionsat Version="2.2.0" as conflicting with the newer .NET target packages and requested its removal, noting it was marked addressed in a later commit. However, the current diff still pins it at 2.2.0. SinceSrc/CrispyWaffle.BackgroundJobs/CrispyWaffle.BackgroundJobs.csprojreferences this package versionlessly against a net8.0 target, this legacy 2.x package remains mixed with 9.x/10.x Extensions packages in the same dependency graph.♻️ Suggested fix
- <PackageVersion Include="Microsoft.AspNetCore.Hosting.Abstractions" Version="2.2.0" />🤖 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 `@Directory.Packages.props` at line 17, The package version pin for Microsoft.AspNetCore.Hosting.Abstractions is still left at the legacy 2.2.0 release and should be removed or updated to match the modern net8.0 dependency set. Update the shared version in Directory.Packages.props and verify the CrispyWaffle.BackgroundJobs project continues to resolve through the intended ASP.NET Core/Extensions package family without mixing in the old 2.x package.
🧹 Nitpick comments (1)
Directory.Packages.props (1)
23-23: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign
Microsoft.Extensions.Httpwith the rest of theMicrosoft.Extensions.*pins
This is the only 10.x package in the set; if that version isn’t intentional, pin it to 9.0.8 to keep the central package graph consistent.🤖 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 `@Directory.Packages.props` at line 23, The Microsoft.Extensions.Http package version is inconsistent with the rest of the Microsoft.Extensions.* pins. Update the central PackageVersion entry for Microsoft.Extensions.Http to match the 9.0.8 baseline used by the other Microsoft.Extensions packages, unless the 10.x upgrade is intentional, and keep the change limited to the PackageVersion declaration.
🤖 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.
Duplicate comments:
In `@Directory.Packages.props`:
- Line 22: The Microsoft.Extensions.Hosting.Abstractions package version is
still pinned to the legacy 2.2.0 value and should be aligned with the other
Microsoft.Extensions.* package pins in Directory.Packages.props. Update the
PackageVersion entry for Microsoft.Extensions.Hosting.Abstractions to the same
9.0.8 version used by the adjacent DependencyInjection, EntityFrameworkCore,
Options, and Logging entries.
- Line 17: The package version pin for Microsoft.AspNetCore.Hosting.Abstractions
is still left at the legacy 2.2.0 release and should be removed or updated to
match the modern net8.0 dependency set. Update the shared version in
Directory.Packages.props and verify the CrispyWaffle.BackgroundJobs project
continues to resolve through the intended ASP.NET Core/Extensions package family
without mixing in the old 2.x package.
---
Nitpick comments:
In `@Directory.Packages.props`:
- Line 23: The Microsoft.Extensions.Http package version is inconsistent with
the rest of the Microsoft.Extensions.* pins. Update the central PackageVersion
entry for Microsoft.Extensions.Http to match the 9.0.8 baseline used by the
other Microsoft.Extensions packages, unless the 10.x upgrade is intentional, and
keep the change limited to the PackageVersion declaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d988723-197d-46a5-950b-f6b034b88c83
📒 Files selected for processing (1)
Directory.Packages.props
Reformat multiline interface and class methods to enhance code readability. Align method parameters across multiple lines consistently in all files. Add missing newline characters at the end of files for better standards compliance. These changes improve maintainability without altering functionality, ensuring code consistency across the project.
Refactor `BackgroundWorker` by extracting methods for fetching and processing jobs, improving code readability and maintenance. This change introduces the following methods: `FetchNextJobAsync`, `RescheduleIfNotDueAsync`, and `ProcessJobWithScopeAsync`. In `HttpClientWrapper`, extracted methods to configure and apply HTTP client settings, enhancing clarity. New helper methods include `ConfigureBaseAddress`, `ConfigureTimeout`, `ApplyDefaultHeaders`, `EnsureAcceptHeader`, and `MergeRequestHeaders`. These changes promote better code organization and ease of understanding while maintaining existing functionality.
Improve code readability by reformatting method signatures and block structures to follow consistent line breaking and indentation practices. This affects the BackgroundWorker and HttpClientWrapper classes, where method headers are now more uniformly formatted, enhancing maintainability. These changes were done to ensure that the code follows better styling conventions and makes the codebase more consistent and easier to read.
5 participants
User description
📑 Description
This pull request adds a complete, lightweight background job module to the CrispyWaffle project.
It includes support for both in-memory and EF Core persistence, a scoped DI-friendly handler system, retry/backoff policies, scheduling (delayed & cron stub), monitoring, and metrics.
Key highlights:
IBackgroundJob,IBackgroundJobHandler<TData>,IJobStore,IJobScheduler,JobEntity,JobPriority,JobStatusBackgroundWorker,BackgroundJobQueue,JobDispatcher,JobHandlerRegistry,JobOptions,JobResultInMemoryJobStore,EfCoreJobStore,JobDbContextDelayedJobScheduler,CronJobService(stub)JobMetrics,MonitoringMiddlewareServiceCollectionExtensionsto register jobs, handlers, and options with DISendEmailHandlerandSendEmailPayloadshowing how to define a custom handler/jobs/metrics.✅ Checks
☢️ Does this introduce a breaking change?
ℹ Additional Information
JobDbContextregistration and migrations.services.AddJobHandler<THandler, TData>("handler-name").Note
I'm currently writing a description for your pull request. I should be done shortly (<1 minute). Please don't edit the description field until I'm finished, or we may overwrite each other. If I find nothing to write about, I'll delete this message.
Description
Changes walkthrough 📝
2 files
CrispyWaffle.HttpClient.csproj
New HTTP Client Wrapper Project AddedSrc/CrispyWaffle.HttpClient/CrispyWaffle.HttpClient.csproj
CrispyWaffle.BackgroundJobs.csproj
Background Jobs Project File UpdatedSrc/CrispyWaffle.BackgroundJobs/CrispyWaffle.BackgroundJobs.csproj
7 files
HttpClientWrapper.cs
HTTP Client Wrapper ImplementationSrc/CrispyWaffle.HttpClient/HttpClientWrapper.cs
HttpRequestOptions.cs
HTTP Request Options Class CreatedSrc/CrispyWaffle.HttpClient/HttpRequestOptions.cs
settings.
HttpResponse{T}.cs
Generic HTTP Response Wrapper AddedSrc/CrispyWaffle.HttpClient/HttpResponse{T}.cs
ServiceCollectionExtensions.cs
Service Collection Extensions for HTTP ClientSrc/CrispyWaffle.HttpClient/ServiceCollectionExtensions.cs
RetryPolicy.cs
Retry Policy for HTTP RequestsSrc/CrispyWaffle.HttpClient/RetryPolicy.cs
IJsonSerializer.cs
JSON Serializer Interface AddedSrc/CrispyWaffle.HttpClient/Serialization/IJsonSerializer.cs
SystemTextJsonSerializer.cs
System.Text.Json Serializer ImplementationSrc/CrispyWaffle.HttpClient/Serialization/SystemTextJsonSerializer.cs
Summary by CodeRabbit