Repository navigation
ControlSocket: end the proxy when either direction closes - #1383
Open
aatishparson-arker wants to merge 1 commit into
Open
aatishparson-arker wants to merge 1 commit into
aatishparson-arker wants to merge 1 commit into
Conversation
`handleClient` proxies the client's unix socket to the guest's vsock with two copy loops in one `withThrowingDiscardingTaskGroup`. Neither direction propagates a half-close. When the guest closes its connection, `vmInbound` ends and that child returns normally -- it does not throw, so the discarding group neither cancels its sibling nor completes. The sibling stays parked on `clientInbound`, waiting for bytes from a client that is itself waiting for the close to arrive, and `executeThenClose` cannot close the client connection until the group returns. So a guest-initiated close is never observable by the client: the connection stays open until the client gives up on its own. The same holds symmetrically when the client goes first, and each stranded session leaves a parked child holding a connection on an event loop group built with `numberOfThreads: 1`. Take the first child to finish and cancel the rest. This loses no data in either direction: a child finishes only once its inbound sequence is exhausted, so everything that side sent has already been written to the other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
yzhuang-oai
requested changes
Oct 23, 2026
| // inbound sequence is exhausted, so everything that side sent has | ||
| // already been written to the other. | ||
| _ = try await group.next() | ||
| group.cancelAll() |
Contributor
There was a problem hiding this comment.
This will discard buffered output.
NIO’s async write() can return before bytes reach the socket. Once the first copy loop finishes, cancelling its sibling lets executeThenClose close both channels, discarding pending writes.
Should be reproducible
This branch has not been deployed
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.
The bug
ControlSocket.handleClientproxies the host's unix socket to the guest's vsock with two copy loops in onewithThrowingDiscardingTaskGroup. Neither direction propagates a half-close.When the guest closes its connection,
vmInboundends and that child returns normally — it does not throw, so the discarding group neither cancels its sibling nor completes. The sibling stays parked forever onclientInbound, waiting for bytes from a client that is itself waiting for the close to arrive.executeThenClosecannot close the client connection until the group returns, so a guest-initiated close is never observable by the client: the connection simply stays open until the client gives up on its own.The same applies symmetrically if the client goes away first.
Each stranded session also leaves a parked child holding a connection on an event loop group built with
numberOfThreads: 1.How we hit it
We reach a macOS guest's agent over this socket. A terminal session's only close signal is EOF on its stream, so a customer's terminal never closed when their shell exited — it hung until their client timed out. Measured on 2.40.1: our host-side EOF path fired 0 times in 30 detaches, while every session ended through the client-side timeout branch.
The guest was closing correctly: it closes its single fd, and adding an explicit
shutdown(SHUT_WR)before thatclose(so the FIN cannot depend on a last reference) changed nothing. The FIN reaches Tart and stops there.ControlSocket.swiftis byte-identical in 2.40.1, 2.41.0, 2.41.1 andmain, so this is not a regression and no released version has it fixed.The fix
Take the first child to finish and cancel the rest. This loses no data in either direction: a child finishes only once its inbound sequence is exhausted, so everything that side sent has already been written to the other.
Verification
handleClientcan't be driven from a test — it dials a live VM through the globalvm, and there is no existing test for the proxy — so I reproduced its exact structure over two socket pairs, carrying both shapes so the result doesn't depend on which one the source has. 5/5 runs, Swift 6.2.4 / macOS 26:A sentinel byte through the proxy is a hard precondition in that harness, so "no EOF" cannot mean "the proxy never started". I'm happy to contribute that as a test if you'd like it in a form that fits your suite.
Then on real hardware, with a patched 2.41.1 build on an EC2
mac-m4.metalhost: the two terminal-close tests that failed deterministically now pass, and our host-side EOF path fired twice — once per test — where it had fired zero times in thirty.