sicd: cover the read-error branch with a unit test; correct the skip rationale; drop vestigial rest

Acting on @reviewer's fourth-pass verdict (all three points accepted): 1. Refactored the pump into pumpStdin(dst, src, reap, errOut) int and covered it with unit tests using a mock erroring io.Reader — which IS the Python reference's mechanism (monkeypatching os.read to raise OSError), translated. Three cases: a genuine read error reaps + exits 1 + diagnoses; EPIPE is head-semantics (return child status, no diagnostic); clean EOF returns child status. The waitForChild added in the prior commit now has coverage. 2. Corrected the pty test's skip string. My earlier rationale was wrong twice, as @reviewer's raw syscall.Read probe showed: it is NOT a Go-vs-CPython poller difference — the KERNEL returns a clean EOF on the slave side in BOTH languages (EIO-after-hangup is master-side); and the Python reference never used a pty (it mocks os.read). The skip now states the corrected facts and points at TestPumpStdin*. 3. Dropped readNetstring's vestigial third return (rest was provably always empty). Go ok (incl. 3 new unit tests), 27/27 Python, go vet clean.
2026-07-23 07:42:14 +02:00
parent 3982706ebb
commit b28f9d3722
2 changed files with 114 additions and 33 deletions
@@ -13,35 +13,35 @@ import (
 	"syscall"
 )

-func readNetstring(buf []byte) ([]byte, []byte, bool) {
+func readNetstring(buf []byte) ([]byte, bool) {
 	i := bytes.IndexByte(buf, ':')
 	if i < 0 {
-		return nil, nil, false
+		return nil, false
 	}
 	// Reject non-digit characters (including leading '+' or '-')
 	for _, c := range buf[:i] {
 		if c < '0' || c > '9' {
-			return nil, nil, false
+			return nil, false
 		}
 	}
 	n, err := strconv.Atoi(string(buf[:i]))
 	if err != nil || n < 0 {
-		return nil, nil, false
+		return nil, false
 	}
 	// Sanity cap: reject lengths > 1<<24
 	if n > 1<<24 {
-		return nil, nil, false
+		return nil, false
 	}
 	start := i + 1
 	end := start + n
 	if len(buf) < end+1 || buf[end] != ',' {
-		return nil, nil, false
+		return nil, false
 	}
 	// Reject trailing data after the netstring's closing comma
 	if len(buf) > end+1 {
-		return nil, nil, false
+		return nil, false
 	}
-	return buf[start:end], buf[end+1:], true
+	return buf[start:end], true
 }

 func waitForChild(cmd *exec.Cmd) int {
@@ -84,7 +84,7 @@ func main() {
 		os.Exit(1)
 	}

-	content, _, ok := readNetstring(nsBuf)
+	content, ok := readNetstring(nsBuf)
 	if !ok {
 		fmt.Fprintf(os.Stderr, "sicd: invalid netstring\n")
 		os.Exit(1)
@@ -132,19 +132,27 @@ func main() {
 		os.Exit(waitForChild(cmd))
 	}

-	// Pump remaining stdin to child. EPIPE on the write side means the
-	// child stopped reading (| head semantics) — that is not an error.
-	// Any other error (EIO, etc.) means sicd's own stdin failed mid-stream
-	// and the transfer was truncated — exit non-zero with a diagnostic.
-	if _, err := io.Copy(stdinPipe, os.Stdin); err != nil && !errors.Is(err, syscall.EPIPE) {
-		fmt.Fprintf(os.Stderr, "sicd: forward stdin: %v\n", err)
-		stdinPipe.Close()
-		waitForChild(cmd)
-		os.Exit(1)
-	}
-	stdinPipe.Close()
+	os.Exit(pumpStdin(stdinPipe, os.Stdin, func() int { return waitForChild(cmd) }, os.Stderr))
+}

-	os.Exit(waitForChild(cmd))
+// pumpStdin copies src (sicd's own stdin) into the child's stdin (dst), closes dst so the
+// child sees EOF, then reaps. EPIPE from the WRITE side means the child stopped reading
+// (| head semantics) — not an error, so the child's own status is returned. Any OTHER error
+// (a genuine read/write failure) means the transfer was truncated: report a diagnostic, reap
+// to avoid a zombie, and return non-zero REGARDLESS of the child's status — a truncated
+// transfer must never be reported as success. Extracted so a mock erroring io.Reader can
+// cover this branch: that is exactly how the Python reference tests it (monkeypatching
+// 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
+	}
+	return reap()
 }

 func writeAll(w io.Writer, data []byte) error {
@@ -457,18 +457,13 @@ func TestFramingAndPumpSurviveTrickledStdin(t *testing.T) {
 }

 func TestStdinReadErrorDiagnosedNotSwallowed(t *testing.T) {
-	t.Skip("unsatisfiable in Go, and the premise is a Python-ism. The original fault injection " +
-		"(open a pty, close the master mid-pump) assumed a hung-up read returns EIO, as CPython's " +
-		"os.read() does. Go's runtime poller instead translates the hang-up to a clean EOF " +
-		"(probed: slave.Read -> io.EOF, io.Copy -> nil; a socketpair peer close is EOF too, since " +
-		"SO_LINGER/RST is TCP-only and AF_UNIX close delivers EOF). No fd mechanism available to a " +
-		"test produces a distinguishable READ error on sicd's stdin. This is not a gap in main.go: " +
-		"sicd's trailing stdin is UNFRAMED by design, so a truncated source and a clean end are " +
-		"both EOF and cannot be told apart — the pump treats EOF as a legitimate end (correct) and " +
-		"already reaps-then-exits-nonzero on a genuine non-EPIPE error, which simply does not arise " +
-		"via pty/socket close in Go. Two models burned 20+ grind attempts here before the premise " +
-		"was probed. Proper coverage = refactor the pump into a unit-testable func driven by an " +
-		"io.Reader that returns a non-EOF error.")
+	t.Skip("no real fd yields a read error on sicd's stdin on this platform, so this " +
+		"integration-style injection is unsatisfiable — a pty slave and a socket peer both see a " +
+		"clean KERNEL EOF on hangup (probed: raw syscall.Read -> n=0, errno=0), identically in Go " +
+		"and CPython; EIO-after-hangup is master-side, not slave-side. The Python reference does " +
+		"NOT use a pty here — it monkeypatches os.read to raise OSError(EIO). The Go equivalent is " +
+		"a mock erroring io.Reader, which now covers the reap-then-exit-nonzero branch as a UNIT " +
+		"test (TestPumpStdin*). This black-box test is kept only as a placeholder for that contract.")
 }

 // --- pty plumbing (Linux) ----------------------------------------------------
@@ -518,3 +513,81 @@ func openPty(t *testing.T) (master, slave *os.File) {
 	}
 	return master, slave
 }
+
+// erroringReader yields data once, then err — the Go translation of the Python reference's
+// fault injection (monkeypatching os.read to raise OSError). Lets us cover pumpStdin's
+// read-error branch without an fd, since no real fd produces a read error on stdin here.
+type erroringReader struct {
+	data []byte
+	err  error
+	done bool
+}
+
+func (r *erroringReader) Read(p []byte) (int, error) {
+	if !r.done && len(r.data) > 0 {
+		n := copy(p, r.data)
+		r.data = r.data[n:]
+		if len(r.data) == 0 {
+			r.done = true
+		}
+		return n, nil
+	}
+	return 0, r.err
+}
+
+type errWriteCloser struct {
+	buf      bytes.Buffer
+	writeErr error // if set, Write returns it (e.g. EPIPE from a widowed pipe)
+	closed   bool
+}
+
+func (w *errWriteCloser) Write(p []byte) (int, error) {
+	if w.writeErr != nil {
+		return 0, w.writeErr
+	}
+	return w.buf.Write(p)
+}
+func (w *errWriteCloser) Close() error { w.closed = true; return nil }
+
+func TestPumpStdinReapsAndExitsNonzeroOnReadError(t *testing.T) {
+	src := &erroringReader{data: []byte("partial payload"), err: syscall.EIO}
+	dst := &errWriteCloser{}
+	reaped := false
+	var errOut bytes.Buffer
+	code := pumpStdin(dst, src, func() int { reaped = true; return 0 }, &errOut)
+	if code != 1 {
+		t.Fatalf("a genuine read error must exit non-zero regardless of child status, got %d", code)
+	}
+	if !reaped {
+		t.Fatal("the child must still be reaped on a read error (no zombie)")
+	}
+	if !dst.closed {
+		t.Fatal("the child's stdin must be closed so it sees EOF")
+	}
+	if !bytes.Contains(errOut.Bytes(), []byte("forward stdin")) {
+		t.Fatalf("a diagnostic must be written, got: %q", errOut.String())
+	}
+}
+
+func TestPumpStdinEpipeIsHeadSemanticsReturnsChildStatus(t *testing.T) {
+	// EPIPE from the write side = the child stopped reading (`| head`) — NOT an error.
+	src := &erroringReader{data: []byte("x"), err: io.EOF} // clean src; the write side EPIPEs
+	dst := &errWriteCloser{writeErr: syscall.EPIPE}
+	var errOut bytes.Buffer
+	code := pumpStdin(dst, src, func() int { return 42 }, &errOut)
+	if code != 42 {
+		t.Fatalf("EPIPE is head-semantics; must return the child's own status 42, got %d", code)
+	}
+	if errOut.Len() != 0 {
+		t.Fatalf("EPIPE must not emit a diagnostic, got: %q", errOut.String())
+	}
+}
+
+func TestPumpStdinCleanEofReturnsChildStatus(t *testing.T) {
+	src := bytes.NewReader([]byte("all of the stdin, cleanly"))
+	dst := &errWriteCloser{}
+	code := pumpStdin(dst, src, func() int { return 7 }, io.Discard)
+	if code != 7 {
+		t.Fatalf("a clean EOF is a legitimate end; must return the child's status 7, got %d", code)
+	}
+}