@copilot please address my feedback

All addressed in 6d4cbac. Summary of changes:

• Merged Interop.ProcThreadAttributeList.cs into Interop.CreateProcess.cs (deleted the former file, updated CreateProcess to accept STARTUPINFOEX)
• Fixed braces style in AddHandleToInheritList
• Simplified all restrictHandles test code to startInfo.InheritedHandles = restrictHandles ? [] : null
• Moved HandleNotInheritedExitCode const inside InheritedHandles_CanRestrictHandleInheritance
• Moved CLOSE_RANGE_CLOEXEC define to the includes section as #if (HAVE_CLOSE_RANGE || HAVE_CLOSE_RANGE_SYSCALL) && !defined(CLOSE_RANGE_CLOEXEC)
• Added Illumos/Solaris fdwalk support via check_function_exists in cmake, HAVE_FDWALK in pal_config.h.in, and a SetCloexecForFd callback + #elif HAVE_FDWALK branch in pal_process.c

Detailed Findings

⚠️ Duplicate stdio handles in PROC_THREAD_ATTRIBUTE_HANDLE_LIST — Dedup check removed

Files: SafeProcessHandle.Windows.cs:408-418, SafeProcessHandle.Windows.cs:117-120

The old AddHandleToInheritList checked for duplicates before adding to the handle list:

// Old code (removed)
for (int i = 0; i < handleCount; i++)
{
    if (handlesToInherit[i] == rawValue)
        return; // already in list
}

The new AddToInheritListIfValid unconditionally appends valid handles. This means if hStdOutput and hStdError resolve to the same raw handle value (which occurs when stdout and stderr are redirected to the same target, e.g., 2>&1, or in some CI environments), both would be added to the PROC_THREAD_ATTRIBUTE_HANDLE_LIST.

Some empirical evidence suggests UpdateProcThreadAttribute returns ERROR_INVALID_PARAMETER with duplicate handle entries. If true, this would cause Process.Start to fail with a Win32Exception when InheritedHandles is set (even []) and stdout/stderr share the same underlying handle.

The user's InheritedHandles list is already deduplicated in ThrowIfInvalid, and ValidateInheritedHandles prevents overlap with stdio handles — but the stdio handles themselves are not deduplicated against each other. (Flagged by all 3 models)

Recommendation: Consider restoring a simple dedup check in AddToInheritListIfValid, or verify with a test that Windows tolerates duplicate entries.

We already have such a test (CanRedirectOutputAndErrorToSamePipe) and it's passing on Windows:

Issue: If another thread calls Dispose() on the SafeHandle between SetHandleInformation and DangerousAddRef

The code uses reader locks for concurrent process creation with InheritedHandles, but multiple threads can race on the same handle's inheritance flags:

there's a theoretical race where handles could be closed between validation and actual use.

This API is documented as not safe for such scenarios. We are NOT going to try to prevent all possible misuses. This is by design.

If BuildProcThreadAttributeList throws an exception after EnableInheritanceAndAddRef has partially executed, the handles that were successfully processed remain in an incorrect state:

This is not true. EnableInheritanceAndAddRef populates an array of handles that require their inheritance to be disabled. If BuildProcThreadAttributeList throws, DisableInheritanceAndRelease is getting executed in the finally block and disables the inheritance for all non-null handles from the list.

Issue: If RLIMIT_NOFILE is set to a very high value (e.g., 1,048,576), this loop will make over 1 million system calls, causing significant performance degradation during process startup.

Fix Required: Add a reasonable upper bound (e.g., 65536) to prevent performance issues in environments with extremely high file descriptor limits.

This is a new API, that user has to explicitly enable. It's introduced for the sake of correctness, at a cost of potential performance degradation. This is documented and by design.

Consider logging the failure or handling it more robustly than just a debug assert.

This is by design. We have two options:

• spawning the process has failed and exception was thrown: if we throw a different exception, it will overwrite it and the user is going to miss the actual source of the problem
• spawning the process has succeeded: we can't throw an exception because it would lead to resource leak

We can't use any general purpose logger because it's base class library. We could emit some kind of event, but it's pointless because users would need to know about it's existence and sign up for it. And the only way to get to the point where we need to throw here is invalid usage of the API, where user has provided us with a handle that some other part of the application has closed.

🤖 Copilot Code Review — PR #126318

Holistic Assessment

Motivation: Well-justified. This implements ProcessStartInfo.InheritedHandles from the API-approved issue #125838, addressing the long-standing request for restricting child process handle inheritance. This is a real-world need for security and resource management.

Approach: Sound. The implementation uses platform-appropriate mechanisms: PROC_THREAD_ATTRIBUTE_HANDLE_LIST on Windows (the recommended Win32 pattern), POSIX_SPAWN_CLOEXEC_DEFAULT on macOS, and close_range(CLOSE_RANGE_CLOEXEC) with multiple fallback layers on Linux/Unix. The code has clearly gone through extensive iteration with @jkotas as co-author.

