Skip to content

Commit 52fd91d

Browse files
CCR 2
Check the abort signal before running a volatile step producer. `step()` awaits a progress flush before dispatching, which is an await point, so `factory.abort` can land between entering the call and running the producer. The journaled path is covered because its next operation goes through `awaitFactoryOperation`, which checks the signal before dispatching; the volatile path returned `producer()` directly and so could start new extension work on an already-cancelled run. The regression test aborts mid-run and asserts the producer never runs. Removing the guard fails it.
1 parent ae624a3 commit 52fd91d

2 files changed

Lines changed: 49 additions & 0 deletions

File tree

nodejs/src/session.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1363,6 +1363,12 @@ export class CopilotSession {
13631363
): Promise<JsonValue> => {
13641364
await progress.flush();
13651365
if (options.volatile) {
1366+
// The flush above is an await point, so an abort can land
1367+
// between entering step() and running the producer. The
1368+
// journaled branch is covered by awaitFactoryOperation;
1369+
// this one has to check for itself, or a cancelled run
1370+
// would still start new extension work.
1371+
throwIfFactoryAborted(controller.signal);
13661372
return producer();
13671373
}
13681374
const cached = await awaitFactoryOperation(

nodejs/test/factory.test.ts

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -971,6 +971,49 @@ describe("factories", () => {
971971
expect(sendRequest).not.toHaveBeenCalled();
972972
});
973973

974+
it("does not start a volatile step producer after the run is aborted", async () => {
975+
const sendRequest = vi.fn();
976+
const session = new CopilotSession("session-volatile-abort", { sendRequest } as never);
977+
let producerRan = false;
978+
const factory = defineFactory({
979+
meta: {
980+
name: "volatile-abort",
981+
description: "Volatile steps honour cancellation",
982+
phases: [],
983+
},
984+
run: async ({ step, runId }) => {
985+
// Abort mid-run, then attempt a volatile step. The producer must
986+
// not run: cancellation has to stop new extension work starting,
987+
// exactly as it does on the journaled path.
988+
await session.clientSessionApis.factory!.abort({
989+
sessionId: session.sessionId,
990+
runId,
991+
});
992+
await step(
993+
"volatile",
994+
() => {
995+
producerRan = true;
996+
return "should not happen";
997+
},
998+
{ volatile: true }
999+
);
1000+
return "completed";
1001+
},
1002+
});
1003+
session.registerFactories([factory]);
1004+
1005+
await expect(
1006+
session.clientSessionApis.factory!.execute({
1007+
sessionId: session.sessionId,
1008+
name: "volatile-abort",
1009+
runId: "run-volatile-abort",
1010+
executionToken: "execution-token",
1011+
args: {},
1012+
})
1013+
).rejects.toThrow();
1014+
expect(producerRan).toBe(false);
1015+
});
1016+
9741017
it("rejects a factory result array with an extra own key", async () => {
9751018
const factory = defineFactory({
9761019
meta: {

0 commit comments

Comments
 (0)