Skip to content

Fix payload retries and entity cleanup after storage failures - #816

Open
wangbill (YunchuWang) wants to merge 6 commits into
mainfrom
yunchuwang-public-sdk-payload-fixes
Open

wangbill (YunchuWang) wants to merge 6 commits into
mainfrom
yunchuwang-public-sdk-payload-fixes

Conversation

@YunchuWang

@YunchuWang wangbill (YunchuWang) commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

Summary

What changed?

  • Keep recognized payload-store tokens unchanged on repeated externalization, including batched completion fields and rewind history.
  • Prepare native orchestration payloads before final worker entity cleanup and chunking. Preserve existing entity release actions and reuse the worker's outstanding-unlock cleanup when a permanent storage failure turns a pending response into a failed completion.
  • Preserve the converter's independent terminal-response invariant; processor cleanup remains an idempotent post-preparation fallback using the same helper.
  • Register preparation through the new C# public infrastructure method InternalOptionsExtensions.SetOrchestratorResponsePreparation in the worker's existing .Internal namespace, instead of granting the production Blob assembly access to worker internals.
  • Add completion-retry and entity-lock regression coverage, and update CHANGELOG.md.

Why is this change needed?

  • Completion retries reuse mutable protobuf responses. When a recognized token exceeds the configured externalization threshold, uploading it again creates a nested reference instead of preserving the original payload.
  • A permanent orchestration payload failure previously cleared all actions, including entity releases. If the orchestration was still waiting on an entity call, terminal cleanup had not run at all. Both cases can leave entities locked after the orchestration fails.

Issues / work items

  • Resolves: N/A; independently reproduced against the public SDK.
  • Related: N/A.

Project checklist

  • Release notes are not required for the next release
    • Otherwise: Notes added to release_notes.md
  • Backport is not required
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • All required tests have been added/updated (unit tests, E2E tests)
  • Breaking change?
    • If yes:
      • Impact: N/A.
      • Migration guidance: N/A.

AI-assisted code disclosure (required)

Was an AI tool used? (select one)

  • No
  • Yes, AI helped write parts of this PR (e.g., GitHub Copilot)
  • Yes, an AI agent generated most of this PR

If AI was used:

  • Tool(s): GitHub Copilot.
  • AI-assisted areas/files: Payload interceptors, worker preparation/cleanup wiring, shared entity conversion, regression tests, and the changelog entry.
  • What you changed after AI output: Iterated on native replay coverage, restored unrelated original file bytes, and removed new helper-visibility/member-order warnings while preserving the existing compiler compatibility types.

AI verification (required if AI was used):

  • I understand the code and can explain it
  • I verified referenced APIs/types exist and are correct
  • I reviewed edge cases/failure paths (timeouts, retries, cancellation, exceptions)
  • I reviewed concurrency/async behavior
  • I checked for unintended breaking or behavior changes

Human attestations and the backport decision are intentionally left for review.


Testing

Automated tests

  • Result: Passed.

Earlier recorded validation

dotnet test .\test\Extensions\AzureBlobPayloads.Tests\AzureBlobPayloads.Tests.csproj --configuration Release --no-restore --filter FullyQualifiedName~OrchestratorPayloadFailureTests --logger "console;verbosity=minimal" --verbosity quiet
# Passed: 12, Failed: 0, Skipped: 0.

dotnet test .\test\Grpc.IntegrationTests\Grpc.IntegrationTests.csproj --configuration Release --no-restore --filter "FullyQualifiedName~AzureBlobPayloadsSideCarInterceptorTests|FullyQualifiedName~LargePayloadTests|FullyQualifiedName~AutochunkTests" --logger "console;verbosity=minimal" --verbosity quiet
# Passed: 64, Failed: 0, Skipped: 0.

dotnet test .\test\Worker\Grpc.Tests\Worker.Grpc.Tests.csproj --configuration Release --no-restore --filter "FullyQualifiedName~PayloadInterceptorTests|FullyQualifiedName~GrpcDurableTaskWorkerTests|FullyQualifiedName~ExecuteWithRetryTests|FullyQualifiedName~GrpcOrchestrationRunnerTests" --logger "console;verbosity=minimal" --verbosity quiet
# Passed: 59, Failed: 0, Skipped: 0.

