Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fuels dev cleanup not killing node #3038

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/kind-students-kneel.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@fuel-ts/account": patch
"fuels": patch
---

fix: `fuels dev` cleanup not killing node
6 changes: 6 additions & 0 deletions .github/actions/test-setup/action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ runs:
version: ${{ inputs.pnpm-version }}
run_install: true

- name: Setup forc and fuel-core paths
shell: bash
run: |
echo "$PWD/internal/forc/forc-binaries" >> $GITHUB_PATH
echo "$PWD/internal/fuel-core/fuel-core-binaries" >> $GITHUB_PATH
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added these to the path so that the spawn can run from the cwd of the temporary test project instead of running pnpm fuels init --path ${paths.init} from our repo's root. Both of them are using the fuels-forc under the hood, but it feels more correct to have all the commands be running from inside the temporary test folder, pnpm fuels init included.

As an alternative, I could initiate the temporary project and pass in the --fuel-core-path and --forc-path arguments to point to these binaries. I opted for adding forc and fuel-core to the path like this because this wasn't the first time I had to debug this problem only to come to the conclusion that forc and fuel-core aren't in the path.


- name: Setup Bun
if: ${{ inputs.should-install-bun == 'true' }}
uses: oven-sh/setup-bun@v1
Expand Down
1 change: 0 additions & 1 deletion packages/account/src/test-utils/launchNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,6 @@ export const launchNode = async ({
}
childState.isDead = true;

removeSideffects();
Copy link
Contributor Author

@nedsalk nedsalk Aug 27, 2024

Choose a reason for hiding this comment

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

Removing this line caused the issue to go away.

I suspect that the root cause of the problem is somewhere in the fuels package because this line didn't cause problems to e.g. launchTestNode. It might be in the interplay between file watchers registered in the dev command and this cleanup function removing the files they're watching, but I couldn't pinpoint the actual root cause. I tried deleting lines I think might be causing it while having this line still on, but to no avail. We can search for the root cause in another issue that's of lesser priority.

The cleanup still happens because of the error and exit event listeners registered on the child above.

if (child.pid !== undefined) {
try {
process.kill(-child.pid);
Expand Down
4 changes: 4 additions & 0 deletions packages/fuels/src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ export const configureCli = () => {
.option('--forc-path <path>', 'Path to the `forc` binary')
.option('--fuel-core-path <path>', 'Path to the `fuel-core` binary')
.option('--auto-start-fuel-core', 'Auto-starts a `fuel-core` node during `dev` command')
.option(
'--fuel-core-port <port>',
'Port to use when starting a local `fuel-core` node for dev mode'
)
.action(withProgram(command, Commands.init, init));

(command = program.command(Commands.dev))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export class Src14OwnedProxyFactory extends ContractFactory<Src14OwnedProxy> {
static deploy (
wallet: Account,
options: DeployContractOptions = {}
): ReturnType<Src14OwnedProxyFactory['deploy']> {
) {
const factory = new Src14OwnedProxyFactory(wallet);
return factory.deploy(options);
}
Expand Down
6 changes: 2 additions & 4 deletions packages/fuels/src/cli/commands/dev/autoStartFuelCore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,7 @@ export const autoStartFuelCore = async (config: FuelsConfig) => {

const port = config.fuelCorePort ?? (await getPortPromise({ port: 4000 }));

const providerUrl = `http://${accessIp}:${port}/v1/graphql`;

const { cleanup, snapshotDir } = await launchNode({
const { cleanup, snapshotDir, url } = await launchNode({
args: [
['--snapshot', config.snapshotDir],
['--db-type', 'in-memory'],
Expand All @@ -43,7 +41,7 @@ export const autoStartFuelCore = async (config: FuelsConfig) => {
bindIp,
accessIp,
port,
providerUrl,
providerUrl: url,
snapshotDir,
killChildProcess: cleanup,
};
Expand Down
3 changes: 2 additions & 1 deletion packages/fuels/src/cli/commands/init/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { log } from '../../utils/logger';
export function init(program: Command) {
const options = program.opts();

const { path, autoStartFuelCore, forcPath, fuelCorePath } = options;
const { path, autoStartFuelCore, forcPath, fuelCorePath, fuelCorePort } = options;

let workspace: string | undefined;
let absoluteWorkspace: string | undefined;
Expand Down Expand Up @@ -61,6 +61,7 @@ export function init(program: Command) {
forcPath,
fuelCorePath,
autoStartFuelCore,
fuelCorePort,
});

writeFileSync(fuelsConfigPath, renderedConfig);
Expand Down
3 changes: 3 additions & 0 deletions packages/fuels/src/cli/templates/fuels.config.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ export default createConfig({
{{#if (isDefined autoStartFuelCore)}}
autoStartFuelCore: {{autoStartFuelCore}},
{{/if}}
{{#if (isDefined fuelCorePort)}}
fuelCorePort: {{fuelCorePort}},
{{/if}}
});

/**
Expand Down
1 change: 1 addition & 0 deletions packages/fuels/src/cli/templates/fuels.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export function renderFuelsConfigTemplate(props: {
forcPath?: string;
fuelCorePath?: string;
autoStartFuelCore?: boolean;
fuelCorePort?: string;
}) {
const renderTemplate = Handlebars.compile(fuelsConfigTemplate, {
strict: true,
Expand Down
79 changes: 79 additions & 0 deletions packages/fuels/test/features/dev-2.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import { execSync, execFileSync, spawn } from 'child_process';
import { mkdirSync, rmSync } from 'fs';
import { tmpdir } from 'os';
import path from 'path';

import { deferPromise, randomUUID } from '../../src';
import { findChildProcessPid } from '../utils/findChildProcessPid';
import { isProcessRunning } from '../utils/isProcessRunning';

function runInit() {
const fuelsPath = path.join(process.cwd(), 'packages/fuels');

const init = path.join(tmpdir(), '.fuels', 'tests', randomUUID());

mkdirSync(init, { recursive: true });

execFileSync('pnpm', ['init'], { cwd: init });
execFileSync('pnpm', ['link', fuelsPath], { cwd: init });

const contractDir = path.join(init, 'contract');
const outputDir = path.join(init, 'output');
mkdirSync(contractDir);
mkdirSync(outputDir);

execSync(`${process.env.FORC_PATH} init`, { cwd: contractDir });
execSync(`pnpm fuels init -o ${outputDir} -c ${contractDir} --fuel-core-port 0`, { cwd: init });

return {
init,
[Symbol.dispose]: () => {
rmSync(init, { recursive: true });
},
};
}

/**
* @group node
*/
describe('dev', () => {
it(
'cleans up resources on graceful shutdown',
async () => {
using paths = runInit();

const devProcess = spawn('pnpm fuels dev', {
shell: 'bash',
detached: true,
cwd: paths.init,
});

const devCompleted = deferPromise();

devProcess.stdout.on('data', (chunk) => {
const text = chunk.toString();
if (text.indexOf('Dev completed successfully!') !== -1) {
devCompleted.resolve(undefined);
}
});

await devCompleted.promise;

const devExited = deferPromise();
devProcess.on('exit', () => {
devExited.resolve(undefined);
});

const devPid = devProcess.pid as number;

const fuelCorePid = findChildProcessPid(devPid, 'fuel-core') as number;

process.kill(-devPid, 'SIGINT');

await devExited.promise;

expect(isProcessRunning(fuelCorePid)).toBe(false);

Check failure on line 75 in packages/fuels/test/features/dev-2.test.ts

View workflow job for this annotation

GitHub Actions / node@18

packages/fuels/test/features/dev-2.test.ts > dev > cleans up resources on graceful shutdown

AssertionError: expected true to be false // Object.is equality - Expected + Received - false + true ❯ it.timeout packages/fuels/test/features/dev-2.test.ts:75:45

Check failure on line 75 in packages/fuels/test/features/dev-2.test.ts

View workflow job for this annotation

GitHub Actions / node@20

packages/fuels/test/features/dev-2.test.ts > dev > cleans up resources on graceful shutdown

AssertionError: expected true to be false // Object.is equality - Expected + Received - false + true ❯ it.timeout packages/fuels/test/features/dev-2.test.ts:75:45

Check failure on line 75 in packages/fuels/test/features/dev-2.test.ts

View workflow job for this annotation

GitHub Actions / node@22

packages/fuels/test/features/dev-2.test.ts > dev > cleans up resources on graceful shutdown

AssertionError: expected true to be false // Object.is equality - Expected + Received - false + true ❯ it.timeout packages/fuels/test/features/dev-2.test.ts:75:45
},
{ timeout: 15000 }
);
});
25 changes: 25 additions & 0 deletions packages/fuels/test/utils/findChildProcessPid.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { execSync } from 'child_process';

export function findChildProcessPid(
parentPid: number,
childProcessName: string
): number | undefined {
const childProcesses = execSync(`ps --ppid ${parentPid} -o pid,cmd --no-headers || true`)
.toString()
.split('\n')
.map((s) => s.trim())
.filter((s) => s !== '');

for (const cp of childProcesses) {
const [pid, name] = cp.split(' ');
if (name.indexOf(childProcessName) !== -1) {
return +pid;
}
const childPid = findChildProcessPid(+pid, childProcessName);
if (childPid) {
return childPid;
}
}

return undefined;
}
19 changes: 19 additions & 0 deletions packages/fuels/test/utils/isProcessRunning.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
export function isProcessRunning(pid: number) {
try {
// Check if the process exists
process.kill(pid, 0);
return true; // If no error, the process is running
} catch (e) {
const error = e as Error & { code: string };
// Error codes:
// ESRCH: No such process
// EPERM: Permission denied (you don't have permissions to check)
if (error.code === 'ESRCH') {
return false; // No such process
}
if (error.code === 'EPERM') {
return true; // Process exists, but we don't have permission to send a signal
}
throw error; // Some other unexpected error
}
}
1 change: 1 addition & 0 deletions vitest.global-setup.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
export default function setup() {
process.env.FUEL_CORE_PATH = 'fuels-core';
process.env.FORC_PATH = 'fuels-forc';
}
Loading