Feature/sentry.quartz (Quartz v4 Alpha version) - #5505
Conversation
… dependencies and streamlining SentryCronJobListener options handling
# Conflicts: # .generated.NoMobile.sln # Sentry.sln
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 5 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 625b3e8. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
michaelmairegger
changed the title
| quartz.UseSentry(builder.Services, options => | ||
| { | ||
| var jobKey1 = new JobKey(nameof(FirstJob)); | ||
| quartz.AddJob<FirstJob>(opts => opts.WithIdentity(jobKey1)); |
There was a problem hiding this comment.
Bug: Job registrations are placed inside the UseSentry options lambda, which executes too late, causing jobs to never be scheduled.
Severity: HIGH
Suggested Fix
Move the job and trigger registration calls, such as quartz.AddJob<FirstJob>(...), outside of the options => { ... } lambda and place them directly within the builder.Services.AddQuartz(quartz => { ... }) configuration block.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: samples/Sentry.Samples.Quartz/Program.cs#L19-L22
Potential issue: In `Program.cs`, the calls to `quartz.AddJob<...>` and
`quartz.AddTrigger<...>` are located within the options configuration lambda passed to
`quartz.UseSentry()`. This lambda is registered via `serviceCollection.Configure()` and
is only executed during DI options resolution, which happens after the Quartz
configuration phase has already finished. As a result, the jobs are never registered
with the Quartz scheduler, leading to a silent failure where the application runs but
the scheduled jobs never execute.
|
|
||
| if (_sentryCronInformation.TryGetValue(jobType, out var info) && info.ShouldWriteStatusToSentry) | ||
| { | ||
| var status = jobException is not null ? CheckInStatus.Error : CheckInStatus.Ok; | ||
| _fireInstanceId.TryRemove(context.FireInstanceId, out var checkInId); |
There was a problem hiding this comment.
Bug: A race condition on the WarningShownForSecondsParameterIssue flag can cause a warning to be logged multiple times during concurrent job executions.
Severity: LOW
Suggested Fix
Use a lock statement around the read and write operations on the WarningShownForSecondsParameterIssue flag to ensure the check-and-set operation is atomic and thread-safe.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/Sentry.Quartz/SentryCronJobListener.cs#L48-L52
Potential issue: The `UpsertCronMonitor` method in `SentryCronJobListener` contains a
race condition. The check and subsequent update of the
`information.WarningShownForSecondsParameterIssue` flag is not an atomic operation. If
multiple instances of the same job type execute concurrently, it's possible for multiple
threads to read the flag as `false` simultaneously, leading to the warning being logged
multiple times instead of just once as intended.
1 participant
This PR implements and showcases Quartz.Net integration for Sentry
Note: Quartz.Net v4 is still alpha, therefore we should wait until v4 is finally released.
fixes #4601