Skip to content

Add "Open from running process" dialog - #3936

Merged
christophwille merged 5 commits into
masterfrom
open-from-running-process
Jul 31, 2026
Merged

Add "Open from running process" dialog#3936
christophwille merged 5 commits into
masterfrom
open-from-running-process

Conversation

@christophwille

@christophwille christophwille commented Jul 29, 2026

Copy link
Copy Markdown
Member

Adds File > Open from running process...: a dialog that lists the running .NET processes, shows the assemblies loaded in the selected one, and opens the ones you pick. This is feature parity with dotPeek's "Explore running processes", but built on the runtime's diagnostics endpoint rather than OS module enumeration, so it works on Windows, Linux and macOS alike.

Selection is driven by buttons (Refresh, Add Entry Assembly, Add Selected, Cancel), modelled on the "Open from NuGet feed" dialog; there is no context menu.

Why this matters more with .NET 10, not less

With a modern .NET app the executable you see in the process list is a native apphost. ILSpy.exe contains no IL at all - open it and you get "PE file does not contain any managed metadata". The assembly you actually want is ILSpy.dll beside it, and neither the process list nor the command line tells you that.

Verified against a real running ILSpy instance from this branch:

name=ILSpy kind=CoreClr runtime=10.0.10 arch=x64
commandLine="Z:\...\ILSpy\bin\Debug\net10.0\ILSpy.exe"      <- what the OS shows: no IL in it
entryAssemblyName=ILSpy
entryPath=Z:\...\ILSpy\bin\Debug\net10.0\ILSpy.dll          <- what you actually want to open
modules=118                                                  incl. ILSpy.ReadyToRun.Plugin.dll

The indirection gets worse, not better, in the cases users care about: single-file bundles (assemblies packed inside the apphost), plugins loaded at runtime, Assembly.Load(byte[]), and custom AssemblyLoadContexts. A process explorer is the only thing that answers "which managed assemblies is this process actually running, and where are they?" - and as a bonus it shows which plugins are genuinely loaded, not merely present on disk.

Options explored

  • Diagnostics IPC (chosen). Since .NET Core 3.0 every CoreCLR process serves an endpoint - named pipe dotnet-diagnostic-{pid} on Windows, unix socket dotnet-diagnostic-{pid}-*-socket in $TMPDIR elsewhere. Its existence is the ".NET process" detector, and ProcessInfo2 returns the managed entry assembly, command line, architecture and runtime version. Same code path on all three OSes; no debugger attach, no ptrace, no privileges beyond same-user. This is how dotnet-trace ps works.
  • EventPipe rundown for the loaded-assembly list (chosen). A short session with requestRundown makes the runtime emit one event per loaded module and assembly, with real paths, from inside the process. Covers dynamic and bundled assemblies that no file-backed listing can see.
  • OS module enumeration - Toolhelp32 / PEB / Process.Modules (rejected for CoreCLR). It answers a different question: its list is dominated by native libraries, and it never sees bundled or in-memory assemblies. It is, however, exactly right for .NET Framework (see below).
  • Microsoft.Diagnostics.NETCore.Client (rejected). It would enumerate processes and start sessions, but it does not parse the resulting events, so TraceEvent would follow - a large dependency for a small need. Both the IPC protocol and a scoped nettrace reader are implemented here instead, so no new package reference; Directory.Packages.props and every packages.lock.json are untouched.
  • ClrMD / Microsoft.Diagnostics.Runtime (rejected for now). It could dump the raw PE bytes of an in-memory assembly out of a target - the one case this PR cannot open. Its macOS support has historically been the weakest of the three platforms, so it is the wrong primary mechanism; it remains the natural route if in-memory assemblies are wanted later.
  • ICorPublish / mscoree for .NET Framework (rejected). COM interop for a Windows-only legacy path, when the desktop CLR's own module list already yields the answer.

Implementation overview

  • ILSpy/Processes/ holds all the non-UI logic behind IProcessExplorer, so the dialog's view model is driven by a fake in headless tests. Placed in the app rather than ICSharpCode.ILSpyX to keep that shipped package's public API unchanged; it is easy to promote later if ilspycmd wants it.
  • DiagnosticsIpcMessage implements the wire format (20-byte header, UTF-16 length-prefixed strings); DiagnosticsIpcClient speaks ProcessInfo2 with a ProcessInfo fallback for pre-.NET 6 runtimes, and runs the EventPipe session.
  • NettraceRundownReader is a deliberately scoped nettrace parser: it walks the FastSerialization object stream far enough to decode loader rundown events and steps over everything else (stacks, sequence points, other providers) by size. It guards the container version and fails with a clear message on anything newer.
  • Two findings that only surfaced by running against live processes:
    • The CLR's own providers ship empty event names in EventPipe metadata, so events must be matched by provider plus numeric id - and the ids collide between providers (152 is ModuleLoad for the runtime provider but DomainModuleDCEnd, a different payload shape, for the rundown provider).
    • The runtime's default rundown keyword (0x80020139) also collects the JIT and IL-to-native-map rundown. In a long-running process that buries the module list and overruns the session buffer: an 11.8 MB trace arrived with zero module events in it. Requesting the loader keyword alone via CollectTracing4 (falling back to CollectTracing2 on older runtimes) turns the same query into 124 KB with all modules present.
  • Menu entry sits between "Open from NuGet feed" and "Reload", enabled on every OS.

