feat(exec): drain guest output before attach - #1022
Conversation
📦 BoxLite review — couldn't completepowered by BoxLite |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExecution output now uses bounded channels and guest-side sequenced buffering. Dropped output is reported explicitly, WebSocket streaming is separated from completion reporting, and wait endpoints provide execution status across REST, runner, and CLI servers. ChangesExecution output and completion flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ExecProtocol
participant OutputManager
participant WebSocketAttach
participant WaitEndpoint
participant Execution
ExecProtocol->>OutputManager: deliver decoded output
WebSocketAttach->>OutputManager: attach to buffered stream
OutputManager-->>WebSocketAttach: ordered output or OutputDropped
WaitEndpoint->>Execution: await terminal completion
Execution-->>WaitEndpoint: execution status and exit code
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
b9f4787 to
b0ee85c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/boxlite/src/portal/interfaces/exec.rs (1)
338-388: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
route_output().awaitignoresshutdown_token— a stalled consumer blocks shutdown indefinitely.The
select!at lines 338-353 only guardsstream.message(); the per-messageSelf::route_output(...).awaitat line 358 (and the terminal-path flushes/sends at lines 371-384) run outside it. Sincempsc::Sender::send().awaiton the now-bounded channel blocks until capacity frees up, a consumer that is alive but not polling (hasn't dropped itsExecStdout/ExecStderr) makes this task ignoreshutdown_token.cancelled()for as long as the channel stays full — exactly the scenario the non-blockingtry_flush()on the cancellation branch was designed to avoid.🔧 Race the per-message routing against cancellation too
match output.transpose() { Some(Ok(output)) => { message_count += 1; - Self::route_output(output, &mut stdout, &mut stderr).await; + tokio::select! { + biased; + _ = shutdown_token.cancelled() => { + stdout.try_flush(); + stderr.try_flush(); + break; + } + _ = Self::route_output(output, &mut stdout, &mut stderr) => {} + } }The same gap applies to the error/EOF terminal flushes (lines 371-384), though its impact there is smaller since the loop is about to exit anyway.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/boxlite/src/portal/interfaces/exec.rs` around lines 338 - 388, Update the attach-stream loop around route_output and the terminal error/EOF handling so every potentially blocking flush or send is raced against shutdown_token.cancelled(). Ensure cancellation exits promptly without awaiting a bounded-channel operation indefinitely, while preserving normal output ordering and existing cleanup behavior when no shutdown occurs.
🧹 Nitpick comments (1)
src/guest/src/service/exec/output.rs (1)
16-19: 🧹 Nitpick | 🔵 TrivialUnbounded ring accumulation across completed executions.
Each
OutputManagerretains its 1 MiB ring after the process exits (held byInner.outputand any live attach-stream clone), so total guest memory grows with the number of completed-but-uncleaned executions. The PR already notes TTL/cleanup as future work; consider bounding this with an eviction/TTL on completed executions before it becomes a guest-OOM vector under high exec churn.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/guest/src/service/exec/output.rs` around lines 16 - 19, Bound completed execution state retained by OutputManager so its 1 MiB ring is not kept indefinitely after process exit. Add eviction or TTL cleanup for completed executions, including releasing Inner.output and any attach-stream-held clones when cleanup occurs, while preserving access for active executions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/boxlite/src/portal/interfaces/exec.rs`:
- Around line 338-388: Update the attach-stream loop around route_output and the
terminal error/EOF handling so every potentially blocking flush or send is raced
against shutdown_token.cancelled(). Ensure cancellation exits promptly without
awaiting a bounded-channel operation indefinitely, while preserving normal
output ordering and existing cleanup behavior when no shutdown occurs.
---
Nitpick comments:
In `@src/guest/src/service/exec/output.rs`:
- Around line 16-19: Bound completed execution state retained by OutputManager
so its 1 MiB ring is not kept indefinitely after process exit. Add eviction or
TTL cleanup for completed executions, including releasing Inner.output and any
attach-stream-held clones when cleanup occurs, while preserving access for
active executions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 104bcb7e-857c-4403-a78a-aac86bcf1d0f
📒 Files selected for processing (9)
src/boxlite/src/litebox/exec.rssrc/boxlite/src/litebox/mod.rssrc/boxlite/src/portal/interfaces/exec.rssrc/boxlite/src/rest/litebox.rssrc/boxlite/tests/run_main_command.rssrc/guest/src/service/exec/mod.rssrc/guest/src/service/exec/output.rssrc/guest/src/service/exec/state.rssrc/shared/proto/boxlite/v1/service.proto
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e8b966028
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/src/commands/serve/handlers/executions.rs (1)
110-140: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
wait/status responses never surfaceerror_message, unlike the REST server and Go runner.
execution_status_bodyonly emitsexecution_id,status, andexit_code. This isn't just a JSON-construction gap —ActiveExecution's wait task (mod.rs) only storesresult.exit_codefromExecution::wait(), discardingresult.error_messageentirely, so there's nothing to surface even if this function were extended. Both the Go runner (ExecutionInfoResponse.ErrorMessage) and the REST client'sExecutionStatusResponse.error_messagecarry this field — e.g. the container-init-death diagnosis attached when a process is SIGKILLed via PID-namespace teardown. Theboxlite servebackend is now the one implementation of this shared "wait" contract that can't report it.Consider storing
error_messagealongsideexit_codeonActiveExecution(populated from the samewait()call) and including it inexecution_status_bodywhen present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/src/commands/serve/handlers/executions.rs` around lines 110 - 140, Extend ActiveExecution to retain result.error_message alongside the exit code when its wait task processes Execution::wait(). Update execution_status_body and the related wait/status response path to include error_message when present, preserving the existing execution_id, status, and exit_code fields and omitting the new field when absent.
🧹 Nitpick comments (2)
src/boxlite/src/rest/litebox.rs (1)
73-90: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftSpawned WS pump task has no lifecycle handle; bounded output channel can block it forever.
tokio::spawn(...)return values are discarded in bothwire_attachandexec(). Combined with the new boundedstdout_tx/stderr_txchannels,attach_ws_pump'sstdout_tx.send(text).await(line 802/806) blocks the entire pump loop — including reading the eventual exit frame and sending keepalive pings — whenever the channel is full. The added testwait_returns_when_unread_output_fills_the_websocket_queuedemonstrates exactly this and has toattach.abort()manually to clean up; production callers have no equivalent hook. A caller that keeps anExecutionalive (e.g., just to callwait()/signal()) without drainingstdout()/stderr()to completion will leave the WS connection and task stuck open until theExecutionitself is dropped.Worth confirming this is the intended tradeoff (bounded memory vs. potentially long-lived stuck connections for non-draining consumers), and if so, consider retaining the
JoinHandleso it can be aborted oncewait_executionresolves (or documenting the drain requirement onExecution).Also applies to: 154-173
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/boxlite/src/rest/litebox.rs` around lines 73 - 90, Retain the JoinHandle returned by the spawned attach_ws_pump task in both wire_attach and exec, and tie its lifecycle to wait_execution so the pump is aborted once execution completion resolves, including when output channels are not drained. Ensure the resulting Execution retains or exposes the necessary handle and cleanup occurs without changing normal output consumption behavior.apps/api/src/boxlite-rest/boxlite-proxy.controller.ts (1)
131-151: 🩺 Stability & Availability | 🔵 TrivialConfirm nothing upstream cuts this connection before the 25h
proxyTimeout.
proxyTimeoutonly bounds the proxy→runner leg. If the Node http server (main.ts, not in this changeset) or any load balancer/reverse proxy in front of this service has a shorter idle/socket timeout, a long-runningwaitcould still be truncated well before 25h, which would surface to SDK callers as a spurious network failure on the very endpoint meant to be authoritative for completion.Also applies to: 255-269
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/boxlite-rest/boxlite-proxy.controller.ts` around lines 131 - 151, The long-running proxyExecWait request may be terminated by shorter upstream Node HTTP server or load-balancer idle/socket timeouts despite its 25-hour proxyTimeout. Inspect and update the relevant server and deployment proxy timeout configuration so the connection remains valid for at least the full wait duration, covering the equivalent configuration for the other affected endpoint as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/boxlite/src/rest/litebox.rs`:
- Around line 974-1015: Update wait_execution to retry transient transport
errors from get_with_timeout using bounded backoff consistent with
WS_RECONNECT_BUDGET before producing an ExecResult failure. Preserve immediate
handling of terminal statuses and non-terminal responses, and send the synthetic
error result only after the retry budget is exhausted.
---
Outside diff comments:
In `@src/cli/src/commands/serve/handlers/executions.rs`:
- Around line 110-140: Extend ActiveExecution to retain result.error_message
alongside the exit code when its wait task processes Execution::wait(). Update
execution_status_body and the related wait/status response path to include
error_message when present, preserving the existing execution_id, status, and
exit_code fields and omitting the new field when absent.
---
Nitpick comments:
In `@apps/api/src/boxlite-rest/boxlite-proxy.controller.ts`:
- Around line 131-151: The long-running proxyExecWait request may be terminated
by shorter upstream Node HTTP server or load-balancer idle/socket timeouts
despite its 25-hour proxyTimeout. Inspect and update the relevant server and
deployment proxy timeout configuration so the connection remains valid for at
least the full wait duration, covering the equivalent configuration for the
other affected endpoint as well.
In `@src/boxlite/src/rest/litebox.rs`:
- Around line 73-90: Retain the JoinHandle returned by the spawned
attach_ws_pump task in both wire_attach and exec, and tie its lifecycle to
wait_execution so the pump is aborted once execution completion resolves,
including when output channels are not drained. Ensure the resulting Execution
retains or exposes the necessary handle and cleanup occurs without changing
normal output consumption behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a936e2d1-b314-4d66-b16f-c402e6108a55
📒 Files selected for processing (11)
apps/api/src/boxlite-rest/boxlite-proxy.controller.tsapps/runner/pkg/api/controllers/boxlite_exec.goapps/runner/pkg/api/controllers/boxlite_exec_test.goapps/runner/pkg/api/server.goopenapi/box.openapi.yamlsrc/boxlite/src/rest/client.rssrc/boxlite/src/rest/litebox.rssrc/boxlite/src/rest/types.rssrc/cli/src/commands/serve/README.mdsrc/cli/src/commands/serve/handlers/executions.rssrc/cli/src/commands/serve/mod.rs
|
Review follow-up:
Tests include REST wait retry coverage and the runner wait-timeout controller test. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openapi/box.openapi.yaml`:
- Around line 644-647: Update the endpoint’s 200-response description in the
OpenAPI definition to document both outcomes: a terminal execution result and a
non-terminal status: "running" response when the 45-minute wait expires. Replace
the “Terminal execution result” wording while preserving the existing timeout
and retry behavior details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3709c682-a0e9-424e-a3a5-cc86ec7fa2b0
📒 Files selected for processing (7)
apps/api/src/boxlite-rest/boxlite-proxy.controller.tsapps/runner/pkg/api/controllers/boxlite_exec.goapps/runner/pkg/api/controllers/boxlite_exec_test.goopenapi/box.openapi.yamlsrc/boxlite/src/rest/litebox.rssrc/cli/src/commands/serve/README.mdsrc/cli/src/commands/serve/handlers/executions.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/api/src/boxlite-rest/boxlite-proxy.controller.ts
- src/cli/src/commands/serve/README.md
- src/cli/src/commands/serve/handlers/executions.rs
- src/boxlite/src/rest/litebox.rs
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f28b16c618
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
BatmanByte
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7b2ce1a8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
DorianZheng
left a comment
DorianZheng
left a comment
There was a problem hiding this comment.
most of these trace to two changes in this PR — the unbounded→bounded output channels, and the narrowed wait-retry predicate:
- one task routes both streams through blocking bounded sends, so reading only one of stdout/stderr can stall the other (and on the ws pump a full channel also blocks stdin + the keepalive ping).
wait_executionretries onlyNetwork, so a transient 503/timeout/429 becomes a cachedexit_code:-1.
the rest are resource retention (guest ring + leaked wait tasks/timers), the auto-pause vs long-poll interaction, and a few decoder/observability items.
c7b2ce1 to
b68c092
Compare
BatmanByte
changed the title
ed4679a to
890ff64
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8685690f26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 381f0ee94e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .entries | ||
| .get(index) | ||
| .expect("ring sequence must exist") | ||
| .output | ||
| .clone(); |
There was a problem hiding this comment.
When attached commands produce substantial output, this clones each entry and advances only the attachment cursor without removing the entry or decrementing buffered_bytes. Because ExecutionRegistry only inserts execution states and has no removal path, every completed execution can permanently retain roughly 1 MiB in the guest, so repeated output-heavy execs can exhaust the VM's memory. Remove consumed entries once the single attachment receives them, while retaining data only before attachment.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. This PR adds bounded per-execution retention but does not yet define the terminal lifecycle for that retained output, so repeated completed executions can accumulate memory. I am not applying the suggested local pop-on-send change here: it would only help attached executions and would silently change the late-Attach replay guarantee while leaving unattached completed executions retained.
I will address this in a focused follow-up with a complete lifecycle: retain the ring while execution is active and for a bounded terminal grace window; release entries already handed to the sole Attach stream; clear terminal rings after the window when no attachment is active; and preserve terminal Wait results independently from output retention. The follow-up will also define what a late Attach observes after expiry, rather than reusing OutputDropped byte counts whose current meaning is ring overwrite. This needs tests for attached, never-attached, terminal, and reconnect cases.
|
@codex review |
Start draining an execution's stdout and stderr as soon as its guest-side state is created. Retain a bounded one-megabyte replay buffer so commands are not blocked when no client attaches, and report overwritten output explicitly when a client attaches late. The dropped control event carries stdout and stderr byte counts so the host only resets the decoder that lost bytes. This keeps an intact stream from receiving a spurious replacement character.
Track the stream cursor separately from ring retention so OutputDropped only reports bytes that have not been handed to the Attach stream. Clean up and reap a spawned process whenever its I/O handle setup or PTY handoff fails, including the container-backed paths. Replace the late-attach test's timing sleep with a guest-side completion marker. The marker is created only after the main command's pipe writes have completed, making the overflow assertion independent of host scheduling.
Replace the timing-based wait in the AsyncFd regression test with a signal emitted after the output stream first registers pending. The blocking-pool probe now starts only after the reader has been polled, avoiding scheduler-dependent false passes.
New guests report per-stream offsets and final byte counts so Attach can detect both mid-stream and terminal output loss. Keep the former dropped event as a deprecated decode-only wire variant for hosts attaching to existing guests.\n\nEmit each stream's terminal frame once its own final entry has been sent, rather than waiting for its sibling pipe. Also reap an init if its stdio cannot register with Tokio's reactor, matching the other spawn paths.
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ef4d6ab to
bf63a81
Compare
OutputDropped was introduced only by this unmerged PR and has no released guest compatibility requirement. Remove the deprecated field and host decoder so the Attach protocol is exclusively per-stream offsets plus final byte counts.
bf63a81 to
d165e21
Compare
![boxlite-agent[bot]](https://avatars.githubusercontent.com/in/4244458?s=60&v=4){kind=small}
| fn read_fd(fd: &OwnedFd, buffer: &mut [u8]) -> io::Result<usize> { | ||
| nix::unistd::read(fd.as_raw_fd(), buffer).map_err(Into::into) | ||
| } | ||
|
|
||
| fn write_fd(fd: &OwnedFd, buffer: &[u8]) -> io::Result<usize> { | ||
| nix::unistd::write(fd, buffer).map_err(Into::into) | ||
| } |
There was a problem hiding this comment.
nix::unistd::read/write errors map straight through; tokio's AsyncFd::async_io only retries the closure on WouldBlock, so an EINTR (plausible here given the guest's SIGCHLD/reap_fence signal use) hits Err(_) => break in the output stream and silently ends it early, or fails an in-flight stdin write — the prior tokio::fs::File path retried EINTR automatically via std's Read/Write impls.
| fn read_fd(fd: &OwnedFd, buffer: &mut [u8]) -> io::Result<usize> { | |
| nix::unistd::read(fd.as_raw_fd(), buffer).map_err(Into::into) | |
| } | |
| fn write_fd(fd: &OwnedFd, buffer: &[u8]) -> io::Result<usize> { | |
| nix::unistd::write(fd, buffer).map_err(Into::into) | |
| } | |
| fn read_fd(fd: &OwnedFd, buffer: &mut [u8]) -> io::Result<usize> { | |
| loop { | |
| match nix::unistd::read(fd.as_raw_fd(), buffer) { | |
| Err(nix::errno::Errno::EINTR) => continue, | |
| result => return result.map_err(Into::into), | |
| } | |
| } | |
| } | |
| fn write_fd(fd: &OwnedFd, buffer: &[u8]) -> io::Result<usize> { | |
| loop { | |
| match nix::unistd::write(fd, buffer) { | |
| Err(nix::errno::Errno::EINTR) => continue, | |
| result => return result.map_err(Into::into), | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Fixed in dc31d49. read_fd/write_fd now retry EINTR. A real output read error is propagated through OutputManager, ExecutionState, and Attach as gRPC INTERNAL instead of being converted into a successful EOF. PTY-master EIO remains the one explicit EOF case.
DorianZheng
left a comment
DorianZheng
left a comment
There was a problem hiding this comment.
the earlier compile break — fallible ExecHandle::new against the infallible #[cfg(test)] callers — is resolved on this push (.expect(...) at the four sites), so that's cleared.
output buffering is the piece worth a deliberate decision. draining at exec-creation into a bounded ring is right for the no-consumer case, but eviction is unconditional (inline at output.rs:221), so a consumer that is attached and reading, yet slower than a >1 MiB burst, silently loses the evicted bytes — the downstream mpsc(100) backpressures replay, not the fd drain, so the producer is never slowed. previously a full bounded channel blocked the reader, and an attached consumer couldn't lose data. the call: backpressure while a consumer is attached and caught up, or accept a lossy attach with a reliable gap notice.
the same evicting stream feeds ssh: spawn_output_pump (src/guest/src/service/ssh/bridge.rs:597, outside this diff) forwards stdout/stderr raw with no offset/gap handling, so an interactive session faster than its client loses bytes with no notice at all.
remaining points are inline. cleanup only, no behavior change: the drain_tasks StdMutex<Vec<JoinHandle>> (output.rs:20), the near-duplicate spawn_stdout/spawn_stderr (:176/:187, drain is already generic), and the dead stream.enabled = true in push (:215, a drain only exists for an already-enabled stream).
| stream.last_sequence = Some(sequence); | ||
| let output = data_output(source, data, offset); | ||
|
|
||
| while state.buffered_bytes + byte_len > BUFFER_CAPACITY_BYTES { |
There was a problem hiding this comment.
eviction here is unconditional — it never checks whether a consumer is attached. an attached, actively-reading consumer that falls behind a >1 MiB burst loses the evicted bytes: replay skips the hole via next_sequence = oldest_sequence (:132). the mpsc(100) in ExecutionState::attach sits downstream of replay (ring -> grpc) and backpressures only that, so the fd -> ring drain, and thus the producing process, is never slowed.
There was a problem hiding this comment.
Fixed in dc31d49. The 1 MiB ring remains a deliberate bounded-retention policy, so loss is allowed but no longer silent: every gRPC stdout/stderr frame carries that stream absolute offset, and its empty terminal frame carries final total_bytes. Consumers detect either a mid-stream offset jump or a tail gap at EOF. SSH tracks the same counters and emits the exact per-stream marker on the affected SSH output stream.
| match reader.async_io(Interest::READABLE, |fd| read_fd(fd, &mut buf)).await { | ||
| Ok(0) => break, // EOF | ||
| Ok(n) => yield buf[..n].to_vec(), | ||
| Err(_) => break, |
There was a problem hiding this comment.
any non-WouldBlock read error breaks the loop and is indistinguishable from clean eof. the end-frame then reports the truncated length as the total, so the host sees no offset gap and treats a truncated stream as complete. (the stdin write_all above turns a transient error into a permanent forward failure the same way.)
There was a problem hiding this comment.
Fixed in dc31d49. Non-PTY read failures now become gRPC INTERNAL through Attach, so no terminal total_bytes can certify a truncated stream. The PTY EIO-on-slave-close case is handled separately as its documented EOF condition.
| if let Some(lost_bytes) = | ||
| stdout.receive(chunk.offset, chunk.data, chunk.total_bytes) | ||
| { | ||
| Self::report_gap(stderr, "stdout", lost_bytes); |
There was a problem hiding this comment.
a stdout gap is reported on the stderr channel here (report_gap(stderr, "stdout", ...)). a consumer capturing stdout alone loses data with no signal, and a consumer parsing stderr gets [boxlite] ... spliced into real stderr. the stderr-gap branch just below is correct; only the stdout branch is cross-wired.
There was a problem hiding this comment.
Fixed in dc31d49. Host gap markers now go to the affected decoded stream, so stdout-only consumers observe stdout loss and intact stderr is not modified. The same rule is applied to the in-process SSH bridge.
| "Guest output buffer dropped older output" | ||
| ); | ||
| let _ = stderr.stream.tx.send(format!( | ||
| "[boxlite] {source} output dropped {lost_bytes} bytes\n" |
There was a problem hiding this comment.
trailing bare \n: in raw/pty mode (opost cleared) this advances a row without a carriage return — staircase + column desync in a tui. \r\n, or routing the notice through the opost-aware path, avoids it.
There was a problem hiding this comment.
Fixed in dc31d49. Gap markers now use CRLF in both the Host and SSH output paths, so raw PTY consumers do not advance without returning to column zero.
| self.stream.flush(); | ||
| } | ||
| self.expected_offset = offset + data.len() as u64; | ||
| self.stream.send_bytes(data); |
There was a problem hiding this comment.
send_bytes(data) forwards the surviving post-gap bytes before the caller runs report_gap, so the "dropped N bytes" marker trails the recovered output and reads as a mid-stream loss rather than one at the gap point.
There was a problem hiding this comment.
Fixed in dc31d49. OutputTracker now separates validation from delivery: it flushes the affected decoder, emits the gap marker, then forwards surviving bytes. Markers use CRLF; the SSH bridge follows the same ordering and line ending.
| if state.attached { | ||
| return Err(Status::already_exists("Already attached")); | ||
| } | ||
| state.attached = true; |
There was a problem hiding this comment.
attached is set once and never cleared, so if the single attach stream drops (a transient disconnect) it can't reconnect — attach returns AlreadyAttached for the rest of the execution — even though the ring still holds recent output and the new offset/total_bytes fields are exactly what resume-from-offset needs. as written those wire fields aren't used for reconnect, so either they're dead weight or this latch blocks the feature they're for.
There was a problem hiding this comment.
Intentionally deferred. #1022 keeps Attach as one non-resumable consumer, so clearing the latch alone would permit a second stream without defining the resume cursor, replay start point, duplicate policy, or terminal retention. The new per-stream offset/total fields make that follow-up possible, but resumable Attach needs a request cursor plus tests for disconnect, replay, expiry, and concurrent reconnect; it should land as a separate protocol design.
| pub async fn attach( | ||
| &self, | ||
| exec_id: &str, | ||
| _exec_id: &str, |
There was a problem hiding this comment.
_exec_id is unused and the per-stream info!(... forwarding task ended) logs that existed before are gone, so the attach path no longer carries the execution id or logs task lifecycle — harder to correlate a stuck or silently-dropping attach against the wait/exit records.
There was a problem hiding this comment.
Fixed in dc31d49. ExecutionState::attach now retains exec_id and logs execution output forwarding ended; OutputManager also logs an output-reader failure with its stream. The duplicate stdout/stderr drain spawners were consolidated.
| .entries | ||
| .get(index) | ||
| .expect("ring sequence must exist") | ||
| .output |
There was a problem hiding this comment.
two things on this replay step:
- it deep-clones the
ExecOutput(itsVec<u8>) while holding the mutex, andpushkeeps buffering into the ring after a consumer attaches, so a live stream is copied twice for its whole life.Arc<[u8]>/Bytespayloads would make this a refcount bump. - the
Next::Waitexit at :165 (updates.changed().await.is_err()) is unreachable: this closure holds amanagerclone, hence awatch::Sender, sochanged()never errors.Next::Doneis the only way out, and it needs a drain to finish naturally; if drains are aborted (shutdown) without the attach consumer also being torn down, this stream hangs. today the release path aborts the consumer too, so it's latent.
There was a problem hiding this comment.
Partially fixed in dc31d49: the unreachable watch::Receiver error branch is removed; the manager owner makes sender closure impossible. I am not changing the Vec clone here: avoiding it needs a Bytes/Arc payload change across the generated protobuf and all Attach consumers, which is a separate wire/API change rather than a local cleanup.
971fe72 to
dc31d49
Compare
| if let Some(failure) = &state.failure { | ||
| Next::Error(Status::internal(failure.clone())) | ||
| } else { | ||
| if next_sequence < state.oldest_sequence { | ||
| next_sequence = state.oldest_sequence; | ||
| } | ||
| if state.stdout.ready_to_end(next_sequence) && !stdout_end_sent { | ||
| stdout_end_sent = true; | ||
| Next::Item(end_output(OutputSource::Stdout, state.stdout.total_bytes)) | ||
| } else if state.stderr.ready_to_end(next_sequence) && !stderr_end_sent { | ||
| stderr_end_sent = true; | ||
| Next::Item(end_output(OutputSource::Stderr, state.stderr.total_bytes)) | ||
| } else if next_sequence < state.next_sequence { | ||
| let index = (next_sequence - state.oldest_sequence) as usize; | ||
| let output = state | ||
| .entries | ||
| .get(index) | ||
| .expect("ring sequence must exist") | ||
| .output | ||
| .clone(); | ||
| next_sequence += 1; | ||
| Next::Item(output) |
There was a problem hiding this comment.
in the attach stream loop, if let Some(failure) = &state.failure is checked before the buffered-entries branch, so once either reader hits an I/O error the attach stream immediately yields Err and ends, even if correctly-captured output from the other (or same) stream is still sitting unread in entries; that data is never delivered to the client.
There was a problem hiding this comment.
Fixed in b5d053e. ReaderFailure now records the next internal sequence at failure time. Attach replays every retained entry before that boundary, then yields INTERNAL; output captured after the failure is intentionally not presented as a complete stream. The regression test asserts buffered stdout is delivered before the error.
DorianZheng
added
the
e2e-local
|
https://github.com/boxlite-ai/boxlite/actions/runs/30514470192/job/90781243192?pr=1022 failed, please take a look and fix it before merge |
DorianZheng
added
e2e-local
## Summary Drain each guest execution stdout and stderr as soon as it is created. This removes the dependency on a host Attach consumer for the guest process to make progress. Guest stdio uses the Tokio I/O reactor rather than tokio fs. Idle pipes no longer occupy blocking-pool workers. Stdin uses the same reactor-backed path because PTY stdin and stdout descriptors share the non-blocking setting. The guest retains at most 1 MiB of output for a late Attach. When that ring overwrites old bytes, Attach emits an OutputDropped control event with separate stdout and stderr byte counts. The host renders the notice and resets only the affected UTF-8 decoder. ## Scope This PR intentionally contains only guest exec stdio, the shared gRPC protocol, and the local gRPC consumer needed to render it. It does not change REST wait behavior, the runner, API, CLI, or host output queueing. ## Verification - make test:integration:rust FILTER=main_command_exits_after_large_output_without_attach - make test:integration:rust FILTER=late_attach_reports_output_dropped - make test:integration:rust FILTER=test_zygote_concurrent_stdin_pipes - make clippy The integration commands ran outside the sandbox against real macOS Hypervisor VMs. make fmt:check is currently blocked before Rust checking by 186 pre-existing Prettier violations in untouched generated apps client files; make fmt:check:rust passed. --------- Co-authored-by: BatmanByte <300328404+BatmanByte@users.noreply.github.com>
2 participants
Summary
Drain each guest execution stdout and stderr as soon as it is created. This removes the dependency on a host Attach consumer for the guest process to make progress.
Guest stdio uses the Tokio I/O reactor rather than tokio fs. Idle pipes no longer occupy blocking-pool workers. Stdin uses the same reactor-backed path because PTY stdin and stdout descriptors share the non-blocking setting.
The guest retains at most 1 MiB of output for a late Attach. When that ring overwrites old bytes, Attach emits an OutputDropped control event with separate stdout and stderr byte counts. The host renders the notice and resets only the affected UTF-8 decoder.
Scope
This PR intentionally contains only guest exec stdio, the shared gRPC protocol, and the local gRPC consumer needed to render it. It does not change REST wait behavior, the runner, API, CLI, or host output queueing.
Verification
The integration commands ran outside the sandbox against real macOS Hypervisor VMs.
make fmt:check is currently blocked before Rust checking by 186 pre-existing Prettier violations in untouched generated apps client files; make fmt:check:rust passed.