Summary: ⚠️ Needs Human Review. The code is well-implemented and has already been through multiple review rounds. I have no blocking issues but flag a few items that warrant human confirmation on design trade-offs around thread safety and exception message changes.

Detailed Findings

✅ API Approval Verification — Matches approved shape

The api-approved label was applied by @bartonjs on issue #125838 on 2026-03-24. The approved API shape for this PR's scope is:

public sealed partial class ProcessStartInfo
{
    public IList<SafeHandle>? InheritedHandles { get; set; }
}

The ref assembly (ref/System.Diagnostics.Process.cs) and implementation both match this exactly. No extra unapproved public API surface was detected.

✅ Windows Implementation — PROC_THREAD_ATTRIBUTE_HANDLE_LIST

The Windows path correctly:

• Uses STARTUPINFOEX + InitializeProcThreadAttributeList + UpdateProcThreadAttribute with PROC_THREAD_ATTRIBUTE_HANDLE_LIST
• Takes a read lock when InheritedHandles is set (allowing concurrent starts with explicit handle lists) vs. the existing write lock for the default path. This is safe because both concurrent paths use the attribute list to restrict inheritance, so temporarily-inheritable handles from one start won't leak to another.
• Correctly handles cleanup: DisableInheritanceAndRelease unconditionally removes HANDLE_FLAG_INHERIT and releases refs even on error paths.
• Sets bInheritHandles = false when handleCount == 0 (empty list + all-invalid stdio), correctly preventing any handle inheritance.

✅ Unix Implementation — Graceful fallback chain

The native C implementation provides a well-layered fallback:

1. close_range(3, UINT_MAX, CLOSE_RANGE_CLOEXEC) — kernel function (FreeBSD 12.2+, glibc ≥ 2.34)
2. syscall(__NR_close_range, ...) — raw syscall for older glibc on Linux ≥ 5.9
3. fdwalk() — Illumos/Solaris
4. Brute-force getrlimit/sysconf loop

Critically, the code uses CLOSE_RANGE_CLOEXEC (set flag) rather than direct close, preserving the waitForChildToExecPipe needed to report exec failures back to the parent. The macOS posix_spawn path uses POSIX_SPAWN_CLOEXEC_DEFAULT + posix_spawn_file_actions_addinherit_np, which is the idiomatic Apple API.

The per-architecture __NR_close_range fallback correctly uses 436 (generic syscall table) with an alpha-specific override of 546.

✅ Handle Lifecycle Management

• DuplicateAsInheritableIfNeeded was refactored from returning SafeFileHandle? to outputting nint + refAdded. The cleanup in the finally block correctly distinguishes between "release ref" (handle was already inheritable) and "close duplicated handle" paths.
• EnableInheritanceAndAddRef stores handles in handlesToRelease before calling SetHandleInformation, ensuring cleanup even when the Win32 call fails.
• The DangerousAddRef/DangerousRelease pairing for inherited handles in Interop.ForkAndExecProcess.cs (Unix) uses inheritedRefsAdded counter to release only successfully AddRef'd handles.

✅ Test Coverage

Comprehensive tests covering:

• Pipe redirection with/without InheritedHandles restriction
• Multiple handles (CanInheritMoreThanOneHandle)
• Both SafeFileHandle and SafePipeHandle inheritance
• Validation: null, invalid, closed, duplicate, unsupported handle types, standard handle overlap
• Edge cases: processes with invalid standard handles, nested child process starts
• Windows-specific: INVALID_HANDLE_VALUE stdio handles with CreateProcess
• Mobile platform coverage updated with InheritedHandles parameter
• The ArgumentList[0] exception name change has a corresponding test update.

⚠️ Thread Safety — Documented but risky concurrent use

The PR documents that "Two concurrent process starts that pass same handle in InheritedHandles are not supported" because SetHandleInformation globally modifies handle inheritance flags. This is a reasonable trade-off, but consider whether the documentation is sufficient to prevent subtle bugs — users may not expect that sharing a SafeFileHandle across two ProcessStartInfo instances started concurrently is unsafe.

Question for reviewers: Is this limitation sufficiently discoverable? A runtime check detecting shared handles across concurrent starts would be expensive, but the current documentation-only approach relies on users reading the remarks.

💡 ArgumentList exception message change — Minor behavioral change

The ArgumentList null validation changed from throw new ArgumentNullException("item", SR.ArgumentListMayNotContainNull) to throw new ArgumentNullException($"ArgumentList[{i}]"). This is an improvement for debuggability (includes the index), but is a behavioral change that removes the resource string ArgumentListMayNotContainNull. The test was updated to match. This is a minor breaking change in exception details that shouldn't affect well-written code.

💡 Ref Assembly Reordering

The ref assembly changes include alphabetical reordering of existing SafeProcessHandle and ProcessStartInfo members (e.g., Kill() before ProcessId, StandardInputEncoding before StandardInputHandle). This is consistent with dotnet/runtime conventions for ref source.

Generated by Code Review for issue #126318 · ◷

/ba-g the failures and foremost timeouts are unrelated (Remote machine provider issue: Agent was purged, cancelling the pipeline)