Platform differences

Windows Linux macOS
Transport named pipe unix socket in $TMPDIR unix socket in $TMPDIR
CoreCLR processes + assemblies yes yes yes
.NET Framework processes yes n/a n/a

The CoreCLR path is genuinely identical everywhere; only the transport differs, and that is isolated in DiagnosticsPortScanner.Windows.cs / .Unix.cs (partial class, both always compiled, dispatched by OperatingSystem.IsWindows() - the existing ShellHelper pattern). On macOS the diagnostics socket is not merely convenient but effectively the only option, since native process introspection (task_for_pid) requires entitlements or root under SIP.

.NET Framework is Windows-only by nature and is the one place where behaviour differs: those processes predate the diagnostics endpoint, so they are detected by clr.dll/mscorwks.dll in their OS module list and their assemblies are read from that same list, filtered to files carrying a CLI header. All of it lives in one [SupportedOSPlatform("windows")] file.

Limitations

  • Same-user processes only, and processes started with DOTNET_EnableDiagnostics=0 expose no endpoint and are invisible. Both are inherent to the mechanism and affect dotnet-trace equally. The dialog states this in a hint line rather than pretending the list is exhaustive.
  • Assemblies that exist only in memory cannot be opened. Assembly.Load(byte[]) and dynamic assemblies have no file anywhere; they are listed and their rows say so, but adding them is not offered. Reading the bytes out of the target would need a ClrMD-style route.
  • Elevated processes are not visible to a non-elevated ILSpy.
  • Containerized processes may place their socket in a namespace that is not reachable.
  • .NET Framework fidelity is lower: only assemblies the desktop loader registered are listed, so byte-array loads are missing there too, and processes that deny module access are skipped silently. Those processes also mostly report NGen native images (mscorlib.ni.dll out of C:\Windows\assembly\NativeImages_v4.0.30319_64\...) rather than the IL assemblies they were compiled from: the images carry metadata and are genuinely what the process loaded, so they are listed truthfully, but their IL lives in an assembly the module list does not name, which makes opening one of limited use. Mapping native images back to their source assemblies would be guesswork against the GAC, so the list stays honest about what is loaded.
  • A suspended runtime (DOTNET_DefaultDiagnosticPortSuspend) is listed but its assembly query will hit the timeout rather than answer.
  • Collecting the assembly list takes a moment per process; it runs on selection, cancels when the selection moves on, and is bounded by a timeout.

What could have been simpler

The scope choice worth revisiting is the loaded-assembly list. Without it, the feature would be roughly a third of the code: process discovery plus one ProcessInfo2 call gives the entry assembly, which is the headline use case ("show me the dll behind this exe"), and ILSpy resolves the rest of an app's references from that assembly's directory anyway. The EventPipe session, the nettrace reader, and the two live-process findings above all exist purely to enumerate assemblies.

It earns its place for plugins, dynamically loaded assemblies and single-file bundles - where the loaded set genuinely cannot be derived from the entry assembly - but a first cut without it would have been a legitimate, much smaller PR.

Smaller simplifications also available: dropping .NET Framework support (Windows-only path, one extra file), and dropping the assembly-name resolution that gives in-memory modules a meaningful label instead of a generic one.

Testing

Full ILSpy.Tests suite green, 55 of the tests new.

The protocol layers are tested against the real thing rather than mocked: the test host is itself a .NET process with a live diagnostics port, so the tests self-inspect on whatever OS CI runs - port scan finds the current pid, ProcessInfo2 reports the test host's managed entry assembly, and a real rundown is collected and parsed. The fixture also emits a dynamic assembly before collecting that rundown, so the "listed but has no file" classification is pinned against a real one. Timeouts are pinned against an endpoint that accepts and then says nothing, and the refused-socket path against a bound-but-not-listening socket planted under a live pid (Linux/macOS, where that transport is used). The view model and dialog are covered headlessly against a fake explorer, and MainMenuTests pins the menu item's position, icon and enabled state on every OS.

The .NET Framework path has its own live tests in the OS-gated ILSpy.Tests.Windows project, using Windows PowerShell 5.1 as a deterministic desktop-CLR process: detection and description, the merge contract with the diagnostics scan, the managed-only module filter, and ProcessExplorer's routing of a Framework process to the module scan.

With a modern .NET app the executable in the process list is a native
apphost that carries no IL, so the path a user can see is precisely the
one a decompiler cannot open - ILSpy itself is an example. Ask the
runtime instead: since .NET Core 3.0 every CoreCLR process serves a
diagnostics endpoint that names its managed entry assembly and, via an
EventPipe rundown, every assembly it has loaded, including ones behind a
single-file bundle or with no file at all. The endpoint answers the same
way on Windows, Linux and macOS and needs no privileges beyond same-user,
which also makes it the only workable route on macOS, where native
process introspection is gated by SIP.

