Skip to content

[patch] Treat a partly failed process-tree kill as best effort, so Cancel() never throws - #112

Merged
matt-edmondson merged 3 commits into
mainfrom
fix/107-trykill-aggregate
Oct 6, 2026
Merged

matt-edmondson merged 3 commits into
mainfrom
fix/107-trykill-aggregate

Conversation

@matt-edmondson

Copy link
Copy Markdown
Contributor

Fixes #107

Problem

Process.Kill(entireProcessTree: true) reports descendants it couldn't terminate as AggregateException, such as a sudo child or an elevated process. TryKill caught only InvalidOperationException, Win32Exception and NotSupportedException, so the exception escaped from all three call sites:

  • The token registration: the caller's own cts.Cancel() threw, or with CancelAfter, the timer thread did.
  • The OperationCanceledException catch: it replaced the documented OperationCanceledException.
  • The failed-read catch: it hid the handler's real exception.

Change

  • TryKill now also catches AggregateException. The runtime's tree kill carries on past each failure, so everything it could kill has already been killed.
  • The seam the issue asked for: the kill itself moved into a private Kill(Process), which keeps the netstandard2.x single-process fallback. TryKill(Process) forwards to a new internal TryKill(Process, Action<Process> kill), so a test can inject a kill that fails. The XML doc says why this method must never throw.

Tests

New CancelShouldNotThrowWhenTheTreeKillCannotTerminateEveryDescendant:

  • Starts a real sleep.
  • Registers TryKill with a kill that throws the runtime's AggregateException(Win32Exception) on a token, the same way RunAsync registers its kill.
  • Checks that cts.Cancel() returns normally, and that a direct TryKill call, as the catch blocks make it, doesn't throw either.

Checked locally (Linux, net10.0):

  • With the new catch removed, the test fails with the exception coming out of CancellationTokenSource.ExecuteCallbackHandlers, which is the issue's cts.Cancel() symptom.
  • With the change, the full suite passes: 58 passed, 2 skipped (the elevation tests). The library builds for all target frameworks with 0 errors.

Not tested end to end: a real process tree with an unkillable descendant. As the issue says, a sandboxed root environment can't easily create one. The seam test covers the exception path that such a tree produces.

Independent of #110 and #111, which touch other parts of RunAsync and AsyncProcessStreamReader.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Bm33TiYrofqKs4oGc3UGUU


Generated by Claude Code

matt-edmondson and others added 3 commits October 6, 2026 03:28
…ncel() never throws

Process.Kill(entireProcessTree: true) reports descendants it could not
terminate (owned by another user, or elevated) as AggregateException, which
TryKill did not catch. It escaped the cancellation registration out of the
caller's Cancel(), replaced the OperationCanceledException RunAsync rethrows,
and hid a handler's real exception. TryKill now catches it, and takes the kill
as a parameter so a test can make it fail.

Fixes #107

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Bm33TiYrofqKs4oGc3UGUU
Resolve the RunCommandTests.cs conflict by keeping both sides' new tests.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01JE3qjjjTXN28xUcTq2h6vD
SonarCloud S2699 flagged the test as having no assertion.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01JE3qjjjTXN28xUcTq2h6vD
@sonarqubecloud

sonarqubecloud Bot commented Oct 6, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants