Keep scheduled task runs from overlapping across instances

docs/installation.md

@@ -47,6 +47,21 @@ instance serves its own copy of the sitemap until the sitemap task runs again, a
 the push notification settings are saved again. A file left behind in `wwwroot` keeps being served after it is
 deleted in the admin, because the shop only deletes on the media path. Thumbnails are regenerated on the media path.
 
+Scheduled tasks need no setup: every instance runs the scheduler, and each due run is claimed in the database by
+exactly one of them. The claiming instance holds a lease on the task while it runs (renewed every minute, five
+minutes long), so a run that takes longer than the task's interval is not started again elsewhere, and a run whose
+instance died is picked up once the lease runs out.
+
+To keep a task on particular instances, set **Leased by machine name** on the task in the admin. In a container the
+machine name is the hostname, which changes on every deployment, so give those instances a stable name with
+`Application:InstanceName` and enter that name instead. Several instances may share it; one of them runs each due
+task:
+
+```bash
+docker run -d --name grandnode2-worker --link mongodb:mongo \
+  -e Application__InstanceName=worker grandnode/grandnode2
+```
+
 ## Open locally in an IDE
 
 Extract the source code package downloaded from the Releases tab to a folder (or clone the repository), and open

src/Business/Grand.Business.Core/Interfaces/System/ScheduleTasks/IScheduleTaskService.cs

@@ -40,13 +40,50 @@ public interface IScheduleTaskService
     ///     Atomically claims a single run of the task, so that only one application instance
     ///     executes it. The claim succeeds only if LastStartUtc still has the value the caller
     ///     read (compare-and-set) - a concurrent claim by another instance makes it fail.
+    ///     It also fails while LeasedUntilUtc of a previous run is still in the future,
+    ///     so a run that outlasts the interval is not started a second time elsewhere.
     /// </summary>
     /// <param name="taskId">Task identifier</param>
     /// <param name="expectedLastStartUtc">LastStartUtc value read before the claim</param>
     /// <param name="runStartUtc">New LastStartUtc value (start of the claimed run)</param>
+    /// <param name="leasedUntilUtc">Until when the claimed run blocks other instances, unless renewed</param>
     /// <param name="instanceId">Unique identifier of the claiming application instance</param>
     /// <returns>true if this instance won the claim and should execute the task</returns>
-    Task<bool> TryClaimTaskRun(string taskId, DateTime? expectedLastStartUtc, DateTime runStartUtc, string instanceId);
+    Task<bool> TryClaimTaskRun(string taskId, DateTime? expectedLastStartUtc, DateTime runStartUtc,
+        DateTime leasedUntilUtc, string instanceId);
+
+    /// <summary>
+    ///     Atomically claims a single run of the task without a lease - the run blocks other
+    ///     instances only until its interval elapses, as before leases were introduced.
+    /// </summary>
+    [Obsolete("Use the overload with leasedUntilUtc - a run claimed without a lease is started again elsewhere once its interval elapses, even if it is still running")]
+    Task<bool> TryClaimTaskRun(string taskId, DateTime? expectedLastStartUtc, DateTime runStartUtc,
+        string instanceId)
+    {
+        return TryClaimTaskRun(taskId, expectedLastStartUtc, runStartUtc, runStartUtc, instanceId);
+    }
+
+    /// <summary>
+    ///     Extends the lease of a running task. Does nothing when the instance no longer holds
+    ///     the lease (released, or taken over by another instance after it ran out).
+    /// </summary>
+    /// <param name="taskId">Task identifier</param>
+    /// <param name="instanceId">Unique identifier of the application instance running the task</param>
+    /// <param name="leasedUntilUtc">New end of the lease</param>
+    Task RenewTaskLease(string taskId, string instanceId, DateTime leasedUntilUtc);
+
+    /// <summary>
+    ///     Records the result of a claimed run and releases its lease. Writes only the result fields,
+    ///     and only while the instance still holds the lease - so it neither reverts changes saved in
+    ///     the admin during the run, nor overwrites the claim of an instance that took the task over
+    ///     after the lease ran out.
+    /// </summary>
+    /// <param name="taskId">Task identifier</param>
+    /// <param name="instanceId">Unique identifier of the application instance that ran the task</param>
+    /// <param name="succeeded">Whether the run completed without an error</param>
+    /// <param name="endUtc">End of the run</param>
+    /// <param name="disableTask">Disables the task (a failed run of a task that stops on error)</param>
+    Task CompleteTaskRun(string taskId, string instanceId, bool succeeded, DateTime endUtc, bool disableTask);
 
     /// <summary>
     ///     Delete the task

src/Core/Grand.Infrastructure/Configuration/AppConfig.cs

@@ -46,4 +46,12 @@ public class AppConfig
     ///     instance at the same shared volume. A relative path is resolved against the content root.
     /// </summary>
     public string MediaPath { get; set; }
