Perf: avoid repeated protobuf sizing during orchestration response chunking

[berndverst]

Summary

Reduces redundant protobuf sizing in CompleteOrchestratorTaskWithChunkingAsync, especially when a response must be split into chunks.

What changed

• Without the LargePayloads capability, action validation intentionally remains first and fail-fast: sizing stops at the first oversized action, before whole-response sizing or later-action work.
• If validation succeeds, those per-action sizes are retained and reused for chunk packing instead of calling CalculateSize() on each action again.
• With LargePayloads, the response is sized first; per-action sizes are computed only if chunking is required.
• TryAddAction receives a cached action size, and chunk packing indexes the protobuf RepeatedField directly without a ToList() copy.

A direct non-LargePayloads send still performs per-action validation followed by whole-response sizing. That extra traversal is deliberate to preserve the existing first-offender/fail-fast behavior. The eliminated redundant pass is the later per-action sizing previously performed during chunk packing.

Preserved behavior

• Oversized actions fail on the first offending action when LargePayloads is unavailable.
• Oversized actions remain allowed when LargePayloads is enabled.
• Chunk boundaries, deprecated compatibility fields, completion metadata, trace context, and retry behavior are unchanged.
• No public API changes.

Tests

Added 10 focused tests covering direct sends, exact and one-byte-over boundaries, fail-fast first-offender behavior, deterministic sizing hooks, LargePayloads behavior, multi-chunk wire compatibility, and retry. Static sizing hooks are isolated in a non-parallel xUnit collection.

The full Worker.Grpc.Tests suite and dependent integration-project build pass.

Fixes #773

[Copilot]

Pull request overview

Optimizes the gRPC worker’s orchestration completion chunking path to reduce CPU overhead by eliminating repeated protobuf sizing work, while preserving existing chunking wire-compatibility and capability behaviors.

Changes:

• Computes total orchestrator response size once and short-circuits to a direct send when it fits.
• When chunking is required, precomputes per-action sizes once and reuses them for both oversized-action validation (when LargePayloads is absent) and chunk packing.
• Adds focused unit tests covering size boundaries, LargePayloads behavior, wire-compatibility field rules across chunks, and retry behavior.

Reviewed changes

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

File	Description
test/Worker/Grpc.Tests/CompleteOrchestratorTaskWithChunkingTests.cs	Adds targeted unit tests for sizing/chunking boundaries, capability behavior, wire-compat fields, and retry semantics.
src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs	Refactors chunking logic to avoid repeated protobuf CalculateSize() calls by using a total-size fast path and cached per-action sizes when chunking.

[berndverst]

Addressed the fail-fast regression flagged in review (commit e68bbe0).

Root cause: the previous version of this PR called response.CalculateSize() (which recursively sizes every action) before per-action validation, so an early oversized action no longer short-circuited before later actions were sized — a regression vs. the original behavior.

Fix: when the LargePayloads capability is not present, the per-action validation loop now runs first again, computing and checking each action's size in order and returning immediately on the first action exceeding maxChunkBytes (matching original fail-fast cost: 1 CalculateSize() call per action up to and including the first offender). Only after validation passes is response.CalculateSize() called for the fits-in-one-chunk fast path, and the sizes already computed during validation are reused for chunk packing instead of being recalculated. When LargePayloads is enabled (or the response fits in one chunk), the zero/near-zero extra-cost behavior from the original optimization is unchanged.

New tests added:

• NoLargePayloadsCapability_FirstActionOversized_FailsOnFirstOffender_DoesNotEvaluateLaterActions — first action oversized with several later actions also individually oversized; asserts the failure references id=0 specifically (not any later id), proving iteration stops at the first offender.
• NoLargePayloadsCapability_FirstActionOversized_ShortCircuitsBeforeSizingLaterActions — calibrated timing guard (200 later actions sharing one large string; measured ~1000ms to size all vs. ~4ms for one) asserting the fail-fast path completes in well under that time (threshold 500ms, ~20x margin above observed pass time and ~2x margin below the regressed baseline).

Validation: full Worker.Grpc.Tests suite passes 147/147 (145 previous + 2 new).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

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

• When LargePayloads is not enabled, this validation loop calls action.CalculateSize() for every action even if the response ultimately fits in a single chunk, and then the fast-path still calls response.CalculateSize() (which re-walks all actions). That contradicts the PR description’s “size once, then direct-send with no per-action sizing” claim and limits the perf win on the common fit-in-one-chunk path. Consider either (a) updating the PR description to reflect this intentional fail-fast validation behavior, or (b) restructuring the method so the single-chunk fast path runs before the per-action sizing loop (accepting the tradeoff that you’d lose the current fail-fast behavior for oversized actions).

            // When the LargePayloads capability is not present, validate that no single action
            // exceeds the maximum chunk size *before* doing any whole-response sizing. This must
            // stay fail-fast: as soon as an oversized action is found, we fail the orchestration
            // immediately without computing the size of any later action. Only when every action
            // is confirmed to individually fit do we keep the sizes we already computed here, so

[berndverst]

Update: replaced flaky timing test with a deterministic one

Addressed the follow-up review finding that the fail-fast regression test (NoLargePayloadsCapability_FirstActionOversized_ShortCircuitsBeforeSizingLaterActions) relied on wall-clock timing (500ms threshold) plus a large (40MB) allocation, which could flake under GC/CI contention.

Change (commit c91c4a7):

• Added a minimal test-only static hook (Processor.testActionSizedHook, a private static Action?) invoked immediately after each action's size is computed in the no-LargePayloads fail-fast validation loop. It is always
  ull in production, so the only runtime cost is a single null-check per validated action.
• Replaced the timing-based test with NoLargePayloadsCapability_FirstActionOversized_DoesNotSizeLaterActions, which sets the hook via reflection (consistent with the existing Fixture reflection pattern used to invoke the private Processor method), runs validation against a response where action id=0 is oversized and ids 1-3 are also individually oversized, and asserts the hook recorded exactly [0] — proving later actions were never sized, deterministically and without any timing or large-allocation dependency.
• The existing behavioral test (asserting the failure message references id=0, not 1-3) is retained as a complementary check.

Validation:

• dotnet build src\Worker\Grpc\Worker.Grpc.csproj -c Debug: 0 errors.
• Targeted: CompleteOrchestratorTaskWithChunkingTests — 10/10 passed (330ms, no timing assumptions).
• Full suite: Worker.Grpc.Tests — 147/147 passed.

Fixes #773

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

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

