Managed fixes for async task tracking

[rcj1]

This PR includes a few additions to the runtime-async handling loop to facilitate inspection by a debugger - specifically, Visual Studio.

1. Timestamp tracking for continuations. In VS there is a feature that lets you see, while F5 debugging, the relative start times and durations of different tasks. There is only one actual Task at the root of an await chain, but we intend to replicate the outward behavior in runtime-async by treating Continuations as "virtual Tasks". As such, we populate a dictionary to track the start times for each logical invocation.

2. TPL events. In Concord in asyncv1 we are notified of Task status changes via TPL events; specifically, we use these events to detect which Tasks are currently active and which thread each Task runs on. There are ways to do this without TPL in Concord; however, I believe a change to this would be across different (non-runtime async) Tasks and beyond the scope of this PR.

[Copilot]

Pull request overview

This PR adds managed fixes for async task tracking by introducing tick count tracking for runtime async tasks and continuations to support debugging scenarios.

Changes:

• Adds two new ConcurrentDictionary fields to track task and continuation tick counts for debugging
• Implements methods to set, get, update, and remove tick count tracking for both tasks and continuations
• Integrates tick count tracking into the continuation allocation and dispatch flow
• Adds TPL event source tracing for synchronous work begin/end and operation begin/end events

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs	Adds static dictionaries and helper methods for tracking task and continuation tick counts, removes unnecessary blank line
src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.CoreCLR.cs	Integrates tick count tracking into continuation lifecycle, adds ContinuationEqualityComparer, extends AsyncDispatcherInfo with Task field, adds TPL event source tracing

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[stephentoub]

🤖 Copilot Code Review — PR #123727

Holistic Assessment

Motivation: The PR adds debugger infrastructure for Visual Studio to track runtime-async task timing/duration and integrates TPL event source tracing. This is needed for VS debugging features that show relative start times and durations of async operations. The motivation is sound and the feature is valuable for debugging experience.

Approach: The implementation mirrors the existing s_currentActiveTasks pattern by introducing two new static dictionaries (s_runtimeAsyncTaskTimestamps and s_runtimeAsyncContinuationTimestamps) with lock-based synchronization. It also adds TPL event logging at key lifecycle points. The approach is consistent with how the existing debugger support works in Task.cs.

Summary: ⚠️ Needs Human Review. The code follows existing patterns and appears correct, but there are several concerns that warrant human attention: (1) potential memory leak on exception paths for continuation timestamps, (2) whether the return → break change affects TPL event ordering, and (3) test coverage relies on internal implementation details. The PR has been actively reviewed by @jkotas and appears close to ready.

---

Detailed Findings

⚠️ Memory — Potential leak of continuation timestamp on exception path

In DispatchContinuations(), when an exception is thrown during curContinuation.ResumeInfo->Resume(...), the code jumps to the catch block before RemoveRuntimeAsyncContinuationTimestamp(curContinuation) is called:

// Line ~502-520 in DispatchContinuations
long timestamp = 0;
if (Task.s_asyncDebuggingEnabled)
{
    timestamp = Task.GetRuntimeAsyncContinuationTimestamp(curContinuation, out long timestampVal) ? timestampVal : Stopwatch.GetTimestamp();
    Task.UpdateRuntimeAsyncTaskTimestamp(this, timestamp);
}
Continuation? newContinuation = curContinuation.ResumeInfo->Resume(curContinuation, ref resultLoc);  // <-- exception here

if (Task.s_asyncDebuggingEnabled)
{
    Task.RemoveRuntimeAsyncContinuationTimestamp(curContinuation);  // <-- skipped on exception
}

The timestamp for curContinuation gets added but never removed if Resume throws. While this only affects debugging scenarios and UnwindToPossibleHandler does call RemoveRuntimeAsyncContinuationTimestamp as it walks the chain, there may be edge cases where entries persist.

Note: This was also flagged by the automated Copilot PR reviewer but marked as "low confidence". The exception path does have cleanup logic in UnwindToPossibleHandler, so this may be intentional or a non-issue in practice. Worth verifying the lifecycle is correct.

⚠️ Ordering — TPL event ordering after return → break change

The return statements were changed to break statements to enable TraceSynchronousWorkEnd to be called at the end:

// After the while(true) loop
if (isTplEnabled)
{
    TplEventSource.Log.TraceSynchronousWorkEnd(CausalitySynchronousWork.Execution);
}

However, TraceSynchronousWorkEnd is now called after TraceOperationEnd in the exception and completion paths. The original design appears to have been that each exit path would directly return. Verify with the TPL event consumer (VS debugger/diagnostics team) that this event ordering is acceptable.

✅ Pattern Consistency — Dictionary initialization pattern

The new timestamp dictionary methods follow the exact same pattern as the existing AddToActiveTasks/RemoveFromActiveTasks:

