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 <danny@coder.com>
This commit is contained in:
+16
-1
@@ -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,
|
||||
}
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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...)
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user