Skip to content

One LineOutputHandler shared by two concurrent ExecuteAsync runs drops one run's partial line and glues the two runs' output into one line #130

Closed

Description

@matt-edmondson

What's wrong

A LineOutputHandler keeps its line-splitting state in instance fields, outputBuffer and errorBuffer (RunCommand/LineOutputHandler.cs:20,26). RunAsync calls outputHandler.Reset() at the start of every run (RunCommand/RunCommand.cs:465), and Reset() swaps in fresh buffers (LineOutputHandler.cs:74-78).

When two runs overlap on the same handler:

  1. Run B's Reset() throws away the partial line run A had buffered.
  2. Both runs then append to the same StringBuilder, so text from A and text from B end up joined into one line.
  3. Each run has its own deliveryLock, so nothing serializes the two runs. Two threads modify the same StringBuilder at once, which is a data race that can corrupt the buffer or throw. It also breaks the promise in OutputHandler.cs:12-14 that the delegates are never run at the same time.

Reproduction

Linux, net10.0, HEAD b4116b5:

var lines = new List<string>();
var h = new LineOutputHandler(l => { lock (lines) lines.Add(l); });
var a = RunCommand.ExecuteAsync("sh", ["-c", "printf AAAA; sleep 0.5; printf 'AAAA\\n'"], h);
await Task.Delay(200);
var b = RunCommand.ExecuteAsync("sh", ["-c", "printf BBBB; sleep 0.6; printf 'BBBB\\n'"], h);
await Task.WhenAll(a, b);
  • Actual: "BBBBAAAA" | "BBBB"
  • Expected: "AAAAAAAA" | "BBBBBBBB", in either order.

Why it matters

Passing one logging handler to several commands run under Task.WhenAll (parallel builds, fan-out tooling) is a natural pattern. The README and the comments describe handlers as reusable, and nothing says they serve one run at a time. The result is output that is silently lost or garbled, and possibly an exception from inside the StringBuilder.

This is not covered by the existing reuse issues:

Suggested fix

Preferred: move the line-splitting state into a per-run object. For example, RunAsync creates an internal LineSplitter for each run, and chunks pass through it before the user's delegates are called. The handler then holds no per-run state, and Reset() is no longer needed.

Cheaper alternative: detect overlapping use with an Interlocked in-use flag, throw InvalidOperationException, and document that a handler serves one run at a time.

Acceptance criteria

  • With the per-run fix, the repro above delivers AAAAAAAA and BBBBBBBB intact, and a test covers two overlapping runs on one handler.
  • With the alternative fix, the second overlapping run throws a clear exception, and the limitation is documented.

Activity

  1. matt-edmondson commented on Oct 31, 2026

    @matt-edmondson
    ContributorAuthor

    Triage

    Next step: take the per-run splitter fix rather than the in-use guard, unless the API should state "one run per handler" outright. Add the overlapping-runs test from the repro.


    Generated by Claude Code

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

Metadata

Metadata

Labels

bugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions