Adding rewind to the sidecar

[sophiatev]

Summary

Adds rewind to the sidecar implementation so that the .NET SDK can directly invoke rewind against DTS

[Copilot]

🟡 Changes recommended

Unresolved findings affect rewind validation, large-history handling, test synchronization, and the Linux emulator test path.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds DTS rewind support to the sidecar so the .NET SDK can invoke rewind operations.

Changes:

• Implements rewind history rewriting and trace handling.
• Updates worker processing for rewind requests.
• Adds unit tests, emulator integration tests, and CI execution.

File summaries

File	Summary
test/Worker/Grpc.Tests/RewindOrchestrationHandlerTests.cs	Tests rewind history rewriting and tracing.
test/Worker/Grpc.Tests/GrpcDurableTaskWorkerTests.cs	Tests worker rewind processing.
test/Grpc.IntegrationTests/run-dts-emulator-tests.ps1	Starts the emulator and runs integration tests.
test/Grpc.IntegrationTests/Grpc.IntegrationTests.csproj	Adds required project references.
test/Grpc.IntegrationTests/DtsRewindIntegrationTests.cs	Adds end-to-end DTS rewind scenarios.
test/Grpc.IntegrationTests/DtsEmulatorFactAttribute.cs	Controls emulator-dependent test execution.
src/Worker/Grpc/RewindOrchestrationHandler.cs	Builds rewind responses and replacement history.
src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs	Detects and processes rewind work items.
src/Shared/Grpc/ProtoUtils.cs	Centralizes trace-context serialization.
.github/workflows/validate-build.yml	Runs emulator tests in CI.

Review details

Suppressed comments (3)

src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs:678

