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

@copilot address my feedback

All feedback addressed in a6e4206. Summary of changes:

• SafeProcessHandle.cs: if/else if/else blocks, single null device handle, disposed in finally after StartCore
• Process.cs: single null device handle opened once after the lock, reused for all stdio
• ProcessStartInfo.cs docs: split into 3 paragraphs with StandardInputHandle/OutputHandle/ErrorHandle refs
• ProcessStartInfo.cs condition: added && string.IsNullOrEmpty(UserName) to the StartDetached branch
• pal_process.c: reduced setsid check nesting
• StartDetachedTests.cs: [Fact] for validation test; removed mobile platform skips; SIGHUP test skips only Windows; added using System.Runtime.InteropServices for PosixSignal

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

🤖 Copilot Code Review — PR #126632

Holistic Assessment

Motivation: Well-justified. A cross-platform API for starting daemon/background processes is a genuine gap — today users must resort to platform-specific flags or shell scripting. Issue #125838 carries the api-approved label and the PR directly addresses it.

Approach: Correct on all three platforms: DETACHED_PROCESS on Windows, POSIX_SPAWN_SETSID on macOS, and setsid() after fork() before exec() on other Unix — all established patterns. Defaulting unset stdio handles to the null device prevents accidental handle inheritance into long-lived detached processes.

Summary: ✅ LGTM. All three models independently raised two concerns that, after careful verification, are false positives:

Detailed Findings

✅ Handle disposal in Process.cs — Correctly handled (false positive resolved)

All three models flagged that the nullDeviceHandle in Process.cs might be disposed multiple times (once per childInputHandle, childOutputHandle, childErrorHandle in the finally block) if all three point to the same SafeFileHandle instance. This is safe: SafeHandle.Dispose() uses Interlocked.CompareExchange to set STATE_CLOSED atomically — subsequent calls are no-ops. The handle is properly disposed exactly once (OS-level), regardless of how many managed variables reference it.

✅ POSIX_SPAWN_SETSID availability — No issue

One model raised a concern about POSIX_SPAWN_SETSID not being available on older macOS. The runtime's CMAKE_OSX_DEPLOYMENT_TARGET is 14.0 (Sonoma). POSIX_SPAWN_SETSID was introduced in macOS 10.15, so this is fine.

✅ API Approval — Shape matches api-approved issue #125838

The ref assembly declares public bool StartDetached { get { throw null; } set { } } in alphabetical order (after StandardOutputHandle, before UseCredentialsForNetworkingOnly). This matches the approved shape exactly.

✅ Null device handle lifetime in SafeProcessHandle.cs

The using SafeFileHandle? nullDeviceHandle is disposed at end of scope, after StartCore returns. StartCore passes the handle to native code which immediately dup2s it — the managed-side dispose after return is safe.

✅ setsid() error handling

pal_process.c correctly propagates setsid() failures via ExitChild(waitForChildToExecPipe[WRITE_END_OF_PIPE], errno), so the managed caller receives the error.

✅ Test quality

• StartDetached_GrandchildSurvivesSignalingParent uses deterministic polling (20 ms increments up to WaitInMS) and guards with IsNotNanoServerAndRemoteExecutorSupported.
• StartDetached_StartsAndExitsSuccessfully provides a lightweight smoke test.
• StartDetached_ThrowsOnUseShellExecute covers the validation path.
• Windows SendSignal correctly discards entireProcessGroup with a documented explanation.

💡 Consider a Simpler Test for Basic StartDetached Functionality

The existing test (StartDetached_GrandchildSurvivesSignalingParent) covers the complex grandchild-survival scenario. A si...

The StartDetached_StartsAndExitsSuccessfully smoke test was added in commit 820aef5 and is already present in the current PR.

🤖 Copilot Code Review — PR #126632

Holistic Assessment

Motivation: Adding ProcessStartInfo.StartDetached addresses a well-known, legitimate need — starting daemon-like processes that are detached from the parent's session/console and survive parent termination. The approved API issue (#125838) demonstrates clear real-world use cases. Justified. ✅

Approach: The implementation correctly uses platform-appropriate mechanisms (DETACHED_PROCESS on Windows, POSIX_SPAWN_SETSID on macOS, setsid() on Linux). The single boolean property is minimal, consistent with existing ProcessStartInfo patterns (CreateNoWindow, CreateNewProcessGroup), and the defaulting of stdio to the null device is the right behavior for detached processes.

Summary: ⚠️ Needs Human Review. The implementation is sound — the API matches the approved shape from issue #125838, native code is correct, and tests cover the critical scenarios. I'm flagging for human review because of a handle ownership pattern divergence between Process.Start() and SafeProcessHandle.Start() that, while functionally correct due to SafeHandle.Dispose() idempotency, warrants a human judgment call on whether to align the patterns.

Detailed Findings

✅ API Approval — Verified against issue #125838

The api-approved label was applied by @bartonjs on 2025-03-24. The approved API shape includes:

public sealed partial class ProcessStartInfo
{
    public bool StartDetached { get; set; }
}

The implementation matches exactly — no extra public surface, correct property signature, correct placement in alphabetical order in the ref assembly (after StandardOutputHandle, before UseCredentialsForNetworkingOnly). ✅

✅ Native Implementation — Correct platform mechanisms

• macOS (posix_spawn path): Uses POSIX_SPAWN_SETSID flag (inside #if defined(TARGET_OSX) block). This is an Apple-specific extension available since macOS 10.14; .NET requires macOS 10.15+, so always available. ✅
• Linux (fork path): Calls setsid() after fork, before exec (pal_process.c). The freshly forked child always has PID ≠ PGID, so setsid() will succeed. Placement after dup2 but before credential setup and exec is correct. ✅
• WASI stub: Updated with (void)startDetached to suppress unused parameter warning. ✅

✅ Windows Implementation — Correct flag usage

DETACHED_PROCESS = 0x00000008 is correctly OR-ed into creation flags. The decision to allow DETACHED_PROCESS + CREATE_NO_WINDOW together was explicitly reviewed and approved by @jkotas (commit 389dcf7 removed the CreateNoWindow guard, noting the two flags are compatible per Windows API semantics — DETACHED_PROCESS takes precedence). ✅

✅ Validation Logic — Correct and complete

• StartDetached + UseShellExecute correctly throws InvalidOperationException ✅
• Error message stored in .resx following conventions ✅
• The StartDetached parameter threads correctly through the entire call chain: ProcessStartInfo → SafeProcessHandle.ForkAndExecProcess → P/Invoke → pal_process.c ✅

⚠️ Handle Ownership — Inconsistent pattern between Process.Start() and SafeProcessHandle.Start()

SafeProcessHandle.Start() uses the clean using pattern:

using SafeFileHandle? nullDeviceHandle = startInfo.StartDetached
    && (childInputHandle is null || childOutputHandle is null || childErrorHandle is null)
    ? File.OpenNullHandle()
    : null;

The handle is owned by a single using variable and disposed exactly once.

Process.Start() assigns the same File.OpenNullHandle() result to up to three variables (childInputHandle, childOutputHandle, childErrorHandle), each then independently disposed in the finally block, resulting in up to three Dispose() calls on the same SafeFileHandle.

This is functionally safe — SafeHandle.Dispose() is idempotent by design; subsequent calls are no-ops. The finally block comment even acknowledges non-owning handles. I also see that @adamsitnik specifically requested this pattern in review: "call File.OpenNullHandle() at max once, ensure it gets disposed after process is started."

However, the shared-handle-with-multiple-dispose pattern in Process.Start() is inconsistent with the cleaner SafeProcessHandle.Start() approach. A human reviewer should confirm whether this divergence is intentional or whether a dedicated nullDeviceHandle local with using (matching the other call site) would be preferable.

Not blocking — code is correct as-is.

✅ Test Quality — Well-designed coverage

Three tests cover the critical scenarios:

1. StartDetached_ThrowsOnUseShellExecute — Validates the incompatibility check via SafeProcessHandle.Start(). ✅
2. StartDetached_StartsAndExitsSuccessfully — Smoke test verifying a detached process runs and exits normally. ✅
3. StartDetached_GrandchildSurvivesSignalingParent — [Theory] with true/false testing that a detached grandchild survives process-group kill while a non-detached one is killed. Good use of RemoteExecutor, proper cleanup via finally { grandchild.Kill(); }, and appropriate conditional attributes. ✅

The 300ms timeout for the "alive" assertion is reasonable since the grandchild does Thread.Sleep(Timeout.Infinite) — it cannot exit on its own, and setsid() ensures the SIGKILL to the parent's process group doesn't reach it. ✅

💡 XML Documentation — Minor clarity opportunity

The XML docs on StartDetached reference StandardInputHandle, StandardOutputHandle, StandardErrorHandle as properties that override null device defaulting. When using Process.Start() (the more common API path), RedirectStandardInput/RedirectStandardOutput/RedirectStandardError also override the null device behavior. The docs already mention these in the first <para>, but the "unless explicitly configured" phrasing in the second <para> could be slightly more inclusive.

Minor — not blocking.

Generated by Code Review for issue #126632 · ◷

/ba-g unrelated