Repository navigation
fix: wake the command runner when output arrives instead of after a fixed nap - #285
Conversation
…ixed nap (#281) On macos-latest the scoped-parent 4 MiB + 1 overflow sometimes hit its 3 s deadline as TimedOut. Each time the reader drained the pipe faster than the writer refilled it, collect slept a fixed 25 ms, and on a busy 3-vCPU runner those naps and their wake-ups added up. On Unix an idle turn now polls the open pipes for up to the same 25 ms and returns as soon as one has output or closes. Windows, whose anonymous pipes can't be polled, keeps the nap. The timeout, the one chunk per stream per turn and the output limits are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
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". |
llvm-cov counts a generic function's branches per instantiation, so the runner's instantiation of wait() left the no-open-pipe nap uncovered and lowered branch coverage. The branches now live in poll_or_nap, which the unit test covers fully. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
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". |
complete()'s stdout-closed, stderr-open case was covered only when a real command's EOFs happened to arrive in separate turns; with the poll wait they arrive together. A unit test now covers each outcome directly, so branch coverage no longer depends on timing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
With the poll wait both streams close in the same turn, so the already-closed branch was no longer reached by the fixtures on Linux. A unit test now covers pending output, data, overflow, EOF and a closed stream directly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
On
macos-latest,command_runner'sscoped-parentfixture sometimes returnedErr(TimedOut)instead ofErr(Failed)for its 4 MiB + 1 overflow within 3 s. The cause was the runner's fixed 25 ms nap whenever a pipe was momentarily empty. On Unix an idle turn nowpolls the open pipes for up to the same 25 ms and wakes as soon as output arrives or a pipe closes. The timeout, the one 8 KiB chunk per stream per turn and the output limits are unchanged.Fixes #281.
Root cause, measured on
macos-latest(3 vCPU)Throwaway branch
probe/command-runner-281, run 40540947352, main's runner with tracing added: thescoped-parentoverflow run 30 times on its own, then the wholecommand_runnertest 20 times.left: Err(TimedOut),right: Err(Failed).The issue's guess was right: the time went to naps, not to reading.
Change
command::pipe::wait(new): on Unix,pollon the open stdout/stderr pipes with the remaining nap as its timeout, rounded up to a whole millisecond. With both pipes closed, or on Windows, whose anonymous pipes can't be polled, it sleeps as before.collectcalls it instead ofthread::sleep, passing only the pipes that are still open. Its complexity is unchanged.poll_or_nap, so its branches are counted once however many pipe types callwait.docs/COMMAND_RUNNER.mddescribes the idle wait.The strict metrics gate first failed this PR on branch coverage. On main,
complete()'s "stdout closed, stderr still open" branch was covered only when a real command's two EOFs happened to land in separate turns, which needed a nap between them. With the poll wait both EOFs arrive in the same turn. Linux CI showed the same effect inCapture::drain. Its "stream already closed" branch was reached only when one stream closed a turn before the other. Unit tests now cover every outcome ofcomplete()andCapture::draindirectly, so the coverage no longer depends on timing. With the drain test, CI's own numbers put branch coverage at 2,969/3,714 (79.94%), against base 2,967/3,712 (79.93%).Evidence
Throwaway branch
probe/command-runner-281-fix, run 40543239624: this change with the same tracing, onmacos-latest.command_runnertestIdle turns with this PR ranged from 1 to 62 per run, but each now ends as soon as output arrives, so they no longer add up.
Tests
command::pipe::tests::an_idle_wait_ends_when_output_arrives_not_after_its_timeout(Unix): a wait with no output lasts its timeout; output written 20 ms into a 10 s wait ends it early; readable data or a closed pipe ends it at once; with no open pipe it naps.command::tests::a_drain_reports_pending_output_eof_a_closed_stream_and_overflow(Unix): nothing yet, data, overflow past the limit, EOF, then a closed stream that is never read again.command::tests::collection_ends_at_the_stop_marker_or_once_both_streams_closed_and_the_command_exited(Unix): the stop marker ends collection, an open stream keeps it going, and both closed plus an exited command end it.command_runnerfixtures are unchanged, including the overflow, hang, descendant and Windows no-console cases from fix: keep the Antigravity CLI's self-updater from opening console windows #280.Validation
Local gates on air (macOS arm64) at
1e7ca45, on main91dbf3b:cargo fmt --all --check,cargo clippy --workspace --all-targets --locked -- -D warningsandcargo test --workspace --lockedall pass (1,051 tests, 0 failed). On air the strict metrics gate (scripts/rust-metrics.py ci --base 91dbf3b) passed at4fed893, with no CRAP increase. Linux CI then also flaggedCapture::drain, which1e7ca45covers. CI runs the same gates on Windows, macOS and Linux, plus strict rust-metrics.🤖 Generated with Claude Code