From f149c27ab03438e5f460304a19103f45467796a2 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Thu, 21 May 2026 00:09:46 +0200 Subject: [PATCH] Address health check review feedback --- doc/wiki/health-checks.md | 14 ++++- src/apps/Elsa.Server.Web/Program.cs | 2 +- .../Extensions/HealthCheckExtensions.cs | 15 ++++- .../ElsaDistributedLockHealthCheck.cs | 10 ++- .../ElsaWorkflowPersistenceHealthCheck.cs | 61 ++++++++++--------- .../ElsaReadinessHealthCheckOptions.cs | 12 ++++ .../ElsaDistributedLockHealthCheckTests.cs | 13 +++- ...ElsaWorkflowPersistenceHealthCheckTests.cs | 17 ++++++ 8 files changed, 103 insertions(+), 41 deletions(-) create mode 100644 src/modules/Elsa.Workflows.Runtime/Options/ElsaReadinessHealthCheckOptions.cs diff --git a/doc/wiki/health-checks.md b/doc/wiki/health-checks.md index e5e1a844e..a36d567a4 100644 --- a/doc/wiki/health-checks.md +++ b/doc/wiki/health-checks.md @@ -7,9 +7,13 @@ Elsa hosts can opt in to Elsa-specific readiness checks on top of the normal ASP Register ASP.NET Core health checks and then add Elsa readiness checks: ```csharp +using System; + services .AddHealthChecks() - .AddElsaReadinessChecks(includeDistributedLocks: true); + .AddElsaReadinessChecks( + includeDistributedLocks: true, + configureOptions: options => options.DistributedLockAcquisitionTimeout = TimeSpan.FromSeconds(1)); ``` Use `includeDistributedLocks: true` when the host enables distributed runtime behavior or relies on distributed locks for workflow coordination. Leave it `false` for simple single-node hosts that do not want a lock probe. @@ -25,6 +29,10 @@ services Map separate endpoints for liveness and readiness: ```csharp +using Elsa.Extensions; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Diagnostics.HealthChecks; + app.MapHealthChecks("/health/live", new() { Predicate = _ => false @@ -32,7 +40,7 @@ app.MapHealthChecks("/health/live", new() app.MapHealthChecks("/health/ready", new() { - Predicate = check => check.Tags.Contains("readiness"), + Predicate = check => check.Tags.Contains(HealthCheckExtensions.ReadinessTag), ResultStatusCodes = { [HealthStatus.Degraded] = StatusCodes.Status503ServiceUnavailable, @@ -72,4 +80,4 @@ readinessProbe: Do not point liveness at Elsa readiness checks. A paused, draining, or temporarily database-degraded runtime should be removed from service by readiness without forcing Kubernetes to restart the process. -When `includeDistributedLocks: true` is enabled, set the readiness probe `timeoutSeconds` higher than the distributed-lock acquisition timeout. The built-in distributed-lock readiness check currently waits up to 1 second to acquire its probe lock, so the example uses `timeoutSeconds: 2` to avoid intermittent Kubernetes probe timeouts while the lock provider is still healthy. +When `includeDistributedLocks: true` is enabled, set the readiness probe `timeoutSeconds` higher than the configured distributed-lock acquisition timeout. The built-in distributed-lock readiness check waits up to 1 second by default to acquire its probe lock, so the example uses `timeoutSeconds: 2` to avoid intermittent Kubernetes probe timeouts while the lock provider is still healthy. diff --git a/src/apps/Elsa.Server.Web/Program.cs b/src/apps/Elsa.Server.Web/Program.cs index 98483f597..3990bf81d 100644 --- a/src/apps/Elsa.Server.Web/Program.cs +++ b/src/apps/Elsa.Server.Web/Program.cs @@ -177,7 +177,7 @@ app.MapHealthChecks("/health/live", new() }); app.MapHealthChecks("/health/ready", new() { - Predicate = check => check.Tags.Contains("readiness"), + Predicate = check => check.Tags.Contains(HealthCheckExtensions.ReadinessTag), ResultStatusCodes = { [HealthStatus.Degraded] = StatusCodes.Status503ServiceUnavailable, diff --git a/src/modules/Elsa.Workflows.Runtime/Extensions/HealthCheckExtensions.cs b/src/modules/Elsa.Workflows.Runtime/Extensions/HealthCheckExtensions.cs index fb3053791..6c9b7195d 100644 --- a/src/modules/Elsa.Workflows.Runtime/Extensions/HealthCheckExtensions.cs +++ b/src/modules/Elsa.Workflows.Runtime/Extensions/HealthCheckExtensions.cs @@ -1,4 +1,5 @@ using Elsa.Workflows.Runtime.HealthChecks; +using Elsa.Workflows.Runtime.Options; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Diagnostics.HealthChecks; @@ -9,7 +10,12 @@ namespace Elsa.Extensions; /// public static class HealthCheckExtensions { - private static readonly string[] ReadinessTags = ["elsa", "readiness"]; + /// + /// Tag applied to Elsa readiness checks. + /// + public const string ReadinessTag = "readiness"; + + private static readonly string[] ReadinessTags = ["elsa", ReadinessTag]; /// /// Adds conservative Elsa-specific readiness probes for the workflow runtime and its core stores. @@ -17,11 +23,16 @@ public static class HealthCheckExtensions /// The health checks builder. /// Whether to probe workflow management/runtime stores. /// Whether to probe the configured distributed lock provider. + /// Configures Elsa readiness probe behavior. public static IHealthChecksBuilder AddElsaReadinessChecks( this IHealthChecksBuilder builder, bool includePersistence = true, - bool includeDistributedLocks = false) + bool includeDistributedLocks = false, + Action? configureOptions = null) { + if (configureOptions != null) + builder.Services.Configure(configureOptions); + builder.AddCheck("elsa-runtime", tags: ReadinessTags); if (includePersistence) diff --git a/src/modules/Elsa.Workflows.Runtime/HealthChecks/ElsaDistributedLockHealthCheck.cs b/src/modules/Elsa.Workflows.Runtime/HealthChecks/ElsaDistributedLockHealthCheck.cs index 80a940763..b794d5675 100644 --- a/src/modules/Elsa.Workflows.Runtime/HealthChecks/ElsaDistributedLockHealthCheck.cs +++ b/src/modules/Elsa.Workflows.Runtime/HealthChecks/ElsaDistributedLockHealthCheck.cs @@ -1,18 +1,22 @@ using Elsa.Common; +using Elsa.Workflows.Runtime.Options; using Medallion.Threading; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Diagnostics.HealthChecks; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; namespace Elsa.Workflows.Runtime.HealthChecks; /// /// Verifies that the configured distributed lock provider can acquire and release a probe lock. /// -public class ElsaDistributedLockHealthCheck(IServiceProvider serviceProvider, ILogger logger) : IHealthCheck +public class ElsaDistributedLockHealthCheck( + IServiceProvider serviceProvider, + IOptions options, + ILogger logger) : IHealthCheck { private static readonly string LockName = $"elsa-health-check-{Environment.MachineName}-{Guid.NewGuid():N}"; - private static readonly TimeSpan LockAcquisitionTimeout = TimeSpan.FromSeconds(1); /// public async Task CheckHealthAsync(HealthCheckContext context, CancellationToken cancellationToken = default) @@ -28,7 +32,7 @@ public class ElsaDistributedLockHealthCheck(IServiceProvider serviceProvider, IL }); } - await using var handle = await distributedLockProvider.TryAcquireLockAsync(LockName, LockAcquisitionTimeout, cancellationToken); + await using var handle = await distributedLockProvider.TryAcquireLockAsync(LockName, options.Value.DistributedLockAcquisitionTimeout, cancellationToken); if (handle == null) { return HealthCheckResult.Degraded("Elsa distributed lock provider was reachable, but the probe lock was not acquired.", data: new Dictionary diff --git a/src/modules/Elsa.Workflows.Runtime/HealthChecks/ElsaWorkflowPersistenceHealthCheck.cs b/src/modules/Elsa.Workflows.Runtime/HealthChecks/ElsaWorkflowPersistenceHealthCheck.cs index aab33bedd..9ee15a50c 100644 --- a/src/modules/Elsa.Workflows.Runtime/HealthChecks/ElsaWorkflowPersistenceHealthCheck.cs +++ b/src/modules/Elsa.Workflows.Runtime/HealthChecks/ElsaWorkflowPersistenceHealthCheck.cs @@ -18,45 +18,46 @@ public class ElsaWorkflowPersistenceHealthCheck(IServiceProvider serviceProvider /// public async Task CheckHealthAsync(HealthCheckContext context, CancellationToken cancellationToken = default) { - var failedStore = ""; - var probes = new List(); - var skippedProbes = new List(); + var probeResults = await Task.WhenAll( + ProbeAsync("workflow-definitions", serviceProvider.GetService(), async (store, ct) => await store.FindAsync(new WorkflowDefinitionFilter { Id = ProbeId }, ct)), + ProbeAsync("workflow-instances", serviceProvider.GetService(), async (store, ct) => await store.CountAsync(new WorkflowInstanceFilter { Id = ProbeId }, ct)), + ProbeAsync("triggers", serviceProvider.GetService(), async (store, ct) => await store.FindAsync(new TriggerFilter { Id = ProbeId }, ct)), + ProbeAsync("bookmark-queue", serviceProvider.GetService(), async (store, ct) => await store.FindAsync(new BookmarkQueueFilter { Id = ProbeId }, ct))); - try + var probes = probeResults.Where(x => !x.Skipped).Select(x => x.StoreName).ToList(); + var skippedProbes = probeResults.Where(x => x.Skipped).Select(x => x.StoreName).ToList(); + var failedProbe = probeResults.FirstOrDefault(x => x.Exception != null); + if (failedProbe != null) { - await ProbeAsync("workflow-definitions", serviceProvider.GetService(), async (store, ct) => await store.FindAsync(new WorkflowDefinitionFilter { Id = ProbeId }, ct)); - await ProbeAsync("workflow-instances", serviceProvider.GetService(), async (store, ct) => await store.CountAsync(new WorkflowInstanceFilter { Id = ProbeId }, ct)); - await ProbeAsync("triggers", serviceProvider.GetService(), async (store, ct) => await store.FindAsync(new TriggerFilter { Id = ProbeId }, ct)); - await ProbeAsync("bookmark-queue", serviceProvider.GetService(), async (store, ct) => await store.FindAsync(new BookmarkQueueFilter { Id = ProbeId }, ct)); - var data = CreateData(); - return probes.Count == 0 - ? HealthCheckResult.Degraded("No Elsa workflow persistence stores are registered.", data: data) - : HealthCheckResult.Healthy("Elsa workflow stores are reachable.", data); - } - catch (Exception e) when (!e.IsFatal()) - { - logger.LogWarning(e, "Elsa workflow store {StoreName} is not reachable.", failedStore); - return HealthCheckResult.Unhealthy($"Elsa workflow store '{failedStore}' is not reachable.", data: new Dictionary - { - ["category"] = "persistence", - ["failedStore"] = failedStore, - ["failedProbe"] = failedStore - }); + data["failedStore"] = failedProbe.StoreName; + data["failedProbe"] = failedProbe.StoreName; + + logger.LogWarning(failedProbe.Exception, "Elsa workflow store {StoreName} is not reachable.", failedProbe.StoreName); + return HealthCheckResult.Unhealthy($"Elsa workflow store '{failedProbe.StoreName}' is not reachable.", data: data); } - async Task ProbeAsync(string storeName, TStore? store, Func probe) where TStore : class - { - failedStore = storeName; + var healthyData = CreateData(); + return probes.Count == 0 + ? HealthCheckResult.Degraded("No Elsa workflow persistence stores are registered.", data: healthyData) + : HealthCheckResult.Healthy("Elsa workflow stores are reachable.", healthyData); + async Task ProbeAsync(string storeName, TStore? store, Func probe) where TStore : class + { if (store == null) { - skippedProbes.Add(storeName); - return; + return new ProbeResult(storeName, true, null); } - probes.Add(storeName); - await probe(store, cancellationToken); + try + { + await probe(store, cancellationToken); + return new ProbeResult(storeName, false, null); + } + catch (Exception e) when (!e.IsFatal()) + { + return new ProbeResult(storeName, false, e); + } } Dictionary CreateData() @@ -75,4 +76,6 @@ public class ElsaWorkflowPersistenceHealthCheck(IServiceProvider serviceProvider return data; } } + + private sealed record ProbeResult(string StoreName, bool Skipped, Exception? Exception); } diff --git a/src/modules/Elsa.Workflows.Runtime/Options/ElsaReadinessHealthCheckOptions.cs b/src/modules/Elsa.Workflows.Runtime/Options/ElsaReadinessHealthCheckOptions.cs new file mode 100644 index 000000000..351531bbe --- /dev/null +++ b/src/modules/Elsa.Workflows.Runtime/Options/ElsaReadinessHealthCheckOptions.cs @@ -0,0 +1,12 @@ +namespace Elsa.Workflows.Runtime.Options; + +/// +/// Configures Elsa workflow runtime readiness health checks. +/// +public class ElsaReadinessHealthCheckOptions +{ + /// + /// The maximum time the distributed-lock readiness probe waits to acquire its probe lock. + /// + public TimeSpan DistributedLockAcquisitionTimeout { get; set; } = TimeSpan.FromSeconds(1); +} diff --git a/test/unit/Elsa.Workflows.Runtime.UnitTests/HealthChecks/ElsaDistributedLockHealthCheckTests.cs b/test/unit/Elsa.Workflows.Runtime.UnitTests/HealthChecks/ElsaDistributedLockHealthCheckTests.cs index 8f744fb8e..a3d35614c 100644 --- a/test/unit/Elsa.Workflows.Runtime.UnitTests/HealthChecks/ElsaDistributedLockHealthCheckTests.cs +++ b/test/unit/Elsa.Workflows.Runtime.UnitTests/HealthChecks/ElsaDistributedLockHealthCheckTests.cs @@ -1,15 +1,17 @@ using Elsa.Workflows.Runtime.HealthChecks; +using Elsa.Workflows.Runtime.Options; using Medallion.Threading; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Diagnostics.HealthChecks; using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; using NSubstitute; namespace Elsa.Workflows.Runtime.UnitTests.HealthChecks; public class ElsaDistributedLockHealthCheckTests { - private static readonly TimeSpan ExpectedLockAcquisitionTimeout = TimeSpan.FromSeconds(1); + private static readonly TimeSpan ExpectedLockAcquisitionTimeout = TimeSpan.FromMilliseconds(250); private readonly IDistributedLockProvider _distributedLockProvider = Substitute.For(); private readonly IDistributedLock _distributedLock = Substitute.For(); private readonly ElsaDistributedLockHealthCheck _sut; @@ -19,7 +21,7 @@ public class ElsaDistributedLockHealthCheckTests _distributedLockProvider.CreateLock(Arg.Any()).Returns(_distributedLock); _distributedLock.TryAcquireAsync(Arg.Any(), Arg.Any()) .Returns(new ValueTask(Substitute.For())); - _sut = new ElsaDistributedLockHealthCheck(CreateServiceProvider(_distributedLockProvider), NullLogger.Instance); + _sut = new ElsaDistributedLockHealthCheck(CreateServiceProvider(_distributedLockProvider), CreateOptions(), NullLogger.Instance); } [Fact] @@ -59,7 +61,7 @@ public class ElsaDistributedLockHealthCheckTests [Fact] public async Task ReturnsDegradedWhenProviderIsNotRegistered() { - var sut = new ElsaDistributedLockHealthCheck(CreateServiceProvider(), NullLogger.Instance); + var sut = new ElsaDistributedLockHealthCheck(CreateServiceProvider(), CreateOptions(), NullLogger.Instance); var result = await sut.CheckHealthAsync(new HealthCheckContext()); @@ -76,4 +78,9 @@ public class ElsaDistributedLockHealthCheckTests return services.BuildServiceProvider(); } + + private static IOptions CreateOptions() => Microsoft.Extensions.Options.Options.Create(new ElsaReadinessHealthCheckOptions + { + DistributedLockAcquisitionTimeout = ExpectedLockAcquisitionTimeout + }); } diff --git a/test/unit/Elsa.Workflows.Runtime.UnitTests/HealthChecks/ElsaWorkflowPersistenceHealthCheckTests.cs b/test/unit/Elsa.Workflows.Runtime.UnitTests/HealthChecks/ElsaWorkflowPersistenceHealthCheckTests.cs index 3834263dd..457668082 100644 --- a/test/unit/Elsa.Workflows.Runtime.UnitTests/HealthChecks/ElsaWorkflowPersistenceHealthCheckTests.cs +++ b/test/unit/Elsa.Workflows.Runtime.UnitTests/HealthChecks/ElsaWorkflowPersistenceHealthCheckTests.cs @@ -66,4 +66,21 @@ public class ElsaWorkflowPersistenceHealthCheckTests Assert.Equal("triggers,bookmark-queue", result.Data["probes"]); Assert.Equal("workflow-definitions,workflow-instances", result.Data["skippedProbes"]); } + + [Fact] + public async Task ReturnsDegradedWithSkippedProbesWhenNoStoresAreRegistered() + { + _serviceProvider.GetService(typeof(IWorkflowDefinitionStore)).Returns((object?)null); + _serviceProvider.GetService(typeof(IWorkflowInstanceStore)).Returns((object?)null); + _serviceProvider.GetService(typeof(ITriggerStore)).Returns((object?)null); + _serviceProvider.GetService(typeof(IBookmarkQueueStore)).Returns((object?)null); + + var result = await _sut.CheckHealthAsync(new HealthCheckContext()); + + Assert.Equal(HealthStatus.Degraded, result.Status); + Assert.Equal("No Elsa workflow persistence stores are registered.", result.Description); + Assert.Equal("persistence", result.Data["category"]); + Assert.Equal("workflow-definitions,workflow-instances,triggers,bookmark-queue", result.Data["skippedProbes"]); + Assert.False(result.Data.ContainsKey("probes")); + } }