Standard output and standard error delegates run concurrently on two threads, so a shared collection silently loses lines (~0.5% of 40,000 lost every run)

[matt-edmondson]

What's wrong

AsyncProcessStreamReader.Start starts two independent ReadToEnd loops, one for stdout and one for stderr (RunCommand/AsyncProcessStreamReader.cs, outputTask/errorTask). Each loop calls its handler delegate directly from its own read continuation, so onStandardOutput and onStandardError can run at the same time on different thread-pool threads. Nothing in the library serialises them, and nothing in the README or XML docs warns about it.

The most natural way to capture a command's full output hits this directly:

List<string> lines = [];
await RunCommand.ExecuteAsync("sh", ["-c", "..."], new LineOutputHandler(lines.Add, lines.Add));

List<T>.Add isn't thread-safe. Concurrent calls overwrite each other's slots, so lines disappear with no exception. The same goes for a shared StringBuilder, or any caller state that isn't synchronised. LineOutputHandler keeps a separate buffer per stream, so its own state is fine. The race is in the caller's delegates.

(The test suite already works around this: RunCommandTests.cs takes lock (output) inside its delegates. Library users get no such hint.)

Reproduction (net10.0, Linux, current main @ 73de151)

using ktsu.RunCommand;
List<string> lines = new();
int active = 0, overlap = 0;
void Add(string l) { if (Interlocked.Increment(ref active) > 1) Interlocked.Increment(ref overlap); lines.Add(l); Thread.SpinWait(50); Interlocked.Decrement(ref active); }
await RunCommand.ExecuteAsync("sh", new[] { "-c", "for i in $(seq 1 20000); do echo out$i; echo err$i >&2; done" },
    new LineOutputHandler(Add, Add));
Console.WriteLine($"{lines.Count} lines (expected 40000), overlapping callbacks {overlap}");

Result: all 20 of 20 runs lost lines, between 18 and 553 per run (e.g. 39447 lines (expected 40000), overlapping callbacks 27911). The overlap counter shows the two delegates running inside each other tens of thousands of times per run. The SpinWait only widens the window, since the counter shows the overlap happening anyway.

In a worse case, the concurrent List<T>.Add can also throw (e.g. IndexOutOfRangeException during a resize). That exception comes out of the reader task, which lands on the hang described in #89.

Suggested fix

Pick one of these and pin it with a test:

1. Serialise delivery (preferred). Take one lock (per OutputHandler or per run) around each onData invocation in ReadToEnd, so stdout and stderr delegates never run at the same time. The reads can still run concurrently, and only delivery is serialised. The cost is small next to a pipe read. Callers can then share state between the two delegates safely, which is what the README examples (Console.Write for both) already suggest.
2. Or document the contract. State in the OutputHandler/LineOutputHandler XML docs and the README that the two delegates may run at the same time on different threads, and that shared state needs synchronisation.

Acceptance criteria

• A test that runs a command interleaving many stdout/stderr lines into a single non-thread-safe List<string> via both delegates gets every line back, repeated enough times to be reliable (the repro above fails every run today). Alternatively, the documentation states the concurrent-invocation contract explicitly.
• Delivery order within each stream is unchanged.

[matt-edmondson]

Triage

• Category: Bug (output is lost silently when both delegates share caller state, which is the most natural way to use the API)
• Priority: High. The repro loses lines on every run (20 of 20), with no exception. It can also throw inside the reader task and land on the hang in ExecuteAsync hangs forever (and leaves the child blocked) when the output handler throws or a strict encoding hits a decode error #89. RunCommand sits underneath other org tools: KtsuBuild uses it, and GitBranchStateCache is moving onto it in PR Bump the ktsu group with 11 updates #51. Any consumer that collects stdout and stderr into one list or StringBuilder can drop build or git output without knowing.
• Area / suggested assignment: RunCommand/AsyncProcessStreamReader.cs (Start / ReadToEnd), plus the OutputHandler/LineOutputHandler docs and README.
• Duplicates: none. Related to ExecuteAsync hangs forever (and leaves the child blocked) when the output handler throws or a strict encoding hits a decode error #89. A concurrent List<T>.Add throwing is one way to reach that hang, but the root causes are different.
• In progress: no. Open PR Kill the process tree whenever a cancelled wait ends the call [patch] #94 ("Kill the process tree whenever a cancelled wait ends the call") also touches the call lifecycle but doesn't serialise delivery. Rebase whichever lands second.

Suggested next step: take option 1 (serialise delivery with one lock per run around each onData call). It's a few lines, the reads stay concurrent, and it makes the README's Console.Write-for-both examples correct as written. Documenting the contract instead would leave every existing consumer to find the race on their own. Once it lands, check whether KtsuBuild's and GitBranchStateCache's handlers carry their own locks that could be removed.

---

Generated by Claude Code