Skip to content

Commit e16cda1

Browse files
committed
refactor: replace 10ms delay with proper readline event synchronization
The previous approach used a 10ms delay to allow readline interfaces to process buffered data after process exit. This was a timing-based workaround that wasn't truly correct-by-construction. ## Problem When a process exits, there's a race between: 1. Process 'exit' event 2. Readline 'close' events (stdout and stderr) For commands like 'grep | head', the stdout stream closes early (when head exits), which triggers readline 'close' BEFORE the process 'exit' event. The 10ms delay hoped to bridge this gap, but it's not deterministic. ## Solution Wait for all three events concurrently using Promise.all(): - exitCode (process exit) - stdoutClosed (readline finished processing stdout) - stderrClosed (readline finished processing stderr) This ensures we never return results before readline has finished processing all buffered data, without relying on arbitrary timing delays. ## Testing - All 955 unit tests pass - All bash integration tests pass, including the grep|head regression test - Both local and SSH runtime tests pass - No timing-based workarounds --- _Generated with `cmux`_
1 parent d41cb2a commit e16cda1

File tree

1 file changed

+18
-7
lines changed

1 file changed

+18
-7
lines changed

src/services/tools/bash.ts

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,15 @@ export const createBashTool: ToolFactory = (config: ToolConfiguration) => {
124124
const stdoutReader = createInterface({ input: stdoutNodeStream });
125125
const stderrReader = createInterface({ input: stderrNodeStream });
126126

127+
// Set up promises to wait for readline interfaces to close
128+
// These must be created BEFORE the 'close' events fire
129+
const stdoutClosed = new Promise<void>((resolve) => {
130+
stdoutReader.on("close", () => resolve());
131+
});
132+
const stderrClosed = new Promise<void>((resolve) => {
133+
stderrReader.on("close", () => resolve());
134+
});
135+
127136
// Collect output
128137
const lines: string[] = [];
129138
let truncated = false;
@@ -237,10 +246,16 @@ export const createBashTool: ToolFactory = (config: ToolConfiguration) => {
237246
}
238247
});
239248

240-
// Wait for process to exit
249+
// Wait for BOTH process exit AND readline interfaces to finish processing
250+
// Key insight: When a process exits early (e.g., grep|head), the stdout stream closes
251+
// which triggers readline 'close' BEFORE the process 'exit' event. We must wait for both.
241252
let exitCode: number;
242253
try {
243-
exitCode = await execStream.exitCode;
254+
// Wait for all three events concurrently:
255+
// 1. Process exit (exitCode)
256+
// 2. Stdout readline finished processing buffered data
257+
// 3. Stderr readline finished processing buffered data
258+
[exitCode] = await Promise.all([execStream.exitCode, stdoutClosed, stderrClosed]);
244259
} catch (err: unknown) {
245260
// Cleanup immediately
246261
stdoutReader.close();
@@ -256,11 +271,7 @@ export const createBashTool: ToolFactory = (config: ToolConfiguration) => {
256271
};
257272
}
258273

259-
// Give readline interfaces a moment to process final buffered data
260-
// Process has exited but readline may still be processing buffered chunks
261-
await new Promise((resolve) => setTimeout(resolve, 10));
262-
263-
// Now cleanup
274+
// All events completed, now cleanup
264275
stdoutReader.close();
265276
stderrReader.close();
266277
stdoutNodeStream.destroy();

0 commit comments

Comments
 (0)