fix: 🐛 clean up sessions tracking in session manager (#3150)

* fix: 🐛 clean up sessions tracking in session manager

* refactor: 💡 PR feedback

* fix: 🐛 add onClose event listener to clean up sessions

* refactor: 💡 update to async/await

* refactor: 💡 move close event after spawn

* refactor: 💡 move onClose session handler to spawn onClose

ui/desktop/electron-app/src/helpers/spawn-promise.js

@@ -5,13 +5,12 @@
 
 const { path: boundaryPath } = require('../cli/path.js');
 const { spawn, spawnSync } = require('child_process');
-const jsonify = require('../utils/jsonify.js');
 
 // You can throw exceptions, or allow them to occur, and this is supported.
 // Exceptions thrown in this way will be returned to the UI as an
 // IPC rejection. However in most cases, you probably want to return a
 // specific exception object of your own design, since Error instances
-// originating from the underlying system will be noisey and not very helpful
+// originating from the underlying system will be noisy and not very helpful
 // to users.
 //
 //throw new Error('Random error!  Should still work!');
@@ -22,57 +21,6 @@ const jsonify = require('../utils/jsonify.js');
 // helpful message for users, so it's recommended to craft nice
 // POJO representation and throw it or promise->reject it.
 module.exports = {
-  /**
-   * Spawns an asynchronous child process that is expected to output JSON
-   * data on either stdout or stderr.  The process is allowed to continue
-   * running after the promise resolves.  This function is intended to launch
-   * the local proxy.
-   * @param {string[]} command
-   * @param {string} token
-   * @param {number} timeout Duration in seconds
-   * @return {Promise}
-   */
-  spawnAsyncJSONPromise(command, token, timeout) {
-    return new Promise((resolve, reject) => {
-      const childProcess = spawn(boundaryPath, command, {
-        env: {
-          ...process.env,
-          BOUNDARY_TOKEN: token,
-        },
-        timeout: timeout ? timeout * 1000 : undefined,
-      });
-      let outputStream = '';
-      let errorStream = '';
-
-      const processData = (data) => {
-        outputStream += data.toString();
-        const jsonData = jsonify(outputStream);
-        if (jsonData) {
-          childProcess.removeAllListeners();
-          resolve({ childProcess, response: jsonData });
-        }
-      };
-
-      childProcess.stdout.on('data', processData);
-
-      // Errors are cli returned json objects
-      const processError = (error) => {
-        errorStream += error.toString();
-        const jsonData = jsonify(errorStream);
-        if (jsonData) {
-          const error = jsonData.api_error || jsonData.error;
-          childProcess.removeAllListeners();
-          reject(new Error(error.message));
-        }
-      };
-
-      childProcess.stderr.on('error', processError);
-      childProcess.stderr.on('data', processError);
-      // Capture spawn errors
-      childProcess.on('error', (error) => reject(error));
-    });
-  },
-
   /**
    * Spawn child process and return output immediately.
    * This function is intended for non-connection related tasks.
@@ -107,8 +55,9 @@ module.exports = {
    * @param path
    */
   spawn(command, options, path = boundaryPath) {
+    const { onClose, ...spawnOptions } = options;
     return new Promise((resolve, reject) => {
-      const childProcess = spawn(path, command, options);
+      const childProcess = spawn(path, command, spawnOptions);
       childProcess.stdout.on('data', (data) => {
         resolve({ childProcess, stdout: data.toString() });
       });
@@ -123,12 +72,13 @@ module.exports = {
       // a timeout occurs so this guarantees we return a response to the caller.
       // Otherwise this should not get hit as we should be returning a response
       // from one of the handlers above.
-      childProcess.on('close', () =>
+      childProcess.on('close', () => {
+        onClose?.();
         resolve({
           childProcess,
           stderr: JSON.stringify({ error: 'Process was closed.' }),
-        }),
-      );
+        });
+      });
     });
   },
 };

ui/desktop/electron-app/src/models/session-manager.js