The protocol and the nettrace container it returns are implemented here
rather than taken from Microsoft.Diagnostics.NETCore.Client, so the
feature costs no new package reference; the reader is scoped to loader
rundown events and steps over everything else by size. The rundown asks
for the loader keyword alone: the runtime's default set also collects the
JIT and IL-to-native-map rundown, which in a long-running process buries
the module list and overruns the session buffer, costing the very events
the dialog needs.

Windows additionally lists .NET Framework processes, which predate the
endpoint and are read from their OS module list instead.

Assisted-by: Claude:claude-fable-5:Claude Code

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.

🟡 Not ready to approve

The new diagnostics IPC code contains compile-breaking issues (e.g., invalid array allocation from a uint, and a disposal pattern likely missing required imports) that must be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Adds a new cross-platform “Open from running process…” feature to ILSpy’s File menu, providing a dialog that discovers running .NET processes via the diagnostics IPC endpoint, enumerates loaded managed assemblies via an EventPipe rundown, and opens selected assemblies (or the entry assembly).

Changes:

  • Adds the “Open from running process…” dialog UI + view model, wired into the File menu.
  • Introduces a new ILSpy/Processes/ implementation for CoreCLR diagnostics IPC + nettrace rundown parsing, with Windows-only .NET Framework fallback.
  • Adds extensive headless/UI and live self-inspection tests for discovery, IPC/process info, rundown collection, and module parsing.
File summaries
File Description
ILSpy/Views/OpenFromProcessDialog.axaml.cs Dialog code-behind wiring, localization hookup, selection forwarding to VM, lifecycle/cancel behavior.
ILSpy/Views/OpenFromProcessDialog.axaml Dialog layout: filter + process grid + modules grid + error bar + action buttons.
ILSpy/ViewModels/ProcessRowViewModel.cs Row VM for process list display + filtering logic.
ILSpy/ViewModels/ProcessModuleRowViewModel.cs Row VM for module list display with in-memory labeling.
ILSpy/ViewModels/OpenFromProcessDialogViewModel.cs Main dialog VM: refresh, filtering, module loading with cancellation/generation guard, close signaling.
ILSpy/Properties/Resources.resx Adds localized strings for the new dialog and menu item.
ILSpy/Properties/Resources.Designer.cs Generated accessors for new resource strings.
ILSpy/Processes/RunningDotNetProcess.cs Process + module data records and entry-assembly path resolution logic.
ILSpy/Processes/ProcessExplorer.cs Implements process/module enumeration (CoreCLR via diagnostics + rundown; .NET Framework via module scanning on Windows).
ILSpy/Processes/NettraceRundownReader.cs Scoped nettrace parser to extract module/assembly data from EventPipe rundown traces.
ILSpy/Processes/NetFrameworkProcesses.Windows.cs Windows-only .NET Framework discovery and module listing via OS module enumeration.
ILSpy/Processes/IProcessExplorer.cs Abstraction for process/module enumeration to enable headless testing.
ILSpy/Processes/DiagnosticsPortScanner.Windows.cs Windows diagnostics endpoint discovery via named pipe enumeration.
ILSpy/Processes/DiagnosticsPortScanner.Unix.cs Unix/macOS diagnostics endpoint discovery via temp-dir socket enumeration and per-pid socket selection.
ILSpy/Processes/DiagnosticsPortScanner.cs Cross-platform dispatcher for diagnostics endpoint scanning.
ILSpy/Processes/DiagnosticsIpcMessage.cs Diagnostics IPC request/response framing and string encoding/decoding helpers.
ILSpy/Processes/DiagnosticsIpcClient.cs Diagnostics IPC client: ProcessInfo/ProcessInfo2 queries + EventPipe rundown collection over the diagnostics transport.
ILSpy/Commands/FileCommands.cs Adds File > Open from running process… menu command and opens returned paths in AssemblyTreeModel.
ILSpy.Tests/Views/OpenFromProcessDialogStructureTests.cs Headless UI structure tests for dialog controls, captions, selection forwarding, and error bar.
ILSpy.Tests/Views/MainMenuTests.cs Pins menu placement/enabled state for the new File menu entry.
ILSpy.Tests/Processes/ProcessExplorerTests.cs Live tests against the running test host for discovery, module enumeration, entry assembly resolution, and managed/native detection.
ILSpy.Tests/Processes/OpenFromProcessDialogViewModelTests.cs Behavior tests of the dialog VM against a fake explorer (filtering, selection, add/close behavior, cancellation).
ILSpy.Tests/Processes/NettraceRundownReaderTests.cs Live rundown collection + nettrace parsing assertions against the test host.
ILSpy.Tests/Processes/NativeRuntimeHost.cs Test helper to locate the native runtime host path cross-platform.
ILSpy.Tests/Processes/FakeProcessExplorer.cs Fake IProcessExplorer for deterministic VM/dialog tests (including cancellation/supersession).
ILSpy.Tests/Processes/DiagnosticsIpcClientTests.cs Golden-byte IPC encoding tests + live ProcessInfo query validation against the test host.
Review details

Files not reviewed (1)

  • ILSpy/Properties/Resources.Designer.cs: Generated file
  • Files reviewed: 25/26 changed files
  • Comments generated: 2
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Comment thread ILSpy/Processes/DiagnosticsIpcClient.cs Outdated
Comment on lines +101 to +103
Stream stream = await ConnectAsync(pid, timeout.Token).ConfigureAwait(false);
await using (stream.ConfigureAwait(false))
{

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This compiles as written, so there is nothing to fix here.

The extension is TaskAsyncEnumerableExtensions.ConfigureAwait(IAsyncDisposable, bool), which lives in System.Threading.Tasks - already imported at the top of this file. System.Runtime.CompilerServices is not involved. The branch builds clean on Windows, Linux and macOS.

It is not redundant either: await using awaits DisposeAsync(), and closing a named pipe or a unix socket has no reason to marshal its continuation back to the captured context. It is consistent with the .ConfigureAwait(false) on every other await in the file. No change.

return null;
if (length > MaxStringLength)
throw new IOException($"Diagnostics IPC string length {length} exceeds the sanity limit.");
var chars = new char[length];

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This compiles as written. An array-creation expression accepts int, uint, long or ulong as the length - new char[someUint] is legal C# and needs no cast. The branch builds clean on Windows, Linux and macOS.

The bound that actually matters is already two lines up: if (length > MaxStringLength) rejects anything over 1 MB before the allocation, so the length can never approach int.MaxValue. No change.

@siegfriedpammer siegfriedpammer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Review of the process-explorer feature, posted as individual comments on each finding. Everything below was verified by running the code on Linux (KDE/Wayland, .NET 10), not by reading alone.

Overall: strong, well-engineered feature. The diagnostics-IPC + EventPipe-rundown approach is the right call and it demonstrably works: driving the real explorer against a live ILSpy instance discovered 10 processes, collected and parsed 116 modules in 142 ms, correctly classified the one dynamic assembly as in-memory, and resolved the headline case - native apphost to actual IL - to ILSpy.dll. The CollectTracing4 payload layout is right, the loader-keyword finding in the description is real, and file headers, ASCII-only, en-US and full string localization all match the repo conventions. All 32 process tests passed on three consecutive runs; dialog and menu tests 24/24.

Three defects look worth fixing before merge - one functional (the whole process list fails when any one process is unreachable) and two UX (filter keystrokes drop the selection; the assemblies progress bar can stick). Each has a reproduction in its comment. The rest are test-coverage gaps and nits.

One hypothesis I checked and could not substantiate, recorded so nobody re-litigates it: I suspected the rundown's background copy task could fault unobserved when a collection is cancelled, which GlobalExceptionHandler would turn into a spurious crash dialog. Six cancellations at varying points produced zero unobserved exceptions - the task ends Canceled, not Faulted. Not an issue.

(This review was produced by an AI agent on Siegfried's behalf.)

Comment thread ILSpy/Processes/ProcessExplorer.cs Outdated
Comment thread ILSpy/ViewModels/OpenFromProcessDialogViewModel.cs Outdated
Comment thread ILSpy/ViewModels/OpenFromProcessDialogViewModel.cs
Comment thread ILSpy/ViewModels/OpenFromProcessDialogViewModel.cs Outdated
Comment thread ILSpy/ViewModels/OpenFromProcessDialogViewModel.cs
Comment thread ILSpy/Processes/NettraceRundownReader.cs
Comment thread ILSpy/Processes/NettraceRundownReader.cs
Comment thread ILSpy.Tests/Processes/NettraceRundownReaderTests.cs Outdated
Comment thread ILSpy/ViewModels/ProcessRowViewModel.cs Outdated
Comment thread ILSpy/Commands/FileCommands.cs Outdated
A refused unix socket raises SocketException, which derives from
Win32Exception rather than IOException, so it escaped the filter meant to
skip one unreachable process and failed the whole concurrent enumeration
instead: a machine where any .NET process exits between the port scan and
the connect showed an empty list. The classification is now a named
predicate covering both transports.

Two dialog defects shared a shape - state left behind by a query nobody is
waiting for any more. Rebuilding the bound collection on every filter
keystroke made the grid drop its selection and write that null back,
discarding the assemblies of a process the new filter still matched; and the
branch taken when nothing is selected cleared no loading flag, while the
query it superseded was no longer allowed to, so the progress bar animated
over an empty pane. Relatedly, the two-second command budget expired into a
process listed with null metadata, which made a slow machine look like a
runtime that answered with nothing; it is longer now, and expiry names the
process it gave up on, since the only thing that ever reaches the far end of
that budget is a runtime which will never answer.

The test gaps are closed the same way: a real dynamic assembly pins the
in-memory classification, the managed-only assertion is stated as the
property instead of a list of native names to exclude, and the .NET
Framework path gets live tests. Those showed that a desktop CLR process
mostly reports NGen native images rather than the IL assemblies behind them,
which is now recorded as the fidelity gap it is.

Assisted-by: Claude:claude-opus-5:Claude Code
@christophwille

Copy link
Copy Markdown
Member Author

Thanks - the review was unusually actionable, and every one of the three defects reproduced exactly as written. All fourteen points are addressed in a follow-up commit; details are in the reply under each comment.

The three defects, each red first, then fixed:

  • SocketException escaping the unreachable-process filter. The classification is now a named ProcessExplorer.IsUnreachable covering Win32Exception, so both transports are handled. Two tests: one that walks every way an endpoint can fail to answer (and checks a reader defect is not absorbed), and your own repro - a bound-but-not-listening socket planted under a live non-.NET child pid, gated to Linux/macOS.
  • Filter keystrokes dropping the selection. ApplyFilter now syncs the bound collection in place instead of clearing it, so a row that still matches is never removed and the grid never writes null back. Pinned against the real dialog, since the view model alone cannot see it. This also turned up the mirror case: filtering the selected row out did not clear the selection either, because Avalonia's DataGrid keeps a removed item - so that is now explicit rather than left to the grid.
  • The assemblies progress bar sticking. The process == null path resets the flag. Your diagnosis of why the suite stayed green was the key: the fake explorer's gate completed inline on cancellation. It now awaits without the token, which is also the truer model of the real explorer, and the new test reproduced the stuck bar before the fix.

The CommandTimeout constant is 10 s instead of 2 s, and no longer degrades silently: an expired budget becomes TimeoutException("Process {pid} did not answer..."). ProcessExplorer already classified TimeoutException as unreachable, so the listing is unchanged - but the client now distinguishes "the target went quiet" from "the caller cancelled", which is what made that test failure unreadable, and the dialog's error bar now names the process. Pinned by an endpoint that accepts and then says nothing, with the budget injected so the test costs 250 ms.

Coverage gaps closed. The in-memory branch is pinned by emitting a real dynamic assembly before the shared rundown is collected. The four-prefix blacklist is replaced by the property itself - every path-backed module must satisfy IsManagedAssembly, asserted per module - and that same property now guards the .NET Framework list. The .NET Framework path gets six live tests in ILSpy.Tests.Windows, with Windows PowerShell 5.1 as a deterministic desktop-CLR fixture.

One thing writing those tests taught me, now in the PR's Limitations: a .NET Framework process mostly reports NGen native images (mscorlib.ni.dll from the NGen cache), not the IL assemblies. They carry metadata and are genuinely what the process loaded, so the list is truthful - but their IL lives in an assembly the module list does not name, which makes opening one of limited use. Mapping images back to their sources would be guesswork against the GAC, so the list stays honest instead of clever.

Both nits are done: ProcessRowViewModel.CommandLine is gone, and the menu entry has an icon (Images/Process, from an SVG Christoph supplied), with MainMenuTests asserting it is there.

Also noted and not re-litigated: your unobserved-exception hypothesis about the rundown's copy task. Recording that it was checked and found not to be an issue is genuinely useful.

The two Copilot findings are both incorrect - new char[uint] is legal C# and the ConfigureAwait(IAsyncDisposable, bool) extension is in System.Threading.Tasks, which is imported. Answered individually under each.

@christophwille

christophwille commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

ai;dr a screenshot (Claude insists on removing it from the main description)

image

Now with ListBox instead:

image

@christophwille
christophwille marked this pull request as ready for review July 30, 2026 06:18

@siegfriedpammer siegfriedpammer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Follow-up review of f574c59, verified by checking out the PR head and running it on Linux (KDE, .NET 10) - not by reading alone.

All 14 points from the previous round are fixed as described in the replies. Each claimed mechanism is present and does what the reply says: ProcessExplorer.IsUnreachable covers Win32Exception (and the refused-socket test pins your repro end to end), ApplyFilter syncs the bound collection in place and explicitly drops a filtered-out selection, the process == null path resets IsLoadingModules, both fire-and-forget sites use HandleExceptions(), ReplaceProcesses unifies the success and failure paths, CancelAndDispose covers every replacement site, the 10 s budget expires into a TimeoutException naming the pid, the in-memory branch is pinned by a real dynamic assembly, the managed-only assertion is the positive property per module, the .NET Framework path has six live tests, CommandLine is gone from the row VM, and the menu icon is asserted in MainMenuTests. Full ILSpy.Tests suite on this branch: 1166 passed, 0 failed, 3 skipped. The ILSpy.Tests.Windows fixtures could not be run here (Windows-only by design) and were reviewed by reading; Windows PowerShell 5.1 as a deterministic desktop-CLR fixture is a sound choice.

One new defect, with a repro and a verified fix - see the inline comment on DiagnosticsIpcClient.cs. The rundown's drain task faults unobserved when StopTracingAsync fails with a non-cancellation error, and GlobalExceptionHandler.OnUnobservedTask turns that into a spurious crash dialog at the next GC. This is the sibling of the hypothesis recorded last round: the cancellation path is indeed clean (the task ends Canceled, six runs out of six), but the non-cancellation branch of the same catch is not, and it reproduces deterministically.

Checked and fine, recorded so nobody re-litigates:

  • ApplyFilter's in-place merge is correct: after the removal pass, Processes is always a subsequence of matching (both preserve allProcesses order, and a refresh replaces every row object so stale rows are always evicted), so inserting at the first mismatch index is sound.
  • HungDiagnosticsEndpoint's discarded accept tasks cannot fault unobserved in the current tests: the client connects in every test that uses the endpoint, so WaitForConnectionAsync/AcceptAsync complete successfully before Dispose runs.
  • CancelAndDispose's dispose-straight-after-cancel is safe as its comment claims: IsCancellationRequested is the only member read after disposal, and it stays valid on a disposed source.

(This review was produced by an AI agent on Siegfried's behalf.)

await StopTracingAsync(pid, sessionId, token).ConfigureAwait(false);
await drain.ConfigureAwait(false);
}
catch

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The drain task faults unobserved when StopTracing fails with a non-cancellation error, and the app turns that into a spurious crash dialog.

Last round's recorded check cleared the cancellation path: cancelled at any point, the copy task ends Canceled, which never counts as unobserved. This catch has a second way in, though: StopTracingAsync failing with a real error - most plausibly the target exiting mid-collection, which resets the stop connection. Then trace is disposed, the exception rethrows, the await using tears down session - and drain is still parked on a read of it. It faults with IOException(SocketException 125), nobody ever awaits it, and at the next GC TaskScheduler.UnobservedTaskException fires, which GlobalExceptionHandler.OnUnobservedTask routes into Report(...): a crash dialog, minutes after the dialog's error bar already explained the failure correctly. The CollectTracing2 fallback retries the same path, so one dying process produces two such faults.

Reproduced deterministically on this branch with a fake endpoint that grants a session id, holds the trace connection open, and resets the stop connection: the client fails with IOException: Connection reset by peer (which the error bar shows - correct), and the finalizer then surfaces AggregateException(IOException(SocketException 125: Operation canceled)) out of Stream.CopyToAsync.

Fix verified against the same repro (the unobserved exception is gone; the primary IOException still propagates unchanged): in this catch, dispose session first - that is what unblocks the drain - then await drain swallowing its failure, then dispose trace. The later await using disposal of session is idempotent.

catch
{
	await session.DisposeAsync().ConfigureAwait(false);
	try
	{
		await drain.ConfigureAwait(false);
	}
	catch
	{
		// The primary failure is propagating; the drain ended on the torn-down
		// connection and its own failure is only a symptom of that.
	}
	await trace.DisposeAsync().ConfigureAwait(false);
	throw;
}
Repro test (self-contained, Linux/macOS; asserts the unobserved exception fires, so it goes red once the fix is in)
[TestFixture]
[Platform(Exclude = "Win")]
public class UnobservedDrainReproTests
{
	const int FakePid = 0x7FFE_0000;

	[Test]
	public async Task Drain_Task_Faults_Unobserved_When_Stop_Fails_NonCancellationally()
	{
		string socketPath = Path.Combine(Path.GetTempPath(), $"dotnet-diagnostic-{FakePid}-1-socket");
		File.Delete(socketPath);
		using var listener = new Socket(AddressFamily.Unix, SocketType.Stream, ProtocolType.Unspecified);
		listener.Bind(new UnixDomainSocketEndPoint(socketPath));
		listener.Listen(4);

		var unobserved = new TaskCompletionSource<Exception>(TaskCreationOptions.RunContinuationsAsynchronously);
		EventHandler<UnobservedTaskExceptionEventArgs> handler = (_, e) => unobserved.TrySetResult(e.Exception!);
		TaskScheduler.UnobservedTaskException += handler;
		try
		{
			var serverTask = RunServerAsync(listener);

			Exception? caught = null;
			try
			{
				using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30));
				using var trace = await DiagnosticsIpcClient.CollectModuleRundownAsync(FakePid, cts.Token);
			}
			catch (Exception ex)
			{
				caught = ex;
			}
			caught.Should().NotBeNull("the fake stop connection dies, so the collection must fail");
			caught.Should().NotBeOfType<OperationCanceledException>(
				"the repro needs the non-cancellation failure path");

			// Give the drain task time to fault, then finalize the abandoned task.
			await Task.Delay(500);
			GC.Collect();
			GC.WaitForPendingFinalizers();
			GC.Collect();

			var fired = await Task.WhenAny(unobserved.Task, Task.Delay(3000));
			fired.Should().Be(unobserved.Task, "the drain task faulted with nobody awaiting it");
		}
		finally
		{
			TaskScheduler.UnobservedTaskException -= handler;
			File.Delete(socketPath);
		}
	}

	static async Task RunServerAsync(Socket listener)
	{
		// Connections alternate: collect (odd) -> success + session id, held open;
		// stop (even) -> closed without an answer.
		for (int i = 1; i <= 4; i++)
		{
			Socket client;
			try
			{
				client = await listener.AcceptAsync();
			}
			catch (Exception)
			{
				return;
			}
			if (i % 2 == 1)
				_ = HandleCollectAsync(client);
			else
				client.Dispose();
		}
	}

	static async Task HandleCollectAsync(Socket client)
	{
		try
		{
			var stream = new NetworkStream(client, ownsSocket: false);
			var buffer = new byte[512];
#pragma warning disable CA2022 // one partial read is fine; the request content is irrelevant
			await stream.ReadAsync(buffer);
#pragma warning restore CA2022

			// Success response: magic, size 28, command set 0xFF, id 0x00, reserved, 8-byte session id.
			var response = new byte[28];
			System.Text.Encoding.ASCII.GetBytes("DOTNET_IPC_V1\0", response);
			BinaryPrimitives.WriteUInt16LittleEndian(response.AsSpan(14), 28);
			response[16] = 0xFF;
			response[17] = 0x00;
			BinaryPrimitives.WriteUInt64LittleEndian(response.AsSpan(20), 0xDEADBEEF);
			await stream.WriteAsync(response);
			// Held open: the client's drain parks on a read that only ends when the
			// client tears the connection down.
		}
		catch (Exception)
		{
			// The client tearing the connection down is expected.
		}
	}
}

