diff --git a/src/Client/OrchestrationServiceClientShim/ShimDurableTaskClient.cs b/src/Client/OrchestrationServiceClientShim/ShimDurableTaskClient.cs index 49414f68..642061fa 100644 --- a/src/Client/OrchestrationServiceClientShim/ShimDurableTaskClient.cs +++ b/src/Client/OrchestrationServiceClientShim/ShimDurableTaskClient.cs @@ -4,6 +4,7 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Globalization; +using System.Security.Cryptography; using DurableTask.Core; using DurableTask.Core.Exceptions; using DurableTask.Core.History; @@ -27,6 +28,16 @@ namespace Microsoft.DurableTask.Client.OrchestrationServiceClientShim; /// The client options. class ShimDurableTaskClient(string name, ShimDurableTaskClientOptions options) : DurableTaskClient(name) { + // Polling parameters for WaitForInstanceStartAsync. PollingInterval matches the historical fixed + // 1-second polling cadence and is used, unjittered, for every steady-state delay -- so long-run + // polling volume never exceeds the historical rate. To desynchronize concurrent callers (avoiding + // synchronized polling bursts against the backend) without inflating that steady-state volume, a + // randomized *initial phase offset* -- uniformly distributed in [0, PollingInterval) -- is applied + // exactly once, before the first delay of a given WaitForInstanceStartAsync call. This is a one-time + // cost per call (not repeated per iteration), so it does not change the long-run polling rate, and + // it still never exceeds the historical 1-second worst-case detection latency. + static readonly TimeSpan PollingInterval = TimeSpan.FromSeconds(1); + readonly ShimDurableTaskClientOptions options = Check.NotNull(options); ShimDurableEntityClient? entities; @@ -270,6 +281,10 @@ public override async Task WaitForInstanceStartAsync( { Check.NotNullOrEmpty(instanceId); + // A one-time randomized phase offset (see ComputeNextPollingDelay) is applied only to the first + // delay of this call so concurrent waiters desynchronize without increasing steady-state + // polling volume beyond the historical fixed 1-second cadence. + bool isInitialDelay = true; while (true) { OrchestrationMetadata? metadata = await this.GetInstancesAsync( @@ -285,7 +300,14 @@ public override async Task WaitForInstanceStartAsync( return metadata; } - await Task.Delay(TimeSpan.FromSeconds(1), cancellation); + // The first delay is a randomized phase offset (bounded by the historical 1-second + // cadence) that desynchronizes concurrent waiters; every delay after that is the fixed + // historical 1-second interval, unjittered, so steady-state polling volume never exceeds + // the historical rate. Either way, the delay never exceeds 1 second, preserving prompt-start + // observation. + TimeSpan delay = ComputeNextPollingDelay(isInitialDelay); + isInitialDelay = false; + await this.DelayAsync(delay, cancellation); } } @@ -344,6 +366,46 @@ await this.TerminateTaskOrchestrationWithReusableRunningStatusAndWaitAsync( return newInstanceId; } + /// + /// Computes the delay to wait before the next polling attempt. + /// + /// + /// if this is the first delay computed for a given call; for every subsequent delay in + /// that call. + /// + /// + /// When is , a one-time randomized phase + /// offset uniformly distributed in [, ) that + /// desynchronizes concurrent callers. Otherwise, the fixed (1 second), + /// unjittered, so steady-state polling volume never exceeds the historical rate. In both cases the + /// returned delay never exceeds , preserving the historical worst-case + /// detection latency for . + /// + internal static TimeSpan ComputeNextPollingDelay(bool isInitialDelay) + { + if (isInitialDelay) + { + return TimeSpan.FromMilliseconds(PollingInterval.TotalMilliseconds * PollingJitter.NextDouble()); + } + + return PollingInterval; + } + + /// + /// Awaits the delay between polling attempts. + /// + /// + /// This is factored out from a direct call + /// purely as an internal seam: it lets tests deterministically observe (and coordinate around) the + /// moment a polling delay begins -- e.g. to cancel only once the delay is genuinely in progress -- + /// without relying on wall-clock timing assumptions. It does not change production behavior. + /// + /// The delay to await. + /// The cancellation token to honor while awaiting the delay. + /// A task that completes after the delay elapses, or is cancelled via . + internal virtual Task DelayAsync(TimeSpan delay, CancellationToken cancellation) => Task.Delay(delay, cancellation); + [return: NotNullIfNotNull("state")] OrchestrationMetadata? ToMetadata(Core.OrchestrationState? state, bool getInputsAndOutputs) { @@ -449,4 +511,37 @@ async Task TerminateTaskOrchestrationWithReusableRunningStatusAndWaitAsync( } } } + + /// + /// A minimal thread-safe random source used to jitter polling + /// delays across concurrent callers. is not thread-safe, and its + /// parameterless constructor can produce correlated sequences when many instances are created around + /// the same tick -- which is exactly the kind of synchronized behavior this jitter is meant to avoid. + /// A single, securely-seeded instance guarded by a lock avoids both issues. + /// + static class PollingJitter + { + static readonly object SyncRoot = new(); + static readonly Random Shared = CreateSeededRandom(); + + /// + /// Returns a thread-safe random double in the range [0.0, 1.0). + /// + /// A random double in the range [0.0, 1.0). + public static double NextDouble() + { + lock (SyncRoot) + { + return Shared.NextDouble(); + } + } + + static Random CreateSeededRandom() + { + byte[] seedBytes = new byte[sizeof(int)]; + using RandomNumberGenerator rng = RandomNumberGenerator.Create(); + rng.GetBytes(seedBytes); + return new Random(BitConverter.ToInt32(seedBytes, 0)); + } + } } diff --git a/src/Worker/Grpc/WorkItemStreamConsumer.cs b/src/Worker/Grpc/WorkItemStreamConsumer.cs index cce0b23a..9c39ef7b 100644 --- a/src/Worker/Grpc/WorkItemStreamConsumer.cs +++ b/src/Worker/Grpc/WorkItemStreamConsumer.cs @@ -60,13 +60,22 @@ internal static class WorkItemStreamConsumer /// reset retry counters that should only count consecutive transport failures. /// /// Outer worker cancellation token. + /// + /// Test-only observability hook invoked synchronously every time the silent-disconnect timer is + /// (re-)armed -- once before the read loop starts, and once per item, immediately before that + /// item is dispatched. Always in production; lets tests prove the + /// per-item reset actually happens (and in what order relative to dispatch) without depending on + /// real elapsed time. Never invoked when disables + /// detection. + /// /// The classified outcome plus whether any message was observed. public static async Task ConsumeAsync( Func> openStream, TimeSpan silentDisconnectTimeout, Action onItem, Action? onFirstMessage, - CancellationToken cancellation) + CancellationToken cancellation, + Action? onSilentDisconnectTimerArmed = null) { bool silentDisconnectEnabled = silentDisconnectTimeout > TimeSpan.Zero; @@ -79,6 +88,7 @@ void ArmSilentDisconnectTimer() if (silentDisconnectEnabled) { timeoutSource.CancelAfter(effectiveTimeout); + onSilentDisconnectTimerArmed?.Invoke(); } } diff --git a/test/Client/OrchestrationServiceClientShim.Tests/ShimDurableTaskClientTests.cs b/test/Client/OrchestrationServiceClientShim.Tests/ShimDurableTaskClientTests.cs index 967d4bde..ecdf29ae 100644 --- a/test/Client/OrchestrationServiceClientShim.Tests/ShimDurableTaskClientTests.cs +++ b/test/Client/OrchestrationServiceClientShim.Tests/ShimDurableTaskClientTests.cs @@ -300,6 +300,161 @@ public async Task WaitForInstanceStart() Validate(metadata, state2, false); } + [Fact] + public async Task WaitForInstanceStart_MultiplePendingPolls_EventuallyReturnsTerminalMetadata() + { + // arrange + DateTimeOffset start = DateTimeOffset.UtcNow; + OrchestrationInstance instance = new() + { + InstanceId = Guid.NewGuid().ToString(), + ExecutionId = Guid.NewGuid().ToString(), + }; + + Core.OrchestrationState pending1 = CreateState("input", start: start); + pending1.OrchestrationInstance = instance; + pending1.OrchestrationStatus = Core.OrchestrationStatus.Pending; + Core.OrchestrationState pending2 = CreateState("input", start: start); + pending2.OrchestrationInstance = instance; + pending2.OrchestrationStatus = Core.OrchestrationStatus.Pending; + Core.OrchestrationState terminal = CreateState("input", start: start); + terminal.OrchestrationInstance = instance; + + this.orchestrationClient.SetupSequence(m => m.GetOrchestrationStateAsync(instance.InstanceId, false)) + .ReturnsAsync([pending1]) + .ReturnsAsync([pending2]) + .ReturnsAsync([terminal]); + + // act + OrchestrationMetadata metadata = await this.client.WaitForInstanceStartAsync( + instance.InstanceId, false, default); + + // assert -- multiple polling iterations (exercising the polling loop, including its one-time + // initial phase offset followed by fixed-interval delays) still converge on the terminal state + // once observed. + this.orchestrationClient.Verify( + m => m.GetOrchestrationStateAsync(instance.InstanceId, false), Times.Exactly(3)); + Validate(metadata, terminal, false); + } + + [Fact] + public void ComputeNextPollingDelay_InitialDelay_NeverExceedsHistoricalOneSecondCadence() + { + // assert -- across many samples, the randomized initial-phase-offset delay must always be + // non-negative and never exceed the historical 1-second polling cadence, enforcing the max + // detection-latency contract for the very first delay of a WaitForInstanceStartAsync call. + for (int i = 0; i < 1000; i++) + { + TimeSpan delay = ShimDurableTaskClient.ComputeNextPollingDelay(isInitialDelay: true); + delay.Should().BeGreaterThanOrEqualTo(TimeSpan.Zero); + delay.Should().BeLessThan(TimeSpan.FromSeconds(1)); + } + } + + [Fact] + public void ComputeNextPollingDelay_SteadyState_ReturnsFixedHistoricalIntervalWithNoJitter() + { + // assert -- every delay after the initial phase offset must be exactly the fixed historical + // 1-second cadence with no jitter and no growth, so steady-state polling volume never exceeds + // (or falls below) the historical rate. This guards against a regression to either unconstrained + // backoff growth or a per-iteration jitter reduction that would increase polling frequency. + for (int i = 0; i < 100; i++) + { + TimeSpan delay = ShimDurableTaskClient.ComputeNextPollingDelay(isInitialDelay: false); + delay.Should().Be(TimeSpan.FromSeconds(1)); + } + } + + [Fact] + public async Task WaitForInstanceStart_InstanceNotFound_ThrowsImmediatelyWithoutPolling() + { + // arrange + string instanceId = Guid.NewGuid().ToString(); + this.orchestrationClient.Setup(m => m.GetOrchestrationStateAsync(instanceId, false)) + .ReturnsAsync([]); + + // act + Func act = () => this.client.WaitForInstanceStartAsync(instanceId, false, default); + + // assert -- not-found behavior is preserved: no retry/polling delay before throwing. + await act.Should().ThrowExactlyAsync() + .WithMessage($"Orchestration with instanceId '{instanceId}' does not exist"); + this.orchestrationClient.Verify( + m => m.GetOrchestrationStateAsync(instanceId, false), Times.Once); + } + + [Fact] + public async Task WaitForInstanceStart_CancelledDuringPollingDelay_ThrowsTaskCanceledException() + { + // arrange + DateTimeOffset start = DateTimeOffset.UtcNow; + OrchestrationInstance instance = new() + { + InstanceId = Guid.NewGuid().ToString(), + ExecutionId = Guid.NewGuid().ToString(), + }; + + Core.OrchestrationState pending = CreateState("input", start: start); + pending.OrchestrationInstance = instance; + pending.OrchestrationStatus = Core.OrchestrationStatus.Pending; + + this.orchestrationClient.Setup(m => m.GetOrchestrationStateAsync(instance.InstanceId, false)) + .ReturnsAsync([pending]); + + using CancellationTokenSource cts = new(); + DelayObservingShimDurableTaskClient client = new( + "test", new ShimDurableTaskClientOptions { Client = this.orchestrationClient.Object }); + + // act -- deterministically coordinate cancellation with the polling delay itself (no wall-clock + // guess, no real timer): wait until the fake delay seam confirms it has been entered for this + // loop iteration, THEN cancel. The fake delay never completes on its own -- it only completes + // when the *exact* cancellation token passed by production code is cancelled -- so this proves + // that token, and not some other code path (e.g. a later poll's own cancellation check), is what + // ends the wait. + Task waitTask = client.WaitForInstanceStartAsync(instance.InstanceId, false, cts.Token); + await client.DelayEntered.WaitAsync(TimeSpan.FromSeconds(10)); + cts.Cancel(); + + Task completedTask = await Task.WhenAny(waitTask, Task.Delay(TimeSpan.FromSeconds(10))); + + // assert -- the wait must end (via cancellation) promptly, without ever performing a second poll. + // Because the fake delay never completes unless the supplied token is cancelled, a regression that + // passes the wrong (or no) token into the delay would leave the delay -- and thus the wait -- + // pending forever, causing this assertion to time out and fail instead of passing vacuously. + completedTask.Should().Be(waitTask, "the wait should be cancelled during the polling delay, not time out"); + Func act = () => waitTask; + await act.Should().ThrowAsync(); + this.orchestrationClient.Verify( + m => m.GetOrchestrationStateAsync(instance.InstanceId, false), Times.Once); + } + + /// + /// A test double whose override replaces + /// the real polling delay with a fully controlled fake: it signals as soon + /// as it is called, then returns a task that never completes on its own (no real timer) and completes + /// -- via cancellation -- only when the *exact* supplied by the caller + /// is cancelled. This lets tests deterministically coordinate cancellation with the delay without any + /// wall-clock timing, and proves that cancelling the caller's token is what actually interrupts the + /// pending wait, rather than some unrelated code path (such as a subsequent poll's own cancellation + /// check) coincidentally producing the same observable outcome. + /// + sealed class DelayObservingShimDurableTaskClient(string name, ShimDurableTaskClientOptions options) + : ShimDurableTaskClient(name, options) + { + readonly TaskCompletionSource delayEntered = new(TaskCreationOptions.RunContinuationsAsynchronously); + + /// Gets a task that completes once has been called. + public Task DelayEntered => this.delayEntered.Task; + + internal override Task DelayAsync(TimeSpan delay, CancellationToken cancellation) + { + TaskCompletionSource pending = new(TaskCreationOptions.RunContinuationsAsynchronously); + cancellation.Register(() => pending.TrySetCanceled(cancellation)); + this.delayEntered.TrySetResult(); + return pending.Task; + } + } + [Fact] public Task ScheduleNewOrchestrationInstance_IdGenerated_NoInput() => this.RunScheduleNewOrchestrationInstanceAsync("test", null, null); diff --git a/test/Worker/Grpc.Tests/WorkItemStreamConsumerTests.cs b/test/Worker/Grpc.Tests/WorkItemStreamConsumerTests.cs index 2464c0c7..ba9104c1 100644 --- a/test/Worker/Grpc.Tests/WorkItemStreamConsumerTests.cs +++ b/test/Worker/Grpc.Tests/WorkItemStreamConsumerTests.cs @@ -2,7 +2,6 @@ // Licensed under the MIT License. using System.Runtime.CompilerServices; -using System.Threading.Channels; using Grpc.Core; using Microsoft.DurableTask.Worker.Grpc; using P = Microsoft.DurableTask.Protobuf; @@ -139,40 +138,55 @@ public async Task OuterCancellation_WithRpcCancelledFromStream_PropagatesExcepti [Fact] public async Task PerItem_HeartbeatReset_KeepsTimerAlive() { - // Feed one item, wait long enough that the original timer would have expired, then complete. - // Synchronize on the first item actually being processed so the second delay is measured from - // the consumer's timer reset instead of from the test thread's write timing. - Channel channel = Channel.CreateUnbounded(); - TimeSpan timeout = TimeSpan.FromMilliseconds(500); - TaskCompletionSource firstItemProcessed = new(TaskCreationOptions.RunContinuationsAsynchronously); - int itemCount = 0; - - Task consumeTask = WorkItemStreamConsumer.ConsumeAsync( - openStream: ct => channel.Reader.ReadAllAsync(ct), - silentDisconnectTimeout: timeout, - onItem: _ => - { - if (Interlocked.Increment(ref itemCount) == 1) - { - firstItemProcessed.TrySetResult(); - } - }, - onFirstMessage: null, - cancellation: CancellationToken.None); - - await Task.Delay(TimeSpan.FromMilliseconds(150)); - await channel.Writer.WriteAsync(new P.WorkItem { HealthPing = new P.HealthPing() }); - await firstItemProcessed.Task.WaitAsync(TimeSpan.FromSeconds(5)); - - // Without the per-item reset, the original timer would fire before this second item arrives. - await Task.Delay(TimeSpan.FromMilliseconds(400)); - await channel.Writer.WriteAsync(new P.WorkItem { HealthPing = new P.HealthPing() }); - channel.Writer.Complete(); + // Proves the per-item timer reset -- not just a single arm at loop start -- is what keeps the + // stream alive. Earlier versions of this test tried to prove the reset by racing real per-item + // delays (each comfortably under the timeout) against the real silent-disconnect timeout (so + // their sum comfortably exceeded it). That was still flaky under CI scheduling pressure: any + // continuation between the "item processed" signal and the next write could be delayed by the + // thread pool/scheduler, silently inflating an intended-short gap past the timeout even though + // production was correct. + // + // This version removes wall-clock timing from the assertion entirely. ConsumeAsync exposes a + // test-only observability hook that fires every time the silent-disconnect timer is (re-)armed: + // once before the read loop starts, and once per item, immediately before that item is + // dispatched to onItem. By recording the exact interleaving of "armed" and "item" events, the + // test proves the structural guarantee directly -- an arm precedes every item, and the total arm + // count is itemCount + 1 -- instead of inferring it from elapsed real time. A regression that + // only arms the timer once at loop start (and never re-arms it per item) fails this assertion + // deterministically, with no dependency on scheduler timing. + const int itemCount = 5; + List events = new(); + int itemIndex = 0; + + P.WorkItem[] items = new P.WorkItem[itemCount]; + for (int i = 0; i < itemCount; i++) + { + items[i] = new P.WorkItem { HealthPing = new P.HealthPing() }; + } - WorkItemStreamResult result = await consumeTask; + WorkItemStreamResult result = await WorkItemStreamConsumer.ConsumeAsync( + openStream: _ => StreamOf(items), + silentDisconnectTimeout: TimeSpan.FromMilliseconds(500), + onItem: _ => events.Add($"item{itemIndex++}"), + onFirstMessage: null, + cancellation: CancellationToken.None, + onSilentDisconnectTimerArmed: () => events.Add("armed")); result.Outcome.Should().Be(WorkItemStreamOutcome.GracefulDrain); result.FirstMessageObserved.Should().BeTrue(); + + // 1 initial arm (before the loop starts) + 1 re-arm per item. + events.Count(e => e == "armed").Should().Be(itemCount + 1); + + // Every item must be immediately preceded by its own re-arm, and the very first event overall + // is the initial pre-loop arm. + events[0].Should().Be("armed"); + for (int i = 0; i < itemCount; i++) + { + int armedIndex = 1 + (i * 2); + events[armedIndex].Should().Be("armed", "item {0} must be preceded by a timer re-arm", i); + events[armedIndex + 1].Should().Be($"item{i}"); + } } [Fact]