From 0cfed0e2440895eacc94d7f0d8fd5051558d2ef0 Mon Sep 17 00:00:00 2001 From: shujaat hasan Date: Mon, 10 Aug 2026 10:48:26 +0200 Subject: [PATCH] fix(push): a self-inflicted closed pipe must not mask the remote ingest error Hit for real during a Windows ingest. The remote script is `set -e; rm -rf D; mkdir -p D; tar -xf - -C D`, so when the dataset dir is root-owned (hostPath ignores fsGroup) `mkdir` fails with EACCES and the shell aborts before tar reads a single byte. StreamLayout's own pr.Close() then unblocks the tar goroutine with io.ErrClosedPipe, and because tarErr is reported first the customer saw only: Error: building tar archive: packaging : io: read/write on closed pipe The actionable cause -- "mkdir: can't create directory '/data/shared/': Permission denied", already captured in stderrBuf -- was discarded, turning a one-line permissions fix into a long investigation. io.ErrClosedPipe on the tar side is not a cause, it's the consequence of our own pr.Close() after exec returned early. So when tarErr is ErrClosedPipe AND streamErr is set, defer to streamErr, which carries the remote stderr hint. A tar error from any other cause (e.g. the stream-time size-cap recheck, whose diagnostic Bugbot originally asked to preserve) still takes precedence, and a closed-pipe tar error with no streamErr is still reported rather than swallowed. Tests: +2. The first pins the customer-visible outcome (remote "Permission denied" survives, "building tar archive" does not appear). The second guards the other direction using a DRAINED stream, so the tar goroutine reaches a genuine open error instead of a pipe error -- writing it undrained proved the premise wrong, since the first header write blocks and fails with ErrClosedPipe, which is exactly the self-inflicted case. Full suite green; gofmt + vet clean. Co-Authored-By: Claude Opus 4.8 --- internal/push/stream.go | 19 ++++++++- internal/push/stream_test.go | 82 ++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 1 deletion(-) diff --git a/internal/push/stream.go b/internal/push/stream.go index 7433022..4ba2867 100644 --- a/internal/push/stream.go +++ b/internal/push/stream.go @@ -295,7 +295,20 @@ func StreamLayout( // is usually the upstream cause. streamErr-only (no tarErr) // is the genuine network/RBAC/remote-tar-failed case where // the exec wording is the right surface. - if tarErr != nil { + // ...WITH ONE EXCEPTION: io.ErrClosedPipe on the tar side is SELF-INFLICTED. It's what + // the `pr.Close()` above produces when exec returned before draining stdin — i.e. the + // remote script died early. Reporting it as the cause hides the only actionable + // information we have (the remote stderr), and produces a genuinely dead-end message: + // + // Error: building tar archive: packaging : io: read/write on closed pipe + // + // A real customer hit exactly that. The remote script is `set -e; rm -rf D; mkdir -p D; + // tar -xf - -C D`, so a `mkdir: Permission denied` (root-owned hostPath dataset dir) + // aborts the shell before tar ever reads stdin — and the diagnosis was thrown away, + // turning a one-line permissions fix into a long investigation. So when the tar side + // only failed because we closed the pipe, defer to streamErr, which carries the remote + // stderr hint. A tar error from any OTHER cause (e.g. the size-cap recheck) still wins. + if tarErr != nil && !(errors.Is(tarErr, io.ErrClosedPipe) && streamErr != nil) { return fmt.Errorf("building tar archive: %w", tarErr) } if streamErr != nil { @@ -305,6 +318,10 @@ func StreamLayout( } return fmt.Errorf("streaming files to %s/%s: %w%s", namespace, podName, streamErr, hint) } + // Belt and braces: a closed-pipe tar error with NO streamErr shouldn't be swallowed. + if tarErr != nil { + return fmt.Errorf("building tar archive: %w", tarErr) + } return nil } diff --git a/internal/push/stream_test.go b/internal/push/stream_test.go index 13f7932..2bcfcf2 100644 --- a/internal/push/stream_test.go +++ b/internal/push/stream_test.go @@ -6,6 +6,8 @@ import ( "context" "errors" "io" + "os" + "path/filepath" "regexp" "sort" "strings" @@ -446,3 +448,83 @@ type fakeProgress struct { func (p *fakeProgress) Add(n int) { p.added += n } func (p *fakeProgress) Finish() { p.finished = true } + +// TestStreamLayout_ClosedPipeDoesNotMaskRemoteError pins the diagnostic +// that a real customer needed and did not get. +// +// The remote script is `set -e; rm -rf D; mkdir -p D; tar -xf - -C D`. +// When the dataset dir is root-owned (hostPath ignores fsGroup), `mkdir` +// fails with EACCES and the shell aborts BEFORE tar reads a single byte. +// StreamLayout's own pr.Close() then unblocks the tar goroutine with +// io.ErrClosedPipe — a self-inflicted error, not the cause. +// +// Reporting that tar error first produced a dead end: +// +// building tar archive: packaging : io: read/write on closed pipe +// +// which says nothing about permissions and cost a long investigation. +// The remote stderr must win: it is the only actionable signal. +func TestStreamLayout_ClosedPipeDoesNotMaskRemoteError(t *testing.T) { + layout, err := Discover(imgcDir(t)) + if err != nil { + t.Fatalf("Discover: %v", err) + } + fe := &fakeExecutor{ + stderrToReturn: []byte("mkdir: can't create directory '/data/shared/ds': Permission denied"), + errToReturn: errors.New("command terminated with exit code 1"), + // The remote died early: stdin is never drained, so the tar + // goroutine ends up on the closed pipe. + drainBeforeReturn: false, + } + err = StreamLayout(context.Background(), fe, "tracebloc", "p", "stage", + layout, "t", NoOpProgress{}) + if err == nil { + t.Fatal("StreamLayout returned nil when the remote script failed") + } + // The actionable cause must survive. + for _, want := range []string{"streaming files", "Permission denied", "exit code 1"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error missing %q: %v", want, err) + } + } + // And the self-inflicted pipe error must NOT be what we surface. + if strings.Contains(err.Error(), "building tar archive") { + t.Errorf("closed-pipe tar error masked the remote cause: %v", err) + } +} + +// TestStreamLayout_TarErrorStillWinsWhenNotClosedPipe guards the other +// side of the exception above: a tar-side failure from a REAL cause +// (not our own pr.Close) must still take precedence, because it is the +// upstream reason the stream died. Without this, narrowing the closed-pipe +// case could regress the size-cap diagnostic Bugbot originally asked for. +func TestStreamLayout_TarErrorStillWinsWhenNotClosedPipe(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "labels.csv"), + []byte("filename,label\nmissing.png,cat\n"), 0o644); err != nil { + t.Fatalf("write labels: %v", err) + } + // A layout naming a file that does not exist: writeLayoutTar fails on + // open (a genuine tar-side cause), not on a closed pipe. + layout := &LocalLayout{ + Root: dir, + LabelsCSV: filepath.Join(dir, "labels.csv"), + Images: []string{filepath.Join(dir, "missing.png")}, + } + fe := &fakeExecutor{ + errToReturn: errors.New("command terminated with exit code 2"), + // Drain, so the tar writes SUCCEED and the goroutine reaches the + // missing file — producing a real open error rather than a pipe error. + // (With an undrained pipe the first header write blocks and fails with + // ErrClosedPipe, which is precisely the self-inflicted case above.) + drainBeforeReturn: true, + } + err := StreamLayout(context.Background(), fe, "tracebloc", "p", "stage", + layout, "t", NoOpProgress{}) + if err == nil { + t.Fatal("StreamLayout returned nil on a tar-side failure") + } + if !strings.Contains(err.Error(), "building tar archive") { + t.Errorf("real tar error should win, got: %v", err) + } +}