From 425e8ada6c94e8932092edbb7c78d1fcd4e6a8f3 Mon Sep 17 00:00:00 2001 From: AdaAibaby Date: Tue, 8 Sep 2026 11:58:57 +0800 Subject: [PATCH] fix(envd): distinguish expected stdin lifecycle errors from CodeInternal Process.SendInput and CloseStdin previously mapped every WriteStdin / WriteTty / CloseStdin failure to CodeInternal (CloseStdin to CodeUnknown). Several of these are expected process-lifecycle outcomes, not envd faults: - stdin was started disabled, or has already been closed; - a PTY-backed process received stdin (input belongs on the pty); - the process exited and its stdin read end is gone, so the write end sees EPIPE / os.ErrClosed ("file already closed") / io.ErrClosedPipe. Collapsing these into CodeInternal means clients cannot tell a normal process-state transition from a genuine invariant failure, and it races the Start response stream: SendInput can return CodeInternal before the process's EndEvent arrives on the independent stream. Introduce typed sentinels (ErrStdinUnavailable, ErrStdinOnPty, ErrTtyUnavailable, ErrCloseStdinOnPty) and an InputErrorCode mapper in the handler package, mirroring StartErrorCode: - process selector missing -> CodeNotFound (already handled) - process cannot accept stdin/pty -> CodeFailedPrecondition - unexpected underlying I/O failure -> CodeInternal CloseStdin follows the same taxonomy. The EndEvent remains the authoritative exit signal; an input error alone is not the terminal process result. Callers that only check for a non-nil error are unaffected. Fixes #3622 --- .../services/process/handler/handler.go | 8 +- .../services/process/handler/input_error.go | 66 ++++++++ .../process/handler/input_error_test.go | 156 ++++++++++++++++++ .../envd/internal/services/process/input.go | 10 +- 4 files changed, 231 insertions(+), 9 deletions(-) create mode 100644 packages/envd/internal/services/process/handler/input_error.go create mode 100644 packages/envd/internal/services/process/handler/input_error_test.go diff --git a/packages/envd/internal/services/process/handler/handler.go b/packages/envd/internal/services/process/handler/handler.go index 4dcc7b1fa8..e9727e5a04 100644 --- a/packages/envd/internal/services/process/handler/handler.go +++ b/packages/envd/internal/services/process/handler/handler.go @@ -493,14 +493,14 @@ func (p *Handler) ResizeTty(size *pty.Winsize) error { func (p *Handler) WriteStdin(data []byte) error { if p.tty != nil { - return errors.New("tty assigned to process — input should be written to the pty, not the stdin") + return ErrStdinOnPty } p.stdinMu.Lock() defer p.stdinMu.Unlock() if p.stdin == nil { - return errors.New("stdin not enabled or closed") + return ErrStdinUnavailable } _, err := p.stdin.Write(data) @@ -515,7 +515,7 @@ func (p *Handler) WriteStdin(data []byte) error { // Only works for non-PTY processes. func (p *Handler) CloseStdin() error { if p.tty != nil { - return errors.New("cannot close stdin for PTY process — send Ctrl+D (0x04) instead") + return ErrCloseStdinOnPty } p.stdinMu.Lock() @@ -535,7 +535,7 @@ func (p *Handler) CloseStdin() error { func (p *Handler) WriteTty(data []byte) error { if p.tty == nil { - return errors.New("tty not assigned to process — input should be written to the stdin, not the tty") + return ErrTtyUnavailable } _, err := p.tty.Write(data) diff --git a/packages/envd/internal/services/process/handler/input_error.go b/packages/envd/internal/services/process/handler/input_error.go new file mode 100644 index 0000000000..9ae459505c --- /dev/null +++ b/packages/envd/internal/services/process/handler/input_error.go @@ -0,0 +1,66 @@ +package handler + +import ( + "errors" + "io" + "io/fs" + "os" + "syscall" + + "connectrpc.com/connect" +) + +// Sentinel errors returned by the stdin/pty input methods for expected process +// lifecycle states — as opposed to genuine I/O failures. They let the service +// layer map an input failure to a precise Connect code instead of collapsing +// everything to CodeInternal (see issue #3622). +var ( + // ErrStdinUnavailable means the process cannot accept stdin right now: it was + // started with stdin disabled, or stdin has already been closed. This is an + // expected precondition failure, not an envd fault. + ErrStdinUnavailable = errors.New("stdin not enabled or closed") + + // ErrStdinOnPty means stdin was written to a PTY-backed process; input must + // go to the pty instead. An expected precondition failure. + ErrStdinOnPty = errors.New("tty assigned to process — input should be written to the pty, not the stdin") + + // ErrTtyUnavailable means a pty write targeted a process that has no tty. + ErrTtyUnavailable = errors.New("tty not assigned to process — input should be written to the stdin, not the tty") + + // ErrCloseStdinOnPty means CloseStdin was called on a PTY-backed process. + ErrCloseStdinOnPty = errors.New("cannot close stdin for PTY process — send Ctrl+D (0x04) instead") +) + +// InputErrorCode maps a WriteStdin/WriteTty/CloseStdin failure to the Connect +// code the client should observe. +// +// - Expected process-lifecycle states (stdin disabled/closed, wrong pipe for +// the process type, or the child's read end already gone after exit) are +// CodeFailedPrecondition: the request was well-formed but the process is not +// in a state to accept it. +// - Everything else is a real underlying I/O failure and stays CodeInternal. +// +// CodeInternal is thus reserved for genuine invariant failures, so a client can +// distinguish a normal process-state transition from an unexpected envd fault. +// The process's EndEvent remains the authoritative exit signal; an input error +// alone must not be treated as the terminal process result. +func InputErrorCode(err error) connect.Code { + switch { + // Explicit precondition sentinels from the input methods. + case errors.Is(err, ErrStdinUnavailable), + errors.Is(err, ErrStdinOnPty), + errors.Is(err, ErrTtyUnavailable), + errors.Is(err, ErrCloseStdinOnPty): + return connect.CodeFailedPrecondition + // The process exited and its stdin/pty read end is gone: the write end sees + // EPIPE, or Go's file wrapper reports it was already closed (os.ErrClosed / + // io.ErrClosedPipe). All are expected once the process is no longer running. + case errors.Is(err, syscall.EPIPE), + errors.Is(err, os.ErrClosed), + errors.Is(err, io.ErrClosedPipe), + errors.Is(err, fs.ErrClosed): + return connect.CodeFailedPrecondition + default: + return connect.CodeInternal + } +} diff --git a/packages/envd/internal/services/process/handler/input_error_test.go b/packages/envd/internal/services/process/handler/input_error_test.go new file mode 100644 index 0000000000..9506d2647c --- /dev/null +++ b/packages/envd/internal/services/process/handler/input_error_test.go @@ -0,0 +1,156 @@ +package handler + +import ( + "errors" + "fmt" + "io" + "io/fs" + "os" + "os/exec" + "syscall" + "testing" + "time" + + "connectrpc.com/connect" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestInputErrorCode(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + err error + name string + want connect.Code + }{ + { + name: "stdin disabled or closed is a precondition failure", + err: ErrStdinUnavailable, + want: connect.CodeFailedPrecondition, + }, + { + name: "stdin on a pty process is a precondition failure", + err: ErrStdinOnPty, + want: connect.CodeFailedPrecondition, + }, + { + name: "pty write with no tty is a precondition failure", + err: ErrTtyUnavailable, + want: connect.CodeFailedPrecondition, + }, + { + name: "close stdin on a pty process is a precondition failure", + err: ErrCloseStdinOnPty, + want: connect.CodeFailedPrecondition, + }, + { + name: "wrapped precondition sentinel is still a precondition failure", + err: fmt.Errorf("error writing to stdin of process '%d': %w", 42, ErrStdinUnavailable), + want: connect.CodeFailedPrecondition, + }, + { + name: "EPIPE after exit is a precondition failure", + err: fmt.Errorf("error writing to stdin of process '%d': %w", 42, syscall.EPIPE), + want: connect.CodeFailedPrecondition, + }, + { + name: "file already closed after exit is a precondition failure", + err: fmt.Errorf("error writing to stdin of process '%d': %w", 42, os.ErrClosed), + want: connect.CodeFailedPrecondition, + }, + { + name: "closed pipe after exit is a precondition failure", + err: fmt.Errorf("wrapped: %w", io.ErrClosedPipe), + want: connect.CodeFailedPrecondition, + }, + { + name: "fs closed after exit is a precondition failure", + err: &fs.PathError{Op: "write", Path: "|1", Err: fs.ErrClosed}, + want: connect.CodeFailedPrecondition, + }, + { + name: "unexpected I/O failure stays internal", + err: fmt.Errorf("error writing to stdin of process '%d': %w", 42, syscall.EIO), + want: connect.CodeInternal, + }, + { + name: "opaque error stays internal", + err: errors.New("something went wrong"), + want: connect.CodeInternal, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + got := InputErrorCode(tc.err) + assert.Equalf(t, tc.want, got, "got %s, want %s", got, tc.want) + }) + } +} + +// Asserts on the code carried by *connect.Error — what the client observes on the wire. +func TestInputErrorClientObservedCode(t *testing.T) { + t.Parallel() + + precond := fmt.Errorf("error writing to stdin: %w", ErrStdinUnavailable) + assert.Equal(t, connect.CodeFailedPrecondition, connect.CodeOf(connect.NewError(InputErrorCode(precond), precond))) + + internal := fmt.Errorf("error writing to stdin: %w", syscall.EIO) + assert.Equal(t, connect.CodeInternal, connect.CodeOf(connect.NewError(InputErrorCode(internal), internal))) +} + +// WriteStdin returns the typed sentinels for the two synchronous precondition +// states so the service layer can map them without string matching. +func TestWriteStdinPreconditionSentinels(t *testing.T) { + t.Parallel() + + t.Run("stdin disabled", func(t *testing.T) { + t.Parallel() + h := &Handler{} // stdin == nil, no tty + err := h.WriteStdin([]byte("x")) + require.Error(t, err) + assert.ErrorIs(t, err, ErrStdinUnavailable) + assert.Equal(t, connect.CodeFailedPrecondition, InputErrorCode(err)) + }) + + t.Run("pty process rejects stdin", func(t *testing.T) { + t.Parallel() + // A non-nil tty routes stdin writes to ErrStdinOnPty before any pipe use. + h := &Handler{tty: os.NewFile(0, "fake-tty")} + err := h.WriteStdin([]byte("x")) + require.Error(t, err) + assert.ErrorIs(t, err, ErrStdinOnPty) + assert.Equal(t, connect.CodeFailedPrecondition, InputErrorCode(err)) + }) +} + +// Reproduces issue #3622: a short-lived non-PTY process whose stdin read end is +// gone after exit. WriteStdin must surface an expected-lifecycle error that +// maps to CodeFailedPrecondition, not CodeInternal. +func TestWriteStdinAfterExitIsPrecondition(t *testing.T) { + t.Parallel() + + cmd := exec.Command("/bin/sh", "-c", "exit 0") + stdin, err := cmd.StdinPipe() + require.NoError(t, err) + require.NoError(t, cmd.Start()) + + h := &Handler{stdin: stdin} + + _ = cmd.Wait() + time.Sleep(50 * time.Millisecond) + + var writeErr error + for i := 0; i < 100; i++ { + if writeErr = h.WriteStdin([]byte("hello\n")); writeErr != nil { + break + } + time.Sleep(5 * time.Millisecond) + } + require.Error(t, writeErr, "expected WriteStdin to fail after process exit") + + code := InputErrorCode(writeErr) + assert.Equalf(t, connect.CodeFailedPrecondition, code, + "post-exit stdin write should be CodeFailedPrecondition, got %s (raw: %v)", code, writeErr) +} diff --git a/packages/envd/internal/services/process/input.go b/packages/envd/internal/services/process/input.go index c1733a8a4a..753b921161 100644 --- a/packages/envd/internal/services/process/input.go +++ b/packages/envd/internal/services/process/input.go @@ -16,13 +16,13 @@ func handleInput(process *handler.Handler, in *rpc.ProcessInput) error { case *rpc.ProcessInput_Pty: err := process.WriteTty(in.GetPty()) if err != nil { - return connect.NewError(connect.CodeInternal, fmt.Errorf("error writing to tty: %w", err)) + return connect.NewError(handler.InputErrorCode(err), fmt.Errorf("error writing to tty: %w", err)) } case *rpc.ProcessInput_Stdin: err := process.WriteStdin(in.GetStdin()) if err != nil { - return connect.NewError(connect.CodeInternal, fmt.Errorf("error writing to stdin: %w", err)) + return connect.NewError(handler.InputErrorCode(err), fmt.Errorf("error writing to stdin: %w", err)) } default: @@ -87,13 +87,13 @@ func (s *Service) CloseStdin( _ context.Context, req *connect.Request[rpc.CloseStdinRequest], ) (*connect.Response[rpc.CloseStdinResponse], error) { - handler, err := s.getProcess(req.Msg.GetProcess()) + proc, err := s.getProcess(req.Msg.GetProcess()) if err != nil { return nil, err } - if err := handler.CloseStdin(); err != nil { - return nil, connect.NewError(connect.CodeUnknown, fmt.Errorf("error closing stdin: %w", err)) + if err := proc.CloseStdin(); err != nil { + return nil, connect.NewError(handler.InputErrorCode(err), fmt.Errorf("error closing stdin: %w", err)) } return connect.NewResponse(&rpc.CloseStdinResponse{}), nil