Implement ProcessStartInfo.KillOnParentExit for Windows

[Copilot]

Description

Implements the ProcessStartInfo.KillOnParentExit property for Windows, which terminates child processes when the parent process exits. This is the Windows part of the KillOnParentExit feature.

API approved at #125838 (comment)

Changes Made

New API surface:

• Added KillOnParentExit property to ProcessStartInfo with [SupportedOSPlatform("windows")] attribute (linux support will be added in upcoming PRs)
• Added to reference assembly (ref/System.Diagnostics.Process.cs) with matching [SupportedOSPlatformAttribute("windows")]
• Added XML documentation with a link to the Windows SDK description of JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE

Windows implementation (SafeProcessHandle.Windows.cs):

• Added a lazily-initialized static SafeJobHandle with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE flag, shared across all child processes with KillOnParentExit=true
• For CreateProcess path (no user credentials): uses PROC_THREAD_ATTRIBUTE_JOB_LIST in the extended startup info for atomic job assignment at process creation
• For CreateProcessWithLogonW path (with user credentials): starts the process suspended, assigns it to the job object via AssignProcessToJobObject, then resumes the thread only on the happy path — on failure, the suspended process is terminated
• ResumeThread return value is checked; failure throws Win32Exception and the suspended process is terminated
• AssignJobAndResumeThread takes (IntPtr hThread, SafeProcessHandle procSH) and uses procSH consistently for all process handle operations (no mixing of processInfo.hProcess and procSH)
• Extended BuildProcThreadAttributeList to support both PROC_THREAD_ATTRIBUTE_HANDLE_LIST and PROC_THREAD_ATTRIBUTE_JOB_LIST attributes
• Uses string.IsNullOrEmpty(startInfo.UserName) for defensive checks when determining the code path

P/Invoke definitions:

• Interop.JobObjects.cs: CreateJobObjectW, SetInformationJobObject, AssignProcessToJobObject (accepts SafeProcessHandle), JOBOBJECT_EXTENDED_LIMIT_INFORMATION, IO_COUNTERS, JOBOBJECTINFOCLASS structs/enums, SafeJobHandle, PROC_THREAD_ATTRIBUTE_JOB_LIST constant
• Interop.ResumeThread.cs: ResumeThread P/Invoke returning uint (matching native DWORD) in its own dedicated file (following the pattern of Interop.TerminateProcess.cs, Interop.GetExitCodeProcess.cs, etc.)
• IO_COUNTERS and JOBOBJECT_EXTENDED_LIMIT_INFORMATION use nuint for fields representing numerical values (sizes, limits, affinity masks) per interop convention

Validation:

• Extended ProcessStartInfo.ThrowIfInvalid to throw InvalidOperationException when KillOnParentExit is combined with UseShellExecute
• Added KillOnParentExitCannotBeUsedWithUseShellExecute string resource

Tests (KillOnParentExitTests.cs):

• KillOnParentExit_DefaultsToFalse / KillOnParentExit_CanBeSetToTrue — property basics
• KillOnParentExit_WithUseShellExecute_Throws — validation test
• KillOnParentExit_ProcessStartsAndExitsNormally — smoke test
• KillOnParentExit_KillsTheChild_WhenParentExits — verifies child is killed (enabled=true) or survives (enabled=false) on normal parent exit, reports grandchild pid via stdout
• KillOnParentExit_KillsTheChild_WhenParentIsKilled — same when parent is forcefully killed
• KillOnParentExit_KillsTheChild_WhenParentCrashes — same when parent crashes (access violation)
• User credentials code path is exercised via the updated ProcessStartInfoTests.TestUserCredentialsPropertiesOnWindows (admin/OuterLoop)

[adamsitnik]

@copilot address my feedback

[Copilot]

@copilot address my feedback

All feedback addressed in 31f6e6b. All 403 tests pass.

[Copilot]

Pull request overview

Implements Windows support for ProcessStartInfo.KillOnParentExit by using Windows Job Objects to ensure spawned processes are terminated when the parent exits.

Changes:

• Added ProcessStartInfo.KillOnParentExit API surface (implementation + ref assembly) and validation against UseShellExecute.
• Implemented Windows job-object based process start paths (CreateProcess job-list attribute, and CreateProcessWithLogonW suspended + assign + resume).
• Added Windows-focused tests validating the behavior.

Reviewed changes

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

Show a summary per file

