[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. 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.

4. 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.

5. 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.

6. Preserve the store's retry policy for execution-pinned lookups

AzureTableInstanceStore wraps the instance-id overload's table queries in Utils.ExecuteWithRetries, but GetOrchestrationStateAsync(instanceId, executionId) queried the state and JumpStart tables directly. Change routes polling through the pinned overload, so without this the two paths would have had different failure behavior: a single transient failure escaping the storage SDK's own retries would fault the entire wait.

Both queries in the pinned overload are now wrapped to match:

AzureTableOrchestrationStateEntity result =
    (await Utils.ExecuteWithRetries(() => this.tableClient.QueryOrchestrationStatesAsync(
        new OrchestrationStateQuery().AddInstanceFilter(instanceId, executionId)),
        string.Empty,
        "GetOrchestrationStateAsync-stateEntity",
        MaxRetriesTableStore,
        IntervalBetweenRetriesSecs).ConfigureAwait(false)).FirstOrDefault();

To make this testable, AzureTableClient.QueryOrchestrationStatesAsync and QueryJumpStartOrchestrationsAsync are now virtual, with a new internal AzureTableInstanceStore(AzureTableClient) constructor. This adds no public API surface: AzureTableClient is an internal type, so public members on it are capped at internal accessibility and can only be overridden by the three InternalsVisibleTo test assemblies. It carries no versioning commitment and can be reverted without a breaking change.

Tests

New Test/DurableTask.ServiceBus.Tests/WaitForOrchestrationTests.cs — 17 tests (21 cases) using an in-memory IOrchestrationServiceInstanceStore that reproduces AzureTableInstanceStore query semantics, including the ContinuedAsNew filter and LastUpdatedTime ordering. New AzureTableInstanceStoreRetryTests.cs — 2 tests driving the real AzureTableInstanceStore through a stubbed table client. Unlike the existing integration tests, none of these need a live Service Bus or Storage account.

Execution tracking:

• The previous-run race under a pinned lookup
• 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

Store retries:

• A transient failure on the state table followed by a successful read
• A transient failure on the JumpStart table fallback followed by a successful read

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 both net8.0 (37.7s) and net48 (39s). 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

Once a continue-as-new tombstone forces the wait to un-pin, a previous run's terminal row can still be returned if it becomes visible before the successor does. This matches pre-existing behavior and is not a regression, but it is not fixed here.

Timestamps cannot close this gap. LastUpdatedTime is stamped with DateTime.UtcNow by whichever worker persists each state (Utils.BuildOrchestrationState), and CreatedTime comes from an ExecutionStartedEvent produced on the client for the first generation but on the worker for each continue-as-new. Neither gives a cross-machine ordering guarantee, so under clock skew a previous run can sort after the tombstone while a successor sorts before it. Closing this properly needs a generation or lineage signal independent of wall-clock ordering, and the persisted schema has no such column today — ParentExecutionId is a sub-orchestration pointer and is copied forward unchanged — so it is left as follow-up.

The primary bug is still fixed: callers that pass an executionId are pinned to that execution and can never receive a previous run's state.

[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

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

[Bernd Verst (berndverst)]

Reviewed commit beaa47c. I recommend fixing the two inline P2 findings before merging:

1. The new CreatedTime floor can reject a valid ContinueAsNew successor when the initiating client's clock is ahead of the worker's, causing a false timeout or an indefinite wait.
2. The execution-specific lookup bypasses AzureTableInstanceStore's existing outer retry policy, so transient storage failures that previously recovered now fault the wait.

Validation: the PR's 23 tests pass on both net8.0 and net48. I reproduced both regressions on both targets; the corresponding regression cases pass with the exact pre-PR production implementation on net8.0. The retry reproduction uses the actual AzureTableInstanceStore with an injected table-client failure. Live Service Bus/Storage integration tests were not run.

Separately, the stale-result protection remains incomplete: omitted execution IDs retain the race acknowledged in the PR description, and a previous completed run whose CreatedTime and LastUpdatedTime both equal the tombstone's values still passes the new filter. I reproduced the equal-timestamp case as well; these are remaining limitations rather than additional newly introduced regressions.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Retry coverage is outside the active test project, and one wait test has potentially nondeterministic timing.

Review effort: Lite
Findings: None

[Copilot]

Copilot review overview

🟡 Changes recommended

Two moderate findings remain unresolved regarding timestamp filtering and retry timeout/cancellation behavior.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Equality boundary can return a prior terminal generation · New

[Copilot]

Copilot review overview

🟡 Changes recommended

A critical clock-skew scenario can still return stale execution output, and the documentation does not match the implementation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

• Clock skew can bypass tombstone floor and return stale output · New
• Equality boundary can return a prior terminal generation

[Copilot]

Copilot review overview

🔵 Needs a closer look

A moderate issue remains with stale previous-run results after unpinning on ContinuedAsNew.

Review effort: Lite
Findings: None

Resolved since last review (2)

• Clock skew can bypass tombstone floor and return stale output
• Equality boundary can return a prior terminal generation

[Copilot]

Copilot review overview

🟢 Approval recommended

The requested execution tracking, state handling, timeout behavior, retries, and associated tests are covered with no unresolved blocking issues.

Review effort: Lite
Findings: None