+
+    /// <summary>
+    ///     Gets or sets the name this instance answers to when a scheduled task is pinned to a machine
+    ///     ("Leased by machine name"). Empty uses only the machine name, which in a container is the
+    ///     hostname and changes on every deployment. Several instances may share the name - one of them
+    ///     runs each due task.
+    /// </summary>
+    public string InstanceName { get; set; }
 }

src/Modules/Grand.Module.ScheduledTasks/BackgroundServices/ScheduleTaskService.cs

@@ -79,26 +79,68 @@ public virtual async Task UpdateTask(ScheduleTask task)
     }
 
     /// <summary>
-    ///     Atomically claims a single run of the task (compare-and-set on LastStartUtc)
+    ///     Atomically claims a single run of the task (compare-and-set on LastStartUtc,
+    ///     refused while the lease of a previous run has not run out)
     /// </summary>
     public virtual async Task<bool> TryClaimTaskRun(string taskId, DateTime? expectedLastStartUtc,
-        DateTime runStartUtc, string instanceId)
+        DateTime runStartUtc, DateTime leasedUntilUtc, string instanceId)
     {
         //truncate to milliseconds so the value survives the database round-trip unchanged
-        runStartUtc = new DateTime(runStartUtc.Ticks - runStartUtc.Ticks % TimeSpan.TicksPerMillisecond,
-            DateTimeKind.Utc);
+        runStartUtc = TruncateToMilliseconds(runStartUtc);
 
         //conditional update - matches only when no other instance has claimed the run
         //in the meantime; on MongoDB a single update is atomic, so exactly one instance wins
         await _taskRepository.UpdateOneAsync(
-            x => x.Id == taskId && x.LastStartUtc == expectedLastStartUtc,
+            x => x.Id == taskId && x.LastStartUtc == expectedLastStartUtc &&
+                 (x.LeasedUntilUtc == null || x.LeasedUntilUtc < runStartUtc),
             UpdateBuilder<ScheduleTask>.Create()
                 .Set(x => x.LastStartUtc, runStartUtc)
+                .Set(x => x.LeasedUntilUtc, TruncateToMilliseconds(leasedUntilUtc))
                 .Set(x => x.LeasedByInstance, instanceId));
 
-        //UpdateOneAsync does not report whether the filter matched - read back the winner
+        //UpdateOneAsync does not report whether the filter matched - read back the winner.
+        //LiteDB returns dates as local time, so compare in UTC - otherwise no claim ever
+        //succeeds on a host outside the UTC time zone
         var task = await GetTaskById(taskId);
-        return task != null && task.LeasedByInstance == instanceId && task.LastStartUtc == runStartUtc;
+        return task != null && task.LeasedByInstance == instanceId &&
+               task.LastStartUtc?.ToUniversalTime() == runStartUtc;
+    }
+
+    /// <summary>
+    ///     Extends the lease of a run, only while the given instance still holds it
+    /// </summary>
+    public virtual async Task RenewTaskLease(string taskId, string instanceId, DateTime leasedUntilUtc)
+    {
+        await _taskRepository.UpdateOneAsync(
+            x => x.Id == taskId && x.LeasedByInstance == instanceId && x.LeasedUntilUtc != null,
+            UpdateBuilder<ScheduleTask>.Create()
+                .Set(x => x.LeasedUntilUtc, TruncateToMilliseconds(leasedUntilUtc)));
+    }
+
+    /// <summary>
+    ///     Records the result of a claimed run and releases its lease, only while the given instance still holds it
+    /// </summary>
+    public virtual async Task CompleteTaskRun(string taskId, string instanceId, bool succeeded, DateTime endUtc,
+        bool disableTask)
+    {
+        var update = UpdateBuilder<ScheduleTask>.Create()
+            .Set(x => x.LeasedUntilUtc, null);
+        if (succeeded)
+            update.Set(x => x.LastSuccessUtc, TruncateToMilliseconds(endUtc))
+                .Set(x => x.LastNonSuccessEndUtc, null);
+        else
+            update.Set(x => x.LastNonSuccessEndUtc, TruncateToMilliseconds(endUtc));
+        if (disableTask)
+            update.Set(x => x.Enabled, false);
+
+        //LeasedByInstance changes only with a new claim, so a match means no other instance has
+        //taken the task over - runs of one task on one instance never overlap
+        await _taskRepository.UpdateOneAsync(x => x.Id == taskId && x.LeasedByInstance == instanceId, update);
+    }
+
+    private static DateTime TruncateToMilliseconds(DateTime value)
+    {
+        return new DateTime(value.Ticks - value.Ticks % TimeSpan.TicksPerMillisecond, DateTimeKind.Utc);
     }
 
     /// <summary>