Skip to content

Isolate in-process work-item completion by delivery token - #815

Merged
wangbill (YunchuWang) merged 4 commits into
mainfrom
yunchuwang-test-host-completion-ownership
Oct 13, 2026
Merged

wangbill (YunchuWang) merged 4 commits into
mainfrom
yunchuwang-test-host-completion-ownership

Conversation

@YunchuWang

@YunchuWang wangbill (YunchuWang) commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Summary

What changed?

  • [test host] Give every activity and orchestration delivery a fresh completion token. Validate the token and logical identity before accepting a result, and let completion and failed-send cleanup atomically claim the same delivery.
  • [test host] Preserve an accepted completion when its pending stream write later fails. Ordinary send failures still propagate, and stale completion or cleanup cannot consume another delivery.
  • [test host] Accept whole orchestrator completion responses only. Remove legacy partial-action accumulation. A valid, matching response with IsPartial = true or any present ChunkIndex, including a final fragment, atomically settles its delivery with a single terminal Failed action and non-retriable NotSupportedException details, while returning InvalidArgument to the sender. Discard all fragment actions; invalid tokens or identities still leave valid deliveries untouched.
  • [tests] Cover token uniqueness, invalid/stale/duplicate completion, competing final responses, activity success/failure results, accepted-completion/send-failure races (including terminal chunk rejection), unchanged abandonment/disconnection, malformed legacy requests, and replay snapshot ownership. A bounded real SDK/local-gRPC regression verifies terminal failure with no fragment-scheduled activities instead of the previous pending hang. Preserve full-response fanout/mixed-action coverage and the single-oversized-action failure case.
  • [SDK unchanged] No SDK worker, protobuf, dispatcher, or in-memory requeue changes. Replay-history streaming remains supported. Both Abandon RPCs retain their baseline acknowledgement-only behavior.

Why is this change needed?

  • Logical instance/task IDs identify work, not a particular delivery. Completion ownership must prevent a late request or send error from settling or requeueing the wrong delivery.
  • This is the prerequisite completion-ownership PR for #814. Explicit abandonment cancellation, token validation for abandonment, and redelivery behavior belong to that dependent follow-up, not this PR.
  • Removing test-host partial-response support is intentional. Deprecated fragment fields are rejected explicitly rather than silently treating the first fragment as a full response. The terminal Failed action uses the existing dispatcher commit path so acknowledgement-only abandonment cannot strand the execution, and no automatic requeue loop is introduced. History streaming is a different protocol and is unchanged.

Breaking Change

Type: behavioral

Impact: Direct gRPC callers of the in-process test host must now echo the received delivery token exactly, including its casing. Missing tokens and mismatched identities return InvalidArgument; unknown, wrong-kind, and settled tokens return NotFound, without consuming valid deliveries. The test host also no longer accepts deprecated partial/chunked orchestrator completions: a valid matching chunk fails the orchestration terminally, releases its execution/snapshot resources through the existing commit path, and returns InvalidArgument to the sender. The SDK's existing oversized-response chunking implementation is unchanged and now encounters this explicit failure instead of leaving the orchestration pending.

Migration: Copy WorkItem.completionToken into every supported ActivityResponse or OrchestratorResponse, preserving the instance ID and activity task ID. Send all orchestrator actions in one response with IsPartial false and ChunkIndex omitted, within the configured SDK/gRPC message-size limits. Normal whole-response SDK callers already echo the token. There is no binary/source API change, protobuf field renumbering, serializer change, or orchestrator replay change. Migration details are also documented in src/InProcessTestHost/README.md.

Issues / work items

  • Resolves: N/A; no issue is closed by this prerequisite.
  • Related #814.

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: Required completion-token echo and intentional test-host-only terminal failure for deprecated chunked completion.
      • Migration guidance: Echo the delivery token; send one full orchestration response without a chunk index. See the Breaking Change section and test-host README.

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.
  • AI-assisted areas/files: Test-host completion receiver and README, completion/snapshot tests, and the coupled local gRPC completion tests.
  • What you changed after AI output: The user directed removal of test-host partial-response support. Review exposed a tightly coupled pending-execution bug, which was corrected with terminal failed-action settlement and validation-first ownership without restoring partial support or changing SDK/Abandon behavior. No separate human-authored edits or additional human approval are claimed.

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

