fix(installer): close stdin in finally, and drain before the write (backend#2246)

[LukasWodka]

Closes tracebloc/backend#2246 — Bugbot finding discussion_r3821144936, deferred at the staging → main prod hop.

The claim, and what actually fires

Bugbot flagged four lines in Invoke-BoundedProcess:

try { $proc.StandardInput.Write($Stdin); $proc.StandardInput.Close() } catch { }

Close() sits in the same try as Write(), so a broken-pipe (or any) throw from Write() skips closing the child's stdin. Callers such as docker exec -i … sh then wait for EOF until the 60s bound and return 124 instead of the child's real exit.

That causal chain does not fire, and the measurement says why. The only exception actually raised here is a broken pipe, and a broken pipe means the child has stopped reading — so it is not sitting waiting for EOF. The two states are near mutually exclusive. Constructed it against the unfixed code: /usr/bin/true with a 200 KB payload returns the child's real Code=0 in 0.2s, skipped Close() and all. No 124, no hang.

But the flagged lines are genuinely defective, for a worse reason. The ReadToEndAsync() drains started after the stdin write. So a child that both reads stdin and writes output deadlocks once the payload passes the ~64 KiB pipe buffer: it fills its stdout pipe, blocks because nobody is draining it, therefore stops reading stdin, therefore our Write() blocks — and WaitForExit($TimeoutSec * 1000) is never reached. Not a 124: no return at all. The hard timeout that is this function's entire reason to exist silently disappears.

Constructed inputs, before → after

-TimeoutSec 20 throughout, each case run in a job under a 60s outer watchdog (the failure is an unbounded hang, so a direct call would hang the whole run rather than fail one test):

#	Child	Payload	Before	After
A	sh -c 'cat >/dev/null; exit 7'	5 B	Code=7, 0.1s	Code=7, 0.1s
B	/usr/bin/true (already exited)	200 KB	Code=0, 0.3s	Code=0, 0.2s
C	/bin/cat (reads stdin and echoes)	200 KB	HUNG past 60s	Code=0, 0.2s, all 200000 B returned
D	sh -c 'cat >/dev/null; exit 7' (quiet)	200 KB	Code=7, 0.1s	Code=7, 0.1s

C is the defect; A, B and D are unchanged, which is the point — B is the broken-pipe case the existing test already covered, and it still returns the child's verdict rather than our plumbing.

Note why B passing does not cover C: /usr/bin/true produces no output, so there is no stdout backpressure. Both are 200 KB; only one deadlocks.

Reachability — stated plainly

Neither mechanism is reachable from today's call sites. There are exactly two:

• docker login … --password-stdin — a credential
• docker exec -i <node> sh — a generated prep script, measured at ~304 bytes

Both are orders of magnitude under the 65536-byte pipe buffer, so neither can deadlock, and neither hits the broken-pipe path in normal operation. This PR hardens a latent contract violation in a general-purpose bounded-exec helper. It is not a live-incident fix, and I am not claiming it is one.

I still think it is worth landing: the function's advertised contract is a hard timeout, and there is a constructible input for which that contract fails completely rather than degrading. The change is four lines, strictly safer, and provably inert on every reachable path.

The change

• the two ReadToEndAsync() drains move ahead of the stdin write
• Close() moves into a finally, so it runs even when Write() throws (and keeps its own inner guard, since Close() flushes and can raise the same broken pipe)

The swallow itself stays. A pipe error is about our plumbing, not the command, and reporting it would replace the child's real verdict — the existing comment block explains this and it still holds.

Tests

The old source guard pinned Write(...); Close() as one literal blob, so it spoke for two independent properties at once and had to be rewritten to change either. Replaced with narrower checks plus the behavioural case that actually reddens:

1. write guard (try { Write($Stdin) } catch)
2. finally placement, andShould -Not -Match the chained shape that regressed
3. drain-before-write ordering, derived by index position from the real function body — and failing closed: each anchor must be -BeGreaterThan -1 first, so two missing anchors cannot compare equal and pass as agreement
4. behavioural: case C above, asserting the specific failure — "deadlocked, outlived a 60s watchdog despite -TimeoutSec 20" is a distinct assertion from Code -ne 124, so a hang is never reported as a timeout, and the full 200000 bytes must round-trip (proof the child was drained, not merely that something returned fast)

The deadlock case is gated on Test-Path /bin/cat — the real precondition — not on $IsWindows, which does not exist under the Windows PowerShell 5.1 that install.ps1 pins and would have turned a missing binary into a false regression.

Mutation table

Anchor occurrences asserted == 1 before each replacement, so an inert mutation cannot masquerade as coverage; both restores verified byte-identical with cmp.

Mutation	Anchors	Result
M1 readers moved back after the stdin write	readers=1, waitforexit=1	ordering guard RED (drain 3976 > write 3851); deadlock case RED (outlived a 60s watchdog despite -TimeoutSec 20). 3 passed / 2 failed
M2Close() chained back into the write's try	finally-close=1	finally guard RED; write guard RED. 3 passed / 2 failed

Also in this PR

scripts/manifest.sha256 regenerated. install-k8s.ps1 is in the supply-chain integrity manifest that install.sh verifies, and make drift caught the stale digest — the guard did its job. Digests only; the cosign signature is the release workflow's.

Verification — macOS 26.5.2, pwsh 7.5.2, Pester 6.0.1

install-k8s.Tests.ps1 749 total, 736 passed, 0 failed, 13 skipped
origin/develop baseline 746 total, 733 passed, 0 failed, 13 skipped → exactly +3, no skip change
installer-parity.Tests.ps1 3/3
install.Tests.ps1 41/41 (incl. "all installer scripts verified against the signed manifest")
telemetry.Tests.ps1 114/114
make lint green — 54 scripts parsed, 61 shellchecked
make drift all 18 guards green
make bats 1302 tests, 0 failures, exit 0 (full suite)
Parser::ParseFile clean on both changed files

bash -n / shellcheck are N/A to the diff — no shell file changed; the PowerShell equivalent (Parser::ParseFile) is above, and make lint was run regardless.

Gaps

• Windows: green in CI, but no native reproduction of the deadlock. I could not run Pester (windows-latest) from macOS; it has since passed on this PR, along with Pester (ubuntu-latest), Lint and Cursor Bugbot. The three source guards are platform-independent and run there; the behavioural deadlock case skips on Windows by design (no /bin/cat). I did not find a Windows child I could confirm streams rather than buffers stdin, so I did not guess at one — the Windows-native reproduction is left uncovered rather than faked, and the ordering guard is what protects that platform from a reorder.
• Timings are indicative only; this machine was heavily loaded. The pass/fail verdicts are what the outer watchdog decides, not wall-clock.

[saqlainsyed007]

Verified at head: Invoke-BoundedProcess now starts the ReadToEndAsync() output/error readers before the StandardInput.Write($Stdin), and closes stdin in a finally. That makes the structure deadlock-safe — a child that both reads stdin and writes >64 KiB no longer stalls the write (readers keep the pipe drained), and a broken-pipe throw from Write() still EOFs the child so WaitForExit/the hard timeout can fire. CI green, Bugbot pass, no open threads. LGTM.