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
• 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, ResumeThread
• JOBOBJECT_EXTENDED_LIMIT_INFORMATION, IO_COUNTERS, JOBOBJECTINFOCLASS structs/enums
• SafeJobHandle, PROC_THREAD_ATTRIBUTE_JOB_LIST constant

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)
• KillOnParentExit_WithUserCredentials — exercises the CreateProcessWithLogonW + suspended + job assignment path using a Windows test account (admin/OuterLoop)
• VerifyProcessIsRunning uses polling with Environment.TickCount64 and a 10-second timeout to avoid flaky assertions from PID reuse on Windows

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

[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.

[adamsitnik]

/azp run runtime-libraries-coreclr outerloop-windows

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jkotas]

When I run this locally on Windows, no crush dump is being generated. But once we enable it on Unix (upcoming PR), then it's going to become a real problem.

Dump generation on crashes is an environment or machine-wide setting that is enabled on Helix machines. If you do not see crash dumps being generated locally, it says nothing about what's going to happen in Helix.

[adamsitnik]

When I run this locally on Windows, no crush dump is being generated. But once we enable it on Unix (upcoming PR), then it's going to become a real problem.

Dump generation on crashes is an environment or machine-wide setting that is enabled on Helix machines. If you do not see crash dumps being generated locally, it says nothing about what's going to happen in Helix.

Thanks, I was not aware of that as I can see these dumps being generated on Linux when I run the tests locally

[adamsitnik]

@jkotas I added that after reading https://github.com/dotnet/arcade/blob/d3eca2571cf5822e11fda70f12747cb3f2c19cfc/src/Microsoft.DotNet.RemoteExecutor/src/RemoteInvokeHandle.cs#L152

I've also asked offline on teams to ensure this is the right approach

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

[adamsitnik]

/azp run runtime-libraries-coreclr outerloop-windows

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

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

• BuildProcThreadAttributeList can be called with attributeCount == 0 (e.g., InheritedHandles is non-null but handleCount ends up 0 and killOnParentExit is false). In that case we still call InitializeProcThreadAttributeList and allocate a buffer even though no attributes will be added; on some Windows versions this call may fail with an invalid-parameter error. Consider short-circuiting when attributeCount == 0 (leave attributeListBuffer null and skip initialization), and only set EXTENDED_STARTUPINFO_PRESENT / STARTUPINFOEX when there is at least one attribute to apply.

        private static unsafe void BuildProcThreadAttributeList(
            IntPtr* handlesToInherit,
            int handleCount,
            bool killOnParentExit,
            int attributeCount,
            ref void* attributeListBuffer,
            ref IntPtr* jobHandles)
        {
            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 generated by Copilot (Claude Opus 4.6), with additional analysis from GPT-5.2.

Holistic Assessment

Motivation: The feature is well-motivated — reliably terminating child processes when the parent exits is a common requirement (build systems, test harnesses, daemon supervisors). Using Windows Job Objects with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE is the idiomatic Windows mechanism for this.

Approach: The implementation is well-structured with two code paths: atomic PROC_THREAD_ATTRIBUTE_JOB_LIST for CreateProcess, and CREATE_SUSPENDED + assign + resume for CreateProcessWithLogonW. The shared static Lazy<SafeJobHandle> is an efficient design. Error handling properly terminates suspended processes on failure.

Summary: ⚠️ Needs Human Review. The implementation is solid and well-tested, but I could not find evidence in the commit messages that the new public API has gone through API review (no linked api-approved issue). A human reviewer should confirm API approval status. I also have a few lower-severity findings below.

---

Detailed Findings

⚠️ API Approval — No linked api-approved issue found

This PR adds new public API surface:

[SupportedOSPlatform("windows")]
public bool KillOnParentExit { get { throw null; } set { } }

Per dotnet/runtime conventions, new public APIs require a linked issue with the api-approved label. I could not find any issue reference in the commit messages or PR branch. The PR author (adamsitnik) is a dotnet team member who likely has or is pursuing approval, but a reviewer should confirm.

Merge-blocking? I'm not sure — a human reviewer should verify.

✅ Correctness — Job Object pattern is correct

• JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE is the correct flag for this use case.
• The job handle being intentionally never closed is correct — its OS destruction on parent exit triggers child termination.
• PROC_THREAD_ATTRIBUTE_JOB_LIST provides atomic job assignment for CreateProcess (no race window).
• CREATE_SUSPENDED + AssignProcessToJobObject + ResumeThread is the standard workaround for CreateProcessWithLogonW, and the catch block properly calls TerminateProcess to clean up on failure.
• Error codes are captured via Marshal.GetLastWin32Error() before any Dispose() calls — correct ordering.
• When CreateProcessWithLogonW fails (retVal is false), the !IsInvalidHandle(processInfo.hProcess) check (line 284) correctly prevents AssignJobAndResumeThread from being called.

✅ Thread Safety — Lazy<SafeJobHandle> is appropriate

The static Lazy<SafeJobHandle> provides thread-safe initialization. The DangerousGetHandle() call in BuildProcThreadAttributeList is safe because the job handle is process-lifetime and never disposed (by design — the OS destroys it when the process exits).

✅ Error Handling — Suspended process cleanup is well-handled

AssignJobAndResumeThread correctly catches exceptions and terminates the suspended process before rethrowing. The finally block in StartCore properly closes processInfo.hThread and cleans up native memory allocations (handlesToInherit, jobHandles, attributeListBuffer).

✅ Interop Declarations — Correct

• JOBOBJECT_BASIC_LIMIT_INFORMATION and JOBOBJECT_EXTENDED_LIMIT_INFORMATION use UIntPtr for pointer-sized fields (MinimumWorkingSetSize, Affinity, etc.), which is correct for both x86 and x64.
• LibraryImport with SetLastError = true on all relevant P/Invoke calls.
• [return: MarshalAs(UnmanagedType.Bool)] on boolean-returning functions.
• PROC_THREAD_ATTRIBUTE_JOB_LIST = 0x0002000D matches the Windows SDK value.

⚠️ Non-Windows Behavior — Silent no-op (consistent with existing patterns)

When KillOnParentExit = true is set on a non-Windows platform, it is silently ignored since the Unix StartCore never reads the property. The [SupportedOSPlatform("windows")] attribute provides an analyzer warning but doesn't prevent runtime usage. Linux has prctl(PR_SET_PDEATHSIG) which could implement this in the future.

This is consistent with other Windows-only properties like LoadUserProfile, so I don't consider it a problem. Flagging for visibility only.

Merge-blocking? No — consistent with existing conventions.

💡 Typo — limitInherianceStr in test lambdas

In KillOnParentExitTests.cs, the lambda parameter is spelled limitInherianceStr (missing 't') in two test methods, while the first method correctly uses limitInheritanceStr:

Line	Variable	Status
66	limitInheritanceStr	✅ correct
97	limitInherianceStr	❌ typo (missing 't')
135	limitInherianceStr	❌ typo (missing 't')

Merge-blocking? No — local variable name, doesn't affect behavior.

✅ Test Quality — Comprehensive coverage

The test suite is well-designed:

• Default value, setter, and validation (UseShellExecute conflict) tests
• Three kill scenarios: parent exits normally, parent is killed, parent crashes (via Marshal.WriteInt32(IntPtr.Zero, 42))
• [Theory] with [InlineData] testing combinations of enabled x restrictInheritance
• HELIX_WORKITEM_UPLOAD_ROOT cleared for the crash test to avoid spurious dump uploads
• VerifyProcessIsRunning uses Environment.TickCount64-based polling — correct for timeout calculations
• Existing tests extended with killOnParentExit parameter (TestUserCredentialsPropertiesOnWindows, ProcessStartedWithInvalidHandles_CanStartChildProcessWithDerivedInvalidHandles)
• Related test improvements: [Fact] to [ConditionalFact] and [SkipOnPlatform] additions for iOS/tvOS/MacCatalyst

💡 Test Timeout — 10s may be tight on loaded CI

The VerifyProcessIsRunning method uses a 10-second timeout for waiting for child process termination. On loaded Helix machines, job object cleanup after parent termination could potentially take longer. The dotnet/runtime testing guidelines suggest generous timeouts for stress pipelines. This is a low-probability concern but worth noting.

---

Review models: Claude Opus 4.6 (primary), GPT-5.2 (secondary). Both models agreed on the key findings.

Generated by Code Review for issue #126699 · ◷

[adamsitnik]

/azp run runtime-libraries-coreclr outerloop-windows

[azure-pipelines]

Azure Pipelines failed to run 1 pipeline(s).

[jkotas]

Should this be in BuildProcThreadAttributeList since it is coupled with the logic in that method?

[jkotas]

These is a mismatch between this condition and the condition used in BuildProcThreadAttributeList. This condition uses restrictInheritedHandles, but the same condition in BuildProcThreadAttributeList does not. I am not able to tell whether it is a real problem, but the two conditions should be identical.