Repository navigation
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.
| reapProcess(h.cmd) | ||
| code := h.cmd.ProcessState.ExitCode() | ||
| h.durationMs = int(time.Since(h.started) / time.Millisecond) | ||
| h.exitCode = &code |
There was a problem hiding this comment.
Wait failure panics and hangs exec
High Severity
run always dereferences cmd.ProcessState after reapProcess, and ProcessExec waits on doneCh with no timeout. If waitid(WNOWAIT) reaps the child or Wait fails, ProcessState is nil, the runner panics before closing doneCh, and exec hangs while the process stays registered.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit fcb2ed6. Configure here.


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. terminal status is published after capture failures are recorded. 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:
6772b03. the container e2e suite compiles locally; it was not rerun locally for this merge.exit_code: -1without panicking.os.ProcessState.ExitCode()explicitly handles nil.remaining validation gap: Linux image checks exercise
waitid(WNOWAIT), but compatibility on Unikraft remains unverified. the reviewed upstream implementation may reap an already-exited child despiteWNOWAIT, affecting exit status and PID reservation.Note
High Risk
Changes core remote command execution (process groups, output quotas, timeout/cancel semantics, and HTTP status codes), which can affect any caller relying on prior exec 500 timeouts or unbounded output buffering.
Overview
Reworks process execution so non-TTY stdout/stderr are captured into bounded on-disk buffers whether or not anyone is streaming, with shared per-server quotas (
PROCESS_OUTPUT_*, default 64 MiB/process and 256 MiB total) and 10s post-exit retention.Exec and spawn share a new
startProcesslifecycle: process groups for automatic timeout/cancel/shutdown (SIGKILL to the group, 1s pipe drain), Linuxwaitid(WNOWAIT)so descendants can be signaled before reap, and manual/killstill signals only the direct child. Exec timeouts, output limits, cancel, and capture errors return HTTP 422 withProcessTerminationReason, partial base64 output, andoutput_truncatedinstead of retryable 500s.SSE streaming is rewritten for replay/resume via
offset/Last-Event-ID(with precedence rules), cursorless shared consumption, SSEidcursors, and unblocking stalled streams when retention expires; scale-to-zero middleware now honors handlerSetWriteDeadline.OpenAPI and
docs/processes.mddocument the contracts; broad unit, lifecycle, and container e2e tests cover the behavior.Reviewed by Cursor Bugbot for commit 6772b03. Bugbot is set up for automated code reviews on this repo. Configure here.