Repository navigation
Isolate inbound payload hydration failures to their deliveries - #829
Open
wangbill (YunchuWang) wants to merge 9 commits into
Open
wangbill (YunchuWang) wants to merge 9 commits into
wangbill (YunchuWang) wants to merge 9 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues block the change to isolate payload hydration failures to individual deliveries.
No open issues 🎉
What changed in this PR
This PR addresses #825 by isolating inbound payload hydration failures to individual deliveries, preserving the Durable Task worker’s shared stream and unrelated in-flight work.
Changes:
- Introduces an internal exception carrying the failed delivery and its completion token.
- Reads work items directly and reuses abandonment logic, tolerating ownership loss while preserving connection-level recovery for other failures.
- Adds consumer and HTTP/2 loopback regression coverage plus an Unreleased changelog entry.
| File | Description |
|---|---|
test/Worker/Grpc.Tests/WorkItemStreamConsumerTests.cs |
Tests delivery recovery, callback ordering, and failure propagation. |
test/Grpc.IntegrationTests/PayloadHydrationFailureTests.cs |
Adds loopback coverage for isolation, redelivery, reconnection, and shutdown. |
src/Worker/Grpc/WorkItemStreamConsumer.cs |
Uses direct stream reads with delivery-local recovery. |
src/Worker/Grpc/Worker.Grpc.csproj |
Grants integration tests access to worker internals. |
src/Worker/Grpc/Logs.cs |
Adds ownership-loss logging. |
src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs |
Handles hydration failures using shared abandonment logic. |
src/Grpc/WorkItemPayloadException.cs |
Defines the internal failed-delivery exception. |
src/Grpc/Grpc.csproj |
Exposes the internal contract to consuming assemblies and tests. |
src/Extensions/AzureBlobPayloads/Interceptors/PayloadInterceptor.cs |
Wraps work-item hydration failures with delivery context. |
CHANGELOG.md |
Documents delivery-local hydration failure handling. |
Review effort: Balanced
Remove redundant consumer tests and exercise the shared failure-isolation matrix with both gRPC and Azure Managed worker configuration. Preserve retryable inbound delivery semantics and document the recovery boundary. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…hy work Replace the standalone test server with focused coverage in existing harnesses. Preserve ordinary background error handling, validate execution metadata before terminal settlement, and release entity locks for inline and streamed history. Recoverable download failures remain delivery-local and retryable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ng healthy work" This reverts commit a0f5320 at the user request. Restore retryable delivery-local abandonment for all incoming hydration failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove the standalone 536-line scripted sidecar and reuse existing worker and integration test harnesses. Leave ordinary background failure handling unchanged, remove unnecessary test friend access and duplicate documentation, and retain delivery-local abandonment for every hydration failure. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use worker shutdown cancellation during delivery recovery and rearm the read watchdog before receiving again. Add gated shutdown and slow-abandon regressions; fix the scoped-payload assertion and prevent test-sidecar concurrency starvation on small CI hosts. Keep all hydration failures abandon-only. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Watch only transport receipt for silent disconnects. Give each active worker payload download its own 120-second cancellation budget while preserving bounded concurrency and drainage. Bound receive-loop abandonment and its existing retries/backoff together to 30 seconds. Keep downloads abandon-only and preserve public unary/history behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
5 open issues
Key=\"$(StrongNamePublicKey)\"is fragile if$(StrongNamePublicKey)can be empty in any… · NewThrowAsync<Exception>), which can mask unrelated… · NewCancellationTokenSourcefor every hydrated message. Sincelifetime… · Newusingis unused in the file. Removing it avoids unnecessary imports and potential analyzer… · New{workItemType}/{completionToken}are lower-cased, which… · New
Review effort: Lite
Move worker-extension infrastructure out of the protobuf assembly and remove its friend access. Preserve protected interceptor overrides and async behavior, simplify call disposal and common-token allocation, and remove timeout test callbacks. Use standard clock scheduling for deterministic deadline, cancellation, and external-subclass regressions; successful transport reads no longer poison later reads with a fired watchdog. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Human assessment is needed because interceptor wrappers can expose healthy payload downloads to watchdog cancellation, while abandon deadlines can be misreported as handshake failures and incorrectly mark channels as poisoned.
No open issues 🎉
5 resolved since last review
ThrowAsync<Exception>), which can mask unrelated…Key=\"$(StrongNamePublicKey)\"is fragile if$(StrongNamePublicKey)can be empty in any…{workItemType}/{completionToken}are lower-cased, which…usingis unused in the file. Removing it avoids unnecessary imports and potential analyzer…CancellationTokenSourcefor every hydrated message. Sincelifetime…
Review effort: Balanced
Use Microsoft.Extensions.TimeProvider.Testing 10.0.0 for the net10 test projects. Its native net10 dependency group does not request the uncached Microsoft.Bcl.TimeProvider 8.0.0 package. Verified both test projects restore from an empty package cache using only the checked-in nuget.config. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Comment on lines
+115
to
+117
| bool hasNext = payloadReader is null | ||
| ? await stream.MoveNext(tokenSource.Token).ConfigureAwait(false) | ||
| : await payloadReader.MoveNextAsync(OnMessageReceived, tokenSource.Token, cancellation).ConfigureAwait(false); |
Contributor
There was a problem hiding this comment.
If a logging interceptor wraps this reader, what happens when receiving a message takes 119 seconds and downloading its payload takes another 2 seconds? Could the 120-second read timeout cancel the download and disconnect the whole stream?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What changed?
GetWorkItemsstream, and never dispatch a partially hydrated item.Worker.Grpc.Internal; remove the added protobuf friend access. Remove the split-reader helpers and newly introduced production test callbacks.Why is this change needed?
Issues / work items
Project checklist
release_notes.mdWorker.Grpc.Internalare explicitly not application APIs; this is not a claim of zero public metadata additions.AI-assisted code disclosure (required)
Was an AI tool used? (select one)
If AI was used:
325f6bbd, and corrected the test package's approved-feed restore in5c1c38eb. Human approval is still required.AI verification (required if AI was used):
Testing
Automated tests
Passed: 283 tests, including exact 119/120-second successful-read boundary cases, download drain/recovery, total abandonment/retry/backoff budgets, shutdown, external subclass dispatch, shared lifetime, and distinct cancellation.
Passed: 397 tests. Scope-denial assertions require the exact delivery wrapper and expected inner storage exception.
Passed: 346; skipped: 10 emulator-only tests. Includes public management/unary/history behavior and call-disposal cancellation.
Passed: 13 cases on each of 10 runs at 1 logical CPU and 10 runs at 2 logical CPUs (260 repeated executions), with the final package version.
Passed: netstandard2.0, net6.0, net8.0, net10.0. An unsigned net10.0 Grpc build with
-p:SignAssembly=false -p:StrongNamePublicKey=also passed and has an empty public-key token.Empty-cache dependency verification, using only the checked-in feed configuration:
Both passed. Test-only
Microsoft.Extensions.TimeProvider.Testing10.0.0 has a native net10 dependency group and does not restoreMicrosoft.Bcl.TimeProvider. No global feeds, credentials, or product dependencies were added. Complete worker/integration suites and the four-target build passed again from this clean package set.git --no-pager diff --checkandgit --no-pager diff --cached --checkpassed before committing.Manual validation (only if runtime/behavior changed)
TimeProvider/FakeTimeProvider, not production test callbacks or long real delays.ConsumeAsync_LateMessage_DoesNotTimeOutHealthyHydrationAsyncis covered by deterministic success/timeout ownership cases and repeated low-CPU runs. Final-head CI tests pass.OrchestrationPatternsshared-fixture tests; its isolatedIsReplayingcase and the complete normal-CPU suite pass. Changed-path tests pass the low-CPU stress runs above.325f6bbdinitially failed restore on uncachedMicrosoft.Bcl.TimeProvider8.0.0;5c1c38ebpassed clean-cache restore and the final GitHub validation workflow. Other analysis checks/human review may still be pending; no merge was performed.Notes for reviewers
NotFoundon abandon remains item-local; other failures/expiry enter connection recovery. Normal completion and background-task failure policy are unchanged.Worker.Grpc.Internalcontracts, not friend access or public types added just for tests.ForTestcallbacks remain. Optional productionTimeProviderscheduling uses the system clock by default on net8+; older targets use native timers. The fake-clock package is test-only.