fix(run): bound child output capture and reaper threads - #340
Conversation
The shell-out helpers redirected child stdout/stderr to tempfiles but then read both streams fully into heap Vecs, so peak RSS scaled 1:1 with child output volume and summed across concurrent calls (measured: 128 MiB child output -> +128 MiB RSS; 8 concurrent captures sum). The doc comments claimed bounded capture; nothing bounded it. - Diagnostics capture is now capped at 256 KiB per stream, tail-preserving (the last bytes are what error messages need), with a diagnostic line on truncation. - `pty list --json` moves to an explicit full-stdout variant: parsed structured output must be whole; stdout is intentionally uncapped there. - One shared reaper thread replaces detached thread-per-timed-out-child, which accumulated without bound under timeout storms. - Eval run steps stream child output straight into their capture files and combined logs instead of buffering whole outputs; agent log dumps stream scrollback to file without materializing it; the bash judge uses null stdio instead of undrained pipes that could deadlock on chatty judges. Decision record: docs/vrs/.decisions/0007. agent-identity: unknown agent-persona: generalist agent-supervisor: unavailable agent-tool: OMP agent-tool-version: 18.0.3 agent-runtime: OMP 18.0.3 tooling-profile: dotfiles@ffce621
Decision 0007 landed with the fix, but the requirement and spec did not state the bound. R32's reaper clause already covered teardown ownership; R34 now requires capture memory to be cap-bounded, tail-preserving, and observable, with unbounded reads opt-in at the call site. The supervision spec gains a child-process execution section pointing at D0007. agent-identity: unknown agent-persona: generalist agent-supervisor: unavailable agent-tool: OMP agent-tool-version: 18.0.3 agent-runtime: OMP 18.0.3 tooling-profile: dotfiles@ffce621
|
End-to-end verification against the real supervisor loop (not just unit tests), pre-fix vs post-fix binaries built from Setup: empty catalog,
On this branch each pass emits the truncation diagnostic ( The uncapped path was also exercised end-to-end by accident in the first run: noise on stdout flows through the intentionally uncapped Posted on behalf of @schickling
|
![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: 1d5bee6e8f
ℹ️ 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".
| cmd.stdout(Stdio::from(out_file)) | ||
| .stderr(Stdio::from(err_file)); | ||
| Ok(cmd.status()?.code().unwrap_or(-1)) |
There was a problem hiding this comment.
When st2 eval is launched from an interactive terminal and a run step reads stdin, replacing Command::output() with Command::status() makes the shell inherit st2's stdin instead of receiving immediate EOF. Such a step can now consume the operator's input or hang the evaluation indefinitely, since run steps have no timeout. Set stdin explicitly to Stdio::null() while retaining the new file-backed stdout/stderr capture.
Useful? React with 👍 / 👎.
2 participants
Closes #339
Problem
The supervisor's shell-out helpers (
src/run.rsoutput_with_input_timeout_observed,src/ding/mod.rsoutput_with_timeout) redirect child stdout/stderr to tempfiles — sound, since files keep an escaped descendant from blocking cleanup and make bounded read-back deadlock-free — then unconditionally rewind both streams andread_to_endthem into fresh heap buffers. Peak supervisor RSS scaled 1:1 with child output volume and summed across concurrent calls, despite doc comments claiming "bounded output capture". Nothing bounded it; no spec or invariant ever defined a bound. The same audit surfaced sibling hazards: detached reaper threads accumulated without bound under timeout storms, eval run steps buffered whole child outputs (and fullpty peek --fullscrollback) in memory before writing log files, and the bash judge piped stdout/stderr it never drained — a deadlock on chatty judges.Measured on Linux x86_64 with a standalone copy of the exact mechanism (
dd-produced child output, per-process VmHWM):The control shows the tempfile design already supports bounded capture; only the unconditional materialization was at fault.
Goal
Supervisor peak RSS no longer scales with child output volume; worst case is bounded by concurrent calls × 2 × cap regardless of child behavior, with truncation observable and the one genuinely unbounded consumer made explicit.
Decisions
pty list --jsonparses structured output that must be whole, so it moves to a distinctly namedoutput_full_stdout_with_timeoutwhose doc comment states stdout is intentionally uncapped and why. Bounded-tail remains the default so an uncapped read is always a visible choice at the call site.reap_detached), replacing per-timeout detached threads in both helpers.Stdio::from(File)+std::io::copy), preserving byte-identical.runs/<id>.{out,err,exit}and combined-log semantics including retry-overwrites-with-final-attempt; the bash judge switches undrained pipes to null stdio (only its exit status was ever consumed).pty list --jsonneeds whole stdout;pty peekconsumes terminal screens (far below the cap); all other sites use stderr only trimmed inside error strings.Decision record:
docs/vrs/.decisions/0007-child-output-capture-is-bounded-and-tail-preserving.md.Verification
.out,.err,.exit, and the combined log with correct ordering.cargo test --offline --lib: 467 passed, 0 failed).st2: truncated stdout capture of \./flood`: keeping last 262144 of 1048584 bytes (cap 262144)`.Complexity
No new dependencies or module boundaries. Adds one shared reaper thread and two small helpers (
read_bounded_tail,output_full_stdout_with_timeout) at the lowest common point; ding's diverged duplicate now reuses them instead of carrying a third copy of read-back logic.Concerns
persistent_advisory_warnings_surface_once_not_per_pass, asserts ≥3 reconcile passes within 300 ms) flaked once under full-suite parallel load during development; it uses a fake runner outside this change's path and passed in isolation, on rerun, and in CI-style runs.Follow-ups
src/opencode_session.rscarries pre-existing unused-variable warnings in its test fixture (untouched here).References
Posted on behalf of @schickling
agent_identityagent_personaagent_supervisoragent_toolagent_tool_versionagent_runtimetooling_profile