Release abandoned orchestrator tasks when an executor is retired - #1390
Release abandoned orchestrator tasks when an executor is retired#1390wangbill (YunchuWang) wants to merge 3 commits into
Conversation
TaskOrchestrationContext creates a TaskCompletionSource<string> per activity, sub-orchestration and timer, and the orchestrator awaits it. When an episode ends while the orchestrator is parked on one of those awaits, the TCS is deliberately abandoned in a pending state -- that is inherent to the replay model. When a debugger is attached, TaskAwaiter.OnCompletedInternal calls OutputWaitEtwEvents, which calls Task.AddToActiveTasks because Task.s_asyncDebuggingEnabled is true. That inserts the task into the process-wide, strongly-referenced static dictionary Task.s_currentActiveTasks. The matching Task.RemoveFromActiveTasks only runs inside the awaiter continuation wrapper, i.e. only when the awaited task completes. Abandoned tasks are therefore rooted forever, and each rooted Task pins its continuation -> async state machine -> TaskOrchestrationContext -> history/inputs/outputs. Only the AddToActiveTasks branch roots the task, which is why the leak is debugger-specific. Fix: cancel the open tasks once the executor is guaranteed never to run again, so the abandoned awaiter continuations run and unregister themselves. - TaskOrchestrationContext.ReleaseOpenTasks() cancels every open task. It is idempotent, sets isReleased before cancelling, and snapshots openTasks before iterating because resumed user code can mutate the dictionary. A ThrowIfReleased guard on ScheduleTaskInternal, CreateSubOrchestrationInstanceCore and CreateTimer stops orchestrator code that swallows the cancellation from scheduling new work. - TaskOrchestrationExecutor implements IDisposable. Dispose() restores the orchestrator ambient environment, releases the open tasks, and observes a faulted result so nothing surfaces as an UnobservedTaskException. - TaskOrchestrationDispatcher owns executor lifetime via workItem.Cursor, so it retires the executor from a new finally on OnProcessWorkItemSessionAsync and at the continue-as-new site. Extended sessions are unaffected: release only happens when the executor is retired, never between episodes. ExtendedSession_OpenTasksSurviveBetweenEpisodes covers this. Ref Azure/azure-functions-durable-extension#340 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses a debugger-specific memory leak in DurableTask.Core by ensuring that “abandoned” orchestrator awaits (open activity/sub-orchestration/timer tasks) are released when an orchestration executor is retired, allowing CLR async-debug bookkeeping (Task.s_currentActiveTasks) to unregister those tasks.
Changes:
- Add
TaskOrchestrationContext.ReleaseOpenTasks()and a released-context guard to prevent scheduling new work after release. - Make
TaskOrchestrationExecutorimplementIDisposableand release open tasks on disposal while restoring orchestrator ambient execution context. - Ensure
TaskOrchestrationDispatcherdisposes retired executors via a centralizedReleaseCursor(...)helper.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Test/DurableTask.Core.Tests/TaskOrchestrationExecutorTests.cs | Adds regression tests for executor retirement/leak behavior, but currently placed in the legacy Test/ tree (not in the active test project). |
| src/DurableTask.Core/TaskOrchestrationExecutor.cs | Implements IDisposable to release abandoned orchestrator continuations when the executor is retired. |
| src/DurableTask.Core/TaskOrchestrationDispatcher.cs | Disposes retired executors in finally at session end and when continuing-as-new, via ReleaseCursor. |
| src/DurableTask.Core/TaskOrchestrationContext.cs | Adds open-task cancellation (ReleaseOpenTasks) and prevents scheduling new open tasks after release (ThrowIfReleased). |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| namespace DurableTask.Core.Tests | ||
| { | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Reflection; | ||
| using System.Threading.Tasks; |
TaskOrchestrationExecutor is a public type, so implementing IDisposable added public API surface that no caller outside DurableTask.Core can use correctly: executor lifetime is owned by TaskOrchestrationDispatcher, which is the only component that knows when an executor will never run again. It also risked tripping CA2000 in downstream repos that construct an executor directly. Replace the public Dispose() with an internal Release() and have ReleaseCursor call that instead. The cleanup behavior is unchanged: install the orchestrator ambient context, call context.ReleaseOpenTasks(), restore ambient state in a finally, and observe the top-level task exception so it cannot surface as an UnobservedTaskException. Cursor clearing and all lifecycle placement in the dispatcher are untouched. Tests call Release() explicitly rather than relying on a using block, and are renamed Dispose_* -> Release_*. All six behavioral guarantees are preserved, including ExtendedSession_OpenTasksSurviveBetweenEpisodes. The public surface of TaskOrchestrationExecutor is now exactly what it was before this PR: three constructors, Execute, ExecuteNewEvents, and IsCompleted. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 019e7654-1037-4e02-b2fd-c03e09d6b7e2
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Test/DurableTask.Core.Tests/TaskOrchestrationExecutorTests.cs:18
- This test file is being added under
Test/(capital T), but the solution references the active Core test project attest/DurableTask.Core.Tests/DurableTask.Core.Tests.csproj. As a result, this file will not be compiled or executed by the test project, so the new regression coverage won’t actually run in CI.
namespace DurableTask.Core.Tests
{
using System;
using System.Collections.Generic;
using System.Linq;
| /// This is intentionally internal rather than an <see cref="IDisposable"/> implementation. | ||
| /// Executor lifetime is owned by <see cref="TaskOrchestrationDispatcher"/>, which is the only | ||
| /// component that knows when an executor will never run again, so exposing this externally would | ||
| /// add public surface that no caller outside this assembly can use correctly. | ||
| /// </para> |
Cancelling the abandoned orchestrator tasks is what lets the CLR drop its
active-task roots, but it is not side-effect free: resuming an abandoned await
runs the orchestrator's catch and finally blocks. There is no supported way to
remove those roots without running the continuations, so paying that cost
unconditionally changes production behavior for a leak that only exists when a
debugger is attached.
Scope the teardown to the case that needs it:
- TaskOrchestrationDispatcher.ReleaseCursor always clears the cursor, but now
only calls TaskOrchestrationExecutor.Release() when Debugger.IsAttached. The
gate lives in the dispatcher because it owns executor lifetime, and it is
lifted into an internal overload that takes the flag so both branches are
unit-testable without an attached debugger. Both retirement sites (the outer
session finally and continue-as-new) keep clearing the cursor unconditionally.
- TaskOrchestrationContext.ThrowIfReleased now throws InvalidOperationException
instead of OperationCanceledException. The first failure at the abandoned
await necessarily remains an OperationCanceledException, but a retired
executor is permanent, so the very common
'catch (OperationCanceledException) { retry; }' shape must not treat it as a
transient cancellation and spin.
Without a debugger the dispatcher behaves exactly as it did before the leak fix:
the executor is dropped and collected, and no orchestrator continuation runs.
See dotnet/runtime#26565 for the upstream proposal to make the active-task table
weak, which would make this teardown unnecessary.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 019e7654-1037-4e02-b2fd-c03e09d6b7e2
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Test/DurableTask.Core.Tests/TaskOrchestrationExecutorTests.cs:18
- This new test file is under
Test/(capital T), but the active test project istest/DurableTask.Core.Tests/DurableTask.Core.Tests.csprojand only compiles sources under that directory. As a result, these tests won't run in CI, so the new Release/ReleaseCursor behavior effectively has no coverage. Please move this file intotest/DurableTask.Core.Tests/(or otherwise include it in the active test .csproj).
namespace DurableTask.Core.Tests
{
using System;
using System.Collections.Generic;
using System.Linq;
|
Closing in favor of documentation. The proposed cleanup must run abandoned orchestrator continuations to release CLR debugger task roots, which changes user-code behavior while debugging. The underlying CLR weak-reference fix remains tracked by dotnet/runtime#26565. |
Problem
TaskOrchestrationContextcreates aTaskCompletionSource<string>for every activity, sub-orchestration, and timer, registers it inopenTasks, and awaitstcs.Task. When an episode ends while the orchestrator is parked on one of those awaits, the TCS is deliberately abandoned in a pending state — that is inherent to the replay model.When a debugger is attached,
TaskAwaiter.OnCompletedInternalcallsOutputWaitEtwEvents, which callsTask.AddToActiveTasks(task)becauseTask.s_asyncDebuggingEnabledis true. That inserts the task into the process-wide, strongly-referenced static dictionaryTask.s_currentActiveTasks. The matchingTask.RemoveFromActiveTasksonly runs inside the awaiter continuation wrapper, i.e. only when the awaited task completes.Abandoned tasks are therefore rooted forever, and each rooted
Taskpins its continuation → async state machine →TaskOrchestrationContext→ history, inputs, and outputs. Only theAddToActiveTasksbranch roots the task (the ETW branch does not), which is exactly why the leak is debugger-specific.Fixes the root cause behind Azure/azure-functions-durable-extension#340 ("Memory leak when debugger attached"), which is filed on the extension repo but labeled
dtfx/externalbecause the defect lives here in DurableTask.Core.The fix
TaskOrchestrationContextgainsinternal void ReleaseOpenTasks(), which cancels every open task so the abandoned awaiter continuations run and unregister themselves. It is idempotent, setsisReleased = truebefore cancelling, and snapshotsopenTasks.Values.ToList()before iterating, because resumed user code can mutate the dictionary (concretely,CreateTimer's cancellation-token callback callsopenTasks.Remove). A new privateThrowIfReleased()guard runs at the top ofScheduleTaskInternal,CreateSubOrchestrationInstanceCore, andCreateTimerso orchestrator code that swallows the cancellation cannot schedule new work that would leak the same way.TaskOrchestrationExecutorgainsinternal void Release(). It restores the orchestrator ambient environment (TaskOrchestrationSynchronizationContext+OrchestrationContext.IsOrchestratorThread) because cancelling resumes orchestrator code synchronously, callscontext.ReleaseOpenTasks(), and observesthis.result.Exceptionif faulted so nothing surfaces as anUnobservedTaskException.TaskOrchestrationDispatcherowns executor lifetime viaworkItem.Cursor, so it is the only component that knows when an executor will never run again. A newReleaseCursorhelper is called from a newfinallyon the outertryofOnProcessWorkItemSessionAsync(covering both the legacySession == nullpath and the session path —workItem.Cursoris only ever assigned inOnProcessWorkItemAsync, which both paths call), and it replaces the bareworkItem.Cursor = null;at the continue-as-new site.Debugger.IsAttachedThis is the part reviewers should look at hardest, and it is a deliberate trade-off rather than a free win.
Cancelling the abandoned tasks is what makes the CLR drop its roots — the entry is only removed when the awaiter continuation runs. There is no supported way to remove a
s_currentActiveTasksroot without running the async state-machine continuation. Running that continuation means the orchestrator'scatchandfinallyblocks execute. So an unconditional release would change user-visible behavior in production to fix a bug that only exists under a debugger.ReleaseCursortherefore always clearsworkItem.Cursor, but only callsTaskOrchestrationExecutor.Release()whenSystem.Diagnostics.Debugger.IsAttached:The gate lives in the dispatcher because the dispatcher owns executor lifetime, and it is lifted into an internal overload taking the flag so both branches are unit-tested without needing an attached debugger in CI. No public API, no private-CLR reflection.
To be candid about what this does and does not claim:
Debugger.IsAttachedcheck per retired cursor.OperationCanceledException, so orchestratorcatch/finallyblocks run during teardown. That is unavoidable with the publicTaskAPI. The compromise is now confined to the dev/debug session where the leak actually occurs.AppContextswitch convention in this repository, so this PR deliberately does not invent a settings surface for a single debug-only branch.Retirement is reported as
InvalidOperationException, not cancellationThrowIfReleased()throwsInvalidOperationException. The first failure at the abandoned await necessarily remains anOperationCanceledException— that is simply what a cancelled TCS raises. But a retired executor is permanent, so the extremely commonshape must not treat it as a transient cancellation and spin. With
InvalidOperationException, such a loop terminates on the retry.A
catch (Exception)infinite loop remains fundamentally impossible to prevent with any managed exception type — but it is now restricted to debugger teardown, and the guard still stops any new work from being scheduled.Evidence
Direct-executor harness, 2000 episodes × fan-out 100, debugger simulated by setting
Task.s_asyncDebuggingEnabled:Task.s_currentActiveTasksWith no debugger, both before and after are 0 — confirming the debugger-specific mechanism.
End-to-end through the real
TaskHubWorker/TaskOrchestrationDispatcherwithLocalOrchestrationService, 200 instances × 3 activities:TaskOrchestration'4.<Execute>, the user orchestrator's<RunTask>,ScheduleTask, bothScheduleTaskToWorkeroverloads,ScheduleTaskInternal, plus ~600 TCS tasks. Exactly 3 abandoned awaits per instance.(A residual ~1,200
Task.Delaypromises appear identically in both runs; that is the emulator's own queue polling, unrelated to this change.)Public API
No public API change.
TaskOrchestrationExecutordoes not implementIDisposableand exposes no new members; the cleanup hook isinternal void Release(), and the dispatcher gate isinternal static void ReleaseCursor(ref OrchestrationExecutionCursor?, bool). Verified by reflecting over the builtDurableTask.Core.dllrather than by reading source:Executor lifetime is owned by the dispatcher, which is the only component that knows when an executor will never run again, so there is nothing a caller outside this assembly could do with a public hook except use it incorrectly.
Extended sessions are explicitly unaffected
Release happens only at permanent executor retirement, never between episodes. Cancelling at the end of every episode would break extended sessions:
HandleTaskCompletedEventcallsinfo.Result.SetResult(...), which throwsInvalidOperationExceptionon an already-cancelled TCS. There is a regression test,ExtendedSession_OpenTasksSurviveBetweenEpisodes, proving that an open task still survives across episodes and is resumed by its result rather than by cancellation.Alternatives rejected
ExecuteCore— breaks extended sessions, as above.Task.WhenAll, awaiting the returnedTask<T>) still register.s_currentActiveTasks— private API, and it cannot reach the intermediate async-method tasks.Tests
New file
Test/DurableTask.Core.Tests/TaskOrchestrationExecutorTests.cs, 8 tests:Release_ResumesAbandonedOrchestratorContinuationsRelease_WithAsyncDebuggingEnabled_DoesNotLeakActiveTasksRelease_DoesNotChangeTheDecisionsAlreadyProducedRelease_OrchestratorThatSwallowsCancellation_CannotScheduleMoreWorkOperationCanceledException; the retry faults withInvalidOperationException, so cancellation-specific retry loops terminateRelease_IsIdempotentReleaseCursor_WithTeardownEnabled_ClearsCursorAndResumesContinuationsReleaseCursor_WithTeardownDisabled_ClearsCursorWithoutRunningContinuationsExtendedSession_OpenTasksSurviveBetweenEpisodesNote on
Release_WithAsyncDebuggingEnabled_DoesNotLeakActiveTasks: it deliberately measures howTask.s_currentActiveTasksgrowth scales with fan-out (1 vs 25 abandoned awaits over 20 episodes) rather than asserting an absolute count. The dictionary is process-wide and the test host's own async plumbing pollutes it, and on .NET Framework an async method's task is not anAsyncStateMachineBox<T>, so entries cannot be attributed by type there. The test is self-validating: it first asserts that unreleased executors DO leak proportionally, so it can never silently stop detecting the regression.Results (Debug — test projects only build in Debug, since
SIGN_ASSEMBLYdisablesInternalsVisibleTo)TaskOrchestrationExecutorTests: 8/8 net8.0, 8/8 net48DurableTask.Core.Tests: 147/147 net8.0; net48 128 passed / 7 failedDurableTask.Emulator.Tests: 5/5 net8.0, 5/5 net48The 7 net48 failures are all in
ContinueAsNewTraceBehaviorTestsand are pre-existing onmain, verified by stashing this change and re-running the same test class at baseline — identical 7 failures. No new failures.Known follow-up (not fixed here)
azure-functions-durable-extensionhas its own secondary TCS leaks of the same shape that this change does not cover, because an orchestrator parked purely on an external event has no DTFx open tasks:DurableOrchestrationContext.pendingExternalEvents(EventTaskCompletionSource<T>)TaskCommonShim.timeoutTaskCompletionSource