v1.0.21#57
Merged
Merged
Conversation
…rter and guard job re-dispatch by status Scope 1 — EF Core DateTime read normalization (closes #55 scope 1): - Add ClockDateTimeValueConverter and ClockDateTimeOffsetValueConverter, both backed by IClock.NormalizeToUtc, covering DateTime, DateTime?, DateTimeOffset, and DateTimeOffset? via EF Core null-lifting. - Inject optional IClock into AetherDbContext<TDbContext> primary constructor so the model-building pipeline can access the clock without breaking existing derived DbContextes. - Thread IClock through ConfigureByConvention → TryConfigureDateTimeUtc and apply the appropriate converter to every DateTime/DateTimeOffset property at model build time, ensuring consistent UTC Kind/Offset on both read and write paths for all entities. Scope 2 — DaprJobExecutionBridge status-aware dispatch guard (closes #55 scope 2): - Add IJobStore.GetByJobNameAsync(string, BackgroundJobStatus?, CancellationToken) overload; nulls preserves existing no-filter semantics. - Implement the overload in EfCoreJobStore with a conditional Where clause. - Update DaprJobExecutionBridge to query only Scheduled jobs; log a warning and return early when the job is not found in the expected state, preventing re-dispatch of already completed, failed, or cancelled jobs.
…ency - Remove overloaded and consolidate active-job filtering (Scheduled | Running) into the single method; introduce computed property on - Change to look up existing jobs by instead of uid=502(U0B006) gid=20(staff) groups=20(staff),501(awagent),12(everyone),61(localaccounts),79(_appserverusr),80(admin),81(_appserveradm),701(com.apple.sharepoint.group.1),702(com.apple.sharepoint.group.2),33(_appstore),98(_lpadmin),100(_lpoperator),204(_developer),250(_analyticsusers),395(com.apple.access_ftp),398(com.apple.access_screensharing),399(com.apple.access_ssh),400(com.apple.access_remote_ae), enabling upsert-by-name semantics - Add status to idempotency check in so previously-failed jobs are not reprocessed - Pass when scheduling Dapr jobs to allow rescheduling without conflicts - Comment out marker in to prevent automatic UoW wrapping on all application services
…erter-for-iclock-normalization-daprjobexecutionbridge-status-aware-lookup fix(background-jobs): normalize DateTime via EF Core ValueConverter and simplify job store API
Reviewer's GuideThis PR refines background job idempotency and activity tracking, hardens domain event outbox publishing, and introduces clock-based DateTime normalization in EF Core while wiring clock usage through the base DbContext; it also relaxes the UoW aspect’s automatic application and removes repo meta files. Sequence diagram for Dapr-based background job scheduling and idempotent executionsequenceDiagram
actor ExternalScheduler
participant DaprJobScheduler
participant DaprJobsClient
participant DaprJobExecutionBridge
participant IJobStore
participant JobDispatcher
participant JobHandler
ExternalScheduler->>DaprJobScheduler: ScheduleAsync(jobName, schedule, payload)
DaprJobScheduler->>DaprJobsClient: ScheduleJobAsync(jobName, schedule, payload, overwrite=true)
DaprJobsClient-->>DaprJobScheduler: scheduled
ExternalScheduler->>DaprJobExecutionBridge: ExecuteAsync(jobName, payload)
DaprJobExecutionBridge->>IJobStore: GetByJobNameAsync(jobName)
IJobStore-->>DaprJobExecutionBridge: BackgroundJobInfo or null (only IsActive)
alt job not found or not active
DaprJobExecutionBridge->>DaprJobExecutionBridge: LogWarning(Job not found in Scheduled state)
DaprJobExecutionBridge-->>ExternalScheduler: return
else active job found
DaprJobExecutionBridge->>JobDispatcher: DispatchAsync(jobInfo)
JobDispatcher->>JobDispatcher: IsJobAlreadyProcessed(jobInfo, jobId, handlerName)
alt Status is Completed or Cancelled or Failed
JobDispatcher->>JobDispatcher: LogWarning(Skipping reprocessing)
JobDispatcher-->>DaprJobExecutionBridge: return
else Status is Scheduled or Running
JobDispatcher->>JobHandler: Handle(jobInfo.Payload)
JobHandler-->>JobDispatcher: completed
JobDispatcher-->>DaprJobExecutionBridge: return
end
DaprJobExecutionBridge-->>ExternalScheduler: return
end
Sequence diagram for domain event outbox publishing with error handlingsequenceDiagram
participant AetherDbContext
participant DomainEventDispatcher
participant EventBus
participant Logger
AetherDbContext->>DomainEventDispatcher: DispatchEventsAsync(eventEnvelopes)
loop for each eventEnvelope
DomainEventDispatcher->>Logger: LogDebug(Writing domain event to outbox)
DomainEventDispatcher->>EventBus: PublishAsync(event, metadata, subject, useOutbox=true)
alt publish succeeds
EventBus-->>DomainEventDispatcher: completed
DomainEventDispatcher->>Logger: LogDebug(Successfully wrote domain event to outbox)
else publish throws
EventBus--xDomainEventDispatcher: Exception
DomainEventDispatcher->>Logger: LogError(exception, Failed to publish domain event to outbox, continue)
DomainEventDispatcher->>DomainEventDispatcher: Proceed to next event
end
end
Class diagram for EF Core clock-based DateTime normalizationclassDiagram
class AetherDbContext_TDbContext {
+AetherDbContext_TDbContext(DbContextOptions_TDbContext options, IDomainEventSink eventSink, IClock clock)
-IDomainEventSink eventSink
-IClock clock
+OnModelCreating(ModelBuilder modelBuilder)
+ConfigureBaseProperties_TEntity(ModelBuilder modelBuilder)
}
class AetherEntityTypeBuilderExtensions {
+static ConfigureByConvention(EntityTypeBuilder builder, IClock clock)
+static TryConfigureDateTimeUtc(EntityTypeBuilder builder, IClock clock)
}
class ClockDateTimeValueConverter {
+ClockDateTimeValueConverter(IClock clock)
}
class ClockDateTimeOffsetValueConverter {
+ClockDateTimeOffsetValueConverter(IClock clock)
}
class IClock {
+NormalizeToUtc_DateTime(DateTime value) DateTime
+NormalizeToUtc_DateTimeOffset(DateTimeOffset value) DateTimeOffset
}
class ValueConverter_DateTime_DateTime
class ValueConverter_DateTimeOffset_DateTimeOffset
AetherDbContext_TDbContext ..> IClock
AetherDbContext_TDbContext ..> IDomainEventSink
AetherDbContext_TDbContext ..> AetherEntityTypeBuilderExtensions
AetherEntityTypeBuilderExtensions ..> ClockDateTimeValueConverter
AetherEntityTypeBuilderExtensions ..> ClockDateTimeOffsetValueConverter
ClockDateTimeValueConverter --|> ValueConverter_DateTime_DateTime
ClockDateTimeOffsetValueConverter --|> ValueConverter_DateTimeOffset_DateTimeOffset
ClockDateTimeValueConverter ..> IClock
ClockDateTimeOffsetValueConverter ..> IClock
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. Warning Ignoring CodeRabbit configuration file changes. For security, only the configuration from the base branch is applied for open source repositories. Note
|
| Cohort / File(s) | Summary |
|---|---|
Configuration Removal .coderabbit.yaml, .github/CODEOWNERS |
Deleted repository configuration files and code ownership mappings. |
Domain Model Enhancements framework/src/BBT.Aether.Domain/BBT/Aether/Domain/Entities/BackgroundJobInfo.cs, framework/src/BBT.Aether.Domain/BBT/Aether/Domain/Repositories/IJobStore.cs |
Added IsActive computed property to BackgroundJobInfo and clarified IJobStore documentation for active job retrieval. |
Background Job Infrastructure framework/src/BBT.Aether.Infrastructure/BBT/Aether/BackgroundJob/... |
Updated job status handling: EfCoreJobStore now queries by job name with status filtering; DaprJobScheduler enables overwrite mode; DaprJobExecutionBridge improves logging when jobs not found; JobDispatcher treats Failed as terminal state; UnitOfWorkAspectProvider empties default marker interface list. |
Event Dispatching framework/src/BBT.Aether.Infrastructure/BBT/Aether/Domain/EntityFrameworkCore/DomainEventDispatcher.cs |
Added try/catch wrapping to the outbox publish path to catch and log errors while continuing dispatch. |
EF Core DateTime Normalization framework/src/BBT.Aether.Infrastructure/BBT/Aether/Domain/EntityFrameworkCore/AetherDbContext.cs, framework/src/BBT.Aether.Infrastructure/BBT/Aether/Domain/EntityFrameworkCore/Modeling/AetherEntityTypeBuilderExtensions.cs, framework/src/BBT.Aether.Infrastructure/BBT/Aether/Domain/EntityFrameworkCore/ValueComparers/ClockDateTime...Converter.cs |
Added optional IClock parameter to AetherDbContext constructor and EF Core extension methods; introduced ClockDateTimeValueConverter and ClockDateTimeOffsetValueConverter for UTC normalization of temporal properties. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
- 15 support local domain events #16: Modifies domain-event infrastructure and integrates event dispatching into EF Core DbContext flow with shared changes to
DomainEventDispatcherandAetherDbContext. - Release v1.0 #29: Overlaps on core files like
AetherDbContext,DaprJobExecutionBridge,JobDispatcher, andEfCoreJobStoreindicating shared domain event and background job handling patterns. - feat(telemetry): HTTP body logging, enricher refactor, and background job improvements #50: Modifies background-job implementation files (
BackgroundJobInfo,EfCoreJobStore,JobDispatcher) with overlapping changes to job status handling and persistence logic.
Suggested labels
enhancement
Suggested reviewers
- middt
- ukaratas
- safakcakir
Poem
🐰 A clockwork garden of jobs takes flight,
With failed states marked and active states bright,
DateTime converters in UTC we trust,
Normalizing moments from dust unto dust!
The dispatcher catches and logs with great care,
While schedules and handlers skip nimbly through air. ⏰
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
master
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
DomainEventDispatcher, the broadcatch (Exception)aroundPublishAsynclogs and swallows failures while continuing the loop, which may make it hard to detect partial outbox write failures; consider surfacing these errors (e.g., via an aggregate exception, callback, or result object) or making the 'continue on failure' behavior explicitly configurable. - Changing
EfCoreJobStore.SaveAsyncto detect existing jobs byGetByJobNameAsync(which now only returns Scheduled/Running) means jobs with the sameJobNamein a Completed/Cancelled/Failed state will no longer be updated but instead create a new row; if that’s not intentional, consider either including inactive statuses in the lookup or documenting/enforcing job-name uniqueness per lifecycle explicitly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `DomainEventDispatcher`, the broad `catch (Exception)` around `PublishAsync` logs and swallows failures while continuing the loop, which may make it hard to detect partial outbox write failures; consider surfacing these errors (e.g., via an aggregate exception, callback, or result object) or making the 'continue on failure' behavior explicitly configurable.
- Changing `EfCoreJobStore.SaveAsync` to detect existing jobs by `GetByJobNameAsync` (which now only returns Scheduled/Running) means jobs with the same `JobName` in a Completed/Cancelled/Failed state will no longer be updated but instead create a new row; if that’s not intentional, consider either including inactive statuses in the lookup or documenting/enforcing job-name uniqueness per lifecycle explicitly.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the CodeRabbit and CODEOWNERS configuration files and integrates IClock into the EF Core infrastructure to ensure consistent UTC normalization for DateTime and DateTimeOffset properties via new value converters. Background job management is updated with an IsActive property, expanded idempotency checks to include failed states, and Dapr scheduling overrides. Critical feedback highlights a potential unique constraint violation in EfCoreJobStore due to status-filtered lookups during save operations, the risk of silent data loss from swallowing exceptions in the DomainEventDispatcher, and the need to remove commented-out code in the UnitOfWorkAspectProvider.
| // Check if job already exists | ||
| var existingJob = await _dbContext.BackgroundJobs | ||
| .FirstOrDefaultAsync(j => j.Id == jobInfo.Id, cancellationToken); | ||
| var existingJob = await GetByJobNameAsync(jobInfo.JobName, cancellationToken); |
There was a problem hiding this comment.
Using GetByJobNameAsync here is problematic because it now filters for only Scheduled or Running jobs. If a job with the same JobName already exists in a terminal state (e.g., Completed or Failed), existingJob will be null, and the code will attempt to AddAsync a new record. This will likely cause a unique constraint violation in the database or result in duplicate job entries. Use a lookup that does not filter by status for this existence check.
var existingJob = await _dbContext.BackgroundJobs
.FirstOrDefaultAsync(j => j.JobName == jobInfo.JobName, cancellationToken);
Comment on lines
+38
to
+54
| try | ||
| { | ||
| logger.LogDebug("Writing domain event to outbox: {EventType} (Name: {EventName}, Version: {Version})", | ||
| metadata.EventType.Name, metadata.EventName, metadata.Version); | ||
|
|
||
| // Write directly to outbox within the transaction | ||
| await eventBus.PublishAsync(@event, metadata, | ||
| subject: EventSubjectExtractor.ExtractSubject(@event), | ||
| useOutbox: true, cancellationToken); | ||
| await eventBus.PublishAsync(@event, metadata, | ||
| subject: EventSubjectExtractor.ExtractSubject(@event), | ||
| useOutbox: true, cancellationToken); | ||
|
|
||
| logger.LogDebug("Successfully wrote domain event to outbox: {EventType}", metadata.EventType.Name); | ||
| logger.LogDebug("Successfully wrote domain event to outbox: {EventType}", metadata.EventType.Name); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| logger.LogError(ex, | ||
| "Failed to publish domain event to outbox: {EventType}. Continuing with remaining events", | ||
| metadata.EventType.Name); | ||
| } |
There was a problem hiding this comment.
Swallowing exceptions when publishing to the outbox can lead to silent data loss. When useOutboxDirectly is enabled, the event publication is typically a critical part of the transaction. If the outbox write fails, the entire operation should fail and roll back to ensure consistency. Exceptions should be allowed to propagate.
logger.LogDebug("Writing domain event to outbox: {EventType} (Name: {EventName}, Version: {Version})",
metadata.EventType.Name, metadata.EventName, metadata.Version);
await eventBus.PublishAsync(@event, metadata,
subject: EventSubjectExtractor.ExtractSubject(@event),
useOutbox: true, cancellationToken);
logger.LogDebug("Successfully wrote domain event to outbox: {EventType}", metadata.EventType.Name);| private readonly static HashSet<string> MarkerInterfaceNames = new() | ||
| { | ||
| "BBT.Aether.Application.IApplicationService" | ||
| //"BBT.Aether.Application.IApplicationService" |
There was a problem hiding this comment.
Commented-out code should be removed to maintain code cleanliness. If this marker interface is no longer intended for automatic Unit of Work application, the entry should be deleted entirely.
Up to standards ✅🟢 Issues
|
|
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Improve background job idempotency and time handling while hardening domain event outbox publishing.
Bug Fixes:
Enhancements:
Chores:
Summary by CodeRabbit
Release Notes
New Features
IsActiveproperty to background jobs for status checkingBug Fixes
Chores