# Previously recorded baseline build; not rerun for this follow-up.
dotnet build .\src\Extensions\AzureBlobPayloads\AzureBlobPayloads.csproj --configuration Release --no-restore --no-incremental -p:BuildProjectReferences=false --verbosity quiet
# Passed for netstandard2.0, net6.0, net8.0, and net10.0; 0 errors.
# Four existing SA1204 warnings remain, one per target framework.

git --no-pager diff --check origin/main...HEAD
# Passed.

Earlier infrastructure-registration validation

dotnet build .\src\Extensions\AzureBlobPayloads\AzureBlobPayloads.csproj --configuration Release --no-restore --no-incremental --verbosity quiet
# Passed for netstandard2.0, net6.0, net8.0, and net10.0, including project references; 0 errors, 169 warnings.
# No CS0436 duplicate-type warnings or warnings in the new infrastructure method.

dotnet test .\test\Worker\Grpc.Tests\Worker.Grpc.Tests.csproj --configuration Release --no-restore --filter "FullyQualifiedName~GrpcDurableTaskWorkerOptionsInternalTests|FullyQualifiedName~PayloadInterceptorTests|FullyQualifiedName~GrpcDurableTaskWorkerTests|FullyQualifiedName~ExecuteWithRetryTests|FullyQualifiedName~GrpcOrchestrationRunnerTests" --logger "console;verbosity=minimal" --verbosity quiet
# Passed: 72, Failed: 0, Skipped: 0.

dotnet test .\test\Extensions\AzureBlobPayloads.Tests\AzureBlobPayloads.Tests.csproj --configuration Release --no-restore --filter "FullyQualifiedName~UseExternalizedPayloadsTests|FullyQualifiedName~OrchestratorPayloadFailureTests" --logger "console;verbosity=minimal" --verbosity quiet
# Passed: 28, Failed: 0, Skipped: 0; includes all 12 unchanged cap/403/success replay cases.

dotnet test .\test\Grpc.IntegrationTests\Grpc.IntegrationTests.csproj --configuration Release --no-restore --filter "FullyQualifiedName~AzureBlobPayloadsSideCarInterceptorTests|FullyQualifiedName~LargePayloadTests|FullyQualifiedName~AutochunkTests" --logger "console;verbosity=minimal" --verbosity quiet
# Passed: 64, Failed: 0, Skipped: 0.

git --no-pager diff --check origin/main...HEAD
# Passed.

Earlier infrastructure-registration metadata inspection also confirmed local Check/Verify definitions in all four extension DLLs. On netstandard2.0, the local IsExternalInit remains internal and the public BlobPurgeOutcome init setter retains its local required modifier. No dependency restore or installation was needed for that earlier follow-up.

Converter-invariant follow-up

# Only after Client.Grpc.Tests initially reported NETSDK1004:
dotnet restore .\test\Client\Grpc.Tests\Client.Grpc.Tests.csproj --configfile .\nuget.config --verbosity minimal
# Passed.

dotnet test .\test\Client\Grpc.Tests\Client.Grpc.Tests.csproj --configuration Release --no-restore --filter FullyQualifiedName~ProtoUtilsTests --logger "console;verbosity=minimal" --verbosity quiet
# Passed: 31, Failed: 0, Skipped: 0.

dotnet test .\test\Worker\Grpc.Tests\Worker.Grpc.Tests.csproj --configuration Release --no-restore --filter "FullyQualifiedName~GrpcDurableTaskWorkerTests|FullyQualifiedName~GrpcOrchestrationRunnerTests" --logger "console;verbosity=minimal" --verbosity quiet
# Passed: 39, Failed: 0, Skipped: 0.

dotnet test .\test\Extensions\AzureBlobPayloads.Tests\AzureBlobPayloads.Tests.csproj --configuration Release --no-restore --filter FullyQualifiedName~OrchestratorPayloadFailureTests --logger "console;verbosity=minimal" --verbosity quiet
# Passed: 12, Failed: 0, Skipped: 0.

dotnet test .\test\Grpc.IntegrationTests\Grpc.IntegrationTests.csproj --configuration Release --no-restore --filter "FullyQualifiedName~AzureBlobPayloadsSideCarInterceptorTests|FullyQualifiedName~LargePayloadTests|FullyQualifiedName~AutochunkTests" --logger "console;verbosity=minimal" --verbosity quiet
# Passed: 64, Failed: 0, Skipped: 0.

git --no-pager diff --check origin/main...HEAD
# Passed.

