Repository navigation
Kill the whole process tree from the netstandard builds where the runtime can [patch] - #142
Open
matt-edmondson wants to merge 3 commits into
Open
matt-edmondson wants to merge 3 commits into
matt-edmondson wants to merge 3 commits into
Conversation
…time can [patch] No build targets .NET Core 3.x or .NET 5-7, so apps on those runtimes load the netstandard2.1 build, which compiled Kill to a plain process.Kill(). A cancelled command's grandchildren (build servers, test hosts, npm children) kept running there, although README promised the whole tree is terminated on .NET Core 3.0 and later. Process.Kill(bool) exists on every one of those runtimes but not on the netstandard reference surface, so the netstandard builds now look it up once by reflection and call it, falling back to process.Kill() on .NET Framework and Mono. TargetInvocationException is unwrapped, so TryKill still treats the overload's AggregateException, InvalidOperationException and Win32Exception as best effort (#107). The README now says which runtimes kill the tree, and CLAUDE.md drops the net5.0-net7.0 targets it still listed and describes the reflection path. KillEntireTreeWhereSupportedShouldEndTheGrandchildrenToo starts a shell with a background sleep and checks the sleep is gone after the kill. The method is compiled on every target so the net10.0 tests reach it. Fixes #134 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vfJW5DyciGEV4wZvik3pQ
KillEntireTreeWhereSupported's fallback for runtimes without Process.Kill(bool) and its TargetInvocationException unwrap had no test, so SonarCloud's new-code coverage was 50% (gate is 80%). The overload's MethodInfo is now a parameter of an internal overload, as TryKill's kill is. One test passes null, standing in for .NET Framework and Mono, and checks the process is still killed. The other kills a Process that was never started and checks the InvalidOperationException arrives unwrapped, so TryKill's catch blocks still see it; with the unwrap removed it gets a TargetInvocationException instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vfJW5DyciGEV4wZvik3pQ
…ead.Sleep SonarCloud flagged the Thread.Sleep polling loop in KillEntireTreeWhereSupportedShouldEndTheGrandchildrenToo (S2925). SpinUntil keeps the same 5 s bound and asserts on its result. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vfJW5DyciGEV4wZvik3pQ
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #134
What changed
No build of the package targets .NET Core 3.x or .NET 5–7, so apps on those runtimes load the
netstandard2.1build. That build compiledKillto a plainprocess.Kill(). On those runtimes a cancelled command's grandchildren kept running, although the README promises tree termination on .NET Core 3.0 and later.Process.Kill(bool)once by reflection and call it withtrue. The method exists on every one of those runtimes; the netstandard reference surface just doesn't declare it. On .NET Framework and Mono, which have no such overload, they fall back toprocess.Kill().TargetInvocationExceptionis unwrapped withExceptionDispatchInfo, soTryKillstill treats the overload'sAggregateException,InvalidOperationExceptionandWin32Exceptionas best effort (Cancelling a command whose process tree has an unkillable descendant (sudo, elevated child) makes cts.Cancel() throw AggregateException, and ExecuteAsync throws it instead of OperationCanceledException #107).Kill(entireProcessTree: true)directly.Tests
KillEntireTreeWhereSupportedShouldEndTheGrandchildrenToostartssh -c "sleep 300 & echo $!; wait", kills it through the new method, and checks that the background sleep is gone. The method is compiled on every target, so the net10.0 test project can reach it. The test is skipped on Windows.process.Kill(), the test failed (Assert.IsFalse(IsRunning(grandchild))). With the change it passes.mainit printedsurviving 'sleep 37': 4896, and on this branch it printssurviving 'sleep 37': none.Generated by Claude Code