Skip to content

fix(streaming): recover setup and close completed IO - #300

Draft
JacobGabrielson wants to merge 1 commit into
containerd:mainfrom
JacobGabrielson:fix/guest-stream-recovery-20260918
Draft

JacobGabrielson wants to merge 1 commit into
containerd:mainfrom
JacobGabrielson:fix/guest-stream-recovery-20260918

Conversation

@JacobGabrielson

Copy link
Copy Markdown

Summary

  • Handle guest stream handshakes concurrently with a 15-second deadline, retry non-closure accept errors with capped backoff, and retain ownership through ACK completion so shutdown closes incomplete handshakes even after an early claim.
  • Close TTY stdin, stdout transport, and console resources after output drain even when processIO is nil; close framed transfer connections after both directions finish without discarding reverse-direction data.
  • Preserve the existing arbitrary-string ID contract. Read ID bytes incrementally up to the declared length rather than allocating from the untrusted length prefix alone; reject truncated IDs. The deadline bounds handshake time, not total memory from a peer that actually sends a very large ID.

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

  • Fail-before/pass-after regressions for accept-loop isolation, accept-error recovery, TTY resource ownership, and completed transfer closure.
  • Long-ID compatibility regression (8192 bytes), incomplete/huge length-prefix tests, shutdown during a pending ACK, bidirectional EOF, and delayed ownership tests.
  • Maintained macOS unit target and repeated focused streaming race tests.
  • Final Linux arm64 guest and streaming-test cross-builds, and Windows arm64 streaming-test cross-build.
  • Linux arm64 TTY/process and real AF_VSOCK retained-buffer/stream tests on a disposable runner, before the final compatibility-only parser adjustment; no platform-specific production code changed after that run.
  • Full VM and native cross-platform CI.
Keep guest stream setup responsive after stalled handshakes or accept errors and release completed terminal and transfer resources without discarding buffered output.

Authored and posted by Codex on behalf of the contributing user.

Signed-off-by: Jacob Gabrielson <jacob.gabrielson@docker.com>
Copilot AI lite review requested due to automatic review settings September 18, 2026 18:36

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

  • Get now waits on r.ready for the handshake to finish, but this call discards the ctx supplied by the StreamGetter API. 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants