Implement SafeProcessHandle.WaitForExit* methods

[Copilot]

Implements the approved SafeProcessHandle.WaitForExit* API surface from #123380.

Description

Adds 5 new public methods to SafeProcessHandle: WaitForExit, TryWaitForExit, WaitForExitOrKillOnTimeout, WaitForExitAsync, and WaitForExitOrKillOnCancellationAsync.

Key changes

• Shared async logic in SafeProcessHandle.cs using RegisterWaitForSingleObject + TaskCompletionSource
• Windows: Uses ProcessWaitHandle + GetExitCodeProcess for synchronous wait and exit code retrieval
• Unix: Integrates with ProcessWaitState for reaping coordination; ProcessId property moved to platform-specific file and reads from _waitStateHolder._state._processId
• Native PAL: WaitPidExitedNoHang now returns terminating signal info via TryConvertSignalCodeToPosixSignal
• ProcessWaitState: Stores ProcessExitStatus? instead of separate _exitCode/_terminatingSignal fields; added Holder.IncrementRefCount() for ref-counted ownership sharing between Process and SafeProcessHandle
• Process.Unix.cs: GetProcessHandle() and StartCore use IncrementRefCount() pattern for proper wait state holder ownership
• Consistency fix: Both ProcessId and GetWaitState() now throw InvalidOperationException when _waitStateHolder is null (previously ProcessId threw PlatformNotSupportedException); GetWaitState() throws PlatformNotSupportedException for non-child processes on Unix
• Ref assembly: Updated with new API surface
• Tests: Comprehensive WaitForExit* tests including sync/async theories, signal reporting, timeout/kill behavior, non-child process handling, and Process.SafeHandle interop

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

[Copilot]

@copilot please address my feedback

All feedback addressed in d2f68c9. All 488 tests pass (0 failures).

[adamsitnik]

@copilot address my feedback

[Copilot]

@copilot address my feedback

All feedback addressed in 7c723d5. All 488 tests pass (0 failures).

[Copilot]

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

🤖 Copilot Code Review — PR #127022

Note

This review was generated by GitHub Copilot using the code-review skill. Findings were cross-validated across multiple model families (Gemini 2.5 Pro, GPT-5.3-Codex).

Holistic Assessment

Motivation: The PR implements the approved SafeProcessHandle.WaitForExit* API surface from #123380. The issue has the api-approved label and the approved API shape matches. This is clearly justified and needed.

Approach: The implementation correctly follows the platform delegation pattern — shared async logic in SafeProcessHandle.cs, platform-specific sync methods in .Unix.cs/.Windows.cs, reusing existing ProcessWaitState infrastructure on Unix and ProcessWaitHandle on Windows. The ref-counted Holder ownership pattern with IncrementRefCount() is clean. The native PAL change to propagate signal information through TryConvertSignalCodeToPosixSignal is the correct approach. The change from NotSupportedException to PlatformNotSupportedException for non-child processes on Unix is consistent with dotnet/runtime conventions.

Summary: ✅ LGTM. The core implementation is correct and well-tested. The API surface matches the approved design. The async patterns are sound, resource cleanup is proper, and all previous reviewer feedback has been addressed. The findings below are advisory observations, not merge blockers.

---

Detailed Findings

✅ API Surface — Matches approved shape for WaitForExit methods

The 5 new methods (WaitForExit, TryWaitForExit, WaitForExitOrKillOnTimeout, WaitForExitAsync, WaitForExitOrKillOnCancellationAsync) match the approved API from the API review comment. Parameter names match between ref and src. Platform attributes are correctly applied — only methods involving process termination have [UnsupportedOSPlatform] attributes. The ref assembly correctly declares all 5 methods with matching signatures. This PR implements a subset of the approved API (the remaining pieces like ProcessStartOptions, Open, StartSuspended, Resume, KillProcessGroup, SignalProcessGroup are separate PRs), and the PR description is explicit about this.

✅ Async patterns — Correct use of RegisterWaitForSingleObject + TCS

RunContinuationsAsynchronously correctly prevents inline continuations. registeredWaitHandle?.Unregister(null) and ctr.Dispose() are called in finally blocks. On Unix, the shared ManualResetEvent is correctly not disposed (it's owned by ProcessWaitState), while on Windows it is disposed since a fresh ProcessWaitHandle is created per wait. Exception handling in cancellation callbacks correctly surfaces SignalCore failures via tcs.TrySetException(ex).

✅ Wait state holder ownership — Clean ref-counting

Each SafeProcessHandle created via ForkAndExecProcess owns a ProcessWaitState.Holder, disposed in ReleaseHandle(). IncrementRefCount() correctly acquires the per-dictionary lock before incrementing. The Holder.IncrementRefCount() method is used when Process.Unix.GetProcessHandle() passes a holder to SafeProcessHandle, ensuring proper lifetime management. The finalizer in Holder correctly calls _state?.ReleaseRef() (the Environment.HasShutdownStarted guard was correctly removed per @jkotas's feedback).

✅ Non-child process guard — PlatformNotSupportedException is correct

GetWaitState() now throws PlatformNotSupportedException for non-child processes on Unix. This is the correct exception type per dotnet/runtime conventions (platform limitation that could work on a different platform — Windows). The test WaitForExit_NonChildProcess_NotSupportedOnUnix correctly asserts PlatformNotSupportedException on Unix and validates the Windows path separately.

✅ Native PAL — Signal propagation is correct

SystemNative_WaitPidExitedNoHang now initializes *exitCode = 0 and *terminatingSignal = 0 before calling waitpid, ensuring defined values on all return paths. The WIFSIGNALED branch correctly maps via TryConvertSignalCodeToPosixSignal. The managed ChildReaped method creates ProcessExitStatus with the signal when terminatingSignal != 0.

✅ Test coverage — Comprehensive

Tests cover: normal exit, timeout, kill-on-timeout, async cancellation, invalid handles, non-child processes, signal reporting (SIGKILL/SIGTERM theory), and Process.SafeHandle interop. The use of [ConditionalTheory] with bool useAsync to merge sync/async test pairs is a good pattern.

💡 Incorrect comment in CreateExitStatus — Advisory

SafeProcessHandle.Unix.cs line 159: comment says "GetWaitState ensures the process is not a child process" but the code actually ensures the process is a child process (!IsChild throws). Should read: "GetWaitState ensures the process is a child process, so obtaining the exit status should never fail." (Flagged by multiple models.)

💡 XML docs could document PlatformNotSupportedException — Advisory

The XML doc <exception> tags on WaitForExit, TryWaitForExit, and WaitForExitAsync list InvalidOperationException but don't list PlatformNotSupportedException (thrown on Unix for non-child processes). The <remarks> mention the Unix limitation, so users can discover it, but an explicit <exception> tag would be more consistent. (Follow-up, not a blocker.)

---

Models contributing: Claude Sonnet 4, Gemini 2.5 Pro, GPT-5.3-Codex.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.

[github-actions]

🤖 Copilot Code Review — PR #127022

Note

This review was generated by Copilot and should be treated as AI-generated feedback.

Holistic Assessment

Motivation: This PR implements WaitForExit, TryWaitForExit, WaitForExitAsync, WaitForExitOrKillOnTimeout, and WaitForExitOrKillOnCancellationAsync on SafeProcessHandle, completing the API surface for process lifetime management without requiring a Process object. This fills a real gap — the existing SafeProcessHandle.Start() + Kill() + Signal() pattern had no way to wait for process exit. The linked issue #126293 establishes the need clearly.

Approach: The implementation reuses existing infrastructure — ProcessWaitState on Unix and ProcessWaitHandle + GetExitCodeProcess on Windows — rather than adding new native syscall wrappers. The ref-counting model for ProcessWaitState.Holder is extended so both Process and SafeProcessHandle can independently hold references to the same wait state. The native PAL is extended to return terminating signal information alongside exit codes. This is a well-integrated, incremental approach.

Summary: ⚠️ Needs Human Review. The code is well-structured and the approach is sound. I found one comment that is misleading (says the opposite of what the code does), the removal of an Environment.HasShutdownStarted safety check in the finalizer that should be explicitly justified, and a minor WASI stub concern. The API approval status could not be verified due to GitHub API rate limits. A human reviewer should confirm the API is approved and evaluate the shutdown-safety change.

---

Detailed Findings

✅ Ref-Count Management — Correctly implemented

The ProcessWaitState.Holder.IncrementRefCount() pattern ensures that both SafeProcessHandle and Process get their own Holder with properly incremented ref counts. The lifecycle is clean:

• ForkAndExecProcess creates one Holder (refcount=1), passes it to SafeProcessHandle
• Process.StartCore calls IncrementRefCount() on the out-param to get its own Holder (refcount=2)
• SafeProcessHandle.Start() (standalone) discards the out-param with out _ — the SafeProcessHandle owns the only Holder (refcount=1)
• ReleaseHandle() disposes the Holder, decrementing the refcount

The IncrementRefCount locks the appropriate dictionary (child vs non-child) before incrementing, consistent with AddRef/ReleaseRef.

✅ Windows Implementation — Clean and correct

The Windows WaitForExitCore, TryWaitForExitCore, and WaitForExitOrKillOnTimeoutCore use ProcessWaitHandle + WaitOne + GetExitCodeProcess, which is the established pattern. The using on ProcessWaitHandle ensures cleanup. The ProcessId property uses the field keyword with lazy init from GetProcessId — the benign race (two threads both reading -1 and calling GetProcessId) is fine since GetProcessId is idempotent and int writes are atomic.

✅ Async Pattern — Well-structured

WaitForExitAsync and WaitForExitOrKillOnCancellationAsync correctly use:

• TaskCompletionSource<bool> with RunContinuationsAsynchronously
• ThreadPool.RegisterWaitForSingleObject with static delegates (avoiding allocations)
• UnsafeRegister for cancellation (appropriate for infrastructure code)
• Platform-aware cleanup of the wait handle (disposed on Windows, not on Unix where it's shared)

The cancellation callback in WaitForExitOrKillOnCancellationAsync properly catches exceptions from SignalCore and propagates them via TrySetException.

✅ Native PAL Change — Correct signal mapping

SystemNative_WaitPidExitedNoHang now returns the terminating signal via TryConvertSignalCodeToPosixSignal. The output parameters are properly zero-initialized before the waitpid call. The TryConvertSignalCodeToPosixSignal function was correctly changed from static to extern visibility, and the header declaration was added.

✅ Test Coverage — Comprehensive

Tests cover happy paths (normal exit), timeouts, cancellation, invalid handles, non-child processes (Unix vs Windows behavior divergence), signal reporting, and post-kill waits. Good use of [ConditionalTheory] with [InlineData(true/false)] for sync/async variants.

⚠️ Misleading Comment in CreateExitStatus (Unix)

In SafeProcessHandle.Unix.cs, line 159:

// GetWaitState ensures the process is not a child process, so obtaining the exit status should never fail.
```

This is **backward** — `GetWaitState()` throws `PlatformNotSupportedException` if the process is **not** a child (`!_waitStateHolder._state.IsChild`). The comment should say:
```
// GetWaitState ensures the process IS a child process, so obtaining the exit status should never fail.

⚠️ Removal of Environment.HasShutdownStarted Check in Holder Finalizer

The old ~Holder() had:

if (_state != null && !Environment.HasShutdownStarted)
{
    _state.ReleaseRef();
}

The new code removes the HasShutdownStarted guard:

_state?.ReleaseRef();

The old comment explicitly said "Don't try to Dispose resources (like ManualResetEvents) if the process is shutting down." ReleaseRef can call Dispose() on the ProcessWaitState, which disposes its ManualResetEvent. During CLR shutdown, finalization order is non-deterministic, so the ManualResetEvent or the static dictionary could be in a torn state.

On .NET Core, this may be less of a concern than on .NET Framework (the static dictionaries survive until process teardown), but the removal should be explicitly justified. If this is intentional, a brief code comment explaining why would help future readers.

💡 WASI Stub Doesn't Initialize Output Parameters

In pal_process_wasi.c:

int32_t SystemNative_WaitPidExitedNoHang(int32_t pid, int32_t* exitCode, int32_t* terminatingSignal)
{
    return -1;
}

Unlike the main pal_process.c which zero-initializes *exitCode and *terminatingSignal, the WASI stub leaves them uninitialized. This is harmless because LibraryImport marshaling initializes out parameters on the managed side, and the -1 return means callers won't read them. Still, for consistency with the non-WASI implementation and defensive coding, adding *exitCode = 0; *terminatingSignal = 0; would be cleaner.

💡 ProcessWaitState Field Naming Convention

_processId and _isChild were renamed to ProcessId and IsChild (PascalCase internal readonly fields). While pragmatic for allowing external access without adding separate properties, this departs from the _camelCase convention used for instance fields elsewhere in the codebase. Consider whether these should be internal readonly properties instead:

internal int ProcessId { get; }
internal bool IsChild { get; }

⚠️ API Approval Verification — Unable to Complete

This PR adds new public API surface to SafeProcessHandle (5 new methods in the ref assembly). The API approval verification procedure requires checking for a linked issue with the api-approved label. Due to GitHub API rate limits, I was unable to verify the approval status of issue #126293. Since this PR is authored by @adamsitnik (a .NET team member) and ProcessExitStatus was already approved and merged on main, the API is likely approved — but a human reviewer should confirm.

Generated by Code Review for issue #127022 · ◷

[adamsitnik]

@copilot address my feedback

[Copilot]

@copilot address my feedback

All feedback addressed in 8907972. Build succeeds with 0 errors, 0 warnings.

[adamsitnik]

LGTM, @jkotas @tmds PTAL, I believe all your feedback was addressed

[Copilot]

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

🤖 Copilot Code Review — PR #127022

Note

This review was generated by GitHub Copilot using the code-review skill. Findings were cross-validated across multiple model families (Gemini 2.5 Pro, GPT-5.3-Codex).

Holistic Assessment

Motivation: The PR implements the approved SafeProcessHandle.WaitForExit* API surface from #123380. The issue has the api-approved label and the approved API shape matches. This is clearly justified and needed.

Approach: The implementation correctly follows the platform delegation pattern — shared async logic in SafeProcessHandle.cs, platform-specific sync methods in .Unix.cs/.Windows.cs, reusing existing ProcessWaitState on Unix and ProcessWaitHandle on Windows. The ref-counted Holder ownership pattern with IncrementRefCount() is clean. The native PAL change to propagate signal info through TryConvertSignalCodeToPosixSignal is the correct approach. The change from _exitCode to _exitStatus in ProcessWaitState is a good simplification.

Summary: ✅ LGTM. The core implementation is correct and well-tested. The API surface matches the approved design. The async patterns are sound, resource cleanup is proper, and all previous reviewer feedback has been addressed. The findings below are advisory observations, not merge blockers.

---

Detailed Findings

✅ API Surface — Matches approved shape for WaitForExit methods

The 5 new methods match the approved API from the API review comment. Parameter names match between ref and src. Platform attributes are correctly applied — only methods involving process termination have [UnsupportedOSPlatform] attributes. This PR implements a subset of the approved API (remaining pieces like ProcessStartOptions, Open, StartSuspended, Resume, KillProcessGroup, SignalProcessGroup are tracked separately).

✅ Async patterns — Correct use of RegisterWaitForSingleObject + TCS

RunContinuationsAsynchronously correctly prevents inline continuations on the wait thread. registeredWaitHandle?.Unregister(null) in the finally block ensures cleanup. On Unix the ManualResetEvent is not disposed (not owned), on Windows it is disposed — correct ownership semantics.

✅ Ref-counting — Thread-safe holder pattern

IncrementRefCount() increments under the dictionary lock and creates a new Holder — correct. ReleaseRef decrements under the same lock. The removal of Debug.Assert(foundState) in ReleaseRef is correct — a race where one thread removes state while another releases its reference is now handled gracefully.

✅ Exception consistency — ProcessId and GetWaitState aligned

Both ProcessId and GetWaitState() now throw InvalidOperationException(SR.InvalidProcessHandle) when _waitStateHolder is null. GetWaitState additionally throws PlatformNotSupportedException for non-child processes. This is consistent.

✅ Native PAL — Signal propagation correct

WaitPidExitedNoHang now initializes output parameters to 0 before waitpid, uses TryConvertSignalCodeToPosixSignal for proper mapping, and correctly distinguishes normal exit from signal termination.

💡 CreateExitStatus canceled logic — Advisory

canceled && signal is not null in CreateExitStatus means that if a process is killed (canceled=true) but somehow exits without a signal, Canceled would be false. This seems intentional — on Unix, SIGKILL always produces a signal, and on Windows canceled is passed directly without the signal check. Both models reviewed agreed this is correct behavior.

💡 TryConvertSignalCodeToPosixSignal visibility — Advisory

The function was changed from static to non-static (file-scope) and declared in pal_signal.h without PALEXPORT. This is correct since it's an internal cross-TU helper, not a managed P/Invoke export. No PALEXPORT is needed.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 7 comments.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

[adamsitnik]

/ba-g browser failures are unrelated