Perf: avoid repeated protobuf sizing during orchestration response chunking

[berndverst]

Summary

CompleteOrchestratorTaskWithChunkingAsync in src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs previously called CalculateSize() on every orchestrator action up to three times per response:

1. Once during oversized-action validation (ValidateActionsSize), when LargePayloads was not enabled.
2. Once implicitly via response.CalculateSize() for the whole-response fits-in-one-chunk check (this recursively calls CalculateSize() on every action again).
3. Once more per action inside TryAddAction while packing chunks.

For large action payloads and/or many actions, this repeated protobuf sizing added unnecessary CPU overhead.

Fix

• The whole response is now sized exactly once, up front (response.CalculateSize()). If it fits within maxChunkBytes, it is sent directly with no per-action validation or sizing pass — a fitting total size guarantees no individual action can exceed the same limit (every field, including each action, contributes a non-negative amount to the total).
• Only when chunking is actually required are individual action sizes computed — once, into a small int[] cache — and those cached sizes are reused for both:
  • the oversized-single-action validation loop (only relevant when LargePayloads capability is not present), and
  • the TryAddAction chunk-packing loop (which now takes a precomputed int actionSize parameter instead of calling CalculateSize() internally).

What's preserved (unchanged)

• Chunk wire compatibility: IsPartial/ChunkIndex (deprecated but still emitted), NumEventsProcessed null-on-chunk-0/0-afterward, OrchestrationTraceContext only on chunk 0, CustomStatus/InstanceId/CompletionToken/RequiresHistory repeated every chunk.
• Size-boundary behavior: an action that exactly fills remaining chunk space still lands in the same chunk; one byte over pushes it to the next chunk.
• LargePayloads capability behavior: oversized actions are still allowed through un-failed when the capability is present, and still fail the orchestration (same error message format) when it is not.
• Oversized-single-action failure behavior: identical TaskFailureDetails/error message construction.
• Retry behavior: sends still go through ExecuteWithRetryAsync exactly as before.
• Public API: no signature changes to any public type.

Tests

Added test/Worker/Grpc.Tests/CompleteOrchestratorTaskWithChunkingTests.cs with 8 focused tests covering:

• Direct-send fast path when the response fits exactly at the limit (asserts reference-equality — no chunk reconstruction).
• Response one byte over the limit but actions still individually fit (single non-partial chunk via the chunking construction path).
• Action-level boundary: exactly fills remaining chunk space vs. exceeds it by one byte.
• LargePayloads off vs. on with a single oversized action (failure vs. pass-through).
• Multi-chunk response verifying all wire-compatibility field rules across 3 chunks.
• Transient gRPC error during send, verifying retry-then-success.

Validation

• dotnet build src\Worker\Grpc\Worker.Grpc.csproj -c Debug — 0 errors.
• dotnet test test\Worker\Grpc.Tests\Worker.Grpc.Tests.csproj — full suite: 145/145 passed (including the 8 new tests).
• dotnet build test\Grpc.IntegrationTests\Grpc.IntegrationTests.csproj -c Debug — 0 errors (confirms no signature-dependent breakage for integration tests).

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