diff --git a/packages/coding-agent/.changes/execute-bash-abort-controller.md b/packages/coding-agent/.changes/execute-bash-abort-controller.md new file mode 100644 index 000000000..c997cafa5 --- /dev/null +++ b/packages/coding-agent/.changes/execute-bash-abort-controller.md @@ -0,0 +1 @@ +- Fixed concurrent `execute_bash_and_wait` commands sharing one bash abort controller: each `executeBash` invocation now gets its own controller, so a finishing command no longer clears a still-running command's abort state and `abortBash` cancels every in-flight command. diff --git a/packages/coding-agent/src/core/agent-session.ts b/packages/coding-agent/src/core/agent-session.ts index bc6a935e4..c0e036cf1 100644 --- a/packages/coding-agent/src/core/agent-session.ts +++ b/packages/coding-agent/src/core/agent-session.ts @@ -1114,7 +1114,7 @@ export class AgentSession { /** Outcome disclosures whose session-file append failed; retained for context rebuilds. */ private readonly _unpersistedOutcomes: CustomMessage[] = []; - private _bashAbortController: AbortController | undefined = undefined; + private _bashAbortControllers = new Set(); private _userBashRunning = false; private _userBashAbortRequested = false; private _pendingBashMessages: BashExecutionMessage[] = []; @@ -11086,7 +11086,9 @@ export class AgentSession { transient?: boolean; }, ): Promise { - this._bashAbortController = new AbortController(); + // Each invocation owns its controller so abortBash reaches every in-flight command. + const abortController = new AbortController(); + this._bashAbortControllers.add(abortController); const prefix = this.settingsManager.getShellCommandPrefix(); const shellPath = this.settingsManager.getShellPath(); @@ -11099,7 +11101,7 @@ export class AgentSession { options?.operations ?? createLocalBashOperations({ shellPath }), { onChunk, - signal: this._bashAbortController.signal, + signal: abortController.signal, }, ); @@ -11108,7 +11110,7 @@ export class AgentSession { } return result; } finally { - this._bashAbortController = undefined; + this._bashAbortControllers.delete(abortController); this._notifySessionInputCheckpointChange(); } } @@ -11279,14 +11281,17 @@ export class AgentSession { abortBash(): void { // A user bash command may not have spawned yet (extension dispatch in // progress); flag the request so runUserBash cancels before executing. - if (this._userBashRunning && this._bashAbortController === undefined) { + // runUserBash clears the flag at each start, so a stale flag is harmless. + if (this._userBashRunning) { this._userBashAbortRequested = true; } - this._bashAbortController?.abort(); + for (const controller of this._bashAbortControllers) { + controller.abort(); + } } get isBashRunning(): boolean { - return this._bashAbortController !== undefined || this._userBashRunning; + return this._bashAbortControllers.size > 0 || this._userBashRunning; } /** Whether there are pending bash messages waiting to be flushed */ diff --git a/packages/coding-agent/test/ipython-provisioner.test.ts b/packages/coding-agent/test/ipython-provisioner.test.ts index dd3a56afb..ed32aab28 100644 --- a/packages/coding-agent/test/ipython-provisioner.test.ts +++ b/packages/coding-agent/test/ipython-provisioner.test.ts @@ -73,7 +73,7 @@ function writeFakeReplRuntime(markerPath: string): string { const fs = require("node:fs"); const readline = require("node:readline"); const emit = (event) => process.stdout.write(JSON.stringify(event) + "\\n"); -emit({ event: "ready", protocol: 2, python: process.version }); +emit({ event: "ready", protocol: 3, python: process.version }); const input = readline.createInterface({ input: process.stdin }); input.on("line", (line) => { const request = JSON.parse(line); diff --git a/packages/coding-agent/test/repl-kernel-protocol-corruption.test.ts b/packages/coding-agent/test/repl-kernel-protocol-corruption.test.ts index 93bdfa062..3e2ee779f 100644 --- a/packages/coding-agent/test/repl-kernel-protocol-corruption.test.ts +++ b/packages/coding-agent/test/repl-kernel-protocol-corruption.test.ts @@ -32,10 +32,10 @@ if (fs.existsSync(process.env.FAKE_REPL_CORRUPT_BOOT)) { process.stdout.write("BROKEN-BOOT\\n"); } else if (fs.existsSync(process.env.FAKE_REPL_READY_WITH_GARBAGE)) { process.stdout.write( - JSON.stringify({ event: "ready", protocol: 2, python: process.version }) + "\\nBROKEN-WITH-READY\\n", + JSON.stringify({ event: "ready", protocol: 3, python: process.version }) + "\\nBROKEN-WITH-READY\\n", ); } else if (!(count > 1 && fs.existsSync(process.env.FAKE_REPL_DELAY_READY))) { - emit({ event: "ready", protocol: 2, python: process.version }); + emit({ event: "ready", protocol: 3, python: process.version }); } const input = readline.createInterface({ input: process.stdin }); input.on("line", (line) => { diff --git a/packages/coding-agent/test/suite/agent-session-bash-persistence.test.ts b/packages/coding-agent/test/suite/agent-session-bash-persistence.test.ts index ef14362de..ba061ef6d 100644 --- a/packages/coding-agent/test/suite/agent-session-bash-persistence.test.ts +++ b/packages/coding-agent/test/suite/agent-session-bash-persistence.test.ts @@ -519,4 +519,88 @@ describe("AgentSession bash and persistence characterization", () => { releaseExec?.(); await first; }); + + it("keeps an earlier bash abortable after a concurrent later bash finishes", async () => { + const harness = await createHarness(); + harnesses.push(harness); + const abortableOperations: BashOperations = { + exec: async (_command, _cwd, options) => + await new Promise<{ exitCode: number | null }>((_resolve, reject) => { + options.signal?.addEventListener("abort", () => reject(new Error("aborted")), { once: true }); + }), + }; + let releaseSecond: (() => void) | undefined; + const secondOperations: BashOperations = { + exec: async () => { + await new Promise((resolve) => { + releaseSecond = resolve; + }); + return { exitCode: 0 }; + }, + }; + + const first = harness.session.executeBash("first", undefined, { operations: abortableOperations }); + const second = harness.session.executeBash("second", undefined, { operations: secondOperations }); + await new Promise((resolve) => setTimeout(resolve, 0)); + + releaseSecond?.(); + const secondResult = await second; + expect(secondResult.cancelled).toBe(false); + + // The earlier command stays running and abortable on its own controller. + expect(harness.session.isBashRunning).toBe(true); + harness.session.abortBash(); + const firstResult = await first; + expect(firstResult.cancelled).toBe(true); + expect(harness.session.isBashRunning).toBe(false); + }); + + it("cancels a pending user bash when abortBash arrives while another bash is in flight", async () => { + const harness = await createHarness(); + harnesses.push(harness); + const operations: BashOperations = { + exec: async (_command, _cwd, options) => + await new Promise<{ exitCode: number | null }>((_resolve, reject) => { + options.signal?.addEventListener("abort", () => reject(new Error("aborted")), { once: true }); + }), + }; + + // The user bash claims its slot first, then a concurrent wait-style + // command (unguarded executeBash path) occupies the controller set while + // the user bash is still in extension dispatch. + const run = harness.session.runUserBash("echo should-not-run"); + const waitBash = harness.session.executeBash("wait", undefined, { operations }); + harness.session.abortBash(); + await run; + expect((await waitBash).cancelled).toBe(true); + + const userBashMessage = harness.session.messages + .slice() + .reverse() + .find((message) => message.role === "bashExecution" && message.command === "echo should-not-run"); + expect(userBashMessage?.role).toBe("bashExecution"); + if (userBashMessage?.role === "bashExecution") { + expect(userBashMessage.cancelled).toBe(true); + expect(userBashMessage.output).toBe(""); + } + }); + + it("aborts every in-flight bash command with abortBash", async () => { + const harness = await createHarness(); + harnesses.push(harness); + const makeAbortableOperations = (): BashOperations => ({ + exec: async (_command, _cwd, options) => + await new Promise<{ exitCode: number | null }>((_resolve, reject) => { + options.signal?.addEventListener("abort", () => reject(new Error("aborted")), { once: true }); + }), + }); + + const first = harness.session.executeBash("first", undefined, { operations: makeAbortableOperations() }); + const second = harness.session.executeBash("second", undefined, { operations: makeAbortableOperations() }); + await new Promise((resolve) => setTimeout(resolve, 0)); + + harness.session.abortBash(); + expect((await first).cancelled).toBe(true); + expect((await second).cancelled).toBe(true); + }); });