Skip to content

fix(ai/mcp-stdio): make createChildProcess synchronous to prevent spawn race condition#5852

Merged
iteratetograceness merged 3 commits intovercel:mainfrom
timjuenemann:fix/StdioMCPTransport-child-process-creation-timing
Apr 20, 2025
Merged

fix(ai/mcp-stdio): make createChildProcess synchronous to prevent spawn race condition#5852
iteratetograceness merged 3 commits intovercel:mainfrom
timjuenemann:fix/StdioMCPTransport-child-process-creation-timing

Conversation

@timjuenemann
Copy link
Copy Markdown
Contributor

@timjuenemann timjuenemann commented Apr 18, 2025 •
edited
Loading

Background

I ran into an issue where StdioMCPTransport never resolved. This happened cause the spawn event was sent before the listener was added.

Summary

I made the createChildProcess synchronous, so the listeners won't miss out on events.

Tasks

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features) -> No docs to update I think
  • If required, a patch changeset for relevant packages has been added
  • You've run pnpm prettier-fix to fix any formatting issues -> For some reason it fixed thousands of files

Future Work

–

@dangtony98
Copy link
Copy Markdown

dangtony98 commented Apr 19, 2025 •
edited
Loading

@timjuenemann Is this PR related to createMCPClient() hanging when used with StdioMCPTransport by any chance?

I followed the documentation only to find that createMCPClient just hangs and so subsequent methods on mcpClient are not reachable at the moment..

const mcpClient = await createMCPClient({
    transport: new StdioMCPTransport({
      command: "npx",
      args: ["-y", "@infisical/mcp"],
      ...
  });

If so, it would be great if the Vercel team can prioritize this issue as I would imagine this to be a big blocker for what I would think to be a critical/popular use case.

Tagging @iteratetograceness @lgrammel to raise awareness to this issue since I think it's quite important.

@timjuenemann
Copy link
Copy Markdown
Contributor Author

timjuenemann commented Apr 20, 2025

Yes, this PR fixes exactly that issue!

@iteratetograceness
Copy link
Copy Markdown
Contributor

iteratetograceness commented Apr 20, 2025

@timjuenemann thank you for catching this race condition! Also, I'm not sure why the changeset command added those extra files; reviewing now

iteratetograceness
iteratetograceness reviewed Apr 20, 2025
View reviewed changes
packages/ai/mcp-stdio/create-child-process.test.ts Outdated
);

expect(childProcess.pid).toBeDefined();
await new Promise<void>((resolve) => {
Copy link
Copy Markdown
Contributor

@iteratetograceness iteratetograceness Apr 20, 2025

Choose a reason for hiding this comment

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

Not sure this is a necessary assertion; we should be able to just keep: expect(childProcess.pid).toBeDefined()

Copy link
Copy Markdown
Contributor Author

@timjuenemann timjuenemann Apr 20, 2025

Choose a reason for hiding this comment

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

Yes you're right!

packages/ai/.changeset/README.md Outdated
@@ -0,0 +1,8 @@
# Changesets
Copy link
Copy Markdown
Contributor

@iteratetograceness iteratetograceness Apr 20, 2025

Choose a reason for hiding this comment

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

remove

packages/ai/.changeset/config.json Outdated
@@ -0,0 +1,11 @@
{
Copy link
Copy Markdown
Contributor

@iteratetograceness iteratetograceness Apr 20, 2025

Choose a reason for hiding this comment

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

also remove pls

packages/ai/.changeset/quick-toys-study.md Outdated
'ai': patch
---

Fix issue where StdioMCPTransport never resolves cause of child process creation timing issue
Copy link
Copy Markdown
Contributor

@iteratetograceness iteratetograceness Apr 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
Fix issue where StdioMCPTransport never resolves cause of child process creation timing issue
fix (ai/mcp-stdio): make `createChildProcess` synchronous to prevent spawn race condition

packages/ai/mcp-stdio/mcp-stdio-transport.test.ts Outdated
@@ -117,12 +117,12 @@ describe('StdioMCPTransport', () => {
});

it('should handle child_process import errors', async () => {
Copy link
Copy Markdown
Contributor

@iteratetograceness iteratetograceness Apr 20, 2025

Choose a reason for hiding this comment

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

non-blocking: should handle child_process import errors is a legacy test (from before we moved stdio transport files out of the ai/core folder)! we can remove

@iteratetograceness iteratetograceness changed the title fix(ai): Fix issue where StdioMCPTransport never resolves cause of child process creation timing issue fix(ai/mcp-stdio): make createChildProcess synchronous to prevent spawn race condition Apr 20, 2025
@timjuenemann
Copy link
Copy Markdown
Contributor Author

timjuenemann commented Apr 20, 2025

Thanks for the review @iteratetograceness :) let me know if there is anything else.

@iteratetograceness iteratetograceness merged commit 5379cc9 into vercel:main Apr 20, 2025
6 of 7 checks passed
samdenty pushed a commit that referenced this pull request Apr 22, 2025
@timjuenemann @iteratetograceness
fix(ai/mcp-stdio): make createChildProcess synchronous to prevent s…
e9380f3 
…pawn race condition (#5852)

Co-authored-by: Grace Yun <74513600+iteratetograceness@users.noreply.github.com>
samdenty added a commit that referenced this pull request Apr 22, 2025
@samdenty @timjuenemann @iteratetograceness
fix(ai/mcp-stdio): make createChildProcess synchronous to prevent s…
06a079c 
…pawn race condition (#5852) (#5888)

Co-authored-by: Tim Jünemann <tim.juene@gmail.com>
Co-authored-by: Grace Yun <74513600+iteratetograceness@users.noreply.github.com>
samdenty pushed a commit that referenced this pull request Apr 24, 2025
Merge remote-tracking branch 'origin/v5' into v5-providerOptions-rema…
75293df 
…ining2

* origin/v5: (25 commits)
  chore(providers/langchain): extract to separate package (#5928)
  Version Packages (canary) (#5898)
  fix (docs): fix typo (#5914) (#5919)
  docs: update chat-with-pdf page to include updated provider information (#5895) (#5917)
  docs: Added documentation for function call parsing middleware (#5900)
  refactoring: extract SharedV2Headers (#5912)
  feat (providers/openai): add support for reasoning summaries (#5906) (#5909)
  fix (docs): fix OpenRouter code examples (#5876) (#5893)
  fix(providers/xai): return actual usage when streaming instead of NaN (#5873) (#5891)
  fix (docs): fix valibotSchema import (#5865) (#5889)
  fix (providers/fal): improve model compatibility (#5855) (#5892)
  chore: extract spec types (#5901)
  chore: remove logprobs (#5896)
  Version Packages (canary) (#5872)
  chore (ci): unify jobs & remove unnecessary example builds (#5894)
  feat (ai): allow using provider default temperature by specifying null (#5890)
  fix(ai/mcp-stdio): make `createChildProcess` synchronous to prevent spawn race condition (#5852) (#5888)
  feat (providers/google): add thinking config to provider options (#5842) (#5887)
  fix (examples): avoid mixed message types in `use-chat-streamdata-multistep` (#5824) (#5886)
  chore: restructure language model supported urls (#5882)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@iteratetograceness iteratetograceness iteratetograceness approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@timjuenemann @dangtony98 @iteratetograceness