fix(stdio): buffered stream and terminal modes for PTY for broken TUI#522
fix(stdio): buffered stream and terminal modes for PTY for broken TUI#522
Conversation
Signed-off-by: Samuel K <skevetter@pm.me>
📝 WalkthroughWalkthroughAdds comprehensive PTY TerminalModes when requesting an SSH PTY and converts the stdio connection to buffered I/O (32 KiB reader/writer) with immediate flush behavior; also introduces tests for the buffered stdio behavior. Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
skevetter
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/stdio/conn_test.go`:
- Around line 60-66: The test currently only asserts wc.closed after
stream.Close(); also assert that buffered data was flushed by checking the
underlying buffer contents: after writing testData to stream.out and calling
stream.Close(), verify output.Bytes() (or output.String()) contains the original
testData (e.g., equals testData) to ensure Close flushed the buffer; update the
test that references stream, stream.out, wc.closed and output to include this
assertion.
In `@pkg/stdio/conn.go`:
- Around line 65-68: The current code returns immediately on s.out.Flush() error
which prevents closing the underlying writer (outRaw) and can leak descriptors;
change the logic in conn.go so you call s.out.Flush() and capture its error,
then always call outRaw.Close() (or s.outRaw.Close()) regardless of the flush
result, and finally return the appropriate error: if only flush failed return
that error, if only close failed return the close error, and if both failed
return a combined/wrapped error that preserves both causes. Ensure you reference
s.out.Flush() and outRaw.Close()/s.outRaw.Close() when making the changes.
pkg/stdio/conn.go
Outdated
| // Flush any remaining buffered data | ||
| if err := s.out.Flush(); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Always close the underlying writer, even if flush fails.
At Line 66, returning on Flush() error prevents Line 75 from executing, so outRaw may never close. That can leak descriptors and miss EOF signaling.
Proposed fix
func (s *StdioStream) Close() error {
- // Flush any remaining buffered data
- if err := s.out.Flush(); err != nil {
- return err
- }
+ flushErr := s.out.Flush()
+ closeErr := s.outRaw.Close()
if s.exitOnClose {
// We kill ourself here because the streams are closed
os.Exit(s.exitCode)
}
- return s.outRaw.Close()
+ if flushErr != nil {
+ return flushErr
+ }
+ return closeErr
}Also applies to: 75-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/stdio/conn.go` around lines 65 - 68, The current code returns immediately
on s.out.Flush() error which prevents closing the underlying writer (outRaw) and
can leak descriptors; change the logic in conn.go so you call s.out.Flush() and
capture its error, then always call outRaw.Close() (or s.outRaw.Close())
regardless of the flush result, and finally return the appropriate error: if
only flush failed return that error, if only close failed return the close
error, and if both failed return a combined/wrapped error that preserves both
causes. Ensure you reference s.out.Flush() and outRaw.Close()/s.outRaw.Close()
when making the changes.
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/stdio/conn.go (1)
63-73:⚠️ Potential issue | 🟠 Major
Flush()failure still preventsoutRaw.Close()— resource leak risk.If
s.out.Flush()returns an error at Line 64, the method returns early andoutRaw.Close()on Line 73 is never reached, potentially leaking the file descriptor and preventing EOF signaling to the peer.Proposed fix — always close the underlying writer
func (s *StdioStream) Close() error { - if err := s.out.Flush(); err != nil { - return err - } + flushErr := s.out.Flush() if s.exitOnClose { os.Exit(s.exitCode) } - return s.outRaw.Close() + closeErr := s.outRaw.Close() + if flushErr != nil { + return flushErr + } + return closeErr }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/stdio/conn.go` around lines 63 - 73, In StdioStream.Close, ensure the underlying writer is always closed even if s.out.Flush() fails: call s.outRaw.Close() regardless of Flush error (use a defer or run Close in a finally-style block) and then handle exit logic (s.exitOnClose / os.Exit(s.exitCode)) after attempting both operations; return the most relevant error (prefer returning the flush error if present, or the close error if flush succeeded) so callers still see failures but we avoid leaking the underlying resource (refer to StdioStream.Close, s.out.Flush, s.outRaw.Close, s.exitOnClose and s.exitCode).
🧹 Nitpick comments (2)
pkg/stdio/conn.go (1)
54-59: Write-side buffering is a no-op when everyWriteflushes immediately.Since
Flush()is called after everyWrite(), no data ever accumulates in the buffer across calls — each write goes straight to the underlying writer in one (or two, for writes > 32 KiB) syscalls, same as unbuffered I/O. The 32 KiB buffer allocation per stream adds overhead without providing the escape-sequence-coalescing benefit described in the comment.If the goal is to prevent a single large write from being fragmented by the kernel,
bufio.Writerdoesn't help: it will itself split data at the buffer boundary. If the real benefit is on the read side only (reducing syscall overhead), consider dropping the write-side buffer and writing directly tooutRaw:Suggested simplification
func (s *StdioStream) Write(b []byte) (n int, err error) { - n, err = s.out.Write(b) - if err != nil { - return n, err - } - return n, s.out.Flush() + return s.outRaw.Write(b) }If there is a caller pattern where multiple small writes should be coalesced before flushing (e.g., batch rendering), the flush should be moved out of
Writeand into an explicitFlush()method or deferred toClose, not called on every write.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/stdio/conn.go` around lines 54 - 59, The Write method on StdioStream currently calls s.out.Flush() on every Write, which negates the bufio.Writer buffering; remove the per-Write flush and either write directly to the raw writer (outRaw) or keep the bufio.Writer (s.out) but expose an explicit Flush method and call it only when callers need coalescing (e.g., on Close or explicit Flush), updating StdioStream.Write to just write to s.out (or outRaw) without flushing and adding/using a StdioStream.Flush() that calls s.out.Flush() when buffering is desired.cmd/machine/ssh.go (1)
278-317: Baud rate values are lower than typical system defaults.The terminal modes configuration is well-documented and correct overall. However,
TTY_OP_ISPEED/TTY_OP_OSPEEDat 14400 baud is notably lower than the standard OS PTY default of 38400 baud (common on Linux systems). While baud rates on pseudo-terminals are advisory and don't affect actual throughput, using 14400 is unusual. Consider aligning with the typical OS default of 38400 or higher (e.g., 115200):- ssh.TTY_OP_ISPEED: 14400, // Input speed (baud rate) - ssh.TTY_OP_OSPEED: 14400, // Output speed (baud rate) + ssh.TTY_OP_ISPEED: 38400, // Input speed (baud rate) + ssh.TTY_OP_OSPEED: 38400, // Output speed (baud rate)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/machine/ssh.go` around lines 278 - 317, The termModes map sets ssh.TTY_OP_ISPEED and ssh.TTY_OP_OSPEED to 14400 which is below common PTY defaults; update these entries in the termModes initialization (refer to termModes, ssh.TTY_OP_ISPEED and ssh.TTY_OP_OSPEED) to a more typical value such as 38400 (or 115200 if you prefer) so the baud rate aligns with standard OS PTY defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/stdio/conn.go`:
- Around line 63-73: In StdioStream.Close, ensure the underlying writer is
always closed even if s.out.Flush() fails: call s.outRaw.Close() regardless of
Flush error (use a defer or run Close in a finally-style block) and then handle
exit logic (s.exitOnClose / os.Exit(s.exitCode)) after attempting both
operations; return the most relevant error (prefer returning the flush error if
present, or the close error if flush succeeded) so callers still see failures
but we avoid leaking the underlying resource (refer to StdioStream.Close,
s.out.Flush, s.outRaw.Close, s.exitOnClose and s.exitCode).
---
Nitpick comments:
In `@cmd/machine/ssh.go`:
- Around line 278-317: The termModes map sets ssh.TTY_OP_ISPEED and
ssh.TTY_OP_OSPEED to 14400 which is below common PTY defaults; update these
entries in the termModes initialization (refer to termModes, ssh.TTY_OP_ISPEED
and ssh.TTY_OP_OSPEED) to a more typical value such as 38400 (or 115200 if you
prefer) so the baud rate aligns with standard OS PTY defaults.
In `@pkg/stdio/conn.go`:
- Around line 54-59: The Write method on StdioStream currently calls
s.out.Flush() on every Write, which negates the bufio.Writer buffering; remove
the per-Write flush and either write directly to the raw writer (outRaw) or keep
the bufio.Writer (s.out) but expose an explicit Flush method and call it only
when callers need coalescing (e.g., on Close or explicit Flush), updating
StdioStream.Write to just write to s.out (or outRaw) without flushing and
adding/using a StdioStream.Flush() that calls s.out.Flush() when buffering is
desired.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/stdio/conn.go (1)
62-76: Previous feedback addressed:outRaw.Close()is now always called regardless of flush outcome.One remaining nit: if both
Flush()andClose()fail,closeErris silently discarded. Considererrors.Join(flushErr, closeErr)so callers see both failures.Optional: preserve both errors
+ "errors" "io"- if flushErr != nil { - return flushErr - } - return closeErr + return errors.Join(flushErr, closeErr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/stdio/conn.go` around lines 62 - 76, In StdioStream.Close, if both s.out.Flush() and s.outRaw.Close() return errors the current code returns only flushErr and discards closeErr; change the return logic to preserve both errors by using errors.Join(flushErr, closeErr) (falling back to the single non-nil error when one is nil) while preserving the existing exit-on-close behavior that uses s.exitOnClose and s.exitCode; update imports to include "errors" if not already present.pkg/stdio/conn_test.go (1)
53-67: Previous flush-assertion feedback has been addressed.Line 66 now verifies buffered data is flushed before close — matches the earlier review request.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/stdio/conn_test.go` around lines 53 - 67, TestStdioStreamClose correctly verifies buffered data is flushed on Close: ensure the test continues to use NewStdioStream to create stream, write unflushed bytes to stream.out, call stream.Close(), assert no error, assert testWriteCloser.closed is true and assert output.Bytes() equals testData; no code changes required since the previous flush-assertion feedback has been addressed.
🧹 Nitpick comments (2)
pkg/stdio/conn_test.go (2)
37-51: Consider testingReadwith a buffer smaller than the input to exercise buffered-reader behavior.The current test uses a 1024-byte buffer for ~42 bytes of data, so the
bufio.Readerwrapper is never really exercised. A complementary case with a small buffer (e.g., 8 bytes) and a loop would validate multi-read and buffering semantics, which is the whole point of thebufio.NewReaderSizechange.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/stdio/conn_test.go` around lines 37 - 51, Add a complementary test to TestStdioStreamRead that exercises NewStdioStream's bufio.NewReaderSize behavior by reading from the stream using a small buffer (e.g., 8 bytes) in a loop until io.EOF, aggregating the bytes and asserting the total length and content equal the original testData; locate this around the existing TestStdioStreamRead and reuse the same input/testData and stream := NewStdioStream(...) setup so the buffered-reader logic is exercised across multiple Read calls.
69-77: Consider adding an error-path helper to cover flush/close failure scenarios.Currently all tests use a
testWriteCloserthat never fails. A variant that returns an error onWriteorClosewould let you verify thatWritepropagates flush errors and thatClosereturns the right error when flush or close fails (especially given the dual-error logic inconn.go).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/stdio/conn_test.go` around lines 69 - 77, Add a failing test helper type to exercise error paths: implement a variant of testWriteCloser (e.g., failingWriteCloser) that embeds bytes.Buffer but whose Write and/or Close methods return a supplied error; update tests that exercise conn.Write and conn.Close to use this helper to assert that Write propagates flush errors and that Close returns the correct error when flush fails or when the underlying Close fails (covering the dual-error logic in conn.go and the methods named testWriteCloser, Write, Close, and the conn Close/flush behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/stdio/conn_test.go`:
- Around line 53-67: TestStdioStreamClose correctly verifies buffered data is
flushed on Close: ensure the test continues to use NewStdioStream to create
stream, write unflushed bytes to stream.out, call stream.Close(), assert no
error, assert testWriteCloser.closed is true and assert output.Bytes() equals
testData; no code changes required since the previous flush-assertion feedback
has been addressed.
In `@pkg/stdio/conn.go`:
- Around line 62-76: In StdioStream.Close, if both s.out.Flush() and
s.outRaw.Close() return errors the current code returns only flushErr and
discards closeErr; change the return logic to preserve both errors by using
errors.Join(flushErr, closeErr) (falling back to the single non-nil error when
one is nil) while preserving the existing exit-on-close behavior that uses
s.exitOnClose and s.exitCode; update imports to include "errors" if not already
present.
---
Nitpick comments:
In `@pkg/stdio/conn_test.go`:
- Around line 37-51: Add a complementary test to TestStdioStreamRead that
exercises NewStdioStream's bufio.NewReaderSize behavior by reading from the
stream using a small buffer (e.g., 8 bytes) in a loop until io.EOF, aggregating
the bytes and asserting the total length and content equal the original
testData; locate this around the existing TestStdioStreamRead and reuse the same
input/testData and stream := NewStdioStream(...) setup so the buffered-reader
logic is exercised across multiple Read calls.
- Around line 69-77: Add a failing test helper type to exercise error paths:
implement a variant of testWriteCloser (e.g., failingWriteCloser) that embeds
bytes.Buffer but whose Write and/or Close methods return a supplied error;
update tests that exercise conn.Write and conn.Close to use this helper to
assert that Write propagates flush errors and that Close returns the correct
error when flush fails or when the underlying Close fails (covering the
dual-error logic in conn.go and the methods named testWriteCloser, Write, Close,
and the conn Close/flush behavior).
1 participant
Signed-off-by: Samuel K skevetter@pm.me
Summary by CodeRabbit
Bug Fixes
Performance
Tests