Let a caller close a command's standard input - #82
Conversation
Standard output and standard error were redirected but standard input was not, so a command inherited the caller's handle and a command that reads it waited there. In a host that is not a console that handle never produces data and never closes, so the wait ended only on cancellation: a hang rather than an error, on the callers least able to notice it. CommandOptions.StandardInput selects between inheriting, which is what commands did before and what an interactive command needs, and closing, which gives the command its own standard input and closes it so a read reports end of stream. Redirecting alone is not enough - it leaves the command holding a pipe nobody writes to, which is the same wait - so the stream is closed immediately after the process starts. Elevation forces UseShellExecute, which has no stream to redirect, so combining the two throws up front as EnvironmentVariables already does. Two tests, because one is not enough to catch both ways this can regress. The behavioural test fails if the stream is redirected but left open. The second compares /proc/self/fd/0 between the caller and the command, which is what catches the option being ignored altogether: a test runner whose own standard input is already at end of stream hands a child the same answer by inheritance, so the behavioural test alone passes there even when nothing is redirected. Both were confirmed to fail against a mutated build. Fixes #81 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Sd7rWEYm4btqSsd2Hbuic
The Windows arm of GetReadStandardInputCommand echoed %line%, which came back literal: at a cmd /c command line an undefined variable is left as written rather than expanding to nothing, which is a batch-file behaviour and not a command-line one. So the report read "read:[%line%]" and said nothing about what the read did. /v:on and !line! expand when the echo runs rather than when the line is parsed, so an undefined variable reports as empty and the Windows arm now means what the POSIX one means. The prompt is left empty by putting nothing between = and the separator, so no prompt text reaches the captured output. Test-only. The library change was already right on Windows: the failing run completed the command in 64ms rather than waiting, which is the behaviour under test - standard input was redirected and closed, and set /p returned at end of stream. Only the assertion's report was wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Sd7rWEYm4btqSsd2Hbuic
Neither expansion survives this command line. %line% came back literal, because expanding an undefined variable to nothing is batch-file behaviour rather than command-line behaviour, and !line! under /v:on came back literal too. Both reported the variable's own name instead of what the read did, so the assertion was checking nothing on Windows either way. The command now reports through control flow: if defined is a run-time test on the name and needs no expansion, and every string reaching standard output is a literal. set line= runs first so a variable inherited from the environment cannot make it report a read that never happened. Still test-only, and the behaviour under test has been green on Windows throughout: both failing runs completed the command in well under the token's 30 seconds, which is what says standard input was redirected and closed and that set /p returned at end of stream. Only the report was wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Sd7rWEYm4btqSsd2Hbuic
Two Windows-only rounds, both in the test's report rather than in the changeRecording this because it is a constraint on how I can verify, not just a pair of typos. The behaviour under test has been green on Windows the whole time. Both failing runs completed the command in well under the 30-second token — 64ms on the first — and produced output. That is precisely what the test exists to prove: standard input was redirected and closed, and What failed twice was how the Windows command reported the read:
Both are the same mistake — asserting on a value that has to survive
The constraint worth knowing: I am working from a Linux container with no Windows and no Wine, so I cannot execute the Windows arm before pushing — only the Linux and macOS arms, which stay green (43 total, 41 passed, 2 skipped, in both Debug and Release). The two rounds above were reasoning about If it fails again, the useful next step is probably to drop the content assertion on the Windows arm and let it assert termination and exit code only — the hang is the defect, and termination is the part Windows can prove without a shell-quoting puzzle. I would rather land that than keep iterating blind, but it loses a little coverage, so I have not done it pre-emptively. Generated by Claude Code |
Windows is green; the macOS job on this head stalled on its runnerThe Windows arm is fixed. On
I have cancelled run 36100037511 and will re-run it once the cancellation settles. That is the one re-run — if macOS fails again on a re-run, it is real and I will treat it as this PR's and root-cause it rather than re-running a second time. Nothing about the change is in question here: no code is being altered for this, and the library diff has now been green on all three platforms across the run history (ubuntu and macOS from the first run, Windows as of this one). Generated by Claude Code |
Green on all three platforms — the macOS stall was the runner, as expectedThe re-run confirms it. Everything on The three SonarCloud findings stay as they areThe quality gate passed. All three are
They only surface here because Sonar scopes to new code — the pre-existing instances of the identical pattern sit outside the leak period. Converting only the three new ones would leave the file inconsistent with itself, so if Nothing further from me on this PR — it is waiting on review. Generated by Claude Code |
The method carried two <remarks> elements, because the Windows note was appended as a second block rather than merged into the existing one. A member takes one, so a documentation tool reading this would keep one and drop the other. It builds clean either way, which is why it survived: the method is private, so no documentation warning fires on it. The Windows note is now a <para> inside the single remarks element. Text unchanged, no behaviour touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Sd7rWEYm4btqSsd2Hbuic
|
2 participants
Fixes #81
The defect
CreateStartInforedirected standard output and standard error but never touched standard input, soRedirectStandardInputstayedfalseand the command inherited the caller's handle. A command that reads standard input then waited on that handle — and in a host that is not a console it never produces data and never closes, so the run ended only on cancellation.Measured at
main(d06609e), .NET SDK 10.0.401, Linux, via a console app callingExecuteAsync("/bin/sh", ["-c", "read x; echo \"got:[$x]\""])with an 8-second token:StandardInputMode.Closed)TaskCanceledExceptionafter 8.0sexit=0in 0.1s, outputgot:[]/dev/nullexit=0in 0.1sexit=0in 0.1sThe change
CommandOptions.StandardInputtakes a newStandardInputMode:Inherit(default) — what commands did before, and what an interactive command needs.Closed— redirects standard input and closes it right afterProcess.Start, so a read reports end of stream.Additive and non-breaking: the default preserves current behaviour, matching the type's stated contract that an instance with nothing set is equivalent to not passing one at all.
Closing is deliberately part of the mode rather than a consequence of redirecting. Redirecting alone leaves the command holding a pipe nobody writes to, which is the same wait as inheriting; closing is what turns a read into end of stream.
Elevation.Elevatedon Windows forcesUseShellExecute, which offers no stream to redirect, so that combination throwsArgumentExceptionup front exactly asEnvironmentVariablesalready does.Tests
Two, because one does not catch both ways this can regress. Both were confirmed to fail against a deliberately mutated build and to pass against the fix:
ExecuteAsyncShouldEndACommandThatReadsStandardInputWhenStandardInputIsClosedExecuteAsyncShouldGiveTheCommandItsOwnStandardInputWhenClosedgot: socket:[21606]The second exists because the first is not sufficient on its own. A test runner whose standard input is already at end of stream hands a child the same answer by inheritance, so a purely behavioural test passes on such a machine even when nothing is redirected. It compares
/proc/self/fd/0between the caller and the command, and is Linux-gated for that reason — this repo's own test host turned out to have a live socket on standard input, which is why the first test also fails when mutated here.ExecuteAsyncShouldRejectClosedStandardInputCombinedWithElevationmirrors the existing environment-variable test and is Windows-gated like it.Verification
dotnet build RunCommand.sln -c Release— succeeded, 0 warningsdotnet test RunCommand.Test -c Release— 43 total, 41 passed, 2 skipped, 0 failed (the two skips are the Windows-gated elevation tests)Not in this change
Whether
Closedshould become the default is a separate call and deliberately left alone. There is an argument for it — standard output and standard error are already redirected away from the caller's console, so a command that prompts cannot be answered anyway — but it would change behaviour for any caller relying on inheritance today, which is a maintainer's decision rather than something to fold into a bug fix.Context
ktsu-dev/GitBranchStateCache#27is blocked on this. ItsGitRunnersetsRedirectStandardInput = trueand closes the stream by hand, with a comment recording that a child holding an open standard input it is waiting on "is a hang rather than an error". That issue's triage named a stdin option onCommandOptionsas one of the two things that would unblock it; this is that option. The other — the strict-UTF-8 decode failure — was already fixed in v1.6.0 by "HonourOutputHandler.Encodingregardless of a byte order mark", which I re-checked against currentmain.Docs updated: a Standard Input section in
README.mdplus theCommandOptionsandStandardInputModeAPI tables, and the key-files, elevation-constraints and a new standard-input section inCLAUDE.mdrecording why testing this needs the descriptor comparison.🤖 Generated with Claude Code
https://claude.ai/code/session_017Sd7rWEYm4btqSsd2Hbuic
Generated by Claude Code