-
Notifications
You must be signed in to change notification settings - Fork 4k
feat(cli): add beta background process execution tools #9074
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
base: main
Are you sure you want to change the base?
Changes from all commits
7b230c5
f2cf8fc
68a575c
a5386d0
a603124
853d269
73bbc89
0bed20f
59b0664
9da6362
0a05cf8
a67161a
74fd449
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,282 @@ | ||
| import { spawn, type ChildProcess } from "child_process"; | ||
|
|
||
| import { getShellCommand } from "../tools/runTerminalCommand.js"; | ||
| import { logger } from "../util/logger.js"; | ||
|
|
||
| import { BaseService } from "./BaseService.js"; | ||
| import { CircularBuffer } from "./CircularBuffer.js"; | ||
|
|
||
| interface ProcessInfo { | ||
| id: number; | ||
| command: string; | ||
| child: ChildProcess | null; | ||
| pid: number | undefined; | ||
| startTime: number; | ||
| exitCode: number | null; | ||
| exitTime: number | null; | ||
| status: "running" | "exited"; | ||
| stdoutBuffer: CircularBuffer; | ||
| stderrBuffer: CircularBuffer; | ||
| lastReadStdoutLine: number; | ||
| lastReadStderrLine: number; | ||
| } | ||
|
|
||
| interface BackgroundProcessServiceState { | ||
| processes: Map<number, ProcessInfo>; | ||
| nextId: number; | ||
| maxProcesses: number; | ||
| } | ||
|
|
||
| export class BackgroundProcessService extends BaseService<BackgroundProcessServiceState> { | ||
| constructor() { | ||
| super("backgroundProcesses", { | ||
| processes: new Map(), | ||
| nextId: 1, | ||
| maxProcesses: 10, | ||
| }); | ||
| } | ||
|
|
||
| async doInitialize(): Promise<BackgroundProcessServiceState> { | ||
| return { | ||
| processes: new Map(), | ||
| nextId: 1, | ||
| maxProcesses: 10, | ||
| }; | ||
| } | ||
|
|
||
| async startProcess( | ||
| command: string, | ||
| cwd: string, | ||
| ): Promise<{ id: number; message: string }> { | ||
| // Check if we've hit the process limit | ||
| const runningCount = Array.from( | ||
| this.currentState.processes.values(), | ||
| ).filter((p) => p.status === "running").length; | ||
|
|
||
| if (runningCount >= this.currentState.maxProcesses) { | ||
| return { | ||
| id: -1, | ||
| message: `Error: Maximum background processes (${this.currentState.maxProcesses}) reached. Use KillProcess to terminate a process first. Use ListProcesses to see running processes.`, | ||
| }; | ||
| } | ||
|
|
||
| // Allocate process ID | ||
| const id = this.currentState.nextId; | ||
| this.setState({ | ||
| ...this.currentState, | ||
| nextId: this.currentState.nextId + 1, | ||
| }); | ||
|
|
||
| // Spawn the process | ||
| const { shell, args } = getShellCommand(command); | ||
| const child = spawn(shell, args, { cwd }); | ||
|
|
||
| const processInfo: ProcessInfo = { | ||
| id, | ||
| command, | ||
| child, | ||
| pid: child.pid, | ||
| startTime: Date.now(), | ||
| exitCode: null, | ||
| exitTime: null, | ||
| status: "running", | ||
| stdoutBuffer: new CircularBuffer(), | ||
| stderrBuffer: new CircularBuffer(), | ||
| lastReadStdoutLine: 0, | ||
| lastReadStderrLine: 0, | ||
| }; | ||
|
|
||
| // Set up output capture | ||
| child.stdout?.on("data", (data) => { | ||
| const lines = data.toString().split("\n"); | ||
| for (const line of lines) { | ||
| if (line) { | ||
| processInfo.stdoutBuffer.append(line); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| child.stderr?.on("data", (data) => { | ||
| const lines = data.toString().split("\n"); | ||
| for (const line of lines) { | ||
| if (line) { | ||
| processInfo.stderrBuffer.append(line); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // Handle process exit | ||
| child.on("close", (code) => { | ||
| processInfo.exitCode = code; | ||
| processInfo.exitTime = Date.now(); | ||
| processInfo.status = "exited"; | ||
| processInfo.child = null; | ||
|
|
||
| logger.debug(`Background process ${id} exited with code ${code}`); | ||
|
|
||
| // Update state | ||
| const updatedProcesses = new Map(this.currentState.processes); | ||
| updatedProcesses.set(id, processInfo); | ||
| this.setState({ | ||
| ...this.currentState, | ||
| processes: updatedProcesses, | ||
| }); | ||
|
|
||
| // Emit event | ||
| this.emit("processExited", { id, exitCode: code }); | ||
| }); | ||
|
|
||
| child.on("error", (error) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed - now updates process status to exited and emits processExited event |
||
| logger.debug(`Background process ${id} error: ${error.message}`); | ||
| processInfo.stderrBuffer.append(`Process error: ${error.message}`); | ||
|
|
||
| // Mark as exited since error event may fire without close event | ||
| // (e.g., when process fails to spawn) | ||
| processInfo.exitCode = 1; | ||
| processInfo.exitTime = Date.now(); | ||
| processInfo.status = "exited"; | ||
| processInfo.child = null; | ||
|
|
||
| // Update state | ||
| const updatedProcesses = new Map(this.currentState.processes); | ||
| updatedProcesses.set(id, processInfo); | ||
| this.setState({ | ||
| ...this.currentState, | ||
| processes: updatedProcesses, | ||
| }); | ||
|
|
||
| // Emit event | ||
| this.emit("processExited", { id, exitCode: 1 }); | ||
| }); | ||
|
|
||
| // Add to registry | ||
| const updatedProcesses = new Map(this.currentState.processes); | ||
| updatedProcesses.set(id, processInfo); | ||
| this.setState({ | ||
| ...this.currentState, | ||
| processes: updatedProcesses, | ||
| }); | ||
|
|
||
| return { | ||
| id, | ||
| message: `Background process started with ID ${id} (PID: ${child.pid}). Use ReadBackgroundProcessOutput to monitor output:\n ReadBackgroundProcessOutput(bash_id: ${id})`, | ||
| }; | ||
| } | ||
|
|
||
| getProcess(id: number): ProcessInfo | undefined { | ||
| return this.currentState.processes.get(id); | ||
| } | ||
|
|
||
| async killProcess( | ||
| id: number, | ||
| ): Promise<{ success: boolean; message: string }> { | ||
| const processInfo = this.currentState.processes.get(id); | ||
|
|
||
| if (!processInfo) { | ||
| return { | ||
| success: false, | ||
| message: `Error: No background process found with ID ${id}. Use ListProcesses to see running processes.`, | ||
| }; | ||
| } | ||
|
|
||
| if (processInfo.status === "exited") { | ||
| return { | ||
| success: false, | ||
| message: `Process ${id} has already exited with code ${processInfo.exitCode}.`, | ||
| }; | ||
| } | ||
|
|
||
| if (processInfo.child) { | ||
| try { | ||
| processInfo.child.kill(); | ||
| return { | ||
| success: true, | ||
| message: `Process ${id} (PID: ${processInfo.pid}) terminated successfully.`, | ||
| }; | ||
| } catch (error) { | ||
| return { | ||
| success: false, | ||
| message: `Error killing process ${id}: ${error instanceof Error ? error.message : String(error)}`, | ||
| }; | ||
| } | ||
| } else { | ||
| return { | ||
| success: false, | ||
| message: `Process ${id} has no child process handle available.`, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| listProcesses(): ProcessInfo[] { | ||
| return Array.from(this.currentState.processes.values()).sort( | ||
| (a, b) => a.id - b.id, | ||
| ); | ||
| } | ||
|
|
||
| readOutput(id: number): { | ||
| stdout: string[]; | ||
| stderr: string[]; | ||
| currentStdoutLine: number; | ||
| currentStderrLine: number; | ||
| status: "running" | "exited"; | ||
| exitCode: number | null; | ||
| } | null { | ||
| const processInfo = this.currentState.processes.get(id); | ||
|
|
||
| if (!processInfo) { | ||
| return null; | ||
| } | ||
|
|
||
| // Get new lines since last read | ||
| const stdout = processInfo.stdoutBuffer.getLines( | ||
| processInfo.lastReadStdoutLine, | ||
| ); | ||
| const stderr = processInfo.stderrBuffer.getLines( | ||
| processInfo.lastReadStderrLine, | ||
| ); | ||
|
|
||
| // Update last read positions | ||
| const updatedProcessInfo = { | ||
| ...processInfo, | ||
| lastReadStdoutLine: processInfo.stdoutBuffer.getTotalLinesWritten(), | ||
| lastReadStderrLine: processInfo.stderrBuffer.getTotalLinesWritten(), | ||
| }; | ||
|
|
||
| const updatedProcesses = new Map(this.currentState.processes); | ||
| updatedProcesses.set(id, updatedProcessInfo); | ||
| this.setState({ | ||
| ...this.currentState, | ||
| processes: updatedProcesses, | ||
| }); | ||
|
|
||
| return { | ||
| stdout, | ||
| stderr, | ||
| currentStdoutLine: updatedProcessInfo.lastReadStdoutLine, | ||
| currentStderrLine: updatedProcessInfo.lastReadStderrLine, | ||
| status: processInfo.status, | ||
| exitCode: processInfo.exitCode, | ||
| }; | ||
| } | ||
|
|
||
| async cleanup(): Promise<void> { | ||
| logger.debug("Cleaning up background processes"); | ||
|
|
||
| const processes = Array.from(this.currentState.processes.values()).filter( | ||
| (p) => p.status === "running", | ||
| ); | ||
|
|
||
| for (const proc of processes) { | ||
| try { | ||
| if (proc.child) { | ||
| proc.child.kill(); | ||
| logger.debug(`Killed background process ${proc.id}`); | ||
| } | ||
| } catch (err) { | ||
| logger.debug(`Failed to kill process ${proc.id}:`, err); | ||
| } | ||
| } | ||
|
|
||
| await super.cleanup(); | ||
| } | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we just use https://www.npmjs.com/package/ringbufferjs ? |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| import RingBuffer from "ringbufferjs"; | ||
|
|
||
| /** | ||
| * CircularBuffer is a wrapper around ringbufferjs that provides | ||
| * line-based storage with line length limits and incremental reading. | ||
| */ | ||
| export class CircularBuffer { | ||
| private buffer: RingBuffer<string>; | ||
| private maxLineLength: number; | ||
| private totalLinesWritten: number = 0; | ||
|
|
||
| constructor(maxLines = 10000, maxLineLength = 2000) { | ||
| this.buffer = new RingBuffer(maxLines); | ||
| this.maxLineLength = maxLineLength; | ||
| } | ||
|
|
||
| append(line: string): void { | ||
| // Truncate line if too long | ||
| const truncatedLine = | ||
| line.length > this.maxLineLength | ||
| ? line.substring(0, this.maxLineLength) + "..." | ||
| : line; | ||
|
|
||
| this.buffer.enq(truncatedLine); | ||
| this.totalLinesWritten++; | ||
| } | ||
|
|
||
| getLines(fromLine?: number): string[] { | ||
| const from = fromLine ?? 0; | ||
| const bufferSize = this.buffer.size(); | ||
|
|
||
| // If buffer is empty, return empty array | ||
| if (bufferSize === 0) { | ||
| return []; | ||
| } | ||
|
|
||
| // Calculate the oldest line still in buffer | ||
| const oldestLineInBuffer = this.totalLinesWritten - bufferSize; | ||
|
|
||
| // If requesting lines before buffer start, clamp to start | ||
| const effectiveFrom = Math.max(from, oldestLineInBuffer); | ||
|
|
||
| // If requesting lines beyond what we've written, return empty | ||
| if (effectiveFrom >= this.totalLinesWritten) { | ||
| return []; | ||
| } | ||
|
|
||
| // Calculate how many lines to skip from the front and how many to return | ||
| const skipCount = effectiveFrom - oldestLineInBuffer; | ||
| const returnCount = this.totalLinesWritten - effectiveFrom; | ||
|
|
||
| // Get all lines from buffer and slice to get the desired range | ||
| const allLines = this.buffer.peekN(bufferSize); | ||
| return allLines.slice(skipCount, skipCount + returnCount); | ||
| } | ||
|
|
||
| getTotalLinesWritten(): number { | ||
| return this.totalLinesWritten; | ||
| } | ||
|
|
||
| clear(): void { | ||
| // Empty the buffer by dequeueing all elements | ||
| while (!this.buffer.isEmpty()) { | ||
| this.buffer.deq(); | ||
| } | ||
| this.totalLinesWritten = 0; | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: The error handler doesn't update process status to 'exited'. When a process fails to spawn (e.g., command not found), Node.js emits the 'error' event but may not emit the 'close' event. This leaves the process incorrectly marked as 'running' indefinitely, which could also cause issues with the
runningCountcheck instartProcesssince failed processes would still count againstmaxProcesses.Prompt for AI agents
✅ Addressed in
f2cf8fc