Skip to content

Preflight externalized payload sizes before uploads - #835

Merged
wangbill (YunchuWang) merged 3 commits into
mainfrom
yunchuwang-payload-size-preflight
Nov 5, 2026
Merged

wangbill (YunchuWang) merged 3 commits into
mainfrom
yunchuwang-payload-size-preflight

Conversation

@YunchuWang

@YunchuWang wangbill (YunchuWang) commented Nov 4, 2026 •

Copy link
Copy Markdown
Member

Summary

What changed?

  • Snapshot and validate eligible UTF-8 payload fields while collecting operations, before starting any upload or substituting a token for the message.
  • Apply preflight to orchestration responses, entity batch requests/results, and nested continue-as-new carryover/rewind history, including every HistoryState field.
  • Preserve inclusive threshold/maximum boundaries, recognized tokens, inline/empty/absent fields, cancellation priority, bounded upload concurrency/drainage, actual I/O-error ordering, and the merged Fix payload retries and entity cleanup after storage failures #816 retry/entity-unlock behavior.
  • Add 79 initial regression cases and strengthen native-worker entity-lock failure coverage with an earlier valid custom status. The review follow-up adds five observable monitor-independence cases, strengthens the two queued nested-snapshot cases, and expands cancellation/failure drainage to rewind as well as carryover.
  • Per review feedback, remove the generic externalization helper's message parameter, assignment monitor, and widened test-hook calls. Keep direct assignments for independent fields while preserving the pre-existing scoped completion/hydration synchronization. Correct the XML contract to describe concurrent distinct-field assignments rather than generic message serialization.
  • Merge latest main (Release v1.28.0 #834) normally, preserving all release/dependency content and keeping this fix under Unreleased. Update the package README and changelog.

Why is this change needed?

  • On upstream main 554b6759b5de5630e0b47bc6cd69cc2640c89a57, the initial four desired-outcome regressions all failed against unchanged production source: valid-first/oversized-later orchestration messages uploaded one blob, and entity batches uploaded two, for both ASCII and Unicode inputs.
  • Those uploads can become unrecorded when permanent-failure conversion discards their references. The sizes were already knowable before I/O, so preflight prevents this deterministic failure path without adding rollback.
  • Generated payload setters write independent string fields, and upload/assignment tasks drain before serialization or native worker completion. Locking every independently assigned message is unnecessary. Five new observable tests timed out with the widened helper and pass with direct assignments, without adding production test seams.

Issues / work items


Project checklist

  • Release notes are not required for the next release
    • Otherwise: Notes added to release_notes.md
  • Backport is not required
    • Otherwise: Backport tracked by issue/PR: N/A; no backport requested.
  • All required tests have been added/updated (unit tests, E2E tests)
  • Breaking change?
    • If yes:
      • Impact: No breaking change — public signatures, protobuf fields, defaults, token formats, and persisted history/JSON representations are unchanged by this fix. Deterministic size validation now precedes hypothetical upload failures, as required by Preflight all externalized payload sizes before starting uploads #820 and documented in the README/changelog. Upstream release/dependency changes come unchanged from Release v1.28.0 #834.
      • Migration guidance: No caller or data migration is required for the preflight fix.

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 (GPT-6.1 Sol).
  • AI-assisted areas/files: Both payload interceptors, interceptor/native-worker regressions, README, changelog, conflict resolution, and PR description.
  • What you changed after AI output: The user requested removal of the newly widened test-hook call and questioned the generic message lock. The agent removed both from the generic helper, preserved pre-existing scoped safeguards, strengthened observable tests, resolved the actual release changelog conflict, and corrected the directly coupled concurrency documentation. No human-authored code edits or human verification attestations are claimed; human review is pending.

AI verification (required if AI was used):

  • I understand the code and can explain it
  • 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

  • Result: Review follow-up passed: 225 tests, zero failures, zero skips on ace93d0076c0fae874bcd1b1917758ca52522817, which merges main f7041c032834ec72a1a3653803a58ab9fb354472. All four extension target frameworks build with zero errors. Initial publication evidence is retained separately below.
  • Final head 08f0074ce7634615d84ff07c8117d91f706e997d adds only the XML concurrency-contract correction; runtime code is unchanged from the validated follow-up. Working-tree and staged whitespace checks passed for this documentation-only commit; runtime tests/builds were not rerun for the comment-only change. Command examples omit optional machine-local results-directory routing; the original TRX evidence is retained locally.

Review follow-up

  • Restores were required because the merged Release v1.28.0 #834 changes dependency manifests. Both commands passed using the unchanged repository feed configuration:
dotnet restore test\Grpc.IntegrationTests\Grpc.IntegrationTests.csproj --configfile nuget.config --verbosity minimal --nologo
dotnet restore test\Extensions\AzureBlobPayloads.Tests\AzureBlobPayloads.Tests.csproj --configfile nuget.config --verbosity minimal --nologo
dotnet test test\Grpc.IntegrationTests\Grpc.IntegrationTests.csproj --configuration Release --no-restore --filter 'FullyQualifiedName~AzureBlobPayloadsSideCarInterceptorTests|FullyQualifiedName~LargePayloadTests' --verbosity minimal --nologo --logger 'trx;LogFileName=assignment-focused.trx'

Result: Passed, 187 tests. Includes the five held-monitor independence cases, queued original nested snapshots behind eight in-flight uploads, carryover/rewind drainage before cancellation/failure returns, and all existing preflight/token-retry/concurrency/error-ordering cases.

dotnet test test\Extensions\AzureBlobPayloads.Tests\AzureBlobPayloads.Tests.csproj --configuration Release --no-restore --filter 'FullyQualifiedName~OrchestratorPayloadFailureTests|FullyQualifiedName~ExternalizedPayloadsInterceptorTests|FullyQualifiedName~UseExternalizedPayloadsTests' --verbosity minimal --nologo --logger 'trx;LogFileName=assignment-worker.trx'

Result: Passed, 38 tests, including actual worker preparation/entity-unlock and DI/interceptor registration coverage.

dotnet build src\Extensions\AzureBlobPayloads\AzureBlobPayloads.csproj --configuration Release --no-restore --verbosity minimal --nologo

Result: Passed for netstandard2.0, net6.0, net8.0, and net10.0; 192 compiler/analyzer warnings and zero errors after the upstream dependency/analyzer refresh. Existing warning sites include missing constructor parameter documentation and member ordering; no warning suppression or unrelated warning cleanup is added.

git --no-pager diff --check
git -c core.whitespace=blank-at-eol,blank-at-eof,space-before-tab,cr-at-eol --no-pager diff --cached --check

Result: Both passed before the follow-up commit. The staged check recognizes the changelog's preserved mixed/CRLF line endings. The resolved changelog was also verified against upstream bytes: every release entry is unchanged, with only the preflight entry added under Unreleased.

Initial publication

  • Initial head 85ef3090c97ac9f18bb737aa58a57c2f6eb5068e passed 217 tests, zero failures, zero skips. The four-target extension build passed with zero errors and 123 warnings. These results apply to that original head/base, not the updated dependency baseline.
  • The two restore commands above initially ran only after fresh-worktree test commands reported missing assets (NETSDK1004); both passed.
  • The build command above passed for all four frameworks.
dotnet test test\Grpc.IntegrationTests\Grpc.IntegrationTests.csproj --configuration Release --no-build --no-restore --filter 'FullyQualifiedName~AzureBlobPayloadsSideCarInterceptorTests|FullyQualifiedName~LargePayloadTests' --verbosity minimal --nologo

Result on the initial head: Passed, 179 tests, including all 79 initial cases and existing concurrency, drainage, error-ordering, failure-conversion, history, and completion-retry coverage.

dotnet test test\Extensions\AzureBlobPayloads.Tests\AzureBlobPayloads.Tests.csproj --configuration Release --no-build --no-restore --filter 'FullyQualifiedName~OrchestratorPayloadFailureTests|FullyQualifiedName~ExternalizedPayloadsInterceptorTests|FullyQualifiedName~UseExternalizedPayloadsTests' --verbosity minimal --nologo

Result on the initial head: Passed, 38 tests.

git --no-pager diff --check origin/main...HEAD

Result on the initial head/base: Passed. Working-tree and staged checks also passed before the initial commit.

Manual validation (only if runtime/behavior changed)

  • Environment (OS, .NET version, components): Windows, .NET SDK 10.0.403; in-memory payload stores and the existing actual-worker/mock-gRPC harness. These runtime exercises are automated, not a separate manual/live-Azure run.
  • Steps + observed results:
    1. Before modifying production source, run four ASCII/Unicode regressions on the original upstream main and observe unwanted uploads (one for orchestration, two for entity batch). They now pass with zero uploads.
    2. Verify zero uploads and unchanged original payload fields on deterministic oversize across orchestration action fields, both entity batch surfaces, all 18 supported nested history event types in carryover/rewind, and later HistoryState fields.
    3. Verify UTF-8 boundaries, all-valid controls, recognized-token retries, precancellation/no-op behavior, pre-dispatch snapshots, bounded I/O/error ordering, and native worker entity releases. In the follow-up, hold independent message monitors from another thread and verify externalization still completes; keep nested fields queued behind eight gated uploads and verify original snapshots survive mutations; assert all started nested uploads/assignments finish before return or failure conversion.
  • Evidence (optional): Initial and follow-up baseline/final TRX files and command/build logs are retained separately in the coding session. No live Storage resources were used.

Notes for reviewers

  • This is not a cross-blob transaction, retroactive orphan cleanup, or a zero-orphan guarantee after real I/O failures, cancellation during uploads, or uncertain backend submission. No catch-and-delete rollback is added.
  • Pre-existing completion Result/Details assignment locks/test hooks and response-state hydration locks are retained in their original scopes; no test seam or generic locking configuration is introduced. The used protected MaybeExternalizeAsync helper remains for the nine single-payload request cases.
  • The full solution suite, live Azure tests, and DTS emulator tests were not run locally for this follow-up; hosted checks and human review remain separate evidence.
  • Release documentation is updated in the existing CHANGELOG.md and package README; no separate release_notes.md or backport was requested. All Release v1.28.0 #834 release entries and manifest/version changes are preserved unchanged from latest main.
  • The follow-up is a normal merge/fix commit on this same PR branch, not an amend, force push, or new PR. No other PR branch was modified. Human verification checkboxes above are intentionally left unchecked.

Validate eligible UTF-8 payload snapshots before dispatching any message uploads, including entity batches and nested carryover/rewind history. Preserve cancellation priority, bounded I/O, retry tokens, and entity unlocks. Fixes #820.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 4, 2026 21:59

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.

🟢 Approval recommended

No active review issues remain for the change that validates payload sizes before uploads begin.

No open issues 🎉

What changed in this PR

This PR addresses #820 by validating eligible payload sizes before any uploads begin, preventing unrecorded blobs when a later field exceeds the configured maximum. It builds on #816’s retry and entity-unlock handling.

Changes:

  • Snapshot and validate UTF-8 payload sizes across orchestration responses, entity batches, and nested carryover/rewind history before uploading.
  • Add regression coverage for zero-upload failures, cancellation, size boundaries, token preservation, snapshots, and entity cleanup.
  • Document validation ordering and the limits of orphan prevention after actual I/O failures.
File Description
test/​Grpc.IntegrationTests/​AzureBlobPayloadsSideCarInterceptorTests.cs Adds preflight, boundary, cancellation, retry, and snapshot regressions.
test/​Extensions/​AzureBlobPayloads.Tests/​OrchestratorPayloadFailureTests.cs Strengthens entity-lock failure coverage with an earlier valid custom status.
src/​Extensions/​AzureBlobPayloads/​README.md Documents size validation guarantees and limitations.
src/​Extensions/​AzureBlobPayloads/​Interceptors/​PayloadInterceptor.cs Separates size validation from upload execution.
src/​Extensions/​AzureBlobPayloads/​Interceptors/​AzureBlobPayloadsSideCarInterceptor.cs Collects validated field snapshots before dispatching bounded uploads.
CHANGELOG.md Records the preflight behavior change under Unreleased.

Review effort: Balanced

Remove the widened message monitor and test hooks from generic externalization assignments while preserving baseline completion/hydration synchronization. Cover independent message monitors, queued nested snapshots, and carryover/rewind drainage. Resolve the release changelog conflict without changing upstream release content.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 5, 2026 04:48

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.

🟢 Approval recommended

No active review issues remain for the message-wide payload-size preflight change.

No open issues 🎉

Review effort: Balanced

Describe distinct captured field assignments and only the retained completion/hydration lock scopes, rather than promising generic message serialization. No runtime behavior changes.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 5, 2026 04:52

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.

🟢 Approval recommended

No active review issues block approval of the payload-size preflight change.

No open issues 🎉

Review effort: Balanced

@YunchuWang wangbill (YunchuWang) left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed commit 08f0074ce7634615d84ff07c8117d91f706e997d; its runtime code is identical to the tested ace93d0 revision. No additional high-confidence correctness issue found.

The generic assignment helper now has no message monitor or widened test-hook calls. I checked the generated string backing fields, original-field snapshots, preflight-before-I/O ordering, inclusive UTF-8 limits, recognized-token retries, cancellation/no-op behavior, drainage and message-order failure selection, and native worker entity-unlock preparation before completion/chunking. The pre-existing completion/hydration synchronization remains scoped rather than being removed as unrelated refactoring. MaybeExternalizeAsync remains used by the nine single-field request/result paths.

The 225 focused regression results (187 interceptor/large-payload and 38 native-worker/DI cases) were verified from the retained test records, including held-monitor independence, queued nested snapshots and cancellation/failure drainage. The four-target package build passed; its 192 compiler/analyzer warnings remain disclosed in the PR. The final documentation-only commit corrects the helper's serialization description; the latest-main changelog conflict is resolved without dropping release entries. All three user feedback threads have been answered and resolved.

This is a comment review, not an approval or merge. Full-solution and live-Azure validation are not claimed; hosted checks are tracked separately.

@YunchuWang
wangbill (YunchuWang) merged commit 5fdb790 into main Nov 5, 2026
11 checks passed
@YunchuWang
wangbill (YunchuWang) deleted the yunchuwang-payload-size-preflight branch November 5, 2026 19:18
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.

Preflight all externalized payload sizes before starting uploads

4 participants