Skip to content

openvmm: support rpc over named pipes - #4389

Open
Daman Mulye (damanm24) wants to merge 10 commits into
microsoft:mainfrom
damanm24:rpc-named-pipe
Open

openvmm: support rpc over named pipes#4389
Daman Mulye (damanm24) wants to merge 10 commits into
microsoft:mainfrom
damanm24:rpc-named-pipe

Conversation

@damanm24

Copy link
Copy Markdown
Contributor

Tonic, a Rust RPC framework that uses Tokio as its async runtime, allows consumers to define a Connector that builds a Channel over non-TCP transports such as UDS or named pipes.

Windows clients could supply a connector over AF_UNIX, but doing so requires a custom async socket adapter: implementing Tokio’s AsyncRead and AsyncWrite traits around Winsock APIs.

Tokio already exposes Windows named pipes as AsyncRead/AsyncWrite streams. Tonic can consume those streams through the standard TokioIo adapter, so Windows OpenVMM clients need only a small connector that opens the named pipe.

This PR allows OpenVMM to expose its RPC server over a named-pipe listener.

@damanm24
Daman Mulye (damanm24) requested a review from a team as a code owner September 4, 2026 17:08
Copilot AI lite review requested due to automatic review settings September 4, 2026 17:08
@github-actions github-actions Bot added the Guide label Sep 4, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new stream adapters should avoid potentially blocking reads on zero-length buffers to prevent subtle hangs in async I/O paths.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR extends OpenVMM’s RPC server to support Windows named pipes as an alternative to Unix domain sockets, enabling non-TCP transports for ttrpc/gRPC clients on Windows.

Changes:

  • Add a Windows named-pipe listener mode for the --rpc server and dispatch loop.
  • Generalize mesh_rpc::Server::{serve_connection, serve_connection_grpc} to accept generic async read/write streams (not just sockets).
  • Update CLI parsing/docs and expand VMM tests to cover named-pipe endpoints on Windows.
