vmm/task: fix stdout stream handoff and cleanup - #229
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the streaming I/O mechanism within the VMM task component to address an observed issue where short-lived Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of occasional missing output from short-lived commands by improving the handling of stdout stream handoff and cleanup. The changes, including deferring channel cleanup until fully drained, handling client-side stream closure gracefully, and ensuring the preemption path works correctly even after a sender is closed, are well-implemented. The addition of regression tests for these scenarios is also a great improvement. I have one suggestion regarding code repetition in handle_stdout for better maintainability, but overall, this is a solid fix.
There was a problem hiding this comment.
Pull request overview
This PR addresses intermittent missing output for short-lived crictl exec -it ... by improving the stdout/stderr streaming handoff behavior during receiver preemption and by deferring output-channel cleanup until the channel is fully drained.
Changes:
- Track
sender_closedperIOChanneland only remove closed output channels once the receiver buffer and anyremaining_dataare drained. - Update stdout/stderr streaming to detect client-initiated close while forwarding output, and keep receiver handoff functional after sender close.
- Add regression tests for sender-closed handoff and drained-channel cleanup; update IO copy teardown to call the new output-close path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
vmm/task/src/streaming.rs |
Adds sender-closed tracking, delayed cleanup logic, client-close detection in stdout forwarding, and new regression tests. |
vmm/task/src/io.rs |
Switches streaming output teardown from remove_channel to close_stdout after container-to-client copy completes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8af8d4d to
197c135
Compare
There was a problem hiding this comment.
Pull request overview
Fixes intermittent missing output for short-lived crictl exec -it commands by making stdout forwarding resilient to client-close, preserving preemption handoff behavior after sender close, and deferring IO channel cleanup until an output channel is fully drained.
Changes:
- Detect stdout client-close while forwarding and exit cleanly without dropping output-hand-off state.
- Introduce
sender_closed+ drained checks to decide when an output channel can be safely removed. - Add regression tests covering sender-closed handoff and drained-channel cleanup.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| vmm/task/src/streaming.rs | Adds sender-closed/drain-aware cleanup, handles stdout stream client-close via stream split + select, and adds regression tests. |
| vmm/task/src/io.rs | Switches container→streaming close behavior from hard removal to sender-closed signaling (close_output). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
40947f5 to
16b89b6
Compare
8e1104a to
44e9b92
Compare
|
@novahe The PR needs rebase, could you update it? |
44e9b92 to
80339fc
Compare
838c920 to
2184f64
Compare
Handle output client close while forwarding stream data, keep the preemption handoff wakeup path working after sender close, and clean up closed output channels only after they are fully drained. Rename the public streaming close helper from close_stdout to close_output so the cleanup API matches the fact that the path is used for both stdout and stderr streaming URLs. Add regression tests for sender-closed handoff and drained channel cleanup. Co-authored-by: novahe <heqianfly@gmail.com> Signed-off-by: novahe <heqianfly@gmail.com>
2184f64 to
22b49e8
Compare
Background
This change is a relay follow-up to #202.
Observed issue:
crictl exec -it xx lsis a short-lived command, and it would occasionally return with no output.Summary
This change simplifies and hardens the
vmm-taskstreaming handoff path for stdout/stderr, and tightens stdin read behavior for partial-buffer reads.The main fixes are:
AsyncReadcallsclose_stdouttoclose_outputto match its actual use for both stdout and stderrflowchart TD S1[Created] S2[Producer Attached] S3[Consumer Attached] S4[Buffered For Reattach<br/>remaining_data=Some] S5[Preempt Requested] S6[Producer Closed - Draining<br/>sender_closed=true] S7[Removed] N1[Waiting Preemptor Retries] E1[preempt_receiver returns NOT_FOUND] O1[handle_stdout returns Ok] S1 -->|get_output| S2 S1 -.->|preempt_receiver first attach| S3 S2 -->|preempt_receiver| S3 S6 -->|preempt_receiver| S3 S3 -->|stream_sender.send fails| S4 S4 -->|next handle_stdout flushes remaining_data first| S3 S3 -->|new consumer arrives| S5 S5 -->|return receiver, rem_data=None, sender_closed=false| S2 S5 -->|return receiver, rem_data=Some| S4 S5 -->|return receiver, rem_data=None, sender_closed=true, drained=no| S6 S5 -->|return receiver, rem_data=None, sender_closed=true, drained=yes| S7 S2 -->|close_output, drained=no| S6 S2 -->|close_output, drained=yes| S7 S3 -->|close_output marks sender_closed=true| S3 S4 -->|close_output marks sender_closed=true| S4 S3 -->|client closes stream| S5 S3 -->|marshal fails, receiver returned| S5 S3 -->|receiver.recv returns empty| S7 S3 -->|receiver.recv returns None| S7 S6 -->|remaining_data cleared and receiver drained| S7 S7 -->|waiting preemptor retries| N1 N1 -->|channel missing| E1 E1 -->|handle_stdout treats finished stream as normal completion| O1