Repository navigation
Preflight externalized payload sizes before uploads - #835
Conversation
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>
There was a problem hiding this comment.
🟢 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>
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>
wangbill (YunchuWang)
left a comment
There was a problem hiding this comment.
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.
Summary
What changed?
HistoryStatefield.Why is this change needed?
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.Issues / work items
Project checklist
release_notes.mdAI-assisted code disclosure (required)
Was an AI tool used? (select one)
If AI was used:
AI verification (required if AI was used):
Testing
Automated tests
ace93d0076c0fae874bcd1b1917758ca52522817, which merges mainf7041c032834ec72a1a3653803a58ab9fb354472. All four extension target frameworks build with zero errors. Initial publication evidence is retained separately below.08f0074ce7634615d84ff07c8117d91f706e997dadds 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
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.
Result: Passed, 38 tests, including actual worker preparation/entity-unlock and DI/interceptor registration coverage.
Result: Passed for
netstandard2.0,net6.0,net8.0, andnet10.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.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
85ef3090c97ac9f18bb737aa58a57c2f6eb5068epassed 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.NETSDK1004); both passed.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.
Result on the initial head: Passed, 38 tests.
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)
HistoryStatefields.Notes for reviewers
Result/Detailsassignment 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 protectedMaybeExternalizeAsynchelper remains for the nine single-payload request cases.CHANGELOG.mdand package README; no separaterelease_notes.mdor backport was requested. All Release v1.28.0 #834 release entries and manifest/version changes are preserved unchanged from latest main.