Preserve gRPC channel recreation when externalized payloads are enabled

[wangbill (YunchuWang)]

Summary

Enabling UseExternalizedPayloads (the AzureBlobPayloads large-payload extension) silently disabled gRPC channel recreation on both the worker and the client. This is a shipped bug on main, introduced by #468 (ae1433aa).

Revision note. This revision responds to halspang's review. The previous revision routed the fix through an internal Func<CallInvoker, CallInvoker> CallInvokerDecorator hook exposed via public extension methods in a .Internal namespace carrying "do not use" remarks. That has been removed. The hook is now a first-class, supported public Interceptors collection modeled on Grpc.Net.ClientFactory's IHttpClientBuilder.AddInterceptor(). The functional fix is unchanged.

Revision note 2. Also responding to halspang: he asked whether Interceptors being mutable meant a post-initialization change could take effect only at the next recreate. It did, and that was a real bug — the worker re-read the live collection on both recreate paths, so a late Add() was inert until an externally-triggered recreate silently activated it. The worker now snapshots the collection once in its constructor. See Read-once contract below.

User-visible impact

Channel recreation is on by default (ChannelRecreateFailureThreshold = 5 in both GrpcDurableTaskWorkerOptions and GrpcDurableTaskClientOptions). Its whole purpose, per SetChannelRecreator's own doc comment, is to recover from "repeated connect failures (e.g., because the backend was replaced and the existing channel is wedged on a half-open HTTP/2 connection)".

With externalized payloads enabled, that recovery was gone. On backend scale-out, upgrade, or node replacement, the worker wedges on a half-open HTTP/2 connection, stops processing work, and never recovers until the process is manually restarted.

Root cause

IConfigureOptions runs beforeIPostConfigureOptions.

1. DTS's ConfigureGrpcChannel (an IConfigureOptions) sets both options.Channel (DurableTaskSchedulerWorkerExtensions.cs:176) and options.SetChannelRecreator(...) (:185).
2. The AzureBlobPayloads PostConfigure then ran, moved the channel onto an intercepted CallInvoker, and nulled options.Channel.

It had to null Channel, because the core contract documents that "Channel ... will supersede CallInvoker" (GrpcDurableTaskClientOptions.cs:17) — leaving Channel set would make core build a raw invoker and bypass the interceptor entirely.

The consequence was that every recreation path died:

Path	Guard	Why it died
Worker path 1 (recreator)	recreator is not null && currentChannel is not null	latestObservedChannel is seeded from grpcOptions.Channel, which was now null
Worker path 2 (owned rebuild)	Channel is null && CallInvoker is null	CallInvoker was now set
Worker path 3	—	"retry forever on the same wedged transport" — the only survivor
Client	CallInvoker branch	"Externally supplied invoker — we do not own the underlying channel and cannot recreate it"

Probe output against the shipped code (DTS-shaped worker, IConfigureOptions setting Channel + recreator, then optional UseExternalizedPayloads):

A. WITHOUT externalized payloads: Channel=set CallInvoker=null path1 REACHABLE -> self-heal AVAILABLE
B. WITH externalized payloads: Channel=null CallInvoker=set path1 DEAD, path2 DEAD -> self-heal LOST

Why the simpler alternatives don't work

1. Just don't null the Channel. Core then takes the Channel branch and builds a raw invoker, so the interceptor is dropped at startup and large payloads break outright.
2. Flip core precedence to prefer CallInvoker. Directly contradicts the documented public contract at GrpcDurableTaskClientOptions.cs:17 and changes behavior for every consumer, not just this extension.
3. Have the extension wrap the recreator. Impossible from outside core: after recreating, the worker built the new invoker with a bare newChannel.CreateCallInvoker() with no extension hook, so the interceptor would be lost on every recreate anyway.

The fix: a public Interceptors collection

Stop mutating Channel/CallInvoker from the extension. Instead the extension registers an interceptor, and core applies the configured interceptors at every point a CallInvoker is produced — including after a channel recreate. Channel stays set (so recreation works) and the interceptor is always applied (so large payloads work).

This is the same shape as Grpc.Net.ClientFactory's IHttpClientBuilder.AddInterceptor(), so it should be immediately familiar to anyone who has configured a gRPC client in ASP.NET Core.

// GrpcDurableTaskClientOptions and GrpcDurableTaskWorkerOptionspublicIList<Grpc.Core.Interceptors.Interceptor>Interceptors{get;}=newList<Interceptor>();

• GrpcDurableTaskClientOptions / GrpcDurableTaskWorkerOptions: new public Interceptors property, documented as the supported way to attach cross-cutting gRPC behavior (auth headers, tracing, logging, payload externalization) — explicitly preferred over supplying a pre-built, already-intercepted CallInvoker, which opts you out of channel recreation.
• GrpcDurableTaskClient.GetCallInvoker → thin wrapper over GetCallInvokerCore that applies the interceptors once. They are applied outsideChannelRecreatingCallInvoker, so the wrapper's internal channel swaps stay transparent to them.
• GrpcDurableTaskWorker.GetCallInvoker → same wrapper/core split.
• GrpcDurableTaskWorker.TryRecreateChannelAsync applies the interceptors to the new invoker at both recreate sites (recreator-owned and worker-owned). This is the crux — without it interceptors would be silently lost on every recreate, which would be worse than the original bug.
• Both AzureBlobPayloads DI extensions: the whole if (Channel) / else if (CallInvoker) / else throw block is replaced with a single opt.Interceptors.Add(new AzureBlobPayloadsSideCarInterceptor(store, opts)). Channel is no longer nulled and CallInvoker is no longer mutated. The worker still adds P.WorkerCapability.LargePayloads.

Ordering is list order: the first interceptor added is the outermost, so it sees each outgoing call first and each response last. This is documented on the property and pinned by a test.

Design invariant: purely additive to core. While Interceptors is empty, the produced invoker is reference-identical to the one the configured transport produces, so behavior is byte-for-byte identical to today. Asserted on both the worker and the client.

Read-once contract

Interceptors is captured once, when the worker or client is constructed, and the resulting chain is fixed for that instance's lifetime — including across channel recreation. It must therefore be populated while options are being configured (Configure / PostConfigure); mutating it later has no effect. This is documented on the property on both options classes.

The client already behaved this way: it builds its invoker once in its constructor, and ChannelRecreatingCallInvoker swaps channels inside the interceptor wrapper, so the collection is never re-read.

The worker was the outlier. It read grpcOptions.Interceptors at three separate points — once in GetCallInvoker at startup, and again on each of the two recreate paths in TryRecreateChannelAsync. Because Intercept() builds a frozen InterceptingCallInvoker graph that captures interceptor references rather than retaining the list, an Add() performed after construction did nothing at the time, then silently took effect at the next recreate. That is reachable rather than theoretical: the worker resolves its options through IOptionsMonitor.Get(name), and options instances are cached per name, so any other holder of the same monitor gets the same instance and can mutate the list the worker re-reads. Since recreate is triggered externally (backend replacement, node restart, consecutive failures), the delay between the mutation and its activation was nondeterministic.

The worker now snapshots into a readonly Interceptor[] in its constructor and uses that at all three sites, matching the client and matching the existing "do not re-read this.grpcOptions.Channel inside the loop" invariant a few lines below. As a side effect this removes a List<T> thread-safety hazard, since recreate runs on the worker's background loop and could previously race a concurrent Add().

Public API surface

The net public API surface is smaller than the previous revision — one property per options class, instead of four extension methods:

+ Microsoft.DurableTask.Client.Grpc.GrpcDurableTaskClientOptions.Interceptors { get; }+ Microsoft.DurableTask.Worker.Grpc.GrpcDurableTaskWorkerOptions.Interceptors { get; }

No .Internal namespace, no "not subject to the same compatibility standards" remarks, and no InternalsVisibleTo. SetChannelRecreator is untouched. No proto change.

Bonus bug fixed

Deleting the else { throw } fixes a second, independent defect: today UseGrpc("http://localhost:4001") — the Address-only form, the most common raw-gRPC worker setup — throws ArgumentException and is completely unusable with externalized payloads. Only UseGrpc(o => o.CallInvoker = ...) worked. Now core creates its channel from Address and the registered interceptor still applies. One change, two bugs.

Tests

New coverage (all four suites green):

• Extension regression tests (ExternalizedPayloadsInterceptorTests, 8 tests) — worker and client Channel is no longer nulled; Address-only no longer throws; the external-CallInvoker path is no longer mutated; LargePayloads capability is still announced; and two end-to-end interception tests that drive a real CreateInstanceRequest through the product's own private invoker-building path and assert the payload was actually uploaded to the store and replaced with a token. (There was no pre-existing interception test.)
• Worker core (GrpcDurableTaskWorkerInterceptorsTests, 13 tests) — interceptors applied on the Channel, Address, and external-CallInvoker paths; applied to the invoker produced after a channel recreate on both recreate paths (reached via reflection on TryRecreateChannelAsync); reference-identity when the list is empty (initial build and after recreate); ordering; and three tests pinning the read-once contract — an interceptor added after construction never runs, on the recreator path, the worker-owned rebuild path, and the startup invoker.
• Client core (GrpcDurableTaskClientInterceptorsTests, 8 tests) — interceptors applied on the Channel, Address, and external-CallInvoker paths; applied outsideChannelRecreatingCallInvoker; reference-identity when the list is empty (with and without a recreator); and ordering.
• Options contract — Interceptors defaults to an empty, non-null list on both options classes.

Tests assert through the real invoker-building path rather than calling a helper directly, so they cannot pass by re-implementing how interceptors are applied.

Regression evidence

The four extension regression assertions were re-run against the base commit (ab4be125) with the Interceptors assertions stripped so they compile there. All four fail before the fix:

Failed! - Failed: 4, Passed: 0, Total: 4
Worker_WithChannel_... Expected options.Channel to refer to Grpc.Net.Client.GrpcChannel ... but found <null>
Client_WithChannel_... Expected options.Channel to refer to Grpc.Net.Client.GrpcChannel ... but found <null>
Worker_WithAddressOnly_DoesNotThrow System.ArgumentException: Channel or CallInvoker must be provided ...
at DurableTaskWorkerBuilderExtensions.AzureBlobPayloads.cs:line 78
Client_WithAddressOnly_DoesNotThrow System.ArgumentException: Channel or CallInvoker must be provided ...
at DurableTaskClientBuilderExtensions.AzureBlobPayloads.cs:line 54

All pass after the fix.

The three read-once tests were likewise confirmed to fail before the snapshot change and pass after:

Failed! - Failed: 3, Passed: 0, Total: 3
Interceptors_AddedAfterWorkerConstruction_NeverTakeEffect_EvenAfterChannelRecreate
Expected log to be equal to {"at-construction"}, but {"added-late"} differs at index 0.
Interceptors_AddedAfterWorkerConstruction_NeverTakeEffect_OnWorkerOwnedRecreate
Expected log to be equal to {"at-construction"}, but {"added-late"} differs at index 0.
Interceptors_AddedAfterWorkerConstruction_DoNotAffectStartupInvoker
Expected log to be equal to {"at-construction"}, but {"added-late", "at-construction"} contains 1 item(s) too many.

Verification

dotnet build Microsoft.DurableTask.sln --no-incremental
base ab4be125 : 0 Error(s), 340 Warning(s)
this branch : 0 Error(s), 340 Warning(s)
normalized warning-set Compare-Object: 93 vs 93 -> IDENTICAL (0 new, 0 removed)
AzureBlobPayloads.Tests Passed! Failed: 0, Passed: 15
Client.Tests Passed! Failed: 0, Passed: 43
Client.Grpc.Tests Passed! Failed: 0, Passed: 59
Worker.Grpc.Tests Passed! Failed: 0, Passed: 165

Grpc.IntegrationTests is part of the solution and builds clean.

Notes

• Independent of Add large-payload blob auto-purge (opt-in singleton job, worker/SDK side) #758 — branched directly off main, does not touch that branch.
• Adds public API (Interceptors on the two gRPC options classes). That is intentional and is the point of this revision. No proto change, no behavior change when the collection is empty.

[Copilot]

Pull request overview

This PR fixes a shipped regression where enabling the AzureBlobPayloads externalized-payloads extension inadvertently disabled gRPC channel recreation for both the worker and client, preventing self-healing from wedged/half-open HTTP/2 connections. It does so by introducing an internal CallInvokerDecorator hook that lets extensions attach interceptors without mutating Channel/CallInvoker, and ensures the decorator is applied consistently (including after worker channel recreation).

Changes:

• Added internal SetCallInvokerDecorator / ApplyCallInvokerDecorator hooks for both worker and client gRPC options.
• Updated worker and client invoker construction paths to apply the decorator in the correct place (client: outside ChannelRecreatingCallInvoker; worker: on initial invoker and on recreated invokers).
• Updated AzureBlobPayloads DI extensions to register a decorator (instead of nulling Channel) and added comprehensive regression and core tests.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
test/Worker/Grpc.Tests/GrpcDurableTaskWorkerOptionsInternalTests.cs	Verifies safe defaults and contract behavior for the new worker CallInvokerDecorator option.
test/Worker/Grpc.Tests/GrpcDurableTaskWorkerCallInvokerDecoratorTests.cs	Ensures worker applies the decorator for initial invokers and both channel recreation paths.
test/Client/Grpc.Tests/GrpcDurableTaskClientCallInvokerDecoratorTests.cs	Ensures client applies the decorator across Channel/Address/external-invoker paths and outside ChannelRecreatingCallInvoker.
test/Extensions/AzureBlobPayloads.Tests/ExternalizedPayloadsCallInvokerDecoratorTests.cs	Regression tests proving externalized payloads no longer null Channel, no longer break Address-only setup, and still intercepts calls end-to-end.
test/Extensions/AzureBlobPayloads.Tests/AzureBlobPayloads.Tests.csproj	Adds DI package reference needed by new test coverage.
src/Worker/Grpc/GrpcDurableTaskWorkerOptions.cs	Introduces internal CallInvokerDecorator option storage for the worker.
src/Worker/Grpc/Internal/InternalOptionsExtensions.cs	Adds worker SetCallInvokerDecorator and ApplyCallInvokerDecorator internal APIs.
src/Worker/Grpc/GrpcDurableTaskWorker.cs	Applies the decorator when building invokers and when recreating channels.
src/Client/Grpc/GrpcDurableTaskClientOptions.cs	Introduces internal CallInvokerDecorator option storage for the client.
src/Client/Grpc/Internal/InternalOptionsExtensions.cs	Adds client SetCallInvokerDecorator and ApplyCallInvokerDecorator internal APIs.
src/Client/Grpc/GrpcDurableTaskClient.cs	Applies the decorator outside core invoker creation to preserve recreation semantics.
src/Extensions/AzureBlobPayloads/DependencyInjection/DurableTaskWorkerBuilderExtensions.AzureBlobPayloads.cs	Switches from mutating Channel/CallInvoker to registering a decorator and preserves LargePayloads capability.
src/Extensions/AzureBlobPayloads/DependencyInjection/DurableTaskClientBuilderExtensions.AzureBlobPayloads.cs	Switches client externalized payloads to decorator-based interception without breaking channel recreation.

---

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

[halspang]

Overall it seems fine, but I'd like to find a different way to handle the internal nature of the options if we can.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (4)

test/Extensions/AzureBlobPayloads.Tests/ExternalizedPayloadsInterceptorTests.cs:120

• This test creates a GrpcChannel solely to obtain a CallInvoker, but the channel is never disposed. Even though this is a unit test, leaking channels can keep timers/sockets alive and cause test flakiness when the suite grows.

 // Arrange
CallInvoker external = GrpcChannel.ForAddress("http://localhost:4001").CreateCallInvoker();
ServiceCollection services = new();
services.AddSingleton<PayloadStore>(new FakePayloadStore());
DefaultDurableTaskWorkerBuilder builder = new(null, services);
builder.UseGrpc(opt => opt.CallInvoker = external);

test/Extensions/AzureBlobPayloads.Tests/ExternalizedPayloadsInterceptorTests.cs:68

• This test creates a GrpcChannel and leaves it undisposed. Disposing the channel at the end of the test helps avoid leaking timers/sockets across the test run.

 // Arrange
GrpcChannel channel = GrpcChannel.ForAddress("http://localhost:4001");
ServiceCollection services = new();
services.AddSingleton<PayloadStore>(new FakePayloadStore());
DefaultDurableTaskClientBuilder builder = new(null, services);
builder.UseGrpc(channel);

src/Client/Grpc/GrpcDurableTaskClientOptions.cs:46

• The PR description states the fix is implemented as an internal CallInvokerDecorator hook and that there is "No public API change". However, this change adds a new public Interceptors property on GrpcDurableTaskClientOptions, which is a public API surface addition (even if backward-compatible). Please either update the PR description to reflect the actual public API change, or adjust the implementation to match the described internal-only design.

 /// <summary>
