-
Notifications
You must be signed in to change notification settings - Fork 72
chore: revoke access tokens on server shutdown [MCP-53] #352
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.
Pull Request Overview
Adds explicit revocation of Atlas access tokens during shutdown and cleans up duplicated shutdown calls.
- Introduce
apiClient.close()
to revoke tokens inApiClient
- Invoke
apiClient.close()
inSession.close()
- Remove duplicate
mcpServer.close()
and adjust shutdown order inServer.close()
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/common/atlas/apiClient.ts | Added a close() method to revoke stored access tokens |
src/session.ts | Called apiClient.close() after disconnect() in Session.close() |
src/server.ts | Moved and deduplicated mcpServer.close() in Server.close() |
Comments suppressed due to low confidence (2)
src/common/atlas/apiClient.ts:98
- The new
close()
method isn’t covered by tests yet. Adding unit tests to verify thataccessToken.revokeAll()
is called when an access token is present would help ensure correct behavior.
public async close(): Promise<void> {
src/server.ts:104
- [nitpick] Closing the MCP server before the session could terminate active sessions prematurely. Consider invoking
session.close()
beforemcpServer.close()
or add a comment explaining the intended shutdown sequence.
await this.mcpServer.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.
LGTM
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.
LGTM, but are atlas tests supposed to fail?
Pull Request Test Coverage Report for Build 16199405806Details
💛 - Coveralls |
Proposed changes
revoke atlas access tokens on server shutdown [MCP-53]
Checklist