[5/8] Add executor process transport for MCP stdio#17987
[5/8] Add executor process transport for MCP stdio#17987aibrahim-oai wants to merge 1 commit intoopenai:dev/remote-mcp-stdio-runtimefrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96607d5b20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fdb0d74 to
377fd73
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 377fd736cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| LaunchedStdioServerTransport::Executor { transport } => ( | ||
| service::serve_client(client_service, transport).boxed(), | ||
| None, | ||
| None, |
There was a problem hiding this comment.
Terminate executor MCP process when handshake aborts
connect_pending_transport wraps serve_client(...) in a timeout; on timeout or handshake error, that future is dropped before RunningService exists. In the executor branch there is no cleanup guard (None), and ExecutorProcessTransport only terminates the child in close(). This can leave remote MCP server processes running after failed initialization.
Useful? React with 👍 / 👎.
377fd73 to
c468eef
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c468eef9bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Err(error) => { | ||
| debug!( | ||
| "Failed to parse remote MCP server message ({}): {error}", | ||
| self.program_name | ||
| ); | ||
| None |
There was a problem hiding this comment.
Fail fast when remote MCP emits invalid JSON-RPC
take_stdout_message drops lines that fail serde_json parsing and keeps the transport alive. If a malformed line is actually a response, the request is never surfaced as an error and can hang until an outer timeout. The local stdio transport treats decode errors as transport failure, so this new path diverges and can hide protocol breakage.
Useful? React with 👍 / 👎.
c468eef to
83e867a
Compare
595325e to
3842a64
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83e867a607
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Err(broadcast::error::RecvError::Lagged(skipped)) => { | ||
| warn!( | ||
| "Remote MCP server output stream lagged ({}): skipped {skipped} events", | ||
| self.program_name | ||
| ); | ||
| self.closed = true; |
There was a problem hiding this comment.
Recover from lagged events instead of ending MCP stream
In ExecutorProcessTransport::receive_message, a broadcast::RecvError::Lagged sets self.closed = true, which makes receive() return EOF on the next loop. Because exec event channels are bounded, bursty/large stdout can trigger lag and this path truncates in-flight JSON-RPC output while the process may still be running. That can break handshakes or tool responses under load instead of resynchronizing via retained output.
Useful? React with 👍 / 👎.
Add an rmcp transport and runtime that bridge stdio bytes through the executor process API. Co-authored-by: Codex <noreply@openai.com>
3842a64 to
b440257
Compare
83e867a to
f6e61df
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6e61df763
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let started = exec_backend | ||
| .start(ExecParams { | ||
| process_id, | ||
| argv, | ||
| cwd: cwd.unwrap_or(default_cwd), | ||
| env_policy: None, | ||
| env, | ||
| tty: false, | ||
| stdin: ExecStdinMode::Piped, | ||
| arg0: None, | ||
| }) | ||
| .await | ||
| .map_err(io::Error::other)?; | ||
|
|
||
| Ok(LaunchedStdioServer { | ||
| transport: LaunchedStdioServerTransport::Executor { | ||
| transport: ExecutorProcessTransport::new(started.process, program_name), | ||
| }, |
There was a problem hiding this comment.
Subscribe to process events before starting executor process
launch_server awaits exec_backend.start(...) and only then builds ExecutorProcessTransport, which calls process.subscribe_events(). tokio::broadcast does not replay old messages, so early Output/Closed events can be lost. If the MCP process exits quickly, the terminal event may be missed and receive() can block until an external timeout (or forever if no timeout is configured).
Useful? React with 👍 / 👎.
|
Recreated as #18088 from an openai/codex head branch so internal CI uses the normal credentials path. |
Summary
Stack