[5/8] Add executor process transport for MCP stdio#18088
[5/8] Add executor process transport for MCP stdio#18088aibrahim-oai wants to merge 1 commit intodev/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: 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".
| 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 lagged event streams instead of terminating transport
RecvError::Lagged means this receiver skipped broadcast entries, not that the process is dead. The code sets closed = true, which makes receive() return EOF and triggers transport shutdown, aborting otherwise healthy MCP sessions during bursty output. Since ExecProcess also exposes retained read() data, this path should resync (or fail explicitly) rather than force-close.
Useful? React with 👍 / 👎.
b440257 to
c603449
Compare
f6e61df to
f11cc84
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f11cc84bdc
ℹ️ 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.
Drain buffered frames after parse errors
take_stdout_message drops an invalid line and returns None, then receive_message immediately waits on events.recv(). If a single output chunk contains bad\n{valid-json}\n, the valid JSON frame remains in self.stdout but is never processed until another event arrives. A quiet server can then stall MCP initialization until timeout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f11cc84bdc
ℹ️ 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".
|
|
||
| impl ExecutorProcessTransport { | ||
| pub(super) fn new(process: Arc<dyn ExecProcess>, program_name: String) -> Self { | ||
| let events = process.subscribe_events(); |
There was a problem hiding this comment.
Replay missed process events before awaiting broadcast
ExecutorProcessTransport::new subscribes to process.subscribe_events() only after start() has already returned. If the child exits immediately, Closed/Failed can be emitted before this subscription and are not replayed by broadcast. receive_message() then waits on events.recv() with closed == false, which can stall MCP handshake/reads indefinitely for dead-on-start servers.
Useful? React with 👍 / 👎.
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0af64c7008
ℹ️ 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 process when handshake setup fails
Add cleanup parity with local stdio: this branch keeps no guard/owner for the spawned executor process. If serve_client errors or time::timeout elapses during initialization, the future is dropped and the process can remain running without a later terminate(), leaking MCP subprocesses across retries.
Useful? React with 👍 / 👎.
c603449 to
db32dd3
Compare
0af64c7 to
aad254a
Compare
Add an rmcp transport and runtime that bridge stdio bytes through the executor process API. Co-authored-by: Codex <noreply@openai.com>
aad254a to
ad686d6
Compare
db32dd3 to
2ccbd13
Compare
Summary
Stack