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

@copilot please address my feedback.

Moreover, please modify the .yml file that is required to let the CI run for this branch. I am not an expert in this area, but I think it should be enough to mo...

All feedback addressed and copilot/*.* added to the CI branch triggers in eng/pipelines/runtime.yml in 28845d4.

@copilot please address all unresolved feedback

All unresolved feedback addressed in cdd13b0: simplified OpenProcess to kill(pid, 0), removed SIGSTOP from signal mappings, and fixed pwd test to be platform-specific.

@copilot please give my suggestion a try.

Done in 4b02d2e — replaced all HAVE_POSIX_SPAWN* feature detection with #ifdef __APPLE__ platform checks and removed the cmake/config defines.

@copilot please address my feedback

Also, annotate the Open_CurrentProcess_Succeeds test with [PlatformSpecific(TestPlatforms.Windows)]

Annotated Open_CurrentProcess_Succeeds with [PlatformSpecific(TestPlatforms.Windows)] and addressed all feedback. Done in 300ba39.

@copilot please apply my suggestion

Applied in 2240ff7.

@copilot please invoke the code-review skill and post the analysis/results as a comment on this PR

@copilot please invoke the code-review skill and post the analysis/results as a comment on this PR

🤖 Copilot Code Review — PR #115913

Holistic Assessment

Motivation: This PR implements the macOS-specific managed and native layers for the new SafeProcessHandle APIs. The change is well-motivated — it provides feature parity with the Windows implementation, enabling cross-platform process management.

Approach: The approach is solid — using posix_spawn for process creation and kqueue/EVFILT_PROC/NOTE_EXIT for wait/timeout/cancellation is the idiomatic macOS way. Reusing existing signal mapping from pal_signal.c and extracting GetEnvVars/ProcessUtils for shared code are good decisions.

Summary: ⚠️ Needs Human Review. The code is well-structured overall and follows the existing codebase patterns. I found a few issues that range from potential correctness concerns to minor style points. I'm unable to verify some concerns without access to a macOS test environment, so human review of the kqueue/timeout behavior is recommended.

Detailed Findings

⚠️ Timeout drift with EINTR in SystemNative_TryWaitForExit

In pal_process.c:1275, the kevent call with a timeout is retried on EINTR without recalculating the remaining timeout:

while ((ret = kevent(queue, &change_list, 1, &event_list, 1, &timeout)) < 0 && errno == EINTR);

On macOS, kevent may or may not update the timeout struct on EINTR (the behavior varies by OS). If it doesn't update the timeout, each retry restarts with the full original timeout, meaning the actual wait could be significantly longer than requested. On macOS specifically, the kevent man page states the timeout value is not updated — so this loop could wait much longer than timeout_ms if signals are being delivered frequently.

This is a low-probability concern in practice (signal storms are rare), but for correctness-sensitive timeout code it's worth noting. A human reviewer should decide whether the current behavior is acceptable or whether a clock-based recalculation is needed.

⚠️ map_wait_status — uninitialized output on WIFSTOPPED

In pal_process.c:1122-1139, map_wait_status handles WIFEXITED and WIFSIGNALED but returns -1 without initializing out_exitCode and out_signal when neither condition is true (e.g., WIFSTOPPED or WIFCONTINUED from a traced/stopped process). While waitpid without WUNTRACED shouldn't return stopped status, this leaves output parameters uninitialized on the error return path. Callers check the return value, so this is unlikely to cause an actual bug, but initializing outputs to 0 at the start of the function (or in TryGetExitCode/WaitForExitAndReap before calling map_wait_status) would be defensive.

⚠️ ReleaseHandle — double-cast of handle field

In SafeProcessHandle.Unix.cs:53-57:

return (int)handle switch
{
    NoPidFd => true,
    _ => Interop.Sys.Close((IntPtr)(int)handle) == 0,
};

The handle field (inherited from SafeHandle) is of type IntPtr. The cast (IntPtr)(int)handle truncates on 64-bit if the handle value ever exceeds 32-bit range. For file descriptors this shouldn't happen (they're small ints), but the double cast is unusual. Consider using Interop.Sys.Close(handle) directly since Close takes IntPtr, or at least document why the truncation is safe.

⚠️ kill_on_parent_death is silently ignored on macOS

In pal_process.c:1076-1077:

// kill_on_parent_death is not supported with posix_spawn on macOS
(void)kill_on_parent_death;

The managed test (KillOnParentExit_CanBeSetToTrue) sets this option and runs on macOS, but the native layer silently ignores it. The test passes because it doesn't actually verify the kill-on-parent-exit behavior — it only checks that the process starts and exits normally. This is a feature gap that should be documented, possibly with a comment in the managed layer about the limitation on macOS. A human reviewer should confirm this is the intended behavior.

💡 SendSignal takes raw int for pidfd but managed code passes NoPidFd (int.MinValue)

In SafeProcessHandle.Unix.cs:271:

int result = Interop.Sys.SendSignal(pidfd, pid, PosixSignal.SIGKILL);

The native SystemNative_SendSignal ignores pidfd entirely ((void)pidfd), so this is not a bug, but the managed code computes pidfd values that are never used. This is fine for now (future Linux pidfd support), just noting the asymmetry.

✅ Signal mapping reuse

Good refactoring — reusing SystemNative_GetPlatformSignalNumber and TryConvertSignalCodeToPosixSignal from pal_signal.c instead of duplicating signal mapping logic. The declaration in pal_signal.h is clean.

✅ Error handling pattern

Consistent use of throw new Win32Exception() (parameterless) across all managed error paths lets the runtime fetch the last error automatically. The ESRCH fallback in KillCore correctly returns false instead of throwing.

✅ Resource cleanup in StartProcessInternal

The try/finally block in StartProcessInternal (lines 97-148) properly frees all allocated native memory even on failure paths.

✅ HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR_NP for tvOS

The restored feature detection for addchdir_np with the comment about tvOS is correct. The refactored #if/#else with unified error handling (post-review suggestion) is clean.

✅ Test coverage

Tests cover the main scenarios: start, kill, wait with timeout, async wait with cancellation, signal sending, and edge cases (invalid signal, signaling exited process). Platform-specific branching in tests is reasonable.

💡 fcntl.h include guard removal

The PR changed #if HAVE_PIPE2 / #include <fcntl.h> to unconditional #include <fcntl.h>. This is correct because:

1. The existing code uses fcntl in !HAVE_PIPE2 paths
2. The new create_kqueue_cloexec function needs fcntl/FD_CLOEXEC

However, note that the original guard was technically wrong (fcntl.h was needed regardless of HAVE_PIPE2), so this change is actually a pre-existing bug fix.

💡 poll.h, time.h, sys/time.h includes

These headers are added unconditionally but only used in the #if HAVE_KQUEUE paths and #ifdef __APPLE__ blocks. They're harmless on all Unix platforms but could be moved under guards for cleanliness. Not blocking.