Stop container system start from leaking into pall8t's stdout - #27
Merged
Conversation
container::system_start() ran `container system start` via Command::status() with no stdio configured, so the child inherited pall8t's real stdout. ensure_container_system() calls it whenever the service is stopped, and cmd_ls calls ensure_container_system() before printing its --json output — so a cold start (e.g. right after a reboot) could interleave the child's stdout ahead of the JSON, corrupting anything parsing it (the herdr integration in particular). Intermittent by nature: only triggers when the service happens to be stopped, so it "works" until it doesn't. Fixed by routing system_start through the run_streaming plumbing added for #13's build-output fix (container's stdout redirected to pall8t's own stderr, stderr inherited). util::run_streaming now takes an explicit `stdin: Stdio` parameter instead of hardcoding Stdio::null() — that default was right for `container build` (no business reading stdin) but wrong for `container system start`, which can prompt interactively on a fresh machine (e.g. a default-kernel- install confirmation); closing its stdin unconditionally would have turned that prompt into an unanswerable EOF. system_start now inherits stdin only when its own stdin is a TTY, and closes it otherwise, so a piped invocation (`echo x | pall8t run`) can't have its payload consumed by an unrelated prompt. Audited every other subprocess spawn in the crate for the same shape (inherited stdout on a path that later prints machine-readable output): everything else either captures via Command::output() or is main.rs's exec_container, which replaces the pall8t process outright for TTY passthrough and prints nothing afterward in-process. No other call sites needed changing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Summary
Fixes the follow-up gap flagged in #26's description:
container::system_start()rancontainer system startviaCommand::status()with no stdio configured, so the child inherited pall8t's real stdout.ensure_container_system()calls it whenever the container system service is stopped, andcmd_lscallsensure_container_system()before printing its--jsonoutput — so a cold start (e.g. right after a reboot) could interleave the child's stdout ahead of the JSON, corrupting anything parsing it (the herdr integration in particular). This is intermittent by nature: it only triggers when the service happens to be stopped, so it "works" most of the time and fails unpredictably.container::system_startnow goes through therun_streamingplumbing added for pall8t buildのログが見たい #13's build-output fix (child stdout redirected to pall8t's own stderr, child stderr inherited).util::run_streamingnow takes an explicitstdin: Stdioparameter instead of hardcodingStdio::null(). That default was right forcontainer build(no business reading stdin) but wrong forcontainer system start, which can prompt interactively on a fresh machine (e.g. apple/container's default-kernel-install confirmation) — closing its stdin unconditionally would turn that prompt into an unanswerable EOF.system_startnow inherits stdin only when its own stdin is a TTY, and closes it otherwise, so a piped invocation (echo x | pall8t run) can't have its payload silently consumed by an unrelated prompt.container::build_image(the otherrun_streamingcaller) now explicitly passesStdio::null(), preserving its original behavior.Audit (as requested)
Grepped every subprocess spawn in the crate for the same shape (inherited stdout on a path that later prints machine-readable output):
util.rs,home.rs(x2),herdr.rs,container.rs. Everything else usesCommand::output(), which always captures rather than inherits, or ismain.rs'sexec_container, which uses.exec()to replace the pall8t process outright for TTY passthrough duringrun/execand prints nothing machine-readable afterward in the same process (on success it never returns; on failure the error goes to stderr only). No other call sites needed changing.Test plan
cargo fmt --checkcargo clippy --all-targets -- -D warningscargo test— 128 tests pass, including a new one proving thestdinparameter is actually threaded through to the child (viash -c "read x"'s exit code as a proxy, since streamed output itself can't be captured in-process)/code-review(self) and two rounds of/skeptical-review(adversarial agent) — the second pass caught a real design gap in my first draft (unconditionalStdio::inherit()forsystem_startcould leak piped stdin content into an unrelated prompt) and a self-contradictory doc comment; both fixed before this commitKnown minor edge case, not fixed
system_start(likebuild_imagebefore it) now hard-fails if pall8t's own stderr (fd 2) is closed, sincerun_streamingneeds todup()it — e.g.pall8t ls --json 2>&-on a cold system now errors instead of starting the service. Pathological (deliberately closing fd 2), and already true forbuild_imagesince #26; this PR just extends the same characteristic tols/run/stop's startup path. Flagging rather than fixing — didn't want to add scope for an edge case this narrow.