These personal review attestations are left for the human reviewer; automated evidence is recorded below.


Testing

Automated tests

  • Result: Passed locally on Windows with .NET SDK 10.0.401.
  • dotnet test test\InProcessTestHost.Tests\InProcessTestHost.Tests.csproj --configuration Release --no-build --no-restore --filter "FullyQualifiedName~WorkItemCompletion|FullyQualifiedName~WorkerHistorySnapshot|FullyQualifiedName~HubLifecycle" --verbosity minimal - passed, 46/46, zero skipped.
  • dotnet test test\InProcessTestHost.Tests\InProcessTestHost.Tests.csproj --configuration Release --no-build --no-restore --verbosity minimal - passed, 98/98, zero skipped.
  • dotnet test test\Grpc.IntegrationTests\Grpc.IntegrationTests.csproj --configuration Release --no-build --no-restore --filter "FullyQualifiedName~AutochunkTests|FullyQualifiedName~LargePayloadTests|FullyQualifiedName~ClassSyntaxIntegrationTests" --verbosity minimal - passed, 29/29, zero skipped. This covers actual SDK chunk rejection/terminal failure, localhost completion, the preserved oversized-action failure, payload externalization/history streaming, and class-based/versioned SDK callers.
  • dotnet build src\InProcessTestHost\InProcessTestHost.csproj --configuration Release --no-restore --verbosity minimal - passed for net6.0, net8.0, and net10.0, zero errors; existing analyzer warnings remain.
  • git diff --check origin/main HEAD - passed.
  • Both original accepted-completion/send-failure regressions failed before the receiver ownership change. Before the terminal-settlement fix, the real SDK regression logged InvalidArgument, then successful Abandon acknowledgement, and timed out after 10 seconds; its bounded cleanup released the old delivery. Eight direct cases also failed, covering stranded snapshots, wrong-kind/unknown token precedence, competing settlement, and a rejected-chunk/late-send race. These now pass with terminal Failed results and isolated ownership.
  • Full-solution tests, the remaining integration classes, and service/emulator-specific lanes were not run locally. Hosted CI for the new head is pending; passing checks on the prior head are not claimed for these updates.

Manual validation (only if runtime/behavior changed)

  • Environment (OS, .NET version, components): Windows, .NET SDK 10.0.401; in-process receiver and localhost gRPC fixture.
  • Steps + observed results: No separate manual runtime exercise was performed. The deterministic automated tests above exercise the changed behavior. Source comparison confirms both Abandon RPC bodies and StreamInstanceHistory are identical to main, and no SDK/protobuf/dispatcher changes are included.
  • Evidence (optional): Local red/green TRX results and the framework-build log were retained; hosted checks will provide shared CI evidence.

Notes for reviewers

  • Suggested review order: token-owned pending state and completion validation; send-failure ownership; validation-first whole-response-only terminal failure; focused completion and snapshot tests; the real SDK regression and migration documentation.
  • Explicit abandonment is deliberately unchanged in this intermediate stage. This PR does not add cancellation, abandonment-token rejection, lease/timeout behavior, or implicit abandonment on stream disconnect.
  • Invalid token/identity errors are non-consuming. Valid unsupported chunks are different: only their matching delivery is settled as terminal Failed, without running or accumulating any fragment action. Faulting/canceling the completion source is intentionally avoided because that would requeue the same unsupported input.
  • #814 is not modified by this publication. A later approved transition can normally merge this prerequisite into its existing source branch, preserve the original combined and race-fix commits, retain the whole-response-only guard while resolving overlaps, and retire superseded/partial-positive tests. Its base can then be this prerequisite branch while unmerged, or main after PR1 merges. No force-push or replacement of Honor explicit abandonment in the in-process test host #814 is required.

Assign fresh completion tokens for both work-item kinds and coordinate validation, completion, and send-failure cleanup using per-delivery pending state. Preserve an accepted result when its pending send later fails.

Retire test-host partial response accumulation and reject deprecated partial/chunked completion fields without settling the delivery. Keep replay history streaming and acknowledgement-only abandonment unchanged. Document direct gRPC migration and include completion/snapshot coverage plus compatible local integration expectations.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5777cc4b-513e-47b7-a159-75ad5e8a7298
Copilot AI balanced review requested due to automatic review settings October 9, 2026 21:46
Comment thread src/InProcessTestHost/Sidecar/Grpc/TaskHubGrpcServer.cs Fixed
Comment thread test/InProcessTestHost.Tests/WorkItemCompletionTests.cs Fixed
Comment thread test/InProcessTestHost.Tests/WorkItemCompletionTests.cs Fixed

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

Core concurrency and migration behavior are well covered; remaining feedback concerns non-blocking test naming.

1 open finding
What changed in this PR

Introduces delivery-scoped completion ownership for the in-process test host.

Changes:

  • Adds atomic token-based completion and send-failure handling.
  • Rejects deprecated chunked orchestrator responses.
  • Expands race, snapshot, token, and integration coverage with migration documentation.
File Description
src/​InProcessTestHost/​Sidecar/​Grpc/​TaskHubGrpcServer.cs Implements token-owned completion settlement.
src/​InProcessTestHost/​README.md Documents behavioral break and migration.
test/​InProcessTestHost.Tests/​WorkItemCompletionTests.cs Covers ownership and completion races.
test/​InProcessTestHost.Tests/​WorkerHistorySnapshotTests.cs Verifies snapshot ownership and cleanup.
test/​Grpc.IntegrationTests/​AutochunkTests.cs Updates integration coverage for full responses.

🧠 Review effort: Balanced


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

Comment thread test/Grpc.IntegrationTests/AutochunkTests.cs Outdated
Copilot AI balanced review requested due to automatic review settings October 12, 2026 21:19
Comment thread src/InProcessTestHost/Sidecar/Grpc/TaskHubGrpcServer.cs

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.

🟡 Changes recommended

Rejecting SDK-generated chunks can leave orchestrations and replay snapshots permanently pending.

2 open findings

🧠 Review effort: Balanced

Comment thread src/InProcessTestHost/Sidecar/Grpc/TaskHubGrpcServer.cs Outdated
wangbill (YunchuWang) and others added 2 commits October 12, 2026 18:22
Document exact case-sensitive delivery-token echo while retaining case-insensitive instance IDs. Initialize activity results with a conditional expression, use disposal-path using declarations for the original readonly session cancellation sources, and add Async suffixes to the newly introduced integration tests.

Keep these nonbehavioral review changes separate from unsupported-chunk settlement.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5777cc4b-513e-47b7-a159-75ad5e8a7298
Validate delivery token and identity before rejecting legacy chunk fields. Atomically settle a valid unsupported completion with one non-retriable Failed action while returning InvalidArgument to the sender. Discard fragment actions and use the existing commit path instead of stranding or requeueing the delivery.

Add bounded real SDK/local-gRPC red-green coverage, malformed-request isolation, competing settlement and late-send-failure cases. Document terminal failure while leaving SDK, history streaming and general abandonment unchanged.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5777cc4b-513e-47b7-a159-75ad5e8a7298
Copilot AI balanced review requested due to automatic review settings October 12, 2026 22:35

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.

🟡 Changes recommended

The PR summary contradicts the implementation’s terminal settlement behavior for rejected chunks.

1 open finding
2 resolved since last review

🧠 Review effort: Balanced

Comment thread src/InProcessTestHost/Sidecar/Grpc/TaskHubGrpcServer.cs
@YunchuWang
wangbill (YunchuWang) merged commit 9173968 into main Oct 13, 2026
11 checks passed
@YunchuWang
wangbill (YunchuWang) deleted the yunchuwang-test-host-completion-ownership branch October 13, 2026 19:17
wangbill (YunchuWang) added a commit that referenced this pull request Oct 13, 2026
Build on the per-delivery completion ownership merged in PR #815. Validate
explicit activity and orchestrator abandonment tokens, atomically claim the
matching pending delivery, and cancel its host-side result wait so the existing
dispatcher releases and requeues the work.

Keep completion, terminal unsupported-chunk failure, send-failure ownership,
history snapshots, SDK, and protobuf behavior unchanged. Add focused cancellation,
stale-delivery, competing-settlement, snapshot, and localhost SDK-redelivery
coverage; retire the prerequisite's acknowledgement-only abandonment regression.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b1e0a4ae-0f35-4308-9ceb-e920bdbb9dab
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.

4 participants