Skip to content

Honor explicit abandonment in the in-process test host - #814

Open
wangbill (YunchuWang) wants to merge 2 commits into
mainfrom
yunchuwang-test-host-explicit-abandonment
Open

wangbill (YunchuWang) wants to merge 2 commits into
mainfrom
yunchuwang-test-host-explicit-abandonment

Conversation

@YunchuWang

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

Copy link
Copy Markdown
Member

Summary

What changed?

  • [test host] Previously, both Abandon RPCs acknowledged the request but left the delivery's result wait pending. They now validate the delivery token, atomically remove the matching pending activity/orchestrator delivery, and cancel its host-side wait so the existing dispatcher releases and requeues the work.
  • [SDK] No change. The worker already sends explicit abandonment requests with the delivered token.
  • [tests] Add focused abandonment cancellation, stale/duplicate-token isolation, completion-versus-abandonment, delayed-send cleanup, history snapshot, and localhost redelivery coverage. Retire the prerequisite's acknowledgement-only abandonment regression.
  • Rebased onto merged #815. Delivery tokens, completion validation/ownership, accepted-result precedence after send failure, and terminal rejection of unsupported chunked completions already landed there and are not this PR's changes. Completion handlers, pending-state classes, SDK, protobuf, dispatchers, storage, hub lifecycle, and existing history tests are unchanged.

Why is this change needed?

  • A worker can explicitly reject a delivery it cannot process. Previously, the test host acknowledged that rejection while continuing to wait indefinitely for its result. Canceling only that delivery's host-side wait lets the existing dispatcher retry it with a fresh delivery token instead of leaving it stuck.

Issues / work items

  • Resolves: N/A
  • Related: #815, the merged completion-ownership prerequisite.
  • Related: #467 added worker-side abandonment on processing failure; this PR changes only the test-host receiver.

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 #issue_or_pr
  • All required tests have been added/updated (unit tests, E2E tests)
  • Breaking change?
    • If yes:
      • Impact: Intentional behavioral change for direct test-host gRPC callers: abandonment now validates the token and settles the matching delivery instead of only acknowledging it. SDK signatures, protobuf fields, persisted data, and orchestrator replay code are unchanged.
      • Migration guidance: Echo the exact delivered WorkItem.completionToken in abandonment requests. Use a replacement delivery's new token; do not reuse a settled token.

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: The two test-host Abandon handlers, focused abandonment direct-server/integration tests, removal of the obsolete acknowledgement-only test, and the test-host README.
  • What you changed after AI output: No separate human-authored revisions are claimed. During this rebase, the agent preserved Isolate in-process work-item completion by delivery token #815's foundation, removed superseded completion/token/send-result tests and positive partial-response tests, and labeled the first integration-test client accurately as protocol-level rather than an SDK worker.

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: Passed locally after rebase. With main's acknowledgement-only handlers, the two direct cancellation cases timed out at 5 seconds and both localhost redelivery cases timed out at 15 seconds, demonstrating the missing behavior. All four pass with the changed handlers.
  • Commands below were run in PowerShell; $evidence is the session-local artifact directory outside the repository.
Command Result
dotnet test test\InProcessTestHost.Tests\InProcessTestHost.Tests.csproj --configuration Release --filter "FullyQualifiedName~WorkItemCompletionTests|FullyQualifiedName~WorkerHistorySnapshotTests" --verbosity quiet --logger "trx;LogFileName=baseline-completion-history.trx" --results-directory "$evidence\baseline-completion-history" Baseline passed: 34/34 before replacing acknowledgement-only behavior.
dotnet test test\InProcessTestHost.Tests\InProcessTestHost.Tests.csproj --configuration Release --no-build --no-restore --filter "FullyQualifiedName~AbandonWorkItem_CancelsPendingExecutionAsync|FullyQualifiedName~WorkItemAbandonmentIntegrationTests" --verbosity normal --logger "trx;LogFileName=abandonment-red.trx" --results-directory "$evidence\abandonment-red" Expected RED: 4/4 failed on main's handlers because the host wait/redelivery remained pending.
dotnet test test\InProcessTestHost.Tests\InProcessTestHost.Tests.csproj --configuration Release --no-restore --filter "FullyQualifiedName~WorkItemCompletionTests|FullyQualifiedName~WorkItemAbandonment|FullyQualifiedName~WorkerHistorySnapshotTests" --verbosity quiet --logger "trx;LogFileName=focused-green.trx" --results-directory "$evidence\focused-green" Passed: 48/48, including both direct cancellation and both real localhost redelivery cases.
dotnet test test\InProcessTestHost.Tests\InProcessTestHost.Tests.csproj --configuration Release --no-build --no-restore --verbosity quiet --logger "trx;LogFileName=owning-suite.trx" --results-directory "$evidence\owning-suite" Passed: 112/112 in the owning suite.
dotnet test test\Grpc.IntegrationTests\Grpc.IntegrationTests.csproj --configuration Release --filter "FullyQualifiedName~AutochunkTests" --verbosity quiet --logger "trx;LogFileName=autochunk.trx" --results-directory "$evidence\autochunk" Passed: 4/4, including ChunkedResponse_FailsWithoutSchedulingFragmentActionsAsync; SDK chunk rejection remains terminal Failed with no fragment activities executed.
dotnet build src\InProcessTestHost\InProcessTestHost.csproj --configuration Release --no-restore --verbosity quiet Passed for net6.0, net8.0, and net10.0: 0 errors; 143 existing warnings across the build graph.
git diff --check origin/main...HEAD Passed.

Dependencies were restored only after the initial no-restore commands reported missing assets. Hosted CI results are not claimed. Full-solution tests, DTS emulator tests, and real Azure tests were not run locally.

Manual validation (only if runtime/behavior changed)

  • Environment (OS, .NET version, components): Windows; .NET SDK 10.0.401; localhost Kestrel/gRPC, the in-memory orchestration service, and an unchanged compatible version-2 SDK worker.
  • Steps + observed results: No separate human manual validation was performed. Automated behavior scenarios:
    1. A protocol-level test client receives an activity or orchestration delivery, stops fetching, and explicitly abandons it using that delivery's token.
    2. A compatible SDK worker completes the redelivery and an unrelated instance. Both cases assert one explicit abandonment, exactly two successful deliveries of the rejected work, distinct nonempty tokens, and completed outputs.
    3. Direct-server tests verify cancellation, invalid/wrong-kind/stale tokens, late completion, competing settlement, delayed old-send cleanup, snapshot release, and captured-reader/replacement safety. Existing completion, disconnected-stream, terminal chunk-failure, and history regressions remain passing.
  • Evidence (optional): Local console logs and TRX files retained outside the repository in the session artifact directory; no hosted CI result is claimed.

Notes for reviewers

Breaking Change

Type: behavioral (direct test-host abandonment request validation and settlement timing).

Impact: Both Abandon RPCs now require a nonempty token for an active delivery of the corresponding kind. Missing tokens return InvalidArgument; unknown, wrong-kind, or already settled tokens return NotFound. Invalid requests do not consume an active delivery. A valid abandonment cancels that delivery's host-side wait and enables retry. Successful completion and abandonment compete under the existing ownership lock; only one can settle the delivery.

Migration: Echo the received WorkItem.completionToken in abandonment requests, preserving its exact casing. A redelivery has a new token. Completion-token migration and unsupported-chunk handling are already documented in #815, not introduced here.

  • Cancellation means canceling the test host's wait, not stopping remote business code or calling orchestration Terminate.
  • Disconnecting GetWorkItems is not implicit abandonment. No timeout, heartbeat, lease, entity, production DTS, worker-policy, storage, or hub-lifecycle change is included.
  • Unsupported chunked completion still settles its validated delivery with one terminal Failed action and non-retriable NotSupportedException details while returning InvalidArgument. A subsequent SDK abandonment of that settled token returns NotFound and cannot undo that result or requeue it. Replay history streaming remains supported separately.
  • Runtime delta: only the two Abandon handler bodies (+24/-2 lines). The remaining changes are their tests/docs and removal of the obsolete acknowledgement-only regression.
  • Release-note, backport, human-review attestations, and hosted CI decisions remain for maintainer review.

Copilot AI balanced review requested due to automatic review settings October 9, 2026 14:47
Comment thread src/InProcessTestHost/Sidecar/Grpc/TaskHubGrpcServer.cs Fixed
Comment thread test/InProcessTestHost.Tests/WorkItemAbandonmentIntegrationTests.cs Fixed
Comment thread test/InProcessTestHost.Tests/WorkItemAbandonmentTests.cs Fixed
Comment thread test/InProcessTestHost.Tests/WorkItemAbandonmentTests.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.

🟡 Changes recommended

A delayed stream-write failure can override an already-acknowledged completion and incorrectly requeue completed work.

1 open finding
What changed in this PR

Adds receiver-side support for the explicit abandonment introduced by #467, using per-delivery completion tokens to prevent stale settlements.

Changes:

  • Cancels and requeues explicitly abandoned work.
  • Coordinates completion, partial responses, snapshots, and abandonment by delivery token.
  • Adds direct-server and localhost integration coverage plus documentation.
File Description
src/​InProcessTestHost/​Sidecar/​Grpc/​TaskHubGrpcServer.cs Implements token-based delivery ownership and abandonment.
src/​InProcessTestHost/​README.md Documents abandonment semantics and migration requirements.
test/​InProcessTestHost.Tests/​WorkItemAbandonmentTests.cs Tests settlement, concurrency, and cleanup behavior.
test/​InProcessTestHost.Tests/​WorkItemAbandonmentIntegrationTests.cs Tests end-to-end redelivery through gRPC.
test/​InProcessTestHost.Tests/​WorkerHistorySnapshotTests.cs Updates snapshot tests for completion tokens.

🧠 Review effort: Balanced


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

Comment thread src/InProcessTestHost/Sidecar/Grpc/TaskHubGrpcServer.cs
Copilot AI balanced review requested due to automatic review settings October 9, 2026 21:11

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

The behavioral breaking change is documented with migration guidance and comprehensively tested.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 13, 2026 19:13

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.

🔵 Needs a closer look

The concurrency-sensitive behavioral breaking change to direct gRPC request acceptance warrants final maintainer review.

0 open findings

🧠 Review effort: Balanced

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

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.

🔵 Needs a closer look

The concurrency-sensitive behavioral breaking change warrants final human review despite comprehensive tests.

0 open findings

🧠 Review effort: Balanced

Dispose the compatible SDK host even if asynchronous stopping fails. Keep both existing cancellation sources alive for the full server session and dispose them through cleanup-scope aliases, following the completion-test pattern.

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

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

The implementation shares the established delivery-ownership lock and has comprehensive focused and integration coverage.

0 open findings

🧠 Review effort: Balanced

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