fix(streaming): drain VM bridge on RPC ctx cancel before vmConn.Close#177
Open
ndeloof wants to merge 1 commit intocontainerd:mainfrom
Open
fix(streaming): drain VM bridge on RPC ctx cancel before vmConn.Close#177ndeloof wants to merge 1 commit intocontainerd:mainfrom
ndeloof wants to merge 1 commit intocontainerd:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the shim streaming bridge so the VM-side stream connection is no longer tied directly to the per-RPC ttrpc context, aiming to stop normal end-of-exec context cancellation from cascading into a full VM teardown on Windows.
Changes:
- Starts the VM stream with
context.WithoutCancel(ctx)instead of the per-RPC context. - Removes the handler’s early return on
ctx.Done()and waits only for the VM-to-client bridge to finish. - Expands inline documentation around the stream-closing protocol and the Windows-specific shutdown cascade.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ndeloof
force-pushed
the
fix-streaming-vmconn-lifetime
branch
from
ba85164 to
338339c
Compare
ndeloof
force-pushed
the
fix-streaming-vmconn-lifetime
branch
from
338339c to
750e1e2
Compare
ndeloof
changed the title
On Windows, vmConn.Close() traverses the AF_UNIX <-> vsock proxy and arrives at vminitd as a vsock SHUTDOWN packet. When the per-RPC ctx cancels mid-stream, the deferred Close() fires while the VM may still have in-flight data; the SHUTDOWN cascades into Task.Kill -> Delete -> Shutdown and tears down the entire VM, surfacing as state_error="rpc error: code = Unknown desc = ttrpc: closed". Linux/macOS use native vsock without the AF_UNIX intermediate, so the cascade does not occur there. Empirical evidence (docker/sandboxes#2529): across 10 parallel CI runs of identical content, every PASS produced exactly 1 EOF event on the proxy AF_UNIX socket while every FAIL produced 13-30. Zero overlap. This change has two pieces, both of which serialise teardown so that v2t (the VM->client bridge) drains cleanly before vmConn.Close() runs: 1. In Stream's <-ctx.Done() branch (per-RPC cancel mid-stream): call vmConn.SetReadDeadline(time.Now()) to interrupt binary.Read via the Go runtime poller -- no wire-level packet -- and drain v2t before the deferred Close() fires. 2. On shim shutdown (ContainerStop of the kit container): track every in-flight stream's vmConn so the shutdown callback can SetReadDeadline on all of them and wait (via a WaitGroup) for every Stream handler to return before reporting back. Without this, N concurrent handlers race ttrpc.server.Close() and the transport may go away before all bridges have drained, leaving partially-drained streams whose eventual Close() re-triggers the SHUTDOWN cascade against the already-stopping VM. Late-arriving Stream RPCs (after shutdown has set closing=true) are rejected with errdefs.ErrUnavailable rather than being allowed to register a vmConn the drain has already finished waiting for. Tests: - TestStreamReturnsAfterContextCancelOnIdleStream (per-RPC drain). - TestStreamPreservesStartStreamCancel (StartStream observes ctx). - TestShutdownDrainsAllInFlightStreams (shim-shutdown drain across N concurrent streams). - TestStreamRejectedAfterShutdown (post-shutdown new-stream rejection). - 3 pre-existing tests still pass. Refs: docker/sandboxes#2529 Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof
force-pushed
the
fix-streaming-vmconn-lifetime
branch
from
750e1e2 to
b12ea73
Compare
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.
Bug
On Windows,
vmConn.Close()traverses the AF_UNIX ↔ vsock proxy and arrives atvminitdas a vsockSHUTDOWN. When the per-RPC ctx cancels mid-stream, the deferredClose()races with in-flight bridge reads; theSHUTDOWNcascades intoTask.Kill -> Delete -> Shutdown, tearing down the VM (state_error="ttrpc: closed"). Linux/macOS use native vsock without the proxy, so no cascade.Fix
In the
<-ctx.Done()branch, callvmConn.SetReadDeadline(time.Now())to interrupt the bridge'sbinary.Readvia the Go runtime poller (no wire-level packet) and drainv2tbefore the deferredClose()runs.Tests
TestStreamReturnsAfterContextCancelOnIdleStream— cancel ctx with no traffic; handler must drain.TestStreamPreservesStartStreamCancel— cancel ctx during a slowStartStream; abort must propagate.Both fail on baseline, pass on this PR. The 4 pre-existing tests still pass.
/sign-build