[DurableTask.ServiceBus] Fixed executionId being ignored in ServiceBusOrchestrationService.WaitForOrchestrationAsync

[Davide Montanari (davidemontanari)]

Fix executionId being ignored in ServiceBusOrchestrationService.WaitForOrchestrationAsync

Problem

WaitForOrchestrationAsync accepted an executionId parameter but never used it, always querying by instance id only. This relied on perfect instance-store synchronization to detect a new pending execution.

For recurring orchestrations this caused a race: if the status was checked before the new pending execution became readable from storage, the wait returned the previous execution's completed state. The caller treated the orchestration as complete and scheduled the next one, which then never executed because the previous execution had not actually finished.

Fixing that surfaced several adjacent defects in the same method, all addressed here.

Changes

1. Honor executionId

Query explicitly by execution id when one is supplied, so the wait tracks the execution it was asked about rather than whatever row happens to be newest:

OrchestrationState state = pinnedToExecution
    ? await GetOrchestrationStateAsync(instanceId, executionId)
    : (await GetOrchestrationStateAsync(instanceId, false))?.FirstOrDefault();

string.IsNullOrWhiteSpace is used rather than a null check so an empty or whitespace executionId still means "current generation", consistent with LocalOrchestrationService.

2. Stop pinning when the execution continues-as-new

In the Service Bus store, rows are keyed by InstanceId + ExecutionId and are write-once, so a ContinuedAsNew row is a permanent tombstone — the live orchestration has moved to a new execution id. Continuing to poll the pinned id would block until timeout, so on seeing one we un-pin and follow the current generation.

The tombstone still counts as that iteration's status check, so the loop falls through to the normal timeout accounting instead of re-querying immediately. Re-querying would let a TimeSpan.Zero wait perform two lookups and reach the next generation, contradicting the documented single-check behavior.

This also avoids returning the ContinuedAsNew state itself, whose Output is not the orchestration result but the next generation's input — it would deserialize into a plausible but wrong value rather than failing loudly.

3. Reject state from a previous run after un-pinning

Un-pinning reopens the original race, because AzureTableInstanceStore applies its ContinuedAsNew filter before ordering by LastUpdatedTime. While the next generation is not yet readable, the newest surviving row can be a previous run's completed execution, and the terminal-state branch would return its output.

The tombstone's timestamps are therefore retained as a floor, and any older row is discarded:

if (state != null
    && (state.CreatedTime < minimumCreatedTime
        || (state.CreatedTime == minimumCreatedTime && state.LastUpdatedTime < minimumLastUpdatedTime)))
{
    state = null;
}

CreatedTime alone is insufficient: it comes from GetExecutionStartedEventOrThrow().Timestamp, i.e. a DateTime.UtcNow reading, which is not unique and can tie between a previous run and the tombstone at clock granularity. LastUpdatedTime breaks the tie safely — a previous run necessarily stopped being updated no later than the continue-as-new that wrote the tombstone, while the generation following the tombstone is updated at or after it.

The comparison is < rather than <= deliberately: <= would reject the next generation whenever a fast continue-as-new lands it in the same clock tick as the tombstone, hanging the wait until timeout.

4. Treat Suspended and ContinuedAsNew as non-terminal

The status check only special-cased Running and Pending. A suspended orchestration is resumable via ExecutionResumedEvent and has a null Output, so returning it as terminal is incorrect. ContinuedAsNew is likewise never final.

The bundled AzureTableInstanceStore happens to filter ContinuedAsNew rows out of the non-pinned lookup, which is what masked this, but that filtering is an implementation detail and is not required by IOrchestrationServiceInstanceStore. The guard makes the method correct for any conforming store.

5. Timeout handling

Timeout.InfiniteTimeSpan is -1ms, so the previous timeoutSeconds > 0 loop condition returned null immediately instead of waiting forever. Infinite is now detected once up front and the decrement skipped entirely, since subtracting from -1ms also trips the negative check. Negative timeouts now throw ArgumentException instead of silently returning null. StatusPollingIntervalInSeconds became a TimeSpan to remove manual * 1000 conversions.

6. Poll for the full timeout window

The budget was decremented by a whole polling interval before the delay it was paying for, and the loop exited without ever performing that final delay and status check. A 4 second wait polled at t=0 and t=2 and then returned null at t=2, discarding half its window and potentially reporting a timeout while the orchestration completed during the remainder. Any timeout shorter than the 2 second interval returned immediately without waiting at all.

The deadline is now checked before the budget is spent, only time actually spent waiting is charged, and the final delay is clamped to what remains:

TimeSpan delay = StatusPollingInterval;

if (!isInfiniteTimeSpan)
{
    if (timeout <= TimeSpan.Zero)
    {
        break;
    }

    if (timeout < delay)
    {
        delay = timeout;
    }

    timeout -= delay;
}

await Task.Delay(delay, cancellationToken);

A 4 second timeout now polls at roughly t=0, t=2 and t=4; a 1 second timeout waits out its window without overshooting it; and TimeSpan.Zero still performs exactly one status check.

Tests

New Test/DurableTask.ServiceBus.Tests/WaitForOrchestrationTests.cs — 19 tests (23 cases) using an in-memory IOrchestrationServiceInstanceStore that reproduces AzureTableInstanceStore query semantics, including the ContinuedAsNew filter and LastUpdatedTime ordering. Unlike the existing integration tests, these need no live Service Bus or Storage account.

Execution tracking:

• The previous-run race, both pinned and after un-pinning
• A previous run sharing the tombstone's CreatedTime, covering the LastUpdatedTime tie-break
• Timing out rather than returning a stale run's result
• Following the next generation after ContinueAsNew
• A store that does not filter tombstones
• Null / empty / whitespace execution ids preserving the legacy path
• Terminal states (Failed) returned via the pinned lookup

Suspend/resume:

• A resumed orchestration (Suspended → Running → Completed) returns the final state, asserting it polls through both intermediate states
• A never-resumed suspended orchestration times out instead of being reported as finished

Timeout parameter:

• Negative values throw, validated before any store lookup
• TimeSpan.Zero checks exactly once and does not poll, returning an already-terminal state if present
• TimeSpan.Zero against a ContinuedAsNew tombstone still performs exactly one lookup
• Positive values poll for the whole window and observe a completion late in it
• A sub-interval timeout waits out its window without overshooting
• Timeout.InfiniteTimeSpan waits for completion and still honors cancellation

The -2ms negative case is deliberate: it sits immediately adjacent to Timeout.InfiniteTimeSpan (-1ms) and pins the boundary between "rejected as negative" and "treated as infinite".

Test results: 23/23 passing on net8.0 (35.7s). Each guard above was additionally verified by reverting it in the production code and confirming a specific test fails, so the suite is known to be non-vacuous rather than merely green.

Known limitation

When no executionId is supplied and the store filters ContinuedAsNew rows, the previous-run race remains possible: the tombstone that would establish the floor is exactly the row the store hides, so nothing marks the generation boundary. Closing this requires an additional allExecutions: true lookup to see the hidden rows; that was prototyped and set aside to keep this change focused. Callers needing a guarantee should pass an executionId.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

🟡 Changes recommended

Unresolved timeout logic and inactive regression tests must be addressed.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR fixes execution tracking, continuation handling, status polling, and timeout behavior in WaitForOrchestrationAsync.

Changes:

• Honors execution IDs and follows ContinueAsNew generations.
• Handles suspended states and zero, negative, and infinite timeouts.
• Adds in-memory regression tests.

File summaries

File	Summary	Final findings
Test/DurableTask.ServiceBus.Tests/WaitForOrchestrationTests.cs	Adds wait-behavior regression tests.	moderate (3 votes): Tests are outside the active lowercase test/ path and are not compiled. moderate (1 vote): Infinite-timeout cancellation coverage does not prove the wait remained incomplete before cancellation.
src/DurableTask.ServiceBus/ServiceBusOrchestrationService.cs	Updates execution-aware polling and timeout handling.	moderate (3 votes): ContinuedAsNew skips timeout accounting and the zero-timeout exit. moderate (1 vote): Timeout accounting before the delay can return early. nit (1 vote): The validation message omits TimeSpan.Zero.

Review details

Suppressed comments (3)

Test/DurableTask.ServiceBus.Tests/WaitForOrchestrationTests.cs:475

• This test does not verify that cancellation caused the wait to finish: an implementation that incorrectly returns null immediately for Timeout.InfiniteTimeSpan would pass the Assert.IsNull assertion before the three-second token cancellation. Assert that the wait remains incomplete until the token is canceled, then accept either null or OperationCanceledException.

                    OrchestrationState state = await service.WaitForOrchestrationAsync(
                        InstanceId,
                        "generation-1",
                        Timeout.InfiniteTimeSpan,
                        cts.Token);

                    Assert.IsNull(state, "A cancelled wait must not return a state.");
                }
                catch (OperationCanceledException)

src/DurableTask.ServiceBus/ServiceBusOrchestrationService.cs:1299

• The timeout is decremented and tested before the delay, so a non-terminal wait returns one polling interval early: a 4-second timeout exits after about 2 seconds, and any positive timeout up to 2 seconds returns immediately. This can report a timeout while the caller's requested wait window is still available. Cap the delay by the remaining timeout, await it, and decrement after the delay (while retaining the zero-timeout no-delay case).

                        timeout -= StatusPollingInterval;

                        // For a user-provided timeout of `TimeSpan.Zero`,
                        // we want to check the status of the orchestration once and then return.
                        // Therefore, we check the timeout condition after the status check.
                        if (timeout <= TimeSpan.Zero)
                        {
                            break;

src/DurableTask.ServiceBus/ServiceBusOrchestrationService.cs:1250

• This exception message omits TimeSpan.Zero, even though zero is explicitly accepted by the validation and documented as a valid timeout. A caller following the guidance would incorrectly avoid a supported value; mention zero in the list of valid choices.

                    $" Please provide either a positive timeout value or Timeout.InfiniteTimeSpan.");

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Address the timeout polling bug and ensure the new tests are included and validate cancellation.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

Test/DurableTask.ServiceBus.Tests/WaitForOrchestrationTests.cs:502

• This assertion does not prove that cancellation was observed: the previous Timeout.InfiniteTimeSpan bug returned null immediately, which also satisfies Assert.IsNull before the 3-second token fires. Because the OperationCanceledException path is accepted without any assertion, this test would pass for both behaviors; assert that the store was polled beyond the initial lookup (or otherwise verify the wait remained active) in both paths.

                    Assert.IsNull(state, "A cancelled wait must not return a state.");
                }
                catch (OperationCanceledException)
                {
                    // Also acceptable: the polling delay observes the token directly.
                }

Test/DurableTask.ServiceBus.Tests/WaitForOrchestrationTests.cs:14

• This new test file is under Test/, but the solution and active ServiceBus test project are under lowercase test/ (DurableTask.sln:8, test/DurableTask.ServiceBus.Tests/DurableTask.ServiceBus.Tests.csproj:42-47). There is no project under Test/, so SDK compile globs will not include these tests and CI will not execute the cases added here. Move the file into the active test/DurableTask.ServiceBus.Tests/ directory (or explicitly include it in that project).

namespace DurableTask.ServiceBus.Tests

src/DurableTask.ServiceBus/ServiceBusOrchestrationService.cs:1250

• The new exception text omits TimeSpan.Zero, even though this method explicitly accepts zero and performs one status check for it (see the updated XML documentation and branch below). A caller following this error message would unnecessarily reject a valid timeout; mention TimeSpan.Zero or say that non-negative values are accepted.

                    $" Please provide either a positive timeout value or Timeout.InfiniteTimeSpan.");

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The regression tests must be included in the active project and strengthened to verify infinite-timeout polling.

Review details

Suppressed comments (2)

Test/DurableTask.ServiceBus.Tests/WaitForOrchestrationTests.cs:31

• These tests are added under Test/DurableTask.ServiceBus.Tests, but DurableTask.sln:8 and the active project use the lowercase test/DurableTask.ServiceBus.Tests path, and there is no Test/**/*.csproj. As a result, this entire regression suite is excluded from compilation and CI; move the file into the active test/ project so the new behavior is actually covered.

    [TestClass]
    public class WaitForOrchestrationTests

Test/DurableTask.ServiceBus.Tests/WaitForOrchestrationTests.cs:563

• The non-throwing branch only asserts that the result is null, so the previous implementation's Timeout.InfiniteTimeSpan bug (returning null before performing any lookup) would pass this test. Assert that the store was queried (and preferably drive cancellation after a poll) so this test actually verifies that the infinite wait remains active until cancellation.

                    Assert.IsNull(state, "A cancelled wait must not return a state.");

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Move the regression tests into the referenced lowercase test project directory so they are built and run by CI.

Review details

Suppressed comments (1)

Test/DurableTask.ServiceBus.Tests/WaitForOrchestrationTests.cs:14

• This test file is under the uppercase Test/DurableTask.ServiceBus.Tests tree, but DurableTask.sln and the SDK test project use the lowercase test/DurableTask.ServiceBus.Tests path. Because SDK default compile items do not include sibling directories, these regression tests are not built or run by the solution/CI; move the file into the referenced lowercase test project directory.

namespace DurableTask.ServiceBus.Tests

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

A timeout test relies on a scheduler-dependent wall-clock assertion and should be made deterministic.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Timestamp equality can allow an older execution to pass the stale-row filter.

Review details

Suppressed comments (1)

src/DurableTask.ServiceBus/ServiceBusOrchestrationService.cs:1277

• The strict comparison does not reliably exclude an older execution: CreatedTime comes from HistoryEvent.Timestamp, which is assigned from DateTime.UtcNow and is not guaranteed unique. If a previous terminal row shares the ContinueAsNew tombstone's timestamp, it passes this < check and can be returned before the next generation is readable. Use a generation-safe discriminator or another explicit exclusion mechanism instead of relying on timestamp ordering alone.

                if (state?.CreatedTime < minimumCreatedTime)

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved correctness issues remain in generation lookup and timestamp tie handling.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/DurableTask.ServiceBus/ServiceBusOrchestrationService.cs:1286

• The tie-breaker is still not deterministic: both CreatedTime and LastUpdatedTime are clock-derived values, and LastUpdatedTime is populated from DateTime.UtcNow, so two generations can share both timestamps. In that case a previous terminal row can survive this strict < check and be returned by the latest-generation lookup before the new generation is visible. Treat an equal timestamp pair as ambiguous and keep polling, or use a deterministic generation/row discriminator instead of accepting the tied row.

                if (state != null
                    && (state.CreatedTime < minimumCreatedTime
                        || (state.CreatedTime == minimumCreatedTime && state.LastUpdatedTime < minimumLastUpdatedTime)))

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate issues remain in ServiceBusOrchestrationService.cs.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/DurableTask.ServiceBus/ServiceBusOrchestrationService.cs:1310

• LastUpdatedTime is not a guaranteed-unique generation marker: Service Bus states are stamped with DateTime.UtcNow (Utils.BuildOrchestrationState), so the previous terminal row and a newer tombstone can tie. With equal timestamps this strict > check does not establish a floor regardless of query ordering, and the stale terminal row is returned; use a generation-aware tie-breaker (and cover the equal-timestamp case) before accepting it.

                    if (newest != null && newest.LastUpdatedTime > state.LastUpdatedTime)

src/DurableTask.ServiceBus/ServiceBusOrchestrationService.cs:1291

• After unpinning, an older execution whose CreatedTime and LastUpdatedTime both equal the tombstone's values passes this filter because both comparisons are strict. LastUpdatedTime is populated from DateTime.UtcNow (Utils.cs:556), so equality is possible; the next latest lookup can then return that stale terminal state. Keep a generation/ExecutionId-aware floor or otherwise handle this equality case instead of treating it as current.

                if (state != null
                    && (state.CreatedTime < minimumCreatedTime
                        || (state.CreatedTime == minimumCreatedTime && state.LastUpdatedTime < minimumLastUpdatedTime)))

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The unpinned lookup can still return stale prior execution results before the next generation is readable.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/DurableTask.ServiceBus/ServiceBusOrchestrationService.cs:1260

• The null/empty/whitespace execution-id path still calls the store's non-pinned lookup, which excludes ContinuedAsNew rows in the bundled Service Bus store. During the gap after a generation writes its tombstone but before the next generation is readable, that query can therefore return an older completed execution and this method immediately returns it. Query state in a way that includes the tombstone (or otherwise carries a generation floor) before accepting a terminal result for the current-generation API.

                OrchestrationState state = pinnedToExecution
                    ? await GetOrchestrationStateAsync(instanceId, executionId)
                    : (await GetOrchestrationStateAsync(instanceId, false))?.FirstOrDefault();

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

No unresolved blocking issues remain.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite