Skip to content

fix: add bounded cancellable worker response delivery retries - #364

Open
wangbill (YunchuWang) wants to merge 6 commits into
microsoft:mainfrom
YunchuWang:yunchuwang-worker-response-retries
Open

wangbill (YunchuWang) wants to merge 6 commits into
microsoft:mainfrom
YunchuWang:yunchuwang-worker-response-retries

Conversation

@YunchuWang

@YunchuWang wangbill (YunchuWang) commented Sep 8, 2026 •

Copy link
Copy Markdown
Member

Summary

What changed?

  • Add one shared worker delivery loop for orchestration/activity/entity completion, version-mismatch failure completion, and orchestration abandonment. Retry the same response/token, never user execution.
  • Match .NET's 10 SDK attempts for UNAVAILABLE, UNKNOWN, DEADLINE_EXCEEDED, and INTERNAL: 200 ms exponential backoff, capped at 15 seconds before +0–20% jitter. Permanent/exhausted failures retain existing error logs.
  • Cancel retry waits/retried RPCs on stop while allowing initial completion during the existing graceful-drain window. Retired stubs wait only for their own pending work; original-run cancellation survives reconnect/restart.
  • Make the existing unary helper's metadata wait abort-aware, with cleanup and no late RPC after cancellation.
  • Route version rejection through private _abandonOrchestrationWorkItem(stub, completionToken, signal?), backed by the same delivery loop. An explicit signal governs initial attempts, retries and backoff; omission preserves graceful-initial/retry-stop behavior. This provides the shared primitive for the independently based history PR, not another retry framework.
  • Preserve caller channel options and Azure-managed transport retry configuration, matching the .NET Azure-managed worker. Ten SDK attempts is not a ten-physical-request guarantee.
  • Explicitly run both real-socket and persisted-backend delivery specs in the existing DTS emulator group on Node 22/24. The backend suite remains opt-in locally; CI supplies its emulator connection string.

Why is this change needed?

Azure-managed workers already configure transport retries, normally for UNAVAILABLE. Core completion/abandon sites lack a bounded SDK delivery loop, including after headers commit and for additional .NET transient statuses. This closes that gap without changing persisted history or the backend's at-least-once work-item delivery contract.

Pinned reference: microsoft/durabletask-dotnet@bc2bc12ca5ee3a12a6e633250ade5efe6ae90ef4, processor ExecuteWithRetryAsync, internal options, and GrpcBackoff.cs. Its Azure-managed CreateChannel also retains GrpcRetryPolicyDefaults.DefaultServiceConfig beneath that loop, confirming the layered retry behavior.

Base: upstream main 28730dfaedaecd696468cae2e2365153b230fa56. Final tested head: 4ce281c32f46ca806fcd39acc8f86b2a8b1e7abe. All final-head CI workflows passed. The coordinator's principal source re-review approved this exact head, including the shared-abandonment delta; no remaining source blocker.

Issues / work items

  • Focused worker-response parity change. No client wait feature, history hydration, chunking, or concurrency implementation.

Project checklist

  • Release notes are not required for the next release
    • Otherwise: Notes added to CHANGELOG.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: No public API/wire-format changes; existing channel configuration is preserved.
      • Migration guidance: Account for both SDK and configured transport retry layers as documented in README.

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 CLI.
  • AI-assisted areas/files: Implementation, tests, CI selector wiring, documentation.
  • What you changed after AI output: Restricted channel retention to per-stub work; wired explicit CI coverage; removed a transport-retry override after inspecting .NET layering; fixed pending metadata cancellation; extracted shared abandonment to close a concrete cross-PR coverage gap. Principal source re-review approved the final head; human confirmation below remains separate.

AI verification (required if AI was used):

  • I understand the code and can explain it (human confirmation pending)
  • 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

Testing

Automated tests

  • 404 passed / 19 local suites at final head, using existing Jest: npx --no-install jest --runInBand --runTestsByPath ... --detectOpenHandles. Covers affected worker/versioning/entity/tracing/host-integration/backoff behavior, Azure-managed builder/retry/options/endpoint behavior, and client regressions for the shared unary helper.
  • Includes 21 real-gRPC socket cases, using OS-assigned loopback ports: committed-header transient failures, permanent failures, bounded delivery, shutdown metadata/RPC/backoff cancellation, graceful initial completion, all existing completion/abandon paths, reconnect ownership, and explicit-signal abandonment.
  • Five explicit-signal tests first failed during initial metadata/RPC, backoff and retry metadata/RPC; they now pass. New socket cases prove transient abandonment reuses its payload/token and explicit cancellation stops initial metadata/RPC immediately. Default drain tests remain unchanged.
  • Metadata regressions first failed with pending work stuck at one. They now assert cleanup before metadata resolves, preserved initial drain, and no late RPC.
  • Configured retry-layer proof: 10 SDK calls and 50 actual server calls under a five-attempt INTERNAL channel policy, observing transport attempt metadata and one user activity execution. Only delay duration is shortened; separate deterministic tests verify exact backoff.
  • All final-head CI workflows passed: Test and Build, DTS Emulator E2E, Validate Samples.
  • Final DTS delivery-group jobs on Node 22 and Node 24 each passed 61 tests / 6 suites, zero skipped, explicitly including both delivery specs and real emulator persisted outcomes.
  • npm run build:core, npm run build:azuremanaged, changed TS ESLint/Prettier and git diff --check: passed. Changelog/workflow formatting passed previously and those files are unchanged by the latest delta. README's pre-existing whole-file formatting was not rewritten.

Manual validation (only if runtime/behavior changed)

  • Windows, Node.js v24.14.0; real HTTPS Azure DTS Consumption, westus2, dedicated workerresponse task hub, Azure CLI authentication. No Azure resources created/deleted; no combined-run hub access.
  • Final-head command: npx --no-install jest --runInBand --runTestsByPath test/e2e-azuremanaged/worker-response-delivery.spec.ts --detectOpenHandles, with the dedicated WORKER_DELIVERY_CONNECTION_STRING. 2 passed.

Real Azure baseline — no injection

  • Orchestration: response-delivery-2ba5d9c0-a76e-49e1-905d-1985050d0a41.
  • Entity: @deliverycounter-response-delivery-2ba5d9c0-a76e-49e1-905d-1985050d0a41@counter.
  • Persisted Completed, output 42, entity state 42; activity/entity each executed once.
  • Four completion payloads (orchestrator x2, activity x1, entity x1), one SDK attempt each.

Real Azure persisted outcome with controlled client-side faults

  • Orchestration: response-delivery-1b138691-3b2a-40aa-893e-4b2db2ba5c41.
  • Entity: @deliverycounter-response-delivery-1b138691-3b2a-40aa-893e-4b2db2ba5c41@counter.
  • Persisted Completed, output 42, entity state 42; activity/entity each executed once.
  • Four pre-send INTERNAL injections, one per completion payload; two SDK attempts per payload. Serialized payloads/completion tokens asserted identical across retries.
  • Injection is a test client interceptor before forwarding to real Azure, not an actual Azure outage. SDK invocation counts are not physical network-attempt counts. Actual server-side faults and explicit-signal abandonment are covered separately by the socket suite.

Notes for reviewers

  • Companion history-PR merge resolution: both independently based PRs define private async _abandonOrchestrationWorkItem(stub, completionToken, signal?): Promise<void>. Keep this PR's retry-backed helper body and both history-PR callers. The history-fetch-error caller supplies its immediate-stop signal; version rejection omits it. Do not retain the history PR's standalone direct-callWithMetadata helper body in the combined tree, or that path bypasses response retries. Combined history-error + transient-abandon fault coverage is owned by integration; this standalone PR does not add history fetching.
  • The SDK bound is an attempt bound, not a new per-RPC wall-clock deadline. Existing transport retries may multiply underlying attempts.
  • Initial completion stays graceful by default; explicitly supplied abandonment cancellation covers every phase immediately. Metadata generation itself has no cancellation argument and may continue, but cannot cause a late RPC.
  • Backend redelivery can still rerun user code; exactly-once assertions concern retries of a single response delivery only.
  • Commit-hook shell execution hit unavailable WSL Bash; the identical npx --no-install lint-staged check passed when run directly before committing.

Retry completion and abandon responses without re-executing user code. Preserve graceful initial delivery and original-stub ownership while cancelling retries on stop. Disable only worker transport retries to avoid multiplying delivery budgets.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e0af01a5-0dfa-4e71-a660-c4186e65d7e0
@YunchuWang
wangbill (YunchuWang) marked this pull request as ready for review September 8, 2026 16:29
Copilot AI lite review requested due to automatic review settings September 8, 2026 16:29
@YunchuWang
wangbill (YunchuWang) marked this pull request as draft September 8, 2026 16:30
Include both real-socket and persisted-backend delivery specs in the existing emulator group and provide its local connection string so backend cases cannot silently skip.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e0af01a5-0dfa-4e71-a660-c4186e65d7e0

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.

🟡 Changes recommended

A new unit test mocks abandonTaskOrchestratorWorkItem with the wrong response type, which can mask contract/behavior regressions and should be corrected.

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

Pull request overview

This PR adds a bounded, cancellable SDK-level retry loop for worker response delivery (orchestration/activity/entity completion plus version-mismatch failure/abandon), aligning retry semantics with the pinned .NET worker while preventing gRPC transport retries from multiplying the effective retry budget.

Changes:

  • Introduces a shared worker response-delivery helper with transient-status retry + capped exponential backoff with jitter, and stop-aware cancellation semantics.
  • Forces worker-created gRPC channels to disable transport retries (grpc.enable_retries: 0) so the worker owns the retry budget.
  • Adds unit + real-socket gRPC tests and opt-in Azure-managed E2E coverage; documents delivery/shutdown behavior and updates changelog.
File summaries
File Description
test/e2e-azuremanaged/worker-response-delivery.spec.ts Opt-in Azure-managed E2E verifying persistence + interceptor-injected delivery faults without re-executing user code.
README.md Documents the worker response delivery retry policy and shutdown/cancellation behavior.
packages/durabletask-js/test/worker-startup.spec.ts Updates expectations for worker channel options to include disabled transport retries.
packages/durabletask-js/test/worker-response-delivery.spec.ts Adds unit tests for retry policy, cancellation behavior, and stub-retirement interactions.
packages/durabletask-js/test/worker-response-delivery-grpc.spec.ts Adds real grpc-js socket tests covering post-header failures, stop behavior, and retry budget enforcement.
packages/durabletask-js/src/worker/task-hub-grpc-worker.ts Implements response delivery retry helper, stop-aware signals, and per-stub pending-work tracking for safe stub retirement.
packages/durabletask-js/src/utils/backoff.util.ts Adds “positive” jitter strategy used by worker delivery retries.
CHANGELOG.md Notes the new worker response delivery retry behavior and transport-retry override.
Review details

Suppressed comments (1)

packages/durabletask-js/test/worker-response-delivery.spec.ts:368

  • This mock always replies with CompleteTaskResponse, even when the spied method is abandonTaskOrchestratorWorkItem (which should respond with AbandonOrchestrationTaskResponse). Returning the correct response type keeps the test faithful to the gRPC contract and avoids masking future response-handling logic changes.
            const respond = typeof optionsOrCallback === "function" ? optionsOrCallback : callback!;
            respond(requests.length === 1 ? grpcError(grpc.status.INTERNAL) : null, new pb.CompleteTaskResponse());
            return unaryCall();
  • Files reviewed: 9/9 changed files
  • Comments generated: 1
  • Review effort level: Lite

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

Comment thread packages/durabletask-js/test/worker-response-delivery.spec.ts Outdated
Match the .NET Azure-managed worker's layered retry behavior. Bound SDK delivery attempts without overriding caller channel options, document the combined budget, and prove ten SDK attempts with fifty actual server calls under a five-attempt channel policy.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e0af01a5-0dfa-4e71-a660-c4186e65d7e0
Observe cancellation before generating metadata and throughout the wait, retaining the initial-response drain window. Prevent late metadata from sending an RPC and cover pending-work cleanup with deterministic and real-gRPC shutdown regressions.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: e0af01a5-0dfa-4e71-a660-c4186e65d7e0
@YunchuWang
wangbill (YunchuWang) marked this pull request as ready for review September 8, 2026 16:58
@YunchuWang
wangbill (YunchuWang) marked this pull request as draft September 8, 2026 17:16
Route version rejection through a private abandonment primitive shared with the standalone history PR. Allow an explicit cancellation signal to govern initial delivery, retries, and backoff without changing default graceful completion behavior.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e0af01a5-0dfa-4e71-a660-c4186e65d7e0
@YunchuWang
wangbill (YunchuWang) marked this pull request as ready for review September 8, 2026 17:25
Keep completion callbacks typed to CompleteTaskResponse by default, allow the shared response-path mock to model the abandon response, and instantiate AbandonOrchestrationTaskResponse for abandonment.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e0af01a5-0dfa-4e71-a660-c4186e65d7e0
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