From 24b20df7d5bebf4dd60264b85f56ed17e11d0747 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Tue, 3 Feb 2026 11:50:28 +0200 Subject: [PATCH] fix: use `os.Pipe` implementation for Windows CLI tests to reduce flakiness (#21874) On Windows, `pty.New()` was creating a `ConPTY` (`PseudoConsole`) even when no process would be attached. `ConPTY` requires a real process to function correctly - without one, the pipe handles become invalid intermittently, causing flaky test failures like `read |0: The handle is invalid.` This affected tests using the `ptytest.New()` + `Attach()` pattern for in-process CLI testing. The fix splits Windows PTY creation into two paths: - `newPty()` now returns a simple pipe-based PTY for the `Attach()` use case - `newConPty()` creates a real `ConPTY`, called by `Start()` when a process will be attached AFAICT this will result in no change in behaviour outside of tests. Fixes coder/internal#1277 _Disclaimer: investigated and implemented by Claude Opus 4.5, reviewed by me._ --------- Signed-off-by: Danny Kopping --- pty/pty_windows.go | 17 ++++++- pty/ptytest/ptytest.go | 2 +- pty/ptytest/ptytest_other.go | 9 ++++ pty/ptytest/ptytest_windows.go | 90 ++++++++++++++++++++++++++++++++++ pty/start_windows.go | 2 +- 5 files changed, 117 insertions(+), 3 deletions(-) create mode 100644 pty/ptytest/ptytest_other.go create mode 100644 pty/ptytest/ptytest_windows.go diff --git a/pty/pty_windows.go b/pty/pty_windows.go index eaf92b9ed2..e7fa719756 100644 --- a/pty/pty_windows.go +++ b/pty/pty_windows.go @@ -22,7 +22,7 @@ var ( ) // See: https://docs.microsoft.com/en-us/windows/console/creating-a-pseudoconsole-session -func newPty(opt ...Option) (*ptyWindows, error) { +func newPty(opt ...Option) (PTY, error) { var opts ptyOptions for _, o := range opt { o(&opts) @@ -37,6 +37,21 @@ func newPty(opt ...Option) (*ptyWindows, error) { return nil, xerrors.Errorf("pty not supported") } + // On Windows, pty.New() without Start() is only used by ptytest.New() for + // in-process CLI testing. ConPTY requires an attached process to function + // correctly, so ptytest has its own pipe-based implementation. Production + // code should use pty.Start() which creates a ConPTY with process attached. + return nil, xerrors.Errorf("pty without process not supported on Windows; use ptytest.New() for tests") +} + +// newConPty creates a PTY backed by a Windows PseudoConsole (ConPTY). This +// should only be used when a process will be attached via Start(). +func newConPty(opt ...Option) (*ptyWindows, error) { + var opts ptyOptions + for _, o := range opt { + o(&opts) + } + pty := &ptyWindows{ opts: opts, } diff --git a/pty/ptytest/ptytest.go b/pty/ptytest/ptytest.go index 5d15078094..efb93ed338 100644 --- a/pty/ptytest/ptytest.go +++ b/pty/ptytest/ptytest.go @@ -27,7 +27,7 @@ import ( func New(t *testing.T, opts ...pty.Option) *PTY { t.Helper() - ptty, err := pty.New(opts...) + ptty, err := newTestPTY(opts...) require.NoError(t, err) e := newExpecter(t, ptty.Output(), "cmd") diff --git a/pty/ptytest/ptytest_other.go b/pty/ptytest/ptytest_other.go new file mode 100644 index 0000000000..0edc45d00d --- /dev/null +++ b/pty/ptytest/ptytest_other.go @@ -0,0 +1,9 @@ +//go:build !windows + +package ptytest + +import "github.com/coder/coder/v2/pty" + +func newTestPTY(opts ...pty.Option) (pty.PTY, error) { + return pty.New(opts...) +} diff --git a/pty/ptytest/ptytest_windows.go b/pty/ptytest/ptytest_windows.go new file mode 100644 index 0000000000..637f4ec68f --- /dev/null +++ b/pty/ptytest/ptytest_windows.go @@ -0,0 +1,90 @@ +//go:build windows + +package ptytest + +import ( + "os" + "sync" + + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/pty" +) + +// testPTY is a pipe-based PTY implementation for in-process CLI testing on +// Windows. ConPTY requires an attached process to function correctly - without +// one, the pipe handles become invalid intermittently. This implementation +// avoids ConPTY entirely for the ptytest.New() + Attach() use case. +type testPTY struct { + inputReader *os.File + inputWriter *os.File + outputReader *os.File + outputWriter *os.File + + closeMutex sync.Mutex + closed bool +} + +func newTestPTY(_ ...pty.Option) (pty.PTY, error) { + p := &testPTY{} + + var err error + p.inputReader, p.inputWriter, err = os.Pipe() + if err != nil { + return nil, xerrors.Errorf("create input pipe: %w", err) + } + p.outputReader, p.outputWriter, err = os.Pipe() + if err != nil { + _ = p.inputReader.Close() + _ = p.inputWriter.Close() + return nil, xerrors.Errorf("create output pipe: %w", err) + } + + return p, nil +} + +func (*testPTY) Name() string { + return "" +} + +func (p *testPTY) Input() pty.ReadWriter { + return pty.ReadWriter{ + Reader: p.inputReader, + Writer: p.inputWriter, + } +} + +func (p *testPTY) Output() pty.ReadWriter { + return pty.ReadWriter{ + Reader: p.outputReader, + Writer: p.outputWriter, + } +} + +func (*testPTY) Resize(uint16, uint16) error { + return nil +} + +func (p *testPTY) Close() error { + p.closeMutex.Lock() + defer p.closeMutex.Unlock() + if p.closed { + return nil + } + p.closed = true + + var firstErr error + if err := p.outputWriter.Close(); err != nil && firstErr == nil { + firstErr = err + } + if err := p.outputReader.Close(); err != nil && firstErr == nil { + firstErr = err + } + if err := p.inputWriter.Close(); err != nil && firstErr == nil { + firstErr = err + } + if err := p.inputReader.Close(); err != nil && firstErr == nil { + firstErr = err + } + return firstErr +} diff --git a/pty/start_windows.go b/pty/start_windows.go index 4e9a755e95..7665fcc41a 100644 --- a/pty/start_windows.go +++ b/pty/start_windows.go @@ -46,7 +46,7 @@ func startPty(cmd *Cmd, opt ...StartOption) (_ PTYCmd, _ Process, retErr error) return nil, nil, err } - winPty, err := newPty(opts.ptyOpts...) + winPty, err := newConPty(opts.ptyOpts...) if err != nil { return nil, nil, err }