Update close stdin logic - #265
Open
dmcgowan wants to merge 3 commits into
Open
Conversation
When an exec fails at Start, the guest never launches the process and never closes the exec's stdout/stderr vsock streams, so the host-side copy goroutines block indefinitely. A subsequent Delete would then stall on the 30s ioShutdown fallback. Untrack the exec and run its ioShutdown asynchronously with a short timeout on start failure, so Start returns promptly and Delete no longer blocks. Satisfies the ExecCommandNotFound conformance test. Signed-off-by: Derek McGowan <derek@mcg.dev>
There was a problem hiding this comment.
Pull request overview
This PR updates nerdbox’s shim-side stdin/stdio handling to ensure stdin is fully drained before delivering EOF, aligning behavior with containerd’s shim FIFO write-reference protocol (detach vs. CloseIO), including Windows named-pipe semantics. It also vendors an updated shimtest to validate attach/detach behavior and adds the missing task v2 API for version-aware task client dispatch.
Changes:
- Reworked shim IO forwarding so EOF is delivered via in-band
CloseWriteafter draining buffered stdin, andioShutdownsafely releases stdin references and waits for stdin draining when possible. - Implemented Windows stdin detach/re-attach behavior by reconnecting to the stdin named pipe until
CloseIO, plus added unit tests for the reconnection contract. - Updated vendored
shimtest(v0.3.3) and introduced version-aware task client selection (v2 vs v3), plus vendored the containerd task v2 API.
Reviewed changes
Copilot reviewed 7 out of 28 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
internal/shim/task/service.go |
Adds exec-start failure cleanup to avoid IO shutdown hangs; refines CloseIO stdin semantics commentary. |
internal/shim/task/io.go |
Integrates new copyStreams return values; releases stdin write reference during shutdown and optionally waits for stdin drain. |
internal/shim/task/io_copystreams_unix.go |
Changes stdin FIFO EOF/detach behavior by holding a shim write reference and only dropping it on CloseIO/shutdown; returns stdinDone. |
internal/shim/task/io_copystreams_windows.go |
Implements stdin detach/re-attach by redialing named pipes until stdinEOF is requested; returns stdinDone. |
internal/shim/task/io_copystreams_windows_test.go |
Adds Windows-only unit tests validating detach/re-attach and EOF delivery behavior. |
vendor/github.com/containerd/shimtest/* |
Updates conformance/bench suites to use version-aware task clients; adds stdin detach/re-attach conformance test and v2/v3 bridge client. |
vendor/github.com/containerd/containerd/api/runtime/task/v2/* |
Vendors containerd task v2 protobuf API needed for shimtest’s v2 bridge client. |
vendor/modules.txt |
Updates vendored module list for new shimtest version and task v2 API path. |
go.mod / go.sum |
Bumps github.com/containerd/shimtest dependency to v0.3.3. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
|
Closes #246 |
dmcgowan
force-pushed
the
close-stdin-logic
branch
from
August 5, 2026 22:53
99c6423 to
724aee4
Compare
dmcgowan
marked this pull request as ready for review
August 5, 2026 22:58
Comment on lines
+220
to
+229
| // The client disconnected without stdinEOF having been | ||
| // called: treat this as a detach and wait for a new | ||
| // client to reconnect on the same pipe path. This wait is | ||
| // deliberately not time-bounded -- a detach may | ||
| // legitimately last a long time -- and is rooted in | ||
| // context.Background() rather than ctx, since ctx is the | ||
| // Exec()/Create() RPC's context and is typically cancelled | ||
| // as soon as that RPC returns. It is only cancelled by | ||
| // stdinEOF (CloseIO, or container teardown calling | ||
| // stdinEOF as a safety net). |
Member
There was a problem hiding this comment.
Does it need to handle if the copy error was due to write failure; in that case would we still redial?
Rework host-side stdin handling so buffered stdin is fully drained before EOF is delivered, mirroring the containerd runc shim's FIFO write-reference protocol: closing the client's write end alone is a detach, and only CloseIO delivers EOF. Fixes truncation seen in the LargeStdioRoundTrip conformance test and enables stdin detach/re-attach. On Windows, named pipes lack a second-writer reference, so the same contract is provided by reconnecting to the stdin pipe on client disconnect until CloseIO fires. Signed-off-by: Derek McGowan <derek@mcg.dev>
Add a StdinDetachReattach conformance test verifying that closing a client's stdin write end without CloseIO does not deliver EOF, and that only CloseIO delivers it, allowing detach and later re-attach. Fix testOutboundTCP, testOutboundUDP, and the stress exec round-trip helper to issue CloseIO for stdin EOF instead of relying on the client's own FIFO/pipe write end closing alone, and fix ordering so the Windows stdin pipe listener is accepting before Create/Exec dials it. Signed-off-by: Derek McGowan <derek@mcg.dev>
dmcgowan
force-pushed
the
close-stdin-logic
branch
from
August 7, 2026 21:45
724aee4 to
a543519
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.
Rework host-side stdin handling so buffered stdin is fully drained
before EOF is delivered, mirroring the containerd runc shim's FIFO
write-reference protocol: closing the client's write end alone is a
detach, and only CloseIO delivers EOF. Fixes truncation seen in the
LargeStdioRoundTrip conformance test and enables stdin detach/re-attach.
On Windows, named pipes lack a second-writer reference, so the same
contract is provided by reconnecting to the stdin pipe on client
disconnect until CloseIO fires.
shimtest update includes attach/detach to validate this change, currently failing with nerdbox