Background Jobs: Refine RecurringBackgroundJobBase API

[ronaldbarendse]

Summary

Refinements to RecurringBackgroundJobBase, which was newly introduced in #22331 and is currently in release/17.5.0 / release/18.0.0 but not yet shipped to consumers (no RC tagged). The refinements significantly improve the ergonomics of writing recurring jobs, especially jobs whose schedule changes at runtime, and they add an IgnoredDelay signalling capability that mirrors what already exists for Period.

What changes

1. Protected setters on Period and IgnoredDelay, with auto-raising events. The setters validate the value, no-op on same value, and raise PeriodChanged / IgnoredDelayChanged automatically. Jobs no longer need to maintain their own _period field plus a custom event override just to react to config changes — Period = newValue is now enough.

2. Period is required via the base constructor. RecurringBackgroundJobBase(TimeSpan period) stores the initial value directly without raising PeriodChanged. This replaces the previous "abstract Period { get; }" contract, eliminates the implicit-default question, and enforces initialisation at compile time.

3. IDisposable on the base. Standard Dispose() / protected virtual Dispose(bool disposing) pattern. Disposal clears _periodChanged and _ignoredDelayChanged so the disposed job does not retain subscriber references and cannot invoke listeners post-teardown.

4. Concrete jobs migrated. TempFileCleanupJob, ReportSiteJob, InstructionProcessJob, and TouchServerJob updated to pass Period via base(period). TouchServerJob in particular shrinks substantially — no more custom event override, no custom _period field, and it now also disposes its IOptionsMonitor.OnChange(...) registration via the new Dispose(bool) hook.

The previously-shipped TouchServerJob pattern (manual _period field + custom PeriodChanged event override + manual _periodChanged?.Invoke(...)) is exactly the pattern the docs PR Complex example currently shows. After this PR, the canonical pattern is roughly half the code (see "Required docs updates" below).

Breaking changes

Change	Impact
RecurringBackgroundJobBase constructor now requires TimeSpan period	Subclasses must call : base(period). Source-breaking but compile-time-detected.
RecurringBackgroundJobBase.Period changed from abstract to virtual with protected set	Existing public override TimeSpan Period => ... still compiles.
RecurringBackgroundJobBase now implements IDisposable	DI disposes instances on container shutdown. Subclasses that previously had no Dispose need no changes.

Why these belong in 17.5 (and 18.0) rather than 17.6/18.1

• Not yet shipped. RecurringBackgroundJobBase is brand-new in Background Jobs: Rewrite RecurringHostedServiceBase with SemaphoreSlim and add signalling support #22331. release/17.5.0 and release/18.0.0 branches exist but no *-rc has been tagged. No consumer can currently depend on the old shape.
• Small and contained. Breaks only affect direct subclasses of RecurringBackgroundJobBase. The IRecurringBackgroundJob interface contract is unchanged (the V19 TODO comments on the events are purely informational — no runtime change).
• Avoids back-to-back deprecations. Shipping the awkward shape and then refactoring in 17.6/18.1 would mean two obsoletion shims on a brand-new API. Landing the clean shape in the RC means consumers learn the right pattern on day one.
• Improves the worst case. The mutable-period + raise-event-on-config-change pattern is the one most likely to be copy-pasted. Shipping the verbose pre-refinement shape would cement that as the canonical pattern in user code.

Changes required to docs PR umbraco/UmbracoDocs#8082

@AndyButland — these are the updates needed if this lands.

1. Minimal example: use : base(period), drop the Period override

 public class CleanUpYourRoom : RecurringBackgroundJobBase
 {
-    public override TimeSpan Period => TimeSpan.FromMinutes(60);
+    public CleanUpYourRoom()
+        : base(TimeSpan.FromMinutes(60))
+    {
+    }

     public override Task RunJobAsync(CancellationToken cancellationToken)
     {
         // your job code goes here
         return Task.CompletedTask;
     }
 }

2. DI example: same pattern, just chain : base(...)

 public CleanUpYourRoom(
     IContentService contentService,
     ICoreScopeProvider scopeProvider)
+    : base(TimeSpan.FromMinutes(60))
 {
     _contentService = contentService;
     _scopeProvider = scopeProvider;
 }
-
-public override TimeSpan Period => TimeSpan.FromMinutes(60);

3. Complex example: dramatically simplified

The current Complex example has ~15 lines of _period + _periodChanged + custom event override + manual invoke. The new equivalent is:

public class CleanUpYourRoom : RecurringBackgroundJobBase
{
    private readonly IContentService _contentService;
    private readonly IServerRoleAccessor _serverRoleAccessor;
    private readonly IProfilingLogger _profilingLogger;
    private readonly ILogger<CleanUpYourRoom> _logger;
    private readonly ICoreScopeProvider _scopeProvider;
    private readonly IDisposable? _onChangeRegistration;

    public CleanUpYourRoom(
        IContentService contentService,
        IServerRoleAccessor serverRoleAccessor,
        IProfilingLogger profilingLogger,
        ILogger<CleanUpYourRoom> logger,
        IOptionsMonitor<HealthChecksSettings> healthChecksSettings,
        ICoreScopeProvider scopeProvider)
        : base(healthChecksSettings.CurrentValue.Notification.Period)
    {
        _contentService = contentService;
        _serverRoleAccessor = serverRoleAccessor;
        _profilingLogger = profilingLogger;
        _logger = logger;
        _scopeProvider = scopeProvider;

        // When the period config changes, assign Period — the setter raises PeriodChanged.
        _onChangeRegistration = healthChecksSettings.OnChange(x => Period = x.Notification.Period);
    }

    public override ServerRole[] ServerRoles => Enum.GetValues<ServerRole>();

    public override Task RunJobAsync(CancellationToken cancellationToken)
    {
        // ... unchanged ...
    }

    protected override void Dispose(bool disposing)
    {
        if (disposing)
        {
            _onChangeRegistration?.Dispose();
        }

        base.Dispose(disposing);
    }
}

Worth calling out in the prose around this example:

• The manual _periodChanged event override is no longer required for the common "external config drives the period" case.
• The protected Period setter is the new mechanism — assignment raises the event automatically and no-ops if the value is unchanged.
• IOptionsMonitor.OnChange returns an IDisposable and should be disposed via the base's Dispose(bool) hook.

4. New IgnoredDelayChanged event

The current docs PR documents the IgnoredDelay property but does not mention IgnoredDelayChanged. Add a short subsection (e.g. under IgnoredDelay or near PeriodChanged):

IgnoredDelayChanged

Mirrors PeriodChanged. The base class raises this event automatically when IgnoredDelay is assigned a new value via the protected setter. The recurring background job host listens for it and interrupts any in-progress ignored back-off so the new value is picked up immediately.

This makes it possible to start a job with IgnoredDelay = Timeout.InfiniteTimeSpan (effectively disabled after the first ignored execution until further notice), and later re-enable it by assigning a finite value — useful when an external signal indicates that the previously-ignored condition has cleared.

5. Manual-only jobs snippet

-public override TimeSpan Period => Timeout.InfiniteTimeSpan;
+public MyJob() : base(Timeout.InfiniteTimeSpan) { }

 public override TimeSpan Delay => Timeout.InfiniteTimeSpan;

6. (Optional) "Base Classes" section

Brief note that RecurringBackgroundJobBase implements IDisposable with a protected virtual Dispose(bool) hook, and that subclasses with disposable resources (e.g. an IOptionsMonitor.OnChange registration) should override that hook and call base.Dispose(disposing).

Test plan

• New RecurringBackgroundJobBaseTests (12 tests) covers: constructor sets initial Period, infinite period allowed, negative throws, IgnoredDelay default, both setters raise the change event on different value, no-op on same value, infinite allowed, negative throws, Dispose clears PeriodChanged and IgnoredDelayChanged subscribers.
• Existing RecurringBackgroundJobHostedServiceTests unchanged and passing.
• RecurringBackgroundJobHostedServiceRunnerTests updated: internal TestJobA / TestJobB call : base(TimeSpan.FromSeconds(30)).
• Build clean. No new warnings.

Fixes the TouchServerJob cleanup gap as a side effect — the previous code held a globalSettings.OnChange(...) registration with no disposal pathway.

Once this is approved, it should also be cherry-picked to release/18.0.0.

[claude]

Claude finished @ronaldbarendse's task in 6m 48s —— View job

---

PR Review

Target: origin/release/17.5.0 · Based on commit: 4e1a0f52 · Skipped: 0 noise files out of 10 total