File	Description
src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj	Includes the new KillOnParentExit test file in the test project.
src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs	Extends existing Windows credential tests to cover KillOnParentExit on that path.
src/libraries/System.Diagnostics.Process/tests/KillOnParentExitTests.cs	Adds new Windows tests validating KillOnParentExit behavior under various parent-exit scenarios.
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.cs	Adds the KillOnParentExit property, docs, and validation with UseShellExecute.
src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj	Links in the new Windows job-object interop file.
src/libraries/System.Diagnostics.Process/src/Resources/Strings.resx	Adds a resource string for the KillOnParentExit + UseShellExecute validation.
src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs	Windows implementation: shared job handle, job-list attribute support, and logon path job assignment.
src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs	Adds the KillOnParentExit property to the reference assembly surface.
src/libraries/Common/src/Interop/Windows/Kernel32/Interop.JobObjects.cs	Adds required job-object P/Invokes/structs/constants.
src/libraries/Common/src/Interop/Windows/Advapi32/Interop.ProcessOptions.cs	Adds CREATE_SUSPENDED for the CreateProcessWithLogonW suspended-start path.

[adamsitnik]

@copilot address my feedback

[Copilot]

@copilot address my feedback

All feedback addressed in cc04598. Source builds and all 401 tests pass.

[jkotas]

This will clear the env variable for the child process. Where is the code in the child process that is going to read it?

[Copilot]

No code in the child reads it — removed the line in 04d67a9.

[adamsitnik]

@jkotas after reading RemoteExecutor code again, I believe that it's going to capture the dump only if the child process does not exit on it's own before RemoteInvokeHandle gets disposed/finalized:

https://github.com/dotnet/arcade/blob/d3eca2571cf5822e11fda70f12747cb3f2c19cfc/src/Microsoft.DotNet.RemoteExecutor/src/RemoteInvokeHandle.cs#L110

The child process crashes, so it's not a problem.
The grand child is created with CreateProcessLong, which uses this method to create it:

runtime/src/libraries/System.Diagnostics.Process/tests/ProcessTestBase.cs

Lines 95 to 102 in 280fb74

	protected Process CreateProcess(Func<string, string, int> method, string arg1, string arg2, bool autoDispose = true)
	{
	Process p = null;
	using (RemoteInvokeHandle handle = RemoteExecutor.Invoke(method, arg1, arg2, new RemoteInvokeOptions { Start = false }))
	{
	p = handle.Process;
	handle.Process = null;
	}

Since it disposes RemoteInvokeHandle, I currently don't see a way to create a dump for this test. I am going to open a RemoteExecutor issue and extend it with an ability to disable it properly via the option bag.

[adamsitnik]

I've added the defensive env var back and added it to Outerloop based on feedback I got offline

[Copilot]

The test failures look related to the changes.

I'm unable to access the AzDO pipeline logs from my environment. Could you share the specific test failure details? The code changes in this commit (4fdc31a) address the other review feedback — if the test failures are related to the nuint/consistency issues, they should be resolved now.

[adamsitnik]

@copilot address my feedback

[Copilot]

@copilot address my feedback

Applied the IsInvalidHandle suggestion in 20559e0.

[adamsitnik]

The test failures look related to the changes.

This test is most likely flaky (sending signals on Windows...)

I've introduced it in #126632 and it has failed once there

The logs:

    System.Diagnostics.Tests.ProcessTests.StartDetached_GrandchildSurvivesSignalingParent(enable: False) [FAIL]
      Assert.Equal() Failure: Values differ
      Expected: False
      Actual:   True
      Stack Trace:
        /_/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs(367,0): at System.Diagnostics.Tests.ProcessTests.StartDetached_GrandchildSurvivesSignalingParent(Boolean enable)
           at InvokeStub_ProcessTests.StartDetached_GrandchildSurvivesSignalingParent(Object, Span`1)
        /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.cs(95,0): at System.Reflection.MethodBaseInvoker.InvokeWithOneArg(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
Invalid number of parameters

I was unable to reproduce it locally and after a long debate with few models I got to the conclusion that it could be caused by PID reuse, so I've pushed this fix. The CI was ofc green, but it does not mean that the test is not flaky.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs:524

• If InitializeProcThreadAttributeList(attributeListBuffer, ...) fails, attributeListBuffer is still non-null and the finally block will call DeleteProcThreadAttributeList on a buffer that was never successfully initialized. That’s undefined per Win32 contract and could lead to AVs in low-memory/error paths. Consider only assigning to attributeListBuffer after successful initialization (or track an initialized flag) so cleanup calls DeleteProcThreadAttributeList only when initialization succeeded; otherwise just free the allocated memory.

            nuint size = 0;
            Interop.Kernel32.InitializeProcThreadAttributeList(null, attributeCount, 0, ref size);
            attributeListBuffer = NativeMemory.Alloc(size);

            if (!Interop.Kernel32.InitializeProcThreadAttributeList(attributeListBuffer, attributeCount, 0, ref size))
            {
                throw new Win32Exception(Marshal.GetLastWin32Error());
            }

[github-actions]

🤖 Copilot Code Review — PR #126699

Note

This review was AI/Copilot-generated.

Holistic Assessment

Motivation: Justified — these are code review follow-up fixes addressing maintainer feedback (jkotas) on the recently-merged KillOnParentExit feature. Each change improves correctness, safety, or code organization.

Approach: Sound — all changes are incremental improvements that align with dotnet/runtime conventions: SafeHandle-based P/Invoke parameters, correct unsigned return types for Windows DWORD APIs, modern nuint syntax, and per-function interop file separation.

Summary: ✅ LGTM. All changes are correct and well-motivated. No new public API surface. No correctness, safety, or performance concerns found. Reviewed by Claude Opus 4.6 and Claude Sonnet 4.5 (GPT-5.4 also consulted but did not complete in time).

---

Detailed Findings

✅ SafeProcessHandle for AssignProcessToJobObject — Correctness improvement

Changing AssignProcessToJobObject from IntPtr hProcess to SafeProcessHandle hProcess is a genuine safety improvement. The old code passed processInfo.hProcess as a raw IntPtr after Marshal.InitHandle(procSH, processInfo.hProcess) had already transferred ownership to procSH. Using SafeProcessHandle ensures the marshaller calls DangerousAddRef/DangerousRelease, preventing the handle from being collected during the P/Invoke. The only consumer of this shared interop declaration is System.Diagnostics.Process (verified via csproj search), so no other callers are broken.

✅ ResumeThread return type and error check — Bug fix

The Windows ResumeThread API returns DWORD (unsigned 32-bit), with failure indicated by (DWORD)-1 = 0xFFFFFFFF. The old code used int return type with == -1 check, which happened to work due to bit pattern equivalence, but was technically incorrect for a DWORD-returning API. The new uint return with == 0xFFFFFFFF is correct per the Windows API specification.

✅ ResumeThread in separate file — No duplicate concern

Interop.ResumeThread.cs (uint ResumeThread(IntPtr)) is compiled only into System.Diagnostics.Process. The existing Interop.Threading.cs (uint ResumeThread(SafeWaitHandle)) is compiled only into System.Private.CoreLib. They never coexist in the same compilation unit, so there is no duplicate or conflict.

✅ UIntPtr → nuint — Clean modernization

nuint is a keyword alias for UIntPtr in modern C#. Same type, same layout, same marshalling. Consistent with the codebase's gradual modernization.

✅ AssignJobAndResumeThread signature — Cleaner interface

Passing only IntPtr hThread instead of the full PROCESS_INFORMATION struct is appropriate since the method only needs the thread handle (the process handle now comes from procSH).

✅ Test changes — All appropriate

• [SkipOnPlatform(iOS | tvOS)] removal: Correctly identified as redundant — the test class is already [PlatformSpecific(TestPlatforms.Windows)].
• [OuterLoop("May create a memory dump")]: Appropriate for a crash test that intentionally triggers an access violation.
• RedirectStandardError = true removal: Correct — the test never reads from StandardError.
• HELIX_WORKITEM_UPLOAD_ROOT = null: Setting to null removes the env var from the child process, disabling Helix dump upload for this intentional-crash test.
• using System.Threading removal: Correct — no longer used after prior refactoring.

💡 Out-of-scope: Test ResumeThread inconsistency

ProcessHandlesTests.Windows.cs (line 187) has a local private static partial int ResumeThread(nint hThread) with == -1 check. This is technically correct (same bit pattern) but inconsistent with the production code's uint/0xFFFFFFFF pattern. Not a bug, and out of scope for this PR, but could be cleaned up in a future pass.

Generated by Code Review for issue #126699 · ◷