fix(streaming): recover setup and close completed IO - #300
Draft
JacobGabrielson wants to merge 1 commit into
Draft
JacobGabrielson wants to merge 1 commit into
JacobGabrielson wants to merge 1 commit into
Conversation
Signed-off-by: Jacob Gabrielson <jacob.gabrielson@docker.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved console shutdown, duplicate-claim, and context-cancellation issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves guest stream handshake recovery and cleanup of TTY, console, and transfer resources.
Changes:
- Adds concurrent, deadline-bound handshakes with retry backoff.
- Preserves buffered bidirectional transfer data during closure.
- Expands ownership cleanup and regression coverage.
Review findings:
internal/vminit/runc/platform.go: critical issue (2 votes) with epoll console shutdown cleanup.plugins/vminit/streaming/plugin.go: moderate duplicate-claim issue (2 votes).plugins/vminit/streaming/plugin.go: moderate context-cancellation issue (1 vote).
File summaries
| File | Summary |
|---|---|
plugins/vminit/streaming/service_test.go |
Tests handshake and registration behavior. |
plugins/vminit/streaming/plugin.go |
Implements handshake lifecycle and transfer closure changes. |
plugins/vminit/streaming/plugin_test.go |
Tests bidirectional transfer closure. |
plugins/vminit/streaming/claim_linux_test.go |
Tests delayed stream claims on Linux. |
internal/vminit/runc/platform.go |
Tracks and closes console resources. |
internal/vminit/runc/platform_test.go |
Tests stdout ownership and draining. |
internal/vminit/process/init.go |
Closes init console resources during deletion. |
internal/vminit/process/init_state.go |
Handles optional stdin ownership. |
internal/vminit/process/exec.go |
Closes exec console resources during deletion. |
internal/vminit/process/exec_test.go |
Tests TTY cleanup without processIO. |
Review details
Suppressed comments (1)
plugins/vminit/streaming/plugin.go:275
Getnow waits onr.readyfor the handshake to finish, but this call discards thectxsupplied by theStreamGetterAPI. A canceled transfer/request can therefore remain blocked for the full 15-second handshake timeout instead of being released with the caller; thread the context through the claim/wait path and clean up the claimed registration when it is canceled.
conn, err := sg.s.Get(name)
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+264
to
+272
| func (c *closingConsole) Close() error { | ||
| c.once.Do(func() { | ||
| var errs []error | ||
| for _, closer := range []io.Closer{c.closeStdin, c.closeStdout, c.EpollConsole} { | ||
| if closer != nil { | ||
| errs = append(errs, closer.Close()) | ||
| } | ||
| } | ||
| c.err = errors.Join(errs...) |
Comment on lines
+201
to
+204
| if _, ok := s.streams[streamID]; ok { | ||
| s.mu.Unlock() | ||
| writeString(conn, fmt.Sprintf("stream %q already exists", streamID)) | ||
| return |
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.
Summary
processIOis nil; close framed transfer connections after both directions finish without discarding reverse-direction data.The sandbox-mutex change is already proposed in #295 and is deliberately excluded here. This PR is independently applicable and contains no libkrun or VM-manager locking changes.
Successful but unclaimed registrations are deliberately not expired: a prepared exec can start much later, and a socket-forward peer can fully close after sending a buffered response before the guest receives the control-plane handoff. Safe abandonment cleanup needs an explicit cancellation contract and is not implemented here.
Test plan
Authored and posted by Codex on behalf of the contributing user.