Uh oh!
There was an error while loading. Please reload this page.
fix(installer): the stdin write must not throw when the child closed the pipe - #730
Conversation
…the pipe
Invoke-BoundedProcess wrote to the child's stdin unguarded:
if ($Stdin) { $proc.StandardInput.Write($Stdin); $proc.StandardInput.Close() }
If the process exits before or during that write -- `docker login` refusing
instantly because the daemon is down, a binary that rejects its args and returns,
a stub in the suite -- the pipe is already closed and .Write() raises
"Broken pipe". That escaped the function as a raw MethodInvocationException.
It is a CONTRACT break, not just noise. Three lines below, the function promises
`@{ Code = <int>; Output = <string> }`, and the header promises a wedged daemon
"can't hang the installer forever". Every other external interaction is already
defended for exactly this reason -- Process::Start has a try/catch, Kill() has a
try/catch -- and only the stdin write was bare. A user hitting it saw a
PowerShell stack trace instead of the child's own exit code and message.
Swallowing is correct here and is not a fail-open: a child that closed stdin has
already decided something, and its exit code and output are read below and
returned unchanged. The error raised is about OUR pipe, not about the command, so
reporting it would replace the child's real verdict with plumbing.
Found the way this class usually surfaces -- as a flake. It failed
`Pester (ubuntu-latest)` on the `main` tip minutes after the 2026-08-16 prod
promotion, on a test whose assertion never ran, while the identical content had
been green on the mirror 23 minutes earlier and passed on windows-latest in the
same run. A re-run cleared it, which is what makes this kind of defect easy to
wave through.
Two tests, and the first REPRODUCES the race rather than describing it: a child
that ignores stdin and exits immediately, with a 200 KB payload chosen to outlive
the OS pipe buffer (a short string is accepted even after exit, so a small payload
would pass with the bug present). The second is a source guard, so unwrapping the
try/catch fails even on a machine where the race happens not to fire.
Mutation-proved: removing the guard reddens BOTH, with the anchor asserted as
applied. Full suite 640 passed / 0 failed locally; `make check` green.
The integrity manifest is regenerated -- install-k8s.ps1 is covered by the signed
manifest Confirm-ScriptIntegrity verifies before any privileged step, so the
digest has to move with the file.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
saadqbal
left a comment
There was a problem hiding this comment.
Nice, careful fix. The guard sits only around the stdin Write/Close, and the child's exit code is still read and returned unchanged below it, so a swallowed broken pipe can't mask a real command failure. Bare catch {} matches the house idiom already used for Start()/Kill() in the same function, and for a stdin pipe there's no meaningful non-broken-pipe write error to lose. Verified locally: the bare write throws "Broken pipe" against /usr/bin/true, the guarded one returns Code 0, and the 200KB payload does trip the race rather than fitting the pipe buffer. manifest.sha256 matches the shipped file.
Uh oh!
There was an error while loading. Please reload this page.
LukasWodka
commented
Aug 19, 2026
/fr-pass Best-effort functional review passed (triage: merged clean, non-interactive; behavioral evidence limited while e2e journey is red — backend#2206). Advancing to Ready for prod. |
Summary
Invoke-BoundedProcesswrote to the child's stdin unguarded:If the process exits before or during that write —
docker loginrefusinginstantly because the daemon is down, a binary that rejects its args and returns,
a stub in the suite — the pipe is already closed and
.Write()raises"Broken pipe", which escaped as a raw
MethodInvocationException.This is a contract break, not just noise. Three lines below, the function
promises
@{ Code = <int>; Output = <string> }, and its header promises a wedgeddaemon "can't hang the installer forever". Every other external interaction is
already defended for exactly this reason —
Process::Starthas a try/catch,Kill()has a try/catch — and only the stdin write was bare. A user hitting itgets a PowerShell stack trace instead of the child's own exit code and message.
Swallowing is correct and is not a fail-open: a child that closed stdin has
already decided something, and its exit code and output are read below and
returned unchanged. The error raised is about our pipe, not about the command —
reporting it would replace the child's real verdict with plumbing.
How it surfaced — as a flake, which is why it's easy to wave through
It failed
Pester (ubuntu-latest)on themaintip minutes after the2026-08-16 prod promotion, on a test whose assertion never ran:
The identical content was green on the mirror 23 minutes earlier and passed on
windows-latestin the same run. A re-run cleared it.Type
fix
Test plan
Two tests, and the first reproduces the race rather than describing it:
chosen to outlive the OS pipe buffer — a short string is accepted even after
exit, so a small payload would pass with the bug present;
race happens not to fire.
Mutation-proved — removing the guard reddens both, anchor asserted as
applied:
Full suite 640 passed / 0 failed locally;
make checkgreen.Integrity manifest
scripts/manifest.sha256is regenerated —install-k8s.ps1is covered by thesigned manifest
Confirm-ScriptIntegrityverifies before any privileged step, sothe digest has to move with the file.
make driftcatches this if it doesn't.Checklist
developNote
Low Risk
Narrow installer helper change with defensive error handling; manifest hash updated for integrity checks.
Overview
Invoke-BoundedProcessininstall-k8s.ps1now wraps stdin write/close in try/catch so a child that exits before the write finishes (broken pipe) no longer throws aMethodInvocationExceptionand break the function’s@{ Code; Output }contract. Exit code and captured output are still returned unchanged.Adds Pester coverage that races a fast-exiting child with a 200 KB stdin payload plus a source guard on the try/catch. Updates
scripts/manifest.sha256for the modified installer script.Reviewed by Cursor Bugbot for commit 4d6c594. Bugbot is set up for automated code reviews on this repo. Configure here.