From df41dba661d18467273330bd751479605e56e0ce Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Wed, 11 Dec 2024 16:54:49 -0500
Subject: [PATCH] fix(cli-repl): only ever run `MongoshRepl.onExit` once

Previously, a task in our CI was flaky because it was running `exit\n`
as literal input to the shell, expecting it to reliably make the shell exit.

`exit` as a command leads to the `MongoshRepl.onExit` function called,
which then calls a) `.close()` and, indirectly, `repl.close()` on the
Node.js REPL instance as well b) the `CliRepl.exit` function (and through
that, indirectly, `process.exit()`).

However, closing the Node.js REPL instance makes it flush the history
file to disk as a typically fast but asynchronous operation, and,
after that, emit `exit` on the REPL; that in turn called
`MongoshRepl.onExit` again (i.e. calling `MongoshRepl.onExit` leads
to another call to the same function, just asynchronously with
a short delay).

The flaky test in CI failed because of a combination of issues,
including the fact that `process.exit()` did not necessarily actually
exit the process because of coverage tooling interfering with it
(which in turn happened because it was running with an altered
working directory pointing at a temporary directory where `nyc`
had not created a subdirectory for storing coverage output).

In typical operations, the REPL would flush history quickly, and
calling `.onExit()` again would allow the process to exit
normally (by ways of calling `CliRepl.exit` and `process.exit()`
a second time). However, in the flaky failure case, the history
flushing operation would take long enough that the original
`process.exit()` call (which failed due to nyc) would set the
internal `process._exiting` flag before throwing, which in turn
prevents `process.nextTick()` from scheduling callbacks, and which
then ultimately prevented the REPL from cleaning itself up,
leading to the process timing out instead of exiting.

This commit introduces safeguards against calling `.onExit()` twice,
always returning the same (never-resolving) `Promise` instance,
and explicitly keeps track of whether the REPL was closed or not.
---
 packages/cli-repl/src/mongosh-repl.ts | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/packages/cli-repl/src/mongosh-repl.ts b/packages/cli-repl/src/mongosh-repl.ts
index 27813995d..883317b5e 100644
--- a/packages/cli-repl/src/mongosh-repl.ts
+++ b/packages/cli-repl/src/mongosh-repl.ts
@@ -127,6 +127,7 @@ type Mutable<T> = {
 class MongoshNodeRepl implements EvaluationListener {
   _runtimeState: MongoshRuntimeState | null;
   closeTrace?: string;
+  exitPromise?: Promise<never>;
   input: Readable;
   lineByLineInput: LineByLineInput;
   output: Writable;
@@ -545,6 +546,7 @@ class MongoshNodeRepl implements EvaluationListener {
     });
 
     repl.on('exit', () => {
+      if (this._runtimeState) this._runtimeState.repl = null;
       this.onExit().catch(() => {
         /* ... */
       });
@@ -1051,7 +1053,10 @@ class MongoshNodeRepl implements EvaluationListener {
     if (rs) {
       this._runtimeState = null;
       this.closeTrace = new Error().stack;
-      rs.repl?.close();
+      if (rs.repl) {
+        rs.repl.close();
+        await once(rs.repl, 'exit');
+      }
       await rs.instanceState.close(true);
       await new Promise((resolve) => this.output.write('', resolve));
     }
@@ -1063,8 +1068,10 @@ class MongoshNodeRepl implements EvaluationListener {
    * @param exitCode The user-specified exit code, if any.
    */
   async onExit(exitCode?: number): Promise<never> {
-    await this.close();
-    return this.ioProvider.exit(exitCode);
+    return (this.exitPromise ??= (async () => {
+      await this.close();
+      return this.ioProvider.exit(exitCode);
+    })());
   }
 
   /**