fix(hooks): let a hook that ignores its stdin succeed - #6
Merged
Merged
Conversation
A hook that exits without reading its stdin was reported as failed. The state is written with write_all and the error propagated, so when the child closed the read end first the write returned EPIPE and the hook was refused even though it had exited 0. That is a race against the pipe buffer. A small payload usually lands before the child goes away and the hook passes; a large one, or a busy machine, does not. It surfaced as a_hook_that_succeeds_is_not_an_error failing once in CI on a change that touched no Rust at all, and passing on a re-run with nothing altered. The runtime-spec writes the state to the hook's stdin but makes the exit code the verdict. A hook is entitled to ignore what it is handed, and most real ones do - they run a command and never read anything. So a broken pipe on that write is not a failure, and the exit status decides as it always did. Any other write error still fails, and now kills the child rather than leaving it behind. The new test does not depend on the race: it hands /bin/true a megabyte, which cannot fit in the pipe buffer, so the write always outlives the child. It fails deterministically without this change.
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.
The bug
one()writes the container state to the hook's stdin and propagates the error:A hook that exits without reading closes the read end first, so the write returns
EPIPEand the hook is reported as failed — even though it exited 0.Why it is a race, not a constant failure
The payload usually fits in the pipe buffer, so the write completes before the
child is reaped and nothing looks wrong. A larger payload, or a loaded machine,
loses that race.
It showed up as
a_hook_that_succeeds_is_not_an_errorfailing in CI on#5, a change that
touched no Rust at all, and then passing on a re-run with nothing altered. It
passed 20 out of 20 runs locally, which is consistent with the diagnosis rather
than against it.
Why the current behaviour is wrong
The runtime-spec writes the state to the hook's stdin but makes the exit code
the verdict. A hook may ignore what it is handed, and most real ones do: they run
a command and never read anything.
marsrefuses those hooks non-deterministically,more often on the busy machines where hooks matter most.
The change
BrokenPipeon that write is no longer fatal; the exit status decides, as italready did. Every other write error still fails, and now kills the child instead
of leaving it running.
The test
a_hook_that_never_reads_its_stdin_is_not_an_errorhands/bin/truea megabyte,which cannot fit in the pipe buffer, so the write always outlives the child. It
does not depend on timing, and it fails deterministically without this change:
cargo fmt --check,cargo clippy --all-targets -- -D warningsandcargo test --lib(107 passed) are green.