fix(eval): keep completion aborts interruptible
This commit is contained in:
@@ -19,10 +19,12 @@ describe("withBridgeTimeoutPause", () => {
|
||||
await Bun.sleep(80);
|
||||
return "done";
|
||||
},
|
||||
{ deferExternalAbort: true },
|
||||
);
|
||||
|
||||
expect(value).toBe("done");
|
||||
expect(events.map(event => event.op)).toEqual([EVAL_TIMEOUT_PAUSE_OP, EVAL_TIMEOUT_RESUME_OP]);
|
||||
expect(events.every(event => event.deferExternalAbort === true)).toBe(true);
|
||||
|
||||
const settledCount = events.length;
|
||||
await Bun.sleep(40);
|
||||
@@ -75,27 +77,26 @@ class TestCancelledError extends Error {
|
||||
}
|
||||
}
|
||||
|
||||
it("defers external aborts until an in-flight bridge call resumes", async () => {
|
||||
it("defers external aborts until an in-flight agent bridge call resumes", async () => {
|
||||
const abortController = new AbortController();
|
||||
const entered = Promise.withResolvers<void>();
|
||||
const triggerAbort = Promise.withResolvers<void>();
|
||||
const observed = Promise.withResolvers<boolean>();
|
||||
const release = Promise.withResolvers<void>();
|
||||
let completed = false;
|
||||
let kernelSignal: AbortSignal | undefined;
|
||||
const kernel: GenericKernel<Record<string, string | null>> = {
|
||||
async execute(_code, options) {
|
||||
kernelSignal = options.signal;
|
||||
entered.resolve();
|
||||
await triggerAbort.promise;
|
||||
options.onDisplay({
|
||||
type: "status",
|
||||
event: { op: EVAL_TIMEOUT_PAUSE_OP },
|
||||
event: { op: EVAL_TIMEOUT_PAUSE_OP, deferExternalAbort: true },
|
||||
} satisfies KernelDisplayOutput);
|
||||
|
||||
abortController.abort(new Error("external interrupt"));
|
||||
expect(kernelSignal?.aborted).toBe(false);
|
||||
|
||||
observed.resolve(options.signal?.aborted ?? false);
|
||||
await release.promise;
|
||||
completed = true;
|
||||
options.onDisplay({
|
||||
type: "status",
|
||||
event: { op: EVAL_TIMEOUT_RESUME_OP },
|
||||
event: { op: EVAL_TIMEOUT_RESUME_OP, deferExternalAbort: true },
|
||||
} satisfies KernelDisplayOutput);
|
||||
return { status: "ok", cancelled: false, timedOut: false };
|
||||
},
|
||||
@@ -113,12 +114,57 @@ it("defers external aborts until an in-flight bridge call resumes", async () =>
|
||||
formatTimeoutAnnotation: () => "timed out",
|
||||
});
|
||||
|
||||
await Promise.resolve();
|
||||
expect(completed).toBe(false);
|
||||
|
||||
await entered.promise;
|
||||
triggerAbort.resolve();
|
||||
expect(await observed.promise).toBe(false);
|
||||
release.resolve();
|
||||
const result = await resultPromise;
|
||||
expect(result.cancelled).toBe(true);
|
||||
expect(result.exitCode).toBeUndefined();
|
||||
});
|
||||
|
||||
it("does not defer external aborts for a completion bridge call", async () => {
|
||||
const abortController = new AbortController();
|
||||
const entered = Promise.withResolvers<void>();
|
||||
const triggerAbort = Promise.withResolvers<void>();
|
||||
const observed = Promise.withResolvers<boolean>();
|
||||
const release = Promise.withResolvers<void>();
|
||||
const kernel: GenericKernel<Record<string, string | null>> = {
|
||||
async execute(_code, options) {
|
||||
entered.resolve();
|
||||
await triggerAbort.promise;
|
||||
options.onDisplay({
|
||||
type: "status",
|
||||
event: { op: EVAL_TIMEOUT_PAUSE_OP },
|
||||
} satisfies KernelDisplayOutput);
|
||||
abortController.abort(new Error("external interrupt"));
|
||||
observed.resolve(options.signal?.aborted ?? false);
|
||||
await release.promise;
|
||||
options.onDisplay({
|
||||
type: "status",
|
||||
event: { op: EVAL_TIMEOUT_RESUME_OP },
|
||||
} satisfies KernelDisplayOutput);
|
||||
return { status: "ok", cancelled: false, timedOut: false };
|
||||
},
|
||||
};
|
||||
|
||||
const resultPromise = executeWithKernelBase({
|
||||
kernel,
|
||||
code: "completion('slow')",
|
||||
options: { signal: abortController.signal },
|
||||
runIdPrefix: "test",
|
||||
errorLogLabel: "test",
|
||||
cancelledErrorClass: TestCancelledError,
|
||||
buildKernelEnvPatch: () => ({}),
|
||||
formatKernelTimeoutAnnotation: () => "kernel timed out",
|
||||
formatTimeoutAnnotation: () => "timed out",
|
||||
});
|
||||
|
||||
await entered.promise;
|
||||
triggerAbort.resolve();
|
||||
expect(await observed.promise).toBe(true);
|
||||
release.resolve();
|
||||
const result = await resultPromise;
|
||||
expect(completed).toBe(true);
|
||||
expect(result.cancelled).toBe(true);
|
||||
expect(result.exitCode).toBeUndefined();
|
||||
});
|
||||
|
||||
@@ -572,7 +572,7 @@ export async function runEvalAgent(args: unknown, options: EvalAgentBridgeOption
|
||||
options.session.recordEvalSubagentUsage?.(result.usage?.output ?? 0);
|
||||
|
||||
return { result, mergeSummary, changesApplied };
|
||||
});
|
||||
}, { deferExternalAbort: true });
|
||||
|
||||
return {
|
||||
text: structured ? result.output : result.output + mergeSummary,
|
||||
|
||||
@@ -26,6 +26,15 @@ export function isEvalTimeoutControlEvent(event: JsStatusEvent): boolean {
|
||||
return event.op === EVAL_TIMEOUT_PAUSE_OP || event.op === EVAL_TIMEOUT_RESUME_OP;
|
||||
}
|
||||
|
||||
/** Optional behavior for a timeout pause around a host bridge call. */
|
||||
export interface BridgeTimeoutPauseOptions {
|
||||
/**
|
||||
* Marks the pause as an `agent()` call whose already-started work must finish
|
||||
* before an external eval abort reaches the kernel.
|
||||
*/
|
||||
deferExternalAbort?: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
* Run {@link operation} while suspending the eval watchdog through
|
||||
* {@link emitStatus}. A no-op wrapper when no status sink is wired.
|
||||
@@ -33,12 +42,21 @@ export function isEvalTimeoutControlEvent(event: JsStatusEvent): boolean {
|
||||
export async function withBridgeTimeoutPause<T>(
|
||||
emitStatus: ((event: JsStatusEvent) => void) | undefined,
|
||||
operation: () => Promise<T>,
|
||||
options?: BridgeTimeoutPauseOptions,
|
||||
): Promise<T> {
|
||||
if (!emitStatus) return operation();
|
||||
emitStatus({ op: EVAL_TIMEOUT_PAUSE_OP });
|
||||
emitStatus(
|
||||
options?.deferExternalAbort
|
||||
? { op: EVAL_TIMEOUT_PAUSE_OP, deferExternalAbort: true }
|
||||
: { op: EVAL_TIMEOUT_PAUSE_OP },
|
||||
);
|
||||
try {
|
||||
return await operation();
|
||||
} finally {
|
||||
emitStatus({ op: EVAL_TIMEOUT_RESUME_OP });
|
||||
emitStatus(
|
||||
options?.deferExternalAbort
|
||||
? { op: EVAL_TIMEOUT_RESUME_OP, deferExternalAbort: true }
|
||||
: { op: EVAL_TIMEOUT_RESUME_OP },
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -198,6 +198,7 @@ function createBridgeAbortShield(source: AbortSignal | undefined): BridgeAbortSh
|
||||
|
||||
shield.signal = controller.signal;
|
||||
shield.handleStatus = (event: JsStatusEvent): void => {
|
||||
if (event.deferExternalAbort !== true) return;
|
||||
if (event.op === EVAL_TIMEOUT_PAUSE_OP) {
|
||||
pauseDepth++;
|
||||
return;
|
||||
|
||||
Reference in New Issue
Block a user