Refactor: Invoke JobRegistry events outside WriteLock to prevent blocking and deadlocks

[Copilot]

✅ Refactoring Complete: Move Event Invocation Outside WriteLock

Summary

Successfully refactored JobRegistry.cs to invoke lifecycle events (JobAdded, JobRemoved, JobUpdated) outside the WriteLock, preventing blocking of the scheduler and reducing deadlock risk.

Changes Completed

• Refactor ScheduleJob(string, Func<IServiceProvider, CancellationToken, Task>) to invoke JobAdded outside WriteLock
• Refactor ScheduleJob(string, Func<IServiceProvider, CancellationToken, Task>, TimeSpan?) to invoke JobAdded outside WriteLock
• Refactor RemoveJob(Guid) to invoke JobRemoved outside WriteLock
• Refactor UpdateSchedule(Guid, string) to invoke JobUpdated outside WriteLock
• Update XML documentation comments on the three event properties to reflect new behavior
• Create comprehensive tests to verify events fire correctly and locks are released before invocation
• Run existing tests to ensure no regressions - ✅ All 155 tests passing
• Run code review - ✅ No issues found
• Run security checks - ✅ No vulnerabilities found
• Address PR review feedback - ✅ Fixed async void handlers, removed unused variables, removed redundant null checks

Latest Changes (PR Review Feedback)

Test Improvements:

• Replaced async void event handlers with synchronous handlers using GetAwaiter().GetResult() to avoid unobserved exceptions
• Fixed unused getJobsTask variables by properly awaiting them with Task.WhenAll()
• All concurrency tests now properly verify that GetJobs() completes while event handlers are running

Code Quality Improvements:

• Removed redundant null checks in RemoveJob (removedJob != null check)
• Removed redundant null checks in UpdateSchedule (updatedJob != null && existingJob != null checks)
• Added null-forgiving operators where variables are guaranteed non-null by control flow

Testing Results

✅ All 155 tests passing (147 original + 8 new tests)

New test coverage in MiniCronTests.EventInvocationOutsideLock.cs:

• ✅ JobRegistry_JobAdded_InvokedOutsideLock_AllowsConcurrentReads - Verifies JobAdded event doesn't block GetJobs()
• ✅ JobRegistry_JobRemoved_InvokedOutsideLock_AllowsConcurrentReads - Verifies JobRemoved event doesn't block GetJobs()
• ✅ JobRegistry_JobUpdated_InvokedOutsideLock_AllowsConcurrentReads - Verifies JobUpdated event doesn't block GetJobs()
• ✅ JobRegistry_EventHandler_Exception_DoesNotPreventOperation - Exception handling works correctly
• ✅ JobRegistry_EventHandler_CanCallBackIntoRegistry_NoDeadlock - Safe re-entrance verified
• ✅ JobRegistry_JobAdded_EventArgs_ContainCorrectJobInfo - Event args validation
• ✅ JobRegistry_JobUpdated_EventArgs_ContainBothJobs - Event args validation for updates
• ✅ JobRegistry_NoEventSubscribers_NoEventInvocation - Operations work without subscribers

Benefits Achieved

✅ Performance: Slow event listeners no longer block the entire scheduling system
✅ Reliability: Significantly reduced deadlock risk when event handlers interact with JobRegistry
✅ Concurrency: GetJobs() calls can proceed immediately without waiting for event handlers
✅ Safety: Event handlers can safely call back into JobRegistry (tested and verified)

Security Summary

✅ CodeQL analysis completed - No vulnerabilities found
✅ No new security risks introduced
✅ Exception handling properly maintained

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Refactor: Invoking events inside WriteLock creates risk of deadlocks and thread contention</issue_title>
<issue_description>## Description
In JobRegistry.cs, the lifecycle events (JobAdded, JobRemoved, JobUpdated) are currently invoked while holding a ReaderWriterLockSlim in Write mode.

While LockRecursionPolicy.SupportsRecursion is enabled, invoking external delegates (event handlers) while holding a lock is a dangerous pattern. If a consumer subscribes to these events and performs a long-running operation (e.g., database logging, HTTP call) or complex logic, it will:

1. Block the Scheduler: The MiniCronBackgroundService calls GetJobs() (which requires a Read Lock) every tick. If the Write Lock is held by a slow event handler, the background service will hang.
2. Increase Deadlock Risk: If the event handler attempts to interact with other thread-safe components that might eventually call back into JobRegistry from a different thread context, it could lead to deadlocks or thread starvation.

Technical Details

Location: src/MiniCron.Core/Services/JobRegistry.cs

Current Implementation:

public Guid ScheduleJob(...)
{
    // ...
    _lock.EnterWriteLock();
    try
    {
        _jobs.Add(job.Id, job);
        // ⚠️ PROBLEM: External code invoked inside the lock
        JobAdded?.Invoke(this, new JobEventArgs(job)); 
    }
    finally
    {
        _lock.ExitWriteLock();
    }
    return job.Id;
}

This pattern is repeated for RemoveJob and UpdateSchedule.

Impact

• Performance: A slow event listener effectively pauses the entire scheduling system.
• Reliability: Exceptions thrown inside the event handler (if not caught) could leave the registry in an inconsistent state or crash the calling thread, although the try/finally block currently ensures the lock is released.

Proposed Solution

Refactor the methods to capture the necessary data, release the lock, and then invoke the events.

Suggested Pattern:

public Guid ScheduleJob(...)
{
    // ... validation ...
    var job = new CronJob(cronExpression, action);
    JobEventArgs? eventArgs = null;

    _lock.EnterWriteLock();
    try
    {
        _jobs.Add(job.Id, job);
        // Prepare event args but do not invoke yet
        if (JobAdded != null)
        {
            eventArgs = new JobEventArgs(job);
        }
    }
    finally
    {
        _lock.ExitWriteLock();
    }

    // Invoke outside the lock
    if (eventArgs != null)
    {
        try 
        {
            JobAdded?.Invoke(this, eventArgs);
        }
        catch (Exception ex)
        {
            _logger?.LogError(ex, "Error in JobAdded event handler");
        }
    }

    return job.Id;
}

This ensures that the internal state of the registry is protected, but the performance of the registry is not coupled to the performance of external event handlers.</issue_description>

Comments on the Issue (you are @copilot in this section)

• Fixes Refactor: Invoking events inside WriteLock creates risk of deadlocks and thread contention #62

---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

[Copilot]

Pull request overview

This PR refactors JobRegistry to invoke lifecycle events (JobAdded, JobRemoved, JobUpdated) outside the WriteLock, preventing slow event handlers from blocking registry operations and eliminating deadlock risk when handlers call back into the registry.

Key Changes:

• Event invocation moved outside WriteLock with exception handling
• Event data captured inside lock, then events invoked after lock release
• Updated XML documentation to reflect new "invoked after lock released" behavior
• Added 8 comprehensive concurrency tests verifying non-blocking behavior

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

File	Description
src/MiniCron.Core/Services/JobRegistry.cs	Refactored ScheduleJob, RemoveJob, and UpdateSchedule to prepare event args inside lock but invoke events outside; updated XML docs to clarify events are raised after lock release
src/MiniCron.Tests/MiniCronTests.EventInvocationOutsideLock.cs	Added comprehensive test suite verifying events don't block concurrent reads, exception handling, safe re-entrance, and correct event args

[jeanlrnt]

@copilot apply changes based on the comments in this thread