Skip to content

Simplify and fix ProcessWrapper.KillProcessTree#53919

Open
Copilot wants to merge 11 commits intomainfrom
copilot/update-killprocess-tree-logic
Open

Simplify and fix ProcessWrapper.KillProcessTree#53919
Copilot wants to merge 11 commits intomainfrom
copilot/update-killprocess-tree-logic

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 16, 2026

KillProcessTreeInternal calls Process.Kill() without the entireProcessTree parameter, missing the native process-tree kill available since .NET Core 3.0. On NETFRAMEWORK, the method manually walks child processes via WMI — that path is unchanged.

  • On non-NETFRAMEWORK targets, call process.Kill(entireProcessTree: true) directly in KillProcessTree, eliminating the need for KillProcessTreeInternal
  • Moved KillProcessTreeInternal behind #if NETFRAMEWORK since it is only needed for the WMI-based recursive child process kill
  • Ensured all Process instances are properly disposed via using
  • Swallow all exceptions around the kill call so the helper remains best-effort and doesn't cause test flakiness, matching the original behavior
  • Disposed WMI objects (ManagementObjectSearcher, ManagementObjectCollection, ManagementObject) in the NETFRAMEWORK path to prevent handle leaks

Copy link
Copy Markdown
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

@copilot I believe there is still place for improvement:

  • we need to ensure every Process instance gets disposed
  • we can simplify the non-framework impl

Following code is pseudocode, please take as inspiration, make sure you can build the product before pushing any changes.

        public static void KillProcessTree(int processId)
        {
            using Process? process = null;

            try
            {
                process = Process.GetProcessById(processId);
            }
            catch (ArgumentException)
            {
                // Process might have already exited.
                return;
            }

            if (!process.HasExited)
            {
#if NETFRAMEWORK
                KillProcessTreeInternal(process.ID);
#else
                process.Kill(entireProcessTree: true);
#endif
            }
        }

#if NETFRAMEWORK
        private static void KillProcessTreeInternal(int pid)
        {
            ManagementObjectSearcher searcher = new ManagementObjectSearcher("Select * From Win32_Process Where ParentProcessID=" + pid);
            ManagementObjectCollection moc = searcher.Get();
            foreach (ManagementObject mo in moc)
            {
                KillProcessTreeInternal(Convert.ToInt32(mo["ProcessID"]));
            }

            try
            {
                using Process proc = Process.GetProcessById(pid);
                proc.Kill();
            }
            catch (ArgumentException)
            {
                // vramak: Process might have already exited.
            }
        }
#endif

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 16, 2026

@copilot I believe there is still place for improvement:

  • we need to ensure every Process instance gets disposed
  • we can simplify the non-framework impl
    ...

Applied both improvements in ddb2308:

  • All Process instances are now disposed (using (process) in KillProcessTree, using (Process proc = ...) in KillProcessTreeInternal)
  • Simplified the non-framework path: process.Kill(entireProcessTree: true) is called directly in KillProcessTree, and KillProcessTreeInternal is now entirely behind #if NETFRAMEWORK

Copilot AI changed the title Use Process.Kill(entireProcessTree: true) on non-NET472 in ProcessWrapper Simplify and fix ProcessWrapper.KillProcessTree Apr 16, 2026
Copilot AI requested a review from adamsitnik April 16, 2026 14:24
@adamsitnik adamsitnik marked this pull request as ready for review April 27, 2026 19:00
@adamsitnik adamsitnik requested a review from vijayrkn as a code owner April 27, 2026 19:00
Copilot AI review requested due to automatic review settings April 27, 2026 19:00
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

Updates the ProcessWrapper test helper to use the built-in process-tree termination API on modern .NET, while keeping the existing WMI-based recursive kill for .NET Framework.

Changes:

  • On non-.NET Framework targets, uses Process.Kill(entireProcessTree: true) directly from KillProcessTree.
  • Restricts KillProcessTreeInternal (WMI recursion) to #if NETFRAMEWORK.
  • Ensures Process instances are disposed via using.

Comment thread test/Microsoft.NET.Sdk.Publish.Tasks.Tests/EndToEnd/ProcessWrapper.cs Outdated
Comment thread test/Microsoft.NET.Sdk.Publish.Tasks.Tests/EndToEnd/ProcessWrapper.cs Outdated
Copy link
Copy Markdown
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

@copilot address my feedback

Comment thread test/Microsoft.NET.Sdk.Publish.Tasks.Tests/EndToEnd/ProcessWrapper.cs Outdated
Comment thread test/Microsoft.NET.Sdk.Publish.Tasks.Tests/EndToEnd/ProcessWrapper.cs Outdated
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 27, 2026

@copilot address my feedback

Both suggestions applied in 060c4a8: bare catch and target-typed new() for ManagementObjectSearcher.

Copilot AI requested a review from adamsitnik April 27, 2026 19:55
Copy link
Copy Markdown
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

LGTM, @vijayrkn the PR is ready for review

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