From 584729748293b3f2d3d7484ac506e11b6a97ac2c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 2 Jan 2026 11:47:46 +0000 Subject: [PATCH 1/3] Initial plan From 6ca5682ba84024c8fb9e5420dcadcec0bf298e6a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 2 Jan 2026 11:53:28 +0000 Subject: [PATCH 2/3] refactor(JobRegistry): Move event invocations outside WriteLock - JobRegistry.cs, MiniCronTests.EventInvocationOutsideLock.cs Co-authored-by: jeanlrnt <63308635+jeanlrnt@users.noreply.github.com> --- src/MiniCron.Core/Services/JobRegistry.cs | 139 ++++++--- ...iniCronTests.EventInvocationOutsideLock.cs | 269 ++++++++++++++++++ 2 files changed, 367 insertions(+), 41 deletions(-) create mode 100644 src/MiniCron.Tests/MiniCronTests.EventInvocationOutsideLock.cs diff --git a/src/MiniCron.Core/Services/JobRegistry.cs b/src/MiniCron.Core/Services/JobRegistry.cs index 8e05cf4..97018eb 100644 --- a/src/MiniCron.Core/Services/JobRegistry.cs +++ b/src/MiniCron.Core/Services/JobRegistry.cs @@ -14,11 +14,11 @@ public class JobRegistry : IDisposable /// Occurs when a job is added to the registry. /// /// - /// This event is raised inside the write lock, ensuring event handlers observe consistent registry state. - /// Event handlers should be lightweight to avoid blocking other registry operations. + /// This event is raised after the write lock is released, ensuring event handlers do not block registry operations. + /// Event handlers observe the job state at the time of addition. /// - /// Note: Event handlers may call back into the JobRegistry (e.g., RemoveJob, UpdateSchedule, ScheduleJob) - /// as the lock supports recursion. However, this should be done judiciously to avoid performance degradation. + /// Note: Event handlers can safely call back into the JobRegistry (e.g., RemoveJob, UpdateSchedule, ScheduleJob) + /// without risk of deadlock, as the event is invoked outside the lock. /// /// public event EventHandler? JobAdded; @@ -27,11 +27,11 @@ public class JobRegistry : IDisposable /// Occurs when a job is removed from the registry. /// /// - /// This event is raised inside the write lock, ensuring event handlers observe consistent registry state. - /// Event handlers should be lightweight to avoid blocking other registry operations. + /// This event is raised after the write lock is released, ensuring event handlers do not block registry operations. + /// Event handlers observe the job state at the time of removal. /// - /// Note: Event handlers may call back into the JobRegistry (e.g., RemoveJob, UpdateSchedule, ScheduleJob) - /// as the lock supports recursion. However, this should be done judiciously to avoid performance degradation. + /// Note: Event handlers can safely call back into the JobRegistry (e.g., RemoveJob, UpdateSchedule, ScheduleJob) + /// without risk of deadlock, as the event is invoked outside the lock. /// /// public event EventHandler? JobRemoved; @@ -40,11 +40,11 @@ public class JobRegistry : IDisposable /// Occurs when a job's schedule is updated in the registry. /// /// - /// This event is raised inside the write lock, ensuring event handlers observe consistent registry state. - /// Event handlers should be lightweight to avoid blocking other registry operations. + /// This event is raised after the write lock is released, ensuring event handlers do not block registry operations. + /// Event handlers observe the job state at the time of update. /// - /// Note: Event handlers may call back into the JobRegistry (e.g., RemoveJob, UpdateSchedule, ScheduleJob) - /// as the lock supports recursion. However, this should be done judiciously to avoid performance degradation. + /// Note: Event handlers can safely call back into the JobRegistry (e.g., RemoveJob, UpdateSchedule, ScheduleJob) + /// without risk of deadlock, as the event is invoked outside the lock. /// /// public event EventHandler? JobUpdated; @@ -65,26 +65,39 @@ public Guid ScheduleJob(string cronExpression, Func @@ -119,25 +132,38 @@ public Guid ScheduleJob(string cronExpression, FuncTrue if the job was found and removed; otherwise, false. public bool RemoveJob(Guid jobId) { + CronJob? removedJob = null; + JobEventArgs? eventArgs = null; + _lock.EnterWriteLock(); try { @@ -178,22 +207,34 @@ public bool RemoveJob(Guid jobId) } // At this point, job is guaranteed to be non-null because Remove returned true - _logger?.LogInformation("Job removed: {JobId} {Cron}", jobId, job!.CronExpression); - try - { - JobRemoved?.Invoke(this, new JobEventArgs(job!)); - } - catch (Exception ex) + removedJob = job!; + _logger?.LogInformation("Job removed: {JobId} {Cron}", jobId, removedJob.CronExpression); + + // Prepare event args if there are subscribers, but don't invoke yet + if (JobRemoved != null) { - _logger?.LogError(ex, "Unhandled exception in JobRemoved event handler for job {JobId} {Cron}", jobId, job!.CronExpression); + eventArgs = new JobEventArgs(removedJob); } - - return true; } finally { _lock.ExitWriteLock(); } + + // Invoke event outside the lock + if (eventArgs != null && removedJob != null) + { + try + { + JobRemoved?.Invoke(this, eventArgs); + } + catch (Exception ex) + { + _logger?.LogError(ex, "Unhandled exception in JobRemoved event handler for job {JobId} {Cron}", jobId, removedJob.CronExpression); + } + } + + return removedJob != null; } /// @@ -206,21 +247,41 @@ public bool UpdateSchedule(Guid jobId, string newCronExpression) { CronHelper.ValidateCronExpression(newCronExpression); + CronJob? updatedJob = null; + CronJob? existingJob = null; + JobEventArgs? eventArgs = null; + _lock.EnterWriteLock(); try { - if (!_jobs.TryGetValue(jobId, out var existingJob)) + if (!_jobs.TryGetValue(jobId, out var oldJob)) { return false; } - var updatedJob = existingJob with { CronExpression = newCronExpression }; + existingJob = oldJob; + updatedJob = existingJob with { CronExpression = newCronExpression }; _jobs[jobId] = updatedJob; _logger?.LogInformation("Job updated: {JobId} {OldCron} -> {NewCron}", jobId, existingJob.CronExpression, newCronExpression); + + // Prepare event args if there are subscribers, but don't invoke yet + if (JobUpdated != null) + { + eventArgs = new JobEventArgs(updatedJob, existingJob); + } + } + finally + { + _lock.ExitWriteLock(); + } + + // Invoke event outside the lock + if (eventArgs != null && updatedJob != null && existingJob != null) + { try { - JobUpdated?.Invoke(this, new JobEventArgs(updatedJob, existingJob)); + JobUpdated?.Invoke(this, eventArgs); } catch (Exception ex) { @@ -231,13 +292,9 @@ public bool UpdateSchedule(Guid jobId, string newCronExpression) existingJob.CronExpression, updatedJob.CronExpression); } - - return true; - } - finally - { - _lock.ExitWriteLock(); } + + return updatedJob != null; } /// diff --git a/src/MiniCron.Tests/MiniCronTests.EventInvocationOutsideLock.cs b/src/MiniCron.Tests/MiniCronTests.EventInvocationOutsideLock.cs new file mode 100644 index 0000000..e3e60fc --- /dev/null +++ b/src/MiniCron.Tests/MiniCronTests.EventInvocationOutsideLock.cs @@ -0,0 +1,269 @@ +using MiniCron.Core.Services; + +namespace MiniCron.Tests; + +public partial class MiniCronTests +{ + /// + /// Verifies that JobAdded event is invoked outside the write lock, + /// allowing concurrent GetJobs() calls to proceed without blocking. + /// + [Fact] + public async Task JobRegistry_JobAdded_InvokedOutsideLock_AllowsConcurrentReads() + { + var registry = new JobRegistry(); + var eventHandlerStarted = new TaskCompletionSource(); + var eventHandlerCanComplete = new TaskCompletionSource(); + var getJobsCompleted = new TaskCompletionSource(); + + // Subscribe to JobAdded with a long-running handler + registry.JobAdded += async (_, _) => + { + eventHandlerStarted.SetResult(true); + // Simulate a slow event handler (e.g., logging to database) + await eventHandlerCanComplete.Task; + }; + + // Schedule a job (which will trigger JobAdded event) + var scheduleTask = Task.Run(() => + { + registry.ScheduleJob("* * * * *", () => { }); + }); + + // Wait for event handler to start + await eventHandlerStarted.Task.WaitAsync(TimeSpan.FromSeconds(5)); + + // While event handler is running, try to read jobs + // This should NOT block because the event is invoked outside the lock + var getJobsTask = Task.Run(() => + { + var jobs = registry.GetJobs(); + Assert.Single(jobs); // Job should be in registry + getJobsCompleted.SetResult(true); + }); + + // Wait for GetJobs to complete (should happen quickly) + await getJobsCompleted.Task.WaitAsync(TimeSpan.FromSeconds(5)); + + // Now allow event handler to complete + eventHandlerCanComplete.SetResult(true); + await scheduleTask; + } + + /// + /// Verifies that JobRemoved event is invoked outside the write lock. + /// + [Fact] + public async Task JobRegistry_JobRemoved_InvokedOutsideLock_AllowsConcurrentReads() + { + var registry = new JobRegistry(); + var jobId = registry.ScheduleJob("* * * * *", () => { }); + + var eventHandlerStarted = new TaskCompletionSource(); + var eventHandlerCanComplete = new TaskCompletionSource(); + var getJobsCompleted = new TaskCompletionSource(); + + // Subscribe to JobRemoved with a long-running handler + registry.JobRemoved += async (_, _) => + { + eventHandlerStarted.SetResult(true); + await eventHandlerCanComplete.Task; + }; + + // Remove the job (which will trigger JobRemoved event) + var removeTask = Task.Run(() => + { + registry.RemoveJob(jobId); + }); + + // Wait for event handler to start + await eventHandlerStarted.Task.WaitAsync(TimeSpan.FromSeconds(5)); + + // While event handler is running, try to read jobs + var getJobsTask = Task.Run(() => + { + var jobs = registry.GetJobs(); + Assert.Empty(jobs); // Job should be removed from registry + getJobsCompleted.SetResult(true); + }); + + // Wait for GetJobs to complete + await getJobsCompleted.Task.WaitAsync(TimeSpan.FromSeconds(5)); + + // Allow event handler to complete + eventHandlerCanComplete.SetResult(true); + await removeTask; + } + + /// + /// Verifies that JobUpdated event is invoked outside the write lock. + /// + [Fact] + public async Task JobRegistry_JobUpdated_InvokedOutsideLock_AllowsConcurrentReads() + { + var registry = new JobRegistry(); + var jobId = registry.ScheduleJob("* * * * *", () => { }); + + var eventHandlerStarted = new TaskCompletionSource(); + var eventHandlerCanComplete = new TaskCompletionSource(); + var getJobsCompleted = new TaskCompletionSource(); + + // Subscribe to JobUpdated with a long-running handler + registry.JobUpdated += async (_, _) => + { + eventHandlerStarted.SetResult(true); + await eventHandlerCanComplete.Task; + }; + + // Update the job schedule (which will trigger JobUpdated event) + var updateTask = Task.Run(() => + { + registry.UpdateSchedule(jobId, "*/5 * * * *"); + }); + + // Wait for event handler to start + await eventHandlerStarted.Task.WaitAsync(TimeSpan.FromSeconds(5)); + + // While event handler is running, try to read jobs + var getJobsTask = Task.Run(() => + { + var jobs = registry.GetJobs(); + Assert.Single(jobs); + Assert.Equal("*/5 * * * *", jobs[0].CronExpression); // Should see updated schedule + getJobsCompleted.SetResult(true); + }); + + // Wait for GetJobs to complete + await getJobsCompleted.Task.WaitAsync(TimeSpan.FromSeconds(5)); + + // Allow event handler to complete + eventHandlerCanComplete.SetResult(true); + await updateTask; + } + + /// + /// Verifies that exceptions in event handlers are caught and logged, + /// and do not prevent the operation from completing. + /// + [Fact] + public void JobRegistry_EventHandler_Exception_DoesNotPreventOperation() + { + var registry = new JobRegistry(); + var exceptionThrown = false; + + // Subscribe with a handler that throws + registry.JobAdded += (_, _) => + { + exceptionThrown = true; + throw new InvalidOperationException("Test exception in event handler"); + }; + + // Schedule a job - should complete successfully despite exception + var jobId = registry.ScheduleJob("* * * * *", () => { }); + + Assert.True(exceptionThrown); + Assert.NotEqual(Guid.Empty, jobId); + + // Job should still be in registry + var jobs = registry.GetJobs(); + Assert.Single(jobs); + Assert.Equal(jobId, jobs[0].Id); + } + + /// + /// Verifies that event handlers can safely call back into JobRegistry + /// without deadlock, since events are invoked outside the lock. + /// + [Fact] + public void JobRegistry_EventHandler_CanCallBackIntoRegistry_NoDeadlock() + { + var registry = new JobRegistry(); + Guid? secondJobId = null; + + // Subscribe with a handler that schedules another job + registry.JobAdded += (sender, args) => + { + var reg = (JobRegistry)sender!; + // This should not deadlock because event is invoked outside the lock + if (args.Job.CronExpression == "* * * * *") + { + secondJobId = reg.ScheduleJob("*/5 * * * *", () => { }); + } + }; + + // Schedule first job - should trigger handler that schedules second job + var firstJobId = registry.ScheduleJob("* * * * *", () => { }); + + Assert.NotNull(secondJobId); + Assert.NotEqual(firstJobId, secondJobId); + + // Both jobs should be in registry + var jobs = registry.GetJobs(); + Assert.Equal(2, jobs.Count); + } + + /// + /// Verifies that JobAdded event receives correct job information. + /// + [Fact] + public void JobRegistry_JobAdded_EventArgs_ContainCorrectJobInfo() + { + var registry = new JobRegistry(); + JobEventArgs? capturedArgs = null; + + registry.JobAdded += (_, args) => + { + capturedArgs = args; + }; + + var jobId = registry.ScheduleJob("*/10 * * * *", () => { }); + + Assert.NotNull(capturedArgs); + Assert.Equal(jobId, capturedArgs.Job.Id); + Assert.Equal("*/10 * * * *", capturedArgs.Job.CronExpression); + Assert.Null(capturedArgs.PreviousJob); + } + + /// + /// Verifies that JobUpdated event receives both current and previous job information. + /// + [Fact] + public void JobRegistry_JobUpdated_EventArgs_ContainBothJobs() + { + var registry = new JobRegistry(); + var jobId = registry.ScheduleJob("* * * * *", () => { }); + + JobEventArgs? capturedArgs = null; + registry.JobUpdated += (_, args) => + { + capturedArgs = args; + }; + + registry.UpdateSchedule(jobId, "*/15 * * * *"); + + Assert.NotNull(capturedArgs); + Assert.Equal(jobId, capturedArgs.Job.Id); + Assert.Equal("*/15 * * * *", capturedArgs.Job.CronExpression); + Assert.NotNull(capturedArgs.PreviousJob); + Assert.Equal("* * * * *", capturedArgs.PreviousJob.CronExpression); + } + + /// + /// Verifies that events are not invoked if there are no subscribers. + /// + [Fact] + public void JobRegistry_NoEventSubscribers_NoEventInvocation() + { + var registry = new JobRegistry(); + + // No subscribers - operations should work normally + var jobId = registry.ScheduleJob("* * * * *", () => { }); + Assert.NotEqual(Guid.Empty, jobId); + + var updated = registry.UpdateSchedule(jobId, "*/5 * * * *"); + Assert.True(updated); + + var removed = registry.RemoveJob(jobId); + Assert.True(removed); + } +} From e5cf6cacac626b705f89f22a742dacdb70499660 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 2 Jan 2026 12:07:42 +0000 Subject: [PATCH 3/3] fix: Address PR review feedback - remove async void handlers, unused variables, and redundant null checks Co-authored-by: jeanlrnt <63308635+jeanlrnt@users.noreply.github.com> --- src/MiniCron.Core/Services/JobRegistry.cs | 10 +++++----- ...iniCronTests.EventInvocationOutsideLock.cs | 19 ++++++++++--------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/MiniCron.Core/Services/JobRegistry.cs b/src/MiniCron.Core/Services/JobRegistry.cs index 97018eb..56f7918 100644 --- a/src/MiniCron.Core/Services/JobRegistry.cs +++ b/src/MiniCron.Core/Services/JobRegistry.cs @@ -222,7 +222,7 @@ public bool RemoveJob(Guid jobId) } // Invoke event outside the lock - if (eventArgs != null && removedJob != null) + if (eventArgs != null) { try { @@ -230,7 +230,7 @@ public bool RemoveJob(Guid jobId) } catch (Exception ex) { - _logger?.LogError(ex, "Unhandled exception in JobRemoved event handler for job {JobId} {Cron}", jobId, removedJob.CronExpression); + _logger?.LogError(ex, "Unhandled exception in JobRemoved event handler for job {JobId} {Cron}", jobId, removedJob!.CronExpression); } } @@ -277,7 +277,7 @@ public bool UpdateSchedule(Guid jobId, string newCronExpression) } // Invoke event outside the lock - if (eventArgs != null && updatedJob != null && existingJob != null) + if (eventArgs != null) { try { @@ -289,8 +289,8 @@ public bool UpdateSchedule(Guid jobId, string newCronExpression) ex, "Unhandled exception in JobUpdated event handler for job {JobId} with cron change {OldCron} -> {NewCron}", jobId, - existingJob.CronExpression, - updatedJob.CronExpression); + existingJob!.CronExpression, + updatedJob!.CronExpression); } } diff --git a/src/MiniCron.Tests/MiniCronTests.EventInvocationOutsideLock.cs b/src/MiniCron.Tests/MiniCronTests.EventInvocationOutsideLock.cs index e3e60fc..c398b68 100644 --- a/src/MiniCron.Tests/MiniCronTests.EventInvocationOutsideLock.cs +++ b/src/MiniCron.Tests/MiniCronTests.EventInvocationOutsideLock.cs @@ -17,11 +17,12 @@ public async Task JobRegistry_JobAdded_InvokedOutsideLock_AllowsConcurrentReads( var getJobsCompleted = new TaskCompletionSource(); // Subscribe to JobAdded with a long-running handler - registry.JobAdded += async (_, _) => + registry.JobAdded += (_, _) => { eventHandlerStarted.SetResult(true); // Simulate a slow event handler (e.g., logging to database) - await eventHandlerCanComplete.Task; + // Block synchronously until signaled + eventHandlerCanComplete.Task.GetAwaiter().GetResult(); }; // Schedule a job (which will trigger JobAdded event) @@ -43,7 +44,7 @@ public async Task JobRegistry_JobAdded_InvokedOutsideLock_AllowsConcurrentReads( }); // Wait for GetJobs to complete (should happen quickly) - await getJobsCompleted.Task.WaitAsync(TimeSpan.FromSeconds(5)); + await Task.WhenAll(getJobsCompleted.Task.WaitAsync(TimeSpan.FromSeconds(5)), getJobsTask); // Now allow event handler to complete eventHandlerCanComplete.SetResult(true); @@ -64,10 +65,10 @@ public async Task JobRegistry_JobRemoved_InvokedOutsideLock_AllowsConcurrentRead var getJobsCompleted = new TaskCompletionSource(); // Subscribe to JobRemoved with a long-running handler - registry.JobRemoved += async (_, _) => + registry.JobRemoved += (_, _) => { eventHandlerStarted.SetResult(true); - await eventHandlerCanComplete.Task; + eventHandlerCanComplete.Task.GetAwaiter().GetResult(); }; // Remove the job (which will trigger JobRemoved event) @@ -88,7 +89,7 @@ public async Task JobRegistry_JobRemoved_InvokedOutsideLock_AllowsConcurrentRead }); // Wait for GetJobs to complete - await getJobsCompleted.Task.WaitAsync(TimeSpan.FromSeconds(5)); + await Task.WhenAll(getJobsCompleted.Task.WaitAsync(TimeSpan.FromSeconds(5)), getJobsTask); // Allow event handler to complete eventHandlerCanComplete.SetResult(true); @@ -109,10 +110,10 @@ public async Task JobRegistry_JobUpdated_InvokedOutsideLock_AllowsConcurrentRead var getJobsCompleted = new TaskCompletionSource(); // Subscribe to JobUpdated with a long-running handler - registry.JobUpdated += async (_, _) => + registry.JobUpdated += (_, _) => { eventHandlerStarted.SetResult(true); - await eventHandlerCanComplete.Task; + eventHandlerCanComplete.Task.GetAwaiter().GetResult(); }; // Update the job schedule (which will trigger JobUpdated event) @@ -134,7 +135,7 @@ public async Task JobRegistry_JobUpdated_InvokedOutsideLock_AllowsConcurrentRead }); // Wait for GetJobs to complete - await getJobsCompleted.Task.WaitAsync(TimeSpan.FromSeconds(5)); + await Task.WhenAll(getJobsCompleted.Task.WaitAsync(TimeSpan.FromSeconds(5)), getJobsTask); // Allow event handler to complete eventHandlerCanComplete.SetResult(true);