Implement ProcessStartInfo.KillOnParentExit for Windows#126699
Implement ProcessStartInfo.KillOnParentExit for Windows#126699
Conversation
Add KillOnParentExit property to ProcessStartInfo that terminates child processes when the parent process exits. Windows implementation uses Job Objects with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE flag. For CreateProcess: uses PROC_THREAD_ATTRIBUTE_JOB_LIST for atomic assignment. For CreateProcessWithLogonW: starts suspended, assigns to job, then resumes. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/61612ca1-930c-4836-8554-d143d05c8321 Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-process |
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address my feedback
...ries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs
Outdated
Show resolved
Hide resolved
...ries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs
Outdated
Show resolved
Hide resolved
...ries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/KillOnParentExitTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/KillOnParentExitTests.cs
Outdated
Show resolved
Hide resolved
…py path, add user credentials test, fix pid reporting via stdout, remove nop UseShellExecute test Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/c4dd95b4-cd8e-4740-83f9-7358444c04ce Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
- simplify the tests - avoid duplication!!
There was a problem hiding this comment.
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.KillOnParentExitAPI surface (implementation + ref assembly) and validation againstUseShellExecute. - 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. |
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.cs
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs
Show resolved
Hide resolved
...ries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs
Outdated
Show resolved
Hide resolved
...ries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/KillOnParentExitTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/KillOnParentExitTests.cs
Outdated
Show resolved
Hide resolved
🤖 Copilot Code Review — PR #126699Note This review was generated by GitHub Copilot. Holistic AssessmentMotivation: Adding Approach: The Windows implementation using Job Objects with Summary: Detailed Findings
|
| Property | [SupportedOSPlatform("windows")] |
Throws PNSE on Unix |
|---|---|---|
CreateNewProcessGroup |
✅ Yes (both ref and src) | ✅ Yes |
LoadUserProfile |
✅ Yes | ✅ Yes |
Domain |
✅ Yes | ✅ Yes |
Password |
✅ Yes | ✅ Yes |
KillOnParentExit |
❌ No | ❌ No |
InheritedHandles |
❌ No (cross-platform) | N/A (implemented on both) |
The XML documentation says "On Windows, this is implemented using Job Objects" — which could imply cross-platform support is planned (Linux has prctl(PR_SET_PDEATHSIG), macOS has kqueue with EVFILT_PROC). If that's the intent, the current behavior (silently ignored on Unix) is reasonable as a stepping stone. If Unix support is not planned, the property should follow the established pattern with [SupportedOSPlatform("windows")] and throw PlatformNotSupportedException on Unix.
Action: A human reviewer should confirm the intended cross-platform strategy. If Unix support is planned, a tracking issue should exist. If not, the property should be decorated with [SupportedOSPlatform("windows")] and throw PNSE on Unix via ProcessStartInfo.Unix.cs.
💡 ResumeThread Return Value — Not checked
In AssignJobAndResumeThread (SafeProcessHandle.Windows.cs line 608):
Interop.Kernel32.ResumeThread(processInfo.hThread);ResumeThread returns (DWORD)-1 on failure. If it fails, the child process remains suspended indefinitely. While this is extremely unlikely with a valid thread handle from CreateProcessWithLogonW, checking the return value and handling the error (terminate the suspended process) would be more defensive:
if (Interop.Kernel32.ResumeThread(processInfo.hThread) == -1)
{
Interop.Kernel32.TerminateProcess(procSH, -1);
throw new Win32Exception(Marshal.GetLastWin32Error());
}This is a low-probability concern but aligns with the defensive error handling already present in this method.
✅ Windows Implementation — Correct and well-structured
The implementation handles both code paths correctly:
CreateProcesspath: UsesPROC_THREAD_ATTRIBUTE_JOB_LISTfor atomic job assignment — the child process is in the job from its very first instruction. No window for orphaning.CreateProcessWithLogonWpath: Correctly usesCREATE_SUSPENDED→AssignProcessToJobObject→ResumeThread. The error recovery (TerminateProcessin the catch block) ensures no zombie suspended processes are left behind.- Singleton job object:
Lazy<T>provides thread-safe, once-only initialization. TheSafeJobHandleis intentionally never disposed (rooted by the static field) — this is correct since the OS automatically destroys the job and kills its processes when the parent exits. DangerousGetHandle(): Safe here because theLazy<T>roots theSafeJobHandlefor the process lifetime, so GC cannot collect it.- Thread handle closure: Correctly moved to the
finallyblock to ensure cleanup even whenAssignJobAndResumeThreadthrows.
✅ Interop Definitions — Correct
The Job Object structs (IO_COUNTERS, JOBOBJECT_BASIC_LIMIT_INFORMATION, JOBOBJECT_EXTENDED_LIMIT_INFORMATION) are correctly laid out with [StructLayout(LayoutKind.Sequential)]. P/Invoke signatures use SetLastError = true and [return: MarshalAs(UnmanagedType.Bool)] where appropriate. The SafeJobHandle correctly extends SafeHandleZeroOrMinusOneIsInvalid.
✅ Validation Logic — Correct
The ThrowIfInvalid method properly prevents combining KillOnParentExit with UseShellExecute. The useExtendedStartupInfo condition correctly avoids using extended startup info when UserName is set (since CreateProcessWithLogonW doesn't support it), falling back to the suspended-process approach.
✅ Test Coverage — Comprehensive for Windows
Tests cover:
- Default value (
false) and set/get UseShellExecuteincompatibility (throwsInvalidOperationException)- Normal process start and exit with
KillOnParentExit=true - Child killed when parent exits normally (
WhenParentExits) - Child killed when parent is killed (
WhenParentIsKilled) - Child killed when parent crashes via access violation (
WhenParentCrashes) - Both
enabled=trueandenabled=falsepaths via[Theory]/[InlineData] - User credentials path (
TestUserCredentialsPropertiesOnWindows) — tests theCreateProcessWithLogonWsuspended-process code path
All tests are correctly marked [PlatformSpecific(TestPlatforms.Windows)].
💡 Lazy(T) Exception Caching — Acceptable but worth noting
If CreateJobObjectW or SetInformationJobObject fails during the first KillOnParentExit process start, the Lazy<T> default mode (ExecutionAndPublication) caches the exception permanently. All subsequent KillOnParentExit=true starts will fail with the same cached exception, even if the underlying system condition was transient. This is acceptable since these calls only fail under extreme system conditions (out of handles/memory), where retrying is unlikely to help.
Generated by Code Review for issue #126699 · ◷
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address my feedback
src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs
Show resolved
Hide resolved
...ries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs
Show resolved
Hide resolved
...ries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.cs
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/KillOnParentExitTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/KillOnParentExitTests.cs
Outdated
Show resolved
Hide resolved
…sumeThread return, remove unused using, improve VerifyProcessIsRunning polling Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/06c58cfa-bdef-4f35-9345-9ab03e7dc542 Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
| /// </remarks> | ||
| /// <value><see langword="true"/> to terminate the child process when the parent exits; otherwise, <see langword="false"/>. The default is <see langword="false"/>.</value> | ||
| [SupportedOSPlatform("windows")] | ||
| public bool KillOnParentExit { get; set; } |
There was a problem hiding this comment.
to other reviewers: I did not bother throwing PNSE right now as I intend to add Unix support in upcoming PR
| VerifyProcessIsRunning(enabled, grandChildPid); | ||
| } | ||
|
|
||
| private static void VerifyProcessIsRunning(bool shouldBeKilled, int processId) |
There was a problem hiding this comment.
to other reviewers: I've opened #126727 to make it easier to perform similar checks
|
/azp list |
|
/azp run runtime-libraries-coreclr outerloop-windows |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| Debug.Assert(!hasInheritedHandles, "Inheriting handles is not supported when starting with alternate credentials."); | ||
| Debug.Assert(startupInfoEx.StartupInfo.cb == sizeof(Interop.Kernel32.STARTUPINFO)); | ||
|
|
||
| // When KillOnParentExit is set and we use CreateProcessWithLogonW (which doesn't support |
| /// This property cannot be used together with <see cref="UseShellExecute"/> set to <see langword="true"/>. | ||
| /// </para> | ||
| /// <para> | ||
| /// On Windows, this is implemented using Job Objects with the <c>JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE</c> flag. |
There was a problem hiding this comment.
Add a link to Windows SDK description of this flag
| } | ||
|
|
||
| if (handleCount > 0 && !Interop.Kernel32.UpdateProcThreadAttribute( | ||
| if (handlesToInherit is not null && handleCount > 0 && !Interop.Kernel32.UpdateProcThreadAttribute( |
There was a problem hiding this comment.
| if (handlesToInherit is not null && handleCount > 0 && !Interop.Kernel32.UpdateProcThreadAttribute( | |
| if (handleCount > 0 && !Interop.Kernel32.UpdateProcThreadAttribute( |
This looks unnecessary
|
|
||
| string firstLine = childHandle.Process.StandardOutput.ReadLine(); | ||
| int grandChildPid = int.Parse(firstLine); | ||
| childHandle.Process.StandardInput.WriteLine("One AccessViolationException please."); |
There was a problem hiding this comment.
Are these intentionally crashing processes going to produce crash dumps? We tend to have a problem with these. The crash dumps fill disk on Helix machines.
See #113732
Do we need to do something similar here?
Description
Implements the
ProcessStartInfo.KillOnParentExitproperty for Windows, which terminates child processes when the parent process exits. This is the Windows part of theKillOnParentExitfeature.API approved at #125838 (comment)
Changes Made
New API surface:
KillOnParentExitproperty toProcessStartInfowith[SupportedOSPlatform("windows")]attribute (linux support will be added in upcoming PRs)ref/System.Diagnostics.Process.cs) with matching[SupportedOSPlatformAttribute("windows")]Windows implementation (
SafeProcessHandle.Windows.cs):SafeJobHandlewithJOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSEflag, shared across all child processes withKillOnParentExit=trueCreateProcesspath (no user credentials): usesPROC_THREAD_ATTRIBUTE_JOB_LISTin the extended startup info for atomic job assignment at process creationCreateProcessWithLogonWpath (with user credentials): starts the process suspended, assigns it to the job object viaAssignProcessToJobObject, then resumes the thread only on the happy path — on failure, the suspended process is terminatedResumeThreadreturn value is checked; failure throwsWin32Exceptionand the suspended process is terminatedBuildProcThreadAttributeListto support bothPROC_THREAD_ATTRIBUTE_HANDLE_LISTandPROC_THREAD_ATTRIBUTE_JOB_LISTattributesstring.IsNullOrEmpty(startInfo.UserName)for defensive checks when determining the code pathP/Invoke definitions (
Interop.JobObjects.cs):CreateJobObjectW,SetInformationJobObject,AssignProcessToJobObject,ResumeThreadJOBOBJECT_EXTENDED_LIMIT_INFORMATION,IO_COUNTERS,JOBOBJECTINFOCLASSstructs/enumsSafeJobHandle,PROC_THREAD_ATTRIBUTE_JOB_LISTconstantValidation:
ProcessStartInfo.ThrowIfInvalidto throwInvalidOperationExceptionwhenKillOnParentExitis combined withUseShellExecuteKillOnParentExitCannotBeUsedWithUseShellExecutestring resourceTests (
KillOnParentExitTests.cs):KillOnParentExit_DefaultsToFalse/KillOnParentExit_CanBeSetToTrue— property basicsKillOnParentExit_WithUseShellExecute_Throws— validation testKillOnParentExit_ProcessStartsAndExitsNormally— smoke testKillOnParentExit_KillsTheChild_WhenParentExits— verifies child is killed (enabled=true) or survives (enabled=false) on normal parent exit, reports grandchild pid via stdoutKillOnParentExit_KillsTheChild_WhenParentIsKilled— same when parent is forcefully killedKillOnParentExit_KillsTheChild_WhenParentCrashes— same when parent crashes (access violation)KillOnParentExit_WithUserCredentials— exercises theCreateProcessWithLogonW+ suspended + job assignment path using a Windows test account (admin/OuterLoop)VerifyProcessIsRunninguses polling withEnvironment.TickCount64and a 10-second timeout to avoid flaky assertions from PID reuse on Windows