Skip to content

Add new process management APIs to SafeProcessHandle#124375

Open
Copilot wants to merge 41 commits intomainfrom
copilot/add-new-apis-to-safeprocesshandle
Open

Add new process management APIs to SafeProcessHandle#124375
Copilot wants to merge 41 commits intomainfrom
copilot/add-new-apis-to-safeprocesshandle

Conversation

Copy link
Contributor

Copilot AI commented Feb 13, 2026

Description

Implements the approved API surface for SafeProcessHandle (#123380): process lifecycle management via Start, Open, Kill, Signal, SignalProcessGroup, and WaitForExit overloads. Windows implementation is complete; Unix stubs throw NotImplementedException (separate PR).

SafeProcessHandle new public API

  • Open(int processId) — opens existing process handle
  • Start(ProcessStartOptions, SafeFileHandle?, SafeFileHandle?, SafeFileHandle?) — process creation with explicit stdio handles
  • Kill() / Signal(PosixSignal) / SignalProcessGroup(PosixSignal) — termination and signaling
  • WaitForExit(), TryWaitForExit(TimeSpan, out ProcessExitStatus?), WaitForExitOrKillOnTimeout(TimeSpan) — synchronous wait
  • WaitForExitAsync(CancellationToken), WaitForExitOrKillOnCancellationAsync(CancellationToken) — async wait

Internal for now (pending follow-up PRs): ProcessId, StartSuspended, Resume, KillProcessGroup.

Windows implementation

  • STARTUPINFOEX + PROC_THREAD_ATTRIBUTE_HANDLE_LIST for explicit handle inheritance
  • Job objects for KillOnParentExit and process group termination
  • OOM-safe handle cleanup in finally; NativeMemory.Alloc with overflow-safe two-arg form
  • ReaderWriterLockSlim (ProcessUtils.s_processStartLock) prevents accidental handle inheritance between Process.Start (write lock) and SafeProcessHandle.Start (read lock)
  • Interlocked.Exchange on _threadHandle prevents double-resume and use-after-free in ReleaseHandle

Architecture — ProcessUtils as shared dependency

  • ProcessUtils.cs holds s_processStartLock to avoid ProcessSafeProcessHandle dependency cycles
  • ProcessUtils.Windows.cs holds BuildArgs(string, IList<string>?) and GetEnvironmentVariablesBlock
  • ProcessStartOptions exposes HasArgumentsBeenAccessed / HasEnvironmentBeenAccessed to avoid allocations

Interop additions

  • Interop.JobObjects.cs, Interop.ProcThreadAttributeList.cs, Interop.ConsoleCtrl.cs (consolidated), Interop.HandleInformation.cs (extended), Interop.ResumeThread_IntPtr.cs

Tests

  • SafeProcessHandleTests.cs — start/kill/wait/timeout tests with Nano Server support (ping fallback)
  • SafeProcessHandleTests.Windows.cs — signal tests as [Theory] with [InlineData]
ProcessStartOptions options = new("cmd.exe") { Arguments = { "/c", "echo hello" } };
using SafeProcessHandle handle = SafeProcessHandle.Start(options, input: null, output: null, error: null);
ProcessExitStatus status = handle.WaitForExit();
// status.ExitCode == 0, status.Canceled == false

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 22 commits February 11, 2026 13:33
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
…, Theory for utils

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
…h for sh

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
…syntax

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
… remove dead code

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
…names

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 13, 2026
Copilot AI and others added 3 commits February 13, 2026 12:24
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
…s, and Windows implementation

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI changed the title [WIP] Add new APIs to SafeProcessHandle class Add new process management APIs to SafeProcessHandle Feb 13, 2026
Copy link
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

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

Copy link
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 please address my feedback

…Fact; add Nano Server support to CreateTenSecondSleep

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copy link
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

Copy link
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 please apply my suggestions to fix the tests that are failing only on Windows Nano

@tmds
Copy link
Member

tmds commented Feb 27, 2026

When CreateNewProcessGroup=true, the WaitForExitOrKillOn* methods only seem to kill the direct child. Is this the desired behavior?

Copy link
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 @tmds feedback by applying my suggestions

using Interop.Kernel32.ProcessWaitHandle processWaitHandle = new(this);
if (!processWaitHandle.WaitOne(milliseconds))
{
wasKilledOnTimeout = KillCore(throwOnError: false, entireProcessGroup: _processGroupJobHandle != IntPtr.Zero);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a test for the bug that was fixed here in the commit 776b250 ?

Copy link
Member

Choose a reason for hiding this comment

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

@copilot address this feedback, provide a test that spawns a process that spawns another process and ensure both run for a long time and get killed on timeout.

{
public partial class SafeProcessHandleTests
{
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsNanoServer))]
Copy link
Member

Choose a reason for hiding this comment

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

Why is this disabled on Nano?

The equivalent Process test does not appear to be disabled on Nano:

[ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
[MemberData(nameof(SignalTestData))]
public void TestCreateNewProcessGroup_HandlerReceivesExpectedSignal(PosixSignal signal)

Copy link
Member

Choose a reason for hiding this comment

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

The test you have referenced is conditional, I suspect RemoteExecutor.IsSupported is not supported on Nano.

Image

}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsNanoServer))]
public void Signal_SIGQUIT_TerminatesProcessInNewProcessGroup()
Copy link
Member

Choose a reason for hiding this comment

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

This and the previous tests only differ by PosixSignal number. They should use theory.

