Skip to content

Commit f5ad522

Browse files
committed
fix: centralize process lifecycle and cancellation handling
Add ProcessManager singleton for centralized process lifecycle management: - Single AbortController per run for coordinated cancellation - Process registry to track all spawned child processes - Signal propagation (SIGINT/SIGTERM) to all children with process tree killing - Terminal state restoration (cursor visibility) on cleanup - Configurable timeouts via environment variables: - MDFLOW_FETCH_TIMEOUT (default: 10s) - MDFLOW_COMMAND_TIMEOUT (default: 30s) - MDFLOW_AGENT_TIMEOUT (default: no timeout) Integration points: - spinner.ts: Coordinates cursor hide/show with ProcessManager - command.ts: Registers spawned processes for cleanup - imports.ts: Uses configurable command timeout, registers inline processes - cli-runner.ts: Initializes ProcessManager, registers cleanup callbacks - index.ts: Early initialization for EPIPE handling with cursor restoration
1 parent 1ff8006 commit f5ad522

File tree

7 files changed

+794
-17
lines changed

7 files changed

+794
-17
lines changed

src/cli-runner.ts

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
resolveCommand, buildArgs, runCommand, extractPositionalMappings,
1616
extractEnvVars, killCurrentChildProcess, hasInteractiveMarker,
1717
} from "./command";
18+
import { getProcessManager } from "./process-manager";
1819
import {
1920
expandImports, hasImports,
2021
expandContentImports, expandCommandImports,
@@ -272,14 +273,14 @@ export class CliRunner {
272273
localFilePath = await this.resolveFilePath(filePath);
273274
}
274275

275-
// Signal handling
276-
const handleSignal = async (signal: string) => {
277-
killCurrentChildProcess();
278-
if (isRemote) await cleanupRemote(localFilePath);
279-
process.exit(signal === "SIGINT" ? 130 : 143);
280-
};
281-
process.on("SIGINT", () => handleSignal("SIGINT"));
282-
process.on("SIGTERM", () => handleSignal("SIGTERM"));
276+
// Initialize ProcessManager for centralized lifecycle management
277+
const pm = getProcessManager();
278+
pm.initialize();
279+
280+
// Register cleanup callback for remote file cleanup
281+
if (isRemote) {
282+
pm.onCleanup(() => cleanupRemote(localFilePath));
283+
}
283284

284285
if (!(await this.env.fs.exists(localFilePath))) {
285286
throw new FileNotFoundError(`File not found: ${localFilePath}`);

src/command.ts

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,27 @@
11
/**
22
* Command execution - simple, direct, unix-style
33
* No abstraction layers, just frontmatter → CLI args → spawn
4+
*
5+
* Integrates with ProcessManager for centralized process lifecycle management
46
*/
57

68
import type { AgentFrontmatter } from "./types";
79
import { basename } from "path";
810
import { teeToStdoutAndCollect, teeToStderrAndCollect } from "./stream";
911
import { stopSpinner, isSpinnerRunning } from "./spinner";
12+
import { getProcessManager } from "./process-manager";
1013

1114
/**
1215
* Module-level reference to the current child process
1316
* Used for graceful signal handling (SIGINT/SIGTERM cleanup)
17+
* @deprecated Use ProcessManager.getInstance() instead for new code
1418
*/
1519
let currentChildProcess: ReturnType<typeof Bun.spawn> | null = null;
1620

1721
/**
1822
* Get the current child process reference
1923
* Returns null if no process is running
24+
* @deprecated Use ProcessManager.getInstance().getActiveProcesses() instead
2025
*/
2126
export function getCurrentChildProcess(): ReturnType<typeof Bun.spawn> | null {
2227
return currentChildProcess;
@@ -25,8 +30,18 @@ export function getCurrentChildProcess(): ReturnType<typeof Bun.spawn> | null {
2530
/**
2631
* Kill the current child process if running
2732
* Returns true if a process was killed, false otherwise
33+
* @deprecated Use ProcessManager.getInstance().killAll() instead
2834
*/
2935
export function killCurrentChildProcess(): boolean {
36+
// First try ProcessManager (which handles process groups)
37+
const pm = getProcessManager();
38+
if (pm.activeCount > 0) {
39+
pm.killAll();
40+
currentChildProcess = null;
41+
return true;
42+
}
43+
44+
// Fallback for legacy code
3045
if (currentChildProcess) {
3146
try {
3247
currentChildProcess.kill("SIGTERM");
@@ -330,7 +345,11 @@ export async function runCommand(ctx: RunContext): Promise<RunResult> {
330345
env: runEnv,
331346
});
332347

333-
// Store reference for signal handling
348+
// Register with ProcessManager for centralized lifecycle management
349+
const pm = getProcessManager();
350+
pm.register(proc, command);
351+
352+
// Store reference for legacy signal handling (deprecated)
334353
currentChildProcess = proc;
335354

336355
let stdout = "";

src/imports.ts

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -261,8 +261,21 @@ export const WARN_TOKENS = 50_000;
261261
export const CHARS_PER_TOKEN = 4;
262262
const MAX_CHARS = MAX_TOKENS * CHARS_PER_TOKEN;
263263

264-
/** Command execution timeout in milliseconds (30 seconds) */
265-
const COMMAND_TIMEOUT_MS = 30_000;
264+
import { getProcessManager } from "./process-manager";
265+
266+
/** Default command execution timeout in milliseconds (30 seconds) */
267+
const DEFAULT_COMMAND_TIMEOUT_MS = 30_000;
268+
269+
/**
270+
* Get command timeout from ProcessManager or use default
271+
*/
272+
function getCommandTimeout(): number {
273+
try {
274+
return getProcessManager().timeouts.commandTimeout;
275+
} catch {
276+
return DEFAULT_COMMAND_TIMEOUT_MS;
277+
}
278+
}
266279

267280
/** Maximum command output size in characters (~25k tokens) */
268281
const MAX_COMMAND_OUTPUT_SIZE = 100_000;
@@ -957,6 +970,7 @@ async function processCommandInline(
957970
// Track process for timeout cleanup
958971
let proc: ReturnType<typeof Bun.spawn> | null = null;
959972
let timedOut = false;
973+
const pm = getProcessManager();
960974

961975
try {
962976
proc = Bun.spawn([shell, ...shellArgs], {
@@ -966,6 +980,9 @@ async function processCommandInline(
966980
env: env as Record<string, string>,
967981
});
968982

983+
// Register with ProcessManager for centralized cleanup on SIGINT/SIGTERM
984+
pm.register(proc, `inline: ${actualCommand.slice(0, 40)}`);
985+
969986
// Buffers for final output
970987
const stdoutChunks: Uint8Array[] = [];
971988
const stderrChunks: Uint8Array[] = [];
@@ -993,12 +1010,14 @@ async function processCommandInline(
9931010
};
9941011

9951012
// Improvement #4: Execution timeout using Promise.race
1013+
// Uses configurable timeout from ProcessManager
1014+
const commandTimeout = getCommandTimeout();
9961015
const timeoutPromise = new Promise<never>((_, reject) => {
9971016
setTimeout(() => {
9981017
timedOut = true;
9991018
proc?.kill();
1000-
reject(new Error(`Command timed out after ${COMMAND_TIMEOUT_MS}ms: ${actualCommand}`));
1001-
}, COMMAND_TIMEOUT_MS);
1019+
reject(new Error(`Command timed out after ${commandTimeout}ms: ${actualCommand}`));
1020+
}, commandTimeout);
10021021
});
10031022

10041023
// Read streams with timeout (stdout/stderr are guaranteed with "pipe" option)

src/index.ts

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,28 +3,37 @@
33
* Entry point for mdflow CLI
44
*
55
* This is a minimal entry point that:
6-
* 1. Sets up EPIPE handlers for graceful pipe handling
7-
* 2. Creates a CliRunner with the real system environment
8-
* 3. Runs the CLI and exits with the appropriate code
6+
* 1. Initializes ProcessManager for centralized lifecycle management
7+
* 2. Sets up EPIPE handlers for graceful pipe handling
8+
* 3. Creates a CliRunner with the real system environment
9+
* 4. Runs the CLI and exits with the appropriate code
910
*
1011
* All orchestration logic is in CliRunner for testability.
1112
*/
1213

1314
import { CliRunner } from "./cli-runner";
1415
import { BunSystemEnvironment } from "./system-environment";
16+
import { getProcessManager } from "./process-manager";
1517

1618
async function main() {
19+
// Initialize ProcessManager early for centralized signal handling
20+
// This ensures cursor restoration and process cleanup on SIGINT/SIGTERM
21+
const pm = getProcessManager();
22+
pm.initialize();
23+
1724
// Handle EPIPE gracefully when downstream closes the pipe early
1825
// (e.g., `md task.md | head -n 5`)
1926
process.stdout.on("error", (err: NodeJS.ErrnoException) => {
2027
if (err.code === "EPIPE") {
28+
pm.restoreTerminal(); // Ensure cursor is visible
2129
process.exit(0);
2230
}
2331
throw err;
2432
});
2533

2634
process.stderr.on("error", (err: NodeJS.ErrnoException) => {
2735
if (err.code === "EPIPE") {
36+
pm.restoreTerminal(); // Ensure cursor is visible
2837
process.exit(0);
2938
}
3039
throw err;

0 commit comments

Comments
 (0)