Skip to content

Conversation

@fmenezes
Copy link
Collaborator

@fmenezes fmenezes commented Nov 10, 2025

Proposed changes

sometimes atlas connect tool would create multiple users instead of one, this fixes it.

Checklist

@fmenezes fmenezes marked this pull request as ready for review November 10, 2025 18:26
@fmenezes fmenezes requested a review from a team as a code owner November 10, 2025 18:26
Copilot AI review requested due to automatic review settings November 10, 2025 18:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug where the atlas-connect-cluster tool could create multiple database users instead of just one. The fix refactors the connection state logic to check and prepare the cluster connection only once before the polling loop, preventing repeated calls to prepareClusterConnection (which creates users) during connection retries.

Key changes:

  • Moved cluster connection preparation logic outside the polling loop
  • Added initial connection state check before the retry loop begins
  • Removed duplicate connection logic from within the polling loop

@coveralls
Copy link
Collaborator

coveralls commented Nov 10, 2025

Pull Request Test Coverage Report for Build 19293756954

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 22 of 30 (73.33%) changed or added relevant lines in 1 file are covered.
  • 11 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 80.169%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/tools/atlas/connect/connectCluster.ts 22 30 73.33%
Files with Coverage Reduction New Missed Lines %
src/tools/atlas/connect/connectCluster.ts 11 73.73%
Totals Coverage Status
Change from base Build 19272242315: -0.2%
Covered Lines: 6488
Relevant Lines: 7980

💛 - Coveralls

beforeAll(async () => {
let connected = false;
for (let i = 0; i < 10; i++) {
const response = await integration.mcpClient().callTool({
Copy link
Collaborator

Choose a reason for hiding this comment

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

[q] why do we need to call connect several times, shouldn't we connect and wait a considerable amount of time and that should be enough?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the edge case where atlas can take a long time to provision the DB user to data plane, in which case we need to keep waiting

Copy link
Collaborator

@blva blva left a comment

Choose a reason for hiding this comment

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

non-blocking comments, thanks for the fix and coverage!

Co-authored-by: Bianca Lisle <40155621+blva@users.noreply.github.com>
@fmenezes fmenezes enabled auto-merge (squash) November 12, 2025 10:08
@fmenezes fmenezes merged commit 2d1586a into main Nov 12, 2025
23 of 25 checks passed
@fmenezes fmenezes deleted the MCP-280 branch November 12, 2025 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants