Skip to content

Add Process.TryGetProcessById and SafeProcessHandle.TryOpen APIs#128695

Merged
adamsitnik merged 8 commits into
copilot/add-safeproceshandle-open-apifrom
copilot/add-trygetprocessbyid-methods
May 29, 2026
Merged

Add Process.TryGetProcessById and SafeProcessHandle.TryOpen APIs#128695
adamsitnik merged 8 commits into
copilot/add-safeproceshandle-open-apifrom
copilot/add-trygetprocessbyid-methods

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 28, 2026

Implements the approved API from #101582 — non-throwing alternatives to Process.GetProcessById and SafeProcessHandle.Open that return false when the target process doesn't exist, avoiding the inherent race condition.

// Returns false if process doesn't exist; throws UnauthorizedAccessException on permission errors
SafeProcessHandle.TryOpen(int processId, out SafeProcessHandle? processHandle);
Process.TryGetProcessById(int processId, out Process? process);

Changes

  • Reference assembly (ref/System.Diagnostics.Process.cs): Added both new APIs with platform attributes and [NotNullWhen(true)]
  • Windows implementation (SafeProcessHandle.Windows.cs): Extracted TryOpenCore — returns false on failure, throws UnauthorizedAccessException for ERROR_ACCESS_DENIED
  • Unix implementation (SafeProcessHandle.Unix.cs): Same pattern — returns false on kill(pid, 0) failure, throws UnauthorizedAccessException on EPERM
  • SafeProcessHandle.cs: Added public TryOpen with argument validation + PNSE guard; refactored Open to delegate directly to TryOpen; XML docs updated to reference UnauthorizedAccessException
  • Process.cs: Added private Process(SafeProcessHandle processHandle) ctor to avoid re-acquiring the handle; TryGetProcessById delegates to SafeProcessHandle.TryOpen and passes the handle to the new ctor
  • Tests (ProcessOpenTests.cs): Consolidated Open/TryOpen and GetProcessById/TryGetProcessById tests using [ConditionalTheory] with [InlineData]; added OS-specific UnauthorizedAccessException tests (Windows: lsass, Linux: PID 1) guarded by IsNotPrivilegedProcess; tests verify Kill + TryWaitForExit on opened handles; Open_ExitedProcess_BehaviorDependsOnPlatform uses (bool tryOpen, bool kill) parameters to verify both exit and kill produce the same platform-dependent behavior; replaced all CreateDefaultProcess usage with CreateProcess + explicit Kill() in finally blocks
  • SafeProcessHandleTests.cs: Fixed build errors by cleanly removing only the Open_* tests that were moved to ProcessOpenTests.cs

Copilot AI and others added 2 commits May 28, 2026 12:20
Implement SafeProcessHandle.TryOpen that attempts to open a process by ID
without throwing when the process doesn't exist (returns false instead).
Throws Win32Exception only for permission errors.

Implement Process.TryGetProcessById that delegates to SafeProcessHandle.TryOpen.

Refactor SafeProcessHandle.Open to delegate to TryOpenCore internally.

Add ProcessOpenTests.cs with tests for the new APIs and moved tests for
Open/GetProcessById from SafeProcessHandleTests.cs and ProcessTests.cs.

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

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

Comment thread src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment thread src/libraries/System.Diagnostics.Process/tests/ProcessOpenTests.cs Outdated
Comment thread src/libraries/System.Diagnostics.Process/tests/ProcessOpenTests.cs Outdated
Copilot AI and others added 2 commits May 28, 2026 12:54
…explicitly, add Process(SafeProcessHandle) ctor, consolidate tests with Theory

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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 my feedback, build the product and run all process tests locally. Don't push any changes unless all process tests are passing

Comment thread src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment thread src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment thread src/libraries/System.Diagnostics.Process/tests/ProcessOpenTests.cs
Comment thread src/libraries/System.Diagnostics.Process/tests/ProcessOpenTests.cs
Comment thread src/libraries/System.Diagnostics.Process/tests/ProcessOpenTests.cs Outdated
Comment thread src/libraries/System.Diagnostics.Process/tests/ProcessOpenTests.cs Outdated
Comment thread src/libraries/System.Diagnostics.Process/tests/ProcessOpenTests.cs Outdated
Copilot AI and others added 2 commits May 28, 2026 17:59
…ess ctor

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
…terns, remove redundant test

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 28, 2026

@copilot address my feedback, build the product and run all process tests locally. Don't push any changes unless all process tests are passing

All feedback addressed. Built product and ran all process tests locally — 631 passed, 0 failed, 7 skipped (platform-specific).

Copilot AI requested a review from adamsitnik May 28, 2026 18:25
Copy link
Copy Markdown
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 we are almost there, please address my feedback


if (tryGet)
{
bool result = Process.TryGetProcessById(processId, out Process? found);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would expect it to behave similar to Open_ExitedProcess_BehaviorDependsOnPlatform namely being able to open a process after it was killed/it exited on Windows.

Edit: I've run the test locally and confirmed that it fails on Windows:

      System.Diagnostics.Tests.ProcessOpenTests.GetProcessById_KilledProcess_Fails(tryGet: True) [FAIL]
        Assert.False() Failure
        Expected: False
        Actual:   True
        Stack Trace:
          D:\projects\runtime_copilot\src\libraries\System.Diagnostics.Process\tests\ProcessOpenTests.cs(234,0): at System.Diagnostics.Tests.ProcessOpenTests.GetProcessById_KilledProcess_Fails(Boolean tryGet)
             at System.RuntimeMethodHandle.InvokeMethod(ObjectHandleOnStack target, Void** arguments, ObjectHandleOnStack sig, BOOL isConstructor, ObjectHandleOnStack result)
          D:\projects\runtime_copilot\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.cs(174,0): at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr)

@copilot please merge GetProcessById_KilledProcess_Fails into Open_ExitedProcess_BehaviorDependsOnPlatform, add another boolean argument that determines whether the process should exit or be killed and then just use these asserts to verify that exit/killed produces the same output in terms of Open/TryOpen

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — merged GetProcessById_KilledProcess_Fails into Open_ExitedProcess_BehaviorDependsOnPlatform with [InlineData(bool tryOpen, bool kill)] (4 combinations). Both exit and kill scenarios now use the same platform-dependent assertions.

…viorDependsOnPlatform with kill parameter

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI requested a review from adamsitnik May 28, 2026 19:57
Copy link
Copy Markdown
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 now!

It's also passing for me locally on Windows:

  ----- start Thu 05/28/2026 22:05:31.74 ===============  To repro directly: =====================================================
  pushd D:\projects\runtime_copilot\artifacts\bin\System.Diagnostics.Process.Tests\Debug\net11.0-windows\
  "D:\projects\runtime_copilot\artifacts\bin\testhost\net11.0-windows-Debug-x64\dotnet.exe" exec --runtimeconfig System.Diagnostics.Process.Tests.runtimeconfig.json --depsfile System.Diagnostics.Process.Tests.deps.json C:\Users\adsitnik\.nuget\packages\microsoft.dotnet.xunitconsolerunner\2.9.3-beta.26257.113\build\..\tools\net\xunit.console.dll System.Diagnostics.Process.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing
  popd
  ===========================================================================================================
    Discovering: System.Diagnostics.Process.Tests (method display = ClassAndMethod, method display options = None)
    Discovered:  System.Diagnostics.Process.Tests (found 409 of 436 test cases)
    Starting:    System.Diagnostics.Process.Tests (parallel test collections = on [24 threads], stop on fail = off)
      System.Diagnostics.Tests.ProcessStartInfoTests.TestWindowStyle [SKIP]
        Condition(s) not met: "IsAdmin_IsNotNano_RemoteExecutorIsSupported"
      System.Diagnostics.Tests.ProcessStartInfoTests.ShellExecute_Nano_Fails_Start [SKIP]
        Condition(s) not met: "IsWindowsNanoServer"
  0 File(s) copied
  Invalid number of parameters
      System.Diagnostics.Tests.ProcessOpenTests.TryOpen_ProcessOwnedByRoot_ThrowsUnauthorizedAccessException [SKIP]
        Condition(s) not met: "IsLinux"
      System.Diagnostics.Tests.ProcessOpenTests.Open_ProcessOwnedByRoot_ThrowsUnauthorizedAccessException [SKIP]
        Condition(s) not met: "IsLinux"
    Finished:    System.Diagnostics.Process.Tests
  === TEST EXECUTION SUMMARY ===
     System.Diagnostics.Process.Tests  Total: 628, Errors: 0, Failed: 0, Skipped: 4, Time: 81.868s
  ----- end Thu 05/28/2026 22:06:54.89 ----- exit code 0 ----------------------------------------------------------

Copy link
Copy Markdown
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.

Overall it LGTM, we just need to remove the two tests that keep failing on Linux.

Comment thread src/libraries/System.Diagnostics.Process/tests/ProcessOpenTests.cs Outdated
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
@adamsitnik adamsitnik merged commit d039b6e into copilot/add-safeproceshandle-open-api May 29, 2026
22 of 24 checks passed
@adamsitnik adamsitnik deleted the copilot/add-trygetprocessbyid-methods branch May 29, 2026 10:56
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.

2 participants