Repository navigation
Isolate in-process work-item completion by delivery token - #815
Open
wangbill (YunchuWang) wants to merge 1 commit into
Open
wangbill (YunchuWang) wants to merge 1 commit into
wangbill (YunchuWang) wants to merge 1 commit into
Conversation
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
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Comment on lines
+641
to
+653
| if (request.FailureDetails == null) | ||
| { | ||
| resultEvent = new TaskCompletedEvent(-1, request.TaskId, request.Result); | ||
| } | ||
| else | ||
| { | ||
| resultEvent = new TaskFailedEvent( | ||
| eventId: -1, | ||
| taskScheduledId: request.TaskId, | ||
| reason: null, | ||
| details: null, | ||
| failureDetails: ProtobufUtils.GetFailureDetails(request.FailureDetails)); | ||
| } |
| sealed class ServerSession : IAsyncDisposable | ||
| { | ||
| readonly CancellationTokenSource stopping = new(); | ||
| readonly CancellationTokenSource connectionCancellation = new(); |
|
|
||
| sealed class ServerSession : IAsyncDisposable | ||
| { | ||
| readonly CancellationTokenSource stopping = new(); |
Contributor
There was a problem hiding this comment.
🟢 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.
| /// </summary> | ||
| [Fact] | ||
| public async Task Autochunk_MultipleChunks_CompletesSuccessfully() | ||
| public async Task FullResponse_MultipleActions_CompletesSuccessfully() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What changed?
IsPartial = trueor any presentChunkIndex, including a final fragment, withInvalidArgumentwithout consuming the delivery.Why is this change needed?
Breaking Change
Type: behavioral
Impact: Direct gRPC callers of the in-process test host must now echo the received delivery token on completion. Missing tokens and mismatched identities return
InvalidArgument; unknown, wrong-kind, and settled tokens returnNotFound. The test host also no longer accepts deprecated partial/chunked orchestrator completions. The SDK's existing oversized-response chunking implementation is unchanged, but those fragments are intentionally unsupported by this test host.Migration: Copy
WorkItem.completionTokeninto every supportedActivityResponseorOrchestratorResponse, preserving the instance ID and activity task ID. Send all orchestrator actions in one response withIsPartialfalse andChunkIndexomitted, 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 insrc/InProcessTestHost/README.md.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):
These personal review attestations are left for the human reviewer; automated evidence is recorded below.
Testing
Automated tests
dotnet test test\InProcessTestHost.Tests\InProcessTestHost.Tests.csproj --configuration Release --no-build --no-restore --filter "FullyQualifiedName~WorkItemCompletion|FullyQualifiedName~WorkerHistorySnapshot" --verbosity minimal- passed, 28/28, zero skipped.dotnet test test\InProcessTestHost.Tests\InProcessTestHost.Tests.csproj --configuration Release --no-build --no-restore --verbosity minimal- passed, 80/80, 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, 28/28, zero skipped. This covers 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.Manual validation (only if runtime/behavior changed)
StreamInstanceHistoryare identical to main, and no SDK/protobuf/dispatcher changes are included.Notes for reviewers