fix(hangfire): Skip recurring jobs with unsupported schedules

src/Sentry.Hangfire/SentryHangfireOptions.cs

@@ -33,8 +33,10 @@ public class SentryHangfireOptions
     /// dropped, and cut to 50 characters. Defaults to <c>false</c>.
     /// </summary>
     /// <remarks>
-    /// Jobs marked with <see cref="SentryMonitorSlugAttribute"/> keep their monitor slug.
-    /// Use <see cref="ExcludedRecurringJobs"/> to leave out some recurring jobs.
+    /// Recurring jobs whose schedule Sentry can't represent are skipped, and so are recurring jobs that share a monitor
+    /// slug with another recurring job on a different schedule. <see cref="SendMonitorConfig"/> lists the schedules
+    /// Sentry can represent. Jobs marked with <see cref="SentryMonitorSlugAttribute"/> keep their monitor slug and
+    /// still send check-ins. Use <see cref="ExcludedRecurringJobs"/> to leave out some recurring jobs.
     /// </remarks>
     public bool MonitorAllRecurringJobs { get; set; }
 

src/Sentry.Hangfire/SentryServerFilter.cs

@@ -41,7 +41,7 @@ internal SentryServerFilter(IHub? hub, IDiagnosticLogger? logger, SentryHangfire
 
     public void OnPerforming(PerformingContext context)
     {
-        var monitorSlug = GetMonitorSlug(context);
+        var (monitorSlug, recurringJobId) = GetMonitorSlug(context);
         if (monitorSlug is null)
         {
             var jobType = context.BackgroundJob.Job.Type;
@@ -52,7 +52,18 @@ public void OnPerforming(PerformingContext context)
             return;
         }
 
-        var checkInId = CaptureInProgressCheckIn(context, monitorSlug);
+        var monitoredByDefault = recurringJobId is not null;
+        var schedule = _options.SendMonitorConfig || monitoredByDefault
+            ? GetRecurringJobSchedule(context, monitorSlug)
+            : null;
+        if (monitoredByDefault && schedule is null)
+        {
+            _logger?.LogDebug("Skipping creating a check-in for recurring job '{0}'. MonitorAllRecurringJobs only " +
+                              "monitors jobs whose schedule Sentry supports.", recurringJobId);
+            return;
+        }
+
+        var checkInId = CaptureInProgressCheckIn(monitorSlug, _options.SendMonitorConfig ? schedule : null);
 
         // Note that we may be overwriting context.Items[SentryCheckInIdKey] here, which is intentional. If that happens
         // then implicitly OnPerforming was called previously with the same context, but we never made it to OnPerformed
@@ -61,30 +72,33 @@ public void OnPerforming(PerformingContext context)
         context.Items[SentryMonitorSlugKey] = monitorSlug;
     }
 
-    private string? GetMonitorSlug(PerformContext context)
+    // The recurring job ID is set only when MonitorAllRecurringJobs supplies the slug.
+    private (string? MonitorSlug, string? RecurringJobId) GetMonitorSlug(PerformContext context)
     {
         var monitorSlug = context.GetJobParameter<string>(SentryMonitorSlugKey);
         if (monitorSlug is not null || !_options.MonitorAllRecurringJobs)
         {
-            return monitorSlug;
+            return (monitorSlug, null);
         }
 
         try
         {
             var recurringJobId = context.GetJobParameter<string>(RecurringJobIdKey);
-            return string.IsNullOrEmpty(recurringJobId) ? null : _options.GetRecurringJobSlug(recurringJobId!);
+            return string.IsNullOrEmpty(recurringJobId) || _options.GetRecurringJobSlug(recurringJobId!) is not { } slug
+                ? (null, null)
+                : (slug, recurringJobId);
         }
         catch (Exception e)
         {
             _logger?.LogError(e, "Failed to read the recurring job ID of job '{0}'.", context.BackgroundJob.Id);
-            return null;
+            return (null, null);
         }
     }
 
-    private SentryId CaptureInProgressCheckIn(PerformingContext context, string monitorSlug)
+    private SentryId CaptureInProgressCheckIn(string monitorSlug, (string Crontab, string TimeZone)? schedule)
     {
         Action<SentryMonitorOptions>? configureMonitorOptions = null;
-        if (_options.SendMonitorConfig && GetRecurringJobSchedule(context, monitorSlug) is (var crontab, var timeZone))
+        if (schedule is (var crontab, var timeZone))
         {
             configureMonitorOptions = options =>
             {

test/Sentry.Hangfire.Tests/ServerFilterTests.cs

@@ -419,7 +419,7 @@ public void OnPerforming_ExcludedRecurringJobWithAttribute_CapturesCheckIn()
     }
 
     [Fact]
-    public void OnPerforming_MonitorAllRecurringJobsIdsShareSlugWithDifferentCron_SendsCheckInWithoutMonitorConfig()
+    public void OnPerforming_MonitorAllRecurringJobsIdsShareSlugWithDifferentCron_DoesNotCaptureCheckIn()
     {
         // Arrange
         var fixture = new RecurringJobFixture
@@ -437,6 +437,87 @@ public void OnPerforming_MonitorAllRecurringJobsIdsShareSlugWithDifferentCron_Se
         // Act
         filter.OnPerforming(fixture.PerformingContext);
 
+        // Assert
+        fixture.Hub.DidNotReceiveWithAnyArgs().CaptureCheckIn(default!, default);
+        fixture.PerformingContext.Items.Should().NotContainKey(SentryServerFilter.SentryCheckInIdKey);
+    }
+
+    [Theory]
+    [InlineData("0 0 L * *", "UTC", true)]
+    [InlineData("0 * * * *", "Not A Time Zone", true)]
+    [InlineData("0 0 L * *", "UTC", false)]
+    public void OnPerforming_MonitorAllRecurringJobsScheduleNotSupportedBySentry_DoesNotCaptureCheckIn(
+        string cron, string timeZoneId, bool sendMonitorConfig)
+    {
+        // Arrange
+        var fixture = new RecurringJobFixture
+        {
+            AttributeSlug = null,
+            Slug = "test-recurring-job",
+            Cron = cron,
+            TimeZoneId = timeZoneId,
+            Options = { MonitorAllRecurringJobs = true }
+        };
+        var filter = fixture.GetSut(sendMonitorConfig);
+
+        // Act
+        filter.OnPerforming(fixture.PerformingContext);
+        filter.OnPerformed(new PerformedContext(fixture.PerformingContext, null, false, null));
+
+        // Assert
+        fixture.Hub.DidNotReceiveWithAnyArgs().CaptureCheckIn(default!, default);
+        fixture.PerformingContext.Items.Should().NotContainKey(SentryServerFilter.SentryCheckInIdKey);
+    }
+
+    [Fact]
+    public void OnPerforming_MonitorAllRecurringJobsRecurringJobDeleted_DoesNotCaptureCheckIn()
+    {
+        // Arrange
+        var fixture = new RecurringJobFixture
+        {
+            AttributeSlug = null,
+            Slug = "test-recurring-job",
+            RecurringJobExists = false,
+            Options = { MonitorAllRecurringJobs = true }
+        };
+        var filter = fixture.GetSut();
+
+        // Act
+        filter.OnPerforming(fixture.PerformingContext);
+
+        // Assert
+        fixture.Hub.DidNotReceiveWithAnyArgs().CaptureCheckIn(default!, default);
+    }
+
+    [Fact]
+    public void OnPerforming_MonitorAllRecurringJobsSendMonitorConfigDisabled_SendsCheckInWithoutMonitorConfig()
+    {
+        // Arrange
+        var fixture = new RecurringJobFixture
+        {
+            AttributeSlug = null,
+            Slug = "test-recurring-job",
+            Options = { MonitorAllRecurringJobs = true }
+        };
+        var filter = fixture.GetSut(sendMonitorConfig: false);
+
+        // Act
+        filter.OnPerforming(fixture.PerformingContext);
+
+        // Assert
+        fixture.AssertCheckInSentWithoutMonitorConfig();
+    }
+
+    [Fact]
+    public void OnPerforming_MonitorAllRecurringJobsAttributeWithScheduleNotSupportedBySentry_SendsCheckInWithoutMonitorConfig()
+    {
+        // Arrange
+        var fixture = new RecurringJobFixture { Cron = "0 0 L * *", Options = { MonitorAllRecurringJobs = true } };
+        var filter = fixture.GetSut();
+
+        // Act
+        filter.OnPerforming(fixture.PerformingContext);
+
         // Assert
         fixture.AssertCheckInSentWithoutMonitorConfig();
     }