The enabled direct-converter regression reproduced a single completion with two missing unlocks before restoration. All 146 focused cases now pass, including enabled/null/disabled direct conversion and all unchanged 12 native cases. This converter-only follow-up did not rerun the full solution or the earlier multi-TFM/metadata checks. Original mixed bytes outside the four restored converter lines are preserved.

Manual validation (only if runtime/behavior changed)

  • Environment (OS, .NET version, components): Windows, .NET SDK 10.0.403 / runtime 10.0.14; local HTTP/2 in-memory sidecar and native worker replay harness.
  • Steps + observed results: The following reproductions are automated; no manual cloud validation was performed.
    1. Fail the first activity or orchestration completion after payload externalization. The retry preserves the same 45-byte ASCII blob:v1: fake token above the 16-byte threshold, does not upload a token, and returns the original output/custom status.
    2. Replay an orchestration that acquired two entity locks, then either exceed the payload cap or fail UploadAsync with RequestFailedException(403), for a returning orchestration or a pending entity call. Inline and streamed histories both preserve the completion token and produce two targeted unlocks before one failed completion, without business actions or chunk fragments. Cap cases perform no upload; 403 cases perform exactly one.
    3. Confirm already-emitted typed and legacy entity releases survive a permanent payload failure, while unrelated actions are discarded.
    4. Complete normally with explicit or synthesized releases, with externalized payloads enabled or disabled. Both entities unlock exactly once before completion, and the original result is preserved.
    5. Resolve default and named worker registrations, invoke the actual registered preparation callback, and verify shared-interceptor identity, payload externalization, forwarded cancellation token, preserved completion token, and isolation from other worker names. Null arguments are rejected and an unconfigured callback remains null.
    6. Call the shared converter directly with a terminal Core action and outstanding lock obligations. Both releases precede completion with the correct action IDs, entity targets, critical-section and parent IDs. A second cleanup call leaves the response identical, while null/disabled state emits no synthetic releases.
  • Evidence (optional): The original retry and cap-failure regressions failed before the relevant fixes. Hosted CI and real Azure Storage/service validation have not run locally.

Notes for reviewers

  • No breaking change: one additive C# public infrastructure configuration extension is introduced in Microsoft.DurableTask.Worker.Grpc.Internal; existing public/protected signatures, protobuf fields, converter defaults, and successful replay behavior are unchanged. The method has the same infrastructure compatibility caveats as neighboring configuration methods, not a new general-purpose public options property.
  • The worker accepts only Func<OrchestratorResponse, CancellationToken, Task>, with no Blob types or reverse dependency. The option remains internal, and the Blob implementation method remains internal. Registration shares the existing interceptor. Native processing remains convert/finalize -> prepare payload -> idempotent fallback cleanup -> chunk/send. The second unary pass still skips known tokens.
  • Converter terminal finalization and processor post-prepare fallback intentionally both call the same EntityConversionState.InsertMissingUnlocks helper. The converter returns a complete terminal response independently; the fallback covers direct terminal responses and pending responses made terminal by payload failure. ResetObligations drains synthesized obligations, explicitly emitted releases remove their own obligations, and permanent-failure conversion preserves existing release actions, preventing duplicate unlocks.
  • The production Blob friend entry and duplicate-helper compile exclusions are removed. Test friends, SharedSection Core, and all local compiler compatibility types are retained; both project files are back to their pre-PR contents. The extension compiles its own Check/Verify, and .NET Standard public-record metadata is preserved.
  • Completion RPCs are unary. Current server streams deliver work items and history with payload-free request messages, so this PR does not add a generic outbound-streaming adapter.
  • Release notes are in the existing CHANGELOG.md; this repository does not currently have the template's release_notes.md.

wangbill (YunchuWang) and others added 2 commits October 16, 2026 14:58
Skip recognized payload tokens before size checks so completion retries remain idempotent. Prepare orchestration payloads before terminal cleanup and chunking, preserve entity release actions, and reuse outstanding unlock obligations.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 78dff439-4d84-4d70-921c-f57421cbc383
Reuse the worker's friend-visible validation helpers without duplicating Check and Verify. Retain assembly-local compiler compatibility types for the public record, order the internal preparation member consistently, and remove changelog whitespace.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 78dff439-4d84-4d70-921c-f57421cbc383
Comment thread src/Extensions/AzureBlobPayloads/AzureBlobPayloads.csproj Outdated
Comment thread src/Worker/Grpc/Worker.Grpc.csproj Outdated
Comment thread src/Worker/Grpc/GrpcDurableTaskWorkerOptions.cs
// the orchestration terminates without leaving its entities locked.
P.OrchestratorAction[] unlocks = response.Actions.Where(action =>
action.SendEntityMessage?.EntityUnlockSent is not null
|| (action.SendEvent is { Name: "release", Instance: { } instance }

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why need this check, and isentityinstance check, seem unnccessary

wangbill (YunchuWang) and others added 2 commits October 26, 2026 19:24
Restore the original fake payload tokens, consolidate terminal unlock synthesis after response preparation, and cover real 403 uploads plus normal entity cleanup.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace production friend access with a narrow infrastructure callback setter and restore the Blob extension's own shared helpers. Preserve preparation and cleanup ordering, and cover the configuration contract and named registration.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Comment on lines +46 to +48
P.OrchestratorAction[] unlocks = response.Actions.Where(action =>
action.SendEntityMessage?.EntityUnlockSent is not null
|| (action.SendEvent is { Name: "release", Instance: { } instance }

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why

Restore converter-side missing entity unlocks with the shared helper while retaining the processor's post-preparation fallback. Cover standalone finalization, null and disabled state, and idempotent repeated cleanup without changing the payload architecture.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@YunchuWang
wangbill (YunchuWang) marked this pull request as ready for review October 27, 2026 17:08
@YunchuWang
wangbill (YunchuWang) requested review from halspang and sophiatev and a balanced review from Copilot October 27, 2026 17:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Replay-sensitive entity cleanup and completion ordering warrant maintainer review, particularly without reported scheduler-backed validation.

No open issues 🎉

What changed in this PR

This PR fixes payload retry handling and entity-lock cleanup in the Durable Task SDK’s gRPC worker and Azure Blob payload extension.

Changes:

  • Preserve recognized payload tokens during repeated externalization.
  • Prepare orchestration payloads before final cleanup and chunking, preserving entity releases when storage failures force completion.
  • Add an infrastructure callback and regression coverage for retries, registration, and cleanup.
File Description
test/​Worker/​Grpc.Tests/​GrpcDurableTaskWorkerOptionsInternalTests.cs Tests callback defaults, registration, and null validation.
test/​Grpc.IntegrationTests/​LargePayloadTests.cs Tests stable payload tokens across completion retries.
test/​Grpc.IntegrationTests/​AzureBlobPayloadsSideCarInterceptorTests.cs Covers repeated externalization and preserved entity releases.
test/​Extensions/​AzureBlobPayloads.Tests/​OrchestratorPayloadFailureTests.cs Adds worker replay tests for storage failures and entity cleanup.
test/​Extensions/​AzureBlobPayloads.Tests/​DependencyInjection/​UseExternalizedPayloadsTests.cs Verifies preparation registration and named-worker isolation.
test/​Client/​Grpc.Tests/​ProtoUtilsTests.cs Tests complete, idempotent converter cleanup.
src/​Worker/​Grpc/​Internal/​InternalOptionsExtensions.cs Adds the response-preparation infrastructure API.
src/​Worker/​Grpc/​GrpcDurableTaskWorkerOptions.cs Stores the optional preparation callback.
src/​Worker/​Grpc/​GrpcDurableTaskWorker.Processor.cs Runs preparation before fallback cleanup and chunking.
src/​Shared/​Grpc/​ProtoUtils.cs Extracts reusable cleanup while preserving converter finalization.
src/​Extensions/​AzureBlobPayloads/​Interceptors/​PayloadInterceptor.cs Skips externalization for recognized tokens.
src/​Extensions/​AzureBlobPayloads/​Interceptors/​AzureBlobPayloadsSideCarInterceptor.cs Preserves entity releases during permanent-failure conversion.
src/​Extensions/​AzureBlobPayloads/​DependencyInjection/​DurableTaskWorkerBuilderExtensions.AzureBlobPayloads.cs Registers preparation using the shared interceptor instance.
CHANGELOG.md Documents retry fixes, cleanup behavior, and the infrastructure API.

Review effort: Balanced

Preserve payload preparation and entity cleanup alongside main's purge contract and entity tags. Keep the two PR notes under Unreleased and retain the current release history.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 28, 2026 22:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Cleanup spans replay obligations, storage-failure response replacement, and scheduler commit ordering, while the regression harness does not verify actual lock release, warranting maintainer validation.

No open issues 🎉

Review effort: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants