fix!: launchNode.cleanup not killing node in last test of test group (

#2718)

.changeset/breezy-tables-jam.md

@@ -0,0 +1,6 @@
+---
+"@fuel-ts/account": minor
+"fuels": minor
+---
+
+fix!: `launchNode.cleanup` not killing node in last test of test group

.knip.json

@@ -29,7 +29,6 @@
     "textlint",
     "textlint-rule-no-dead-link",
     "@elasticpath/textlint-rule-no-dead-relative-link",
-    "tree-kill",
     "ts-generator",
     "webdriverio"
   ]

packages/account/package.json

@@ -67,7 +67,6 @@
     "graphql-tag": "^2.12.6",
     "portfinder": "^1.0.32",
     "ramda": "^0.29.0",
-    "tree-kill": "^1.2.2",
     "uuid": "^10.0.0"
   },
   "devDependencies": {

packages/account/src/test-utils/launchNode-singular-test.test.ts

@@ -0,0 +1,22 @@
+import { launchNode } from './launchNode';
+
+/**
+ * The test runner creates a test environment per file,
+ * which we can use to isolate the faulty behavior.
+ */
+/**
+ * @group node
+ */
+describe('launchNode-singular-test', () => {
+  const killedNode = {
+    url: '',
+  };
+  afterEach(async () => {
+    await expect(fetch(killedNode.url)).rejects.toThrow('fetch failed');
+  });
+  test('synchronous cleanup kills node before test runner exits', async () => {
+    const { cleanup, url } = await launchNode({ loggingEnabled: false });
+    killedNode.url = url;
+    cleanup();
+  });
+});

packages/account/src/test-utils/launchNode.test.ts

@@ -1,14 +1,13 @@
 import { ErrorCode } from '@fuel-ts/errors';
 import { safeExec, expectToThrowFuelError } from '@fuel-ts/errors/test-utils';
-import { defaultSnapshotConfigs } from '@fuel-ts/utils';
+import { defaultSnapshotConfigs, sleep } from '@fuel-ts/utils';
 import { waitUntilUnreachable } from '@fuel-ts/utils/test-utils';
 import * as childProcessMod from 'child_process';
+import * as fsMod from 'fs';
 
 import { Provider } from '../providers';
 
-import { killNode, launchNode } from './launchNode';
-
-type ChildProcessWithoutNullStreams = childProcessMod.ChildProcessWithoutNullStreams;
+import { launchNode } from './launchNode';
 
 vi.mock('child_process', async () => {
   const mod = await vi.importActual('child_process');
@@ -18,6 +17,14 @@ vi.mock('child_process', async () => {
   };
 });
 
+vi.mock('fs', async () => {
+  const mod = await vi.importActual('fs');
+  return {
+    __esModule: true,
+    ...mod,
+  };
+});
+
 /**
  * @group node
  */
@@ -130,63 +137,96 @@ describe('launchNode', () => {
     cleanup();
   });
 
-  test('should kill process only if PID exists and node is alive', () => {
-    const killFn = vi.fn();
-    const state = { isDead: true };
-
-    // should not kill
-    let child = {
-      pid: undefined,
-      stdout: {
-        removeAllListeners: () => {},
-      },
-      stderr: {
-        removeAllListeners: () => {},
-      },
-    } as ChildProcessWithoutNullStreams;
-
-    killNode({
-      child,
-      configPath: '',
-      killFn,
-      state,
-    });
+  test('cleanup removes temporary directory', async () => {
+    const mkdirSyncSpy = vi.spyOn(fsMod, 'mkdirSync');
+    const { cleanup } = await launchNode();
 
-    expect(killFn).toHaveBeenCalledTimes(0);
-    expect(state.isDead).toEqual(true);
-
-    // should not kill
-    child = {
-      pid: 1,
-      stdout: {
-        removeAllListeners: () => {},
-      },
-      stderr: {
-        removeAllListeners: () => {},
-      },
-    } as ChildProcessWithoutNullStreams;
-
-    killNode({
-      child,
-      configPath: '',
-      killFn,
-      state,
-    });
+    expect(mkdirSyncSpy).toHaveBeenCalledTimes(1);
+    const tempDirPath = mkdirSyncSpy.mock.calls[0][0];
+    cleanup();
 
-    expect(killFn).toHaveBeenCalledTimes(0);
-    expect(state.isDead).toEqual(true);
+    // wait until cleanup finishes (done via events)
+    await sleep(1500);
+    expect(fsMod.existsSync(tempDirPath)).toBeFalsy();
+  });
 
-    // should kill
-    state.isDead = false;
+  test('temporary directory gets removed on error', async () => {
+    const mkdirSyncSpy = vi.spyOn(fsMod, 'mkdirSync');
 
-    killNode({
-      child,
-      configPath: '',
-      killFn,
-      state,
-    });
+    const invalidCoin = {
+      asset_id: 'whatever',
+      tx_id: '',
+      output_index: 0,
+      tx_pointer_block_height: 0,
+      tx_pointer_tx_idx: 0,
+      owner: '',
+      amount: 0,
+    };
+
+    const { error } = await safeExec(async () =>
+      launchNode({
+        loggingEnabled: false,
+        snapshotConfig: {
+          ...defaultSnapshotConfigs,
+          stateConfig: {
+            coins: [invalidCoin],
+            messages: [],
+          },
+        },
+      })
+    );
+    expect(error).toBeDefined();
 
-    expect(killFn).toHaveBeenCalledTimes(1);
-    expect(state.isDead).toEqual(true);
+    expect(mkdirSyncSpy).toHaveBeenCalledTimes(1);
+    const tempDirPath = mkdirSyncSpy.mock.calls[0][0];
+
+    // wait until cleanup finishes (done via events)
+    await sleep(1500);
+    expect(fsMod.existsSync(tempDirPath)).toBeFalsy();
+  });
+
+  test('calling cleanup multiple times does not retry process killing', async () => {
+    const killSpy = vi.spyOn(process, 'kill');
+
+    const { cleanup } = await launchNode({ loggingEnabled: false });
+
+    cleanup();
+
+    expect(killSpy).toHaveBeenCalledTimes(1);
+
+    cleanup();
+
+    expect(killSpy).toHaveBeenCalledTimes(1);
+  });
+
+  test('external killing of node runs side-effect cleanup', async () => {
+    const mkdirSyncSpy = vi.spyOn(fsMod, 'mkdirSync');
+
+    const { pid } = await launchNode({ loggingEnabled: false });
+
+    expect(mkdirSyncSpy).toHaveBeenCalledTimes(1);
+    const tempDirPath = mkdirSyncSpy.mock.calls[0][0];
+
+    childProcessMod.execSync(`kill -- -${pid}`);
+    // wait until cleanup finishes (done via events)
+    await sleep(1500);
+    expect(fsMod.existsSync(tempDirPath)).toBeFalsy();
+  });
+
+  test('calling cleanup on externally killed node does not throw', async () => {
+    const mkdirSyncSpy = vi.spyOn(fsMod, 'mkdirSync');
+    const logSpy = vi.spyOn(console, 'log');
+
+    const { pid, cleanup } = await launchNode({ loggingEnabled: false });
+    expect(mkdirSyncSpy).toHaveBeenCalledTimes(1);
+
+    childProcessMod.execSync(`kill -- -${pid}`);
+    // wait until cleanup finishes (done via events)
+    await sleep(1500);
+    cleanup();
+
+    expect(logSpy).toHaveBeenCalledWith(
+      `fuel-core node under pid ${pid} does not exist. The node might have been killed before cleanup was called. Exiting cleanly.`
+    );
   });
 });

packages/account/src/test-utils/launchNode.ts

@@ -3,13 +3,11 @@ import { randomBytes } from '@fuel-ts/crypto';
 import { FuelError } from '@fuel-ts/errors';
 import type { SnapshotConfigs } from '@fuel-ts/utils';
 import { defaultConsensusKey, hexlify, defaultSnapshotConfigs } from '@fuel-ts/utils';
-import type { ChildProcessWithoutNullStreams } from 'child_process';
 import { randomUUID } from 'crypto';
 import { existsSync, mkdirSync, rmSync, writeFileSync } from 'fs';
 import os from 'os';
 import path from 'path';
 import { getPortPromise } from 'portfinder';
-import treeKill from 'tree-kill';
 
 import { Provider } from '../providers';
 import { Signer } from '../signer';
@@ -56,35 +54,9 @@ export type LaunchNodeResult = Promise<{
   port: string;
   url: string;
   snapshotDir: string;
+  pid: number;
 }>;
 
-export type KillNodeParams = {
-  child: ChildProcessWithoutNullStreams;
-  configPath: string;
-  killFn: (pid: number) => void;
-  state: {
-    isDead: boolean;
-  };
-};
-
-export const killNode = (params: KillNodeParams) => {
-  const { child, configPath, state, killFn } = params;
-  if (!state.isDead) {
-    if (child.pid) {
-      state.isDead = true;
-      killFn(Number(child.pid));
-    }
-
-    // Remove all the listeners we've added.
-    child.stderr.removeAllListeners();
-
-    // Remove the temporary folder and all its contents.
-    if (existsSync(configPath)) {
-      rmSync(configPath, { recursive: true });
-    }
-  }
-};
-
 function getFinalStateConfigJSON({ stateConfig, chainConfig }: SnapshotConfigs) {
   const defaultCoins = defaultSnapshotConfigs.stateConfig.coins.map((coin) => ({
     ...coin,
@@ -232,7 +204,7 @@ export const launchNode = async ({
         '--debug',
         ...remainingArgs,
       ].flat(),
-      { stdio: 'pipe' }
+      { stdio: 'pipe', detached: true }
     );
 
     if (loggingEnabled) {
@@ -242,13 +214,45 @@ export const launchNode = async ({
       });
     }
 
-    const cleanupConfig: KillNodeParams = {
-      child,
-      configPath: tempDir,
-      killFn: treeKill,
-      state: {
-        isDead: false,
-      },
+    const removeSideffects = () => {
+      child.stderr.removeAllListeners();
+      if (existsSync(tempDir)) {
+        rmSync(tempDir, { recursive: true });
+      }
+    };
+
+    child.on('error', removeSideffects);
+    child.on('exit', removeSideffects);
+
+    const childState = {
+      isDead: false,
+    };
+
+    const cleanup = () => {
+      if (childState.isDead) {
+        return;
+      }
+      childState.isDead = true;
+
+      removeSideffects();
+      if (child.pid !== undefined) {
+        try {
+          process.kill(-child.pid);
+        } catch (e) {
+          const error = e as Error & { code: string };
+          if (error.code === 'ESRCH') {
+            // eslint-disable-next-line no-console
+            console.log(
+              `fuel-core node under pid ${child.pid} does not exist. The node might have been killed before cleanup was called. Exiting cleanly.`
+            );
+          } else {
+            throw e;
+          }
+        }
+      } else {
+        // eslint-disable-next-line no-console
+        console.error('No PID available for the child process, unable to kill launched node');
+      }
     };
 
     // Look for a specific graphql start point in the output.
@@ -264,11 +268,12 @@ export const launchNode = async ({
 
         // Resolve with the cleanup method.
         resolve({
-          cleanup: () => killNode(cleanupConfig),
+          cleanup,
           ip: realIp,
           port: realPort,
           url: `http://${realIp}:${realPort}/v1/graphql`,
           snapshotDir: snapshotDirToUse as string,
+          pid: child.pid as number,
         });
       }
       if (/error/i.test(text)) {
@@ -279,18 +284,18 @@ export const launchNode = async ({
     });
 
     // Process exit.
-    process.on('exit', () => killNode(cleanupConfig));
+    process.on('exit', cleanup);
 
     // Catches ctrl+c event.
-    process.on('SIGINT', () => killNode(cleanupConfig));
+    process.on('SIGINT', cleanup);
 
     // Catches "kill pid" (for example: nodemon restart).
-    process.on('SIGUSR1', () => killNode(cleanupConfig));
-    process.on('SIGUSR2', () => killNode(cleanupConfig));
+    process.on('SIGUSR1', cleanup);
+    process.on('SIGUSR2', cleanup);
 
     // Catches uncaught exceptions.
-    process.on('beforeExit', () => killNode(cleanupConfig));
-    process.on('uncaughtException', () => killNode(cleanupConfig));
+    process.on('beforeExit', cleanup);
+    process.on('uncaughtException', cleanup);
 
     child.on('error', reject);
   });

packages/fuels/package.json

@@ -84,7 +84,6 @@
     "lodash.camelcase": "^4.3.0",
     "portfinder": "^1.0.32",
     "toml": "^3.0.0",
-    "tree-kill": "^1.2.2",
     "yup": "^1.4.0"
   },
   "devDependencies": {