Dictionary<object, long> continuationTimestamps =
    Volatile.Read(ref s_runtimeAsyncContinuationTimestamps) ??
    Interlocked.CompareExchange(ref s_runtimeAsyncContinuationTimestamps, new Dictionary<object, long>(ReferenceEqualityComparer.Instance), null) ??
    s_runtimeAsyncContinuationTimestamps;

This is the established pattern in Task.cs for lazy dictionary initialization. Good.

✅ Thread Safety — Lock-based synchronization is consistent

The use of lock(dictionary) for all operations matches the existing s_currentActiveTasks pattern. Given this is only enabled when s_asyncDebuggingEnabled is true (debugger-controlled), the locking overhead is acceptable.

✅ Correctness — AsyncDispatcherInfo.Task field initialization

The diff shows asyncDispatcherInfo.Task = this; is added in DispatchContinuations(), correctly initializing the new field.

💡 Style — NotifyDebuggerOfRuntimeAsyncState empty method

The empty method with [MethodImpl(MethodImplOptions.NoOptimization)] is a common pattern for debugger integration (a "breakpoint target"). The pragma suppressions are appropriate. However, consider adding a brief comment explaining this is a debugger integration point:

// Provides a stable point for debuggers to inspect runtime async state.
[MethodImpl(MethodImplOptions.NoOptimization)]
public void NotifyDebuggerOfRuntimeAsyncState()
{
}

💡 Test — Relies on reflection into private fields

The test uses reflection to access internal implementation details:

typeof(Task).GetField("s_asyncDebuggingEnabled", BindingFlags.NonPublic | BindingFlags.Static).SetValue(null, true);
// ...
int taskCount = ((Dictionary<int, long>)typeof(Task).GetField("s_runtimeAsyncTaskTimestamps", BindingFlags.NonPublic | BindingFlags.Static).GetValue(null)).Count;

This is fragile and will break if the implementation changes. However, given this is testing debugger-only infrastructure with no public API surface, this approach may be acceptable. The test correctly uses RemoteExecutor to isolate the debugger-enabled state.

✅ Platform Handling — #if !MONO guards

The new timestamp dictionaries and methods are correctly guarded with #if !MONO since runtime-async is a CoreCLR feature. The test file is also correctly excluded from Mono builds via the csproj condition.

💡 Comment Typo — "Debuggger"

There's a typo in the Task constructor comments:

_ = s_asyncDebuggingEnabled; // Debuggger depends on Task cctor being run when any Task gets created

Should be "Debugger".

---

Cross-cutting Analysis

Sibling types: The changes are isolated to the runtime-async infrastructure. The existing TPL task debugging (s_currentActiveTasks, AddToActiveTasks, RemoveFromActiveTasks) is untouched and the new code follows the same patterns.

Callers: The new timestamp methods are only called from AsyncHelpers.CoreCLR.cs and are guarded by Task.s_asyncDebuggingEnabled, which is set by the debugger. Normal runtime paths are unaffected.

---

Summary

The PR is well-structured and follows existing patterns. The main concerns are:

1. Verify continuation timestamp cleanup on exception paths is complete
2. Confirm TPL event ordering change is acceptable
3. Minor typo fix

Assuming #1 and #2 are verified, the PR appears ready to merge.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

[rcj1]

@333fred Is this a known Roslyn issue?

No, please file a bug.

I don’t believe this is an action item, as the failing legs are mono

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

[rcj1]

🤖 Copilot Code Review — PR #123727

Holistic Assessment

Motivation: The PR adds debugger infrastructure for Visual Studio to track runtime-async task timing/duration and integrates TPL event source tracing. This is needed for VS debugging features that show relative start times and durations of async operations. The motivation is sound and the feature is valuable for debugging experience.

Approach: The implementation mirrors the existing s_currentActiveTasks pattern by introducing two new static dictionaries (s_runtimeAsyncTaskTimestamps and s_runtimeAsyncContinuationTimestamps) with lock-based synchronization. It also adds TPL event logging at key lifecycle points. The approach is consistent with how the existing debugger support works in Task.cs.

Summary: ⚠️ Needs Human Review. The code follows existing patterns and appears correct, but there are several concerns that warrant human attention: (1) potential memory leak on exception paths for continuation timestamps, (2) whether the return → break change affects TPL event ordering, and (3) test coverage relies on internal implementation details. The PR has been actively reviewed by @jkotas and appears close to ready.

Detailed Findings

⚠️ Memory — Potential leak of continuation timestamp on exception path

In DispatchContinuations(), when an exception is thrown during curContinuation.ResumeInfo->Resume(...), the code jumps to the catch block before RemoveRuntimeAsyncContinuationTimestamp(curContinuation) is called:

// Line ~502-520 in DispatchContinuations
long timestamp = 0;
if (Task.s_asyncDebuggingEnabled)
{
    timestamp = Task.GetRuntimeAsyncContinuationTimestamp(curContinuation, out long timestampVal) ? timestampVal : Stopwatch.GetTimestamp();
    Task.UpdateRuntimeAsyncTaskTimestamp(this, timestamp);
}
Continuation? newContinuation = curContinuation.ResumeInfo->Resume(curContinuation, ref resultLoc);  // <-- exception here

if (Task.s_asyncDebuggingEnabled)
{
    Task.RemoveRuntimeAsyncContinuationTimestamp(curContinuation);  // <-- skipped on exception
}

The timestamp for curContinuation gets added but never removed if Resume throws. While this only affects debugging scenarios and UnwindToPossibleHandler does call RemoveRuntimeAsyncContinuationTimestamp as it walks the chain, there may be edge cases where entries persist.

Note: This was also flagged by the automated Copilot PR reviewer but marked as "low confidence". The exception path does have cleanup logic in UnwindToPossibleHandler, so this may be intentional or a non-issue in practice. Worth verifying the lifecycle is correct.

⚠️ Ordering — TPL event ordering after return → break change

The return statements were changed to break statements to enable TraceSynchronousWorkEnd to be called at the end:

// After the while(true) loop
if (isTplEnabled)
{
    TplEventSource.Log.TraceSynchronousWorkEnd(CausalitySynchronousWork.Execution);
}

However, TraceSynchronousWorkEnd is now called after TraceOperationEnd in the exception and completion paths. The original design appears to have been that each exit path would directly return. Verify with the TPL event consumer (VS debugger/diagnostics team) that this event ordering is acceptable.

✅ Pattern Consistency — Dictionary initialization pattern

The new timestamp dictionary methods follow the exact same pattern as the existing AddToActiveTasks/RemoveFromActiveTasks:

Dictionary<object, long> continuationTimestamps =
    Volatile.Read(ref s_runtimeAsyncContinuationTimestamps) ??
    Interlocked.CompareExchange(ref s_runtimeAsyncContinuationTimestamps, new Dictionary<object, long>(ReferenceEqualityComparer.Instance), null) ??
    s_runtimeAsyncContinuationTimestamps;

This is the established pattern in Task.cs for lazy dictionary initialization. Good.

✅ Thread Safety — Lock-based synchronization is consistent

The use of lock(dictionary) for all operations matches the existing s_currentActiveTasks pattern. Given this is only enabled when s_asyncDebuggingEnabled is true (debugger-controlled), the locking overhead is acceptable.

✅ Correctness — AsyncDispatcherInfo.Task field initialization

The diff shows asyncDispatcherInfo.Task = this; is added in DispatchContinuations(), correctly initializing the new field.

💡 Style — NotifyDebuggerOfRuntimeAsyncState empty method

The empty method with [MethodImpl(MethodImplOptions.NoOptimization)] is a common pattern for debugger integration (a "breakpoint target"). The pragma suppressions are appropriate. However, consider adding a brief comment explaining this is a debugger integration point:

// Provides a stable point for debuggers to inspect runtime async state.
[MethodImpl(MethodImplOptions.NoOptimization)]
public void NotifyDebuggerOfRuntimeAsyncState()
{
}

💡 Test — Relies on reflection into private fields

The test uses reflection to access internal implementation details:

typeof(Task).GetField("s_asyncDebuggingEnabled", BindingFlags.NonPublic | BindingFlags.Static).SetValue(null, true);
// ...
int taskCount = ((Dictionary<int, long>)typeof(Task).GetField("s_runtimeAsyncTaskTimestamps", BindingFlags.NonPublic | BindingFlags.Static).GetValue(null)).Count;

This is fragile and will break if the implementation changes. However, given this is testing debugger-only infrastructure with no public API surface, this approach may be acceptable. The test correctly uses RemoteExecutor to isolate the debugger-enabled state.

✅ Platform Handling — #if !MONO guards

The new timestamp dictionaries and methods are correctly guarded with #if !MONO since runtime-async is a CoreCLR feature. The test file is also correctly excluded from Mono builds via the csproj condition.

💡 Comment Typo — "Debuggger"

There's a typo in the Task constructor comments:

_ = s_asyncDebuggingEnabled; // Debuggger depends on Task cctor being run when any Task gets created

Should be "Debugger".

Cross-cutting Analysis

Sibling types: The changes are isolated to the runtime-async infrastructure. The existing TPL task debugging (s_currentActiveTasks, AddToActiveTasks, RemoveFromActiveTasks) is untouched and the new code follows the same patterns.

Callers: The new timestamp methods are only called from AsyncHelpers.CoreCLR.cs and are guarded by Task.s_asyncDebuggingEnabled, which is set by the debugger. Normal runtime paths are unaffected.

Summary

The PR is well-structured and follows existing patterns. The main concerns are:

1. Verify continuation timestamp cleanup on exception paths is complete
2. Confirm TPL event ordering change is acceptable
3. Minor typo fix

Assuming #1 and #2 are verified, the PR appears ready to merge.

1. and 3. have been fixed, and 2. is consistent with current implementation of async state machine boxes.