Skip to content

Fix MCP trace loading hang#45

Open
ivberg wants to merge 3 commits intomainfrom
user/ivberg/fix-mcp-trace-loading-regression
Open

Fix MCP trace loading hang#45
ivberg wants to merge 3 commits intomainfrom
user/ivberg/fix-mcp-trace-loading-regression

Conversation

@ivberg
Copy link
Copy Markdown
Collaborator

@ivberg ivberg commented Apr 7, 2026

Fix MCP trace loading hang: add PMC sample support and remove dangerous UI fallback

  • Add PerfInfoPMCSample handler to BuildProcessSummary so traces collected with hardware counter profiling (not just standard CPU sampling) correctly enumerate processes
  • Remove fallback from OpenTrace to OpenTraceAsync when process list extraction fails — the old code returned a clean error, but baa4f95 introduced a path that opened a ProfileLoadWindow which also couldn't load, causing a hang
  • Reopen trace source for merged ETL files ("[multiple files]") so the second Process() pass gets fresh events instead of an exhausted source
  • Close ProfileLoadWindow in finally blocks to prevent dialog accumulation across repeated MCP open_trace calls
  • Guard MessageBox.Show calls with SuppressDialogsForAutomation to prevent UI deadlocks during MCP automation
  • Cache FindTraceProcesses result to avoid double ETL parsing when GetAvailableProcessesAsync and OpenTraceAsync run back-to-back
  • Use bidirectional Contains matching for process names so compound inputs like "msedgewebview2.exe webview-exe-name=searchhost.exe" resolve correctly
  • Add diagnostic logging throughout MCP flow for future debugging

…us UI fallback

  - Add PerfInfoPMCSample handler to BuildProcessSummary so traces collected
    with hardware counter profiling (not just standard CPU sampling) correctly
    enumerate processes
  - Remove fallback from OpenTrace to OpenTraceAsync when process list extraction
    fails — the old code returned a clean error, but baa4f95 introduced a path
    that opened a ProfileLoadWindow which also couldn't load, causing a hang
  - Reopen trace source for merged ETL files ("[multiple files]") so the second
    Process() pass gets fresh events instead of an exhausted source
  - Close ProfileLoadWindow in finally blocks to prevent dialog accumulation
    across repeated MCP open_trace calls
  - Guard MessageBox.Show calls with SuppressDialogsForAutomation to prevent
    UI deadlocks during MCP automation
  - Cache FindTraceProcesses result to avoid double ETL parsing when
    GetAvailableProcessesAsync and OpenTraceAsync run back-to-back
  - Use bidirectional Contains matching for process names so compound inputs
    like "msedgewebview2.exe webview-exe-name=searchhost.exe" resolve correctly
  - Add diagnostic logging throughout MCP flow for future debugging
Copilot AI review requested due to automatic review settings April 7, 2026 21:17
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes an MCP automation hang when opening traces by improving process enumeration for more trace types and removing UI-based fallback paths that could deadlock during automation.

Changes:

  • Add PMC (PerfInfoPMCSample) support and trace re-open logic in ETW process summary building to handle hardware-counter traces and merged ETLs.
  • Update MCP OpenTrace flow to avoid falling back to UI loading on process-list extraction failure, add diagnostics, and improve process-name matching.
  • Add automation hooks in ProfileLoadWindow to suppress modal dialogs and allow injecting a preloaded process list to avoid double ETL parsing.

Reviewed changes

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

File Description
src/ProfileExplorerUI/Windows/ProfileLoadWindow.xaml.cs Adds MCP automation hooks (skip TextChanged processing, inject preloaded process list) and guards MessageBox usage to avoid UI deadlocks.
src/ProfileExplorerUI/Mcp/McpActionExecutor.cs Adds cached process list reuse between GetAvailableProcessesAsync and OpenTraceAsync, ensures ProfileLoadWindow cleanup, and adds diagnostic logging.
src/ProfileExplorerCore/Profile/ETW/ETWEventProcessor.cs Extends process-summary building to handle PMC samples and reopens merged traces so the second processing pass can read events.
src/ProfileExplorer.Mcp/ProfileExplorerMcpServer.cs Improves OpenTrace matching logic and removes the UI fallback when process enumeration fails; adds trace timing diagnostics.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/ProfileExplorerUI/Mcp/McpActionExecutor.cs
Comment thread src/ProfileExplorerUI/Mcp/McpActionExecutor.cs
Comment thread src/ProfileExplorerCore/Profile/ETW/ETWEventProcessor.cs
Comment thread src/ProfileExplorer.Mcp/ProfileExplorerMcpServer.cs
ivberg added 2 commits April 7, 2026 15:35
…eason

  - Rename CloseProfileLoadWindowOnFailureAsync to CloseProfileLoadWindowIfVisibleAsync
    since it's called unconditionally in finally blocks as a safety net
  - Change FailureReason from "ProcessListEmpty" to "GetAvailableProcessesFailed"
    to accurately describe the operation that failed regardless of underlying cause
  When a trace has no CPU sampling, PMC, or CSwitch data (all process
  weights are zero), return a NoProfilingData error from OpenTrace instead
  of attempting to load an empty profile. Also fall back to process
  start/stop events for the process list when no sample events exist,
  so the trace contents are still discoverable via GetAvailableProcesses

// First, check if this might be an ambiguous query by getting available processes
GetAvailableProcessesResult processesResult = await _executor.GetAvailableProcessesAsync(profileFilePath);
Trace.TraceInformation($"[MCP] OpenTrace: GetAvailableProcesses completed in {stopwatch.ElapsedMilliseconds}ms, success={processesResult.Success}, count={processesResult.Processes?.Length ?? 0}");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call Stop on the stopwatch right before this line

Description = "The trace file does not contain CPU sampling or performance counter data. It may have been collected without profiling enabled.",
Timestamp = DateTime.UtcNow
};
return System.Text.Json.JsonSerializer.Serialize(noSamplesResult, new System.Text.Json.JsonSerializerOptions { WriteIndented = true });
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update SerializeOpenTraceResult for these new errors, or create a new method to handle this.

if (!traceReopened) {
kernel = ReopenTrace();
DiagnosticLogger.LogInfo("[BuildProcessSummary] Reopened trace for merged ETL second pass");
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What bug is this one fixing?

}
};

// Also handle PMC (Performance Monitor Counter) sample events, which are used
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to flag that the event comes from PMC in the sample event data we are collecting?

if (result.Count == 0 && profile.Processes != null && profile.Processes.Count > 0) {
DiagnosticLogger.LogInfo($"[BuildProcessSummary] No samples found, building process list from {profile.Processes.Count} discovered processes");
foreach (var proc in profile.Processes) {
result.Add(new ProcessSummary(proc, TimeSpan.Zero));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about having a negative weight? Or null weight?

0 weight could cause some issues if we have some sampled data?

/// Prevents dialog accumulation across multiple open_trace calls.
/// On success the dialog closes itself via LoadButton_Click; this is a safety net.
/// </summary>
private async Task CloseProfileLoadWindowIfVisibleAsync(ProfileExplorer.UI.ProfileLoadWindow window)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More reason to move this to a library we can consume...

textChangedMethod.Invoke(profileLoadWindow, new object[] { profileLoadWindow, new RoutedEventArgs() });
}
});
var textChangedMethod = profileLoadWindow.GetType().GetMethod("ProfileAutocompleteBox_TextChanged",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could break very easily

var sw = Stopwatch.StartNew();
var processSummaries = await ETWProfileDataProvider.FindTraceProcesses(
profileFilePath, options, _ => { }, cancelableTask);
DiagnosticLogger.LogInfo($"[MCP] GetAvailableProcessesAsync: FindTraceProcesses completed in {sw.ElapsedMilliseconds}ms, count={processSummaries?.Count ?? 0}");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we still needing these debug stop watches?

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.

3 participants