@@ -6,14 +6,16 @@
 const Session = require('./session.js');
 
 class SessionManager {
-  #sessions = [];
+  #sessions = new Map();
 
   /**
    * Checks for running sessions
    * @returns {boolean}
    */
   get hasRunningSessions() {
-    return Boolean(this.#sessions.find((session) => session.isRunning));
+    return Boolean(
+      this.#sessions.values().find((session) => session.isRunning),
+    );
   }
 
   /**
@@ -25,25 +27,31 @@ class SessionManager {
    * @param {string} host_id
    * @param {number} session_max_seconds
    */
-  start(addr, target_id, token, host_id, session_max_seconds) {
+  async start(addr, target_id, token, host_id, session_max_seconds) {
     const session = new Session(
       addr,
       target_id,
       token,
       host_id,
       session_max_seconds,
+      (sessionId) => this.#sessions.delete(sessionId),
     );
-    this.#sessions.push(session);
-    return session.start();
+    const sessionDetails = await session.start();
+    // Store session by id for tracking and stopping later
+    // Needs to be done after session.start() resolves
+    // since session id is generated in start()
+    this.#sessions.set(session.id, session);
+    return sessionDetails;
   }
 
   /**
-   * Stop a session using identifier.
+   * Stop a session using identifier
+   * and remove it from tracking.
    * @param {string} session_id
    */
   stopById(session_id) {
-    const session = this.#sessions.find((session) => session.id === session_id);
-    return session?.stop?.();
+    const session = this.#sessions.get(session_id);
+    return session?.stop();
   }
 
   /**
@@ -52,16 +60,19 @@ class SessionManager {
    * @returns {Session} The session object
    */
   getSessionById(sessionId) {
-    return this.#sessions.find((session) => session.id === sessionId);
+    return this.#sessions.get(sessionId);
   }
 
   /**
    * Stop all active and pending target sessions
    * Returning Promise.all() ensures all sessions in the list have been
    * stopped before calling the next fn
+   * along with clearing the sessions list.
    */
   stopAll() {
-    return Promise.all(this.#sessions.map((session) => session.stop()));
+    return Promise.all(
+      this.#sessions.values().map((session) => session.stop()),
+    );
   }
 }
 

ui/desktop/electron-app/src/models/session.js

@@ -4,11 +4,9 @@
  */
 
 const sanitizer = require('../utils/sanitizer.js');
-const {
-  spawnAsyncJSONPromise,
-  spawnSync,
-} = require('../helpers/spawn-promise.js');
+const { spawnSync, spawn } = require('../helpers/spawn-promise.js');
 const log = require('electron-log/main');
+const jsonify = require('../utils/jsonify.js');
 
 class Session {
   #id;
@@ -19,6 +17,7 @@ class Session {
   #targetId;
   #proxyDetails;
   #sessionMaxSeconds;
+  #onClose;
 
   /**
    * Initialize a session to a controller address
@@ -28,13 +27,15 @@ class Session {
    * @param {string} token
    * @param {string} hostId
    * @param {number} sessionMaxSeconds
+   * @param {function} onClose
    */
-  constructor(addr, targetId, token, hostId, sessionMaxSeconds) {
+  constructor(addr, targetId, token, hostId, sessionMaxSeconds, onClose) {
     this.#addr = addr;
     this.#targetId = targetId;
     this.#token = token;
     this.#hostId = hostId;
     this.#sessionMaxSeconds = sessionMaxSeconds;
+    this.#onClose = onClose;
   }
 
   /**
@@ -93,18 +94,37 @@ class Session {
    * Using cli, initialize a session to a target.
    * Tracks local proxy details if successful.
    */
-  start() {
+  async start() {
     const sanitizedToken = sanitizer.base62EscapeAndValidate(this.#token);
-    return spawnAsyncJSONPromise(
+    const options = {
+      env: {
+        ...process.env,
+        BOUNDARY_TOKEN: sanitizedToken,
+      },
+      timeout: this.#sessionMaxSeconds
+        ? this.#sessionMaxSeconds * 1000
+        : undefined,
+      onClose: () => this.#onClose(this.#id),
+    };
+    const { childProcess, stdout, stderr } = await spawn(
       this.connectCommand,
-      sanitizedToken,
-      this.#sessionMaxSeconds,
-    ).then((spawnedSession) => {
-      this.#process = spawnedSession.childProcess;
-      this.#proxyDetails = spawnedSession.response;
-      this.#id = this.#proxyDetails.session_id;
-      return this.#proxyDetails;
-    });
+      options,
+    );
+
+    if (stderr) {
+      const errorResponse = jsonify(stderr);
+      const error = errorResponse.api_error || errorResponse.error;
+      throw new Error(
+        error?.message ?? 'Unknown error occurred while starting session',
+      );
+    }
+
+    const response = jsonify(stdout);
+    this.#process = childProcess;
+    this.#proxyDetails = response;
+    this.#id = response.session_id;
+
+    return response;
   }
 
   /**