Repository navigation
Keep scheduled task runs from overlapping across instances - #914
Merged
Merged
Conversation
A claimed run now holds a lease (LeasedUntilUtc) that the running instance renews, so another instance cannot start the same task while it still runs. Tasks pinned with LeasedByMachineName can also match Application:InstanceName, since container hostnames change on every deployment. The loop sleeps for the exact remaining time instead of whole minutes, and dates read back from LiteDB as local time are compared in UTC.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Comment on lines
+240
to
+243
| catch (Exception exc) | ||
| { | ||
| logger.LogWarning(exc, "Could not renew the lease of task {TaskName}", Name); | ||
| } |
Write only the run result and release the lease, and only while this instance still holds it. The whole-entity write reverted admin edits made during the run and, after a lease that ran out, overwrote the claim of the instance that took the task over, letting a third one start. Keep the four-argument TryClaimTaskRun as an obsolete overload so callers built against 2.4 keep working.
| IScheduleTaskService service = _service; | ||
|
|
||
| #pragma warning disable CS0618 | ||
| var claimed = await service.TryClaimTaskRun(task.Id, task.LastStartUtc, DateTime.UtcNow, "instance-a"); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves: no issue (follow-up to the review of #878)
Type: bugfix
Issue
The scheduler's claim (#878 and before) guarantees that one instance starts each run, but several things still went wrong with more than one instance (pods/containers):
LeasedByMachineNamewas compared withEnvironment.MachineName, which is the container hostname (Docker) or the pod name (Kubernetes). Both change on every deployment, so after a redeploy no instance matched and the pinned task silently stopped running.(int)(nextRunUtc - utcNow).TotalMinutes. In the last minute before a run this is0, so the loop re-read the task from the database without any delay until it was due.LastStartUtc == runStartUtcby ticks, so on a host in another time zone (e.g. UTC+2) the instance never saw itself as the winner and tasks never ran.Decidealso compared local dates withutcNow. Containers usually run in UTC, which is why this did not show up there.Solution
TryClaimTaskRunalso setsLeasedUntilUtc(the field already existed but was never used) and refuses the claim while a previous run's lease is still in the future. While the task executes, the instance renews the lease every minute (5-minute lease) from its own DI scope. The final write releases it (LeasedUntilUtc = null). If the instance dies, another one takes the run once the lease runs out.DecidereturnsRetryuntil the lease expires instead of trying to claim.IScheduleTaskService.RenewTaskLease: extends the lease only while the same instance still holds it, so a late renewal cannot recreate a released lease or take over someone else's.Application:InstanceName: a task pinned with "Leased by machine name" also matches this configured name (case-insensitive, like the machine name now). Several instances can share it, e.g. a dedicatedworkerdeployment, and they then compete for runs like unpinned instances. Added to the fourappsettings.jsonfiles and documented indocs/installation.md.ScheduleDecisioncarries aTimeSpan, so the loop sleeps for the exact remaining time.Decidecompare dates viaToUniversalTime().Breaking changes
IScheduleTaskService.TryClaimTaskRuntakes an additionalDateTime leasedUntilUtcparameter, andRenewTaskLeaseis a new interface member. The only implementation is inGrand.Module.ScheduledTasks. A custom replacement of the service would need to add both.LeasedByMachineNameis now compared case-insensitively.LeasedUntilUtcis null on existing tasks, which means "not leased".Not changed: "Run now" in the admin still executes immediately, without a lease, inside the request.
Testing
dotnet test src/Tests/Grand.Web.Common.Tests: 420/420. NewDecidetests cover the exact delay (including the last-minute case that used to be0), pinning by instance name, case-insensitive machine names, retry while leased, run after the lease expires, and local-time dates. A loop test checks that the claim carries a 5-minute lease and that the final write releases it.dotnet test src/Tests/Grand.Modules.Tests: 356/356. NewScheduleTaskServiceTests(MongoDB) cover claim refused while leased, claim after the lease ran out, renewal of own lease, no takeover of another instance's lease, and no recreation of a released lease. NewScheduleTaskServiceLiteDbTestscover the same filter on LiteDB. The successful-claim test failed on LiteDB before the UTC fix on a UTC+2 machine.dotnet build src/Web/Grand.Web: succeeded.