Repository navigation
fix process output capture and timeouts - #463
Conversation
chruffins
left a comment
There was a problem hiding this comment.
reviewed — requesting changes. two correctness bugs and one structural item to address first.
Bugs
server/cmd/api/api/process_runner.go:202—exitCodeis published before pending capture failures are drained (:208-220).GETstatus can returnexitedwithout theoutput_limit/output_errorreason oroutput_truncated: true. SSE and exec wait ondoneCh, so they're unaffected. consider drainingfailuresbefore publishingexitCode; all capture producers have finished at that point, soclose(failures)plus a range is safe.server/cmd/api/api/process_runner.go:29— PTY spawns create a capture file they never use (TTY output never reachescaptureOutput,:134), so a PTY spawn fails whenPROCESS_OUTPUT_DIRis unusable or disk/fd pressure is high. consider opening capture only on the non-TTY path.server/cmd/api/api/process.go:292(fallback inchild_process_other.go:20-29) — the non-Linux fallback turns INT/HUP intoKill(), where base forwarded the requested signal. non-Linux is dev-only, so low urgency, but the fallback could forward non-KILL signals withProcess.Signal.server/cmd/api/api/process_runner.go:201withprocess_wait_linux.go:16— on Unikraft (staginglib/posix-process/wait.c),waitidreaps an already-exited child even withWNOWAIT. the followingcmd.Wait()then getsECHILD, soProcessStateis nil andexit_codereports-1for fast exits. this also breaks the PID-reuse invariant described atprocess_wait_linux.go:12-13. not yet confirmed on the deployed runtime; worth an exec oftrueon a Unikraft build. one possible fix is passing a non-nilinfopand reading the status from it.
Structural / Maintainability
server/cmd/api/api/process.go:393-450— each SSE request builds anio.Pipe, a cancel wrapper, and about four goroutines before the response visitor (:465) writes bytes. the visitor could own the replay loop and write directly, which removes the pipe and its goroutines rather than moving them.
Coverage
19 non-generated changed files reviewed statically. server/lib/oapi/oapi.go is generated and was checked for schema consistency only. no tests were run.
chruffins
left a comment
There was a problem hiding this comment.
reviewed — approving. one structural item below worth doing as a follow-up or before merging if you have time.
Structural / Maintainability
server/cmd/api/api/process_runner.go:136-204— capture completion and capture failure use separate channels, aWaitGroup, a waiter goroutine, two completion flags, and a post-loop drain. each reader could send exactly one result (nilon success) onto a single channel, and the loop would consume those before publishing terminal status. that removes the waiter goroutine, the separate failure queue, the drain pass, and the redundant flags, and the runner's coordination gets much easier to follow.
Coverage
23 changed files reviewed statically at ae4aca7c. server/lib/oapi/oapi.go is generated and was checked for schema consistency only. no tests were run.
hiroTamada
left a comment
There was a problem hiding this comment.
reviewed — no blocking findings. two nonblocking suggestions:
server/cmd/api/api/process.go:388–391— consider lettingLast-Event-IDoverride the query cursor. anEventSourceinitialized with?offset=0otherwise replays everything on automatic reconnect.server/cmd/api/api/process_lifecycle_test.go:208— consider adding permanent tests for exec request cancellation and spawn shutdown. temporary checks covering both passed.
validation: full non-e2e suite passed with -race; go vet ./... passed. container e2e compiled but wasn't run.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fcb2ed6. Configure here.

Summary
commands can stall when nobody consumes their output, and process exit can race capture and lose final output or the exit event. exec timeouts kill only the direct child and return a retryable 500; spawn does not enforce its timeout.
capture non-TTY stdout/stderr into bounded temporary files independently of readers. cursorless streams keep shared consumption, so reconnects continue unread output without replaying consumed output or exit events. explicit
offset=0, saved offsets, andLast-Event-IDopt into independent replay/resume.Last-Event-IDtakes precedence over the query cursor so automatic EventSource reconnects do not repeat output. the response visitor writes SSE directly. retained output expires after 10 seconds, including stalled streams; the default limits are 64 MiB per process and 256 MiB total.exec and spawn enforce deadlines against the process group. exec failures return 422 with a reason and partial output. capture readers report completion and errors through one result channel; terminal status is published after both results are processed. manual
/killstill forwards the requested signal to the direct process, and TTY sessions no longer allocate unused capture files. inherited pipes have a one-second drain bound; deliberately detached descendants may survive.Validation
built and ran both headless and headful images from
fc08a38, then sent requests to their running APIs. these are the key checks and observed results:tail. it finished without a reader. connecting two seconds later returned every byte, includingtail, followed by the exit event.first, disconnected, then let the command printsecond. reconnecting returned onlysecond. explicitly requesting the history returnedfirstsecond.printf partial; sleep 30with a one-second timeout. the API returned422 process_timeoutand preservedpartial. the SDK sent only one request. separate checks confirmed ordinary child processes also stopped.output_error, previously captured output remained readable, and new commands could write again after cleanup. exceeding the configured output limit similarly reportedoutput_limit.helloproducedreply:hello.example output printed by the SDK timeout check, on both images:
{"requests":1,"status":422,"code":"process_timeout"}known trade off: a child that deliberately detaches can survive a timeout. that check returned after about two seconds with output marked incomplete; the surviving child was cleaned up afterward.
Note
High Risk
Changes core remote command execution (process groups, output limits, HTTP 422 contracts, and long-lived SSE), which can affect callers relying on old timeout/500 behavior or unbounded output.
Overview
Refactors process exec/spawn so non-TTY stdout/stderr are captured into bounded on-disk buffers (per-process and global quotas via
PROCESS_OUTPUT_*) whether or not anyone is streaming, with 10s retention and replay/resume via SSEoffset/Last-Event-ID.Exec and spawn share
startProcesslifecycle: process-group deadlines and cancellation/shutdown use SIGKILL on the group (with a short post-exit drain),/process/exectimeouts and server-driven stops return HTTP 422 withtermination_reasonand partial base64 output instead of retryable 500s, and status/stream exit events exposeoutput_truncated. TTY spawn skips file capture; manual/killstill signals only the direct process.Adds
docs/processes.md, OpenAPI/schema updates, scale-to-zero fix so handlerSetWriteDeadlineis not overridden on long streams, plus broad unit/e2e coverage.Reviewed by Cursor Bugbot for commit fc08a38. Bugbot is set up for automated code reviews on this repo. Configure here.