fix(sandbox): drain the capture pipes while the child runs (#149)#166
Open
dzerik wants to merge 1 commit into
Open
fix(sandbox): drain the capture pipes while the child runs (#149)#166dzerik wants to merge 1 commit into
dzerik wants to merge 1 commit into
Conversation
…el#149) `wait()` read stdout/stderr only after the child had exited. A child that writes more than one pipe buffer — 64 KiB by default — blocks in `write()` once the pipe fills, so it can never reach the exit `wait()` is waiting for. The run hangs until the caller's timeout, forever if there is none, and the output is lost. The drains now start before the exit wait and are joined after it, so the pipe keeps moving and the child can finish. Reads still end at EOF. Two properties the drains have to keep, both easy to lose here: They are owned by the `Sandbox`, not by the `wait()` future. Cancelling a `wait()` — a `timeout` or `select!` around it, then `kill()` and wait again — is an in-tree idiom; if the first poll moved the read ends into readers that future owned, dropping it would take the output with them and every later `wait()` would report nothing at all. `Drop for Sandbox` aborts them. They are ordinary tasks, not `spawn_blocking`. A blocking read would hold two threads of the shared blocking pool for the child's whole lifetime, even for a child that writes nothing — and the COW copier and the fork-tracking worker need that pool *while* a child is alive, so a saturated pool deadlocks workloads instead of merely slowing them down. `Receiver::from_owned_fd` sets O_NONBLOCK on the supervisor's read end only; the child writes to the write end, a separate open file description, so its writes stay blocking and the pipe still applies back-pressure. Scope: this covers every path that captures through `wait()` — `run`, `run_interactive`, `run_with_handlers`, `dry_run`, `Process::wait`. It does not cover `Pipeline::run`/`Gather::run`, which read their own capture pipes after the stage-wait loop (pipeline.rs), or `fork`/`reduce`, which reads the per-clone pipe before the reducer drains it (sandbox.rs). Those have the same defect and are unchanged here. A stream the caller took with `Process::take_stdout`/`take_stderr` is `None` here and remains theirs to drain, unchanged. Tests: three integration cases over the buffer — stdout, stderr, and both at once; the combined one is what a *sequential* drain still deadlocks on, since stdout only reaches EOF when the child exits and the child is blocked writing stderr. Plus one case that cancels a `wait()` and waits again (fails if the drains live in the future), and one that runs a second sandbox while a first, silent child is alive on a runtime with two blocking threads (fails if the drains use the pool). Each bounds itself with a timeout so a regression fails the test instead of hanging the suite.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #149.
wait()read the capture pipes only after awaiting child exit. A child that writes more than one pipe buffer — 64 KiB by default — blocks inwrite()once the pipe fills, so it never reaches the exitwait()is waiting for: the run hangs until the caller's timeout, forever if there is none, and the output is lost. On current main, 64 KiB returns in 17 ms and 1 MiB never returns.The drains now start before the exit wait and are joined after it. They are ordinary async tasks (
tokio::net::unix::pipe::Receiver), so no blocking-pool threads and no new OS threads are used — which also keepsrun()usable where thread creation is refused (#47).O_NONBLOCKis set on the supervisor's read end only; the child writes the write end, a separate open file description, so its writes stay blocking and back-pressure is preserved.The handles are parked on the
Sandboxrather than on thewait()future, so cancellingwait()no longer loses the capture: a laterwait()joins the drains and returns the complete output. Before this change a retry returned only what had fit in one pipe buffer, and theRuntimeState::Stoppedearly return reported nothing at all.Dropaborts the drains.Streams taken through
Process::take_stdout/take_stderrare unaffected and remain the caller's to drain.Scope
Covers every path that captures through
wait():run,run_interactive,run_with_handlers,dry_run,Process::wait.Not covered:
Pipeline::run/Gather::run, which read their own pipes after the stage-wait loop, andfork/reduce, which blocks insandbox_read_exactbefore the reducer drains the clone pipes. Same defect, deliberately unchanged here — happy to follow up separately if you want it in one go.Properties worth stating
EOF liveness is unchanged. The drains end at EOF, which requires every holder of the write end to close it, so a descendant that inherited fd 1/2 and outlives the child still keeps
wait()blocked. That is exactly the previous behaviour — the old post-exitread_to_endblocked on the same condition, inline and uncancellable.Memory trade. Capture is now bounded by the workload rather than by the pipe:
wait()holds the whole stream in memory, where the pipe buffer previously stopped it at 64 KiB by hanging. Callers running output-unbounded workloads should take the stream and cap it themselves. Acapture_limitbuilder option would be a reasonable follow-up; note that a capped reader must still drain to EOF or it reintroduces this issue.Tests
Three over-buffer cases — stdout, stderr, and both at once. The combined one is the discriminating case: a sequential drain still deadlocks on it, because stdout only reaches EOF when the child exits and the child is blocked writing stderr.
Two more for the properties above: cancel-then-retry must still return the capture, and a runtime with
max_blocking_threads(2)must not deadlock a second capture-mode run while a zero-output sandbox is alive.Each fails without its part of the change, and all are timeout-bounded so a regression fails the test instead of hanging CI.