Skip to content

Deliver a line ending in a bare CR at once instead of on the next write [patch] - #129

Merged
matt-edmondson merged 2 commits into
mainfrom
fix/128-emit-line-at-bare-cr
Nov 4, 2026
Merged

matt-edmondson merged 2 commits into
mainfrom
fix/128-emit-line-at-bare-cr

Conversation

@matt-edmondson

Copy link
Copy Markdown
Contributor

Fixes #128

Summary

Since the #85 fix, LineOutputHandler kept a line buffered whenever a read ended in \r, until the next read showed whether a \n followed. A progress line like Downloading 100%\r followed by a quiet period was only delivered when the command next wrote, or at EOF. In the issue's repro that was about 3 s late, where Process.OutputDataReceived delivers it at once.

This PR follows .NET's AsyncStreamReader:

  • A CR ends its line immediately, including as the last character of a read.
  • Each stream keeps a SkipLeadingLineFeed flag. When the next read starts with \n, that LF completes the CRLF and is skipped instead of producing an empty line.
  • Reset() replaces the per-stream state, flag included. Complete() clears the flag and no longer strips a trailing CR. CompletePendingCarriageReturn is removed.

The per-stream state is now a small internal LineBuffer (text plus flag). outputBuffer and errorBuffer keep their names, and ToString() still returns the partial line. Reset() still swaps in fresh objects, so a delivery from a cancelled run finishes against its own state.

Tests

  • Replaced HandleStandardOutputDataShouldHoldBackTrailingCarriageReturn, which pinned the delayed behaviour, with:
    • HandleStandardOutputDataShouldDeliverALineEndingInATrailingCarriageReturnAtOnce
    • HandleStandardErrorDataShouldDeliverALineEndingInATrailingCarriageReturnAtOnce
  • Added:
    • CompleteShouldNotDeliverALineAlreadyEndedByATrailingCarriageReturn
    • ResetShouldForgetAnLfStillDueToBeSkipped
  • Unchanged and still passing: the LineOutputHandler emits a spurious empty line when a CRLF line ending is split across two reads #85 CRLF-split tests ("x\r" + "\ny\n" → x, y) and the multi-read case ("a\r", "\nb\r", "\r", "c\n" → a, b, "", c).
  • With the fix reverted, three of the new tests fail. With it, all 17 LineOutputHandlerTests pass.
  • Full RunCommand.Test suite: 67 passed and 2 skipped out of 69, locally on Linux with the .NET 10 SDK.

Generated by Claude Code

…te [patch]

Since #85, a read that ended in CR kept its whole line buffered until the
next read showed whether an LF followed. A progress or status line ending
in a bare CR therefore arrived only when the command next wrote, or at EOF,
which could be seconds late.

A CR now ends its line immediately, even as the last character of a read.
Each stream remembers that its last read ended on a CR, and an LF at the
start of the next read completes the CRLF and is skipped instead of ending
an empty line, which is what Process.OutputDataReceived does. Reset() and
Complete() clear that state, and Complete() no longer strips a trailing CR.

The test that pinned the held-back CR is replaced by tests that the line is
delivered at once on both streams, that Complete() does not deliver it
again, and that Reset() forgets the pending LF skip. The #85 CRLF-split
tests are unchanged and still pass.

Fixes #128

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017vfJW24pNCmCLDhz6z3ZdY

Copy link
Copy Markdown
Contributor Author

Blocked: ci / .NET / Test on macos-latest hangs. The test step ran for about 55 minutes until it was cancelled. This happened on the first run, and again on the one re-run (job 124550149634). Ubuntu and Windows pass in 2–6 minutes.

I'm treating this as caused by this PR until shown otherwise. macOS passed on main (nightly, 2026-10-30, 1m21s) and on #127.

Why I couldn't root-cause it here:

  • Neither cancelled job kept a log. The logs endpoint returns 404 BlobNotFound, so I can't see which test was running.
  • This session has no macOS machine.
  • What I checked: the change only affects how LineOutputHandler splits lines on a bare CR. Every branch of the new loop advances i or returns, so it can't loop forever. No test in RunCommandTests writes a CR. Every wait I found in the tests has a timeout.

What would unblock it: run dotnet test --project RunCommand.Test/RunCommand.Test.csproj on a Mac with this branch, or run the macOS job with a hang dump enabled (for example --hangdump --hangdump-timeout 10m). Either would show the test that hangs. If it turns out not to reproduce on a local Mac, a fresh re-run of the macOS job would tell us whether the runner itself is at fault.


Generated by Claude Code

Resolves the conflict in LineOutputHandler.cs: keeps this branch's
immediate CR delivery with its per-stream skip-leading-LF state, and takes
main's CreateRunHandler (#132), its class remarks, and its narrowing of
IsLineBreak to LF only (#136).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017vfJW24pNCmCLDhz6z3ZdY

Copy link
Copy Markdown
Contributor Author

Following up on the macOS hang: it was the runner, not this change. The re-run on 2026-11-04 passed in 58 s on the same commit.

The PR had since picked up a merge conflict with main in LineOutputHandler.cs. I've merged main in, which brings in #132, #136 and #137. The resolution keeps this PR's immediate CR delivery and its per-stream skip-leading-LF state. From main, it takes CreateRunHandler, the class remarks, and IsLineBreak narrowed to LF only. The full suite passes locally on Linux with the .NET 10 SDK: 90 passed and 2 skipped out of 92. CI is re-running on the merge commit.


Generated by Claude Code

@sonarqubecloud

sonarqubecloud Bot commented Nov 4, 2026

Copy link
Copy Markdown

@matt-edmondson
matt-edmondson merged commit ac583c0 into main Nov 4, 2026
15 checks passed
@matt-edmondson
matt-edmondson deleted the fix/128-emit-line-at-bare-cr branch November 4, 2026 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants