implement binary protocol, reduce buffers, fixes an echo race

This commit is contained in:
Peter Steinberger 2025-06-20 20:49:02 +02:00
parent 774379e8fc
commit e43ef5ad49
8 changed files with 478 additions and 62 deletions

View file

@ -7,7 +7,6 @@ import (
"os"
"os/exec"
"os/signal"
"path/filepath"
"strings"
"sync"
"syscall"
@ -15,6 +14,7 @@ import (
"github.com/creack/pty"
"github.com/vibetunnel/linux/pkg/protocol"
"golang.org/x/sys/unix"
"golang.org/x/term"
)
@ -175,6 +175,7 @@ func NewPTY(session *Session) (*PTY, error) {
log.Printf("[ERROR] NewPTY: Failed to configure PTY terminal: %v", err)
// Don't fail on terminal configuration errors, just log them
}
// Set PTY size using our enhanced function
if err := setPTYSize(ptmx, uint16(session.info.Width), uint16(session.info.Height)); err != nil {
@ -251,12 +252,23 @@ func NewPTY(session *Session) (*PTY, error) {
// Don't fail if control FIFO creation fails - it's optional
}
return &PTY{
ptyObj := &PTY{
session: session,
cmd: cmd,
pty: ptmx,
streamWriter: streamWriter,
}, nil
}
// For spawned sessions that will be attached, disable echo immediately
// to prevent race condition where output is processed before Attach() disables echo
if session.info.IsSpawned {
debugLog("[DEBUG] NewPTY: Spawned session detected, disabling PTY echo immediately")
if err := ptyObj.disablePTYEcho(); err != nil {
log.Printf("[ERROR] NewPTY: Failed to disable PTY echo for spawned session: %v", err)
}
}
return ptyObj, nil
}
func (p *PTY) Pid() int {
@ -350,7 +362,7 @@ func (p *PTY) Run() error {
go func() {
debugLog("[DEBUG] PTY.Run: Starting output reading goroutine")
buf := make([]byte, 4*1024) // 4KB buffer for more responsive output
buf := make([]byte, 1024) // 1KB buffer for maximum responsiveness
for {
// Use a timeout-based approach for cross-platform compatibility
@ -483,6 +495,13 @@ func (p *PTY) Attach() error {
}
p.oldState = oldState
// When attaching to a PTY interactively, we need to disable ECHO on the PTY
// to prevent double-echoing (since the controlling terminal is in raw mode)
if err := p.disablePTYEcho(); err != nil {
log.Printf("[WARN] PTY.Attach: Failed to disable PTY echo: %v", err)
// Continue anyway - some programs might handle this themselves
}
defer func() {
if err := term.Restore(int(os.Stdin.Fd()), oldState); err != nil {
log.Printf("[ERROR] PTY.Attach: Failed to restore terminal: %v", err)
@ -545,6 +564,28 @@ func (p *PTY) updateSize() error {
})
}
// disablePTYEcho disables echo on the PTY to prevent double-echoing
// when the controlling terminal is in raw mode
func (p *PTY) disablePTYEcho() error {
// Get current PTY termios
termios, err := unix.IoctlGetTermios(int(p.pty.Fd()), unix.TIOCGETA)
if err != nil {
return fmt.Errorf("failed to get PTY termios: %w", err)
}
// Disable echo flags to prevent double-echoing
// Keep other flags like ICANON for line processing
termios.Lflag &^= unix.ECHO | unix.ECHOE | unix.ECHOK | unix.ECHOKE | unix.ECHOCTL
// Apply the new settings
if err := unix.IoctlSetTermios(int(p.pty.Fd()), unix.TIOCSETA, termios); err != nil {
return fmt.Errorf("failed to set PTY termios: %w", err)
}
debugLog("[DEBUG] PTY.disablePTYEcho: Disabled echo on PTY")
return nil
}
func (p *PTY) Resize(width, height int) error {
if p.pty == nil {
return fmt.Errorf("PTY not initialized")

View file

@ -70,7 +70,7 @@ func fdIsSet(set *syscall.FdSet, fd int) bool {
// pollWithSelect polls multiple file descriptors using select
func (p *PTY) pollWithSelect() error {
// Buffer for reading
buf := make([]byte, 4*1024) // 4KB buffer for more responsive output
buf := make([]byte, 1024) // 1KB buffer for maximum responsiveness
// Get file descriptors
ptyFd := int(p.pty.Fd())

View file

@ -32,43 +32,62 @@ func configurePTYTerminal(ptyFile *os.File) error {
// Match node-pty's default behavior: keep most settings from the parent terminal
// but ensure proper signal handling and character processing
// Ensure proper input processing
// Ensure proper input processing to match node-pty behavior
// ICRNL: Map CR to NL on input (important for Enter key)
termios.Iflag |= unix.ICRNL
// Clear software flow control by default to match node-pty
termios.Iflag &^= (unix.IXON | unix.IXOFF | unix.IXANY)
// IXON: Enable XON/XOFF flow control on output
// IXANY: Allow any character to restart output
// IMAXBEL: Ring bell on input queue full
// BRKINT: Send SIGINT on break
termios.Iflag |= unix.ICRNL | unix.IXON | unix.IXANY | unix.IMAXBEL | unix.BRKINT
// Note: We KEEP flow control enabled to match node-pty behavior
// Configure output flags
// OPOST: Enable output processing
// ONLCR: Map NL to CR-NL on output (important for proper line endings)
termios.Oflag |= unix.OPOST | unix.ONLCR
// Configure control flags
// Configure control flags to match node-pty
// CS8: 8-bit characters
// CREAD: Enable receiver
termios.Cflag |= unix.CS8 | unix.CREAD
// HUPCL: Hang up on last close
termios.Cflag |= unix.CS8 | unix.CREAD | unix.HUPCL
termios.Cflag &^= unix.PARENB // Disable parity
// Configure local flags
// Configure local flags to match node-pty behavior exactly
// ISIG: Enable signal generation (SIGINT on Ctrl+C, etc)
// ICANON: Enable canonical mode (line editing)
// IEXTEN: Enable extended functions
termios.Lflag |= unix.ISIG | unix.ICANON | unix.IEXTEN
// IMPORTANT: Don't enable ECHO for PTY master
// The terminal emulator (slave) handles echo, not the master
// Enabling echo on the master causes duplicate output
termios.Lflag &^= (unix.ECHO | unix.ECHOE | unix.ECHOK | unix.ECHONL)
// ECHO: Enable echo (node-pty enables this!)
// ECHOE: Echo erase character as BS-SP-BS
// ECHOK: Echo KILL by erasing line
// ECHOKE: BS-SP-BS entire line on KILL
// ECHOCTL: Echo control characters as ^X
termios.Lflag |= unix.ISIG | unix.ICANON | unix.IEXTEN | unix.ECHO | unix.ECHOE | unix.ECHOK | unix.ECHOKE | unix.ECHOCTL
// Set control characters to sensible defaults
termios.Cc[unix.VEOF] = 4 // Ctrl+D
termios.Cc[unix.VERASE] = 127 // DEL
termios.Cc[unix.VINTR] = 3 // Ctrl+C
termios.Cc[unix.VKILL] = 21 // Ctrl+U
termios.Cc[unix.VMIN] = 1 // Minimum characters for read
termios.Cc[unix.VQUIT] = 28 // Ctrl+\
termios.Cc[unix.VSUSP] = 26 // Ctrl+Z
termios.Cc[unix.VTIME] = 0 // Timeout for read
// Set control characters to match node-pty defaults exactly
termios.Cc[unix.VEOF] = 4 // Ctrl+D
termios.Cc[unix.VERASE] = 0x7f // DEL (127)
termios.Cc[unix.VWERASE] = 23 // Ctrl+W (word erase)
termios.Cc[unix.VKILL] = 21 // Ctrl+U
termios.Cc[unix.VREPRINT] = 18 // Ctrl+R (reprint line)
termios.Cc[unix.VINTR] = 3 // Ctrl+C
termios.Cc[unix.VQUIT] = 0x1c // Ctrl+\ (28)
termios.Cc[unix.VSUSP] = 26 // Ctrl+Z
termios.Cc[unix.VSTART] = 17 // Ctrl+Q (XON)
termios.Cc[unix.VSTOP] = 19 // Ctrl+S (XOFF)
termios.Cc[unix.VLNEXT] = 22 // Ctrl+V (literal next)
termios.Cc[unix.VDISCARD] = 15 // Ctrl+O (discard output)
termios.Cc[unix.VMIN] = 1 // Minimum characters for read
termios.Cc[unix.VTIME] = 0 // Timeout for read
// Set platform-specific control characters for macOS
// These constants might not exist on all platforms, so we check
if vdsusp, ok := getControlCharConstant("VDSUSP"); ok {
termios.Cc[vdsusp] = 25 // Ctrl+Y (delayed suspend)
}
if vstatus, ok := getControlCharConstant("VSTATUS"); ok {
termios.Cc[vstatus] = 20 // Ctrl+T (status)
}
// Apply the terminal attributes
if err := unix.IoctlSetTermios(fd, ioctlSetTermios, termios); err != nil {
@ -77,7 +96,7 @@ func configurePTYTerminal(ptyFile *os.File) error {
return nil
}
debugLog("[DEBUG] PTY terminal configured to match node-pty defaults")
debugLog("[DEBUG] PTY terminal configured to match node-pty defaults with echo and flow control enabled")
return nil
}

View file

@ -8,3 +8,18 @@ const (
ioctlGetTermios = unix.TIOCGETA
ioctlSetTermios = unix.TIOCSETA
)
// getControlCharConstant returns the platform-specific control character constant if it exists
func getControlCharConstant(name string) (uint8, bool) {
// Platform-specific constants for Darwin
switch name {
case "VDSUSP":
// VDSUSP is index 11 on Darwin
return 11, true
case "VSTATUS":
// VSTATUS is index 18 on Darwin
return 18, true
default:
return 0, false
}
}

View file

@ -8,3 +8,9 @@ const (
ioctlGetTermios = unix.TCGETS
ioctlSetTermios = unix.TCSETS
)
// getControlCharConstant returns the platform-specific control character constant if it exists
func getControlCharConstant(name string) (uint8, bool) {
// No platform-specific constants for Linux
return 0, false
}

View file

@ -9,3 +9,9 @@ const (
ioctlGetTermios = unix.TCGETS
ioctlSetTermios = unix.TCSETS
)
// getControlCharConstant returns the platform-specific control character constant if it exists
func getControlCharConstant(name string) (uint8, bool) {
// No platform-specific constants for other systems
return 0, false
}

View file

@ -1,7 +1,6 @@
package terminal
import (
"bytes"
"encoding/binary"
"sync"
"unicode/utf8"
@ -151,38 +150,283 @@ func (tb *TerminalBuffer) Resize(cols, rows int) {
// SerializeToBinary converts the buffer snapshot to the binary format expected by the web client
func (snapshot *BufferSnapshot) SerializeToBinary() []byte {
var buf bytes.Buffer
// Pre-calculate actual data size for efficiency
dataSize := 28 // Header size (2 magic + 1 version + 1 flags + 4*6 for dimensions/cursor/reserved)
// Write dimensions (4 bytes each, little endian)
binary.Write(&buf, binary.LittleEndian, uint32(snapshot.Cols))
binary.Write(&buf, binary.LittleEndian, uint32(snapshot.Rows))
binary.Write(&buf, binary.LittleEndian, uint32(snapshot.ViewportY))
binary.Write(&buf, binary.LittleEndian, uint32(snapshot.CursorX))
binary.Write(&buf, binary.LittleEndian, uint32(snapshot.CursorY))
// Write cells
for y := 0; y < snapshot.Rows; y++ {
for x := 0; x < snapshot.Cols; x++ {
cell := snapshot.Cells[y][x]
// Encode character as UTF-8
charBytes := make([]byte, 4)
n := utf8.EncodeRune(charBytes, cell.Char)
// Write char length (1 byte)
buf.WriteByte(byte(n))
// Write char bytes
buf.Write(charBytes[:n])
// Write foreground color (4 bytes)
binary.Write(&buf, binary.LittleEndian, cell.Fg)
// Write background color (4 bytes)
binary.Write(&buf, binary.LittleEndian, cell.Bg)
// Write flags (1 byte)
buf.WriteByte(cell.Flags)
// First pass: calculate exact size needed
for row := 0; row < snapshot.Rows; row++ {
rowCells := snapshot.Cells[row]
if isEmptyRow(rowCells) {
// Empty row marker: 2 bytes
dataSize += 2
} else {
// Row header: 3 bytes (marker + length)
dataSize += 3
// Trim trailing blank cells
trimmedCells := trimRowCells(rowCells)
for _, cell := range trimmedCells {
dataSize += calculateCellSize(cell)
}
}
}
return buf.Bytes()
buffer := make([]byte, dataSize)
offset := 0
// Write header (32 bytes)
binary.LittleEndian.PutUint16(buffer[offset:], 0x5654) // Magic "VT"
offset += 2
buffer[offset] = 0x01 // Version 1
offset++
buffer[offset] = 0x00 // Flags
offset++
binary.LittleEndian.PutUint32(buffer[offset:], uint32(snapshot.Cols))
offset += 4
binary.LittleEndian.PutUint32(buffer[offset:], uint32(snapshot.Rows))
offset += 4
binary.LittleEndian.PutUint32(buffer[offset:], uint32(snapshot.ViewportY))
offset += 4
binary.LittleEndian.PutUint32(buffer[offset:], uint32(snapshot.CursorX))
offset += 4
binary.LittleEndian.PutUint32(buffer[offset:], uint32(snapshot.CursorY))
offset += 4
binary.LittleEndian.PutUint32(buffer[offset:], 0) // Reserved
offset += 4
// Write cells with optimized format
for row := 0; row < snapshot.Rows; row++ {
rowCells := snapshot.Cells[row]
if isEmptyRow(rowCells) {
// Empty row marker
buffer[offset] = 0xfe // Empty row marker
offset++
buffer[offset] = 1 // Count of empty rows (for now just 1)
offset++
} else {
// Row with content
buffer[offset] = 0xfd // Row marker
offset++
trimmedCells := trimRowCells(rowCells)
binary.LittleEndian.PutUint16(buffer[offset:], uint16(len(trimmedCells)))
offset += 2
// Write each cell
for _, cell := range trimmedCells {
offset = encodeCell(buffer, offset, cell)
}
}
}
// Return exact size buffer
return buffer[:offset]
}
// Helper functions for binary serialization
// isEmptyRow checks if a row contains only empty cells
func isEmptyRow(cells []BufferCell) bool {
if len(cells) == 0 {
return true
}
if len(cells) == 1 && cells[0].Char == ' ' && cells[0].Fg == 0 && cells[0].Bg == 0 && cells[0].Flags == 0 {
return true
}
for _, cell := range cells {
if cell.Char != ' ' || cell.Fg != 0 || cell.Bg != 0 || cell.Flags != 0 {
return false
}
}
return true
}
// trimRowCells removes trailing blank cells from a row
func trimRowCells(cells []BufferCell) []BufferCell {
lastNonBlank := len(cells) - 1
for lastNonBlank >= 0 {
cell := cells[lastNonBlank]
if cell.Char != ' ' || cell.Fg != 0 || cell.Bg != 0 || cell.Flags != 0 {
break
}
lastNonBlank--
}
// Keep at least one cell
if lastNonBlank < 0 {
return cells[:1]
}
return cells[:lastNonBlank+1]
}
// calculateCellSize calculates the size needed to encode a cell
func calculateCellSize(cell BufferCell) int {
isSpace := cell.Char == ' '
hasAttrs := cell.Flags != 0
hasFg := cell.Fg != 0
hasBg := cell.Bg != 0
isAscii := cell.Char <= 127
if isSpace && !hasAttrs && !hasFg && !hasBg {
return 1 // Just a space marker
}
size := 1 // Type byte
if isAscii {
size++ // ASCII character
} else {
charBytes := utf8.RuneLen(cell.Char)
size += 1 + charBytes // Length byte + UTF-8 bytes
}
// Attributes/colors byte
if hasAttrs || hasFg || hasBg {
size++ // Flags byte for attributes
if hasFg {
if cell.Fg > 255 {
size += 3 // RGB
} else {
size++ // Palette
}
}
if hasBg {
if cell.Bg > 255 {
size += 3 // RGB
} else {
size++ // Palette
}
}
}
return size
}
// encodeCell encodes a single cell into the buffer
func encodeCell(buffer []byte, offset int, cell BufferCell) int {
isSpace := cell.Char == ' '
hasAttrs := cell.Flags != 0
hasFg := cell.Fg != 0
hasBg := cell.Bg != 0
isAscii := cell.Char <= 127
// Type byte format:
// Bit 7: Has extended data (attrs/colors)
// Bit 6: Is Unicode (vs ASCII)
// Bit 5: Has foreground color
// Bit 4: Has background color
// Bit 3: Is RGB foreground (vs palette)
// Bit 2: Is RGB background (vs palette)
// Bits 1-0: Character type (00=space, 01=ASCII, 10=Unicode)
if isSpace && !hasAttrs && !hasFg && !hasBg {
// Simple space - 1 byte
buffer[offset] = 0x00 // Type: space, no extended data
return offset + 1
}
var typeByte byte = 0
if hasAttrs || hasFg || hasBg {
typeByte |= 0x80 // Has extended data
}
if !isAscii {
typeByte |= 0x40 // Is Unicode
typeByte |= 0x02 // Character type: Unicode
} else if !isSpace {
typeByte |= 0x01 // Character type: ASCII
}
if hasFg {
typeByte |= 0x20 // Has foreground
if cell.Fg > 255 {
typeByte |= 0x08 // Is RGB
}
}
if hasBg {
typeByte |= 0x10 // Has background
if cell.Bg > 255 {
typeByte |= 0x04 // Is RGB
}
}
buffer[offset] = typeByte
offset++
// Write character
if !isAscii {
charBytes := make([]byte, 4)
n := utf8.EncodeRune(charBytes, cell.Char)
buffer[offset] = byte(n)
offset++
copy(buffer[offset:], charBytes[:n])
offset += n
} else if !isSpace {
buffer[offset] = byte(cell.Char)
offset++
}
// Write extended data if present
if typeByte&0x80 != 0 {
// Attributes byte (convert our flags to Node.js format)
var attrs byte = 0
if cell.Flags&0x01 != 0 { // Bold
attrs |= 0x01
}
if cell.Flags&0x02 != 0 { // Italic
attrs |= 0x02
}
if cell.Flags&0x04 != 0 { // Underline
attrs |= 0x04
}
if cell.Flags&0x08 != 0 { // Inverse/Dim - map inverse to dim in Node.js
attrs |= 0x08
}
// Note: Node.js has additional attributes we don't support yet
if hasAttrs || hasFg || hasBg {
buffer[offset] = attrs
offset++
}
// Foreground color
if hasFg {
if cell.Fg > 255 {
// RGB
buffer[offset] = byte((cell.Fg >> 16) & 0xff)
offset++
buffer[offset] = byte((cell.Fg >> 8) & 0xff)
offset++
buffer[offset] = byte(cell.Fg & 0xff)
offset++
} else {
// Palette
buffer[offset] = byte(cell.Fg)
offset++
}
}
// Background color
if hasBg {
if cell.Bg > 255 {
// RGB
buffer[offset] = byte((cell.Bg >> 16) & 0xff)
offset++
buffer[offset] = byte((cell.Bg >> 8) & 0xff)
offset++
buffer[offset] = byte(cell.Bg & 0xff)
offset++
} else {
// Palette
buffer[offset] = byte(cell.Bg)
offset++
}
}
}
return offset
}
// handlePrint handles printable characters

View file

@ -1,6 +1,7 @@
package terminal
import (
"encoding/binary"
"testing"
)
@ -109,10 +110,94 @@ func TestBufferSerialization(t *testing.T) {
snapshot := buffer.GetSnapshot()
data := snapshot.SerializeToBinary()
// Binary format should contain:
// - 5 uint32s for dimensions (20 bytes)
// - 4 cells with char data and attributes
if len(data) < 20 {
t.Errorf("Serialized data too short: %d bytes", len(data))
// Check header
if len(data) < 32 {
t.Fatalf("Serialized data too short: %d bytes", len(data))
}
// Check magic bytes "VT" (0x5654)
if data[0] != 0x54 || data[1] != 0x56 { // Little endian
t.Errorf("Invalid magic bytes: %02x %02x", data[0], data[1])
}
// Check version
if data[2] != 0x01 {
t.Errorf("Invalid version: %02x", data[2])
}
// Check dimensions at correct offsets
cols := binary.LittleEndian.Uint32(data[4:8])
rows := binary.LittleEndian.Uint32(data[8:12])
if cols != 2 || rows != 2 {
t.Errorf("Invalid dimensions: %dx%d", cols, rows)
}
}
func TestBinaryFormatOptimizations(t *testing.T) {
// Test empty row optimization
buffer := NewTerminalBuffer(10, 3)
buffer.Write([]byte("Hello")) // First row has content
buffer.Write([]byte("\r\n")) // Second row empty
buffer.Write([]byte("\r\nWorld")) // Third row has content
snapshot := buffer.GetSnapshot()
data := snapshot.SerializeToBinary()
// Skip header (28 bytes - the Node.js comment says 32 but it's actually 28)
offset := 28
// First row should have content marker (0xfd)
if data[offset] != 0xfd {
t.Errorf("Expected row marker 0xfd at offset %d, got %02x (decimal %d)", offset, data[offset], data[offset])
}
// Find empty row marker (0xfe) - it should be somewhere in the data
foundEmptyRow := false
for i := offset; i < len(data)-1; i++ {
if data[i] == 0xfe {
foundEmptyRow = true
break
}
}
if !foundEmptyRow {
t.Error("Empty row marker not found in serialized data")
}
// Test ASCII character encoding with type byte
buffer3 := NewTerminalBuffer(5, 1)
buffer3.Write([]byte("A")) // Single ASCII character
snapshot3 := buffer3.GetSnapshot()
data3 := snapshot3.SerializeToBinary()
// Look for ASCII type byte (0x01) followed by 'A' (0x41)
foundAsciiEncoding := false
for i := 28; i < len(data3)-1; i++ {
if data3[i] == 0x01 && data3[i+1] == 0x41 {
foundAsciiEncoding = true
break
}
}
if !foundAsciiEncoding {
t.Error("ASCII encoding (type 0x01 + char) not found in serialized data")
}
// Test Unicode character encoding
buffer4 := NewTerminalBuffer(5, 1)
buffer4.Write([]byte("世")) // Unicode character
snapshot4 := buffer4.GetSnapshot()
data4 := snapshot4.SerializeToBinary()
// Look for Unicode type byte (bit 6 set = 0x40+)
foundUnicodeEncoding := false
for i := 32; i < len(data4); i++ {
if (data4[i] & 0x40) != 0 { // Unicode bit set
foundUnicodeEncoding = true
break
}
}
if !foundUnicodeEncoding {
t.Error("Unicode encoding (type with bit 6 set) not found in serialized data")
}
}