Skip to content

Commit e2f4487

Browse files
committed
Keep worker director questions from dying under the tool watchdog
A configured tools.timeoutMs would abort ask_director, cancel the pending ask, and turn a later send_input into a steer. The watchdog now skips that tool the same way it skips wait_agents. Register and interrupt also fail closed so a cancelled session cannot hang on a late question.
1 parent bcf39a6 commit e2f4487

6 files changed

Lines changed: 160 additions & 9 deletions

File tree

src/subagent/ask-director.test.ts

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,13 +108,17 @@ describe("evaluateAskDirector", () => {
108108
const state = createAskDirectorState();
109109
const controller = new AbortController();
110110
let cancelled = 0;
111+
let rejectAnswer: ((reason: unknown) => void) | undefined;
111112
const port = {
112113
register: (): Promise<string> => {
113114
controller.abort();
114-
return new Promise<string>(() => {});
115+
return new Promise<string>((_resolve, reject) => {
116+
rejectAnswer = reject;
117+
});
115118
},
116119
cancel: () => {
117120
cancelled += 1;
121+
rejectAnswer?.(new Error("ask_director aborted"));
118122
},
119123
};
120124
const message = await handleAskDirector({
@@ -129,6 +133,44 @@ describe("evaluateAskDirector", () => {
129133
expect(cancelled).toBeGreaterThan(0);
130134
});
131135

136+
test("abort after register does not leave an unhandled rejection", async () => {
137+
const state = createAskDirectorState();
138+
const controller = new AbortController();
139+
const unhandled: unknown[] = [];
140+
const onUnhandled = (reason: unknown) => {
141+
unhandled.push(reason);
142+
};
143+
process.on("unhandledRejection", onUnhandled);
144+
try {
145+
let rejectAnswer: ((reason: unknown) => void) | undefined;
146+
const port = {
147+
register: (): Promise<string> => {
148+
const answer = new Promise<string>((_resolve, reject) => {
149+
rejectAnswer = reject;
150+
});
151+
controller.abort();
152+
return answer;
153+
},
154+
cancel: () => {
155+
rejectAnswer?.(new Error("ask_director aborted"));
156+
},
157+
};
158+
const message = await handleAskDirector({
159+
question: "which file?",
160+
state,
161+
port,
162+
signal: controller.signal,
163+
});
164+
expect(message).toContain("cancelled");
165+
expect(state.questions).toBe(0);
166+
expect(state.pending).toBe(false);
167+
await new Promise((r) => setTimeout(r, 0));
168+
expect(unhandled).toEqual([]);
169+
} finally {
170+
process.off("unhandledRejection", onUnhandled);
171+
}
172+
});
173+
132174
test("register throw does not consume a cap slot", async () => {
133175
const state = createAskDirectorState();
134176
const port = {

src/subagent/ask-director.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,11 @@ export async function handleAskDirector(args: {
112112
});
113113
if (args.signal.aborted) {
114114
onAbort();
115+
try {
116+
await answerP;
117+
} catch {
118+
// cancelAsk rejects this; await so it is not an unhandledRejection.
119+
}
115120
return "Error: ask_director was cancelled.";
116121
}
117122
commitAskDirector(args.state);

src/subagent/session-store.test.ts

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1135,6 +1135,66 @@ describe("pending ask_director", () => {
11351135
expect(resolved).toBe("src/foo.ts");
11361136
});
11371137

1138+
test("interrupt then late registerAsk returns false and leaves no pending ask", () => {
1139+
const store = createSubAgentSessionStore();
1140+
const session = store.start({
1141+
description: "d",
1142+
agentId: "a",
1143+
brief: "b",
1144+
retained: true,
1145+
});
1146+
store.markRunning(session.id);
1147+
store.registerInterrupt(session.id, () => {});
1148+
expect(store.interruptOne(session.id).ok).toBe(true);
1149+
1150+
expect(
1151+
store.registerAsk(session.id, {
1152+
question: "late?",
1153+
questionId: "ask-1",
1154+
resolve: () => {
1155+
throw new Error("should not resolve");
1156+
},
1157+
reject: () => {
1158+
throw new Error("should not reject");
1159+
},
1160+
}),
1161+
).toBe(false);
1162+
expect(store.hasPendingAsk(session.id)).toBe(false);
1163+
});
1164+
1165+
test("interruptOne with missing handle does not cancel the pending ask", () => {
1166+
const store = createSubAgentSessionStore();
1167+
const session = store.start({
1168+
description: "d",
1169+
agentId: "a",
1170+
brief: "b",
1171+
retained: true,
1172+
});
1173+
store.markRunning(session.id);
1174+
let rejected: unknown;
1175+
let resolved: string | undefined;
1176+
store.registerAsk(session.id, {
1177+
question: "which file?",
1178+
questionId: "ask-1",
1179+
resolve: (answer) => {
1180+
resolved = answer;
1181+
},
1182+
reject: (reason) => {
1183+
rejected = reason;
1184+
},
1185+
});
1186+
1187+
expect(store.interruptOne(session.id)).toEqual({
1188+
ok: false,
1189+
status: "running",
1190+
});
1191+
expect(store.hasPendingAsk(session.id)).toBe(true);
1192+
expect(rejected).toBeUndefined();
1193+
expect(resolved).toBeUndefined();
1194+
expect(store.resolveAsk(session.id, "src/foo.ts")).toBe(true);
1195+
expect(resolved).toBe("src/foo.ts");
1196+
});
1197+
11381198
test("complete and close cancel a pending ask", async () => {
11391199
const store = createSubAgentSessionStore();
11401200
const completed = store.start({ description: "c", agentId: "a", brief: "b" });

src/subagent/session-store.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1242,7 +1242,9 @@ export function createSubAgentSessionStore(
12421242
reject: (reason: unknown) => void;
12431243
},
12441244
): boolean {
1245-
if (!sessions.has(id)) return false;
1245+
const session = sessions.get(id);
1246+
if (session === undefined) return false;
1247+
if (session.lifecycle.state !== "running") return false;
12461248
if (pendingAsks.has(id)) return false;
12471249
pendingAsks.set(id, ask);
12481250
mutate(id, () => {});
@@ -1278,11 +1280,11 @@ export function createSubAgentSessionStore(
12781280
if (!isLiveStrip(session.lifecycle)) {
12791281
return { ok: false, status: projectLifecycleStatus(session.lifecycle) };
12801282
}
1281-
settleCancelsAsks(id, "session interrupted");
12821283
const interrupt = interruptHandles.get(id);
12831284
if (interrupt === undefined) {
12841285
return { ok: false, status: projectLifecycleStatus(session.lifecycle) };
12851286
}
1287+
settleCancelsAsks(id, "session interrupted");
12861288
interrupt();
12871289
mutate(id, (s) => {
12881290
s.lifecycle = {

src/tui/tool-execution-watchdog.test.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,22 @@ describe("tool execution watchdog", () => {
6464
).toBeUndefined();
6565
});
6666

67+
test("ask_director with no settings timeout is unbounded", () => {
68+
expect(
69+
resolveToolExecutionTimeoutMs(undefined, { id: "1", name: "ask_director", arguments: {} }),
70+
).toBeUndefined();
71+
});
72+
73+
test("ask_director is exempt from the settings watchdog", () => {
74+
// Awaiting the director can outlast settings.tools.timeoutMs; aborting
75+
// would cancel the pending ask so later send_input steers instead of answering.
76+
const call = { id: "1", name: "ask_director", arguments: {} };
77+
expect(resolveToolExecutionTimeoutMs({ defaultMs: 660_000 }, call)).toBeUndefined();
78+
expect(
79+
resolveToolExecutionTimeoutMs({ defaultMs: 660_000, maxMs: 1_800_000 }, call),
80+
).toBeUndefined();
81+
});
82+
6783
test("wait_agents run outlasting the generic budget completes with its own report", async () => {
6884
const runner = createDynamicToolRunner(
6985
[
@@ -83,6 +99,24 @@ describe("tool execution watchdog", () => {
8399
expect(result.content).toContain("worker report");
84100
});
85101

102+
test("ask_director run outlasting the generic budget completes with its own answer", async () => {
103+
const runner = createDynamicToolRunner(
104+
[
105+
stringTool("ask_director", async () => {
106+
await new Promise((r) => setTimeout(r, 120));
107+
return "src/foo.ts";
108+
}),
109+
],
110+
{ defaultMs: 30 },
111+
);
112+
const result = await runner.run(
113+
{ id: "t", name: "ask_director", arguments: {} },
114+
new AbortController().signal,
115+
);
116+
expect(result.isError).toBeUndefined();
117+
expect(result.content).toContain("src/foo.ts");
118+
});
119+
86120
test("omitted config does not arm a default watchdog", () => {
87121
expect(resolveToolExecutionTimeoutMs(undefined)).toBeUndefined();
88122
expect(resolveToolExecutionTimeoutMs({})).toBeUndefined();

src/tui/tool-execution-watchdog.ts

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -70,11 +70,13 @@ export const MAX_TOOL_APPROVAL_PAUSE_MS = 1_800_000;
7070
* layer cannot beat shell-guard). A requested run_shell timeout is not clamped
7171
* to MAX_TOOL_EXECUTION_TIMEOUT_MS or tools.maxTimeoutMs.
7272
*
73-
* spawn_agent returns immediately; wait_agents is the long block. Both are
74-
* exempt: wait_agents can outlast settings.tools.timeoutMs while workers
75-
* still run, and aborting collect would not stop those workers. spawn_agent
76-
* stays exempt so the generic per-tool budget cannot abort a dispatch that
77-
* should return at once (or a worker that carries its own bound).
73+
* spawn_agent returns immediately; wait_agents and ask_director are the long
74+
* blocks. All three are exempt: wait_agents can outlast settings.tools.timeoutMs
75+
* while workers still run, and aborting collect would not stop those workers.
76+
* spawn_agent stays exempt so the generic per-tool budget cannot abort a
77+
* dispatch that should return at once (or a worker that carries its own bound).
78+
* ask_director is a long block awaiting the director; aborting it cancels the
79+
* pending ask so later send_input steers instead of answering.
7880
*
7981
* mcp__* tool calls are the opposite of exempt: they arm unconditionally (see
8082
* resolveMcpToolTimeoutMs) even when no Settings are configured, because an
@@ -85,7 +87,13 @@ export function resolveToolExecutionTimeoutMs(
8587
config?: ToolWatchdogConfig,
8688
call?: ToolCall,
8789
): number | undefined {
88-
if (call?.name === "spawn_agent" || call?.name === "wait_agents") return undefined;
90+
if (
91+
call?.name === "spawn_agent" ||
92+
call?.name === "wait_agents" ||
93+
call?.name === "ask_director"
94+
) {
95+
return undefined;
96+
}
8997
if (call?.name === "run_shell") {
9098
const requested = requestedRunShellTimeoutMs(call);
9199
if (requested !== undefined) {

0 commit comments

Comments
 (0)