• This sends the complete replacement history as one RewindOrchestrationAction. The existing completion chunker rejects any single action larger than the configured chunk size when LargePayloads is not advertised (the default only advertises history streaming), and the payload interceptor does not externalize RewindOrchestrationAction. Consequently, rewinding a sufficiently large failed orchestration is converted into a validation failure instead of being rewound. Add protocol-specific splitting/externalization for rewind history, or otherwise handle this size case before exposing rewind as supported.

                await this.CompleteOrchestratorTaskWithChunkingAsync(
                    RewindOrchestrationHandler.CreateResponse(
                        request,
                        pastEvents,

src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs:683

• This early-return path starts the orchestration activity but never stops or disposes it. When tracing is enabled, the activity remains active/current after the rewind response and its span is never closed; wrap completion in try/finally and dispose traceActivity there, including when response creation or completion fails.

            if (isInitialRewind)
            {
                await this.CompleteOrchestratorTaskWithChunkingAsync(
                    RewindOrchestrationHandler.CreateResponse(
                        request,
                        pastEvents,
                        completionToken,
                        traceActivity),
                    this.worker.grpcOptions.CompleteOrchestrationWorkItemChunkSizeInBytes,
                    cancellationToken);
                return;

test/Grpc.IntegrationTests/DtsRewindIntegrationTests.cs:509

• WaitForInstanceCompletionAsync returns immediately when an instance is already terminal. After RewindInstanceAsync only enqueues the request, this helper can return the pre-rewind Failed metadata before DTS processes the rewind; the later assertions (and the second rewind) then race the backend. Wait for the execution ID or status to change from the failed execution before waiting for the new terminal state.

        return await client.WaitForInstanceCompletionAsync(
            instanceId,
            getInputsAndOutputs: true,
            this.TimeoutToken);

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

---

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

[Copilot]

🟡 Changes recommended

Unresolved rewind-history correctness issues and unreliable or skipped integration coverage remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs:658

• The missing-ExecutionStartedEvent guard is nested under ParentTraceContext is not null, so an initial rewind without trace context bypasses it. CreateResponse can then emit replacement history without an ExecutionStarted event, which cannot be replayed by the worker; perform this validation for every initial rewind and only conditionally clone the trace context.

            if (isInitialRewind
                && rewindEvent!.ParentTraceContext is not null)
            {
                if (executionStartedEvent is null)
                {

src/Worker/Grpc/RewindOrchestrationHandler.cs:32

• The validation only checks the second event. A malformed request whose first event is not OrchestratorStarted is accepted and that event is then copied into the replacement history, despite the error contract requiring OrchestratorStarted followed by ExecutionRewound. Validate the first event as well so an invalid rewind cannot produce an invalid history.

        if (request.NewEvents.Count != 2
            || request.NewEvents[1].EventTypeCase != P.HistoryEvent.EventTypeOneofCase.ExecutionRewound)

test/Grpc.IntegrationTests/DtsRewindIntegrationTests.cs:211

• This is the only test in the new suite that exercises rewinding multiple failed sub-orchestrations, but the hard-coded Skip means the DTS CI job never runs it. Regressions in the recursive child-rewrite path can therefore pass CI; make the scenario runnable with the image used by CI or add an equivalent non-skipped test when the required sidecar fix is available.

    [DtsEmulatorFact(Skip = "Requires a DTS emulator image containing the sidecar rewind history fix.")]

test/Grpc.IntegrationTests/DtsRewindIntegrationTests.cs:580

• The fixed two-second delay is not a synchronization guarantee for the recreated child records. Under CI load the emulator may take longer to make these instances queryable, causing this test to fail even though rewind eventually succeeds. Poll each child with TimeoutToken until it exists and reaches the expected terminal state instead of sleeping for a fixed interval.

        await Task.Delay(TimeSpan.FromSeconds(2));

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

[Copilot]

🔵 Needs a closer look

Unresolved rewind cleanup, validation, synchronization, and CI path issues remain.

Review details

Suppressed comments (9)

Previously missed (2) — in code that hasn't changed since the last review.

src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs:684

• This early-return path never disposes traceActivity, unlike the normal path below (which calls Dispose). Every initial rewind can therefore leave its orchestration span open/current, leaking tracing state across repeated work items. Dispose the activity in a using/finally before returning, including when response creation or completion fails.

This issue also appears on line 676 of the same file.
test/Grpc.IntegrationTests/DtsRewindIntegrationTests.cs:276

• The fixed two-second delay is not a visibility guarantee for the recreated child state. Under eventual consistency or CI load, the following GetInstanceAsync calls can still observe missing/old metadata; poll with the test cancellation token until the expected child reaches its updated terminal state instead.

This issue also appears on line 580 of the same file.

src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs:680

• The rewind history is serialized as one RewindOrchestrationAction, but this call routes it through CompleteOrchestratorTaskWithChunkingAsync, which rejects any individual action larger than the configured 1–3.9 MB chunk limit instead of splitting it. A failed orchestration with a larger history will therefore be completed as failed rather than rewound; this needs a protocol-compatible way to split the rewind payload or an explicit supported-size limit.

                    RewindOrchestrationHandler.CreateResponse(
                        request,
                        pastEvents,
                        completionToken,
                        traceActivity),

src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs:655

• The missing-ExecutionStartedEvent guard is nested under the ParentTraceContext check. A failed rewind without a parent trace therefore falls through and sends a replacement history with no execution-start event, instead of rejecting the malformed history; validate the event unconditionally before applying the optional trace-context rewrite.

            if (isInitialRewind
                && rewindEvent!.ParentTraceContext is not null)

src/Worker/Grpc/RewindOrchestrationHandler.cs:46

• This local is computed but never read; the actual trace-context parsing below reads the rewritten ExecutionStarted event instead. Remove the dead declaration so the rewind path does not carry an unused fallback value and compiler warning.

        P.TraceContext? orchestrationParentTraceContext = rewindEvent.ParentTraceContext
            ?? executionStartedEvent?.ExecutionStarted.ParentTraceContext;

test/Grpc.IntegrationTests/DtsRewindIntegrationTests.cs:775

• RewindInstanceAsync only enqueues the operation, while WaitForInstanceCompletionAsync returns immediately when the instance is already terminal. If the sidecar has not processed the rewind yet, this helper returns the previous Failed metadata, so every post-rewind assertion (and the second rewind) races the backend. Poll for an updated state/timestamp before calling the terminal wait.

        return await client.WaitForInstanceCompletionAsync(
            instanceId,
            getInputsAndOutputs: true,
            this.TimeoutToken);

test/Grpc.IntegrationTests/DtsRewindIntegrationTests.cs:454

• A fixed two-second sleep is not a reliable synchronization point for recreated child metadata. The emulator/storage path is eventually consistent, so a slower CI run can still return null on the following GetInstanceAsync; poll until the child reaches Completed using TimeoutToken instead.

        await Task.Delay(TimeSpan.FromSeconds(2));

test/Grpc.IntegrationTests/DtsRewindIntegrationTests.cs:580

• A fixed two-second sleep is not a reliable synchronization point for recreated child metadata. The emulator/storage path is eventually consistent, so a slower CI run can still return null on the following GetInstanceAsync calls; poll each child until it reaches Completed using TimeoutToken instead.

        await Task.Delay(TimeSpan.FromSeconds(2));

test/Grpc.IntegrationTests/run-dts-emulator-tests.ps1:68

• The workflow runs this script with PowerShell on ubuntu-latest, but this raw path uses a Windows backslash. On the Linux runner the backslash can be passed to dotnet as part of the filename, so the project path is not found. Build the argument with Join-Path (or use /) instead.

        "$PSScriptRoot\Grpc.IntegrationTests.csproj" `

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

[Copilot]

🟡 Changes recommended

Unresolved critical rewind-history handling issues and additional runtime, integration-test, and CI concerns block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (10)

Previously missed (2) — in code that hasn't changed since the last review.

test/Grpc.IntegrationTests/DtsRewindIntegrationTests.cs:276

• This fixed two-second delay makes the child assertions depend on emulator scheduling: slower CI can still race, while fast runs always pay the full delay. Poll GetInstanceAsync for each child until the expected terminal state using TimeoutToken, following the existing polling pattern in GrpcDurableTaskClientIntegrationTests.cs:664-683.
  test/Grpc.IntegrationTests/DtsRewindIntegrationTests.cs:580
• A fixed two-second sleep does not guarantee that the purged children have been recreated and are visible through GetInstanceAsync; under CI or emulator load this assertion can race, while fast runs just waste time. Poll for each child with TimeoutToken, following the existing integration-test polling pattern, instead of sleeping for a fixed duration.

src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs:655

• The missing-history guard is nested under ParentTraceContext. A malformed initial rewind without a parent trace therefore reaches CreateResponse and sends replacement history without an ExecutionStarted event, bypassing the runtime-state invariant. Validate executionStartedEvent for every initial rewind, then conditionally clone only the trace context.

            if (isInitialRewind)
            {

src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs:675

• On the initial rewind path this returns before the existing disposal at line 977, so any sampled traceActivity remains undisposed/current and can leak across work items. Complete this call in a try/finally (or use a scoped using) and dispose the activity before returning, including when completion fails.

                orchestrationTraceContext);

            if (isInitialRewind)

src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs:620

• This path explicitly supports rewinds whose committed history is delivered through StreamInstanceHistory, but all rewind tests populate PastEvents inline and leave RequiresHistoryStreaming false. Add a regression test that combines RequiresHistoryStreaming = true with ExecutionRewound and verifies the streamed history is used to build the replacement history.

            if (rewindEvent is not null || request.RequiresHistoryStreaming)
            {
                materializedPastEvents = await this.GetPastEventsAsync(request, cancellationToken);

src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs:676

• The rewind response puts the complete replacement history into one RewindOrchestrationAction. The existing completion helper treats each action as indivisible and, without LargePayloads, converts any action larger than the roughly 3.9 MB limit into a failed completion; response chunking cannot split this action. Large histories can therefore not be rewound, so this needs protocol-compatible chunking/externalization or an explicit supported-size design.

            if (isInitialRewind)
            {

test/Grpc.IntegrationTests/DtsRewindIntegrationTests.cs:775

• RewindInstanceAsync only enqueues the rewind, while WaitForInstanceCompletionAsync returns immediately for any terminal status. Every post-rewind call to this helper can therefore return the old Failed metadata before DTS starts the new execution (and RewindTwiceAsync can enqueue the second rewind too early). Add a state-transition guard that polls GetInstanceAsync until the status leaves Failed before waiting for terminal completion.

        }
        catch
        {
            host.Dispose();

test/Grpc.IntegrationTests/DtsRewindIntegrationTests.cs:454

• A fixed two-second sleep does not guarantee that a purged child has been recreated and is visible through GetInstanceAsync; under CI or emulator load this assertion can race, while fast runs just waste time. Poll for the child metadata with TimeoutToken, following the existing integration-test polling pattern, instead of sleeping for a fixed duration.

            nestedInstanceId,

test/Grpc.IntegrationTests/run-dts-emulator-tests.ps1:68

• The workflow runs this script on ubuntu-latest, but this constructs the project path with a Windows \ separator. PowerShell Core on Linux passes that backslash as part of the filename, so dotnet test cannot find the project and the new CI job fails before running tests. Use Join-Path $PSScriptRoot 'Grpc.IntegrationTests.csproj' (as the repository's cross-platform scripts do).

        "$PSScriptRoot\Grpc.IntegrationTests.csproj" `

test/Grpc.IntegrationTests/run-dts-emulator-tests.ps1:70

• This standalone script invokes dotnet test without an explicit restore or the repository's NuGet configuration. The build and CodeQL workflows restore with nuget.config (which clears default feeds), so a clean runner can fail to restore or resolve a different package set; add an explicit restore with the checked-in config and run the test with --no-restore.

    dotnet test `
        "$PSScriptRoot\Grpc.IntegrationTests.csproj" `
        --configuration $Configuration `
        --filter "Category=DtsEmulator"

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

[Copilot]

🟡 Changes recommended

Large rewind histories can exceed the default transport limit, and trace-context and cross-platform path issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/Worker/Grpc/RewindOrchestrationHandler.cs:116

• The replacement child context copies trace ID, flags, and trace state from the failed execution's parent context, not from the new rewind orchestration activity. If sampling changes for the new activity, failed sub-orchestrations can be propagated with stale sampling/state and diverge from normal child scheduling, which uses the current activity context (src/Shared/Grpc/ProtoUtils.cs:335-338). Build this context from orchestrationActivity.Context when available, falling back to orchestrationParentContext.

                ActivityContext newParentTraceContext = new(
                    orchestrationParentContext.TraceId,
                    ActivitySpanId.CreateRandom(),
                    orchestrationParentContext.TraceFlags,
                    orchestrationParentContext.TraceState);

test/Grpc.IntegrationTests/run-dts-emulator-tests.ps1:68

• The workflow runs this script with pwsh on ubuntu-latest, but this argument builds the project path with a Windows backslash. On Unix PowerShell that can be passed to dotnet as a filename containing \, so the emulator job can fail with a project-not-found error. Use Join-Path for the cross-platform path.

        "$PSScriptRoot\Grpc.IntegrationTests.csproj" `

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

[Copilot]

🔵 Needs a closer look

Two moderate findings remain: increased history memory use and a non-portable emulator test path.

Review details

Suppressed comments (2)

src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs:314

• For every history-streamed orchestration, this now retains the complete protobuf event list and then BuildRuntimeState creates a second complete converted list. The previous streaming path converted each chunk as it arrived, so large histories now require roughly two full history representations and can add avoidable worker memory pressure; keep single-pass conversion for normal execution and materialize protobuf events only when rewind processing needs them (or otherwise release them before conversion).

                List<P.HistoryEvent> pastEvents = new();
                await foreach (P.HistoryChunk chunk in streamResponse.ResponseStream.ReadAllAsync(cancellation))
                {
                    foreach (P.HistoryEvent protoEvent in chunk.Events)
                    {
                        pastEvents.Add(protoEvent);

test/Grpc.IntegrationTests/run-dts-emulator-tests.ps1:68

• The CI job runs this script on ubuntu-latest, where \ is not a filesystem separator. Passing "$PSScriptRoot\Grpc.IntegrationTests.csproj" therefore gives dotnet a path containing a literal backslash and the test project cannot be found; construct the path with Join-Path instead.

        "$PSScriptRoot\Grpc.IntegrationTests.csproj" `

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