/// Gets the gRPC interceptors applied to every <see cref="CallInvoker"/> the client builds from its
/// configured transport, including invokers rebuilt after the underlying channel is recreated.
/// </summary>
/// <remarks>
/// <para>
/// This is the supported way to attach cross-cutting gRPC behavior — authentication headers, tracing,
/// logging, payload externalization — to the Durable Task gRPC client. Prefer it over supplying a
/// pre-built, already-intercepted <see cref="CallInvoker"/> in place of <see cref="Channel"/>: an
/// externally-supplied invoker opts the client out of gRPC channel recreation, so a wedged connection
/// can never be replaced.
/// </para>
/// <para>
/// Interceptors run in list order — the first interceptor added is the outermost, so it observes each
/// outgoing call first and each response last. Registration is purely additive: while this collection
/// is empty, the client uses exactly the invoker its configured transport produces.
/// </para>
/// </remarks>
public IList<Interceptor> Interceptors { get; } = new List<Interceptor>();

test/Extensions/AzureBlobPayloads.Tests/ExternalizedPayloadsInterceptorTests.cs:48

• This test creates a GrpcChannel and leaves it undisposed. Disposing the channel at the end of the test helps avoid leaking timers/sockets across the test run.

This issue also appears in the following locations of the same file:

• line 62
• line 115

 // Arrange
GrpcChannel channel = GrpcChannel.ForAddress("http://localhost:4001");
ServiceCollection services = new();
services.AddSingleton<PayloadStore>(new FakePayloadStore());
DefaultDurableTaskWorkerBuilder builder = new(null, services);
builder.UseGrpc(channel);

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (3)

test/Extensions/AzureBlobPayloads.Tests/ExternalizedPayloadsInterceptorTests.cs:63

• GrpcChannel implements IDisposable; this test creates a channel but never disposes it, which can leak sockets/resources across the test run.

 GrpcChannel channel = GrpcChannel.ForAddress("http://localhost:4001");

test/Extensions/AzureBlobPayloads.Tests/ExternalizedPayloadsInterceptorTests.cs:116

• This creates a GrpcChannel just to obtain a CallInvoker, but the channel is never disposed. Hold on to the channel and dispose it at the end of the test to avoid leaking resources.

 CallInvoker external = GrpcChannel.ForAddress("http://localhost:4001").CreateCallInvoker();

test/Extensions/AzureBlobPayloads.Tests/ExternalizedPayloadsInterceptorTests.cs:44

• GrpcChannel implements IDisposable; this test creates a channel but never disposes it, which can leak sockets/resources across the test run.

This issue also appears in the following locations of the same file:

• line 63
• line 116

 GrpcChannel channel = GrpcChannel.ForAddress("http://localhost:4001");

[halspang]

Since this remains mutable, could we technically change the value after initialization and then have that value take effect only after a recreate?

[wangbill (YunchuWang)]

Yes — you could, and that was a real bug. Fixed in a295ed5.

The worker read grpcOptions.Interceptors live in three places: once in GetCallInvoker at startup, and again on each of the two recreate paths in TryRecreateChannelAsync. Intercept() builds a frozen InterceptingCallInvoker graph that captures the interceptor references rather than holding onto the list, so an Add() after construction did nothing at the time it happened, then silently activated at the next channel recreate — which is triggered by backend replacement / node restart / consecutive failures, so the delay was unbounded and externally driven.

It was reachable, not just theoretical: the worker resolves options via IOptionsMonitor.Get(name) and those instances are cached per name, so anything else holding the same monitor gets the same instance and can mutate the exact list the recreate path re-reads.

The fix is to snapshot once into a readonly Interceptor[] in the constructor and use it at all three sites. Interceptors are now taken at startup and never change for the lifetime of the worker. Side benefits:

• The worker now matches the client, which already had read-once semantics (it builds its invoker once in the constructor, and ChannelRecreatingCallInvoker swaps channels inside the interceptor wrapper, so the collection was never re-read there).
• It matches the invariant already documented ~60 lines below about not re-reading this.grpcOptions.Channel inside the loop.
• It removes a List<T> thread-safety hazard, since recreate runs on the worker's background loop and could previously race a concurrent Add().

The read-once contract is now documented in the <remarks> on Interceptors for both the worker and client options: the collection must be populated while options are being configured, and mutating it afterwards has no effect.

Added three tests, including one that pins your exact scenario — mutate the live collection after the worker is built, force a recreate, assert the rebuilt invoker still carries only the chain captured at construction. It inserts at index 0 rather than appending, so the late interceptor would land outermost and actually be observable; appending would leave it innermost where the outer short-circuit would mask it and make the assertion vacuous. It fails without the snapshot.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.