This PR refines RecurringBackgroundJobBase (introduced in #22331, pre-RC) to add protected setters on Period/IgnoredDelay that auto-raise change events, require initial Period via constructor, add IDisposable support, and introduce a new IgnoredDelayChanged signal that lets the hosted service interrupt an in-progress back-off when the delay is reassigned at runtime.

• Modified public API: RecurringBackgroundJobBase (new required TimeSpan period constructor; Period changed from abstract to virtual with protected set; IgnoredDelay gains protected set; IgnoredDelayChanged event added; IDisposable implemented; OnPeriodChanged/OnIgnoredDelayChanged virtual hooks added); IRecurringBackgroundJob (IgnoredDelayChanged event added with no-op default)
• Affected implementations (outside this PR): None — all four RecurringBackgroundJobBase subclasses in src/ are migrated in this PR; the new interface member uses a no-op default so existing direct IRecurringBackgroundJob implementations compile unchanged
• Breaking changes: Removing the implicit parameterless constructor of RecurringBackgroundJobBase is source-breaking for any external subclass. Per CLAUDE.md Pattern 1, the standard remedy is to preserve the old constructor with [Obsolete]. The "not yet released — no RC tagged" justification is compelling and accepted here; if partner or preview consumers are building against the release branch, they would be broken.
• Other changes: RecurringBackgroundJobHostedService.IgnoreAndWaitAsync now uses a while(true) loop with a rotated CancellationTokenSource to handle mid-wait IgnoredDelay changes; TouchServerJob gains a disposal path for its previously-leaked IOptionsMonitor.OnChange registration.

---

Important

• RecurringBackgroundJobHostedService.cs:220–226: Potential ObjectDisposedException in OnIgnoredDelayChanged during shutdown. Dispose(bool) first unsubscribes the handler, then calls _ignoredDelayChangeCts.Dispose(). If the handler is already in-flight when the unsubscription runs (a config change callback arriving on a threadpool thread during shutdown), it will call Interlocked.Exchange capturing the just-disposed CTS as oldCts, then oldCts.Cancel() throws ObjectDisposedException. The window is narrow but real in any job that reacts to live config. Minimal fix:

  private void OnIgnoredDelayChanged(object? sender, EventArgs e)
  {
      CancellationTokenSource oldCts = Interlocked.Exchange(ref _ignoredDelayChangeCts, new CancellationTokenSource());
      try
      {
          oldCts.Cancel();
      }
      catch (ObjectDisposedException)
      {
          // Race: CTS disposed during shutdown before this handler could cancel it.
      }
  }

Suggestions

• RecurringBackgroundJobBase.cs:132–133 and 142–143: OnPeriodChanged and OnIgnoredDelayChanged invoke the private backing delegates (_periodChanged, _ignoredDelayChanged) directly rather than dispatching through the virtual event. This means a subclass that overrides PeriodChanged without also overriding OnPeriodChanged will silently fail to notify — the base Period setter calls OnPeriodChanged, bypassing the override. The remarks document the obligation, but it's a subtle footgun. Consider whether calling through the event accessor (PeriodChanged?.Invoke(this, e)) directly (or calling ((EventHandler?)PeriodChanged)?.Invoke(this, e)) would be safer — though that carries its own trade-offs.

• RecurringBackgroundJobBase.cs:43–44: The constructor XML doc notes the "stored without raising PeriodChanged" behaviour inline. This is correct and intentional, but a reader setting Period via the setter later may not see it. Worth mirroring the remark on the Period property setter (lines 58–59) alongside the existing "raises PeriodChanged" note.

---

Approved with Suggestions for improvement

Good to go, but please carefully consider the importance of the suggestions.

The race condition in OnIgnoredDelayChanged is worth addressing before merge — narrow window, trivial fix. Everything else is solid: IDisposable is correctly implemented, event subscription/unsubscription is symmetric, the IgnoreAndWaitAsync elapsed-time arithmetic correctly handles all transitions (infinite→finite, reduce delay, zero-exit), and the test coverage is comprehensive (12 unit tests + 4 new hosted-service tests covering the key back-off transitions).

[Copilot]

Pull request overview

Refines the new recurring background jobs API to make runtime schedule/back-off changes easier and safer by enhancing RecurringBackgroundJobBase, wiring up IgnoredDelayChanged handling in the hosted service, and migrating existing concrete jobs and tests to the new constructor-based period initialization.

Changes:

• Update RecurringBackgroundJobBase to require an initial Period via constructor, add protected setters for Period/IgnoredDelay that auto-raise change events, and implement IDisposable.
• Enhance RecurringBackgroundJobHostedService to react to IgnoredDelayChanged by interrupting in-progress ignored back-off waits and recomputing remaining time.
• Migrate built-in jobs and unit tests to the updated base-class API and add new base-class unit tests.

Reviewed changes

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

Show a summary per file

File	Description
src/Umbraco.Infrastructure/BackgroundJobs/RecurringBackgroundJobBase.cs	Makes period constructor-required, adds change-signalling setters + events, and implements IDisposable.
src/Umbraco.Infrastructure/BackgroundJobs/IRecurringBackgroundJob.cs	Adds IgnoredDelayChanged (default implementation) and updates event commentary for future nullable-event change.
src/Umbraco.Infrastructure/BackgroundJobs/RecurringBackgroundJobHostedService.cs	Subscribes to IgnoredDelayChanged and adds logic to interrupt/recompute ignored back-off waits.
src/Umbraco.Infrastructure/BackgroundJobs/Jobs/TempFileCleanupJob.cs	Migrates to : base(period) instead of overriding Period.
src/Umbraco.Infrastructure/BackgroundJobs/Jobs/ReportSiteJob.cs	Migrates to : base(period) instead of overriding Period.
src/Umbraco.Infrastructure/BackgroundJobs/Jobs/ServerRegistration/InstructionProcessJob.cs	Migrates to : base(period) instead of storing a _period field + override.
src/Umbraco.Infrastructure/BackgroundJobs/Jobs/ServerRegistration/TouchServerJob.cs	Migrates to constructor period + uses protected Period setter on options changes; disposes options registration via Dispose(bool).
tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/BackgroundJobs/RecurringBackgroundJobHostedServiceTests.cs	Adds tests verifying ignored back-off can be interrupted/recomputed when IgnoredDelay changes.
tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/BackgroundJobs/RecurringBackgroundJobHostedServiceRunnerTests.cs	Updates internal test jobs to call : base(period) per new base-class contract.
tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/BackgroundJobs/RecurringBackgroundJobBaseTests.cs	Adds unit tests for constructor validation, event raising/no-op behavior, and subscriber clearing on dispose.

[ronaldbarendse]

Thanks for the reviews. Pushed 9ce948c addressing the concrete fixes:

• OnIgnoredDelayChanged shutdown race — wrapped oldCts.Cancel() in a try/catch (ObjectDisposedException). The in-flight-during-Dispose window is narrow but real; swallowing is safe because the wait loop is already tearing down by the time we reach this point.
• Negative IgnoredDelay regression — restored the <= TimeSpan.Zero guard (with != Timeout.InfiniteTimeSpan exclusion) at the top of the IgnoreAndWaitAsync loop. The pre-refinement code had this defense; my new loop only checked == TimeSpan.Zero, which let negative values flow through ComputeNextDelay → clamped to zero → tight loop. Added a regression test Skips_Wait_When_IgnoredDelay_Is_Negative.
• Setter doc remark — mirrored the "stored without raising the event" note on both the Period and IgnoredDelay setter <remarks>, so the constructor/setter asymmetry is documented from either side.

On the OnPeriodChanged/OnIgnoredDelayChanged suggestion (dispatching through the virtual event accessor instead of the private backing): I considered this and deliberately kept the protected-virtual OnXxx method pattern. C# does not permit ?.Invoke() on an event member from inside its declaring type when add/remove are custom accessors — the event name only refers to the backing storage for field-like events. The OnXxx virtual is the canonical .NET pattern for this exact situation (cf. Control.OnClick, INotifyPropertyChanged implementations), and the <remarks> on both methods explicitly tell overriders to override OnXxx when overriding the event. There is no override of either event in the repo today, so the documented obligation only kicks in for future subclasses.

Happy to revisit if you would prefer a different trade-off (e.g. dropping virtual from the events so subclasses cannot override them and the base setter always dispatches correctly — at the cost of removing the TouchServerJob-style "custom event with its own backing" capability, which we no longer use anyway).

[AndyButland]

Looks good @ronaldbarendse. I pushed a small update to XML docs and introduced a helper for the "rotate and cancel" pattern you had used in a couple of places for the cancellation token.

I've verified with the various test jobs I had available from the last PR and all looks to work as expected. Will merge this in, cherry-pick up for 18 and update the docs.