A reused LineOutputHandler prepends a cancelled run's partial line to the next run's first line ("partial" + "hello" → "partialhello")

[matt-edmondson]

What's wrong

LineOutputHandler holds unterminated text in outputBuffer / errorBuffer (RunCommand/LineOutputHandler.cs:16-21). The only thing that clears them is Complete() (LineOutputHandler.cs:57-63), and its doc comment says it "leaves both buffers empty so a later run on this handler starts clean". Reusing a handler is therefore supported.

RunAsync calls Complete() only on the success path (RunCommand/RunCommand.cs:493-500). When a run is cancelled, cancellationToken.ThrowIfCancellationRequested() throws first, and the buffers keep the partial line. A run that faults, for example because a callback throws, keeps them too. Nothing resets the buffers when the next run starts, so the leftover text is glued onto that run's first line.

There's also a thread-safety problem. If a cancelled run's readers were abandoned because a descendant still holds the pipe, those readers can keep appending to the same unsynchronized buffers while the next run is using them.

Repro (verified on Linux, net10.0, at 12afce7)

var lines = new List<string>();
var h = new LineOutputHandler(l => lines.Add(l));

using (var cts = new CancellationTokenSource(1500))
{
    try { await RunCommand.ExecuteAsync("sh", ["-c", "printf partial; sleep 10"], h, cts.Token); }
    catch (OperationCanceledException) { }
}

await RunCommand.ExecuteAsync("sh", ["-c", "echo hello"], h);
// observed: lines == ["partialhello"]
// expected: lines == ["hello"]

Why it matters

A common pattern is to keep one handler, often a field, that forwards lines to a logger or UI, and run commands through it with a timeout or a cancel button. After any cancelled or failed run, the next command's first line is corrupted. A caller that parses output, such as git rev-parse or a version probe, gets a wrong value and no error.

Suggested fix

• Reset handler state at the start of each run: add an internal virtual void Reset() on OutputHandler, override it in LineOutputHandler to clear both buffers and any pending-CR state, and call it from RunAsync before Start. Alternatively, clear the buffers in a finally when a run does not complete normally. Either way, discard the leftover text rather than flushing it, because a cancelled run's partial line isn't a real line.
• An abandoned reader from a cancelled run must not be able to write into the buffers of a later run. For example, give each run its own buffer state, or have the reader check a per-run generation token.

Acceptance criteria

• The repro above yields ["hello"].
• A test covers reusing a handler after a cancelled run, and another covers reuse after a run whose callback threw.

[matt-edmondson]

Triage

• Category: Bug
• Priority: High. After any cancelled or faulted run, the next run's first line is silently corrupted when a handler is reused, a documented pattern. Callers that parse output (git rev-parse, version probes) get a wrong value with no error. Several org libraries (GitIntegration, GitBranchStateCache, KtsuBuild) build on RunCommand, so the impact spreads downstream. There is also a thread-safety hazard from readers abandoned by a cancelled run.
• Area / suggested assignee: LineOutputHandler.cs:16-21, 57-63 (buffers and Complete()) and RunCommand.cs:493-500 (Complete() only on the success path). Maintainer: @matt-edmondson.
• Duplicates: None. Related:
  • LineOutputHandler is quadratic in line length: a 16 MB line with no newline takes ~221 s (OutputHandler: 39 ms) and stalls the child #92 (quadratic LineOutputHandler buffering) will likely restructure the same buffers, so land the two in a coordinated order, or together, to avoid conflicts.
  • ExecuteAsync hangs forever (and leaves the child blocked) when the output handler throws or a strict encoding hits a decode error #89 (hang when the handler throws) covers the faulted-run path that also leaves stale buffers here.
• In progress: No open PR covers this.

Next steps: Add an internal Reset() on OutputHandler, overridden in LineOutputHandler to clear both buffers and pending-CR state, and call it at the start of RunAsync. Discard leftover text rather than flushing it. Isolate per-run buffer state, or use a generation token, so an abandoned reader can't write into a later run. Add tests for reuse after a cancelled run and after a run whose callback threw.

---

Generated by Claude Code