mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-27 21:07:01 -06:00
fix(agents): reject cancelled foreground commands (#115694)
Co-authored-by: 赵旺0668001248 <zhao.wang1@xydigit.com>
This commit is contained in:
committed by
GitHub
parent
104322c5cc
commit
7c6eba5467
@@ -2,6 +2,7 @@
|
||||
* Exec tool policy, host dispatch, and process lifecycle pipeline.
|
||||
*/
|
||||
import { normalizeOptionalString } from "@openclaw/normalization-core/string-coerce";
|
||||
import { createAbortError } from "../infra/abort-signal.js";
|
||||
import {
|
||||
type ExecHost,
|
||||
loadExecApprovals,
|
||||
@@ -548,6 +549,7 @@ export function createExecTool(
|
||||
let yielded = false;
|
||||
let yieldTimer: NodeJS.Timeout | null = null;
|
||||
let registeredAbortSignal: AbortSignal | null = null;
|
||||
let toolAborted = false;
|
||||
|
||||
// Tool-call abort should not kill backgrounded sessions; timeouts still must.
|
||||
const onAbortSignal = () => {
|
||||
@@ -562,6 +564,13 @@ export function createExecTool(
|
||||
if (yielded || run.session.backgrounded) {
|
||||
return;
|
||||
}
|
||||
// Cancellation must win over foreground-to-background promotion while
|
||||
// the child settles; detached background sessions keep their owner.
|
||||
toolAborted = true;
|
||||
if (yieldTimer) {
|
||||
clearTimeout(yieldTimer);
|
||||
yieldTimer = null;
|
||||
}
|
||||
run.kill();
|
||||
};
|
||||
|
||||
@@ -584,6 +593,14 @@ export function createExecTool(
|
||||
}
|
||||
|
||||
return new Promise<AgentToolResult<ExecToolDetails>>((resolve, reject) => {
|
||||
const rejectIfAborted = () => {
|
||||
if (!toolAborted) {
|
||||
return false;
|
||||
}
|
||||
reject(createAbortError("Tool execution was aborted", { cause: signal?.reason }));
|
||||
return true;
|
||||
};
|
||||
|
||||
const resolveRunning = () => {
|
||||
cleanupToolRunListeners();
|
||||
resolve({
|
||||
@@ -607,7 +624,7 @@ export function createExecTool(
|
||||
};
|
||||
|
||||
const onYieldNow = () => {
|
||||
if (yielded) {
|
||||
if (yielded || toolAborted) {
|
||||
return;
|
||||
}
|
||||
if (settledOutcome) {
|
||||
@@ -634,7 +651,7 @@ export function createExecTool(
|
||||
resolveRunning();
|
||||
};
|
||||
|
||||
if (allowBackground && yieldWindow !== null) {
|
||||
if (!toolAborted && allowBackground && yieldWindow !== null) {
|
||||
if (yieldWindow === 0) {
|
||||
onYieldNow();
|
||||
} else {
|
||||
@@ -647,7 +664,7 @@ export function createExecTool(
|
||||
run.promise
|
||||
.then((outcome) => {
|
||||
cleanupToolRunListeners();
|
||||
if (yielded || run.session.backgrounded) {
|
||||
if (rejectIfAborted() || yielded || run.session.backgrounded) {
|
||||
return;
|
||||
}
|
||||
resolve(
|
||||
@@ -660,7 +677,7 @@ export function createExecTool(
|
||||
})
|
||||
.catch((err: unknown) => {
|
||||
cleanupToolRunListeners();
|
||||
if (yielded || run.session.backgrounded) {
|
||||
if (rejectIfAborted() || yielded || run.session.backgrounded) {
|
||||
return;
|
||||
}
|
||||
reject(err as Error);
|
||||
|
||||
@@ -7,6 +7,7 @@ const taskTracking = vi.hoisted(() => ({
|
||||
|
||||
vi.mock("./bash-tools.exec-task-tracking.js", () => taskTracking);
|
||||
|
||||
import { getFinishedSession } from "./bash-process-registry.js";
|
||||
import { createExecTool } from "./bash-tools.exec-run.js";
|
||||
|
||||
describe("exec background task wiring", () => {
|
||||
@@ -36,4 +37,78 @@ describe("exec background task wiring", () => {
|
||||
outcome: expect.objectContaining({ status: "completed" }),
|
||||
});
|
||||
});
|
||||
|
||||
it.each([
|
||||
{
|
||||
label: "foreground execution",
|
||||
defaults: { allowBackground: false },
|
||||
args: {},
|
||||
},
|
||||
{
|
||||
label: "foreground execution before the yield timer",
|
||||
defaults: { allowBackground: true, backgroundMs: 60_000 },
|
||||
args: { yieldMs: 60_000 },
|
||||
},
|
||||
])("finalizes and rejects an aborted real $label", async ({ defaults, args }) => {
|
||||
const abortController = new AbortController();
|
||||
const abortReason = new Error("operator cancelled the foreground command");
|
||||
const onUpdate = vi.fn(() => abortController.abort(abortReason));
|
||||
const tool = createExecTool({
|
||||
host: "gateway",
|
||||
security: "full",
|
||||
ask: "off",
|
||||
...defaults,
|
||||
});
|
||||
const command =
|
||||
`${JSON.stringify(process.execPath)} -e ` +
|
||||
`"process.stdout.write('ready\\n');setTimeout(() => {}, 30_000)"`;
|
||||
|
||||
await expect(
|
||||
tool.execute("abort-real-foreground", { command, ...args }, abortController.signal, onUpdate),
|
||||
).rejects.toMatchObject({
|
||||
name: "AbortError",
|
||||
message: "Tool execution was aborted",
|
||||
cause: abortReason,
|
||||
});
|
||||
|
||||
expect(onUpdate).toHaveBeenCalledTimes(1);
|
||||
expect(taskTracking.createBackgroundExecTask).not.toHaveBeenCalled();
|
||||
expect(taskTracking.finalizeBackgroundExecTask).toHaveBeenCalledWith({
|
||||
handle: null,
|
||||
outcome: expect.objectContaining({ status: "failed" }),
|
||||
});
|
||||
});
|
||||
|
||||
it("keeps a real background process running after its tool signal aborts", async () => {
|
||||
const abortController = new AbortController();
|
||||
const tool = createExecTool({
|
||||
host: "gateway",
|
||||
security: "full",
|
||||
ask: "off",
|
||||
allowBackground: true,
|
||||
backgroundMs: 0,
|
||||
});
|
||||
const command =
|
||||
`${JSON.stringify(process.execPath)} -e ` +
|
||||
`"setTimeout(() => process.stdout.write('background-survived\\n'), 30)"`;
|
||||
const result = await tool.execute(
|
||||
"abort-real-background",
|
||||
{ command, background: true },
|
||||
abortController.signal,
|
||||
);
|
||||
|
||||
expect(result.details.status).toBe("running");
|
||||
if (result.details.status !== "running") {
|
||||
throw new Error("expected a running background process");
|
||||
}
|
||||
const { sessionId } = result.details;
|
||||
abortController.abort();
|
||||
|
||||
await expect
|
||||
.poll(() => getFinishedSession(sessionId)?.status, {
|
||||
timeout: 5_000,
|
||||
interval: 10,
|
||||
})
|
||||
.toBe("completed");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1086,12 +1086,12 @@ describe("exec backgrounded onUpdate suppression", () => {
|
||||
const abortController = new AbortController();
|
||||
const onUpdateSpy = vi.fn(() => abortController.abort());
|
||||
// Run a command that produces output over time.
|
||||
const command = joinCommands([
|
||||
shellEcho("before-abort"),
|
||||
shortDelayCmd,
|
||||
shellEcho("after-abort"),
|
||||
]);
|
||||
await execTool.execute(nextCallId(), { command }, abortController.signal, onUpdateSpy);
|
||||
const beforeAbort = shellEcho("before-abort");
|
||||
const afterAbort = shellEcho("after-abort");
|
||||
const command = joinCommands([beforeAbort, shortDelayCmd, afterAbort]);
|
||||
await expect(
|
||||
execTool.execute(nextCallId(), { command }, abortController.signal, onUpdateSpy),
|
||||
).rejects.toMatchObject({ name: "AbortError" });
|
||||
expect(onUpdateSpy).toHaveBeenCalledTimes(1);
|
||||
// Allow a tick for any straggling stdout data events.
|
||||
await waitOneTurn();
|
||||
|
||||
Reference in New Issue
Block a user