From b2d51fddbe9f777af5e51be506bb43de9eeb3d16 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 10 Sep 2025 14:30:23 +0200 Subject: [PATCH 1/7] fix(mongodb-runner): make close operation more resilient 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: https://github.com/mongodb-js/devtools-shared/issues/483 --- .../mongodb-runner/src/mongocluster.spec.ts | 6 +- packages/mongodb-runner/src/mongoserver.ts | 79 ++++++++++++++++--- packages/mongodb-runner/src/runner-helpers.ts | 15 ++-- packages/mongodb-runner/src/util.ts | 13 +++ 4 files changed, 87 insertions(+), 26 deletions(-) diff --git a/packages/mongodb-runner/src/mongocluster.spec.ts b/packages/mongodb-runner/src/mongocluster.spec.ts index 841eed06..7ae0c4a5 100644 --- a/packages/mongodb-runner/src/mongocluster.spec.ts +++ b/packages/mongodb-runner/src/mongocluster.spec.ts @@ -258,10 +258,10 @@ describe('MongoCluster', function () { secondaries: 0, }); cluster = await MongoCluster.deserialize(cluster.serialize()); - const { ok } = await cluster.withClient(async (client) => { - return await client.db('admin').command({ ping: 1 }); + const doc = await cluster.withClient(async (client) => { + return await client.db('admin').collection('mongodbrunner').findOne(); }); - expect(ok).to.equal(1); + expect(doc?._id).to.be.a('string'); await cluster.close(); }); }); diff --git a/packages/mongodb-runner/src/mongoserver.ts b/packages/mongodb-runner/src/mongoserver.ts index 505d6184..5a267d6f 100644 --- a/packages/mongodb-runner/src/mongoserver.ts +++ b/packages/mongodb-runner/src/mongoserver.ts @@ -13,7 +13,7 @@ import type { Document } from 'mongodb'; import { MongoClient } from 'mongodb'; import path from 'path'; import { once } from 'events'; -import { uuid, debug } from './util'; +import { uuid, debug, pick } from './util'; export interface MongoServerOptions { binDir?: string; @@ -24,32 +24,49 @@ export interface MongoServerOptions { docker?: string | string[]; // Image or docker options } +interface SerializedServerProperties { + _id: string; + pid?: number; + port?: number; + dbPath?: string; + startTime: string; +} + export class MongoServer { + private uuid: string = uuid(); private buildInfo?: Document; private childProcess?: ChildProcess; private pid?: number; private port?: number; private dbPath?: string; private closing = false; + private startTime = new Date().toISOString(); private constructor() { /* see .start() */ } - serialize(): unknown /* JSON-serializable */ { + serialize(): SerializedServerProperties { return { + _id: this.uuid, pid: this.pid, port: this.port, dbPath: this.dbPath, + startTime: this.startTime, }; } - static async deserialize(serialized: any): Promise { + static async deserialize( + serialized: SerializedServerProperties, + ): Promise { const srv = new MongoServer(); - srv.pid = serialized.pid; + srv.uuid = serialized._id; srv.port = serialized.port; - srv.dbPath = serialized.dbPath; - await srv._populateBuildInfo(); + srv.closing = !!(await srv._populateBuildInfo('restore-check')); + if (!srv.closing) { + srv.pid = serialized.pid; + srv.dbPath = serialized.dbPath; + } return srv; } @@ -116,7 +133,7 @@ export class MongoServer { const srv = new MongoServer(); if (!options.docker) { - const dbPath = path.join(options.tmpDir, `db-${uuid()}`); + const dbPath = path.join(options.tmpDir, `db-${srv.uuid}`); await fs.mkdir(dbPath, { recursive: true }); srv.dbPath = dbPath; } @@ -234,7 +251,10 @@ export class MongoServer { logEntryStream.resume(); srv.port = port; - await srv._populateBuildInfo(); + const buildInfoError = await srv._populateBuildInfo('insert-new'); + if (buildInfoError) { + throw buildInfoError; + } } catch (err) { await srv.close(); throw err; @@ -270,16 +290,49 @@ export class MongoServer { this.dbPath = undefined; } - private async _populateBuildInfo(): Promise { - if (this.buildInfo?.version) return; - this.buildInfo = await this.withClient( - async (client) => await client.db('admin').command({ buildInfo: 1 }), - ); + private async _populateBuildInfo( + mode: 'insert-new' | 'restore-check', + ): Promise { + if (this.buildInfo?.version) return null; + try { + this.buildInfo = await this.withClient(async (client) => { + const admin = client.db('admin'); + const coll = + admin.collection('mongodbrunner'); + const insertedInfo = pick(this.serialize(), [ + '_id', + 'pid', + 'port', + 'dbPath', + 'startTime', + ]); + if (mode === 'insert-new') { + await coll.insertOne(insertedInfo); + } else { + const match = await coll.findOne(); + if (!match) { + throw new Error( + 'Cannot find mongodbrunner entry, assuming that this instance was not started by mongodb-runner', + ); + } + if (match._id !== insertedInfo._id) { + throw new Error( + `Mismatched mongodbrunner entry: ${JSON.stringify(match)} !== ${JSON.stringify(insertedInfo)}`, + ); + } + } + return await admin.command({ buildInfo: 1 }); + }); + } catch (err) { + debug('failed to get buildInfo, treating as closed server', err); + return err as Error; + } debug( 'got server build info through client', this.serverVersion, this.serverVariant, ); + return null; } async withClient any>( diff --git a/packages/mongodb-runner/src/runner-helpers.ts b/packages/mongodb-runner/src/runner-helpers.ts index 4181b3eb..55c30913 100644 --- a/packages/mongodb-runner/src/runner-helpers.ts +++ b/packages/mongodb-runner/src/runner-helpers.ts @@ -84,14 +84,9 @@ export async function stop(argv: { id?: string; all?: boolean; }) { - const toStop: Array = []; - for await (const instance of instances(argv)) { - if (instance.id === argv.id || argv.all) toStop.push(instance); - } - await Promise.all( - toStop.map(async ({ filepath, serialized }) => { - await (await MongoCluster.deserialize(serialized)).close(); - await fs.rm(filepath); - }), - ); + await parallelForEach(instances(argv), async (instance: StoredInstance) => { + if (instance.id !== argv.id && !argv.all) return; + await (await MongoCluster.deserialize(instance.serialized)).close(); + await fs.rm(instance.filepath); + }); } diff --git a/packages/mongodb-runner/src/util.ts b/packages/mongodb-runner/src/util.ts index 8c1e36f6..bda4d3de 100644 --- a/packages/mongodb-runner/src/util.ts +++ b/packages/mongodb-runner/src/util.ts @@ -28,3 +28,16 @@ export async function parallelForEach( return await Promise.allSettled(result); } + +export function pick( + obj: T, + keys: K[], +): Pick { + const ret: Partial> = {}; + for (const key of Object.keys(obj) as K[]) { + if (keys.includes(key)) { + ret[key] = obj[key]; + } + } + return ret as Pick; +} From 6670bfc507a81beabf2f8fe94288a808c3d05bd3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 10 Sep 2025 15:51:21 +0200 Subject: [PATCH 2/7] fixup: account for mongos when inserting --- .../mongodb-runner/src/mongocluster.spec.ts | 2 +- packages/mongodb-runner/src/mongoserver.ts | 63 +++++++++++-------- 2 files changed, 38 insertions(+), 27 deletions(-) diff --git a/packages/mongodb-runner/src/mongocluster.spec.ts b/packages/mongodb-runner/src/mongocluster.spec.ts index 7ae0c4a5..156b3ea7 100644 --- a/packages/mongodb-runner/src/mongocluster.spec.ts +++ b/packages/mongodb-runner/src/mongocluster.spec.ts @@ -259,7 +259,7 @@ describe('MongoCluster', function () { }); cluster = await MongoCluster.deserialize(cluster.serialize()); const doc = await cluster.withClient(async (client) => { - return await client.db('admin').collection('mongodbrunner').findOne(); + return await client.db('config').collection('mongodbrunner').findOne(); }); expect(doc?._id).to.be.a('string'); await cluster.close(); diff --git a/packages/mongodb-runner/src/mongoserver.ts b/packages/mongodb-runner/src/mongoserver.ts index 5a267d6f..9ed1bbb2 100644 --- a/packages/mongodb-runner/src/mongoserver.ts +++ b/packages/mongodb-runner/src/mongoserver.ts @@ -290,38 +290,49 @@ export class MongoServer { this.dbPath = undefined; } + private async _ensureMatchingMetadataColl( + client: MongoClient, + mode: 'insert-new' | 'restore-check', + ): Promise { + const hello = await client.db('admin').command({ hello: 1 }); + const isMongoS = hello.msg === 'isdbgrid'; + const insertedInfo = pick(this.serialize(), [ + '_id', + 'pid', + 'port', + 'dbPath', + 'startTime', + ]); + const runnerColl = client + .db(isMongoS ? 'config' : 'local') + .collection('mongodbrunner'); + if (mode === 'insert-new') { + await runnerColl.insertOne(insertedInfo); + } else { + const match = await runnerColl.findOne(); + if (!match) { + throw new Error( + 'Cannot find mongodbrunner entry, assuming that this instance was not started by mongodb-runner', + ); + } + if (match._id !== insertedInfo._id) { + throw new Error( + `Mismatched mongodbrunner entry: ${JSON.stringify(match)} !== ${JSON.stringify(insertedInfo)}`, + ); + } + } + } + private async _populateBuildInfo( mode: 'insert-new' | 'restore-check', ): Promise { if (this.buildInfo?.version) return null; try { this.buildInfo = await this.withClient(async (client) => { - const admin = client.db('admin'); - const coll = - admin.collection('mongodbrunner'); - const insertedInfo = pick(this.serialize(), [ - '_id', - 'pid', - 'port', - 'dbPath', - 'startTime', - ]); - if (mode === 'insert-new') { - await coll.insertOne(insertedInfo); - } else { - const match = await coll.findOne(); - if (!match) { - throw new Error( - 'Cannot find mongodbrunner entry, assuming that this instance was not started by mongodb-runner', - ); - } - if (match._id !== insertedInfo._id) { - throw new Error( - `Mismatched mongodbrunner entry: ${JSON.stringify(match)} !== ${JSON.stringify(insertedInfo)}`, - ); - } - } - return await admin.command({ buildInfo: 1 }); + // Insert the metadata entry, except if we're a freshly started mongos + // (which does not have its own storage to persist) + await this._ensureMatchingMetadataColl(client, mode); + return await client.db('admin').command({ buildInfo: 1 }); }); } catch (err) { debug('failed to get buildInfo, treating as closed server', err); From bd016d8afe0dca8ce6cb9430cce0ee9b6ab161d3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 10 Sep 2025 17:09:24 +0200 Subject: [PATCH 3/7] fixup: retryWrites=false --- packages/mongodb-runner/src/mongoserver.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/mongodb-runner/src/mongoserver.ts b/packages/mongodb-runner/src/mongoserver.ts index 9ed1bbb2..33b12b6c 100644 --- a/packages/mongodb-runner/src/mongoserver.ts +++ b/packages/mongodb-runner/src/mongoserver.ts @@ -350,7 +350,8 @@ export class MongoServer { fn: Fn, ): Promise> { 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`, ); try { return await fn(client); From 217cb6f4c394d0425a51eeab3b1f60b77d02d5c7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 10 Sep 2025 17:58:40 +0200 Subject: [PATCH 4/7] fixup: improve debugging --- packages/mongodb-runner/src/mongoserver.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/mongodb-runner/src/mongoserver.ts b/packages/mongodb-runner/src/mongoserver.ts index 33b12b6c..482ab89c 100644 --- a/packages/mongodb-runner/src/mongoserver.ts +++ b/packages/mongodb-runner/src/mongoserver.ts @@ -306,10 +306,13 @@ export class MongoServer { const runnerColl = client .db(isMongoS ? 'config' : 'local') .collection('mongodbrunner'); + debug('ensuring metadata collection entry', insertedInfo, { isMongoS }); if (mode === 'insert-new') { await runnerColl.insertOne(insertedInfo); + debug('inserted metadata collection entry', insertedInfo); } else { const match = await runnerColl.findOne(); + debug('read metadata collection entry', insertedInfo, match); if (!match) { throw new Error( 'Cannot find mongodbrunner entry, assuming that this instance was not started by mongodb-runner', From 5514654ba00ebe6888dd8a1db150656e28f82014 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 10 Sep 2025 18:07:11 +0200 Subject: [PATCH 5/7] fixup: do not short-circuit when buildInfo has been read from the logs --- packages/mongodb-runner/src/mongoserver.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/mongodb-runner/src/mongoserver.ts b/packages/mongodb-runner/src/mongoserver.ts index 482ab89c..cd71ba5b 100644 --- a/packages/mongodb-runner/src/mongoserver.ts +++ b/packages/mongodb-runner/src/mongoserver.ts @@ -329,7 +329,6 @@ export class MongoServer { private async _populateBuildInfo( mode: 'insert-new' | 'restore-check', ): Promise { - if (this.buildInfo?.version) return null; try { this.buildInfo = await this.withClient(async (client) => { // Insert the metadata entry, except if we're a freshly started mongos From 78268cdaba17fc8fd182c16d3ef8aaee390ee5b4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 11 Sep 2025 02:08:40 +0200 Subject: [PATCH 6/7] fixup: allow for failed metadata insertions --- packages/mongodb-runner/src/mongoserver.ts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/mongodb-runner/src/mongoserver.ts b/packages/mongodb-runner/src/mongoserver.ts index cd71ba5b..9b6e5647 100644 --- a/packages/mongodb-runner/src/mongoserver.ts +++ b/packages/mongodb-runner/src/mongoserver.ts @@ -30,6 +30,7 @@ interface SerializedServerProperties { port?: number; dbPath?: string; startTime: string; + hasInsertedMetadataCollEntry: boolean; } export class MongoServer { @@ -41,6 +42,7 @@ export class MongoServer { private dbPath?: string; private closing = false; private startTime = new Date().toISOString(); + private hasInsertedMetadataCollEntry = false; private constructor() { /* see .start() */ @@ -53,6 +55,7 @@ export class MongoServer { port: this.port, dbPath: this.dbPath, startTime: this.startTime, + hasInsertedMetadataCollEntry: this.hasInsertedMetadataCollEntry, }; } @@ -253,7 +256,7 @@ export class MongoServer { srv.port = port; const buildInfoError = await srv._populateBuildInfo('insert-new'); if (buildInfoError) { - throw buildInfoError; + debug('failed to get buildInfo', buildInfoError); } } catch (err) { await srv.close(); @@ -305,12 +308,21 @@ export class MongoServer { ]); const runnerColl = client .db(isMongoS ? 'config' : 'local') - .collection('mongodbrunner'); + .collection< + Omit + >('mongodbrunner'); debug('ensuring metadata collection entry', insertedInfo, { isMongoS }); if (mode === 'insert-new') { await runnerColl.insertOne(insertedInfo); debug('inserted metadata collection entry', insertedInfo); + this.hasInsertedMetadataCollEntry = true; } else { + if (!this.hasInsertedMetadataCollEntry) { + debug( + 'skipping metadata collection match check as we never inserted metadata', + ); + return; + } const match = await runnerColl.findOne(); debug('read metadata collection entry', insertedInfo, match); if (!match) { From 81862a62ea053a8275f3880ffbe179a76f4393e4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 11 Sep 2025 13:53:39 +0200 Subject: [PATCH 7/7] fixup: only set retryWrites=false for internal client --- packages/mongodb-runner/src/mongoserver.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/packages/mongodb-runner/src/mongoserver.ts b/packages/mongodb-runner/src/mongoserver.ts index 9b6e5647..234a8513 100644 --- a/packages/mongodb-runner/src/mongoserver.ts +++ b/packages/mongodb-runner/src/mongoserver.ts @@ -9,7 +9,7 @@ import { isFailureToSetupListener, } from './mongologreader'; import { Readable } from 'stream'; -import type { Document } from 'mongodb'; +import type { Document, MongoClientOptions } from 'mongodb'; import { MongoClient } from 'mongodb'; import path from 'path'; import { once } from 'events'; @@ -342,12 +342,14 @@ export class MongoServer { mode: 'insert-new' | 'restore-check', ): Promise { try { + // directConnection + retryWrites let us write to `local` db on secondaries + const clientOpts = { retryWrites: false }; this.buildInfo = await this.withClient(async (client) => { // Insert the metadata entry, except if we're a freshly started mongos // (which does not have its own storage to persist) await this._ensureMatchingMetadataColl(client, mode); return await client.db('admin').command({ buildInfo: 1 }); - }); + }, clientOpts); } catch (err) { debug('failed to get buildInfo, treating as closed server', err); return err as Error; @@ -362,11 +364,12 @@ export class MongoServer { async withClient any>( fn: Fn, + clientOptions: MongoClientOptions = {}, ): Promise> { - const client = await MongoClient.connect( - // directConnection + retryWrites let us write to `local` db on secondaries - `mongodb://${this.hostport}/?directConnection=true&retryWrites=false`, - ); + const client = await MongoClient.connect(`mongodb://${this.hostport}/`, { + directConnection: true, + ...clientOptions, + }); try { return await fn(client); } finally {