File summaries
File Description
vmm_tests/vmm_tests/tests/tests/ttrpc.rs Adds Windows coverage for RPC over named pipes and threads endpoint type through the OpenVMM launcher and client dialer.
support/mesh/mesh_rpc/src/server.rs Generalizes server connection handlers to accept futures::AsyncRead/AsyncWrite streams, improving transport flexibility.
openvmm/openvmm_entry/src/ttrpc/mod.rs Introduces Listener abstraction and named-pipe accept/dispatch path on Windows, including prefix-based protocol sniffing for pipes.
openvmm/openvmm_entry/src/lib.rs Wires --rpc listener selection into runtime: unix socket vs Windows named pipe.
openvmm/openvmm_entry/src/cli_args.rs Extends --rpc syntax with `listener=<unix
Guide/src/reference/openvmm/management/grpc.md Documents listener=pipe usage for Windows named-pipe RPC endpoints.
Review details

Suppressed comments (1)

support/mesh/mesh_rpc/src/server.rs:386

  • In this Wrap<T> Tokio AsyncRead adapter, it’s safer to short-circuit when the ReadBuf has no remaining capacity. Otherwise the inner futures::AsyncRead may be polled with an empty slice, which can block instead of returning immediately.
            impl<T: AsyncRead + Unpin> tokio::io::AsyncRead for Wrap<T> {
                fn poll_read(
                    self: Pin<&mut Self>,
                    cx: &mut std::task::Context<'_>,
                    buf: &mut tokio::io::ReadBuf<'_>,
  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • 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 thread openvmm/openvmm_entry/src/ttrpc/mod.rs
Copilot AI review requested due to automatic review settings September 4, 2026 17:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The Windows named-pipe client dialer lacks bounded retry handling for common transient pipe-open failures, risking flaky tests, and CLI parsing should reject listener=pipe on non-Windows earlier for a clearer failure mode.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

openvmm/openvmm_entry/src/cli_args.rs:1998

  • listener=pipe is documented as Windows-only, but RpcCli::from_str currently accepts it on non-Windows targets and only errors later during runtime startup. Rejecting it during option parsing provides a clearer, earlier failure mode and avoids accepting an unsupported configuration.
    vmm_tests/vmm_tests/tests/tests/ttrpc.rs:1113
  • Named pipe connection setup can be transiently unavailable (e.g. server still starting, pipe not yet created, or ERROR_PIPE_BUSY). dial() currently fails immediately on these cases, which can make the Windows named-pipe test path flaky. Consider retrying open for a bounded time when the error is NotFound or ERROR_PIPE_BUSY.
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Comment thread Guide/src/reference/openvmm/management/grpc.md Outdated
Comment thread openvmm/openvmm_entry/src/ttrpc/mod.rs Outdated
Copilot AI review requested due to automatic review settings September 4, 2026 18:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The current named-pipe path detection is case-sensitive on Windows (can misroute valid \\.\PIPE\... inputs), and the public UnixDialierUnixDialer rename should preserve compatibility via a deprecated alias.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

support/mesh/mesh_rpc/src/client.rs:72

  • Renaming the public UnixDialier type to UnixDialer is an API-breaking change for any external users of mesh_rpc. Consider keeping a deprecated type alias for backward compatibility (the alias will still expose ::new and other inherent methods).
  • Files reviewed: 7/7 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread openvmm/openvmm_entry/src/lib.rs Outdated
Comment thread vmm_tests/vmm_tests/tests/tests/ttrpc.rs
Comment thread vmm_tests/vmm_tests/tests/tests/ttrpc.rs Outdated
Copilot AI review requested due to automatic review settings September 4, 2026 19:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

It introduces at least one breaking public API rename and a new named-pipe accept loop that currently drops accept errors silently, both of which should be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

support/mesh/mesh_rpc/src/client.rs:73

  • UnixDialier was a public type; renaming it to UnixDialer is a breaking change for downstream crates. Consider keeping a deprecated type alias so existing callers continue to compile while migrating to the corrected name.
  • Files reviewed: 8/8 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread openvmm/openvmm_entry/src/ttrpc/mod.rs
Comment thread support/mesh/mesh_rpc/src/lib.rs
Copilot AI review requested due to automatic review settings September 4, 2026 19:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new named-pipe accept loop drops pending accepts in a way that can block the async runtime, and the gRPC stream adapter’s shutdown path likely fails on PolledPipe (both are fixable but should be addressed before approval).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

support/mesh/mesh_rpc/src/server.rs:399

  • The Tokio adapter for futures::AsyncWrite in this Wrap<T> impl maps poll_shutdown() to poll_close() (see below in this impl). For pal_async::pipe::PolledPipe, poll_close() returns Unsupported, which will likely surface as an I/O error when serving gRPC over named pipes. Consider treating Unsupported as a successful shutdown (drop will still close the handle).
  • Files reviewed: 8/8 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread openvmm/openvmm_entry/src/ttrpc/mod.rs Outdated
Copilot AI review requested due to automatic review settings September 4, 2026 19:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The Windows named-pipe server path in openvmm_entry/src/ttrpc/mod.rs contains a compile-breaking call signature mismatch in serve_pipe.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 8/8 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread openvmm/openvmm_entry/src/ttrpc/mod.rs Outdated
Comment thread openvmm/openvmm_entry/src/ttrpc/mod.rs Outdated
Copilot AI review requested due to automatic review settings September 4, 2026 19:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The new named-pipe accept loop currently drops accept/wrap errors without logging, which can mask connectivity failures and complicate diagnosis.

Review details

Suppressed comments (1)

openvmm/openvmm_entry/src/ttrpc/mod.rs:329

  • In the named-pipe accept loop, errors from accept and from PolledPipe::new are silently ignored, which can make failures hard to diagnose (and can lead to a server that appears to hang without any logs). Please log these error cases explicitly instead of dropping them.
                result = accept => {
                    accept.set(listener.accept(driver)?.fuse());
                    if let Ok(conn) = result.and_then(|conn| pal_async::pipe::PolledPipe::new(driver, conn)) {
                        tasks.push(async move {
                            let _ = serve_pipe(server, conn, transport)
  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 4, 2026 21:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The gRPC stream adapter can treat named-pipe shutdown as an error (unsupported close semantics), which risks spurious failures/log noise for gRPC over named pipes.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

support/mesh/mesh_rpc/src/server.rs:399

  • serve_connection_grpc maps tokio::io::AsyncWrite::poll_shutdown to the underlying stream’s futures::AsyncWrite::poll_close. This returns ErrorKind::Unsupported for pal_async::pipe::PolledPipe, which can surface as spurious connection errors when serving gRPC over named pipes. Consider treating Unsupported as a successful shutdown (best-effort), since pipes don’t support a socket-like half-close.
  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants