Skip to content

Release cancellation registrations after external-event waits complete - #838

Merged
wangbill (YunchuWang) merged 3 commits into
mainfrom
yunchuwang-external-event-registration-cleanup
Nov 6, 2026
Merged

wangbill (YunchuWang) merged 3 commits into
mainfrom
yunchuwang-external-event-registration-cleanup

Conversation

@YunchuWang

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

Copy link
Copy Markdown
Member

Summary

What changed?

  • Dispose pending external-event cancellation registrations when their original task completes, using ExecuteSynchronously and TaskScheduler.Default; skip registration for noncancellable or already-completed buffered waits.
  • Retain six added functional cases for pre-cancellation/token identity, buffered-event precedence, synchronous/reentrant cancellation, LIFO ordering, and cancellation/completion races.
  • In follow-up commit 98849443d07a53c4bd17dfdd93723bc22b49ae16, remove both GC-dependent collectability cases and their unused helper/import. No allocation-pressure, timing, private-CTS inspection, or test-only production replacement was added.
  • The production fix and src/Worker/Core/RELEASENOTES.md are unchanged by this revision. No public API, protocol, dependency, or event-dispatch changes.

Why is this change needed?

  • WaitForExternalEvent<T> discarded its CancellationTokenRegistration; the callback captured the completion source and could retain a completed task and payload while the token source remained live.
  • Historical investigation on pre-fix main retained 1,000 completed tasks with a live shared token source, versus zero in noncancellable/disposed-source controls. That GC-based reproduction is no longer in the regression suite. This is not production telemetry or evidence of cross-invocation leakage.

Issues / work items

  • Resolves: N/A; no existing issue is claimed.
  • Related #801 (canceled-wait event preservation) and #509 (LIFO ordering); both behaviors remain intact.

Project checklist

  • Release notes are not required for the next release
    • Otherwise: Notes added to src/Worker/Core/RELEASENOTES.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: N/A; original returned task, cancellation token, synchronous completion, LIFO ordering, and buffering semantics are preserved.
      • Migration guidance: N/A.

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 coding agent.
  • AI-assisted areas/files: src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs, test/Worker/Core.Tests/Shims/TaskOrchestrationContextWrapperTests.cs, and src/Worker/Core/RELEASENOTES.md.
  • What you changed after AI output: User-directed removal of the forced-GC tests and helper; functional tests and production code were preserved. No human edits or verification 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 attestations are left for a human reviewer; the automated evidence below is agent-run.


Testing

Automated tests

  • Result: Passed for the revised source; wrapper count is now 28, not 30.
# Passed 28/28 (Debug)
dotnet test test\Worker\Core.Tests\Worker.Tests.csproj --no-restore --filter FullyQualifiedName~TaskOrchestrationContextWrapperTests --verbosity minimal

# Passed 28/28 (Release)
dotnet test test\Worker\Core.Tests\Worker.Tests.csproj --configuration Release --no-restore --filter FullyQualifiedName~TaskOrchestrationContextWrapperTests --verbosity minimal

# Passed 3/3 existing public event/timer cases
dotnet test test\InProcessTestHost.Tests\InProcessTestHost.Tests.csproj --configuration Release --no-restore --filter FullyQualifiedName~ExternalEventTests --verbosity minimal

# Passed 8/8 existing gRPC stack/buffering/timeout/cancellation cases
dotnet test test\Grpc.IntegrationTests\Grpc.IntegrationTests.csproj --configuration Release --no-restore --filter "FullyQualifiedName~ExternalEventStackTests|FullyQualifiedName~WaitForExternalEvent_WithTimeoutAndCancellationToken" --verbosity minimal

# Passed scoped formatting check
dotnet format style test\Worker\Core.Tests\Worker.Tests.csproj --no-restore --include test\Worker\Core.Tests\Shims\TaskOrchestrationContextWrapperTests.cs --diagnostics IDE0055 --verify-no-changes --verbosity minimal

# Passed whitespace check before commit
git --no-pager diff --check

The retained race test performs 100 cancellation/completion races per run and passed in both wrapper configurations. A source scan found no forced-GC calls, weak-reference machinery, deleted helper/import, or timing-based substitute. Production code and release notes match the prior commit exactly. Existing unrelated analyzer warnings remain; original file endings are preserved because the repository's newline rules conflict.

The full solution, other runtimes, and live Azure/DTS deployments were not run for this revision. Remote CI and human verification are not asserted here.

Manual validation (only if runtime/behavior changed)

  • Environment (OS, .NET version, components): Windows, .NET SDK 10.0.403; net10.0 test hosts, netstandard2.0 worker, existing in-process/public gRPC paths.
  • Steps + observed results:
    1. Removed only the GC-dependent theory, helper, and import: one test file, 63 deletions.
    2. Reran the scoped suites above; functional cancellation/delivery cases remain unchanged and pass.
    3. Confirmed no production or release-note change in the follow-up commit.
  • Evidence (optional): Agent-run automated checks only; no human manual or production validation is claimed.

Notes for reviewers

  • Coverage tradeoff: functional cancellation/delivery regressions remain, but the suite no longer directly asserts GC-based collectability. No flaky replacement is introduced.
  • The event-source stack and CompleteExternalEvent are untouched. Buffered events still win over already-canceled tokens, and canceled waiters do not consume events.
  • The cleanup continuation only disposes the registration; it does not replace the returned task or schedule orchestrator logic. It handles inline cancellation or completion before attachment. Pending cancellable waits incur one continuation and boxed registration.
  • The branch remains independent and based directly on main; no merge is requested.

Dispose registrations when their external-event tasks settle while preserving the original task, synchronous completion, waiter ordering, and canceled-event buffering. Add bounded retention controls, cancellation race coverage, and a worker package release note.

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

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 were identified for the cancellation-registration cleanup for completed external-event waits.

No open issues 🎉

What changed in this PR

This PR fixes completed external-event wait retention in the shared Durable Task worker core by releasing cancellation registrations while their token sources remain live. It preserves the original returned task, LIFO event delivery, and canceled-wait event preservation.

Changes:

  • Dispose cancellation registrations on task completion without depending on the orchestrator scheduler; skip registration for noncancellable tokens and already-completed waits.
  • Add eight regression cases covering task/payload retention, cancellation, synchronous continuations, ordering, and completion races.
  • Document the cleanup in the worker release notes.
File Description
test/​Worker/​Core.Tests/​Shims/​TaskOrchestrationContextWrapperTests.cs Adds retention and event-wait behavior regression tests.
src/​Worker/​Core/​Shims/​TaskOrchestrationContextWrapper.cs Releases completed waits’ cancellation registrations and avoids unnecessary registrations.
src/​Worker/​Core/​RELEASENOTES.md Documents the cancellation-registration retention fix.

Review effort: Balanced

Remove the collectability theory and its unused helper/import in response to review feedback. Preserve the production registration cleanup and all functional cancellation, buffered-event, synchronous/reentrant, LIFO, and race regressions unchanged.

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

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 cancellation-registration retention fix in external-event waits.

No open issues 🎉

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 6, 2026 01: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 cancellation-registration retention fix has no active review issues.

No open issues 🎉

Review effort: Balanced

@YunchuWang
wangbill (YunchuWang) merged commit ae1464c into main Nov 6, 2026
11 checks passed
@YunchuWang
wangbill (YunchuWang) deleted the yunchuwang-external-event-registration-cleanup branch November 6, 2026 14:45
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.

5 participants