extend process spawn to allow allocating a tty, attaching #96
Conversation
Mesa Descriptionusing the same approach that
demo running against the image running in unikraft: https://screen.studio/share/6imUEYCv Note This PR adds the ability to spawn processes with a pseudo-terminal (PTY) and attach to them for interactive sessions, similar to
Written by Cursor Bugbot for commit 55adbad. This will update automatically on new commits. Configure here. Description generated by Mesa. Update settings |
![mesa-dot-dev[bot]](https://avatars.githubusercontent.com/in/1050077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of 1d2d45e...7d69152
Analysis
-
Goroutine and Connection Leaks: The
HandleProcessAttachfunction creates multiple goroutines for I/O copying without proper cleanup mechanisms. There's no context-based cancellation, connection timeouts, or tracking to prevent resource exhaustion when clients disconnect improperly. -
PTY Process Cleanup Issues: If
pty.Start(cmd)fails, there's no guaranteed cleanup of potentially started processes, which could lead to zombie process leaks. -
Security Vulnerabilities: The
/attachendpoint lacks proper authentication beyond process ID validation, has no permission validation for process attachment, and allows multiple clients to attach to the same PTY which could cause output duplication and potential DoS attacks. -
Client Timeout Problems: While the client implementation handles terminal modes and resize events properly, it has indefinite blocking for I/O operations without proper timeout handling.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
9 files reviewed | 0 comments | Edit Agent Settings • Read Docs
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Sayan-
left a comment
Sayan-
left a comment
There was a problem hiding this comment.
overall lgtm! nothing blocking ^^
| // Raw attach endpoint (HTTP hijack) - not part of OpenAPI spec | ||
| r.Get("/process/{process_id}/attach", func(w http.ResponseWriter, r *http.Request) { | ||
| id := chi.URLParam(r, "process_id") | ||
| apiService.HandleProcessAttach(w, r, id) | ||
| }) |
There was a problem hiding this comment.
from my understanding these routes will keep using all the router's middlewares. does that match yours? specifically curious on the stz implications
server/cmd/api/api/process.go
Outdated
| _, _ = io.Copy(processRW, clientConn) | ||
| shutdown() |
There was a problem hiding this comment.
nice how simple this one gets to be
Resolve conflicts in process.go (keep both PTY cleanup and scale-to-zero re-enable logic) and regenerate oapi.go from merged openapi.yaml.
| r.Get("/process/{process_id}/attach", func(w http.ResponseWriter, r *http.Request) { | ||
| id := chi.URLParam(r, "process_id") | ||
| apiService.HandleProcessAttach(w, r, id) | ||
| }) |
There was a problem hiding this comment.
Yes, this route uses all the router's middlewares including scale-to-zero. The behavior is actually correct for attach:
- When an attach request comes in, the STZ middleware disables scale-to-zero
- The handler hijacks the connection and blocks on
<-doneuntil the session ends - Since
HandleProcessAttachstays alive for the entire attach session, scale-to-zero remains disabled - When the session ends (client disconnects or process exits), the handler returns and the middleware's
deferre-enables scale-to-zero
So the STZ integration works correctly - the machine won't scale to zero while there's an active interactive session.
- Use buf.Reader instead of raw conn after HTTP hijack to consume any buffered data that was read ahead before the hijack - Add uint16 overflow validation for rows/cols in ProcessResize (returns 400 if values > 65535) - Add uint16 overflow validation for rows/cols in ProcessSpawn before starting the PTY process Addresses cursor[bot] review comments about potential data loss after hijack and silent integer overflow when converting to uint16.
| if errno == syscall.EAGAIN || errno == syscall.EWOULDBLOCK { | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
Incorrect error type assertion fails to handle errno
The type assertion rerr.(syscall.Errno) in copyPTYToConn will never succeed because os.File.Read() wraps errors in *os.PathError, not raw syscall.Errno. This causes the EAGAIN/EWOULDBLOCK continue logic at lines 746-748 to fail silently—instead of continuing the loop to retry, the function returns early on line 750. Using errors.Is(rerr, syscall.EIO) and similar would correctly unwrap and match these errors.
using the same approach that
docker exec ...uses:./cmd/shellcontains a proof of concept for what would eventually land in thekernelCLI.demo running against the image running in unikraft:
https://screen.studio/share/6imUEYCv
Note
Implements interactive PTY support for spawned processes and an attach/resize workflow, plus a small CLI demo and minor wrapper tweaks.
ProcessSpawnRequestwithallocate_tty,rows,cols; server spawns PTY-backed cmds viacreack/pty, tracksptyFile/isTTY, and defers pipe readers in PTY modeGET /process/{process_id}/attach(HTTP hijack) andPOST /process/{process_id}/resize; enforces single active attach and uses poll-based PTY→conn copy; cleans up FDs and retains process status briefly post-exitopenapi.yaml) and generated client/server (lib/oapi) to include PTY fields andprocessResize; adds depscreack/pty,x/sys,x/termcmd/shellPoC: spawns/bin/bashwith TTY, hijacks attach over TCP/TLS, resizes onSIGWINCHHOSTNAMEearly in headful/headless imagesWritten by Cursor Bugbot for commit 7d47214. This will update automatically on new commits. Configure here.