Bound the output pump by the cancellation token

[matt-edmondson]

Fixes #78.

The bug

AsyncProcessStreamReader.Start pumped both pipes until each reached end of stream. End of stream means every handle on the write end has closed — and killing the command closes only the handles the command itself held. A descendant that inherited stdout/stderr and outlived its parent keeps the pipe open, so the wait never ends.

RunAsync awaits Task.WhenAll(outputReader.Start(), process.WaitForExitAsync(cancellationToken)). The token reached the wait but never reached the reads: ReadAndCallback calls streamReader.ReadAsync(buffer, 0, buffer.Length), which returns on new data or EOF and nothing else. So on cancellation the process died, WaitForExitAsync came back, and WhenAll sat on a read nobody was going to finish — contradicting the library's documented contract that cancellation kills the process and lets the await proceed.

Reproduced on main, before any change here:

sh -c "sh -c 'sleep 30 &'; sleep 30"     cancelled immediately  ->  never returns

The inner shell backgrounds a sleep and exits, so that sleep is reparented to init and Kill(entireProcessTree: true) cannot walk to it — but it still holds the stdout and stderr handles it inherited. The outer sleep keeps the direct child alive so cancellation is what ends it.

Worth recording, since it cost a detour: setsid is not sufficient to reproduce this. It gives the child its own session but leaves its parent alone (ppid unchanged), so the tree kill still reaches it and the pipe still closes. Measured in this container — setsid sleep 25 & from a shell at pid 922 produced pid 924, ppid 922, sess 924. A test written that way passes on main and proves nothing. The double-fork above gives ppid 1, which is the condition that matters.

The fix

The issue offers two routes: pass the token into ReadAsync, or race the read against a cancellation-linked task. This takes the second, and the choice is deliberate rather than convenient — StreamReader has no cancellable overload on every target here, and a pipe read is not reliably interruptible even where one exists. Racing works uniformly across all five TFMs, including the netstandard2.x ones the issue notes are hit hardest.

• Start takes the CancellationToken and races every wait against a TaskCompletionSource the token completes: the WhenAny in the read loop, the drain after the loop, and the post-exit final read.
• On cancellation it returns promptly, leaving RunAsync's existing trailing ThrowIfCancellationRequested() to raise OperationCanceledException — so the cancellation path stays in one place rather than gaining a second thrower.
• Abandoned reads get a continuation that observes their eventual fault. Disposing the readers ends them, but it ends them faulted, and a faulted task nobody looks at raises TaskScheduler.UnobservedTaskException on finalization — which would have turned this fix into an intermittent failure somewhere unrelated.
• The drain helper still awaits the reads on the non-cancelled path, so a decode failure is propagated exactly as before. That is load-bearing: ADecodeFailureIsNotDiscardedWhenTheProcessKeepsRunning and the byte-order-mark tests depend on it.

No public API change — AsyncProcessStreamReader is internal.

Tests

ExecuteAsyncShouldReturnWhenCancelledWhileADetachedDescendantHoldsTheOutputPipe in RunCommandTests, the regression the issue asks for. Verified both ways:

	result
against the previous implementation	failed — the 10s bound elapsed, the call never returned
after the change	passed in 1.1s

It is bounded with Task.WhenAny(execution, Task.Delay(10s)) and asserts on which task completed, rather than a bare await: without the fix a bare await hangs, which takes the whole run down instead of reporting a failure. It then asserts OperationCanceledException, so a call that returns an exit code does not pass either.

Skipped on Windows with Assert.Inconclusive, matching the existing precedent in ADecodeFailureIsNotDiscardedWhenTheProcessKeepsRunning — it needs a shell that can orphan a child out of its own process tree. The code being fixed is platform independent, so the other legs cover it.

Full suite: 40 total, 39 passed, 0 failed, 1 skipped (the pre-existing Windows-only elevation skip). Release build of the solution across all five target frameworks (net10.0, net9.0, net8.0, netstandard2.0, netstandard2.1): 0 warnings, 0 errors.

Scope

One method reshaped, two small private helpers added, one call site updated, one test. Deliberately not in scope: a read that never ends at all still costs its buffer until the process handle is released. Reclaiming that would mean owning the pipe lifetime rather than borrowing Process's, which is a larger change than the hang warrants — the contract this fixes is that a cancelled call returns, and it now does.

Worth knowing downstream: this is the DrainTimeout concern raised as the third caveat on ktsu-dev/GitBranchStateCache#27, where GitRunner's hand-rolled bounded drain exists precisely because a grandchild can hold the pipe open. That caveat is addressed by this change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Wi82CWVJxLjZR27vTkenFm

---

Generated by Claude Code

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
87.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[matt-edmondson]

Disposition of the 3 new SonarQube issues

Quality gate passed; all three are INFO/MINOR code smells with no inline comments, so none is blocking. Fetched them from the SonarCloud API to judge them on merit rather than leave them unexamined. One is provably wrong, two are declined for stated reasons. Nothing pushed — none of these justifies a CI cycle on its own, and there is no other code change coming on this PR to carry them.

csharpsquid:S8969 (MINOR) — wrong, and the build proves it

AsyncProcessStreamReader.cs:59 — Remove this null-forgiving operator; the compiler already knows this expression is not null here.

It does not. The overload bound is CancellationToken.Register(Action<object?>, object?), so state is genuinely nullable. I removed the ! and built:

AsyncProcessStreamReader.cs(59,21): error CS8600: Converting null literal or possible null value to non-nullable type.
AsyncProcessStreamReader.cs(59,21): error CS8602: Dereference of a possibly null reference.

Two errors, on every target framework (net10.0, net9.0, net8.0, netstandard2.0, netstandard2.1) — this repository builds warnings-as-errors, so acting on this finding would break the build. The operator stays. Probe reverted; nothing pushed for it.

external_roslyn:MSTEST0061 (INFO) — declined, matches the file's convention

RunCommandTests.cs:474 — Use [OSCondition] instead of RuntimeInformation.IsOSPlatform with Assert.Inconclusive.

Reasonable in the abstract, but ADecodeFailureIsNotDiscardedWhenTheProcessKeepsRunning and both elevation tests already use exactly this idiom. Sonar only reports on new code, which is why only the new test is flagged — the pattern is pre-existing, not something this PR introduces. Converting one test to [OSCondition] would leave it inconsistent with its neighbours; converting all of them is a separate cleanup worth its own PR if wanted.

external_roslyn:MSTEST0049 (INFO) — declined, would weaken the test

RunCommandTests.cs:499 — Consider passing TestContext.CancellationToken.

That line is the Task.Delay(10s) that bounds the assertion. Making it cancellable is actively counterproductive here: if the token fired, the delay would fault, Task.WhenAny would return it, and Assert.AreSame would fail with a message pointing at the wrong thing. The bound needs to be unconditional for the test to mean what it says. The delay is abandoned after the call returns in ~1.1s, which is the cost of keeping it simple.

---

Generated by Claude Code