(Review comment written by an AI agent.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Confirmed, and fixed in a7920b1. The analysis is right on every step: the non-cancellation path through this catch never touches drain, drain is parked on a read of session, the await using on the way out is what fails that read, and GlobalExceptionHandler.OnUnobservedTask turns the finalizer's report into Report(...). Two of them per dying target, because the CollectTracing2 fallback walks the same path.

Worth adding to the account: on that path trace is disposed while the drain is still copying into it, so the drain can equally well fault on a write to a disposed MemoryStream. The ordering in the suggested fix removes that too, which is why I took it as given.

What landed is the same three steps, in a named method so the ordering constraint has somewhere to live, and with each await on the cleanup path unable to skip the one after it (a throwing DisposeAsync must not be what costs us the await drain):

internal static async Task AbandonCollectionAsync(Stream session, Task drain, MemoryStream trace)
{
    await SuppressFailureAsync(session.DisposeAsync().AsTask()).ConfigureAwait(false);
    await SuppressFailureAsync(drain).ConfigureAwait(false);
    await trace.DisposeAsync().ConfigureAwait(false);
}

On the test, I went a different way than the suggested repro, and it is worth saying why. I first built the end-to-end version of it as a cross-platform fake endpoint (named pipe and unix socket, dispatching on the command id in the request header) and it passed on Windows before the fix - vacuously: the fake never received the stop connection at all, the client sat out the full 30 s rundown budget, and a TimeoutException plus a cancelled drain is not an unobserved fault. The Windows named-pipe transport cannot express this symptom in the first place, since an aborted overlapped read ends the copy cancelled rather than faulted, and cancelled tasks are never unobserved. So an end-to-end test of it would have been unix-only, and I have no unix box here to see it fail on - a regression guard I cannot watch go red is not one I want to add.

Instead the invariant is pinned where it holds on all three OSes - the drain must not still be running once the failure path is done with it:

using var session = new BlockedConnection();   // reads complete only when it is torn down, then fail
var trace = new MemoryStream();
Task drain = session.CopyToAsync(trace);
drain.IsCompleted.Should().BeFalse(...);

await DiagnosticsIpcClient.AbandonCollectionAsync(session, drain, trace);

drain.IsCompleted.Should().BeTrue(
    "a drain still running once the failure path is done with it is a drain nobody will ever await");
trace.CanRead.Should().BeFalse("the half-copied trace of a failed collection is dropped");

Red against the previous body of that method (Expected drain.IsCompleted to be True ... but found False), green after, and it fails on Windows too if someone drops the await drain later - which the unix-only version would not have.

A collection that fails once the session has been granted - most plausibly
the target exiting before the stop command reaches it - left the task copying
the trace connection behind. That connection is torn down on the way out, the
copy faults, nobody awaits it, and the finalizer hands it to
TaskScheduler.UnobservedTaskException, which this app reports as a crash: a
second report of a failure the dialog's error bar had already explained
correctly, minutes later and detached from the gesture that caused it. The
CollectTracing2 fallback walks the same path, so one dying target produced two
of them.

The teardown order is the substance of the fix. The session connection has to
go first, because after the failure nothing else will ever end the read the
copy is parked on; the drain second, so that its own failure is observed
rather than abandoned; and the half-copied trace last, so nothing is still
writing into it when it is dropped.

The regression test pins the invariant rather than the symptom, because the
symptom is transport-specific: a Windows named pipe reports an aborted
overlapped read as cancellation, and a cancelled task is never unobserved, so
only the unix transport can produce the crash at all. What holds everywhere is
that no drain may still be running once the failure path is done with it.

Assisted-by: Claude:claude-opus-5:Claude Code
ProDataGrid derives its scroll extent from the current scroll offset, so an
offset that is briefly too large inflates the extent, which permits a larger
offset again. A trackpad reaches that state within a few hundred sub-row
events: instrumented in the running app, a ~1500px list reported 8735px and
kept growing, the thumb collapsed to its minimum, and the end of the list ran
away from the user. A second defect slid the rows sideways by up to 10px - the
star-sized column is measured against the width including the vertical scroll
bar, leaving the grid convinced it has a scroll bar's worth of content to
reach.

Disabling the horizontal scroll bar, giving each grid its own
DefaultRowHeightEstimator, pinning RowHeight, forcing the vertical scroll bar
visible, and disabling scrolling on the template's inner ScrollViewer each
removed a symptom at most; none can break a loop that runs through the scroll
offset, and 12.0.4 is the newest release. A ListBox's virtualizing panel keeps
the extent a function of the items alone. The price is laying out the columns
here - so the header row and the item template have to be kept in step - and
losing sortable, resizable headers.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
@christophwille

Copy link
Copy Markdown
Member Author

Why the dialog's two grids became ListBoxes

Trackpad scrolling in this dialog was unusable, and the cause turned out to be two
independent defects in ProDataGrid 12.0.4 (the newest release; nothing newer to upgrade to).
Recording this here so the decision is not re-litigated from scratch, and so anyone who
switches back knows what to answer for.

Repro

  1. Open the dialog on a machine with enough .NET processes to overflow the list (~60 here).
  2. Two-finger scroll the process list downwards with a trackpad, past the last row.
  3. Keep pushing, then scroll back up.

A mouse wheel or dragging the scroll bar does not reproduce it: both move in whole rows
or set an absolute position. A trackpad sends a stream of sub-row events - measured on
macOS, deltas of 0.02-0.12 (roughly 0.3-3px), each with a small horizontal component
alongside the vertical one. That is what walks the control into the broken state.

Exhibited behavior

  • A horizontal scroll bar appears and disappears every few frames, taking its height off
    the rows area each time, so the grid flickers and the rows jump. Frame-stepping a screen
    recording showed it toggling on 62 of 120 sampled frames.
  • The rows slide sideways by up to 10px during a purely vertical scroll.
  • The end of the list runs away: the scroll thumb shrinks to its minimum and you can keep
    scrolling long past the last row, which drifts in and out of view.

Measurements

Instrumenting the running dialog (logging extent, viewport, offset, column widths and
scroll-bar state on every layout pass, then scrolling by hand) gave 776 samples:

observation value
Reported content height while scrolled to the top 288, 1354, 1554, 1950, 8735 px
...as multiples of the 24px row 12, 56, 65, 81, 364 rows
Actual list height ~1500px
Relationship that held throughout extentHeight ~= offsetY + 168
Largest vertical offset reached 8567px
Horizontal offset during vertical scrolling up to 10.24px
Samples where the columns were 824px wide in an 806px viewport 594 / 776

Root causes

The vertical runaway. DataGrid.EdgedRowsHeightCalculated returns
_verticalOffset - NegVerticalOffset + <heights of the displayed rows> whenever the last
row is realized, and separately writes an offset-derived value back into the row-height
estimator (RowHeightEstimate = num4 / num6). So an offset that is briefly too large
inflates the per-row estimate, which inflates the extent, which permits a larger offset -
a positive feedback loop that ratchets up and does not recover, as the "back at the top"
numbers above show. Both statements are unconditional: no public property gates either.

The sideways drift. The star-sized last column is measured against the full grid width
(824px) rather than the cells viewport (806px), so the grid always believes it has one
vertical scroll bar's worth of content to reach. DataGridRowsPresenter.SyncOffset pushes
the grid's own HorizontalOffset straight into the presenter, clamped only by that phantom
overflow - which is why the horizontal component of a trackpad scroll shifts the rows.

Fixes tried, and why each was not enough

Attempt Result
HorizontalScrollBarVisibility="Disabled" Removes the flickering scroll bar - confirmed in the running app, 0 of 521 recorded frames. Does not stop the sideways drift: it hides the scroll bar without removing the phantom overflow that HorizontalOffset is clamped to.
Per-grid DefaultRowHeightEstimator instead of the default AdvancedRowHeightEstimator Makes the estimate exact in a headless test (1.1x -> 1.0x), but cannot break a loop whose input is the scroll offset. The runaway persisted in the app.
RowHeight="24" (pinning a uniform row height) No effect on the reported extent.
VerticalScrollBarVisibility="Visible" No effect; the phantom 18px is not caused by the scroll bar appearing.
ScrollViewer.AllowAutoHide No effect.
Fixed-width or Auto last column instead of star Trades the flicker for a permanently visible horizontal scroll bar.
Disabling scrolling on the template's inner PART_ScrollViewer No effect - the offset does not come from the ScrollViewer.

What replaced it

Both lists are now plain ListBoxes with a column-aligned ItemTemplate and a header row.
A ListBox's virtualizing panel derives its extent from the items alone, so the feedback
loop has nothing to feed on. Regression tests assert, over 900 trackpad-sized wheel events,
that the horizontal offset stays exactly 0, the content height is single-valued and equal to
rowCount * rowHeight, the maximum offset lands exactly at extent - viewport, and the last
row comes to rest fully inside the viewport - the four things the DataGrid could not do.

The cost: the column widths now live in two places (the header row and the item template)
and have to be kept in step, and the sortable, resizable column headers are gone.

Worth reporting upstream to wieslawsoltes/ProDataGrid; the same two defects will affect any
DataGrid here with a star-sized last column (SearchPane, CompareView,
OpenFromGacDialog), since the estimator default is process-wide.

Avalonia windows default to ShowInTaskbar=true, so every owned modal
dialog (Open from GAC/NuGet feed/running process, Manage Assembly
Lists, Export Project, Create List, Set Target Framework) got its own
taskbar button, which is not standard Windows behavior. The assertion
and crash dialogs intentionally keep their buttons so an interrupted
session stays easy to find.

Assisted-by: Claude:claude-fable-5:Claude Code
@christophwille
christophwille merged commit 03965a2 into master Jul 31, 2026
16 of 17 checks passed
@christophwille
christophwille deleted the open-from-running-process branch July 31, 2026 10:43
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