Fix truncated output when a command exits - #104
Open
dcalvo wants to merge 1 commit into
Open
Conversation
process_stdio() only forwards as much as the flow control window allows, and child_wait_loop() stops polling the output fds once that window is empty. Reaping the child returns immediately, so whatever is still in the pipes is dropped, which is why the loss comes in whole windows. Nothing surfaces this: cmd/3 returns a short binary next to a successful exit status. A command writing 66890 bytes lost output in 40 of 40 runs here, keeping as little as 10240 of them. Everything the child wrote is in the pipe by the time it exits, so drain before returning. A zero-timeout poll recognizes an already-empty pipe for the cost of one syscall, and the window is refilled by waiting for acks the way the exit path already does.
Owner
|
Thanks for sending his up. I really appreciate the time you're putting into making this use case work. The CI failure is unrelated. The code update looks good on quick skim. My plan is to put in some time on muontrap next week and I'll merge your PR then with that pass. |
Contributor
Author
|
Thank you for your work! I have found your project extremely useful both personally and professionally, just doing what I can to contribute back :) |
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.
Follow-up to #98. Fixing the
:epipecrash uncovered a second problem on thesame path: output gets dropped and nothing tells you about it. The crash was
happening first, so it hid this. I'm using MuonTrap to run commands in an agent
harness and feed the output back to a model, which has no way to tell a
truncated result from a complete one.
process_stdio()only forwards as much as the flow control window allows, andchild_wait_loop()stops polling the output fds once that window is empty. Whenthe child is reaped we return right away, so anything still sitting in the pipes
goes away with it. That's why what gets lost is always a whole number of
windows.
cmd/3just hands back a short binary with a 0 exit status, so there's nothingto key off of. Two commands, 40 runs each:
print_and_exit.test, 66890 bytes: 0/40 runs came back complete, worst casekept 10240 bytes
seq 1 100000, 588895 bytes: 24/40 complete, worst case kept 542720 bytesThe child's writes have all finished by the time it exits, so draining the pipes
before returning is enough. Polling with a 0 timeout tells us when a pipe is
empty, so the usual case costs one extra syscall. If the window is empty I wait
on acks the same way the exit path already does.
The test I added fails at 461/1001 lines without the C change. Related:
test/print_a_lot.chas had asleep(1)in it since flow control landed ind3d0336, commented "muontrap doesn't wait for all output to be consumed". That
sleep is why the existing "prints a lot" tests pass, so I left it alone and used
print_and_exit.testinstead.Only tested on aarch64 Linux, so the
splice()branch. The read/write fallbackgoes through the same drain loop but I have no way to run it here. cgroup tests
are excluded since I didn't set up the
muontrap_testcgroup; the rest pass.