Skip to content

Conversation

addaleax
Copy link
Collaborator

Store identifying information about a server in a database, then only actually run process.kill() on servers where this identifying information matches what we expect it to match.

Also, do not kill processes to which we cannot connect, and instead assume they have already been closed before.

Fixes: #483

Description

Open Questions

Checklist

addaleax and others added 6 commits September 10, 2025 14:32
Store identifying information about a server in a database, then only actually
run `process.kill()` on servers where this identifying information matches
what we expect it to match.

Also, do not kill processes to which we cannot connect, and instead assume
they have already been closed before.

Fixes: #483
Copy link
Collaborator

@gagik gagik left a comment

Choose a reason for hiding this comment

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

Looks good, 1 Q.
Hoping this will help with some flakiness or at least make the tests fail quicker
ah on mongosh end, this will mainly affect smoke tests

const client = await MongoClient.connect(
`mongodb://${this.hostport}/?directConnection=true`,
// directConnection + retryWrites let us write to `local` db on secondaries
`mongodb://${this.hostport}/?directConnection=true&retryWrites=false`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

can retryWrites end up messing with our tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm ... how would it do that? I can't think of a place where we would even make use of this client. It is a public API, to be fair, but where we do use the withClient() feature, I'm pretty sure we do that on the MongoCluster level, not the MongoServer level

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah yeah, I was thinking that we were interfacing with it through this client somehow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gagik thought this over a bit and I think you're right, it's unlikely to cause trouble but there's also no good reason not to restrict retryWrites: false to where it's actually being used, so I limited the scope of that to the added code here in 81862a6

@addaleax addaleax merged commit 375d4e8 into main Sep 11, 2025
33 of 34 checks passed
@addaleax addaleax deleted the resilient-mongodbrunner-close branch September 11, 2025 13:05
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.

mongodb-runner doesn't kill all instances

2 participants