From ba09f1c9e0dfd3703dc7886e0f564725c2241617 Mon Sep 17 00:00:00 2001 From: Mario Zechner Date: Sat, 23 May 2026 10:30:00 +0200 Subject: [PATCH] Refine async tool control flow --- packages/coding-agent/src/core/tools/bash.ts | 121 +++++++++---------- packages/coding-agent/src/core/tools/edit.ts | 91 +++++++------- 2 files changed, 97 insertions(+), 115 deletions(-) diff --git a/packages/coding-agent/src/core/tools/bash.ts b/packages/coding-agent/src/core/tools/bash.ts index bc1d3cc4..efe3a5af 100644 --- a/packages/coding-agent/src/core/tools/bash.ts +++ b/packages/coding-agent/src/core/tools/bash.ts @@ -65,73 +65,62 @@ export interface BashOperations { */ export function createLocalBashOperations(options?: { shellPath?: string }): BashOperations { return { - exec: (command, cwd, { onData, signal, timeout, env }) => { - return new Promise((resolve, reject) => { - void (async () => { - const { shell, args } = getShellConfig(options?.shellPath); - try { - await fsAccess(cwd, constants.F_OK); - } catch { - reject(new Error(`Working directory does not exist: ${cwd}\nCannot execute bash commands.`)); - return; - } - if (signal?.aborted) { - reject(new Error("aborted")); - return; - } - const child = spawn(shell, [...args, command], { - cwd, - detached: process.platform !== "win32", - env: env ?? getShellEnv(), - stdio: ["ignore", "pipe", "pipe"], - windowsHide: true, - }); - if (child.pid) trackDetachedChildPid(child.pid); - let timedOut = false; - let timeoutHandle: NodeJS.Timeout | undefined; - // Set timeout if provided. - if (timeout !== undefined && timeout > 0) { - timeoutHandle = setTimeout(() => { - timedOut = true; - if (child.pid) killProcessTree(child.pid); - }, timeout * 1000); - } - // Stream stdout and stderr. - child.stdout?.on("data", onData); - child.stderr?.on("data", onData); - // Handle abort signal by killing the entire process tree. - const onAbort = () => { - if (child.pid) killProcessTree(child.pid); - }; - if (signal) { - if (signal.aborted) onAbort(); - else signal.addEventListener("abort", onAbort, { once: true }); - } - // Handle shell spawn errors and wait for the process to terminate without hanging - // on inherited stdio handles held by detached descendants. - waitForChildProcess(child) - .then((code) => { - if (child.pid) untrackDetachedChildPid(child.pid); - if (timeoutHandle) clearTimeout(timeoutHandle); - if (signal) signal.removeEventListener("abort", onAbort); - if (signal?.aborted) { - reject(new Error("aborted")); - return; - } - if (timedOut) { - reject(new Error(`timeout:${timeout}`)); - return; - } - resolve({ exitCode: code }); - }) - .catch((err) => { - if (child.pid) untrackDetachedChildPid(child.pid); - if (timeoutHandle) clearTimeout(timeoutHandle); - if (signal) signal.removeEventListener("abort", onAbort); - reject(err); - }); - })().catch((err: unknown) => reject(err instanceof Error ? err : new Error(String(err)))); + exec: async (command, cwd, { onData, signal, timeout, env }) => { + const { shell, args } = getShellConfig(options?.shellPath); + try { + await fsAccess(cwd, constants.F_OK); + } catch { + throw new Error(`Working directory does not exist: ${cwd}\nCannot execute bash commands.`); + } + if (signal?.aborted) { + throw new Error("aborted"); + } + + const child = spawn(shell, [...args, command], { + cwd, + detached: process.platform !== "win32", + env: env ?? getShellEnv(), + stdio: ["ignore", "pipe", "pipe"], + windowsHide: true, }); + if (child.pid) trackDetachedChildPid(child.pid); + let timedOut = false; + let timeoutHandle: NodeJS.Timeout | undefined; + const onAbort = () => { + if (child.pid) killProcessTree(child.pid); + }; + + try { + // Set timeout if provided. + if (timeout !== undefined && timeout > 0) { + timeoutHandle = setTimeout(() => { + timedOut = true; + if (child.pid) killProcessTree(child.pid); + }, timeout * 1000); + } + // Stream stdout and stderr. + child.stdout?.on("data", onData); + child.stderr?.on("data", onData); + // Handle abort signal by killing the entire process tree. + if (signal) { + if (signal.aborted) onAbort(); + else signal.addEventListener("abort", onAbort, { once: true }); + } + // Handle shell spawn errors and wait for the process to terminate without hanging + // on inherited stdio handles held by detached descendants. + const exitCode = await waitForChildProcess(child); + if (signal?.aborted) { + throw new Error("aborted"); + } + if (timedOut) { + throw new Error(`timeout:${timeout}`); + } + return { exitCode }; + } finally { + if (child.pid) untrackDetachedChildPid(child.pid); + if (timeoutHandle) clearTimeout(timeoutHandle); + if (signal) signal.removeEventListener("abort", onAbort); + } }, }; } diff --git a/packages/coding-agent/src/core/tools/edit.ts b/packages/coding-agent/src/core/tools/edit.ts index 7c6367a6..2812b661 100644 --- a/packages/coding-agent/src/core/tools/edit.ts +++ b/packages/coding-agent/src/core/tools/edit.ts @@ -314,61 +314,54 @@ export function createEditToolDefinition( const absolutePath = resolveToCwd(path, cwd); return withFileMutationQueue(absolutePath, async () => { - let aborted = signal?.aborted ?? false; - const onAbort = () => { - aborted = true; - }; + // Do not reject from an abort event listener here: that would release the + // mutation queue while an in-flight filesystem operation may still finish. + // Checking signal.aborted after each await observes the same aborts while + // keeping the queue locked until the current operation has settled. const throwIfAborted = (): void => { - if (aborted || signal?.aborted) { - throw new Error("Operation aborted"); - } + if (signal?.aborted) throw new Error("Operation aborted"); }; - signal?.addEventListener("abort", onAbort, { once: true }); + throwIfAborted(); + + // Check if file exists. try { + await ops.access(absolutePath); + } catch (error: unknown) { throwIfAborted(); - - // Check if file exists. - try { - await ops.access(absolutePath); - } catch (error: unknown) { - throwIfAborted(); - const errorMessage = - error instanceof Error && "code" in error ? `Error code: ${error.code}` : String(error); - throw new Error(`Could not edit file: ${path}. ${errorMessage}.`); - } - throwIfAborted(); - - // Read the file. - const buffer = await ops.readFile(absolutePath); - throwIfAborted(); - - // Strip BOM before matching. The model will not include an invisible BOM in oldText. - const rawContent = buffer.toString("utf-8"); - const { bom, text: content } = stripBom(rawContent); - const originalEnding = detectLineEnding(content); - const normalizedContent = normalizeToLF(content); - const { baseContent, newContent } = applyEditsToNormalizedContent(normalizedContent, edits, path); - throwIfAborted(); - - const finalContent = bom + restoreLineEndings(newContent, originalEnding); - await ops.writeFile(absolutePath, finalContent); - throwIfAborted(); - - const diffResult = generateDiffString(baseContent, newContent); - const patch = generateUnifiedPatch(path, baseContent, newContent); - return { - content: [ - { - type: "text", - text: `Successfully replaced ${edits.length} block(s) in ${path}.`, - }, - ], - details: { diff: diffResult.diff, patch, firstChangedLine: diffResult.firstChangedLine }, - }; - } finally { - signal?.removeEventListener("abort", onAbort); + const errorMessage = + error instanceof Error && "code" in error ? `Error code: ${error.code}` : String(error); + throw new Error(`Could not edit file: ${path}. ${errorMessage}.`); } + throwIfAborted(); + + // Read the file. + const buffer = await ops.readFile(absolutePath); + const rawContent = buffer.toString("utf-8"); + throwIfAborted(); + + // Strip BOM before matching. The model will not include an invisible BOM in oldText. + const { bom, text: content } = stripBom(rawContent); + const originalEnding = detectLineEnding(content); + const normalizedContent = normalizeToLF(content); + const { baseContent, newContent } = applyEditsToNormalizedContent(normalizedContent, edits, path); + throwIfAborted(); + + const finalContent = bom + restoreLineEndings(newContent, originalEnding); + await ops.writeFile(absolutePath, finalContent); + throwIfAborted(); + + const diffResult = generateDiffString(baseContent, newContent); + const patch = generateUnifiedPatch(path, baseContent, newContent); + return { + content: [ + { + type: "text", + text: `Successfully replaced ${edits.length} block(s) in ${path}.`, + }, + ], + details: { diff: diffResult.diff, patch, firstChangedLine: diffResult.firstChangedLine }, + }; }); }, renderCall(args, theme, context) {