• The new implementation still sizes/validates every action up front when the LargePayloads capability is not present (before the whole-response size check). This contradicts the PR description’s stated fast path (“if the response fits, send directly with no per-action validation/sizing”) and means the perf win won’t apply to the common non-chunked, no-LargePayloads case unless the behavior or the description/tests are adjusted.

            // When the LargePayloads capability is not present, validate that no single action
            // exceeds the maximum chunk size *before* doing any whole-response sizing. This must
            // stay fail-fast: as soon as an oversized action is found, we fail the orchestration
            // immediately without computing the size of any later action. Only when every action
            // is confirmed to individually fit do we keep the sizes we already computed here, so
            // they can be reused below instead of being recalculated.
            int[]? actionSizes = null;
            if (!largePayloads)
            {

[berndverst]

Round 3 fix (commit 63cf103): closed the whole-response-sizing coverage gap.

Issue: The round-2 hook (testActionSizedHook) only observes explicit per-action CalculateSize() calls inside the fail-fast validation loop. It would NOT catch a regression that reintroduces/hoists response.CalculateSize() (whole-response sizing) to run before validation, because protobuf's internal recursive sizing sizes every nested action without going through the instrumented per-action call site.

Fix:

• Added a second test-only hook, Processor.testResponseSizedHook (static Action?), invoked immediately adjacent to the existing response.CalculateSize() call site (unconditionally null in production; adds only a null-check).
• Extended NoLargePayloadsCapability_FirstActionOversized_DoesNotSizeLaterActions to also assert this hook is invoked zero times during the fail-fast path — proving whole-response sizing is never reached before the first-oversized-action rejection, regardless of how a future regression might reorder the code around that call site.
• Added a sanity-check assertion in the existing happy-path test (ResponseFitsExactlyAtLimit_SendsOriginalResponseDirectly_NoChunking) that the hook fires exactly once on a normal request. This guards against the fail-fast test's "never invoked" assertion being a false positive from broken hook wiring (e.g. a reflection/field-name typo).
• Both hooks are set/reset in try/finally blocks (thread-safe/reset-safe, no static leakage across tests).
• No timing or large allocations — deterministic, small payloads only.

Validation:

• Build: 0 errors, no new analyzer warnings (same #pragma warning disable/restore CS0649 pattern reused for the new field).
• Targeted (CompleteOrchestratorTaskWithChunkingTests): 10/10 passed.
• Full Worker.Grpc.Tests suite: 147/147 passed.
• Diff reviewed and scoped to the two files touched in round 2/3.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (3)

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

• The comment above the total-size fast path says it sends the response "without any further per-action work", but when LargePayloads is not enabled the method already did a full per-action CalculateSize() validation loop before reaching this point. Either the comment should be clarified, or the code should be reordered to actually skip per-action sizing when the response fits in one chunk.

            // Calculate the whole response size exactly once. If it fits in a single chunk, send
            // it directly without any further per-action work.
            int totalSize = response.CalculateSize();

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

• The PR description states the whole-response size is computed up front and that if it fits within maxChunkBytes the response is sent with no per-action sizing/validation pass. However, for workers without the LargePayloads capability this implementation still sizes every action (Validate loop) before computing totalSize, even when the response ultimately fits in a single chunk. Consider reordering to: (1) compute totalSize first and return early if it fits; (2) only if chunking is required, compute per-action sizes once for validation and packing (or update the PR description/tests if the current fail-fast-before-total-sizing behavior is intentional).

            // When the LargePayloads capability is not present, validate that no single action
            // exceeds the maximum chunk size *before* doing any whole-response sizing. This must
            // stay fail-fast: as soon as an oversized action is found, we fail the orchestration
            // immediately without computing the size of any later action. Only when every action
            // is confirmed to individually fit do we keep the sizes we already computed here, so
            // they can be reused below instead of being recalculated.
            int[]? actionSizes = null;

test/Worker/Grpc.Tests/CompleteOrchestratorTaskWithChunkingTests.cs:23

• These tests mutate global static hooks (via reflection) to observe sizing behavior. Because xUnit runs test classes in parallel by default, any other test exercising the same Processor method concurrently could observe or trigger these hooks, making the assertions flaky. Consider putting this class into a non-parallelized test collection to isolate the static hook state.

public class CompleteOrchestratorTaskWithChunkingTests

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

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

• In the !largePayloads path, per-action sizing/validation runs unconditionally before response.CalculateSize(). That means even when the response ends up fitting in one chunk, each action is sized once during validation and then sized again as part of whole-response sizing, which undermines the stated perf goal of avoiding redundant protobuf sizing on the direct-send path.

If the intent is to avoid per-action sizing when the response fits, consider calculating totalSize first and returning early when totalSize <= maxChunkBytes (skipping validation entirely in that case, since an oversized single action cannot exist when the total fits). If the current ordering is intentional to preserve fail-fast behavior for oversized actions (avoid whole-response sizing before rejecting), the PR description/tests should be aligned to that tradeoff.

            bool largePayloads = this.worker.grpcOptions.Capabilities.Contains(P.WorkerCapability.LargePayloads);

            // When the LargePayloads capability is not present, validate that no single action
            // exceeds the maximum chunk size *before* doing any whole-response sizing. This must
            // stay fail-fast: as soon as an oversized action is found, we fail the orchestration
            // immediately without computing the size of any later action. Only when every action
            // is confirmed to individually fit do we keep the sizes we already computed here, so
            // they can be reused below instead of being recalculated.
            int[]? actionSizes = null;
            if (!largePayloads)
            {
                actionSizes = new int[response.Actions.Count];
                for (int i = 0; i < response.Actions.Count; i++)
                {