From 62ed5a1c081b111b93c01b8d1d25e653da8e9836 Mon Sep 17 00:00:00 2001 From: "Claude (noether)" Date: Thu, 23 Jul 2026 19:26:10 +0200 Subject: [PATCH] sicd: reap on child-exit instead of blocking on stdin (fixes the v2 stdin hang) The v2 client (0.2.0) forwards its own stdin and only closes the ssh stdin channel when that stdin hits EOF. When sic is invoked with an inherited stdin that never closes (a pipe held open by the caller -- exactly how non-interactive tooling runs it), the channel stays open, and pumpStdin's `io.Copy(child.stdin, ssh-stdin)` stayed BLOCKED on the read long after the remote command had already exited. sicd never reaped, ssh never returned, and the whole call hung until stdin EOF or a timeout. Measured: `sic host true it did not need the rest of stdin; stop forwarding, abandon the read, return the child's status. (the hang fix) * copy ends first (EOF/EPIPE/error) -> its outcome is meaningful; a genuine read error that truncated the transfer to a still-live child is still a non-zero failure. * near-simultaneous -> a bounded 50ms window still catches a real truncation, but a genuinely blocked stdin can no longer re-introduce the hang. Daemon-only change; the client is untouched, and v1/v2 dual-read is unaffected. New regression TestPumpStdinReturnsOnChildExitDespiteBlockedStdin (never-EOFing stdin + exited child -> prompt child status, not a hang). Existing pump tests (read-error->1, EPIPE head-semantics, clean EOF) unchanged and green. Verified live: bare `sic host true` 20s -> 3.9s; piped/redirected stdin still forward. Co-Authored-By: Claude Opus 4.8 --- cmd/sicd/main.go | 48 ++++++++++++++++++++++++++++++++++++------- cmd/sicd/sicd_test.go | 22 ++++++++++++++++++++ 2 files changed, 63 insertions(+), 7 deletions(-) diff --git a/cmd/sicd/main.go b/cmd/sicd/main.go index 7443077..17c6672 100644 --- a/cmd/sicd/main.go +++ b/cmd/sicd/main.go @@ -13,6 +13,7 @@ import ( "strconv" "strings" "syscall" + "time" ) func readNetstring(buf []byte) ([]byte, bool) { @@ -344,14 +345,47 @@ func runV1(r *bufio.Reader) { // os.read to raise OSError), since no real fd on this platform yields a read error on stdin // (a pty slave and a socket peer both see a clean kernel EOF on hangup, in Go AND CPython). func pumpStdin(dst io.WriteCloser, src io.Reader, reap func() int, errOut io.Writer) int { - _, err := io.Copy(dst, src) - dst.Close() - if err != nil && !errors.Is(err, syscall.EPIPE) { - fmt.Fprintf(errOut, "sicd: forward stdin: %v\n", err) - reap() - return 1 + // Race the stdin copy against the child's exit. The old code did io.Copy THEN reap, so a + // client that never closes its stdin (holds the ssh channel open with no bytes) kept the + // copy blocked on the READ long after the child had exited — sicd never reaped, ssh never + // returned, the whole call hung until stdin EOF or a timeout. Now: whichever ends first wins. + copyDone := make(chan error, 1) + go func() { + _, err := io.Copy(dst, src) + dst.Close() // a still-reading child sees EOF + copyDone <- err + }() + childExited := make(chan int, 1) + go func() { childExited <- reap() }() + + select { + case err := <-copyDone: + // The stdin transfer concluded (EOF / EPIPE / error) — its outcome is meaningful. + if err != nil && !errors.Is(err, syscall.EPIPE) { + // A genuine read error truncated the transfer to a still-live child -> failure, + // regardless of the child's own eventual status (the child cannot tell a truncated + // EOF from a clean one). + fmt.Fprintf(errOut, "sicd: forward stdin: %v\n", err) + <-childExited + return 1 + } + return <-childExited + case status := <-childExited: + // The child exited BEFORE the copy finished: it did not need the rest of stdin (e.g. a + // command that ignores stdin, like `true`). Stop forwarding — abandon the (possibly + // blocked) read. This is the hang fix. Still honour a truncation the child DID care + // about if the copy is finishing this very instant, but bound the wait so a genuinely + // blocked stdin cannot re-introduce the hang. + select { + case err := <-copyDone: + if err != nil && !errors.Is(err, syscall.EPIPE) { + fmt.Fprintf(errOut, "sicd: forward stdin: %v\n", err) + return 1 + } + case <-time.After(50 * time.Millisecond): + } + return status } - return reap() } func writeAll(w io.Writer, data []byte) error { diff --git a/cmd/sicd/sicd_test.go b/cmd/sicd/sicd_test.go index bf95c2d..4708837 100644 --- a/cmd/sicd/sicd_test.go +++ b/cmd/sicd/sicd_test.go @@ -591,6 +591,28 @@ func TestPumpStdinEpipeIsHeadSemanticsReturnsChildStatus(t *testing.T) { } } +// blockingReader.Read blocks forever — models a client that holds its stdin open and never +// sends EOF (exactly how the Bash tool invokes sic non-interactively). +type blockingReader struct{} + +func (blockingReader) Read([]byte) (int, error) { select {} } + +func TestPumpStdinReturnsOnChildExitDespiteBlockedStdin(t *testing.T) { + // Regression for the fleet-wide hang: a never-EOFing stdin must NOT keep sicd blocked in the + // copy after the child has exited. pumpStdin must return the child's status promptly. + dst := &errWriteCloser{} + done := make(chan int, 1) + go func() { done <- pumpStdin(dst, blockingReader{}, func() int { return 7 }, io.Discard) }() + select { + case code := <-done: + if code != 7 { + t.Fatalf("expected the child's status 7, got %d", code) + } + case <-time.After(2 * time.Second): + t.Fatal("pumpStdin hung on a never-EOFing stdin after the child exited (the bug)") + } +} + func TestPumpStdinCleanEofReturnsChildStatus(t *testing.T) { src := bytes.NewReader([]byte("all of the stdin, cleanly")) dst := &errWriteCloser{}