fix: shut down stdio server on stdin EOF/close#44
Conversation
Prevent orphaned MCP processes by handling stdin EOF/close and transport onclose with a guarded shutdown path.
📝 WalkthroughWalkthroughThe PR unifies process shutdown by introducing a guarded ChangesUnified Shutdown Flow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/index.ts (1)
340-343: ⚡ Quick winKeep startup-failure termination on the same shutdown path.
At Line 340-343,
process.exit(1)still bypasses the new guarded shutdown path (reason logging + single-exit semantics). Consider exposing a small publicstop()/shutdown()entrypoint and using it here for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 340 - 343, Replace the direct process.exit(1) in the server.run() catch handler with a call to a single guarded shutdown entrypoint (e.g., export an async stop() or shutdown(reason?: Error|number) on your app/server module) so startup failures follow the same shutdown path; update the catch to call shutdown(error) after logger.error('Fatal error in main():', error) and ensure the new shutdown/stop implementation performs the reason logging, runs cleanup, and enforces single-exit semantics (only then call process.exit with the appropriate code).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/index.ts`:
- Around line 284-298: Replace the persistent listeners with one-shot handlers:
change the process and stdin event registrations that currently call
process.on(...) for 'SIGINT', 'SIGTERM', and the 'end'/'close' listeners on
process.stdin to use .once(...) so each event invokes the class's shutdown(...)
handler only once per process lifecycle; update the registrations around the
shutdown(...) references (the SIGINT, SIGTERM, process.stdin 'end' and 'close'
handlers) accordingly to prevent listener accumulation during repeated test
instantiation.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 340-343: Replace the direct process.exit(1) in the server.run()
catch handler with a call to a single guarded shutdown entrypoint (e.g., export
an async stop() or shutdown(reason?: Error|number) on your app/server module) so
startup failures follow the same shutdown path; update the catch to call
shutdown(error) after logger.error('Fatal error in main():', error) and ensure
the new shutdown/stop implementation performs the reason logging, runs cleanup,
and enforces single-exit semantics (only then call process.exit with the
appropriate code).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| process.on('SIGINT', () => { | ||
| process.exit(0); | ||
| this.shutdown(0, 'SIGINT'); | ||
| }); | ||
|
|
||
| process.on('SIGTERM', () => { | ||
| process.exit(0); | ||
| this.shutdown(0, 'SIGTERM'); | ||
| }); | ||
|
|
||
| process.stdin.on('end', () => { | ||
| this.shutdown(0, 'stdin end'); | ||
| }); | ||
|
|
||
| process.stdin.on('close', () => { | ||
| this.shutdown(0, 'stdin close'); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the specific lines in question
head -n 310 src/index.ts | tail -n +280 | cat -nRepository: kimsungwhee/apple-docs-mcp
Length of output: 996
🏁 Script executed:
# Get more context around the shutdown setup
head -n 350 src/index.ts | tail -n +270 | cat -nRepository: kimsungwhee/apple-docs-mcp
Length of output: 2740
🏁 Script executed:
# Check the class structure and constructor
head -n 100 src/index.ts | cat -nRepository: kimsungwhee/apple-docs-mcp
Length of output: 4313
🏁 Script executed:
# Check if this class is instantiated multiple times in tests or elsewhere
rg -n "new\s+\w+Server\|new\s+\w+App\|new\s+\w+Manager" --type ts --type tsxRepository: kimsungwhee/apple-docs-mcp
Length of output: 98
🏁 Script executed:
# Look for any test files that might instantiate this class multiple times
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx | head -20Repository: kimsungwhee/apple-docs-mcp
Length of output: 845
🏁 Script executed:
# Check the export and how the class is used
rg -n "export.*class|class.*{" src/index.ts -A 3Repository: kimsungwhee/apple-docs-mcp
Length of output: 178
🏁 Script executed:
# Check basic test file
head -n 100 tests/basic.test.ts | cat -nRepository: kimsungwhee/apple-docs-mcp
Length of output: 1109
🏁 Script executed:
# Check index test file
head -n 100 tests/index.test.ts | cat -nRepository: kimsungwhee/apple-docs-mcp
Length of output: 3701
🏁 Script executed:
# Search for any test setup that creates multiple instances
rg -n "new AppleDeveloperDocsMCPServer" --type tsRepository: kimsungwhee/apple-docs-mcp
Length of output: 913
🏁 Script executed:
# Check if there are any test fixtures or utilities that instantiate the server
rg -n "AppleDeveloperDocsMCPServer" --type ts -B 2 -A 2Repository: kimsungwhee/apple-docs-mcp
Length of output: 5333
Use one-shot handlers for process/stdin shutdown events to prevent listener accumulation in tests.
The listeners at lines 284-298 are registered with .on(...) in the constructor. Since this class is instantiated multiple times in test suites (beforeEach patterns in tests/index.test.ts, tests/response-format.test.ts, and regression tests), handlers accumulate on the global process and process.stdin objects even though isShuttingDown prevents duplicate exits. Switching to .once(...) is appropriate since these signals and stdin events should only trigger shutdown once per process lifecycle.
Suggested patch
- process.on('SIGINT', () => {
+ process.once('SIGINT', () => {
this.shutdown(0, 'SIGINT');
});
- process.on('SIGTERM', () => {
+ process.once('SIGTERM', () => {
this.shutdown(0, 'SIGTERM');
});
- process.stdin.on('end', () => {
+ process.stdin.once('end', () => {
this.shutdown(0, 'stdin end');
});
- process.stdin.on('close', () => {
+ process.stdin.once('close', () => {
this.shutdown(0, 'stdin close');
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| process.on('SIGINT', () => { | |
| process.exit(0); | |
| this.shutdown(0, 'SIGINT'); | |
| }); | |
| process.on('SIGTERM', () => { | |
| process.exit(0); | |
| this.shutdown(0, 'SIGTERM'); | |
| }); | |
| process.stdin.on('end', () => { | |
| this.shutdown(0, 'stdin end'); | |
| }); | |
| process.stdin.on('close', () => { | |
| this.shutdown(0, 'stdin close'); | |
| }); | |
| process.once('SIGINT', () => { | |
| this.shutdown(0, 'SIGINT'); | |
| }); | |
| process.once('SIGTERM', () => { | |
| this.shutdown(0, 'SIGTERM'); | |
| }); | |
| process.stdin.once('end', () => { | |
| this.shutdown(0, 'stdin end'); | |
| }); | |
| process.stdin.once('close', () => { | |
| this.shutdown(0, 'stdin close'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` around lines 284 - 298, Replace the persistent listeners with
one-shot handlers: change the process and stdin event registrations that
currently call process.on(...) for 'SIGINT', 'SIGTERM', and the 'end'/'close'
listeners on process.stdin to use .once(...) so each event invokes the class's
shutdown(...) handler only once per process lifecycle; update the registrations
around the shutdown(...) references (the SIGINT, SIGTERM, process.stdin 'end'
and 'close' handlers) accordingly to prevent listener accumulation during
repeated test instantiation.
1 participant
Summary
Why
In stdio MCP hosts, parent/client disconnect can close stdin without delivering a signal. Without stdin close handling, apple-docs-mcp can remain orphaned and accumulate over time.
Verification
Summary by CodeRabbit
Bug Fixes
Refactor