From bc758c13a05cd131b1132b7da64f18596b26c1ef Mon Sep 17 00:00:00 2001 From: Ammar Date: Thu, 11 Dec 2025 09:12:22 -0600 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=96=20fix:=20serialize=20concurrent=20?= =?UTF-8?q?bash=5Foutput=20calls=20to=20prevent=20duplicate=20output?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes race condition where parallel bash_output tool calls on the same process could both read from the same offset before either updates the read position, resulting in duplicate output being returned. Root cause: When disableParallelToolUse is false (our default), Claude can issue multiple bash_output calls in a single response. Without serialization, both calls would: 1. Read proc.outputBytesRead = 0 2. Start async readOutput(0) 3. Both read same content 4. Both set proc.outputBytesRead to same final value Fix: Add outputLock (AsyncMutex) to BackgroundProcess to serialize getOutput() calls. Also consolidate all per-process state (outputBytesRead, outputLock) into the BackgroundProcess object so cleanup is automatic when the process is removed from the processes map - no separate maps to keep in sync. _Generated with mux_ --- .../services/backgroundProcessManager.test.ts | 44 ++++++++++++++++++ src/node/services/backgroundProcessManager.ts | 45 ++++++++++--------- 2 files changed, 68 insertions(+), 21 deletions(-) diff --git a/src/node/services/backgroundProcessManager.test.ts b/src/node/services/backgroundProcessManager.test.ts index e627740138..a1590a3a30 100644 --- a/src/node/services/backgroundProcessManager.test.ts +++ b/src/node/services/backgroundProcessManager.test.ts @@ -637,6 +637,50 @@ describe("BackgroundProcessManager", () => { expect(output.elapsed_ms).toBeGreaterThanOrEqual(250); expect(output.elapsed_ms).toBeLessThan(1000); // Didn't hang }); + + it("should serialize concurrent getOutput calls to prevent duplicate output", async () => { + // This test verifies the fix for the race condition where parallel bash_output + // calls could both read from the same offset before either updates the position. + // Without serialization, both calls would return the same output. + const result = await manager.spawn( + runtime, + testWorkspaceId, + "echo 'line1'; echo 'line2'; echo 'line3'", + { cwd: process.cwd(), displayName: "test" } + ); + + expect(result.success).toBe(true); + if (!result.success) return; + + // Wait for all output to be written + await new Promise((resolve) => setTimeout(resolve, 200)); + + // Call getOutput twice in parallel - without serialization, both would + // read from offset 0 and return duplicate "line1\nline2\nline3" + const [output1, output2] = await Promise.all([ + manager.getOutput(result.processId), + manager.getOutput(result.processId), + ]); + + expect(output1.success).toBe(true); + expect(output2.success).toBe(true); + if (!output1.success || !output2.success) return; + + // Combine outputs - should contain all lines exactly once + const combinedOutput = output1.output + output2.output; + const line1Count = (combinedOutput.match(/line1/g) ?? []).length; + const line2Count = (combinedOutput.match(/line2/g) ?? []).length; + const line3Count = (combinedOutput.match(/line3/g) ?? []).length; + + // Each line should appear exactly once across both outputs (no duplicates) + expect(line1Count).toBe(1); + expect(line2Count).toBe(1); + expect(line3Count).toBe(1); + + // One call should get the content, the other should get empty (already read) + const hasContent = output1.output.trim().length > 0 || output2.output.trim().length > 0; + expect(hasContent).toBe(true); + }); }); describe("integration: spawn and getOutput", () => { diff --git a/src/node/services/backgroundProcessManager.ts b/src/node/services/backgroundProcessManager.ts index bf798b7184..538476dea9 100644 --- a/src/node/services/backgroundProcessManager.ts +++ b/src/node/services/backgroundProcessManager.ts @@ -2,6 +2,7 @@ import type { Runtime, BackgroundHandle } from "@/node/runtime/Runtime"; import { spawnProcess } from "./backgroundProcessExecutor"; import { getErrorMessage } from "@/common/utils/errors"; import { log } from "./log"; +import { AsyncMutex } from "@/node/utils/concurrency/asyncMutex"; import { EventEmitter } from "events"; @@ -20,7 +21,9 @@ export interface BackgroundProcessMeta { } /** - * Represents a background process with file-based output + * Represents a background process with file-based output. + * All per-process state is consolidated here so cleanup is automatic when + * the process is removed from the processes map. */ export interface BackgroundProcess { id: string; // Process ID (display_name from the bash tool call) @@ -36,14 +39,11 @@ export interface BackgroundProcess { displayName?: string; // Human-readable name (e.g., "Dev Server") /** True if this process is being waited on (foreground mode) */ isForeground: boolean; -} - -/** - * Tracks read position for incremental output retrieval. - * Each call to getOutput() returns only new content since the last read. - */ -interface OutputReadPosition { - outputBytes: number; + /** Tracks read position for incremental output retrieval */ + outputBytesRead: number; + /** Mutex to serialize getOutput() calls (prevents race condition when + * parallel tool calls read from same offset before position is updated) */ + outputLock: AsyncMutex; } /** @@ -89,11 +89,10 @@ export class BackgroundProcessManager extends EventEmitter(); - // Tracks read positions for incremental output retrieval - private readPositions = new Map(); - // Base directory for process output files private readonly bgOutputDir: string; // Tracks foreground processes (started via runtime.exec) that can be backgrounded @@ -218,6 +217,8 @@ export class BackgroundProcessManager extends EventEmitter&1) - const result = await proc.handle.readOutput(pos.outputBytes); + const result = await proc.handle.readOutput(proc.outputBytesRead); accumulatedRaw += result.content; // Update read position - pos.outputBytes = result.newOffset; + proc.outputBytesRead = result.newOffset; // Refresh process status const refreshedProc = await this.getProcess(processId); @@ -699,6 +700,8 @@ export class BackgroundProcessManager extends EventEmitter this.terminate(p.id))); // Remove from memory (output dirs left on disk for OS/workspace cleanup) + // All per-process state (outputBytesRead, outputLock) is stored in the + // BackgroundProcess object, so cleanup is automatic when we delete here. for (const p of matching) { this.processes.delete(p.id); }