Skip to content

Commit 425e8ad

Browse files
committed
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
1 parent cc7c574 commit 425e8ad

4 files changed

Lines changed: 231 additions & 9 deletions

File tree

‎packages/envd/internal/services/process/handler/handler.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -493,14 +493,14 @@ func (p *Handler) ResizeTty(size *pty.Winsize) error {
493493

494494
func (p *Handler) WriteStdin(data []byte) error {
495495
if p.tty != nil {
496-
return errors.New("tty assigned to process — input should be written to the pty, not the stdin")
496+
return ErrStdinOnPty
497497
}
498498

499499
p.stdinMu.Lock()
500500
defer p.stdinMu.Unlock()
501501

502502
if p.stdin == nil {
503-
return errors.New("stdin not enabled or closed")
503+
return ErrStdinUnavailable
504504
}
505505

506506
_, err := p.stdin.Write(data)
@@ -515,7 +515,7 @@ func (p *Handler) WriteStdin(data []byte) error {
515515
// Only works for non-PTY processes.
516516
func (p *Handler) CloseStdin() error {
517517
if p.tty != nil {
518-
return errors.New("cannot close stdin for PTY process — send Ctrl+D (0x04) instead")
518+
return ErrCloseStdinOnPty
519519
}
520520

521521
p.stdinMu.Lock()
@@ -535,7 +535,7 @@ func (p *Handler) CloseStdin() error {
535535

536536
func (p *Handler) WriteTty(data []byte) error {
537537
if p.tty == nil {
538-
return errors.New("tty not assigned to process — input should be written to the stdin, not the tty")
538+
return ErrTtyUnavailable
539539
}
540540

541541
_, err := p.tty.Write(data)
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
package handler
2+
3+
import (
4+
"errors"
5+
"io"
6+
"io/fs"
7+
"os"
8+
"syscall"
9+
10+
"connectrpc.com/connect"
11+
)
12+
13+
// Sentinel errors returned by the stdin/pty input methods for expected process
14+
// lifecycle states — as opposed to genuine I/O failures. They let the service
15+
// layer map an input failure to a precise Connect code instead of collapsing
16+
// everything to CodeInternal (see issue #3622).
17+
var (
18+
// ErrStdinUnavailable means the process cannot accept stdin right now: it was
19+
// started with stdin disabled, or stdin has already been closed. This is an
20+
// expected precondition failure, not an envd fault.
21+
ErrStdinUnavailable = errors.New("stdin not enabled or closed")
22+
23+
// ErrStdinOnPty means stdin was written to a PTY-backed process; input must
24+
// go to the pty instead. An expected precondition failure.
25+
ErrStdinOnPty = errors.New("tty assigned to process — input should be written to the pty, not the stdin")
26+
27+
// ErrTtyUnavailable means a pty write targeted a process that has no tty.
28+
ErrTtyUnavailable = errors.New("tty not assigned to process — input should be written to the stdin, not the tty")
29+
30+
// ErrCloseStdinOnPty means CloseStdin was called on a PTY-backed process.
31+
ErrCloseStdinOnPty = errors.New("cannot close stdin for PTY process — send Ctrl+D (0x04) instead")
32+
)
33+
34+
// InputErrorCode maps a WriteStdin/WriteTty/CloseStdin failure to the Connect
35+
// code the client should observe.
36+
//
37+
// - Expected process-lifecycle states (stdin disabled/closed, wrong pipe for
38+
// the process type, or the child's read end already gone after exit) are
39+
// CodeFailedPrecondition: the request was well-formed but the process is not
40+
// in a state to accept it.
41+
// - Everything else is a real underlying I/O failure and stays CodeInternal.
42+
//
43+
// CodeInternal is thus reserved for genuine invariant failures, so a client can
44+
// distinguish a normal process-state transition from an unexpected envd fault.
45+
// The process's EndEvent remains the authoritative exit signal; an input error
46+
// alone must not be treated as the terminal process result.
47+
func InputErrorCode(err error) connect.Code {
48+
switch {
49+
// Explicit precondition sentinels from the input methods.
50+
case errors.Is(err, ErrStdinUnavailable),
51+
errors.Is(err, ErrStdinOnPty),
52+
errors.Is(err, ErrTtyUnavailable),
53+
errors.Is(err, ErrCloseStdinOnPty):
54+
return connect.CodeFailedPrecondition
55+
// The process exited and its stdin/pty read end is gone: the write end sees
56+
// EPIPE, or Go's file wrapper reports it was already closed (os.ErrClosed /
57+
// io.ErrClosedPipe). All are expected once the process is no longer running.
58+
case errors.Is(err, syscall.EPIPE),
59+
errors.Is(err, os.ErrClosed),
60+
errors.Is(err, io.ErrClosedPipe),
61+
errors.Is(err, fs.ErrClosed):
62+
return connect.CodeFailedPrecondition
63+
default:
64+
return connect.CodeInternal
65+
}
66+
}
Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
package handler
2+
3+
import (
4+
"errors"
5+
"fmt"
6+
"io"
7+
"io/fs"
8+
"os"
9+
"os/exec"
10+
"syscall"
11+
"testing"
12+
"time"
13+
14+
"connectrpc.com/connect"
15+
"github.com/stretchr/testify/assert"
16+
"github.com/stretchr/testify/require"
17+
)
18+
19+
func TestInputErrorCode(t *testing.T) {
20+
t.Parallel()
21+
22+
for _, tc := range []struct {
23+
err error
24+
name string
25+
want connect.Code
26+
}{
27+
{
28+
name: "stdin disabled or closed is a precondition failure",
29+
err: ErrStdinUnavailable,
30+
want: connect.CodeFailedPrecondition,
31+
},
32+
{
33+
name: "stdin on a pty process is a precondition failure",
34+
err: ErrStdinOnPty,
35+
want: connect.CodeFailedPrecondition,
36+
},
37+
{
38+
name: "pty write with no tty is a precondition failure",
39+
err: ErrTtyUnavailable,
40+
want: connect.CodeFailedPrecondition,
41+
},
42+
{
43+
name: "close stdin on a pty process is a precondition failure",
44+
err: ErrCloseStdinOnPty,
45+
want: connect.CodeFailedPrecondition,
46+
},
47+
{
48+
name: "wrapped precondition sentinel is still a precondition failure",
49+
err: fmt.Errorf("error writing to stdin of process '%d': %w", 42, ErrStdinUnavailable),
50+
want: connect.CodeFailedPrecondition,
51+
},
52+
{
53+
name: "EPIPE after exit is a precondition failure",
54+
err: fmt.Errorf("error writing to stdin of process '%d': %w", 42, syscall.EPIPE),
55+
want: connect.CodeFailedPrecondition,
56+
},
57+
{
58+
name: "file already closed after exit is a precondition failure",
59+
err: fmt.Errorf("error writing to stdin of process '%d': %w", 42, os.ErrClosed),
60+
want: connect.CodeFailedPrecondition,
61+
},
62+
{
63+
name: "closed pipe after exit is a precondition failure",
64+
err: fmt.Errorf("wrapped: %w", io.ErrClosedPipe),
65+
want: connect.CodeFailedPrecondition,
66+
},
67+
{
68+
name: "fs closed after exit is a precondition failure",
69+
err: &fs.PathError{Op: "write", Path: "|1", Err: fs.ErrClosed},
70+
want: connect.CodeFailedPrecondition,
71+
},
72+
{
73+
name: "unexpected I/O failure stays internal",
74+
err: fmt.Errorf("error writing to stdin of process '%d': %w", 42, syscall.EIO),
75+
want: connect.CodeInternal,
76+
},
77+
{
78+
name: "opaque error stays internal",
79+
err: errors.New("something went wrong"),
80+
want: connect.CodeInternal,
81+
},
82+
} {
83+
t.Run(tc.name, func(t *testing.T) {
84+
t.Parallel()
85+
86+
got := InputErrorCode(tc.err)
87+
assert.Equalf(t, tc.want, got, "got %s, want %s", got, tc.want)
88+
})
89+
}
90+
}
91+
92+
// Asserts on the code carried by *connect.Error — what the client observes on the wire.
93+
func TestInputErrorClientObservedCode(t *testing.T) {
94+
t.Parallel()
95+
96+
precond := fmt.Errorf("error writing to stdin: %w", ErrStdinUnavailable)
97+
assert.Equal(t, connect.CodeFailedPrecondition, connect.CodeOf(connect.NewError(InputErrorCode(precond), precond)))
98+
99+
internal := fmt.Errorf("error writing to stdin: %w", syscall.EIO)
100+
assert.Equal(t, connect.CodeInternal, connect.CodeOf(connect.NewError(InputErrorCode(internal), internal)))
101+
}
102+
103+
// WriteStdin returns the typed sentinels for the two synchronous precondition
104+
// states so the service layer can map them without string matching.
105+
func TestWriteStdinPreconditionSentinels(t *testing.T) {
106+
t.Parallel()
107+
108+
t.Run("stdin disabled", func(t *testing.T) {
109+
t.Parallel()
110+
h := &Handler{} // stdin == nil, no tty
111+
err := h.WriteStdin([]byte("x"))
112+
require.Error(t, err)
113+
assert.ErrorIs(t, err, ErrStdinUnavailable)
114+
assert.Equal(t, connect.CodeFailedPrecondition, InputErrorCode(err))
115+
})
116+
117+
t.Run("pty process rejects stdin", func(t *testing.T) {
118+
t.Parallel()
119+
// A non-nil tty routes stdin writes to ErrStdinOnPty before any pipe use.
120+
h := &Handler{tty: os.NewFile(0, "fake-tty")}
121+
err := h.WriteStdin([]byte("x"))
122+
require.Error(t, err)
123+
assert.ErrorIs(t, err, ErrStdinOnPty)
124+
assert.Equal(t, connect.CodeFailedPrecondition, InputErrorCode(err))
125+
})
126+
}
127+
128+
// Reproduces issue #3622: a short-lived non-PTY process whose stdin read end is
129+
// gone after exit. WriteStdin must surface an expected-lifecycle error that
130+
// maps to CodeFailedPrecondition, not CodeInternal.
131+
func TestWriteStdinAfterExitIsPrecondition(t *testing.T) {
132+
t.Parallel()
133+
134+
cmd := exec.Command("/bin/sh", "-c", "exit 0")
135+
stdin, err := cmd.StdinPipe()
136+
require.NoError(t, err)
137+
require.NoError(t, cmd.Start())
138+
139+
h := &Handler{stdin: stdin}
140+
141+
_ = cmd.Wait()
142+
time.Sleep(50 * time.Millisecond)
143+
144+
var writeErr error
145+
for i := 0; i < 100; i++ {
146+
if writeErr = h.WriteStdin([]byte("hello\n")); writeErr != nil {
147+
break
148+
}
149+
time.Sleep(5 * time.Millisecond)
150+
}
151+
require.Error(t, writeErr, "expected WriteStdin to fail after process exit")
152+
153+
code := InputErrorCode(writeErr)
154+
assert.Equalf(t, connect.CodeFailedPrecondition, code,
155+
"post-exit stdin write should be CodeFailedPrecondition, got %s (raw: %v)", code, writeErr)
156+
}

‎packages/envd/internal/services/process/input.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,13 @@ func handleInput(process *handler.Handler, in *rpc.ProcessInput) error {
1616
case *rpc.ProcessInput_Pty:
1717
err := process.WriteTty(in.GetPty())
1818
if err != nil {
19-
return connect.NewError(connect.CodeInternal, fmt.Errorf("error writing to tty: %w", err))
19+
return connect.NewError(handler.InputErrorCode(err), fmt.Errorf("error writing to tty: %w", err))
2020
}
2121

2222
case *rpc.ProcessInput_Stdin:
2323
err := process.WriteStdin(in.GetStdin())
2424
if err != nil {
25-
return connect.NewError(connect.CodeInternal, fmt.Errorf("error writing to stdin: %w", err))
25+
return connect.NewError(handler.InputErrorCode(err), fmt.Errorf("error writing to stdin: %w", err))
2626
}
2727

2828
default:
@@ -87,13 +87,13 @@ func (s *Service) CloseStdin(
8787
_ context.Context,
8888
req *connect.Request[rpc.CloseStdinRequest],
8989
) (*connect.Response[rpc.CloseStdinResponse], error) {
90-
handler, err := s.getProcess(req.Msg.GetProcess())
90+
proc, err := s.getProcess(req.Msg.GetProcess())
9191
if err != nil {
9292
return nil, err
9393
}
9494

95-
if err := handler.CloseStdin(); err != nil {
96-
return nil, connect.NewError(connect.CodeUnknown, fmt.Errorf("error closing stdin: %w", err))
95+
if err := proc.CloseStdin(); err != nil {
96+
return nil, connect.NewError(handler.InputErrorCode(err), fmt.Errorf("error closing stdin: %w", err))
9797
}
9898

9999
return connect.NewResponse(&rpc.CloseStdinResponse{}), nil

0 commit comments

Comments
 (0)