Repository navigation
The capture drain treats an interrupted read as end-of-pipe #355
Description
Question
ProcessRunner's drain loop treats a read error as end-of-pipe. At rust/devlaunch-runner/src/lib.rs:816-826 (as of PR #343), Ok(0) | Err(_) => break collapses every error into EOF, where the read_to_end it replaced retried on ErrorKind::Interrupted.
The review of PR #343 proved it with a reader yielding "ab", one Interrupted, then "cd": the old code returns "abcd", the new code returns "ab". The result is silent truncation reported as a successful Ran — precisely the harm that PR's design notes cite as the reason not to have a bounded join return String::new().
Unreachable today, and it is worth writing down why, because that is what makes this low priority rather than urgent: every signal handler in the workspace is installed through libc::signal, which on glibc carries BSD semantics with SA_RESTART, so a slow read is restarted by the kernel rather than failing with EINTR. Anything that installs a handler with sigaction and omits SA_RESTART makes this live — so the fix is cheap insurance against a change nobody would connect to this file.
The fix is one match arm: retry on ErrorKind::Interrupted, break on other errors. Red first, with the injected-reader shape the reviewer used — a test that only exercises real pipes cannot reach this.
While there, two nits from the same review:
capturepaysDRAIN_GRACEtwice serially (measured 1.01 s with both pipes held) where the doc comment reads as a single bound. Either make it one shared deadline across both pipes or correct the doc.session's reader is an abandon rather than a bound, which is correct as written — no change wanted, listed so it is not mistaken for an oversight.
Activity
blooop commentedon Aug 29, 2026
Claimed 2026-08-29: build in progress from an interactive session (opus agent in an isolated worktree, managed as part of closing out map #299). PR to follow. If you are another session seeing this, do not double-build.
blooop commentedon Aug 29, 2026
Correcting my claim above: no PR is coming, because there is nothing left to build. The fix landed inside PR #343 itself, after the review that raised this. Commit 24e4696 ("fix: an interrupted read is a retry, not the end of the pipe", 2026-08-23) is an ancestor of
main, and the ticket was never closed behind it.What is on
maintoday, atrust/devlaunch-runner/src/lib.rs:909:The test this ticket asked for is there too, with exactly the injected-reader shape the reviewer used:
Interrupting(devlaunch-runner/src/tests.rs:677) yields its pieces in order raisingEINTRbetween them, anda_drain_treats_an_interrupted_read_as_a_retry_not_an_endingassertscollect(drain(..)) == "abcdef".I checked it guards rather than merely passes. Disabling the retry arm and rerunning:
That is the reviewer's
"ab"vs"abcd"reproduction, one piece longer. Arm restored, tree clean.On the two nits:
session's reader (lib.rs:588) still readsOk(0) | Err(_) => break, and that is fine on both counts. It is the abandon-not-a-bound the ticket said needed no change, and itsErr(_)cannot beEINTRanyway:BufRead::read_untilretriesErrorKind::Interruptedinside std, so the error never surfaces here.DRAIN_GRACEpaid twice is unaddressed as behaviour.capturestill callscollect(stdout)thencollect(stderr)serially (lib.rs:497), so two held pipes cost up to 1.0 s. The doc half of that nit reads accurate as written though:DRAIN_GRACEsays "how longcollectgives a drained pipe", which is per-pipe and true. So what is left is a behaviour question (one shared deadline across both pipes) rather than a wrong comment, and it is worth its own ticket if anyone wants it, not a reopen of this one.blooop commentedon Aug 29, 2026
Closing as fixed by PR #343: commit 24e4696 (an ancestor of main) made the drain retry on ErrorKind::Interrupted rather than treating it as end-of-pipe, and the injected-reader test this ticket asked for exists (rust/devlaunch-runner/src/tests.rs, a_drain_treats_an_interrupted_read_as_a_retry_not_an_ending) and was verified non-vacuous by mutation today: disabling the retry arm reproduces the truncation ("ab" vs "abcdef"). Evidence in the comment above. The one surviving remnant, capture paying DRAIN_GRACE once per pipe serially, is split out to its own issue.