-
Notifications
You must be signed in to change notification settings - Fork 0
fix: harden MCP HTTP service lifecycle in Obsidian #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f511e2d6cf
ℹ️ 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".
packages/mcp/src/httpServer.ts
Outdated
| await new Promise<void>((resolve) => httpServer.close(() => resolve())); | ||
| await Promise.allSettled( | ||
| Array.from(sessions.values()).flatMap((s) => [s.transport.close(), s.server.close()]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Close MCP sessions before shutting down the HTTP server
When /__ailss/shutdown is hit, close() waits on httpServer.close() before closing transports. httpServer.close() blocks until all active connections end, but streamable MCP requests can keep SSE/streaming connections open; since s.transport.close() only runs after httpServer.close() resolves, the shutdown can hang indefinitely and the port never frees. Consider closing sessions/transports first (or using closeAllConnections/closeIdleConnections) so active streaming connections are terminated and shutdown reliably completes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codex implemented fix, review changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcff485714
ℹ️ 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".
packages/mcp/src/httpServer.ts
Outdated
| const shutdownPathRaw = options.shutdown?.path ?? "/__ailss/shutdown"; | ||
| const shutdownPath = shutdownPathRaw.startsWith("/") ? shutdownPathRaw : `/${shutdownPathRaw}`; | ||
| const exitProcessOnShutdown = options.shutdown?.exitProcess ?? false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gate shutdown endpoint behind an explicit opt-in
Because shutdownPathRaw is always defaulted, the shutdown route is effectively enabled for every startAilssMcpHttpServer caller, even when options.shutdown is omitted. That means any authenticated MCP client (i.e., anyone with the normal request token) can now terminate the server, which is a new availability/DoS surface in multi-client or shared-token setups. Consider making shutdown opt-in (e.g., require options.shutdown to be set) or requiring a separate admin token so regular clients cannot stop the service.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codex implemented fix, review code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codex Review: Didn't find any major issues. Hooray!
ℹ️ 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".
What
Why
How