fix(engine): run user commands via async exec to avoid blocking event loop
The merger's test/build verification, executor's worktreeInitCommand,
setupScript, and script-mode workflow steps all used execSync, which
blocks Node's event loop until the child process exits. A hanging
pnpm test could freeze the entire engine (no logs, heartbeats, or
other task progress) for the full 5-minute timeout.
Switch these call sites to promisify(exec) with awaited calls so the
engine keeps running while user-configured commands execute. Short
internal git plumbing (rev-parse, branch -d, worktree remove) still
uses execSync since those commands are bounded and measured in ms.
Document the rule in AGENTS.md under a new "Engine process rules"
section so future agents don't reintroduce blocking behavior.
Tests: update child_process mocks in merger.test, executor.test, and
restart.integration.test to route the new async exec through the
existing execSync mock and expose promisify.custom so destructuring
{ stdout, stderr } matches real child_process.exec semantics.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -57,10 +57,42 @@ vi.mock("./worktree-names.js", async () => {
|
||||
};
|
||||
});
|
||||
|
||||
// Mock node modules used by executor
|
||||
vi.mock("node:child_process", () => ({
|
||||
execSync: vi.fn(),
|
||||
}));
|
||||
// Mock node modules used by executor.
|
||||
// Route async `exec` through the `execSync` mock so existing tests that set
|
||||
// up mockedExecSync.mockImplementation for user commands (worktreeInitCommand,
|
||||
// workflow step scripts, etc.) keep working unchanged after the switch to
|
||||
// promisify(exec) in executor.ts.
|
||||
vi.mock("node:child_process", async () => {
|
||||
const { promisify } = await import("node:util");
|
||||
const execSyncFn = vi.fn();
|
||||
const execFn: any = vi.fn((cmd: any, opts: any, cb: any) => {
|
||||
const callback = typeof opts === "function" ? opts : cb;
|
||||
const forwardedOpts = typeof opts === "function" ? undefined : opts;
|
||||
try {
|
||||
const out = execSyncFn(cmd, forwardedOpts);
|
||||
const stdout = out === undefined ? "" : out.toString();
|
||||
if (typeof callback === "function") callback(null, stdout, "");
|
||||
} catch (err: any) {
|
||||
if (typeof callback === "function") {
|
||||
callback(err, err?.stdout?.toString?.() ?? "", err?.stderr?.toString?.() ?? "");
|
||||
}
|
||||
}
|
||||
});
|
||||
// Mirror real child_process.exec: promisify resolves to { stdout, stderr }.
|
||||
execFn[promisify.custom] = (cmd: any, opts?: any) =>
|
||||
new Promise((resolve, reject) => {
|
||||
execFn(cmd, opts, (err: any, stdout: any, stderr: any) => {
|
||||
if (err) {
|
||||
err.stdout = stdout;
|
||||
err.stderr = stderr;
|
||||
reject(err);
|
||||
} else {
|
||||
resolve({ stdout, stderr });
|
||||
}
|
||||
});
|
||||
});
|
||||
return { execSync: execSyncFn, exec: execFn };
|
||||
});
|
||||
vi.mock("node:fs", () => ({
|
||||
existsSync: vi.fn().mockReturnValue(true),
|
||||
}));
|
||||
|
||||
@@ -1,4 +1,7 @@
|
||||
import { execSync } from "node:child_process";
|
||||
import { execSync, exec } from "node:child_process";
|
||||
import { promisify } from "node:util";
|
||||
|
||||
const execAsync = promisify(exec);
|
||||
import { join } from "node:path";
|
||||
import { existsSync } from "node:fs";
|
||||
import { readFile, writeFile } from "node:fs/promises";
|
||||
@@ -953,11 +956,12 @@ export class TaskExecutor {
|
||||
}
|
||||
|
||||
// Run worktree init command for fresh worktrees (skip for pooled — caches are warm)
|
||||
// Non-blocking: uses async exec so the executor event loop keeps running
|
||||
// while the user-configured command (e.g. `pnpm install`) executes.
|
||||
if (settings.worktreeInitCommand) {
|
||||
try {
|
||||
execSync(settings.worktreeInitCommand, {
|
||||
await execAsync(settings.worktreeInitCommand, {
|
||||
cwd: worktreePath,
|
||||
stdio: "pipe",
|
||||
timeout: 120_000,
|
||||
});
|
||||
await this.store.logEntry(task.id, "Worktree init command completed", settings.worktreeInitCommand, this.currentRunContext);
|
||||
@@ -972,9 +976,8 @@ export class TaskExecutor {
|
||||
const scriptCommand = settings.scripts?.[settings.setupScript];
|
||||
if (scriptCommand) {
|
||||
try {
|
||||
execSync(scriptCommand, {
|
||||
await execAsync(scriptCommand, {
|
||||
cwd: worktreePath,
|
||||
stdio: "pipe",
|
||||
timeout: 120_000,
|
||||
});
|
||||
await this.store.logEntry(task.id, `Setup script '${settings.setupScript}' completed`, scriptCommand, this.currentRunContext);
|
||||
@@ -2859,17 +2862,18 @@ ${failureFeedback}
|
||||
await this.store.logEntry(task.id, `Workflow step '${workflowStep.name}' executing script '${scriptName}': ${scriptCommand}`);
|
||||
|
||||
try {
|
||||
const output = execSync(scriptCommand, {
|
||||
// Non-blocking: async exec so the executor event loop keeps running
|
||||
// while the user-configured workflow script executes.
|
||||
const { stdout: out } = await execAsync(scriptCommand, {
|
||||
cwd: worktreePath,
|
||||
stdio: "pipe",
|
||||
timeout: 120_000,
|
||||
});
|
||||
const stdout = output.toString().trim();
|
||||
const stdout = out.toString().trim();
|
||||
return { success: true, output: stdout || `Script '${scriptName}' completed successfully` };
|
||||
} catch (err: any) {
|
||||
const stderr = err.stderr?.toString()?.trim() || "";
|
||||
const stdout = err.stdout?.toString()?.trim() || "";
|
||||
const exitCode = err.status;
|
||||
const exitCode = err.code ?? err.status;
|
||||
const parts: string[] = [];
|
||||
if (exitCode !== undefined) parts.push(`Exit code: ${exitCode}`);
|
||||
if (stdout) parts.push(`stdout: ${stdout}`);
|
||||
|
||||
@@ -13,9 +13,40 @@ vi.mock("./pi.js", () => ({
|
||||
compactSessionContext: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("node:child_process", () => ({
|
||||
execSync: vi.fn(),
|
||||
}));
|
||||
// Route async `exec` through the `execSync` mock so existing tests that set up
|
||||
// mockedExecSync.mockImplementation for verification commands (vitest run,
|
||||
// pnpm build, etc.) keep working unchanged. `promisify(exec)` in merger.ts
|
||||
// resolves/rejects based on the callback wired here.
|
||||
vi.mock("node:child_process", async () => {
|
||||
const { promisify } = await import("node:util");
|
||||
const execSyncFn = vi.fn();
|
||||
const execFn: any = vi.fn((cmd: any, opts: any, cb: any) => {
|
||||
const callback = typeof opts === "function" ? opts : cb;
|
||||
try {
|
||||
const out = execSyncFn(cmd, { stdio: ["pipe", "pipe", "pipe"] });
|
||||
const stdout = out === undefined ? "" : out.toString();
|
||||
if (typeof callback === "function") callback(null, stdout, "");
|
||||
} catch (err: any) {
|
||||
if (typeof callback === "function") {
|
||||
callback(err, err?.stdout?.toString?.() ?? "", err?.stderr?.toString?.() ?? "");
|
||||
}
|
||||
}
|
||||
});
|
||||
// Mirror real child_process.exec: promisify resolves to { stdout, stderr }.
|
||||
execFn[promisify.custom] = (cmd: any, opts?: any) =>
|
||||
new Promise((resolve, reject) => {
|
||||
execFn(cmd, opts, (err: any, stdout: any, stderr: any) => {
|
||||
if (err) {
|
||||
err.stdout = stdout;
|
||||
err.stderr = stderr;
|
||||
reject(err);
|
||||
} else {
|
||||
resolve({ stdout, stderr });
|
||||
}
|
||||
});
|
||||
});
|
||||
return { execSync: execSyncFn, exec: execFn };
|
||||
});
|
||||
|
||||
vi.mock("node:fs", () => ({
|
||||
existsSync: vi.fn().mockReturnValue(true),
|
||||
|
||||
@@ -1,4 +1,7 @@
|
||||
import { execSync } from "node:child_process";
|
||||
import { execSync, exec } from "node:child_process";
|
||||
import { promisify } from "node:util";
|
||||
|
||||
const execAsync = promisify(exec);
|
||||
import { existsSync } from "node:fs";
|
||||
import { join } from "node:path";
|
||||
import { getTaskMergeBlocker, type TaskStore, type MergeResult, type MergeDetails, type WorkflowStep, type WorkflowStepResult, type Settings, type AgentPromptsConfig } from "@fusion/core";
|
||||
@@ -325,15 +328,16 @@ async function runVerificationCommand(
|
||||
};
|
||||
|
||||
try {
|
||||
// Execute the command with timeout
|
||||
const output = execSync(command, {
|
||||
// Execute the command with timeout (non-blocking: uses async exec so the
|
||||
// engine event loop keeps running while the child process executes)
|
||||
const { stdout, stderr } = await execAsync(command, {
|
||||
cwd: rootDir,
|
||||
encoding: "utf-8",
|
||||
maxBuffer: VERIFICATION_COMMAND_MAX_BUFFER,
|
||||
timeout: 300_000, // 5 minute timeout for verification commands
|
||||
stdio: ["pipe", "pipe", "pipe"],
|
||||
});
|
||||
result.stdout = output;
|
||||
result.stdout = stdout;
|
||||
result.stderr = stderr;
|
||||
result.exitCode = 0;
|
||||
result.success = true;
|
||||
mergerLog.log(`${taskId}: ${type} command succeeded`);
|
||||
|
||||
@@ -30,6 +30,10 @@ vi.mock("./reviewer.js", () => ({
|
||||
}));
|
||||
vi.mock("node:child_process", () => ({
|
||||
execSync: vi.fn().mockReturnValue(Buffer.from("")),
|
||||
exec: vi.fn((_cmd: any, opts: any, cb: any) => {
|
||||
const callback = typeof opts === "function" ? opts : cb;
|
||||
if (typeof callback === "function") callback(null, "", "");
|
||||
}),
|
||||
}));
|
||||
vi.mock("node:fs", () => ({
|
||||
existsSync: vi.fn().mockReturnValue(true),
|
||||
|
||||
Reference in New Issue
Block a user