Also, I expect you are going to have the same test - just for more signals - for Unix too. Should it be in OS-neutral file?

Copy link
Member

Choose a reason for hiding this comment

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

@copilot address this feedback

Comment on lines 238 to 239
fixed (char* applicationNamePtr = &applicationName.GetPinnableReference())
fixed (char* commandLinePtr = &commandLine.GetPinnableReference())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fixed (char* applicationNamePtr = &applicationName.GetPinnableReference())
fixed (char* commandLinePtr = &commandLine.GetPinnableReference())
fixed (char* applicationNamePtr = applicationName)
fixed (char* commandLinePtr = commandLine)

Nit: I do not think you need an explicit GetPinnableReference reference call.

Copy link
Member

Choose a reason for hiding this comment

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

@copilot address this feedback

Comment on lines 241 to 254
bool retVal = Interop.Kernel32.CreateProcess(
applicationNamePtr,
commandLinePtr,
ref unused_SecAttrs,
ref unused_SecAttrs,
true,
creationFlags,
environmentBlockPtr,
workingDirectory,
ref startupInfoEx,
ref processInfo
);
if (!retVal)
errorCode = Marshal.GetLastPInvokeError();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bool retVal = Interop.Kernel32.CreateProcess(
applicationNamePtr,
commandLinePtr,
ref unused_SecAttrs,
ref unused_SecAttrs,
true,
creationFlags,
environmentBlockPtr,
workingDirectory,
ref startupInfoEx,
ref processInfo
);
if (!retVal)
errorCode = Marshal.GetLastPInvokeError();
if (!Interop.Kernel32.CreateProcess(
applicationNamePtr,
commandLinePtr,
ref unused_SecAttrs,
ref unused_SecAttrs,
true,
creationFlags,
environmentBlockPtr,
workingDirectory,
ref startupInfoEx,
ref processInfo))
{
errorCode = Marshal.GetLastPInvokeError();
}

Nit: Coding conventions for { }

Copy link
Member

Choose a reason for hiding this comment

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

@copilot address this feedback

Copy link
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 please address the feedback

Comment on lines 238 to 239
fixed (char* applicationNamePtr = &applicationName.GetPinnableReference())
fixed (char* commandLinePtr = &commandLine.GetPinnableReference())
Copy link
Member

Choose a reason for hiding this comment

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

@copilot address this feedback

Comment on lines 241 to 254
bool retVal = Interop.Kernel32.CreateProcess(
applicationNamePtr,
commandLinePtr,
ref unused_SecAttrs,
ref unused_SecAttrs,
true,
creationFlags,
environmentBlockPtr,
workingDirectory,
ref startupInfoEx,
ref processInfo
);
if (!retVal)
errorCode = Marshal.GetLastPInvokeError();
Copy link
Member

Choose a reason for hiding this comment

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

@copilot address this feedback

using Interop.Kernel32.ProcessWaitHandle processWaitHandle = new(this);
if (!processWaitHandle.WaitOne(milliseconds))
{
wasKilledOnTimeout = KillCore(throwOnError: false, entireProcessGroup: _processGroupJobHandle != IntPtr.Zero);
Copy link
Member

Choose a reason for hiding this comment

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

@copilot address this feedback, provide a test that spawns a process that spawns another process and ensure both run for a long time and get killed on timeout.

{
public partial class SafeProcessHandleTests
{
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsNanoServer))]
Copy link
Member

Choose a reason for hiding this comment

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

The test you have referenced is conditional, I suspect RemoteExecutor.IsSupported is not supported on Nano.

Image

}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsNanoServer))]
public void Signal_SIGQUIT_TerminatesProcessInNewProcessGroup()
Copy link
Member

Choose a reason for hiding this comment

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

@copilot address this feedback

@jkotas
Copy link
Member

jkotas commented Feb 27, 2026

The test you have referenced is conditional, I suspect RemoteExecutor.IsSupported is not supported on Nano

It would be a surprise to me. This is the implementation of the condition: https://github.com/dotnet/arcade/blob/55c97a45af8fd75bc87e9e4b4f61aef3fead254d/src/Microsoft.DotNet.RemoteExecutor/src/RemoteExecutor.cs#L110-L118 . I do not see anything that would disable it on Nano.

There are subtle behavior difference between full Windows and Nano in how to signals behave (see #115206 (comment)), but as far as I know the signals generally work just fine on Nano.

If I had to guess, it may be a problem with the process group emulator via Windows jobs.

@adamsitnik
Copy link
Member

adamsitnik commented Feb 27, 2026

If I had to guess, it may be a problem with the process group emulator via Windows jobs.

The other test (CreateNewProcessGroup_CanBeSetToTrue) uses process group and works fine on Nano. The emulation is very simple, it's just one Job created and assigned to process.

When I tried enabling Windows signals on Nano, I was getting ERROR_INVALID_FUNCTION (in the CI).

And the test that you have referenced is silently swallowing this error:

if (!Interop.GenerateConsoleCtrlEvent(dwCtrlEvent, (uint)processId))
{
int error = Marshal.GetLastWin32Error();
if (error == Interop.Errors.ERROR_INVALID_FUNCTION && PlatformDetection.IsInContainer)
{
// Docker in CI runs without a console attached.
throw new SkipTestException($"GenerateConsoleCtrlEvent failed with ERROR_INVALID_FUNCTION. The process is not a console process or does not have a console.");
}

…PinnableReference, fix CreateProcess braces

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants