Repository navigation
guest-agent: stream non-TTY exec output as it is produced - #490
palashswapnil wants to merge 2 commits into
Conversation
Buffering stdout/stderr until exit stalls long-lived commands (relays, tails). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
hey! this seems like a straightforward improvement but I'm confused by your use case for this. is this for a project that you're using hypeman for? |
|
Yes! We run a small fleet of With (disclaimer: this was a /goal run on one of my agents tried to use a remote mac-mini as a fleet of kernel-based browsers and it worked around the launch restriction with this PR; feel free to drop as well) |
| if n > 0 { | ||
| chunk := append([]byte(nil), buf[:n]...) | ||
| sendMu.Lock() | ||
| _ = stream.Send(wrap(chunk)) |
There was a problem hiding this comment.
could you handle stream.Send errors here instead of continuing to drain output? please also tie the command context to stream.Context() so a client disconnect cancels the command.
There was a problem hiding this comment.
done in 0a3988c. exec ctx now comes from stream.Context() (timeout layered on top), so a disconnect kills the command; this covers tty too since it's set in Exec. a failed Send cancels the command; the pump keeps draining so the process can't block on a full pipe, and the handler returns the send error instead of an exit code
| go pump(stderr, func(b []byte) *pb.ExecResponse { | ||
| return &pb.ExecResponse{Response: &pb.ExecResponse_Stderr{Stderr: b}} | ||
| }) | ||
| wg.Wait() |
There was a problem hiding this comment.
wg.Wait() can block on a pump stuck in stream.Send, which means cmd.Wait() never runs. A command timeout won’t unblock a send stalled by flow control. could you define how a stalled client is handled and ensure the process still gets reaped? consider adding a regression test for this path.
There was a problem hiding this comment.
good catch, fixed in 0a3988c
- stdout/stderr are now our own
os.Pipes (notStdoutPipe), socmd.Wait()runs right away and always reaps, even with aSendstuck on flow control - stalled client: while the command runs it just backpressures (what relays want). after exit / timeout / disconnect, leftover output gets 5s (
execDrainTimeout) to flush, then the handler returns, which cancels the stream and unblocks the stuckSend. also bounds the old hang when a bg child holds stdout open - added tests for blocked send + timeout, failed send, and disconnect via
Exec; all three hang on the previous commit, pass now (go test -race, linux/arm64)
executeTTY has the same gap (ignores send errors, wg.Wait() can block on a stalled client); happy to do that in a follow-up so this one stays small
- Derive the command context from stream.Context() so a client disconnect kills the command. - On a failed Send, cancel the command and keep draining its output. - Own the stdout/stderr pipes so cmd.Wait reaps the process even while a Send is blocked on flow control; give output execDrainTimeout to flush after exit, then return so the stream is cancelled. - Regression tests for a stalled client, a failed send, and a disconnect.
This branch has not been deployed
2 participants
Problem
executeNoTTYreads stdout and stderr withio.ReadAlland sends nothing until the process exits. Any long-running non-TTY exec therefore delivers no output while it runs:tail -f/instances/{id}/exec)This showed up while using exec as a TCP relay for CDP and WebRTC to browser instances. HTTP/1.0 requests worked, because the server closes the connection and the relay exits. Keep-alive and WebSocket traffic hung forever.
Fix
Pump stdout and stderr concurrently, sending each chunk as it is read (up to 32 KiB, as before). Both pipes are still fully drained before
cmd.Wait(), and the exit code is still sent last. The TTY path already streams this way.One behaviour change: stdout and stderr chunks may now interleave in the order they are produced, instead of all stdout followed by all stderr.
Tests
TestExecuteNoTTYStreamsOutputBeforeExitrunsecho first; sleep 2; echo secondand asserts the first chunk arrives well before exit. It fails onmain("first chunk arrived at 2.003s but command exited at 2.004s") and passes with this change.go test ./lib/system/guest_agent/passes (linux/arm64, golang:1.25)./instances/{id}/execnow stream. That covered CDP keep-alive and WebSocket traffic, plus neko WebRTC over ICE-TCP, across a fleet of 7 instances.🤖 Generated with Claude Code